Skip to content

fix(sdk/go): strip credential headers on cross-host redirect#602

Merged
AbirAbbas merged 2 commits into
mainfrom
fix/sdk-go-strip-credentials-cross-host-redirect
May 29, 2026
Merged

fix(sdk/go): strip credential headers on cross-host redirect#602
AbirAbbas merged 2 commits into
mainfrom
fix/sdk-go-strip-credentials-cross-host-redirect

Conversation

@AbirAbbas
Copy link
Copy Markdown
Contributor

@AbirAbbas AbirAbbas commented May 29, 2026

Summary

The Go SDK control-plane client (sdk/go/client) attaches the operator's credentials as custom request headers — X-API-Key, and when DID auth is configured X-Caller-DID / X-DID-Signature / X-DID-Timestamp / X-DID-Nonce — on an http.Client that follows redirects with no CheckRedirect policy.

Go's net/http strips Authorization, Cookie, Cookie2 and WWW-Authenticate when following a redirect to a different host, but it does not strip arbitrary application headers. So if the configured baseURL returned a cross-host redirect, the client would replay X-API-Key (and the DID headers) to the redirect target. Authorization set via WithBearerToken was already stripped by the stdlib; the gap was the custom headers.

Fix

  • Add a CheckRedirect hook (stripSensitiveHeadersOnRedirect) that removes every credential header the client attaches when a redirect targets a host other than the originally configured one. The comparison is against via[0] so credentials only ever reach the operator-configured host, even across a multi-hop redirect chain.
  • Same-host redirects keep the headers, so ordinary redirect following (e.g. a trailing-slash or path redirect) is unchanged.
  • The credential list is sourced from the existing did_auth header constants so it can't drift if a header is renamed.
  • Applied to both the default client and clients passed via WithHTTPClient that don't already define their own redirect policy.

Tests

  • TestStripsCredentialsOnCrossHostRedirect — end-to-end: a cross-host 302 no longer leaks X-API-Key / Authorization.
  • TestKeepsCredentialsOnSameHostRedirect — same-host redirect still carries the credential (no regression).
  • TestSendsCredentialsWithoutRedirect — baseline, no redirect.
  • TestStripSensitiveHeadersOnRedirectUnit — deterministic coverage of the full 6-header credential set; also asserts non-credential headers (e.g. Accept) are preserved.

Full sdk/go suite passes; gofmt and go vet clean on the changed files.

Refs GHSA-jp8j-g39q-qxwx.

🤖 Generated with Claude Code


Also included: control-plane coverage CI fix

Second commit (fix(handlers): stop flaky coverage CI from global-logger race). Pre-existing flake on main, unrelated to the SDK change but folded in to keep this to a single PR.

TestDiscoveryLoggingIncludesOptionalRequestID and TestExecutionCleanupService_StartStopBranches both swap the process-global logger.Logger (via setupExecutionCleanupTestLogger) while running t.Parallel(); a sibling test reassigned the global mid-run, emptying the captured buffer and failing the coverage (control-plane) job (which cascaded into coverage-summary). Fix drops t.Parallel() from both, matching the existing non-parallel logger-swapping tests in execution_cleanup_test.go. Verified with a 20x stress run of the previously-failing command; coverage jobs went green on the dedicated branch before folding in.

Separate pre-existing -race-only data races in the heartbeat coverage tests were observed but left for a follow-up (CI does not run -race).

The control-plane client attaches X-API-Key (and, when DID auth is
configured, the X-Caller-DID / X-DID-Signature / X-DID-Timestamp /
X-DID-Nonce headers) as custom request headers on an http.Client with no
CheckRedirect. Go's net/http strips Authorization/Cookie on a cross-host
redirect but not arbitrary application headers, so a redirect from the
configured baseURL to another host would replay the operator's
credentials to that host.

Add a CheckRedirect hook that drops every credential header the client
attaches when a redirect targets a host other than the originally
configured one (compared against via[0], so credentials only reach the
operator-configured host across a multi-hop chain). Same-host redirects
keep the headers, so ordinary redirect following is unaffected. The
credential list is sourced from the did_auth header constants so it
cannot drift. Also applied to clients supplied via WithHTTPClient that
define no redirect policy.

Refs GHSA-jp8j-g39q-qxwx.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@AbirAbbas AbirAbbas requested a review from a team as a code owner May 29, 2026 21:28
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

Performance

SDK Memory Δ Latency Δ Tests Status
Go 218 B -22% 0.60 µs -40%

✓ No regressions detected

…llel tests

TestDiscoveryLoggingIncludesOptionalRequestID and
TestExecutionCleanupService_StartStopBranches both call
setupExecutionCleanupTestLogger, which swaps the process-global
logger.Logger, while also calling t.Parallel(). When they ran alongside
another parallel test, a sibling reassigned the global logger mid-run, so
the discovery test's captured buffer came back empty and its assertions
("discovery request failed", `"request_id":"req-123"`, `"format":"compact"`)
failed. This broke the `coverage (control-plane)` job and cascaded into
`coverage-summary` (missing control-plane.total.txt).

The non-test build is unaffected, so it slipped past the required gates
and only showed up in the coverage job's full parallel run.

Drop t.Parallel() from both tests so they run in the serial phase, where
nothing else mutates the global logger concurrently -- matching the
existing non-parallel logger-swapping tests in execution_cleanup_test.go.
Verified by running the previously-failing command 20x clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

📊 Coverage gate

Thresholds from .coverage-gate.toml: per-surface ≥ 86%, aggregate ≥ 88%, max per-surface regression ≤ 1.0 pp, max aggregate regression ≤ 0.50 pp.

Surface Current Baseline Δ
control-plane 87.50% 87.30% ↑ +0.20 pp 🟡
sdk-go 92.00% 90.70% ↑ +1.30 pp 🟢
sdk-python 93.73% 93.63% ↑ +0.10 pp 🟢
sdk-typescript 92.80% 92.56% ↑ +0.24 pp 🟢
web-ui 89.93% 90.01% ↓ -0.08 pp 🟡
aggregate 89.03% 89.01% ↑ +0.02 pp 🟡

✅ Gate passed

No surface regressed past the allowed threshold and the aggregate stayed above the floor.

@github-actions
Copy link
Copy Markdown
Contributor

📐 Patch coverage gate

Threshold: 80% on lines this PR touches vs origin/main (from .coverage-gate.toml:thresholds.min_patch).

Surface Touched lines Patch coverage Status
control-plane 0 ➖ no changes
sdk-go 13 100.00%
sdk-python 0 ➖ no changes
sdk-typescript 0 ➖ no changes
web-ui 0 ➖ no changes

✅ Patch gate passed

Every surface whose lines were touched by this PR has patch coverage at or above the threshold.

@AbirAbbas AbirAbbas merged commit 6b74c63 into main May 29, 2026
36 of 38 checks passed
@AbirAbbas AbirAbbas deleted the fix/sdk-go-strip-credentials-cross-host-redirect branch May 29, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant