fix: normalize the URL when storing in the database#844
Conversation
Summary of ChangesHello @kalbasit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances data consistency within the cache database by introducing a URL normalization step for NAR (Nix Archive) URLs. By ensuring that NAR URLs are consistently formatted before being used in database operations, the system can accurately identify and manage NAR files, preventing the creation of redundant Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where different NAR URL formats could lead to duplicate nar_file records in the database by normalizing the NAR URL in storeInDatabase and storeNarInfoInDatabase. However, a critical security vulnerability has been identified: the normalization logic in pkg/nar/url.go is insecure as it fails open when path traversal attempts are detected. This, combined with the use of unsanitized hashes in temporary file creation elsewhere in pkg/cache/cache.go, poses a significant security risk. Additionally, the same normalization is needed in the PutNar function (and its related CDC code paths) to fully resolve the duplication issue for directly uploaded NARs. Please ensure that the Normalize function fails securely and that all file system operations use properly sanitized inputs, and consider extending the normalization to PutNar for a comprehensive fix.
d8933c2 to
deedcdf
Compare
deedcdf to
0fcd7a4
Compare
By normalizing the NAR URL before storing it or checking for its existence in the database, we ensure that the hash used in the 'nar_files' table matches the actual hash of the file in the storage layer. This prevents duplicate 'nar_file' records when the same NAR is referenced by different 'narinfo' files with varying URL formats (e.g., with or without a hash prefix). Changes: - Call '.Normalize()' on the parsed NAR URL in 'storeInDatabase'. - Call '.Normalize()' on the parsed NAR URL in 'storeNarInfoInDatabase'.
0fcd7a4 to
58513d4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #844 +/- ##
=====================================
Coverage 3.96% 3.96%
=====================================
Files 6 6
Lines 429 429
=====================================
Hits 17 17
Misses 409 409
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.8
git worktree add -d .worktree/backport-844-to-release-0.8 origin/release-0.8
cd .worktree/backport-844-to-release-0.8
git switch --create backport-844-to-release-0.8
git cherry-pick -x a6df95184767eaa7dbfc3489d9cf804eabcc69a3 |
1 similar comment
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.8
git worktree add -d .worktree/backport-844-to-release-0.8 origin/release-0.8
cd .worktree/backport-844-to-release-0.8
git switch --create backport-844-to-release-0.8
git cherry-pick -x a6df95184767eaa7dbfc3489d9cf804eabcc69a3 |
By normalizing the NAR URL before storing it or checking for its existence in the database, we ensure that the hash used in the 'nar_files' table matches the actual hash of the file in the storage layer. This prevents duplicate 'nar_file' records when the same NAR is referenced by different 'narinfo' files with varying URL formats (e.g., with or without a hash prefix). Changes: - Call '.Normalize()' on the parsed NAR URL in 'storeInDatabase'. - Call '.Normalize()' on the parsed NAR URL in 'storeNarInfoInDatabase'. (cherry picked from commit a6df951)
…857) By normalizing the NAR URL before storing it or checking for its existence in the database, we ensure that the hash used in the 'nar_files' table matches the actual hash of the file in the storage layer. This prevents duplicate 'nar_file' records when the same NAR is referenced by different 'narinfo' files with varying URL formats (e.g., with or without a hash prefix). Changes: - Call '.Normalize()' on the parsed NAR URL in 'storeInDatabase'. - Call '.Normalize()' on the parsed NAR URL in 'storeNarInfoInDatabase'. (cherry picked from commit a6df951)

By normalizing the NAR URL before storing it or checking for its
existence in the database, we ensure that the hash used in the
'nar_files' table matches the actual hash of the file in the storage
layer. This prevents duplicate 'nar_file' records when the same NAR is
referenced by different 'narinfo' files with varying URL formats (e.g.,
with or without a hash prefix).
Changes: