Skip to content

Commit

Permalink
Ensure we delete media if we reject due to spam check (element-hq#17246)
Browse files Browse the repository at this point in the history
Fixes up element-hq#17239

We need to keep the spam check within the `try/except` block. Also makes
it so that we don't enter the top span twice.

Also also ensures that we get the right thumbnail length.
  • Loading branch information
erikjohnston authored and Mic92 committed Jun 14, 2024
1 parent 5d0d3b8 commit f11ca38
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 32 deletions.
1 change: 1 addition & 0 deletions changelog.d/17246.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix errors in logs about closing incorrect logging contexts when media gets rejected by a module.
5 changes: 5 additions & 0 deletions synapse/media/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,11 @@ async def _generate_thumbnails(
finally:
t_byte_source.close()

# We flush and close the file to ensure that the bytes have
# been written before getting the size.
f.flush()
f.close()

t_len = os.path.getsize(fname)

# Write to database
Expand Down
59 changes: 27 additions & 32 deletions synapse/media/media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,42 +137,37 @@ async def store_into_file(
dirname = os.path.dirname(fname)
os.makedirs(dirname, exist_ok=True)

main_media_repo_write_trace_scope = start_active_span(
"writing to main media repo"
)
main_media_repo_write_trace_scope.__enter__()

with main_media_repo_write_trace_scope:
try:
try:
with start_active_span("writing to main media repo"):
with open(fname, "wb") as f:
yield f, fname

except Exception as e:
try:
os.remove(fname)
except Exception:
pass

raise e from None

with start_active_span("writing to other storage providers"):
spam_check = (
await self._spam_checker_module_callbacks.check_media_file_for_spam(
ReadableFileWrapper(self.clock, fname), file_info
with start_active_span("writing to other storage providers"):
spam_check = (
await self._spam_checker_module_callbacks.check_media_file_for_spam(
ReadableFileWrapper(self.clock, fname), file_info
)
)
)
if spam_check != self._spam_checker_module_callbacks.NOT_SPAM:
logger.info("Blocking media due to spam checker")
# Note that we'll delete the stored media, due to the
# try/except below. The media also won't be stored in
# the DB.
# We currently ignore any additional field returned by
# the spam-check API.
raise SpamMediaException(errcode=spam_check[0])

for provider in self.storage_providers:
with start_active_span(str(provider)):
await provider.store_file(path, file_info)
if spam_check != self._spam_checker_module_callbacks.NOT_SPAM:
logger.info("Blocking media due to spam checker")
# Note that we'll delete the stored media, due to the
# try/except below. The media also won't be stored in
# the DB.
# We currently ignore any additional field returned by
# the spam-check API.
raise SpamMediaException(errcode=spam_check[0])

for provider in self.storage_providers:
with start_active_span(str(provider)):
await provider.store_file(path, file_info)

except Exception as e:
try:
os.remove(fname)
except Exception:
pass

raise e from None

async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]:
"""Attempts to fetch media described by file_info from the local cache
Expand Down

0 comments on commit f11ca38

Please sign in to comment.