fix: clean Bedrock headers#24718
Conversation
|
/coder-agents-review |
There was a problem hiding this comment.
Targeted fix for a real Bedrock auth failure when ANTHROPIC_API_KEY is in the process environment. The transport-layer approach is the correct interception point given the Anthropic SDK's structure: there is no hook between header injection and SigV4 signing. Implementation is clean, well-scoped, and the test reproduces the exact production scenario.
The main architectural concern is that the transport independently loads AWS config via config.LoadDefaultConfig, creating a shadow credential path detached from the SDK's own config. Today they agree; the coupling is invisible until they diverge. 1 P2 on that, plus 4 P3s (testing gaps, comment clarity, unconditional re-signing) and 2 nits.
As Pariston notes: the root fix is likely two lines in coder/anthropic-sdk-go's bedrock middleware (strip X-Api-Key and Anthropic-Version before signing, alongside the existing anthropic-beta handling at bedrock.go:233-234). That would eliminate the transport, the re-signing, the independent config load, and all of the findings below. This workaround is correct if the SDK fix is tracked; it becomes permanent maintenance cost if it is not.
"The vulnerability is in the architecture, not the implementation." (Hisoka)
1 P2, 4 P3, 2 Nit.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 10 R1 findings resolved. The author took the panel's architectural recommendation (DEREM-4) and moved the header stripping into the SDK's bedrock middleware, deleting the 153-line transport-layer workaround entirely. The net diff is now 78 lines: one regression test and an SDK version bump. The shadow credential path (P2), the sync.Once permanent failure cache, the unconditional re-signing, and every test coverage gap dissolved with the layer change.
The regression test is genuine: it reproduces the exact production scenario (ANTHROPIC_API_KEY in env polluting Bedrock requests), exercises the full middleware stack end-to-end, and verifies five independent properties of the outgoing request. Multiple reviewers traced the injection and stripping paths through three repos and confirmed the test would fail if the SDK fix were reverted.
3 P3s remain, all documentation/metadata.
"The agent accepted that the R1 approach was wrong and acted on the structural feedback rather than patching the symptoms." (Mafu-san)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 17 findings across 3 rounds addressed. Zero open items.
R1 raised 1 P2 + 4 P3 + 2 Nit on a transport-layer workaround. R2: the author deleted the workaround and moved the fix into the SDK's bedrock middleware, resolving all 10 findings. R2 panel raised 3 P3s (stale go.mod comment, PR title scope, missing test doc comment). R3: all 3 addressed. Netero R3 scan clean.
The final diff is 87 lines: a well-documented regression test, an SDK version bump, and an updated go.mod comment. Clean.
🤖 This review was automatically generated with Coder Agents.
Bedrock chat provider requests can inherit Anthropic public API headers from the process environment, which causes mixed Anthropic and Bedrock auth headers on signed requests.
Update the Anthropic SDK fork so its Bedrock middleware strips Anthropic-only headers before signing requests, and keep a chatprovider regression test for the production request shape.