-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
fix: Fix file not found error with individual recipe export/download. #3579
fix: Fix file not found error with individual recipe export/download. #3579
Conversation
…itable test. Split temporary_zip_path into temporary_zip_path and unlinking_temporary_zip_path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should resolve the security issue. The code QL comment is placed at the end of the method because technically that's where the "exploit" is, but the source of the issue is in the method definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments that I think will make the code more readable using a context manager. This is what FastAPI was supposed to be doing with the dependency injection, but it's broken (hence the bug)
…clean up the temporary assets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for sticking with it!
temporary_zip_path
intotemporary_zip_path
andunlinking_temporary_zip_path
.What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
(REQUIRED)
This PR fixes the issue that was preventing the recipe download and the
/api/recipes/{slug}/exports/zip
endpoint more generally from working. As described in #3095, it would fail with a file not found. This was caused by the file being deleted before the Response read the file from the filesystem.Which issue(s) this PR fixes:
(REQUIRED)
Fixes #3095
Special notes for your reviewer:
(fill-in or delete this section)
I created a version of
temporary_zip_path
that doesn't unlink when the view completes and added background task to the Response to clean up the file that is run after the Response is delivered to the client.Renamed the original
temporary_zip_path
tounlinking_temporary_zip_file
and updated references to point to the new name. This seemed like clearer naming given the changes.Testing
test_get_recipe_as_zip
that failed with the error described in [BUG] - Cannot Download Recipe(s) #3095 before the fix, and succeeded afterwards.