security: harden OAuth discovery and token endpoints#480
Conversation
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}
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/02-output-ansi-sanitize
branch
from
1e3cb4c to
acf9bf6
Compare
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
fdf0fc8 to
2c0c841
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
| // SanitizeTerminal removes terminal escape sequences and control characters | ||
| // from API-controlled text before it reaches a terminal sink. | ||
| // | ||
| // ansi.Strip alone is not enough: it only removes ESC-prefixed (7-bit) | ||
| // sequences and preserves UTF-8-encoded Unicode C1 controls — U+009B (CSI), |
There was a problem hiding this comment.
Flagged for human review — the sanitizer is not part of this PR: this PR is stacked on #479 (base branch security/02), which introduces richtext.SanitizeTerminal and describes it. The sanitizer appearing in this diff is stack-sync bleed from newer commits on the base branch, not scope expansion; leaving the description decision to the author.
There was a problem hiding this comment.
Acknowledged, but leaving this for the author's call rather than resolving: the internal/richtext sanitizer this refers to is introduced by the stacked base branch (#479 / security/02-output-ansi-sanitize), not by this PR's own diff — it only appears in #480's range because #480 is stacked on top. The PR title/description scope is #480's OAuth-endpoint hardening; the sanitizer is documented on #479 where it belongs. No code change here.
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. |
- 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.
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.