🐛 fix: Middlewares immutable config handling#3494
Conversation
|
""" WalkthroughThe changes introduce conditional logic in two middleware components to handle string conversion differently based on the application's immutability configuration. Specifically, string conversion methods are chosen depending on whether the Changes
Sequence Diagram(s)sequenceDiagram
participant AppConfig as App Config
participant Middleware as Middleware Function
participant Utils as Utils
AppConfig->>Middleware: Check Immutable flag
alt Immutable is true
Middleware->>Middleware: Use standard string/byte conversion
else Immutable is false
Middleware->>Utils: Use UnsafeString conversion
end
Middleware->>Middleware: Continue with authentication/proxy logic
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3494 +/- ##
==========================================
- Coverage 83.89% 83.83% -0.06%
==========================================
Files 120 120
Lines 12268 12272 +4
==========================================
- Hits 10292 10288 -4
- Misses 1553 1559 +6
- Partials 423 425 +2
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 updates middleware to respect the Immutable flag in the app config, avoiding unsafe string conversions when immutability is enabled and routing requests appropriately.
- Proxy middleware now uses
SetRequestURIByteswhenImmutableis true, andSetRequestURIotherwise. - Basic auth middleware switches between
string(raw)andutils.UnsafeString(raw)based on theImmutableflag.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/proxy/proxy.go | Added conditional branch to use byte-based URI setting |
| middleware/basicauth/basicauth.go | Refactored credential extraction to handle immutable config |
Comments suppressed due to low confidence (2)
middleware/proxy/proxy.go:80
- Introduce unit tests for both branches of the Immutable flag to verify that
SetRequestURIBytesandSetRequestURI(utils.UnsafeString)behave as expected.
if c.App().Config().Immutable {
middleware/basicauth/basicauth.go:50
- Add coverage in your basic-auth tests for when
Immutableis true and false to ensure bothstring(raw)andutils.UnsafeString(raw)paths are validated.
if c.App().Config().Immutable {
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/basicauth/basicauth.go(1 hunks)middleware/proxy/proxy.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
middleware/basicauth/basicauth.go
[warning] 51-51: middleware/basicauth/basicauth.go#L51
Added line #L51 was not covered by tests
middleware/proxy/proxy.go
[warning] 81-81: middleware/proxy/proxy.go#L81
Added line #L81 was not covered by tests
🪛 GitHub Check: CodeQL
middleware/proxy/proxy.go
[failure] 81-81: Uncontrolled data used in network request
The URL of this request depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: unit (1.24.x, windows-latest)
🔇 Additional comments (1)
middleware/proxy/proxy.go (1)
80-84:Details
❓ Verification inconclusive
LGTM! Consistent implementation of immutable config handling.
The conditional logic properly handles URI setting based on immutability configuration:
- Uses
SetRequestURIBytesto work directly with bytes when immutable mode is enabled- Uses
SetRequestURIwith unsafe string conversion for performance when immutable mode is disabledThis approach is consistent with the basicauth middleware changes and follows the same safety-vs-performance pattern.
Two concerns to address:
Test Coverage: Static analysis indicates this code path lacks test coverage. Please add tests for both immutable and non-immutable modes.
Security Analysis: CodeQL flagged "Uncontrolled data used in network request" for line 81. While this may be a false positive (since we're using the same request URI), please verify this doesn't introduce security risks:
🏁 Script executed:
#!/bin/bash # Verify the relationship between SetRequestURI and SetRequestURIBytes methods ast-grep --pattern $'func ($$$) SetRequestURI($$$) { $$$ }' # Check if there are any security validations around URI handling rg -A 5 -B 5 "SetRequestURI.*validation|sanitize.*URI|validate.*URI"Length of output: 133
To locate the actual implementations and verify any URI validation, let’s search for the definitions and usages of both methods:
🏁 Script executed:
#!/bin/bash # Find definitions of SetRequestURI and SetRequestURIBytes rg -n "func .*SetRequestURI" . rg -n "func .*SetRequestURIBytes" . # Inspect all call sites to see if any validation/sanitization is applied upstream rg -n "SetRequestURI(" . rg -n "SetRequestURIBytes(" .Length of output: 297
Add Tests and Manually Verify URI Setter Security
Before merging, please address two points:
Test Coverage
• Add unit tests forproxy.goto cover bothc.App().Config().Immutable = trueandfalsepaths, ensuringSetRequestURIBytesandSetRequestURI(utils.UnsafeString(...))behave as expected.Security Review of URI Setters
We couldn’t findSetRequestURIorSetRequestURIBytesdefinitions in this repo (they’re likely in the external fasthttp library). Please manually inspect those implementations to confirm that usingUnsafeString(and the byte‐based setter) does not bypass necessary URI validation or introduce injection risks.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 81-81: middleware/proxy/proxy.go#L81
Added line #L81 was not covered by tests🪛 GitHub Check: CodeQL
[failure] 81-81: Uncontrolled data used in network request
The URL of this request depends on a user-provided value.
|
Let me generate more tests with immutable True |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
middleware/proxy/proxy_test.go (1)
881-904: LGTM! Well-implemented test for immutable configuration.The test follows established patterns and properly verifies that the proxy middleware continues to function correctly when the Fiber app is configured with
Immutable: true. The test structure is solid with appropriate error handling and assertions.Consider enhancing the test to specifically verify immutable behavior differences (e.g., testing that string conversions are handled safely), though the current implementation serves well as a regression test.
middleware/basicauth/basicauth_test.go (1)
203-219: LGTM! Clean test implementation for immutable configuration.The test correctly verifies that BasicAuth middleware functions properly when the Fiber app is configured with
Immutable: true. The implementation follows existing test patterns with proper base64 encoding, error handling, and assertions.Consider adding a test case for invalid credentials under immutable mode to ensure comprehensive coverage, though the current positive test case effectively serves as a regression test for immutable functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/basicauth/basicauth_test.go(1 hunks)middleware/proxy/proxy_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: repeated
Summary