Police OAuth egress with per-provenance address enforcement - #656
Conversation
github-actions
Bot
added
commands
The Manager passed its one general-purpose client to every SDK OAuth entry point, and a caller-supplied client is the caller's, enforcement included — so the SDK's dial-time SSRF address policy (basecamp-sdk#804/#810) was never live in the CLI. OAuth traffic now rides two lazily built lanes whose policies derive from operator configuration: BC5 (cfg.BaseURL; discovery both hops via WithIssuerHTTPClient, device authorization and polling, bc5-typed refreshes) and Launchpad (launchpadURL(); web-flow exchange, launchpad-typed refreshes), each admitting loopback exactly when its own anchor is local, so a localhost Launchpad override cannot grant loopback to a production BC5 flow. Each lane wraps a proxy-aware, fail-closed transport: resolver errors refuse the request outright, an operator opt-out (BASECAMP_OAUTH_USE_PROXY=1) routes only positively-proxied requests through a cloned DefaultTransport pinned to the one construction-time httpproxy snapshot (downgrades logged), and everything else stays on the surfguard-policed direct transport — NO_PROXY exclusions included, so there is no path to unguarded direct egress. Protected mode warns, deduplicated and by effective routing, when a configured proxy is ignored. checkAuthClientRedirect moves to internal/auth with its tests (appctx now passes nil and owns none of this), refreshLocked/exchangeCode stop flattening SDK errors to ErrAPI(0) so surfguard.ErrBlocked and typed statuses survive the CLI boundary, and the SDK pins to v0.15.0 via scripts/bump-sdk.sh (Tool.Name *string fallout included).
ae35477 to
9d950e4
Compare
There was a problem hiding this comment.
Pull request overview
Adds provenance-specific, SSRF-policed OAuth clients and proxy routing while upgrading the SDK to v0.15.0.
Changes:
- Separates BC5 and Launchpad OAuth egress policies.
- Adds guarded proxy routing and preserves typed SDK errors.
- Updates dependencies, tests, and SDK pointer compatibility.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/auth/client.go |
Implements guarded OAuth clients and proxy routing. |
internal/auth/client_test.go |
Tests policies, routing, and error preservation. |
internal/auth/auth.go |
Routes OAuth operations through provenance lanes. |
internal/auth/auth_test.go |
Updates redirect-guard test context. |
internal/appctx/context.go |
Enables Manager-owned OAuth clients. |
internal/appctx/context_test.go |
Removes relocated redirect tests. |
internal/commands/tools.go |
Handles nullable SDK tool names. |
internal/version/sdk-provenance.json |
Records the SDK/API revisions. |
go.mod |
Upgrades SDK and adds surfguard. |
go.sum |
Updates dependency checksums. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // TestOptOutMode_MalformedValuesAreOffWithWarning: only the exact value "1" | ||
| // opts out. Anything else — including set-but-empty — is treated as off, with | ||
| // a warning, and enforcement stays on. |
| // its query or userinfo — endpoint paths are diagnostic, query strings can | ||
| // carry parameters that don't belong in a terminal. | ||
| func redactedEndpoint(u *url.URL) string { | ||
| return u.Scheme + "://" + u.Host + u.Path |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae35477f56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // its query or userinfo — endpoint paths are diagnostic, query strings can | ||
| // carry parameters that don't belong in a terminal. | ||
| func redactedEndpoint(u *url.URL) string { | ||
| return u.Scheme + "://" + u.Host + u.Path |
There was a problem hiding this comment.
When a proxy applies, the endpoint may come from an attacker-controlled OAuth discovery document, and url.Parse decodes percent escapes into u.Path. Passing that path directly to warnf means an endpoint containing encoded newlines or terminal control sequences (for example %0a or %1b) can forge warning lines or manipulate the terminal. Render the escaped path or apply the repository's single-line sanitization before using the value in warnings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
internal/auth/client.go:195
u.Pathis decoded, so an endpoint containing%1b...or%0aputs terminal controls or a newline into this warning. Since this value is written to a single-line stderr sink, retain the escaped path (or apply the repository's terminal sanitizer) before logging it.
return u.Scheme + "://" + u.Host + u.Path
| name := "" | ||
| if tool.Name != nil { | ||
| name = *tool.Name | ||
| } | ||
| summary := fmt.Sprintf("%s (%s) at position %s", tool.Title, name, posStr) |
| t.warnOnce(req.URL, "warning: OAuth request to %s routed through proxy %s WITHOUT the SSRF address policy (%s=1)", | ||
| redactedEndpoint(req.URL), proxyURL.Redacted(), oauthUseProxyEnv) | ||
| return t.proxied.RoundTrip(req) | ||
| default: | ||
| t.warnOnce(req.URL, "warning: ignoring proxy %s for OAuth request to %s: the SSRF address policy requires direct egress; set %s=1 to route OAuth through the proxy without address enforcement", | ||
| proxyURL.Redacted(), redactedEndpoint(req.URL), oauthUseProxyEnv) |
The CLI passes its own
m.httpClientto every SDK OAuth entry point, and a caller-supplied client is the caller's, enforcement included — so neither basecamp-sdk#804's nor #810's SSRF address policy was live here: a malicious BC5 discovery document could still steer the device-authorization and token POSTs (carryingclient_id,device_code, refresh token) into private address space. This PR makes the policy live, pinned to SDK v0.15.0 (1dd547b3— the exact release commit, recorded viascripts/bump-sdk.sh).Design: per-provenance lanes, selected at the call sites
One loopback-enabled client shared by every lane would let a localhost
BASECAMP_LAUNCHPAD_URLgrant loopback to a production BC5 flow. Instead, two lazily built, cached, error-returning lanes on the Manager (bc5Client()/launchpadClient()), each{30 s, checkAuthClientRedirect, per-lane transport}:cfg.BaseURL;AllowLoopbackiff that host is local): discovery both hops —WithIssuerHTTPClientis passed unconditionally so a local resource's local advertised issuer isn't refused by the SDK's internal default right after hop 1 succeeds — plus device authorization + polling, and refreshes of bc5-typed credentials.launchpadURL(), env override included): web-flow code exchange and launchpad-typed refreshes. Refresh lane selection is by the storedOAuthType(a policy anchor, not a claimed binding —OAuthTypeandTokenEndpointpersist independently).hostutil.IsLocalhost(which is case-sensitive); errors name the anchor without echoing its value.appctxnow passes nil, andcheckAuthClientRedirect(the credential-replay guard, preserved on both lanes) moved intointernal/authwith its tests —appctxno longer owns behavior it doesn't use.Proxy handling: per-request, fail-closed, never unguarded direct
Each lane's transport is a proxy-aware wrapper over two sub-transports — the surfguard-policed direct transport, and (opt-out mode only) a clone of
http.DefaultTransportpinned to the one construction-time snapshot ofhttpproxy.FromEnvironment().ProxyFunc()(neverhttp.ProxyFromEnvironment, whose process-global cache could diverge from the snapshot). Per request, against the actual request URL (discovered issuer, device, polling, persisted refresh endpoints all evaluated):httpproxyproduces — CGIREQUEST_METHOD+HTTP_PROXY— is tested, plus an injected-resolver unit test);BASECAMP_OAUTH_USE_PROXY=1→ proxied sub-transport, enforcement off for exactly that request, downgrade logged (deduplicated per endpoint — the device poll re-POSTs the same URL);NO_PROXYexclusion stays enforced even in opt-out mode; there is no path to unguarded direct egress (the decisive regression test: opt-out +NO_PROXY-covered private target →surfguard.ErrBlocked, zero dials).Default (protected) mode enforces unconditionally and warns — deduplicated, driven by effective routing rather than variable presence — when a configured proxy is ignored for OAuth traffic, naming the opt-out knob. Malformed opt-out values (
yes,2) are off, with a warning.Error taxonomy across the CLI boundary
refreshLockedandexchangeCodeused to stringify SDK errors intoErrAPI(0, …), which would discard basecamp-sdk#813's redirect status and breakerrors.Is(err, surfguard.ErrBlocked). They now wrap preserving code, HTTP status, retryability, and the cause chain — tested forerrors.Is(ErrBlocked)anderrors.As(*basecamp.Error)survival on both paths.Deviations from the reviewed plan, on the record
BASECAMP_OAUTH_USE_PROXY=""(set-but-empty) is treated as unset silently, not warned: env-scrubbing (t.Setenv, direnv) is indistinguishable from intent, and warning would fire in every scrubbed environment. Non-empty malformed values warn as planned.httpproxy.Config.initsilently drops unparsable values — so that case degrades to no-proxy → guarded direct, still enforced (tested as such). The fail-closed branch exists for the errors the resolver does produce.internal/commands/tools.go:Tool.Namebecame*stringin SDK v0.15.0 — one-line deref fallout from the pin, matching the neighboringPositionhandling.Follow-up tied to the next SDK re-pin
TestRefreshLocked_RedirectStatusSurvivesis committed but skipped: v0.15.0 predates basecamp-sdk#813's token-endpoint redirect classification. Un-skip at the next re-pin.Verification
go build ./...,go vet ./...,gofmt -s -lclean, fullmake test,make lint(0 issues),make provenance-check. Existing OAuth tests (which inject clients) unchanged and green. Manualbasecamp auth loginagainst production and against a localhost bc3 dev server still deserves a pass before merge — the end-to-end local-chain admission/refusal is covered byTestDiscoverOAuth_LocalIssuerChainFollowsBaseURL, but a live login exercises the browser/device interaction this suite can't.Summary by cubic
Police OAuth egress with per-provenance address enforcement. Previously a single injected client bypassed the SDK’s SSRF policy; a malicious discovery doc could steer device/token POSTs into private address space. Now OAuth traffic rides lane-specific clients that enforce policy and preserve SDK error taxonomy.
BC5 lane: policy from cfg.BaseURL; loopback only if that host is local. Used for discovery (both hops via oauth.WithIssuerHTTPClient), device authorization/polling, and bc5-typed refreshes.
Launchpad lane: policy from validated launchpadURL(); used for web-flow code exchange and launchpad-typed refreshes. Lane chosen by stored OAuthType (not TokenEndpoint); loopback allowance does not leak between lanes.
Proxy routing: per-request resolver, fail-closed on resolver error. BASECAMP_OAUTH_USE_PROXY=1 routes only positively-proxied requests via a cloned DefaultTransport pinned to a construction-time
httpproxysnapshot (downgrades logged, deduped per endpoint). Otherwise uses the surfguard-guarded direct transport; NO_PROXY still enforced; no path to unguarded direct egress.Redirect guard on lane clients: block non-GET/HEAD redirects; cap at 10 hops.
Error handling: refresh/exchange wrap SDK errors without flattening; errors.Is(err, surfguard.ErrBlocked) and errors.As(*basecamp.Error) work; code/status/retryability survive.
Wiring: appctx now passes nil so internal/auth builds policed clients; redirect guard lives in internal/auth.
Dependencies: pin
github.com/basecamp/basecamp-sdk/goto v0.15.0; addgithub.com/basecamp/surfguard/go. Adjust for Tool.Name as*string.Rollout notes:
Written for commit 9d950e4. Summary will update on new commits.