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

fix: Fix file not found error with individual recipe export/download. #3579

Merged
merged 8 commits into from
May 20, 2024

Conversation

nephlm
Copy link
Contributor

@nephlm nephlm commented May 10, 2024

  • Fix file not found error with individual recipe export. ([BUG] - Cannot Download Recipe(s) #3095 )
  • Add a suitable test.
  • Split temporary_zip_path into temporary_zip_path and unlinking_temporary_zip_path.

What type of PR is this?

(REQUIRED)

  • bug

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 to unlinking_temporary_zip_file and updated references to point to the new name. This seemed like clearer naming given the changes.

Testing

  • Manually tested and ran the test suite.
  • Added test_get_recipe_as_zip that failed with the error described in [BUG] - Cannot Download Recipe(s) #3095 before the fix, and succeeded afterwards.
  • Verified the file was deleted after download.
  • Manually checked the endpoints potentially impacted by the dependency name change.

…itable test. Split temporary_zip_path into temporary_zip_path and unlinking_temporary_zip_path.
Copy link
Collaborator

@michael-genson michael-genson left a 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

mealie/routes/recipe/recipe_crud_routes.py Outdated Show resolved Hide resolved
mealie/routes/recipe/recipe_crud_routes.py Outdated Show resolved Hide resolved
mealie/core/dependencies/dependencies.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@michael-genson michael-genson left a 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)

mealie/core/dependencies/dependencies.py Outdated Show resolved Hide resolved
mealie/routes/recipe/recipe_crud_routes.py Outdated Show resolved Hide resolved
mealie/routes/recipe/recipe_crud_routes.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@michael-genson michael-genson left a 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!

@michael-genson michael-genson merged commit c70a5cb into mealie-recipes:mealie-next May 20, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] - Cannot Download Recipe(s)
2 participants