Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ForbiddenHandlerModule with origin- and response-level forbidden detection/rewrites, expands tests for forbidden behavior and response-writer propagation, and ensures router middleware/on-request handlers expose the active response writer to modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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
Comment |
StarpTech
changed the title
…pper-discards-responsewriter
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2605 +/- ##
==========================================
- Coverage 64.25% 62.69% -1.56%
==========================================
Files 301 244 -57
Lines 42656 25776 -16880
Branches 4558 0 -4558
==========================================
- Hits 27409 16161 -11248
+ Misses 15226 8251 -6975
- Partials 21 1364 +1343
🚀 New features to boost your workflow:
|
StarpTech
requested review from
Noroth,
devsergiy,
endigma and
jensneuse
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/cmd/custom/module/module.go`:
- Around line 63-75: The origin hooks still run for streaming requests and thus
can buffer/consume streams; update ForbiddenHandlerModule.OnOriginRequest and
ForbiddenHandlerModule.OnOriginResponse to early-return (skip
creating/replacing/reading responses) when the request is a streaming request by
checking the same context flag used in Middleware (e.g.,
ctx.GetBool("streaming") or the project’s streaming flag set in Middleware) so
streaming flows are not consumed — mirror the Middleware streaming opt-out logic
to short-circuit both OnOriginRequest and OnOriginResponse.
- Around line 161-178: The middleware captures the original writer w but
replaces it with bw before calling next.ServeHTTP, causing downstream type
assertions against http.Flusher, http.Hijacker, http.Pusher, and
http.NewResponseController to fail; fix by having bufferedWriter hold the
captured real writer (set bw.real = w before calling next.ServeHTTP) and
implement the optional interfaces (Flush, Hijack, Push, NewResponseController)
on bufferedWriter to delegate to bw.real when available, or if delegation isn't
possible restore ctx.ResponseWriter() back to the original w after
next.ServeHTTP; update the bufferedWriter type and its methods and ensure
ForbiddenHandlerModule.Middleware sets the real writer on bw prior to
next.ServeHTTP so downstream code (which does w.(http.Flusher) etc.) continues
to work.
- Around line 201-205: The isStreamingRequest function currently checks the raw
Accept header case-sensitively; normalize the header value by calling
strings.ToLower on r.Header.Get("Accept") (and optionally strings.TrimSpace)
before matching so media types like "Text/Event-Stream" or "Multipart/Mixed" are
detected; update the checks in isStreamingRequest to search the lowercased
accept string for "text/event-stream" and "multipart/mixed".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b0ba98e-1c26-4380-9c37-173f1febadd9
📒 Files selected for processing (2)
router-tests/modules/middleware_writer_propagation_test.gorouter/cmd/custom/module/module.go
…pt header parsing
…pper-discards-responsewriter
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/modules/custom-forbidden-handler/module.go`:
- Around line 183-187: The handler currently copies all headers from bw.header
via maps.Copy then only removes Content-Length before writing
forbiddenErrorBody, which can leave payload-specific headers (e.g.,
Content-Encoding, ETag, Trailer, Transfer-Encoding, Content-Range) that no
longer match the response; change the logic in this branch (around maps.Copy,
w.Header(), bw.header and where forbiddenErrorBody is written) to either remove
those payload-specific headers explicitly after copying or, safer, build the
response headers by whitelisting only desired headers (e.g., Cache-Control,
Vary, custom tracing headers) and set those on w.Header() before writing
http.StatusOK and forbiddenErrorBody so no stale payload headers are forwarded.
- Around line 159-163: The streaming_request flag is set in the handler but
never inspected by the origin hooks, so update OnOriginRequest and
OnOriginResponse to check the request context for "streaming_request" (the same
key set via ctx.Set("streaming_request", true)) and, when present/true, skip the
forbidden-handling logic (short-circuit and body-rewrite paths) so streaming
requests are left untouched; locate these checks inside the OnOriginRequest and
OnOriginResponse handlers and gate the existing forbidden handling branches with
a guard that returns/continues when the flag is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb021c54-d923-43ac-90dc-1438a60de883
📒 Files selected for processing (1)
router-tests/modules/custom-forbidden-handler/module.go
… clean response headers
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router-tests/modules/custom-forbidden-handler/module.go (1)
39-41: Consider protectingforbiddenErrorBodyfrom accidental mutation.Package-level
varwith a byte slice can be modified at runtime. Since this is effectively a constant value, consider documenting the immutability expectation or converting usage to return a fresh copy when writing.💡 Alternative: use a function to return a fresh slice
-var forbiddenErrorBody = []byte(`{"errors":[{"message":"Insufficient permissions to fulfill the request.","extensions":{"errorCode":"FORBIDDEN"}}],"data":null}`) +var forbiddenErrorBody = []byte(`{"errors":[{"message":"Insufficient permissions to fulfill the request.","extensions":{"errorCode":"FORBIDDEN"}}],"data":null}`) + +// getForbiddenErrorBody returns a copy of the standardized error body. +// Use this if there's any risk of downstream mutation. +func getForbiddenErrorBody() []byte { + return append([]byte(nil), forbiddenErrorBody...) +}Since Go's
Writemethods don't typically mutate the input slice, this is low-risk, but worth noting for defensive coding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/modules/custom-forbidden-handler/module.go` around lines 39 - 41, forbiddenErrorBody is declared as a package-level var holding a byte slice that can be mutated; change usage to ensure immutability by replacing the mutable var with a provider that returns a fresh slice (e.g., a function like ForbiddenErrorBody() that returns a new []byte copy) or by documenting immutability and always writing a copy before use; update all call sites that pass forbiddenErrorBody to io.Writer methods (refer to forbiddenErrorBody and any writers in module.go) to use the provider/copy so the original bytes cannot be changed at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router-tests/modules/custom-forbidden-handler/module.go`:
- Around line 39-41: forbiddenErrorBody is declared as a package-level var
holding a byte slice that can be mutated; change usage to ensure immutability by
replacing the mutable var with a provider that returns a fresh slice (e.g., a
function like ForbiddenErrorBody() that returns a new []byte copy) or by
documenting immutability and always writing a copy before use; update all call
sites that pass forbiddenErrorBody to io.Writer methods (refer to
forbiddenErrorBody and any writers in module.go) to use the provider/copy so the
original bytes cannot be changed at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77ddc365-1e88-42e1-85c2-47878413ce13
📒 Files selected for processing (1)
router-tests/modules/custom-forbidden-handler/module.go
jensneuse
approved these changes
Mar 6, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/pkg/trace/meter.go (1)
171-179:TestErrorHandleris ignored unlessMemoryExporteris set.The only read of
Config.TestErrorHandlersits inside theconfig.MemoryExporter != nilbranch. If a caller sets the new hook with the normal OTLP exporters, this code still falls through tootel.SetErrorHandler(errHandler(config))below, so the new field becomes a silent no-op. Either wire it in both branches or narrow the field contract/name to make that limitation explicit.Also applies to: 225-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/trace/meter.go` around lines 171 - 179, The TestErrorHandler is currently only used when MemoryExporter is non-nil (in the MemoryExporter branch around MemoryExporter and errorLoggingExporter) which silently makes that hook a no-op for other exporters; update the logic so the TestErrorHandler is honored unconditionally: either wrap whatever exporter is being used with errorLoggingExporter when config.Config.TestErrorHandler != nil (not just inside the MemoryExporter branch) or, alternately, only call otel.SetErrorHandler(errHandler(config)) when config.Config.TestErrorHandler is set; in short, move the TestErrorHandler check out of the MemoryExporter-specific branch and apply it to the general exporter creation/wrapping code paths (references: MemoryExporter, errorLoggingExporter, errHandler(config), and otel.SetErrorHandler).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/trace/errors.go`:
- Around line 56-61: The ExportSpans method on errorLoggingExporter currently
calls e.handler(err) but still returns the error which causes the SDK (e.g.,
when used with sdktrace.WithSyncer / SimpleSpanProcessor) to call otel.Handle
and double-report; modify errorLoggingExporter.ExportSpans so that after calling
e.handler(err) it returns nil (only return the actual error when you want global
handling), ensuring local handling prevents otel.Handle from being invoked
twice.
---
Nitpick comments:
In `@router/pkg/trace/meter.go`:
- Around line 171-179: The TestErrorHandler is currently only used when
MemoryExporter is non-nil (in the MemoryExporter branch around MemoryExporter
and errorLoggingExporter) which silently makes that hook a no-op for other
exporters; update the logic so the TestErrorHandler is honored unconditionally:
either wrap whatever exporter is being used with errorLoggingExporter when
config.Config.TestErrorHandler != nil (not just inside the MemoryExporter
branch) or, alternately, only call otel.SetErrorHandler(errHandler(config)) when
config.Config.TestErrorHandler is set; in short, move the TestErrorHandler check
out of the MemoryExporter-specific branch and apply it to the general exporter
creation/wrapping code paths (references: MemoryExporter, errorLoggingExporter,
errHandler(config), and otel.SetErrorHandler).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33e1a3c1-5a1e-4b91-972d-6a9ac3f7d1a1
📒 Files selected for processing (6)
router-tests/error_handling_test.gorouter-tests/telemetry/attribute_processor_test.gorouter-tests/testenv/testenv.gorouter/pkg/trace/config.gorouter/pkg/trace/errors.gorouter/pkg/trace/meter.go
StarpTech
deleted the
dustin/eng-9117-router-module-middleware-wrapper-discards-responsewriter
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests
Checklist