Police OAuth egress with per-provenance address enforcement - #656
Conversation
github-actions
Bot
added
commands
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.
![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".
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
9d950e4 to
6bd254e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
internal/auth/client.go:171
req.URLcan come from OAuth metadata, and this warning is emitted before the guarded transport runs. A percent-encoded escape sequence is decoded intoURL.Path, soredactedEndpointcan place raw terminal controls on stderr (for example%1b%5b31m). Sanitize the fully formatted warning withrichtext.SanitizeSingleLinebefore invoking the terminal sink, and add a regression case with encoded ESC/C1 characters; this matches the terminal-sink handling ininternal/richtext/sanitize.go:35-49.
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)
internal/auth/client_test.go:445
- The test below asserts that set-but-empty is silent, so this test description currently states the opposite behavior. Limit “with warning” to non-empty malformed values.
// 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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Review round converged at All six threads addressed and resolved (escape-safe endpoint rendering, host-only proxy redaction, nil |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/auth/client.go:139
- This bullet says a malformed
HTTP_PROXYcannot degrade into egress, buthttpproxydrops unparsable proxy URLs;proxyURL == nilthen permits guarded direct egress to public addresses, asTestOAuthTransport_MalformedProxyValueStaysEnforceddocuments. Distinguish resolver errors (which are refused) from malformed proxy URLs so this security contract matches the implementation.
// - the snapshot proxy resolver errors → the request is refused before
// either sub-transport runs. A malformed HTTP_PROXY must not degrade
// into egress the operator asked to route elsewhere;
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).
…tool name, nix hash - Render OAuth endpoint paths percent-encoded in proxy warnings and dedupe keys: url.Parse decodes escapes into Path, so a hostile discovery document could put terminal control sequences or a newline into stderr. - Log proxies as scheme://host only. url.URL.Redacted masks passwords but preserves bare usernames and query values, both of which HTTP(S)_PROXY can carry as credentials. - Omit the "(name)" parenthetical in `tools show` when Tool.Name is nil, which SDK v0.15.0 documents as always for the Get projection. - Fix two doc comments that claimed set-but-empty BASECAMP_OAUTH_USE_PROXY warns; it is silently treated as unset by design. - Refresh the Nix vendorHash for the surfguard dependency (the flake job was red).
EscapedPath only covered the path: url.Parse admits UTF-8 C1 controls (U+009B CSI) in a host verbatim, so a discovery-controlled endpoint host could still inject terminal controls through the proxy warnings. Scrub the whole rendered message once in Manager.warnf, which covers the endpoint host and path, the proxy host, and any field a later warning interpolates.
TestCatalogModelProvenance requires the MCP model snapshot to match the SDK version go.mod pins. Synced via scripts/sync-mcp-model.sh from go/v0.15.0 (1dd547b3): API revision 2026-08-05 → 2026-08-11, description and required-field tightening only, no operation changes.
0adf164 to
53c210a
Compare
The rebase merged main's dependency bumps (basecamp/cli, x/crypto, mcp) with this branch's SDK v0.15.0 + surfguard pins, which invalidates the fixed-output derivation hash. Value from the flake job's own computation.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/commands/tools.go:132
- The new nil-name branch is the compatibility behavior required by SDK v0.15, but the existing
tools showfixtures always includenameand no test asserts the rendered summary when it is omitted. Add atools showcase with a response lackingnameand verify the summary isTitle at position N(without()), so this SDK fallout cannot regress unnoticed.
summary := fmt.Sprintf("%s at position %s", tool.Title, posStr)
if tool.Name != nil {
summary = fmt.Sprintf("%s (%s) at position %s", tool.Title, *tool.Name, posStr)
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
SDK v0.15.0's Get projection omits name; pin that the summary reads 'Title at position N' with no empty parenthetical, and 'Title (name) at position N' when a name is present.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/auth/client.go:27
- This security-sensitive opt-out is the only supported way for OAuth to work in proxy-only environments, but it is not documented in the README’s OAuth environment-variable table. Please document the exact
=1behavior, that it disables address enforcement only for requests actually routed through a proxy, and thatNO_PROXYrequests remain guarded; otherwise operators must infer a security-critical rollout setting from runtime warnings or source code.
const oauthUseProxyEnv = "BASECAMP_OAUTH_USE_PROXY"
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Review round converged at Since the last summary:
Verification: all checks green on One thread left open on purpose ( |
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
Previously one injected client bypassed the SDK's SSRF policy; a malicious discovery doc could steer device/token POSTs into private address space. OAuth egress now rides per-provenance clients that enforce the address policy and preserve SDK error taxonomy.
cfg.BaseURL(loopback only when that anchor is local) and carries discovery both hops, device auth/polling, and bc5-typed refreshes.launchpadURL()and carries web-flow code exchange and launchpad-typed refreshes; loopback allowance does not cross lanes.BASECAMP_OAUTH_USE_PROXY=1routes only positively-proxied requests through a clonedDefaultTransportpinned to a construction-timehttpproxysnapshot (downgrades logged, deduped per endpoint); otherwise the surfguard-guarded direct transport is used withNO_PROXYstill enforced — no path to unguarded direct egress. Warnings are sanitized at the sink and render endpoints percent-encoded and proxies asscheme://hostonly.errors.Is(err, surfguard.ErrBlocked)and typed code/status/retryability survive.appctxpasses nil sointernal/authbuilds the policed clients and owns the redirect guard.github.com/basecamp/basecamp-sdk/goto v0.15.0, addsgithub.com/basecamp/surfguard/go, rebases onto main'sbasecamp/cli,x/crypto, andmcpbumps, and syncs the vendored MCP model (description/required-field tightening only, no operation changes).tools showomits the name parenthetical whenTool.Nameis nil.Rollout:
BASECAMP_OAUTH_USE_PROXY=1; any other non-empty value is off with a warning; empty is treated as unset.BASECAMP_LAUNCHPAD_URLare correct; loopback is allowed only when that lane's anchor is local.http.Clientin production; it disables enforcement by design.Written for commit d71a015. Summary will update on new commits.