fix(adaptor): prevent CopyContextToFiberContext panic and add comprehensive test coverage #3770
Conversation
…rehensive test coverage - Fix panic when CopyContextToFiberContext receives non-pointer context structs - Add proper nil pointer checking and type validation - Add comprehensive test suite with 297+ lines covering all error paths - Include unit tests for resolveRemoteAddr, isUnixNetwork helper functions - Add Unix socket integration tests and error handling scenarios - Improve test maintainability with DRY helper functions - Add benchmarks for performance validation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCopyContextToFiberContext now safely handles pointers/nil and only processes structs; middleware enforces a 10MB request-body cap and tightens remote-address validation and error paths. Tests expanded to cover conversions, context copying, error flows, body limits, Unix sockets, and address resolution. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant NetHTTP as net/http Handler
participant Adaptor as Middleware/Adaptor
participant Resolver as resolveRemoteAddr
participant Fiber as fasthttp Handler
Client->>NetHTTP: send request (body)
NetHTTP->>Adaptor: ConvertRequest
Adaptor->>Adaptor: enforce 10MB body limit (return 413 if exceed)
Adaptor->>Resolver: resolveRemoteAddr(remoteAddr)
alt resolved
Resolver-->>Adaptor: host:port
else error (empty/long/invalid)
Resolver-->>Adaptor: error
end
Adaptor->>Adaptor: CopyContextToFiberContext(src)
note right of Adaptor #D3F6D9: CopyContextToFiberContext:\n- deref pointers\n- nil & kind checks\n- safe field handling
Adaptor->>Fiber: forward converted request
Fiber-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 @sixcolors, 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 addresses a critical panic in the Highlights
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
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a panic in CopyContextToFiberContext when handling non-pointer context structs and adds comprehensive test coverage for the adaptor middleware. The main issue was that the original code assumed contexts were always pointers, causing panics with non-pointer context types.
Key changes:
- Fixed context type handling to support both pointer and non-pointer structs with nil checking
- Added 297+ lines of comprehensive test coverage including error paths, edge cases, and Unix socket testing
- Introduced helper functions for cleaner and more maintainable tests
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/adaptor/adaptor.go | Fixed panic in CopyContextToFiberContext by adding proper pointer/non-pointer handling and nil checking |
| middleware/adaptor/adaptor_test.go | Added comprehensive test suite with helper functions, error handling tests, Unix socket tests, and benchmarks |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3770 +/- ##
==========================================
+ Coverage 91.54% 91.64% +0.10%
==========================================
Files 113 113
Lines 11912 11937 +25
==========================================
+ Hits 10905 10940 +35
+ Misses 740 733 -7
+ Partials 267 264 -3
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:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a panic in CopyContextToFiberContext by correctly handling both pointer and non-pointer context types. The addition of comprehensive test coverage, including numerous edge cases and error scenarios, significantly improves the robustness and maintainability of the adaptor middleware. The changes are well-structured, and the new test helpers improve the test suite's clarity. I have one minor suggestion to simplify the reflection logic in CopyContextToFiberContext.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/adaptor/adaptor_test.go (1)
731-807: Refactor parallel benchmark to avoid shared bytes.Buffer data race
Shared bytes.Buffer is reused across goroutines, causing a data race and stale reader—allocate a fresh reader inside each pb.Next() iteration:@@ Benchmark_FiberHandlerFunc_Parallel(b *testing.B) { - b.RunParallel(func(pb *testing.PB) { - w := httptest.NewRecorder() - r := http.Request{Method: http.MethodPost} - r.Body = io.NopCloser(bodyBuffer) - defer r.Body.Close() //nolint:errcheck - for pb.Next() { - handlerFunc(w, &r) - } - }) + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + w := httptest.NewRecorder() + r := http.Request{Method: http.MethodPost} + if bm.bodyContent != nil { + r.Body = io.NopCloser(bytes.NewReader(bm.bodyContent)) + } else { + r.Body = http.NoBody + } + handlerFunc(w, &r) + if r.Body != http.NoBody { + _ = r.Body.Close() + } + } + })
🧹 Nitpick comments (4)
middleware/adaptor/adaptor.go (1)
56-56: Avoid shadowing thecontextpackage in parameter namesRename the first parameter to
srcorctxAnyfor clarity and to prevent accidental confusion with the importedcontextpackage.middleware/adaptor/adaptor_test.go (3)
493-537: Strengthen “no panic” assertion in conversion error testCurrent logic only asserts if an error exists. Make the intent explicit by asserting non-panicking behavior and accepting either error/no-error.
- _, err := ConvertRequest(ctx, true) // Use forServer=true which does more validation - if err == nil { - // If the above doesn't fail, try a different approach - ctx.Request().SetRequestURI("\x00\x01\x02") // Invalid characters in URI - _, err = ConvertRequest(ctx, true) - } - // Note: This test may pass if fasthttpadaptor is very permissive - // The important thing is that our function doesn't panic - if err != nil { - require.Error(t, err, "Expected error from fasthttpadaptor.ConvertRequest") - } + require.NotPanics(t, func() { + _, _ = ConvertRequest(ctx, true) + // If permissive, try a clearly invalid URI as a second attempt + ctx.Request().SetRequestURI("\x00\x01\x02") + _, _ = ConvertRequest(ctx, true) + })
539-567: Add a nil-pointer context case to validate new guardGiven the new nil checks in
CopyContextToFiberContext, add an explicit nil-pointer test to lock in behavior.t.Run("unsupported context type", func(t *testing.T) { @@ }) t.Run("context with unknown field", func(t *testing.T) { @@ }) + +t.Run("nil pointer context", func(t *testing.T) { + t.Parallel() + var fctx fasthttp.RequestCtx + var ptr *struct{ key, val any } // nil + require.NotPanics(t, func() { + CopyContextToFiberContext(ptr, &fctx) + }) +})
656-729: Benchmark correctness: reuse of request/recorder skews results
- Body is consumed on first iteration and reused thereafter.
- Single
httptest.NewRecorder()reused across iterations accumulates state.Refactor to create a fresh request/recorder per iteration and reset the body.
- w := httptest.NewRecorder() - var bodyBuffer *bytes.Buffer - // Handle the "No Content" case where bodyContent is nil - if bm.bodyContent != nil { - bodyBuffer = bytes.NewBuffer(bm.bodyContent) - } else { - bodyBuffer = bytes.NewBuffer([]byte{}) // Empty buffer for no content - } - r := http.Request{ - Method: http.MethodPost, - Body: nil, - } - // Replace the empty Body with our buffer - r.Body = io.NopCloser(bodyBuffer) - defer r.Body.Close() //nolint:errcheck // not needed - b.ReportAllocs() - for b.Loop() { - handlerFunc.ServeHTTP(w, &r) - } + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + w := httptest.NewRecorder() + var r http.Request + r.Method = http.MethodPost + if bm.bodyContent != nil { + r.Body = io.NopCloser(bytes.NewReader(bm.bodyContent)) + } else { + r.Body = http.NoBody + } + handlerFunc.ServeHTTP(w, &r) + if r.Body != http.NoBody { + _ = r.Body.Close() + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/adaptor/adaptor.go(1 hunks)middleware/adaptor/adaptor_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/adaptor/adaptor.gomiddleware/adaptor/adaptor_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/adaptor/adaptor.gomiddleware/adaptor/adaptor_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
middleware/adaptor/adaptor.go
🧬 Code graph analysis (1)
middleware/adaptor/adaptor_test.go (1)
middleware/adaptor/adaptor.go (4)
ConvertRequest(47-53)CopyContextToFiberContext(56-104)HTTPMiddleware(107-137)FiberHandlerFunc(145-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (9)
middleware/adaptor/adaptor.go (1)
125-126: Nice: using RequestCtx() for adaptor interopUsing
c.RequestCtx()here aligns with Fiber v3’s renaming (Context()→RequestCtx(),UserContext()→Context()), and matches prior guidance. Looks good.Based on learnings
middleware/adaptor/adaptor_test.go (8)
437-454: Helpers look good
createTestRequestandexecuteHandlerTestreduce duplication and improve clarity. LGTM.
569-594: Middleware error propagation coverage looks solidThis verifies HTTP middleware interactions and Fiber error handler mapping correctly. LGTM.
595-621: Good IO error simulationThe
failingReaderpath ensuresio.Copyerrors return 500 without panics. LGTM.
622-644: Good handler error coverageAsserting the status from
fiber.NewErroris propagated as expected. LGTM.
838-899: Remote addr resolver cases are comprehensiveThe tests cover unix sockets, missing ports, and invalid formats. Nicely done.
901-926: Unix network detection test is straightforward and clearLGTM.
928-944: Good fallback assertion when addr resolution failsEnsures handler still runs when
resolveRemoteAddrreturns error. LGTM.
946-974: Unix socket adaptor test is valuableEnd-to-end check via real UDS, with timeouts and graceful shutdown. LGTM.
middleware/adaptor/adaptor.go
Outdated
| } | ||
|
|
||
| // CopyContextToFiberContext copies the values of context.Context to a fasthttp.RequestCtx. | ||
| // This function safely handles struct fields without using unsafe operations where possible. |
There was a problem hiding this comment.
without using unsafe operations
and
This function uses reflection and unsafe pointers
This sounds contradictory, maybe should reword.
…larify unsafe operations usage
sixcolors
changed the title
sixcolors
changed the title
… check pointer addresses
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the adaptor middleware. The fix for the panic in CopyContextToFiberContext is robust and well-tested, handling various edge cases gracefully. The security hardening measures, such as adding a body size limit and validating remote addresses, are valuable additions. The test coverage is impressively comprehensive, covering new functionality, error paths, and edge cases, which greatly increases confidence in the changes.
I have one suggestion regarding the implementation of the request body size limit to further enhance security by preventing potential HTTP request smuggling vulnerabilities. Overall, this is a high-quality contribution.
|
@gemini-code-assist what is your suggestion egarding the implementation of the request body size limit to further enhance security by preventing potential HTTP request smuggling vulnerabilities? |
|
Hello @sixcolors, That's an excellent question, and it highlights a critical aspect of web security. The current implementation of the request body size limit in
To further enhance security by preventing potential HTTP request smuggling vulnerabilities, beyond just mitigating their impact, my suggestion would be to add an explicit check for ambiguous message length headers within the HTTP request smuggling often arises from discrepancies in how different components (e.g., a frontend proxy and the backend application server) interpret HTTP message length headers, specifically Specific Suggestion: Consider adding a check to explicitly reject requests that contain both a Here's a conceptual example of how this could be implemented in // ... existing code ...
// Convert net/http -> fasthttp request with size limit
const maxBodySize = 10 * 1024 * 1024 // 10MB limit
// --- BEGIN SUGGESTION FOR FURTHER SMUGGLING PREVENTION ---
transferEncoding := r.Header.Get("Transfer-Encoding")
if transferEncoding != "" {
// If Transfer-Encoding is present, it should ideally be the only message length indicator.
// If Content-Length is also present and not -1 (meaning net/http didn't ignore it due to TE),
// this could indicate an ambiguous situation exploitable by request smuggling.
if r.ContentLength != -1 {
http.Error(w, "Ambiguous Content-Length and Transfer-Encoding headers detected. Rejecting to prevent HTTP request smuggling.", http.StatusBadRequest)
return
}
// Further check: if Transfer-Encoding is present but not "chunked", it might be malformed or unexpected.
// The net/http server usually handles this, but an explicit check adds robustness.
if strings.ToLower(transferEncoding) != "chunked" {
http.Error(w, "Unsupported or malformed Transfer-Encoding header. Rejecting to prevent HTTP request smuggling.", http.StatusBadRequest)
return
}
}
// --- END SUGGESTION ---
if r.Body != nil {
// ... existing body size limit and copy logic ...Rationale for this check:
It's also crucial to ensure that all components in your application's request path (load balancers, proxies, and the application server itself) are configured to parse HTTP headers, particularly |
|
@sixcolors llm making stuff up 😂 |
ReneWerner87
deleted the
fix/adaptor-context-panic-and-test-coverage
branch
🐛 Bug Fix & Security Hardening
Fixes a panic in
CopyContextToFiberContextwhen handling non-pointer context structs, adds comprehensive test coverage, and hardens the adaptor middleware against security vulnerabilities and performance issues.📝 Changes
Bug Fixes
CopyContextToFiberContext: Added proper handling for both pointer and non-pointer context types with minimal unsafe usage for unexported fields.Security Hardening
resolveRemoteAddr: Validates remote addresses to block malformed inputs and excessive lengths.Performance Improvements
Test Coverage Improvements
resolveRemoteAddr,isUnixNetwork, body size limits.createTestRequest,executeHandlerTestfor maintainable tests.Test Functions Added
Test_ConvertRequest- Comprehensive conversion testing with error handling.Test_CopyContextToFiberContext- Context copying with edge cases.Test_HTTPMiddleware_ErrorHandling- Middleware error path testing.Test_FiberHandler_IOError- IO failure scenarios.Test_FiberHandler_WithErrorInHandler- Handler error propagation.Test_resolveRemoteAddr- Address resolution unit tests with security validations.Test_isUnixNetwork- Network type validation.Test_FiberHandler_ErrorFallback- Invalid address fallback behavior.Test_FiberHandler_WithUnixSocket- Unix socket context handling.Test_FiberHandler_BodySizeLimit- Body size enforcement.Test_CopyContextToFiberContext_Safe- Safe handling of unexported fields.TestUnixSocketAdaptor- Full Unix socket integration test.🧪 Testing
All tests pass and provide comprehensive coverage of:
📊 Coverage
This PR significantly improves test coverage and security for the adaptor middleware, ensuring robust handling of: