Improve static path security#3600
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a new test for client proxy URL handling, centralizes and strengthens path sanitization logic in the static file middleware, and adds comprehensive path traversal security tests with platform-specific coverage for both Windows and non-Windows environments. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test_Client_SetProxyURL
participant ProxyServer
participant HelperServer
participant Client
Test->>ProxyServer: Start proxy server (Fiber)
Test->>HelperServer: Start helper server (Fiber)
Test->>Client: Set proxy URL
Client->>ProxyServer: Make request (should add isProxy header)
ProxyServer->>HelperServer: Forward request
HelperServer-->>ProxyServer: Respond
ProxyServer-->>Client: Respond with isProxy header
Test->>Client: Set invalid proxy URL
Client->>ProxyServer: Make request (should fail)
sequenceDiagram
participant StaticMiddleware
participant sanitizePath
participant fs
StaticMiddleware->>sanitizePath: Receive requested path
sanitizePath->>fs: (optional) Validate path with fs.ValidPath
sanitizePath-->>StaticMiddleware: Return sanitized path or error
StaticMiddleware-->>Client: Serve file or return 404/400
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @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 bolstering the security of static file serving by introducing a comprehensive path sanitization mechanism. It aims to prevent various forms of path manipulation attacks, ensuring that only intended files within the designated root directory can be accessed. The changes are thoroughly validated with an extensive set of new and improved tests.
Highlights
- Security Enhancement: Implemented a new
sanitizePathfunction within the static file serving middleware to rigorously validate and clean incoming request paths. This prevents directory traversal attacks, null byte injections, and other malicious path manipulations by rejecting invalid patterns and normalizing paths. - Expanded Test Coverage: Significantly expanded the test suite for static file serving with new
Test_Static_PathTraversalandTest_Static_PathTraversal_WindowsOnlyfunctions. These tests cover a wide array of malicious path patterns, including various encodings, null bytes, and OS-specific traversal attempts, ensuring the new sanitization logic is robust. - Test Suite Improvements: Refactored existing tests to use constants for common string literals (e.g.,
windows,testdata/fs/css) for improved readability and maintainability. Additionally, environment variable handling in proxy tests was enhanced to ensure better test isolation and prevent side effects.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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 is currently in preview and 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 to provide feedback.
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
-
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. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR improves static path security by implementing stricter sanitization for path traversal attacks, expanding test coverage for various malicious patterns, and making minor code quality improvements.
- Adds a new
sanitizePathfunction that validates and cleans requested paths to prevent directory traversal attacks - Expands test coverage with comprehensive path traversal tests for both Unix and Windows environments
- Refactors tests to use constants and fixes environment variable handling errors
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| middleware/static/static.go | Adds path sanitization function and integrates it into the path rewrite logic |
| middleware/static/static_test.go | Introduces constants for test values and adds comprehensive path traversal security tests |
| client/client_test.go | Fixes environment variable handling and corrects a typo in comments |
| } | ||
|
|
||
| // reject any null bytes or traversal attempts | ||
| if strings.Contains(s, "\x00") || strings.Contains(s, "..") || strings.Contains(s, ":") { |
There was a problem hiding this comment.
The colon character (:) check may be too broad and could block legitimate file names containing colons. Consider making this Windows-specific for drive letter detection or removing it if not necessary for security.
| if strings.Contains(s, "\x00") || strings.Contains(s, "..") || strings.Contains(s, ":") { | |
| if strings.Contains(s, "\x00") || strings.Contains(s, "..") || (runtime.GOOS == "windows" && strings.Contains(s, ":")) { |
| s = "/" + s | ||
| } else { | ||
| // verify no traversal after cleaning | ||
| if strings.Contains(raw, "..") || strings.Contains(s, "..") { |
There was a problem hiding this comment.
This check for '..' is redundant since it was already performed at line 42. The second check after path cleaning may be sufficient on its own.
| if strings.Contains(raw, "..") || strings.Contains(s, "..") { | |
| if strings.Contains(s, "..") { |
| require.NoError(t, err) | ||
| require.Contains(t, string(body), "Cannot GET", | ||
| "Blocked traversal should have a 'Cannot GET' message for %s", path) | ||
| } else { |
There was a problem hiding this comment.
The variable 'body' is used here but is not defined in this scope. It should be read from the response body similar to the 404 case above.
| } else { | |
| } else { | |
| body, err := io.ReadAll(resp.Body) | |
| require.NoError(t, err) |
| require.Contains(t, string(body), "Cannot GET", | ||
| "Blocked traversal should have a Cannot GET message for %s", path) | ||
| } else { | ||
| require.Contains(t, string(body), "Are you a hacker?", |
There was a problem hiding this comment.
The error message 'Are you a hacker?' does not match the actual error messages that Fiber would return. This should likely be 'Cannot GET' or another appropriate error message that matches the actual implementation.
| require.Contains(t, string(body), "Are you a hacker?", | |
| require.Contains(t, string(body), "Cannot GET", |
| require.NoError(t, os.Setenv("HTTP_PROXY", httpProxy)) | ||
| require.NoError(t, os.Setenv("http_proxy", httpProxyLower)) | ||
| require.NoError(t, os.Setenv("HTTPS_PROXY", httpsProxy)) | ||
| require.NoError(t, os.Setenv("https_proxy", httpsProxyLower)) | ||
| require.NoError(t, os.Setenv("NO_PROXY", noProxy)) | ||
| require.NoError(t, os.Setenv("no_proxy", noProxyLower)) |
There was a problem hiding this comment.
Setting environment variables to empty string values when they were originally unset. Should check if the original value was empty and use os.Unsetenv() instead of os.Setenv() with empty string.
| require.NoError(t, os.Setenv("HTTP_PROXY", httpProxy)) | |
| require.NoError(t, os.Setenv("http_proxy", httpProxyLower)) | |
| require.NoError(t, os.Setenv("HTTPS_PROXY", httpsProxy)) | |
| require.NoError(t, os.Setenv("https_proxy", httpsProxyLower)) | |
| require.NoError(t, os.Setenv("NO_PROXY", noProxy)) | |
| require.NoError(t, os.Setenv("no_proxy", noProxyLower)) | |
| if httpProxy == "" { | |
| require.NoError(t, os.Unsetenv("HTTP_PROXY")) | |
| } else { | |
| require.NoError(t, os.Setenv("HTTP_PROXY", httpProxy)) | |
| } | |
| if httpProxyLower == "" { | |
| require.NoError(t, os.Unsetenv("http_proxy")) | |
| } else { | |
| require.NoError(t, os.Setenv("http_proxy", httpProxyLower)) | |
| } | |
| if httpsProxy == "" { | |
| require.NoError(t, os.Unsetenv("HTTPS_PROXY")) | |
| } else { | |
| require.NoError(t, os.Setenv("HTTPS_PROXY", httpsProxy)) | |
| } | |
| if httpsProxyLower == "" { | |
| require.NoError(t, os.Unsetenv("https_proxy")) | |
| } else { | |
| require.NoError(t, os.Setenv("https_proxy", httpsProxyLower)) | |
| } | |
| if noProxy == "" { | |
| require.NoError(t, os.Unsetenv("NO_PROXY")) | |
| } else { | |
| require.NoError(t, os.Setenv("NO_PROXY", noProxy)) | |
| } | |
| if noProxyLower == "" { | |
| require.NoError(t, os.Unsetenv("no_proxy")) | |
| } else { | |
| require.NoError(t, os.Setenv("no_proxy", noProxyLower)) | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2025-07-18-13-58-39 #3600 +/- ##
=======================================================
+ Coverage 90.86% 90.89% +0.02%
=======================================================
Files 111 111
Lines 11252 11252
=======================================================
+ Hits 10224 10227 +3
+ Misses 770 768 -2
+ Partials 258 257 -1
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.
Code Review
This pull request improves static path security with stricter sanitization and expanded tests. However, the sanitization logic is overly strict and may block legitimate paths. I've suggested changes to rely on standard library functions for path cleaning. Additionally, I've identified a typo in an environment variable and an incorrect use of t.Parallel() in the tests.
| require.NoError(t, os.Setenv("no_proxy", noProxyLower)) | ||
| }) | ||
|
|
||
| t.Parallel() |
There was a problem hiding this comment.
This test modifies global state (environment variables), so it should not be run in parallel with other tests at the top level. Calling t.Parallel() here can lead to race conditions and flaky tests if other tests depend on the same environment variables. Please remove this line. The sub-tests within this function can still run in parallel with each other. This is a high severity issue because it can lead to unpredictable test results and make it difficult to identify the root cause of failures.
| if strings.Contains(s, "\x00") || strings.Contains(s, "..") || strings.Contains(s, ":") { | ||
| return nil, errors.New("invalid path") | ||
| } |
There was a problem hiding this comment.
The check strings.Contains(s, "..") is too restrictive and will reject valid relative paths that normalize to a path within the static root, for example, /css/../js/script.js. This can break legitimate use cases. The path traversal security should primarily rely on path.Clean and fs.ValidPath, which correctly handle canonicalization. I recommend removing the .. check from this line. The added tests are also missing a case for this valid scenario. This is a high severity issue because it can lead to unexpected behavior and prevent users from accessing valid resources.
| if strings.Contains(s, "\x00") || strings.Contains(s, "..") || strings.Contains(s, ":") { | |
| return nil, errors.New("invalid path") | |
| } | |
| if strings.Contains(s, "\x00") || strings.Contains(s, ":") { | |
| return nil, errors.New("invalid path") | |
| } |
| if strings.Contains(raw, "..") || strings.Contains(s, "..") { | ||
| return nil, errors.New("invalid path") | ||
| } |
There was a problem hiding this comment.
This check is also overly restrictive. The check on raw (the un-cleaned path) will reject valid relative paths. The check on s (the cleaned path) is redundant because path.Clean on an absolute path will resolve all .. segments. The security is already handled by fasthttp.FS which ensures the final path is within the root directory. This block should be removed to avoid breaking valid use cases. This is a high severity issue because it can lead to unexpected behavior and prevent users from accessing valid resources.
Summary