feat: Canonical OAuth error code constants - #167
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request removes the deprecated BaseURL configuration in favor of a unified Issuer URL to comply with RFC 8414 §3, updating the configuration loading, validation, and various handlers accordingly. It also introduces a new oautherror package to centralize canonical OAuth 2.0 error code constants, replacing scattered string literals across the codebase. The review feedback suggests enhancing validateIssuer to reject URLs containing user information for security, and expanding the oautherror package to include standard CIBA Core 1.0 §11 error codes along with their corresponding test cases.
rsharath
changed the title
rsharath
marked this pull request as ready for review
May 26, 2026 03:25
rsharath
changed the title
There was a problem hiding this comment.
Pull request overview
Introduces a centralized set of canonical OAuth/OpenID error-code constants and updates handler/service emission sites to use them, reducing duplicated string literals and making RFC-defined wire values easier to verify and keep consistent.
Changes:
- Added
internal/oautherrorwith RFC-grouped string constants + a unit test ensuring constants are non-empty. - Swept multiple handler/service OAuth error emission paths to use
oautherror.*constants instead of raw strings. - (Stacked/adjacent) Updates related to
Token.BaseURLremoval in favor ofToken.Issueracross config, server wiring, docs, and tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/oautherror/codes.go |
Defines canonical protocol error-code constants grouped by spec. |
internal/oautherror/oautherror_test.go |
Guards against accidentally-empty error-code constants. |
internal/service/oauth.go |
Replaces many OAuth error-code literals with constants in token flows. |
internal/service/oauth_error.go |
Uses constants for invalid_client / server_error structured errors. |
internal/service/backchannel.go |
Switches multiple CIBA OAuth error emissions to constants (plus remaining literals noted). |
internal/handler/oauth.go |
Uses constants in token endpoint error responses and extraction mapping. |
internal/handler/dynamic_registration.go |
Uses constants for DCR/DCR-management OAuth error responses. |
internal/handler/auth_verify.go |
Uses constants when setting WWW-Authenticate and JSON error responses. |
internal/handler/wellknown.go |
Uses issuer as the URL prefix for advertised metadata endpoints. |
internal/handler/routes.go |
Removes baseURL from API wiring; keeps only issuer. |
server.go |
Updates API construction to match the new NewAPI signature (issuer-only). |
config.go |
Adds issuer validation + rejects removed token.base_url / ZEROID_BASE_URL. |
zeroid.yaml |
Updates sample config and comments to reflect issuer semantics. |
docs/dpop-and-dcr.md |
Updates docs to refer to cfg.Token.Issuer instead of removed BaseURL. |
tests/sdk/test_sdk_smoke.py |
Updates smoke-test env to set ZEROID_ISSUER to the local server URL. |
tests/sdk/sdk-smoke.test.ts |
Same as above for TS smoke test. |
tests/integration/helpers_test.go |
Removes redundant BaseURL test config field. |
tests/integration/discovery_compliance_test.go |
Updates assertion message to refer to Issuer (not BaseURL). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replaces bare OAuth error code strings ("invalid_token", "invalid_grant",
"invalid_dpop_proof", etc.) scattered across handler/service emission sites
with named constants from a new internal/oautherror package, grouped by RFC
with spec URLs preserved as comments.
Covered specs:
- RFC 6749 §5.2 (token endpoint errors)
- RFC 6750 §3.1 (Bearer Token error codes)
- RFC 7591 §3.2.2 / RFC 7592 §2.3 (DCR errors)
- RFC 9396 §5.4 (RAR authorization_details)
- RFC 9449 §5 (DPoP)
Constants are plain string consts (not a typed alias) so they remain
assignable to the many string parameter slots existing emission sites use,
without forcing conversions at every call site.
Test files asserting on observed wire output are intentionally left as
literal strings — they verify bytes-on-the-wire, not the internal symbol.
…des stay literal Addresses PR-167 review (concern #1): the inconsistency where some emitted error codes go through oautherror constants (RFC-defined) while others remain bare string literals (Highflame-specific, e.g. "policy_violation" in extractOAuthError) had no in-code disambiguator. Future contributors adding new emission sites needed a signal to decide which path to use. This commit adds an explicit "Scope" paragraph to the package doc that: - States the invariant: every constant here MUST map to a clause in a published RFC. - Names the alternative: Highflame-internal codes either stay literal or move to a separate Highflame-namespaced package. No code change — just the doc comment.
… §11 codes
Three gemini-code-assist findings:
1. config.go validateIssuer — added u.User != nil check. Issuer URLs with
embedded user-info (https://user:pass@host) would leak credentials
into the JWT iss claim and every published URI. RFC 8414 §2's "URL
using the https scheme" language excludes user-info by reference to
RFC 3986's URI composition rules for OAuth identifiers. Rejected
with a clear message naming the security concern.
2. internal/oautherror/codes.go — added the OpenID CIBA Core 1.0 §11
error codes (authorization_pending, slow_down, expired_token,
access_denied) under a new RFC group. CIBA codes are
spec-defined wire values just like the OAuth ones; they belong here
for the same reason RFC 6750 codes belong here.
3. internal/oautherror/oautherror_test.go — added the four new
constants to TestConstantsAreNonEmpty so an accidental empty-string
declaration fails the existing guard.
Also: swept internal/service/backchannel.go (13 emission sites) to use
the new constants. Per the package's documented invariant (every
constant maps to an RFC clause; emission sites use the constant), the
sweep keeps the codebase consistent — adding CIBA constants without
sweeping their emission sites would leave a stale-string anti-pattern.
One bare string intentionally left at backchannel.go:744 — it's a
code-comment reference ("Most likely an OAuthError("access_denied")"),
not an emission site, so the package's invariant doesn't apply.
Full integration test suite passes.
After the main-merge update brought PR-164's RAR-side code in,
internal/service/backchannel.go gained 3 new emission sites with bare
"invalid_authorization_details" strings (lines 758, 769, 801).
These are RFC 9396 §5.4 codes that ARE in the oautherror package,
so per PR-167's documented invariant ("every constant maps to an RFC
clause; emission sites use the constant") they should use the
existing oautherror.InvalidAuthorizationDetails — leaving them as
literals would be the anti-pattern this PR exists to eliminate.
No new constants needed; the package already had it.
Full integration test suite passes.
rsharath
force-pushed
the
feat/oauth-error-code-constants
branch
from
May 26, 2026 04:51
26be8e5 to
20e91c8
Compare
Two concerns flagged in the post-rebase code review:
1. internal/oautherror/codes.go — the "Scope" paragraph said emission sites
use constants but didn't explicitly cover comments. Future grep results
for bare error code strings in non-test code would hit the constant
declarations AND any prose-style comments that quote the wire value
("returns invalid_grant on ..."), creating a false-positive
"incomplete sweep" signal. Added a "Convention" paragraph that
spells out the rule: emission sites = constants, comment quotes =
bare strings allowed.
2. config.go validateIssuer userinfo error — the inline message was 175
characters, spilling past terminal width on operators' startup logs.
The actionable signal ("must not contain user-info") was already in
the first clause; the security rationale was duplicated from the
comment block above. Trimmed to just the actionable signal; the
rationale stays in the comment for code readers.
Skipped: the CIBA Core §11 naming suggestion (CibaAccessDenied, etc.) —
the current names match the RFC strings verbatim and there's no
naming clash with another RFC in the codebase's actual usage today.
Renaming would obscure the spec mapping for speculative benefit.
…or docs The original "Scope" paragraph (from 8d8945e earlier in this PR) ended with "future Highflame-internal codes either stay literal or move to a separate Highflame-namespaced constants package." Read literally that sounded like such a package exists or is being created — it doesn't, and it isn't. The clause is hypothetical guidance for *if* Highflame ever decides to formalize its product-specific codes (like "policy_violation"). Rephrased to make the hypothetical nature explicit: names the example path (internal/highflameerror), states why the separation matters (keeps oautherror's RFC-only invariant trustworthy), and ends with "No such package exists or is planned today; noted for if/when the question comes up." Pure documentation; no code change.
4 tasks
saucam
approved these changes
May 26, 2026
…rinfo check
config.go's validateIssuer was added via PR-163 without tests, and the
userinfo check this PR adds inherits that gap. Closes both before merge.
Table-driven test (style matches the adjacent TestValidateWIMSEDomain)
with 16 cases covering:
- Valid shapes: bare host, with path, http on localhost, custom port
- Empty / trailing-slash: empty string, "/" with and without path
- Scheme: ftp://, missing scheme, schemeless "//host" double-slash
- Host: opaque URL shape that leaves Host empty after parse
- User-info: "user:pass@" and bare "user@" forms (PR-167's new check)
- Query / fragment: ?key=val and #frag
- Parse-failure: control character in URL (one of the few inputs that
trips url.Parse — Go's url.Parse is otherwise permissive)
The user-info cases are the security-relevant ones this PR added:
issuer URLs with embedded credentials would leak them into the JWT iss
claim, every metadata document, and every published URI.
Substring assertions (not exact-equality) match the precedent and let
operators read the error messages without the tests being fragile to
wording tweaks.
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.
Problem
The zeroid codebase had bare OAuth error code strings —
"invalid_token","invalid_request","invalid_client\","invalid_grant", etc. scattered across handler and service emission sites. String literals compile cleanly when mistyped, offer no autocomplete, and have no central source of truth tying each value back to the spec that defines it.Change
Introduces
internal/oautherrorwith canonical constants grouped by RFC. Each group is preceded by a comment naming the spec section and a URL to the canonical text so future readers can verify the wire values without grep-archaeology.Sweeps all emission sites in
internal/handler/andinternal/service/to use the constants. Test files that assert on observed wire-format JSON are intentionally left as literal strings, they verify bytes-on-the-wire, not the internal symbol, and pinning them to the constant would defeat their purpose as a contract test.Constants are plain string consts (not a typed alias) so they remain assignable to the many
stringparameter slots existing emission sites use, without forcing conversions at every call site. This is internal scaffolding, not a public API.RFCs covered
invalid_client,invalid_grant,unauthorized_client,unsupported_grant_type,invalid_scope,server_errorinvalid_request,invalid_token,insufficient_scopeinvalid_client_metadata,invalid_redirect_uri,invalid_software_statementinvalid_authorization_detailsinvalid_dpop_proofFiles swept
internal/handler/auth_verify.gointernal/handler/dynamic_registration.gointernal/handler/oauth.gointernal/service/backchannel.gointernal/service/oauth.gointernal/service/oauth_error.goNotes for reviewers
feat/rfc-9728-www-authenticate-breadcrumb) is introducing. Once both land, the PR-E author will rebase to use the constants from this package — there is no need to coordinate the literal values across the two PRs since both will converge onoautherror.*.feat/eliminate-token-baseurl) merging first. This branch is cut from that PR's tip to avoid merge conflicts in adjacent emission sites.domain/errors.go(Highflame-specificZEROID-40101codes) is intentionally untouched — that's a different namespace (application-level, not OAuth protocol-level).Test plan
GOEXPERIMENT=jsonv2 go build ./...cleanGOEXPERIMENT=jsonv2 go vet ./...cleanGOEXPERIMENT=jsonv2 go test -count=1 ./...— full unit + integration suite green (37s integration runtime)internal/oautherrorpackage test confirms no constant is accidentally empty