🐛 bug: fix sanitizePath validation logic#4064
Conversation
WalkthroughPath sanitization in the static middleware is strengthened with stricter validation rules, including early backslash rejection, normalization via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gaby, 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 focuses on enhancing the security and robustness of static file serving by implementing stricter path sanitization. The primary goal is to prevent directory traversal vulnerabilities, including those specific to Windows environments, by introducing comprehensive validation rules within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4064 +/- ##
==========================================
- Coverage 91.10% 91.05% -0.06%
==========================================
Files 119 119
Lines 11203 11229 +26
==========================================
+ Hits 10207 10225 +18
- Misses 630 634 +4
- Partials 366 370 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens static file path sanitization to better prevent traversal and Windows/UNC/drive-letter escape vectors, and expands static middleware tests/benchmarks to validate those behaviors across OS-backed and fs.FS filesystems.
Changes:
- Update
sanitizePathto normalize separators, reject backslashes/null bytes, and add Windows/UNC/volume-prefix blocking (whenfilesystem == nil) while preservingfs.ValidPathbehavior forfs.FS. - Expand
middleware/static/static_test.gowith many additional traversal and Windows/UNC cases, plus benchmark coverage. - Ensure the router request handler exits immediately after error handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| router.go | Adds an explicit return after error handling in requestHandler. |
| middleware/static/static.go | Strengthens path sanitization/validation to reduce traversal and Windows path escape vectors. |
| middleware/static/static_test.go | Extends traversal/path sanitization tests and benchmarks for more comprehensive coverage. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/static/static_test.go (1)
1086-1096:⚠️ Potential issue | 🟡 MinorBug:
bodyin theelsebranch refers to the valid-request body (line 1071), not the blocked-request response.In
assertTraversalBlocked, whenstatus != 404(i.e., status 400), the assertion on line 1093 checksbodyfrom the outer scope (the valid CSS response from line 1071) instead of readingresp.Body. Compare withTest_Static_PathTraversal(line 963) which correctly reads fromresp.Bodyinside the helper.🐛 Proposed fix
if status == 404 { respBody, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Contains(t, string(respBody), "Not Found", "Blocked traversal should have a \"Not Found\" message for %s", path) } else { - require.Contains(t, string(body), "Are you a hacker?", + respBody, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Contains(t, string(respBody), "Are you a hacker?", "Blocked traversal should have a \"Not Found\" message for %s", path) }
🧹 Nitpick comments (3)
middleware/static/static.go (2)
73-78:strings.Split+slices.Containsallocates on every request for a check that is unreachable afterpathpkg.Clean("/" + …).
pathpkg.Cleanwith a rooted prefix already eliminates all..segments (they resolve against/), so this block is pure defense-in-depth and will never trigger. If you want to keep it, consider a cheaper scan that avoids the allocation:♻️ Suggested zero-alloc alternative
- trimmed := utils.TrimLeft(s, '/') - if trimmed != "" { - if slices.Contains(strings.Split(trimmed, "/"), "..") { - return nil, ErrInvalidPath - } - } + trimmed := utils.TrimLeft(s, '/') + if trimmed != "" { + // Fast path: if ".." doesn't appear at all, skip the detailed check. + if strings.Contains(trimmed, "..") { + for part := range strings.SplitSeq(trimmed, "/") { + if part == ".." { + return nil, ErrInvalidPath + } + } + } + }
80-97:filepath.VolumeNameis a no-op on Linux — the manual drive-letter check on lines 88-93 is the real cross-platform guard.
filepath.VolumeName(line 85) returns""on non-Windows OSes regardless of input, so it will never trigger there. The manual check on lines 88-93 is the one that actually provides cross-platform protection for drive-letter paths. This is fine as-is, but worth a brief comment so future maintainers know the layering.Also,
normalizedClean(line 81) isfilepath.ToSlash(trimmed)wheretrimmedalready had backslashes rejected (line 57-59) and wasfilepath.ToSlash'd (line 66), making this secondToSlashredundant. Similarly, the"//"prefix check on line 82 is unreachable becausetrimmedhas leading slashes stripped, and the check on line 94 is unreachable becausepathpkg.Cleannormalizes multiple leading slashes.The redundancy isn't harmful — defense-in-depth is reasonable for security-critical code — but inline comments would clarify intent.
middleware/static/static_test.go (1)
1098-1134: A few test cases are duplicated within this function.Lines 1107/1117, 1111/1121 assert the same paths. Not harmful, but the duplicates could be removed for clarity.
There was a problem hiding this comment.
Code Review
This pull request hardens the sanitizePath function to prevent various path traversal vulnerabilities and adds extensive test cases to cover these scenarios. The changes are well-implemented and significantly improve security when serving static files. I've found a small opportunity to simplify the new logic in sanitizePath by removing some redundant checks, which should improve code clarity and maintainability. The fix in router.go is also correct.
Motivation
fs.FSinputs.Description
sanitizePathto normalize backslashes, reject residual backslashes and null bytes, detect and reject residual..segments after cleaning, and disallow UNC/double-slash and Windows volume/drive prefixes whenfilesystem == nil.filesystem != nilby usingfs.ValidPath, returning"/"for an empty filesystem path, and preserving trailing-slash semantics.middleware/static/static_test.gowith comprehensive cases for encoded/double-encoded traversals, Windows/UNC vectors,os.DirFSscenarios, and add/adjust benchmarks to exercisesanitizePathvariations.