refactor: replace pre-built SigV4 body signing with lazy BodySigner closure passed through request handlers - #4735
Conversation
|
|
|
Warning Review limit reached
More reviews will be available in 14 minutes and 47 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughOpenAI and Anthropic request handlers now accept optional body signers and apply returned headers when present. Mantle wires SigV4 signers into those handlers, and provider call sites were updated to pass the expanded argument lists. ChangesSigned request plumbing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Confidence Score: 4/5Safe to merge for the targeted use case (non-large-payload SigV4 Bedrock Mantle requests); the streaming handlers have a known gap where LP passthrough and body-signing cannot be composed correctly, which the team is addressing in a dedicated follow-up. The core signing seam works correctly for the intended path: non-streaming handlers reach the signer only after the large-payload early-return guard so jsonData is always the real body; streaming handlers call the signer on the same bytes that setStreamingRequestBody will place on the wire in normal mode. The mantleSigV4Headers improvement propagating x-amz-* from extraHeaders into the canonical request is a net correctness win. However, streaming handlers have no guard preventing the signer from receiving a nil slice when the large-payload passthrough reader is active, which will produce a SHA-256 over zero bytes while the wire body is the unbuffered passthrough content — a confirmed failure mode for anyone combining SigV4 Mantle with large-payload streaming. That interaction is already acknowledged and tracked for a follow-up; the refactor itself does not make the non-streaming path worse. core/providers/openai/openai.go and core/providers/anthropic/anthropic.go streaming handlers (HandleOpenAIChatCompletionStreaming, HandleOpenAIResponsesStreaming, HandleAnthropicChatCompletionStreaming, HandleAnthropicResponsesStream) — these are the sites where the large-payload passthrough + signer guard needs to be added in the follow-up. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller as Caller (e.g. BedrockProvider)
participant Handler as OpenAI/Anthropic Handler
participant Signer as BodySigner closure
participant AWS as AWS SigV4 (mantleSigV4Headers)
participant Endpoint as Bedrock Mantle Endpoint
Caller->>Handler: HandleOpenAIChatCompletionRequest(..., signer)
Handler->>Handler: CheckContextAndGetRequestBody() → jsonData
alt "signer != nil (SigV4 path)"
Handler->>Signer: signer(jsonData)
Signer->>AWS: mantleSigV4Headers(jsonData, url, accept, key, region, extraHeaders)
AWS->>AWS: SHA256(jsonData), build canonical request
AWS-->>Signer: "{Authorization, X-Amz-Date, x-amz-content-sha256, Accept}"
Signer-->>Handler: sigHeaders
Handler->>Handler: req.Header.Set(sigHeaders...)
end
Handler->>Handler: req.SetBody(jsonData)
Handler->>Endpoint: POST with SigV4 headers + body
Endpoint-->>Handler: 200 OK
Handler-->>Caller: BifrostChatResponse
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller as Caller (e.g. BedrockProvider)
participant Handler as OpenAI/Anthropic Handler
participant Signer as BodySigner closure
participant AWS as AWS SigV4 (mantleSigV4Headers)
participant Endpoint as Bedrock Mantle Endpoint
Caller->>Handler: HandleOpenAIChatCompletionRequest(..., signer)
Handler->>Handler: CheckContextAndGetRequestBody() → jsonData
alt "signer != nil (SigV4 path)"
Handler->>Signer: signer(jsonData)
Signer->>AWS: mantleSigV4Headers(jsonData, url, accept, key, region, extraHeaders)
AWS->>AWS: SHA256(jsonData), build canonical request
AWS-->>Signer: "{Authorization, X-Amz-Date, x-amz-content-sha256, Accept}"
Signer-->>Handler: sigHeaders
Handler->>Handler: req.Header.Set(sigHeaders...)
end
Handler->>Handler: req.SetBody(jsonData)
Handler->>Endpoint: POST with SigV4 headers + body
Endpoint-->>Handler: 200 OK
Handler-->>Caller: BifrostChatResponse
Reviews (7): Last reviewed commit: "refactor: sign request body inside share..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 240-245: The signing path in anthropic request handling is using
the original jsonBody even when setAnthropicRequestBody switches to
large-payload passthrough and the request body is not actually set, so update
the signer input to use the exact bytes attached/sent on the request. Apply the
same fix in both streaming handlers as well, and in the relevant
request-building functions around setAnthropicRequestBody and signer, ensure any
signer error returns nil, bErr after performing the appropriate stream cleanup.
- Around line 245-247: The signer failure path in the Anthropic request flow
returns before cleanup, leaving acquired fasthttp resources and large-payload
passthrough state behind. Update the signer error handling in the affected
request/streaming paths around signer, resp, and DrainLargePayloadRemainder so
every error return first releases any acquired request/response objects and
drains any remaining large payload data before returning bErr. Ensure the same
cleanup is applied in all three affected locations, including the streaming
handlers.
In `@core/providers/bedrock/mantle.go`:
- Around line 80-82: The Mantle SigV4 signer currently signs only a synthetic
request with Accept, so any signable provider.networkConfig.ExtraHeaders
(especially x-amz-* headers) can be sent unsigned and break verification. Update
the signer closures in the Mantle flow to pass the relevant ExtraHeaders into
mantleSigV4Headers, or derive the canonical request from the prepared request
headers so the signed headers match what is actually sent.
In `@core/providers/openai/openai.go`:
- Around line 853-862: The large-payload passthrough path is returning before
SigV4 signing runs, so Mantle requests with an empty API key can skip request
signing. Move the signer execution ahead of the early return in
handleOpenAILargePayloadPassthrough so sigHeaders are always applied before any
passthrough decision. Also apply the same ordering fix in the corresponding
Responses helper in openai.go, using the same signer and header-setting flow.
- Around line 1054-1063: The signing logic in the streaming request flow is
applied before the final body and headers are fully assembled, so the signature
may not match what is actually sent. Update the streaming chat and responses
paths to call setStreamingRequestBody(...) before invoking signer(...), then
apply the returned signature headers to the request. Use the existing
setStreamingRequestBody and signer calls to ensure the body bytes and
Content-Type are finalized prior to signing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fc973f3-67a5-4dcc-84a4-f022afe7a996
📒 Files selected for processing (20)
core/providers/anthropic/anthropic.gocore/providers/azure/azure.gocore/providers/bedrock/mantle.gocore/providers/cerebras/cerebras.gocore/providers/fireworks/fireworks.gocore/providers/groq/groq.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/nebius/nebius.gocore/providers/ollama/ollama.gocore/providers/openai/openai.gocore/providers/opencode/opencode.gocore/providers/openrouter/openrouter.gocore/providers/parasail/parasail.gocore/providers/perplexity/perplexity.gocore/providers/sgl/sgl.gocore/providers/utils/bodysigner.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/providers/xai/xai.go
6f73e03 to
36367b7
Compare
255444b to
5ed0ed4
Compare
45b7753 to
ee786b0
Compare
6b75376 to
f0c7527
Compare
ee786b0 to
85f57cb
Compare
Merge activity
|
85f57cb to
d3fb617
Compare
f0c7527 to
453f7e9
Compare
The base branch was changed.
…ia a BodySigner hook
d3fb617 to
0ffa262
Compare
… closure passed through request handlers (#4735) ## Summary Replaces the pre-build-and-sign approach for Bedrock Mantle SigV4 authentication with a `BodySigner` callback that is invoked after the request handler has marshaled the body. This ensures the signature always covers the exact bytes sent on the wire, eliminating the previous double-marshal pattern where the body was built once for signing and again inside the handler. ## Changes - Introduces a new `BodySigner` type (`func(jsonData []byte) (map[string]string, *schemas.BifrostError)`) in `core/providers/utils/bodysigner.go`. Handlers call it after building the request body and apply the returned headers to the outgoing request. - Adds the `signer` parameter to `HandleOpenAIChatCompletionRequest`, `HandleOpenAIChatCompletionStreaming`, `HandleOpenAIResponsesRequest`, `HandleOpenAIResponsesStreaming`, `HandleAnthropicChatCompletionRequest`, `HandleAnthropicChatCompletionStreaming`, `HandleAnthropicResponsesRequest`, and `HandleAnthropicResponsesStream`. All existing callers pass `nil`. - Rewrites Bedrock Mantle's SigV4 paths (`mantleChatCompletions`, `mantleChatCompletionsStream`, `mantleResponses`, `mantleResponsesStream`) to construct a `BodySigner` closure when no API key is present, instead of pre-building the body, signing it, and merging the signature headers into `extraHeaders`. The Bearer path no longer needs a separate early-return branch. - Removes the now-unnecessary `maps` import and the intermediate `extraHeaders` map copies in the Mantle code paths. ## Type of change - [ ] Bug fix - [x] Refactor - [ ] Feature - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./... ``` For Bedrock Mantle with SigV4 (empty key value), verify that requests to chat completions, streaming chat completions, responses, and streaming responses are signed correctly and accepted by the Bedrock endpoint. For Bearer key paths, confirm that no signing is attempted and the `Authorization` header is set as expected. ## Breaking changes - [ ] Yes - [x] No ## Security considerations The `BodySigner` callback signs the exact serialized bytes that are placed on the wire. Previously, the body was serialized twice (once for signing, once inside the handler), which could in theory produce a signature mismatch if marshaling were non-deterministic. This change closes that gap by signing after the final body is set. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
… closure passed through request handlers (maximhq#4735) ## Summary Replaces the pre-build-and-sign approach for Bedrock Mantle SigV4 authentication with a `BodySigner` callback that is invoked after the request handler has marshaled the body. This ensures the signature always covers the exact bytes sent on the wire, eliminating the previous double-marshal pattern where the body was built once for signing and again inside the handler. ## Changes - Introduces a new `BodySigner` type (`func(jsonData []byte) (map[string]string, *schemas.BifrostError)`) in `core/providers/utils/bodysigner.go`. Handlers call it after building the request body and apply the returned headers to the outgoing request. - Adds the `signer` parameter to `HandleOpenAIChatCompletionRequest`, `HandleOpenAIChatCompletionStreaming`, `HandleOpenAIResponsesRequest`, `HandleOpenAIResponsesStreaming`, `HandleAnthropicChatCompletionRequest`, `HandleAnthropicChatCompletionStreaming`, `HandleAnthropicResponsesRequest`, and `HandleAnthropicResponsesStream`. All existing callers pass `nil`. - Rewrites Bedrock Mantle's SigV4 paths (`mantleChatCompletions`, `mantleChatCompletionsStream`, `mantleResponses`, `mantleResponsesStream`) to construct a `BodySigner` closure when no API key is present, instead of pre-building the body, signing it, and merging the signature headers into `extraHeaders`. The Bearer path no longer needs a separate early-return branch. - Removes the now-unnecessary `maps` import and the intermediate `extraHeaders` map copies in the Mantle code paths. ## Type of change - [ ] Bug fix - [x] Refactor - [ ] Feature - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./... ``` For Bedrock Mantle with SigV4 (empty key value), verify that requests to chat completions, streaming chat completions, responses, and streaming responses are signed correctly and accepted by the Bedrock endpoint. For Bearer key paths, confirm that no signing is attempted and the `Authorization` header is set as expected. ## Breaking changes - [ ] Yes - [x] No ## Security considerations The `BodySigner` callback signs the exact serialized bytes that are placed on the wire. Previously, the body was serialized twice (once for signing, once inside the handler), which could in theory produce a signature mismatch if marshaling were non-deterministic. This change closes that gap by signing after the final body is set. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
… closure passed through request handlers (#4735) ## Summary Replaces the pre-build-and-sign approach for Bedrock Mantle SigV4 authentication with a `BodySigner` callback that is invoked after the request handler has marshaled the body. This ensures the signature always covers the exact bytes sent on the wire, eliminating the previous double-marshal pattern where the body was built once for signing and again inside the handler. ## Changes - Introduces a new `BodySigner` type (`func(jsonData []byte) (map[string]string, *schemas.BifrostError)`) in `core/providers/utils/bodysigner.go`. Handlers call it after building the request body and apply the returned headers to the outgoing request. - Adds the `signer` parameter to `HandleOpenAIChatCompletionRequest`, `HandleOpenAIChatCompletionStreaming`, `HandleOpenAIResponsesRequest`, `HandleOpenAIResponsesStreaming`, `HandleAnthropicChatCompletionRequest`, `HandleAnthropicChatCompletionStreaming`, `HandleAnthropicResponsesRequest`, and `HandleAnthropicResponsesStream`. All existing callers pass `nil`. - Rewrites Bedrock Mantle's SigV4 paths (`mantleChatCompletions`, `mantleChatCompletionsStream`, `mantleResponses`, `mantleResponsesStream`) to construct a `BodySigner` closure when no API key is present, instead of pre-building the body, signing it, and merging the signature headers into `extraHeaders`. The Bearer path no longer needs a separate early-return branch. - Removes the now-unnecessary `maps` import and the intermediate `extraHeaders` map copies in the Mantle code paths. ## Type of change - [ ] Bug fix - [x] Refactor - [ ] Feature - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./... ``` For Bedrock Mantle with SigV4 (empty key value), verify that requests to chat completions, streaming chat completions, responses, and streaming responses are signed correctly and accepted by the Bedrock endpoint. For Bearer key paths, confirm that no signing is attempted and the `Authorization` header is set as expected. ## Breaking changes - [ ] Yes - [x] No ## Security considerations The `BodySigner` callback signs the exact serialized bytes that are placed on the wire. Previously, the body was serialized twice (once for signing, once inside the handler), which could in theory produce a signature mismatch if marshaling were non-deterministic. This change closes that gap by signing after the final body is set. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Replaces the pre-build-and-sign approach for Bedrock Mantle SigV4 authentication with a
BodySignercallback that is invoked after the request handler has marshaled the body. This ensures the signature always covers the exact bytes sent on the wire, eliminating the previous double-marshal pattern where the body was built once for signing and again inside the handler.Changes
BodySignertype (func(jsonData []byte) (map[string]string, *schemas.BifrostError)) incore/providers/utils/bodysigner.go. Handlers call it after building the request body and apply the returned headers to the outgoing request.signerparameter toHandleOpenAIChatCompletionRequest,HandleOpenAIChatCompletionStreaming,HandleOpenAIResponsesRequest,HandleOpenAIResponsesStreaming,HandleAnthropicChatCompletionRequest,HandleAnthropicChatCompletionStreaming,HandleAnthropicResponsesRequest, andHandleAnthropicResponsesStream. All existing callers passnil.mantleChatCompletions,mantleChatCompletionsStream,mantleResponses,mantleResponsesStream) to construct aBodySignerclosure when no API key is present, instead of pre-building the body, signing it, and merging the signature headers intoextraHeaders. The Bearer path no longer needs a separate early-return branch.mapsimport and the intermediateextraHeadersmap copies in the Mantle code paths.Type of change
Affected areas
How to test
go test ./...For Bedrock Mantle with SigV4 (empty key value), verify that requests to chat completions, streaming chat completions, responses, and streaming responses are signed correctly and accepted by the Bedrock endpoint. For Bearer key paths, confirm that no signing is attempted and the
Authorizationheader is set as expected.Breaking changes
Security considerations
The
BodySignercallback signs the exact serialized bytes that are placed on the wire. Previously, the body was serialized twice (once for signing, once inside the handler), which could in theory produce a signature mismatch if marshaling were non-deterministic. This change closes that gap by signing after the final body is set.Checklist
docs/contributing/README.mdand followed the guidelines