Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure we delete media if we reject due to spam check #17246

Merged
merged 3 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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()
Comment on lines +1052 to +1055
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine I guess. Think you could have also used ftell, however that's mapped into Python, but going to the filesystem should be OK..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I mean we could likely even get that from the thumbnail source, but didn't want to make big changes.


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
Loading