Add tests for _get_attachment, and do not use attachment filename for temporary storage
#1553
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
groovecoder
previously requested changes
Feb 18, 2022
|
I found Sentry issues for omitted filenames as well https://sentry.prod.mozaws.net/operations/fx-private-relay-prod/issues/16863848 The line is: fx-private-relay/emails/views.py Line 768 in 04cfa86 and the exception is:
This change should also fix that, but we should have a test case for omitted filenames as well. Any, we may need to adjust downstream processing of attachments without filenames, but let's deal with that later. |
_get_attachment uses SpooledTemporaryFile, which keeps data in memory until it exceeds a minumum size. Test a short file, a long file, and a file with a URL for a name (currently fails).
Do not use the filename of the attachment when creating a SpooledTemporaryFile. This allows an attachment with a filename, like a URL, that contains characters like a colon (:) not allowed in some file systems.
|
I've rebased on main, refactored the common code to |
jwhitlock
dismissed
groovecoder’s stale review
Changes made, reviewer is unavailable for re-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #1551 by using the prefix
"relay_attachment_", and not basing the filename prefix or suffix on user data from the attachment filename. It also adds some tests for_get_attachment()._get_attachment()uses SpooledTemporaryFile, which holds an attachment in memory until it reaches 150 KiB, when it is stored in a temporary file. Previously, we used the attachment filename to make the prefix and suffix for the temporary file. However, this fails when the attachment filename has invalid characters, such as a colon (:). Now that we support larger files, we're starting to see some large attachments with a URL for the filename (https://example.com/image.jpg), and failures when writing to a tempfile with that name, such as:With this change, the temporary file will now have a randomly generated name, not related to the attachment name.
There is another potential bug for attachments with no filename, but I have not seen the signature in production yet, where calling
prefix=os.path.splitext(fn)[0]results in the error: