Repository navigation
fix(oauth2,oidc)!: harden the authorization endpoint, issuer and metadata - #135
Merged
Merged
Conversation
euskadi31
force-pushed
the
fix/audit2-oauth2-front
branch
from
October 21, 2026 19:30
0fde4f5 to
eb70e31
Compare
ValidateRedirectURI, which /authorize also applies to the registered URI a request resolves to, treated http and https alike: under Profile20BCP and Profile21Draft, http://app.example/cb, LAN addresses and http://localhost received the code, the state and iss in clear text. RFC 9700 §2.6: "authorization servers MUST NOT allow redirection URIs that use the http scheme except for native clients that use loopback interface redirection as described in Section 7.3 of [RFC8252]"; OAuth 2.1 §1.5 and §8.4.2 say the same. ValidateRedirectURI now takes the server's profile: outside Profile20 (unknown profiles included) an http redirect URI must be the loopback IP literal http://127.0.0.1 or http://[::1] with an optional port — the definition that already grants port variance (RFC 8252 §7.3) — never localhost (RFC 8252 §8.3), another loopback address or another spelling of these two. /authorize answers any other registered http URI with the bare 400 of an invalid redirection URI (RFC 6749 §3.1.2.4). Profile20 keeps RFC 6749 §3.1.2.1, where TLS is a SHOULD. The examples move to loopback IP literals: examples/oauth2 to http://127.0.0.1:1337, examples/oidc-provider registers http://127.0.0.1:9000/oidc/demo/callback and validates its configured redirect URI with ValidateRedirectURI at startup, and e2e/oidc swaps its hosts (provider on localhost, relying party on 127.0.0.1). BREAKING CHANGE: ValidateRedirectURI(uri) becomes ValidateRedirectURI(profile, uri); under Profile20BCP and Profile21Draft, registered non-loopback http redirect URIs, localhost included, are refused. Audit: F-03.
The core published, recorded and sent as iss whatever the IssuerResolver returned: an empty issuer dropped iss from every authorization response while the metadata advertised authorization_response_iss_parameter_supported, and http or query-carrying issuers were published with endpoint URLs built from them. When the resolver failed, /authorize loaded the client without checking its issuer binding, validated its redirect URI and redirected a server_error without iss — carrying the requester's state — to a client of another realm, before any user authentication. RFC 8414 §2 and RFC 9207 §2: the issuer is "a URL that uses the https scheme" without query or fragment, and every authorization response, error responses included, MUST carry it. The server now validates the issuer with oidc/provider's rules (https, a host, no user info, query or fragment, canonical form) plus a valid port: NewServer refuses an invalid StaticIssuer (ErrInvalidIssuer), and every resolution is checked in resolveIssuer, so an invalid issuer is answered as a resolver failure — server_error at /token, /introspect and the metadata, 503 at /revoke. http on a loopback host needs the new ServerConfig.AllowInsecureLoopbackIssuer (development only). /authorize resolves the issuer before loading any client, and answers a resolver failure or an invalid issuer with a bare 500 that is never redirected (reversing the redirected, iss-less server_error kept by #84): no answer could name the issuer, nor could the client's binding be checked. Every authorization response now carries iss. oidcprovider.New writes one loopback-issuer setting to both its Config and the server's, and oidcprovider.ErrInvalidIssuer wraps oauth2.ErrInvalidIssuer, so errors.Is matches either side. BREAKING CHANGE: invalid issuers fail NewServer (StaticIssuer) or every request (custom resolvers); http issuers need AllowInsecureLoopbackIssuer and a loopback host; a resolver failure at /authorize is a bare 500 instead of a redirected server_error. Audit: F-04, I-01.
A request without response_type was answered unsupported_response_type.
RFC 6749 §4.1.1 makes the parameter REQUIRED and §4.1.2.1 defines
invalid_request for "a request missing a required parameter"; an empty
occurrence is an omitted one (§3.1). It is now answered invalid_request,
redirected like before (or a bare 400 for a client TrustedRedirect does
not trust).
/authorize also minted codes on a server without the authorization_code
grant: codes nothing could redeem, while the metadata (I-05) stops
advertising the code response type there. response_type=code is now
answered unsupported_response_type ("the authorization_code grant is not
enabled"), like a disabled implicit flow, before the ConsentFunc runs.
BREAKING CHANGE: a missing response_type yields invalid_request instead
of unsupported_response_type; a server without the authorization_code
grant refuses response_type=code.
Audit: I-02, I-05.
AuthorizeConfig.CodeTTL accepted any duration: a year-long code was stored and redeemable. RFC 6749 §4.1.2 RECOMMENDS a maximum lifetime of 10 minutes and OAuth 2.1 §4.1.2 requires codes to "expire shortly after it is issued"; a client redeems its code right after the redirect, so a longer lifetime only widens the window of a leaked code. AuthorizeHandler now panics on a CodeTTL beyond 10 minutes, like its other misconfiguration panics (oidcprovider.Provider.AuthorizeHandler inherits it); zero or negative still means 10 minutes. BREAKING CHANGE: AuthorizeHandler panics when AuthorizeConfig.CodeTTL exceeds 10 minutes. Audit: I-03.
A redirect answered in the query had no fragment, so browsers re-attached the fragment of the authorization request — whatever the author of the link put there — to the client's redirect URI. RFC 9700 §4.1.3: "servers MAY prevent browsers from reattaching fragments to redirection URLs by attaching an arbitrary fragment identifier, for example #_". Every authorization response answered in the query (codes, errors, 303s answering a POST) now ends with "#_"; the implicit response, answered in the fragment, is unchanged. Clients parsing the redirect as a URL are unaffected. The shared query-mode test helpers assert the reset fragment, and the anchored e2e/oidc regexes end in "#_$"; the browser carries the "#_" on to the relying party's own redirect home, as RFC 9700 §4.1.3 describes. BREAKING CHANGE: query-mode authorization responses end with "#_". Audit: I-04.
A client_credentials token names the client in sub (RFC 9068 §2.2). The implicit flow minted a resource owner's access token with whatever subject the ConsentFunc approved: a client registered under a user's subject got tokens resource servers could not tell from that user's (RFC 9700 §4.15). Like the resource-owner grants of /token, the implicit flow now refuses to mint a token whose subject equals the client's ID: a server_error redirected in the fragment, the cause oauth2.ErrSubjectIsClient for the ErrorHook, nothing generated or persisted — so that sub == client_id marks a token without resource owner exactly (RFC 9700 §4.15.1). Audit: F-08.
The metadata never published scopes_supported (RFC 8414 §2:
RECOMMENDED), advertised a jwks_uri the oauth2 module does not serve,
and advertised an authorization endpoint, the code response type and
PKCE methods on servers without the authorization_code grant —
grant_types_supported even encoded null without grants, which RFC 8414
§2 reads as ["authorization_code", "implicit"].
- ServerConfig.ScopesSupported is published as scopes_supported:
NewServer refuses a value that is not one RFC 6749 §3.3 scope token,
or a duplicate, and keeps its own copy (Config returns a copy).
- jwks_uri is published only with ServerConfig.AdvertiseJWKSURI, when
the issuer's JWK Set is served at {issuer}/.well-known/jwks.json.
- authorization_endpoint, response_modes and the iss support flag
follow the authorization flows served; "code" and the PKCE methods
need the authorization_code grant; the REQUIRED arrays are [] rather
than null. The implicit flag stays server-wide (documented).
oidcprovider.New sets AdvertiseJWKSURI (it serves the JWK Set there)
and gives the server the OpenID Connect scopes followed by the
deployment's own, which both metadata documents publish.
BREAKING CHANGE: jwks_uri is no longer advertised unless
ServerConfig.AdvertiseJWKSURI is set; servers without the
authorization_code grant no longer advertise an authorization endpoint.
Audit: I-05.
CHANGELOG, MIGRATION (before/after tables and steps for the redirect-URI transport rule, issuer validation, the /authorize changes and the metadata), LIMITATIONS, security-considerations, observability and the oidc-provider matrix follow the code: the iss-less server_error and the unvalidated issuer are gone from LIMITATIONS; the jwks_uri, D-02, D-04 and implicit-metadata entries are corrected. D-02 stays a documented, deliberate deviation, now named as such: with AuthorizeConfig.TrustedRedirect nil, errors before authentication are redirected to every registered client, against the RFC 9700 §4.11.2 MUST — the library has no dynamic client registration and OIDC Core §3.1.2.6 redirects prompt=none refusals; TrustedRedirect closes it. D-04 stays documented: realm membership is deployment data, and codes and tokens are issuer-bound whatever the client. Audit: F-03, F-04, I-01, I-02, I-03, I-04, I-05, D-02, D-04, F-08.
- CHANGELOG, MIGRATION and the architecture flow no longer describe the redirected server_error of a failed IssuerResolver: /authorize resolves the issuer first and answers a failure with a bare 500. - The issuer rules compare with the form Go's net/url writes: spellings RFC 3986 §6.2 holds equivalent but net/url keeps (upper-case host, ":443", a leading-zero port, dot segments, an IPv6 zone) are accepted and published as given — documented, with the advice to use the RFC 3986 §6.2.2 normal form that normalizing relying parties compare iss with (RFC 9207 §2.4). - oidc/provider drops its issuer re-checks at /authorize and /token: the core validates the same issuers first, with the loopback setting New gives both, so they could no longer fail. The exchange test now pins the core's refusal (TestExchangeAtARefusedIssuerIssuesNoIDToken). - Tests: port variance of a registered http://[::1] at /authorize, and the bodies of /token, /revoke and /introspect when the issuer is invalid (no issuer, no cause echoed). Comment and paragraph reflow. Audit: F-03, F-04, I-01.
euskadi31
force-pushed
the
fix/audit2-oauth2-front
branch
from
October 21, 2026 19:34
eb70e31 to
ab7d801
Compare
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.
Fixes the following findings of the internal security audit of 2026-10-21 (commit 1551aa6): F-03, F-04, I-01, I-02, I-03, I-04, I-05, D-02, D-04 — and the implicit-flow part of F-08 (option C).
Findings
F-03 — plain-http redirect URIs under
Profile20BCP/Profile21DraftValidateRedirectURI(used at registration and by/authorizeon the registered URI a request resolves to) treated http and https alike, so the profiles claiming RFC 9700 redirected codes,stateandisstohttp://app.example/cb, LAN addresses orhttp://localhostin clear text.ValidateRedirectURI(p Profile, uri string). OutsideProfile20(unknown profiles included) an http redirect URI must be the RFC 8252 §7.3 loopback IP literalhttp://127.0.0.1/http://[::1]with an optional valid port — the same raw-string definition that grants port variance, so "may use http" and "may vary the port" cannot disagree.localhost, other loopback addresses and other spellings (127.1,0x7f000001,[::ffff:127.0.0.1],HTTP://,:080…) are refused;/authorizeanswers with the existing bare 400, never redirected.Profile20is unchanged.127.0.0.1;examples/oidc-providervalidates its configured redirect URI at startup;e2e/oidcruns the RP on127.0.0.1and the provider onlocalhost.F-04 — the issuer is never validated
IssuerResolverreturned was published and sent asiss: an empty issuer silently droppedisswhile the metadata advertisedauthorization_response_iss_parameter_supported; http or query-carrying issuers were published and endpoint URLs built from them.checkIssuer— https (http on a loopback host only with the newServerConfig.AllowInsecureLoopbackIssuer), a host, no user info, query or fragment, a valid port, the form Go'snet/urlwrites (oidc/provider's rules, stricter on ports).NewServerrefuses an invalidStaticIssuer(oauth2.ErrInvalidIssuer);resolveIssuer, which every endpoint goes through, validates every resolution, so an invalid issuer is answered as a resolver failure.oidcprovider.Newwrites one loopback setting to both configs;oidcprovider.ErrInvalidIssuerwraps the core's. RFC 3986-equivalent spellingsnet/urlkeeps (upper-case host,:443) are accepted and documented.NewServeror every request; http issuers need the opt-in. MIGRATION section.I-01 —
IssuerResolverfailure: binding unchecked,server_errorredirected withoutiss/authorizeloaded the client without its issuer binding, validated its redirect URI and redirected aniss-lessserver_error, carrying the requester'sstate, to a client of another realm — before any user authentication./authorizeresolves the issuer before loading any client; a failure (or an invalid issuer) is a bare 500, never redirected, TrustedRedirect and the ConsentFunc never asked. Every authorization response now carriesiss. (Reverses the oidc/provider: implement Authorization Code + PKCE, ID Tokens, Discovery and UserInfo #84 choice, by decision.)I-02 — missing
response_typeinvalid_requestinstead ofunsupported_response_type(RFC 6749 §4.1.1, §4.1.2.1, §3.1: an empty occurrence is omitted). R1 (decided): a server without theauthorization_codegrant answersresponse_type=codewithunsupported_response_typeinstead of minting codes nothing can redeem.I-03 —
CodeTTLunboundedAuthorizeHandlerpanics on aCodeTTLbeyond 10 minutes (RFC 6749 §4.1.2; OAuth 2.1 §4.1.2), like its other misconfiguration panics; zero still means 10 minutes.I-04 — query-mode redirects carry no fragment
#_(RFC 9700 §4.1.3), so browsers do not re-attach the authorization request's fragment; implicit responses are unchanged. Clients parsing the redirect as a URL are unaffected (MIGRATION covers raw splitters and hash-routing SPAs).I-05 — metadata completeness and consistency
ServerConfig.ScopesSupported(published asscopes_supported; one RFC 6749 §3.3 token per value, no duplicate, copied byNewServerand cloned byConfig()); newServerConfig.AdvertiseJWKSURI—jwks_uriis no longer advertised by default since the module serves none (oidcprovider.Newsets it); without theauthorization_codegrant noauthorization_endpoint,codeor PKCE methods are advertised, and the REQUIRED arrays encode[], nevernull. The implicit flag stays server-wide (the server cannot observe which handler is mounted; documented, fails closed).jwks_urineedsAdvertiseJWKSURI(own MIGRATION row).D-02 — every registered client trusted to receive pre-authentication error redirects
TrustedRedirectnil the server deviates from the RFC 9700 §4.11.2 MUST ("authenticate the user first … before redirecting"), under every profile;TrustedRedirectcloses it. Reason: the library has no dynamic client registration (RFC 7591, the attacker's path in §4.11.2), and OpenID Connect Core §3.1.2.6 expectsprompt=nonerefusals to be redirected; a fail-closed default would turn every pre-authentication error into a bare page for every deployment. LIMITATIONS, security-considerations,ProfileandTrustedRedirectgodoc now say so.D-04 — clients span every issuer by default
IssuerBoundClient),StaticIssuerhas one realm, and codes/tokens are issuer-bound whatever the client.F-08 (implicit flow, option C)
server_error, causeoauth2.ErrSubjectIsClient, nothing minted or persisted — consistent with the/tokengrants of the oauth2-back PR (RFC 9068 §2.2, RFC 9700 §4.15).Tests
TestValidateRedirectURIProfileTransport: "Expected error … but got nil";TestAuthorizeIssuerResolverFailureIsABare500: the ClientStore mock expecting no call was called;TestQueryResponsesResetTheFragment: no#_;TestMetadataWithoutAuthorizationFlow:"grant_types_supported":null).MockIssuerResolver/MockClientStore; the shared query-mode helpers (oauth2, oidc/provider, interop, example) assert the#_fragment.make e2e-oidc(Tales, Chrome): 21/21 scenarios pass.Checks
make generate sync build test lint check-modulesgreen on Go 1.27.3 (lint 0 issues,make syncno diff).New code is fully covered; the uncovered lines of touched files are pre-existing (crypto/rand failure, a JSON-encode error).
Follow-ups
grant.ErrSubjectIsClient = oauth2.ErrSubjectIsClientand merge its "Client versus resource owner" security-considerations bullet with "Implicit tokens never name their client".http://localhostRedirectURL that this server now refuses under BCP/2.1; the example RP lands on/#_after login (it could reset its own redirect's fragment).parseIssuercould reuse the core's check (the core is stricter on ports); its two issuer re-checks at /authorize and /token are removed here (unreachable).resourceis multi-valued (keep it out of the single-valued parameters),invalid_targetjoins the authorization-endpoint codes.Notes for the maintainer
fix(oauth2,oidc)!: harden the authorization endpoint, issuer and metadata— the PR also changesoidc/providerbehaviour (shared loopback setting,ErrInvalidIssuerwrapping,AdvertiseJWKSURI, scopes).ValidateRedirectURI); the proposed replacement text is with the orchestrator.