fix: enhance NAR URL normalization and fix potential path traversal

[kalbasit]

The previous implementation of Normalize() was vulnerable to path
traversal if the NAR hash contained ".." or other malicious sequences.
This change refactors Normalize() to use stricter regular expressions
for hash validation and returns an error if the hash is invalid.

Additionally, it refactors ensureNarFile in pkg/cache/cache.go to use a
new helper createOrUpdateNarFile, reducing code duplication.

Key changes:

• Introduced narHashLenientRegexp and narNormalizedHashRegexp for robust hash validation.
• Modified nar.URL.Normalize() to return (URL, error).
• Updated all callers in pkg/server, pkg/storage, and testdata to handle the new error return.
• Added filepath.Base() to temp file creation in pkg/cache/cache.go for enhanced security.

[kalbasit]

• fix: enhance NAR URL normalization and fix potential path traversal #856 👈 (View in Graphite)
• fix: normalize the URL when storing in the database #844
• fix: narinfo should return NAR URL normalized #843
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[dosubot]

Related Documentation

Checked 7 published document(s) in 1 knowledge base(s). No updates required.

^{How did I do? Any feedback?}  

[gemini-code-assist]

Summary of Changes

Hello @kalbasit, I'm Gemini Code Assist¹! 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 significantly enhances the security and robustness of NAR (Nix Archive) URL handling. It refactors the Normalize method for nar.URL to explicitly return an error upon detecting invalid or malicious hash patterns, such as path traversal attempts. Consequently, all instances where Normalize is used have been updated to check and propagate these errors. Additionally, temporary file creation now uses filepath.Base to extract only the filename from NAR hashes, further mitigating path traversal risks.

Highlights

• Security Fix: Path Traversal Prevention: The nar.URL.Normalize method now returns an error for invalid hashes that could lead to path traversal vulnerabilities (e.g., containing '..' or starting with '/').
• Error Handling for NAR URL Normalization: All call sites of nar.URL.Normalize across the codebase have been updated to correctly handle the newly introduced error, ensuring robust error propagation and preventing the use of potentially malicious URLs.
• Safe Temporary File Naming: Temporary NAR files are now created using filepath.Base on the hash, which extracts only the filename component, further mitigating path traversal risks during file creation.

Changelog

• pkg/cache/cache.go
  • Imported the path/filepath package.
  • Modified putNarWithCDC to use filepath.Base for temporary file naming.
  • Modified createTempNarFile to use filepath.Base for temporary file naming.
  • Updated storeInDatabase to handle the error returned by narURL.Normalize().
  • Updated storeNarInfoInDatabase to handle the error returned by narURL.Normalize().
• pkg/nar/url.go
  • Changed the Normalize method signature to return (URL, error).
  • Added error return for hashes containing '..' or starting with '/', preventing path traversal.
• pkg/nar/url_test.go
  • Modified TestNormalize to include an errStr field in test cases for expected errors.
  • Added a test case for an invalid hash c12lxpykv6sld7a0sakcnr3y0la70x8w-../../etc/passwd that now expects an error.
  • Updated test assertion logic to check for expected errors or successful normalization.
• pkg/server/server.go
  • Updated getNarInfo to handle the error returned by narURL.Normalize(), logging the error and returning an http.StatusInternalServerError.
• pkg/storage/local/local.go
  • Updated HasNar, GetNar, PutNar, and DeleteNar methods to handle the error returned by narURL.Normalize().
• pkg/storage/s3/s3.go
  • Updated narPath method to handle the error returned by narURL.Normalize().
• testdata/server.go
  • Updated handler to handle the error returned by nar.URL.Normalize() (ignoring it for test data purposes).

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a critical security fix for a path traversal vulnerability in nar.URL.Normalize. The Normalize function now correctly returns an error for invalid hashes containing path traversal elements, instead of returning the original invalid URL. This change is propagated throughout the codebase, with call sites updated to handle the new error return. Additionally, path traversal protection has been added when creating temporary NAR files by using filepath.Base to sanitize filenames derived from NAR hashes. The changes are well-implemented and significantly improve the security of the application. My review includes one suggestion to refine the HTTP status code for normalization errors to provide more accurate client feedback.

[kalbasit]

Draft as I need to do this after #842

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.96%. Comparing base (2bb8649) to head (3fd977b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #856   +/-   ##
=====================================
  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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kalbasit]

/gemini review

[gemini-code-assist]

Code Review

This pull request effectively addresses a path traversal vulnerability in nar.URL.Normalize() by replacing a fragile string-based implementation with a robust, regex-based validation approach. The change to return an error from Normalize() is a good design choice, and all call sites have been updated accordingly. The additional use of filepath.Base when creating temporary files provides another layer of security. My feedback includes suggestions to simplify the new regular expressions for better clarity and to improve error handling in the test server for increased robustness.

[kalbasit]

Backport failed for release-0.8, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-0.8
git worktree add -d .worktree/backport-856-to-release-0.8 origin/release-0.8
cd .worktree/backport-856-to-release-0.8
git switch --create backport-856-to-release-0.8
git cherry-pick -x e3119d4b6d548fe25f8f52c12ef3a5e79e9448a0