security: harden OAuth discovery and token endpoints#480
Merged
Conversation
This was referenced May 30, 2026
There was a problem hiding this comment.
Pull request overview
Hardens OAuth discovery-derived endpoints to reduce protocol abuse and credential replay risk when interacting with potentially malicious OAuth/discovery servers.
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.
Changes:
- Validates authorization endpoints before browser dispatch, allowing only HTTPS or loopback HTTP.
- Validates BC3 dynamic client registration endpoints with existing secure URL checks.
- Disables redirects for the auth HTTP client used by OAuth discovery/token refresh and adds coverage for unsafe authorization schemes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/auth/auth.go |
Adds endpoint validation for dynamic registration and authorization URL construction. |
internal/auth/auth_test.go |
Adds regression coverage for accepted/rejected authorization endpoint schemes. |
internal/appctx/context.go |
Configures the auth HTTP client to avoid following redirects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
7b1d3ea to
61aea03
Compare
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
d858f7f to
9e7bb58
Compare
![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: 9e7bb58e48
ℹ️ 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".
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
61aea03 to
e06c5c2
Compare
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
9e7bb58 to
3f9352d
Compare
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
e06c5c2 to
9103d13
Compare
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
3f9352d to
396ecac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 396ecac645
ℹ️ 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".
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
396ecac to
6a3a964
Compare
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
9103d13 to
dd63a13
Compare
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
dd63a13 to
4566262
Compare
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
6a3a964 to
8add014
Compare
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
2c0c841 to
04096c1
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
commands
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
04096c1 to
77e1dbe
Compare
github-actions
Bot
removed
commands
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
parsePath now validates absolute URLs against the configured base URL host:
a path is accepted only if it's relative or an absolute Basecamp URL on the
same host (path extracted, account segment dropped — the SDK re-prefixes the
configured account). Absolute URLs on any other host are rejected before any
network call, so the SDK never attaches the Authorization: Bearer header to a
foreign host. A stray leading slash ('/https://evil/…') and mixed-case schemes
('HTTPS://…') are normalized first so neither can smuggle an absolute URL past
the host check. Error propagated through get/post/put/delete; api_test.go
covers same-host extraction, query preservation, and foreign-host rejection.
API-controlled strings (names, summaries, notices, headlines, affordance
commands) reached styled/markdown sinks and the --watch TUI without ANSI
stripping, allowing terminal/OSC injection. Sanitize WithSummary/WithNotice/
WithDiagnostic at the source, the timeline watch TUI fields (creator name
stripped before the empty check so an all-escape name still falls back to a
placeholder), and presenter format helpers, with defense-in-depth at the
render sinks. Stripping is centralized in RenderTemplate + RenderHeadline so
schema headlines ({{.name}}/{{.subject}}/{{.content}}) and affordance commands
are all covered, including the identity-label headline fallback.
Assert the exact sanitized string instead of mirroring ansi.Strip, which is a weaker contract than richtext.SanitizeTerminal and ties the expectation to the third-party stripper's behavior.
Stripping keys let a hostile key like "\x1b[31mtitle" collapse onto a legitimate "title", silently replacing or hiding that field in TTY --jq output. Keys changed by sanitization are now visibly escaped via strconv.Quote, so clean keys always keep their slot and no entry is dropped. Also update a stale test comment that referenced ansi.Strip.
The escaped form of a hostile key could still collide with a literal key that mimics escape notation (e.g. a real ESC-prefixed title vs the text "\x1b[31mtitle"), nondeterministically dropping one entry. Clean keys now claim their names first, and escaped keys re-quote with delimiters until unique, in sorted order for deterministic output.
Request IDs and string ids print to line-oriented sinks, but SanitizeTerminal preserves newlines/tabs (which inject lines or columns into ids output) and deletes bare CR (which joins adjacent words). A shared sanitizeSingleLine helper normalizes CR/CRLF to newlines first, then strips and collapses all whitespace to single spaces.
- Scheme-validate the discovery authorization_endpoint (https, or http on loopback) before it's dispatched to the OS browser launcher, so a hostile discovery doc can't hand the OS a file:// (or other) URL. - Enforce RequireSecureURL on the DCR registration_endpoint. - Set CheckRedirect on the OAuth HTTP client so token exchange/refresh POST bodies (auth code, refresh_token) aren't replayed to a redirect target. Idempotent GET/HEAD (OAuth discovery) may still follow redirects, so we don't break discovery or force an unintended Launchpad fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Harden OAuth discovery and token endpoints
(Threat: T1 malicious/compromised OAuth/discovery server.)
authorization_endpointscheme validation (auth.gobuildAuthURL): the endpoint comes from the server-controlled discovery doc and is later handed toOpenBrowser→xdg-open/open. Now required to behttps(orhttpon loopback); afile:///--leading value is rejected before dispatch.registration_endpointscheme validation (registerBC3Client): the DCR path now validates the endpoint against the stricterisSecureEndpointURLwhitelist (absolute URL with a non-empty host;https, orhttpon loopback), and re-validates every redirect hop with the same rule.appctx/context.go):CheckRedirect: ErrUseLastResponse. The exchange/refresh requests setGetBody, so Go would replayrefresh_token/auth code to a redirect target; RFC 6749 token endpoints don't legitimately 3xx-redirect POSTs. Mirrors the SDK API client's redirect guard.Tests
TestBuildAuthURL_RejectsUnsafeScheme(accepts https / http-loopback, rejectsfile://, foreignhttp://,javascript:,-flag).Part 3/6 of the stacked security series. Base:
security/02-output-ansi-sanitize.📚 Stack (merge bottom-up)
apito prevent token leak #478 — reject foreign-host URLs inapi(basemain)Each is independent except #482 depends on #481 (shared
root.go). #478 can land first/alone.