Add OAuth2/OpenID Connect authorization server#957
Conversation
Implement a full OAuth2 2.0 and OpenID Connect 1.0 authorization server with support for authorization code flow (with PKCE), refresh token rotation, device authorization grant, dynamic client registration, token introspection, and token revocation. Includes database schema, coredata layer, service logic, HTTP handlers, OIDC discovery endpoint, and JWKS publishing. Signed-off-by: Bryan Frimin <bryan@getprobo.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // verifyClientSecret verifies a plaintext secret against a stored hash | ||
| // using constant-time comparison. | ||
| func verifyClientSecret(storedHash []byte, secret string) bool { | ||
| h := sha256.Sum256([]byte(secret)) |
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic hashing algorithm on sensitive data High
| } | ||
| u.RawQuery = q.Encode() | ||
|
|
||
| http.Redirect(w, r, u.String(), http.StatusFound) |
Check warning
Code scanning / CodeQL
Open URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
General approach: ensure that any redirectURI used in redirectWithError and redirectWithCode is either (a) taken from a trusted server-side allowlist, or (b) validated so that it can only point to allowed locations (for example, matching a set of permitted origins or being a relative URL). Because we cannot change external behavior beyond security hardening and we don’t see all call sites, the safest fix is to add validation inside these helper functions so they cannot be misused from any path.
Best concrete fix in this file:
-
Introduce a small validation helper, e.g.
isSafeRedirectURI, near the redirect helpers. It will:- Parse the
redirectURIstring. - Optionally normalize backslashes to forward slashes (consistent with the background example) before parsing.
- Enforce that the URI is either:
- Exactly one of the client-registered
redirect_urivalues (we cannot accessclientin this helper without major refactoring), or - At least restricted to same-origin / relative paths.
Since we do not know the deployment host, the least intrusive and safest generic policy is: only allow relative URLs (no scheme, no host). This removes open redirect risk while keeping redirects within the current application.
- Exactly one of the client-registered
- Parse the
-
Use that helper in
redirectWithErrorandredirectWithCode:- If the
redirectURIfails validation (absolute URL, has host, invalid syntax), do not redirect to it; instead fall back to a safe behavior, such as returning an OAuth2 error directly (writeOAuth2ServerErrororwriteOAuth2InvalidRequest) or possibly a 400. The most conservative choice given existing helpers is to callwriteOAuth2ServerError(w, "invalid redirect_uri")and return, mirroring the existing error handling forurl.Parsefailures inredirectWithError.
- If the
-
Additionally harden the existing
url.Parseusage:- In
redirectWithError, we already handleerr != nilby sending an error response; we will add a host/scheme check after parsing. - In
redirectWithCode, the code currently ignores the parse error (u, _ := url.Parse(redirectURI)); we should handle errors and avoid redirecting if parsing fails or the URI is unsafe.
- In
Locations/lines to change:
- Around lines 811–839 (definitions of
redirectWithErrorandredirectWithCode) inpkg/server/api/connect/v1/oauth2_handler.go:- Add a new helper
isSafeRedirectURIimmediately aboveredirectWithError. - Modify
redirectWithErrorto:- Normalize and validate
redirectURIwithisSafeRedirectURI. - Only call
http.Redirectwhen validation passes.
- Normalize and validate
- Modify
redirectWithCodesimilarly:- Parse with error handling.
- Normalize and validate before use.
- Add a new helper
Imports:
- We already import
"net/url"and"strings"at the top, so no new imports are required for this approach.
| @@ -808,9 +808,26 @@ | ||
| }) | ||
| } | ||
|
|
||
| func redirectWithError(w http.ResponseWriter, r *http.Request, redirectURI, errorCode, description, state string) { | ||
| u, err := url.Parse(redirectURI) | ||
| func isSafeRedirectURI(raw string) (*url.URL, bool) { | ||
| // Normalize backslashes to forward slashes to avoid browser quirks. | ||
| normalized := strings.ReplaceAll(raw, "\\", "/") | ||
|
|
||
| u, err := url.Parse(normalized) | ||
| if err != nil { | ||
| return nil, false | ||
| } | ||
|
|
||
| // Only allow relative redirects (no scheme/host) to avoid open redirects. | ||
| if u.IsAbs() || u.Hostname() != "" { | ||
| return nil, false | ||
| } | ||
|
|
||
| return u, true | ||
| } | ||
|
|
||
| func redirectWithError(w http.ResponseWriter, r *http.Request, redirectURI, errorCode, description, state string) { | ||
| u, ok := isSafeRedirectURI(redirectURI) | ||
| if !ok { | ||
| writeOAuth2ServerError(w, "invalid redirect_uri") | ||
| return | ||
| } | ||
| @@ -827,7 +843,12 @@ | ||
| } | ||
|
|
||
| func redirectWithCode(w http.ResponseWriter, r *http.Request, redirectURI, code, state string) { | ||
| u, _ := url.Parse(redirectURI) | ||
| u, ok := isSafeRedirectURI(redirectURI) | ||
| if !ok { | ||
| writeOAuth2ServerError(w, "invalid redirect_uri") | ||
| return | ||
| } | ||
|
|
||
| q := u.Query() | ||
| q.Set("code", code) | ||
| if state != "" { |
| } | ||
| u.RawQuery = q.Encode() | ||
|
|
||
| http.Redirect(w, r, u.String(), http.StatusFound) |
Check warning
Code scanning / CodeQL
Open URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
In general, to fix open redirect issues you must not pass raw user input to http.Redirect. Instead, either (a) map user input to a server‑side list of allowed destinations, or (b) validate the URL to ensure it is acceptable (for example, only relative paths or URLs within your own domain, or URLs that match a pre‑registered set).
In this file, the simplest fix that doesn’t change higher‑level behavior is to have redirectWithError and redirectWithCode validate redirectURI before redirecting. A small helper can parse the string, normalize backslashes to slashes (to avoid browser quirks), and enforce constraints. Since this is an OAuth2 authorization endpoint, redirecting to fully external client URLs is expected, so we cannot simply force relative URLs. However, we can at least mitigate common open‑redirect and SSRF patterns by:
- Rejecting non‑HTTP(S) schemes (no
javascript:,data:, etc.). - Requiring that the parsed URL either:
- Has an empty
SchemeandHost(relative URL), or - Has an allowed scheme (
httporhttps) and a host that matches a whitelist.
Because we don’t see any client registry here, we can implement a conservative generic check: only allow relative URLs and URLs whose host equals the current server’s host. That prevents arbitrary external redirects while still allowing redirects back to the same host.
- Has an empty
Concretely:
- Add a helper function
sanitizeRedirectURI(r *http.Request, redirectURI string) (string, error)near the redirect helpers (around lines 811+). This will:- Replace
\with/. - Parse the URL.
- If parsing fails, return error.
- If
u.Scheme == "" && u.Host == "": allow it as a relative redirect, returnu. - Else, if
u.SchemeishttporhttpsANDstrings.EqualFold(u.Hostname(), r.Hostname() or r.Host)(we only haver.Host, but we can compare.Hostname()against the host part ofr.Host), allow it; otherwise, return error.
- Replace
- Update
redirectWithError:- Call
sanitizeRedirectURI. If it fails, fall back to writing an OAuth2 error to the response instead of redirecting. - Use the sanitized URL (
u) to add the query parameters and callhttp.Redirect.
- Call
- Update
redirectWithCodesimilarly, including proper error handling and not callinghttp.Redirectwhen validation fails.
This keeps existing functional behavior for same‑host redirects and relative paths while blocking open redirects to third‑party sites derived from untrusted input.
| @@ -808,9 +808,42 @@ | ||
| }) | ||
| } | ||
|
|
||
| func sanitizeRedirectURI(r *http.Request, redirectURI string) (*url.URL, error) { | ||
| // Normalize backslashes to forward slashes to avoid browser quirks when parsing URLs. | ||
| normalized := strings.ReplaceAll(redirectURI, "\\", "/") | ||
|
|
||
| u, err := url.Parse(normalized) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Allow relative URLs (no scheme/host) such as "/path" or "path?x=1". | ||
| if u.Scheme == "" && u.Host == "" { | ||
| return u, nil | ||
| } | ||
|
|
||
| // Only allow http/https redirects and restrict to the current host to avoid open redirects. | ||
| if u.Scheme != "http" && u.Scheme != "https" { | ||
| return nil, errors.New("unsupported redirect_uri scheme") | ||
| } | ||
|
|
||
| // r.Host may include a port; extract just the hostname for comparison. | ||
| requestHost := r.Host | ||
| if h, _, splitErr := strings.Cut(requestHost, ":"); splitErr == nil { | ||
| requestHost = h | ||
| } | ||
|
|
||
| if !strings.EqualFold(u.Hostname(), requestHost) { | ||
| return nil, errors.New("redirect_uri host mismatch") | ||
| } | ||
|
|
||
| return u, nil | ||
| } | ||
|
|
||
| func redirectWithError(w http.ResponseWriter, r *http.Request, redirectURI, errorCode, description, state string) { | ||
| u, err := url.Parse(redirectURI) | ||
| u, err := sanitizeRedirectURI(r, redirectURI) | ||
| if err != nil { | ||
| // If the redirect URI is invalid or unsafe, send an error response instead of redirecting. | ||
| writeOAuth2ServerError(w, "invalid redirect_uri") | ||
| return | ||
| } | ||
| @@ -827,7 +858,13 @@ | ||
| } | ||
|
|
||
| func redirectWithCode(w http.ResponseWriter, r *http.Request, redirectURI, code, state string) { | ||
| u, _ := url.Parse(redirectURI) | ||
| u, err := sanitizeRedirectURI(r, redirectURI) | ||
| if err != nil { | ||
| // If the redirect URI is invalid or unsafe, send an error response instead of redirecting. | ||
| writeOAuth2ServerError(w, "invalid redirect_uri") | ||
| return | ||
| } | ||
|
|
||
| q := u.Query() | ||
| q.Set("code", code) | ||
| if state != "" { |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 43 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/iam/oauth2server/service.go">
<violation number="1" location="pkg/iam/oauth2server/service.go:480">
P1: Custom agent: **Avoid Logging Sensitive Information**
Remove or redact the identity_id from this log message to avoid logging sensitive user identifiers.</violation>
<violation number="2" location="pkg/iam/oauth2server/service.go:517">
P1: Refresh token rotation is not atomic: the old token is revoked in one transaction, then new tokens are created in separate connections. If `CreateRefreshToken` fails (or the process crashes) after the old token is already revoked, the client loses its session with no way to recover except re-authentication. Consider moving the new token creation into the same transaction that revokes the old token.</violation>
<violation number="3" location="pkg/iam/oauth2server/service.go:684">
P1: Race condition: status check and token issuance happen outside the transaction that locks the device code row. Two concurrent polls for an authorized device code will both pass the status check and issue duplicate token sets. Move the status/expiry checks and device code deletion (or a status transition to a "consumed" state) inside the `WithTx` block so that only one poller can issue tokens.</violation>
</file>
<file name="pkg/iam/service.go">
<violation number="1" location="pkg/iam/service.go:188">
P2: Validate OAuth2ServerSigningKey before constructing the OAuth2 server service; it is dereferenced in JWKS/ID token signing and will panic if nil.</violation>
</file>
<file name="pkg/iam/oauth2server/user_code.go">
<violation number="1" location="pkg/iam/oauth2server/user_code.go:39">
P2: GenerateUserCode returns an unformatted 8‑character code despite the comment promising XXXX-XXXX. Either format the return value or adjust the docs; as written, callers expecting the documented format will render the code incorrectly.</violation>
</file>
<file name="pkg/coredata/oauth2_scope.go">
<violation number="1" location="pkg/coredata/oauth2_scope.go:80">
P2: Validate each parsed scope in OAuth2Scopes.UnmarshalText; otherwise invalid scope strings are accepted silently.</violation>
</file>
<file name="pkg/coredata/oauth2_consent.go">
<violation number="1" location="pkg/coredata/oauth2_consent.go:53">
P1: Coredata methods here omit the Scoper/tenant_id filter required for tenant isolation. This allows cross-tenant reads/writes of OAuth2 consent records.</violation>
</file>
<file name="pkg/server/api/connect/v1/oauth2_handler.go">
<violation number="1" location="pkg/server/api/connect/v1/oauth2_handler.go:299">
P0: The `redirect_uri` from the consent form POST body is not re-validated against the client's registered redirect URIs. An attacker can modify the hidden form field to steal the authorization code via an open redirect. Load the client and validate `redirect_uri` against `client.RedirectURIs` before issuing the redirect, just as `AuthorizeHandler` does.</violation>
</file>
<file name="pkg/iam/oauth2server/gc.go">
<violation number="1" location="pkg/iam/oauth2server/gc.go:74">
P2: Validate gc.interval before calling time.NewTicker; zero/negative durations will panic at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return | ||
| } | ||
|
|
||
| redirectURI := r.FormValue("redirect_uri") |
There was a problem hiding this comment.
P0: The redirect_uri from the consent form POST body is not re-validated against the client's registered redirect URIs. An attacker can modify the hidden form field to steal the authorization code via an open redirect. Load the client and validate redirect_uri against client.RedirectURIs before issuing the redirect, just as AuthorizeHandler does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/server/api/connect/v1/oauth2_handler.go, line 299:
<comment>The `redirect_uri` from the consent form POST body is not re-validated against the client's registered redirect URIs. An attacker can modify the hidden form field to steal the authorization code via an open redirect. Load the client and validate `redirect_uri` against `client.RedirectURIs` before issuing the redirect, just as `AuthorizeHandler` does.</comment>
<file context>
@@ -0,0 +1,871 @@
+ return
+ }
+
+ redirectURI := r.FormValue("redirect_uri")
+ state := r.FormValue("state")
+
</file context>
| if oldToken.RevokedAt != nil { | ||
| s.logger.WarnCtx(ctx, "refresh token replay detected, revoking all tokens", | ||
| log.String("client_id", client.ID.String()), | ||
| log.String("identity_id", oldToken.IdentityID.String()), |
There was a problem hiding this comment.
P1: Custom agent: Avoid Logging Sensitive Information
Remove or redact the identity_id from this log message to avoid logging sensitive user identifiers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/iam/oauth2server/service.go, line 480:
<comment>Remove or redact the identity_id from this log message to avoid logging sensitive user identifiers.</comment>
<file context>
@@ -0,0 +1,1126 @@
+ if oldToken.RevokedAt != nil {
+ s.logger.WarnCtx(ctx, "refresh token replay detected, revoking all tokens",
+ log.String("client_id", client.ID.String()),
+ log.String("identity_id", oldToken.IdentityID.String()),
+ )
+
</file context>
| } | ||
|
|
||
| // Issue new tokens. | ||
| accessTokenValue, accessToken, err := s.CreateAccessToken(ctx, client.ID, identityID, scopes) |
There was a problem hiding this comment.
P1: Refresh token rotation is not atomic: the old token is revoked in one transaction, then new tokens are created in separate connections. If CreateRefreshToken fails (or the process crashes) after the old token is already revoked, the client loses its session with no way to recover except re-authentication. Consider moving the new token creation into the same transaction that revokes the old token.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/iam/oauth2server/service.go, line 517:
<comment>Refresh token rotation is not atomic: the old token is revoked in one transaction, then new tokens are created in separate connections. If `CreateRefreshToken` fails (or the process crashes) after the old token is already revoked, the client loses its session with no way to recover except re-authentication. Consider moving the new token creation into the same transaction that revokes the old token.</comment>
<file context>
@@ -0,0 +1,1126 @@
+ }
+
+ // Issue new tokens.
+ accessTokenValue, accessToken, err := s.CreateAccessToken(ctx, client.ID, identityID, scopes)
+ if err != nil {
+ return nil, fmt.Errorf("cannot create access token: %w", err)
</file context>
| return nil, fmt.Errorf("%w: invalid device code", ErrInvalidGrant) | ||
| } | ||
|
|
||
| if time.Now().After(dc.ExpiresAt) { |
There was a problem hiding this comment.
P1: Race condition: status check and token issuance happen outside the transaction that locks the device code row. Two concurrent polls for an authorized device code will both pass the status check and issue duplicate token sets. Move the status/expiry checks and device code deletion (or a status transition to a "consumed" state) inside the WithTx block so that only one poller can issue tokens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/iam/oauth2server/service.go, line 684:
<comment>Race condition: status check and token issuance happen outside the transaction that locks the device code row. Two concurrent polls for an authorized device code will both pass the status check and issue duplicate token sets. Move the status/expiry checks and device code deletion (or a status transition to a "consumed" state) inside the `WithTx` block so that only one poller can issue tokens.</comment>
<file context>
@@ -0,0 +1,1126 @@
+ return nil, fmt.Errorf("%w: invalid device code", ErrInvalidGrant)
+ }
+
+ if time.Now().After(dc.ExpiresAt) {
+ return nil, ErrExpiredToken
+ }
</file context>
| panic(fmt.Sprintf("unsupported order by: %s", orderBy)) | ||
| } | ||
|
|
||
| func (c *OAuth2Consent) LoadByIdentityAndClient( |
There was a problem hiding this comment.
P1: Coredata methods here omit the Scoper/tenant_id filter required for tenant isolation. This allows cross-tenant reads/writes of OAuth2 consent records.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/coredata/oauth2_consent.go, line 53:
<comment>Coredata methods here omit the Scoper/tenant_id filter required for tenant isolation. This allows cross-tenant reads/writes of OAuth2 consent records.</comment>
<file context>
@@ -0,0 +1,249 @@
+ panic(fmt.Sprintf("unsupported order by: %s", orderBy))
+}
+
+func (c *OAuth2Consent) LoadByIdentityAndClient(
+ ctx context.Context,
+ conn pg.Conn,
</file context>
| svc.OAuth2ServerService = oauth2server.NewService( | ||
| pgClient, | ||
| oauth2server.Config{ | ||
| SigningKey: cfg.OAuth2ServerSigningKey, |
There was a problem hiding this comment.
P2: Validate OAuth2ServerSigningKey before constructing the OAuth2 server service; it is dereferenced in JWKS/ID token signing and will panic if nil.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/iam/service.go, line 188:
<comment>Validate OAuth2ServerSigningKey before constructing the OAuth2 server service; it is dereferenced in JWKS/ID token signing and will panic if nil.</comment>
<file context>
@@ -177,6 +182,16 @@ func NewService(
+ svc.OAuth2ServerService = oauth2server.NewService(
+ pgClient,
+ oauth2server.Config{
+ SigningKey: cfg.OAuth2ServerSigningKey,
+ SigningKID: cfg.OAuth2ServerSigningKID,
+ BaseURL: cfg.BaseURL.String(),
</file context>
| code[i] = userCodeAlphabet[n.Int64()] | ||
| } | ||
|
|
||
| return string(code), nil |
There was a problem hiding this comment.
P2: GenerateUserCode returns an unformatted 8‑character code despite the comment promising XXXX-XXXX. Either format the return value or adjust the docs; as written, callers expecting the documented format will render the code incorrectly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/iam/oauth2server/user_code.go, line 39:
<comment>GenerateUserCode returns an unformatted 8‑character code despite the comment promising XXXX-XXXX. Either format the return value or adjust the docs; as written, callers expecting the documented format will render the code incorrectly.</comment>
<file context>
@@ -0,0 +1,49 @@
+ code[i] = userCodeAlphabet[n.Int64()]
+ }
+
+ return string(code), nil
+}
+
</file context>
Each scope parsed from the space-delimited string is now validated via OAuth2Scope.UnmarshalText, rejecting invalid scope values instead of accepting them silently. Signed-off-by: Bryan Frimin <bryan@getprobo.com>
1 participant
Summary
oauth2serverservice package, HTTP handlers with CSRF bypasses for token endpoints, OIDC discovery (/.well-known/openid-configuration), and JWKS publishingTest plan
/.well-known/openid-configurationSummary by cubic
Adds a first-party OAuth2.0 and OpenID Connect 1.0 authorization server, wired into IAM and the HTTP API. Supports auth code with PKCE, device code, refresh token rotation, token introspection/revocation, OIDC discovery, and JWKS publishing; also adds strict scope parsing to reject invalid scopes.
New Features
oauth2serverservice with RSA-signed JWTs (configurable signing key/KID)./connect/v1/oauth2:/token,/device,/userinfo,/introspect,/revoke,/jwks; OIDC discovery at/.well-known/openid-configuration.coredatamodels and DB schema for clients, codes, tokens, device codes, and consents.OAuth2Scopes.UnmarshalTextvalidates each scope and rejects invalid values.Migration
auth.oauth2-server(signing-key-file, optionalsigning-kid), then restart./.well-known/openid-configurationand/connect/v1/oauth2/jwksreturn expected metadata/keys.Written for commit e2dd212. Summary will update on new commits.