Add OAuth2/OpenID Connect authorization server#957
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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/errors.go">
<violation number="1" location="pkg/iam/oauth2server/errors.go:33">
P1: `ErrInvalidRedirectURI` is not mapped in `OAuth2ErrorCode`, so it will be reported as `server_error` instead of a client error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
pkg/probod/auth_config.go
Outdated
| SigningKeyFile string `json:"signing-key-file"` | ||
| SigningKID string `json:"signing-kid"` |
There was a problem hiding this comment.
should be an array no, so we can rotate them if needed?
There was a problem hiding this comment.
2 issues found across 13 files (changes from recent commits).
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/server/api/connect/v1/oauth2_handler.go">
<violation number="1" location="pkg/server/api/connect/v1/oauth2_handler.go:223">
P1: Nil pointer dereference: `session.ID` is accessed without a nil check, but `session` can be nil. The code already nil-checks `session` for `authTime` but not for `SessionID`. If this handler is ever reached via a non-session auth path (e.g., API key), this will panic.</violation>
</file>
<file name="pkg/coredata/oauth2_consent.go">
<violation number="1" location="pkg/coredata/oauth2_consent.go:132">
P2: `CountByIdentityID` is missing the `AND approved = TRUE` filter that was added to `LoadByIdentityID`. The count will include non-approved consents that the load query excludes, producing incorrect pagination totals.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| r.Context(), | ||
| &oauth2server.AuthorizeRequest{ | ||
| IdentityID: identity.ID, | ||
| SessionID: session.ID, |
There was a problem hiding this comment.
P1: Nil pointer dereference: session.ID is accessed without a nil check, but session can be nil. The code already nil-checks session for authTime but not for SessionID. If this handler is ever reached via a non-session auth path (e.g., API key), this will panic.
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 223:
<comment>Nil pointer dereference: `session.ID` is accessed without a nil check, but `session` can be nil. The code already nil-checks `session` for `authTime` but not for `SessionID`. If this handler is ever reached via a non-session auth path (e.g., API key), this will panic.</comment>
<file context>
@@ -244,42 +218,44 @@ func (h *OAuth2Handler) AuthorizeHandler(w http.ResponseWriter, r *http.Request)
- client,
&oauth2server.AuthorizeRequest{
IdentityID: identity.ID,
+ SessionID: session.ID,
ResponseType: q.Get("response_type"),
ClientID: clientID,
</file context>
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
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:407">
P1: Directly indexing `s.signingKeys[0]` can panic when no signing keys are configured; validate non-empty keys before use.</violation>
</file>
<file name="pkg/probod/probod.go">
<violation number="1" location="pkg/probod/probod.go:441">
P2: Validate that at least one OAuth2 signing key is configured before building the IAM service; otherwise OpenID token signing will panic on `s.signingKeys[0]`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
pkg/iam/oauth2server/service.go
Outdated
| Scopes: scopes, | ||
| AccessTokenID: accessTokenID, | ||
| CreatedAt: now, | ||
| ExpiresAt: now.Add(30 * 24 * time.Hour), |
There was a problem hiding this comment.
should be service configuration.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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:113">
P1: Handle the case where no signing keys are marked active; otherwise signingKey() will panic at runtime when len(activeSigningIdx)==0.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
pkg/iam/oauth2server/service.go
Outdated
| for i, k := range cfg.SigningKeys { | ||
| if k.Active { | ||
| activeIdx = append(activeIdx, i) | ||
| } | ||
| } |
There was a problem hiding this comment.
P1: Handle the case where no signing keys are marked active; otherwise signingKey() will panic at runtime when len(activeSigningIdx)==0.
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 113:
<comment>Handle the case where no signing keys are marked active; otherwise signingKey() will panic at runtime when len(activeSigningIdx)==0.</comment>
<file context>
@@ -106,18 +109,33 @@ type (
func NewService(pgClient *pg.Client, cfg Config) *Service {
+ var activeIdx []int
+ for i, k := range cfg.SigningKeys {
+ if k.Active {
+ activeIdx = append(activeIdx, i)
</file context>
| for i, k := range cfg.SigningKeys { | |
| if k.Active { | |
| activeIdx = append(activeIdx, i) | |
| } | |
| } | |
| var activeIdx []int | |
| for i, k := range cfg.SigningKeys { | |
| if k.Active { | |
| activeIdx = append(activeIdx, i) | |
| } | |
| } | |
| if len(activeIdx) == 0 && len(cfg.SigningKeys) > 0 { | |
| activeIdx = append(activeIdx, 0) | |
| } |
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
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/server/api/connect/v1/oauth2_error.go">
<violation number="1" location="pkg/server/api/connect/v1/oauth2_error.go:86">
P2: `server_error` is missing from the redirectable errors list. RFC 6749 §4.1.2.1 explicitly defines `server_error` as a redirectable authorization endpoint error, noting it exists precisely because a 500 cannot be returned via redirect. Without it, authorization-endpoint server errors render as JSON instead of redirecting back to the client.</violation>
</file>
<file name="pkg/server/api/connect/v1/oauth2_handler.go">
<violation number="1" location="pkg/server/api/connect/v1/oauth2_handler.go:102">
P2: Invalid client responses no longer include a `WWW-Authenticate` challenge header when Basic client auth fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
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:675">
P1: Avoid passing raw internal errors to `ErrServerError.WithDescription`; it leaks internal details via `error_description`.</violation>
<violation number="2" location="pkg/iam/oauth2server/service.go:1156">
P1: Do not expose `err.Error()` in OAuth2 server errors; return a sanitized server_error description instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/server/api/connect/v1/oauth2_handler.go">
<violation number="1" location="pkg/server/api/connect/v1/oauth2_handler.go:320">
P2: Handle `DecodeForm` errors in `DeviceVerifySubmit`; the current code swallows invalid form data and continues with an empty `user_code`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
gearnode
force-pushed
the
gearnode/oauth2-openid-server
branch
from
06999a6 to
898cb63
Compare
| } | ||
| 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 7 days ago
In general, to fix open redirect issues you must not redirect directly to user-controlled URLs. Instead, either (a) restrict redirects to a known-safe set (e.g., registered client redirect URIs) or (b) enforce that the redirect target is local (same host) and relative. Since we cannot see or change how client redirect URIs are stored or validated in other parts of the system, the most robust, self-contained fix here is to ensure that redirectWithError only redirects to URLs that are local (no scheme/host) and do not perform an external redirect.
The best targeted fix in this code is to add validation in redirectWithError before calling http.Redirect. We can normalize the redirectURI string to mitigate backslash ambiguities, parse it, and then ensure that u.Scheme and u.Host are empty (so it’s a relative URL) and that it doesn’t contain suspicious patterns such as protocol-relative URLs (//example.com) in the path. If the URL fails validation, instead of redirecting we should fall back to rendering a generic OAuth2 error response (or at least a non-redirect HTTP error) to avoid sending the user to an attacker-provided URL.
Concretely:
- In
pkg/server/api/connect/v1/oauth2_error.go, insideredirectWithError:- Before
url.Parse, replace backslashes with forward slashes inredirectURI. - Parse the URL into
u. - After parsing and before modifying the query, check that
u.Scheme == ""andu.Host == "". - Optionally, ensure
u.Pathdoes not start with//(which could be interpreted as protocol-relative). - If validation fails, log or handle an internal error by rendering an error response (consistent with existing handling).
- Before
- This only requires importing
"strings"in that file; all other imports can remain unchanged. - No changes are required to the types in
types/oauth2.goor toAuthorizeHandler; they can continue passingredirectURIas-is, withredirectWithErroracting as the enforcement point.
| @@ -18,6 +18,7 @@ | ||
| "errors" | ||
| "net/http" | ||
| "net/url" | ||
| "strings" | ||
|
|
||
| "go.gearno.de/kit/httpserver" | ||
| "go.gearno.de/kit/log" | ||
| @@ -76,12 +77,24 @@ | ||
| } | ||
|
|
||
| func redirectWithError(w http.ResponseWriter, r *http.Request, redirectURI, state string, err error) { | ||
| u, parseErr := url.Parse(redirectURI) | ||
| // Normalize backslashes to forward slashes before parsing to avoid | ||
| // browser-dependent interpretation of the URL. | ||
| normalized := strings.ReplaceAll(redirectURI, "\\", "/") | ||
|
|
||
| u, parseErr := url.Parse(normalized) | ||
| if parseErr != nil { | ||
| httpserver.RenderError(w, http.StatusInternalServerError, errors.New("internal server error")) | ||
| return | ||
| } | ||
|
|
||
| // Ensure that we only redirect to a local (relative) URL. Reject any URL | ||
| // that specifies a scheme or host, or that starts with '//' which some | ||
| // browsers may treat as a protocol-relative URL. | ||
| if u.Scheme != "" || u.Host != "" || strings.HasPrefix(u.Path, "//") { | ||
| httpserver.RenderError(w, http.StatusBadRequest, errors.New("invalid redirect URI")) | ||
| return | ||
| } | ||
|
|
||
| oauthErr, ok := errors.AsType[*oauth2server.OAuth2Error](err) | ||
| if !ok { | ||
| httpserver.RenderError(w, http.StatusInternalServerError, errors.New("internal server error")) |
| } | ||
| 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 6 days ago
In general, to fix an open redirect you should not redirect directly to user-provided URLs. Instead, either (a) map user-provided identifiers to a server-maintained whitelist of redirect targets, or (b) constrain the redirect to same-origin / local paths (e.g., only relative URLs or URLs with your expected host and scheme), and reject any others.
For this codebase, the minimal change that preserves existing behavior while adding protection is to validate redirectURI inside redirectWithCode before calling http.Redirect. We can implement a small helper, isSafeRedirectURL, in oauth2_handler.go that:
- Replaces any backslashes in the candidate URL with forward slashes (to avoid browser backslash quirks).
- Parses the URL with
url.Parse. - Accepts the URL only if:
- It is relative (no scheme and no host), or
- It is absolute but same-origin as the current request (
scheme,host, andportequal tor.URL.Scheme/r.Hostor, more robustly, user.URLandr.Host).
If validation fails, instead of redirecting to the untrusted URL, we should respond with a safe error response. Since this function is used in the OAuth2 authorization flow, the safest backward-compatible behavior is to send a 400 Bad Request and a simple JSON error message (or potentially call an existing error renderer). However, to keep changes tightly scoped and avoid dependencies on other handlers, I will return a 400 with a short plaintext or JSON-style error from within redirectWithCode. This changes behavior only when the redirect URI is unsafe; legitimate, registered redirect URIs (which should be relative or same-origin, or the system can be configured accordingly) will not be affected.
Concretely:
- In
pkg/server/api/connect/v1/oauth2_handler.go:- Add a helper
isSafeRedirectURL(r *http.Request, rawURL string) (string, bool)that normalizes and validates the redirect URL, returning the cleaned URL string if it is acceptable. - Update
redirectWithCodeto:- Call
isSafeRedirectURL. - If invalid, send
http.Error(w, "invalid redirect_uri", http.StatusBadRequest)and return. - If valid, build the query (
code,state) on the parsed URL and redirect as before.
- Call
- Add a helper
No changes are required in pkg/server/api/connect/v1/types/oauth2.go; that file just copies the query parameter and doesn’t perform redirect logic.
| @@ -23,6 +23,7 @@ | ||
| "html/template" | ||
| "net/http" | ||
| "net/url" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "go.gearno.de/kit/httpserver" | ||
| @@ -655,8 +656,49 @@ | ||
| } | ||
| } | ||
|
|
||
| // isSafeRedirectURL validates a redirect URL to prevent open redirects. | ||
| // It allows relative URLs (no scheme/host) and absolute URLs that are | ||
| // same-origin with the current request. | ||
| func isSafeRedirectURL(r *http.Request, rawURL string) (string, bool) { | ||
| if rawURL == "" { | ||
| return "", false | ||
| } | ||
|
|
||
| // Normalize backslashes to forward slashes to avoid browser quirks. | ||
| rawURL = strings.ReplaceAll(rawURL, "\\", "/") | ||
|
|
||
| u, err := url.Parse(rawURL) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
|
|
||
| // Allow relative redirects (no scheme and no host). | ||
| if u.Scheme == "" && u.Host == "" { | ||
| return u.String(), true | ||
| } | ||
|
|
||
| // For absolute URLs, require same host as the current request. | ||
| // r.Host is the authority part (host[:port]). | ||
| if u.Host != "" && u.Host == r.Host { | ||
| return u.String(), true | ||
| } | ||
|
|
||
| return "", false | ||
| } | ||
|
|
||
| func redirectWithCode(w http.ResponseWriter, r *http.Request, redirectURI, code, state string) { | ||
| u, _ := url.Parse(redirectURI) | ||
| safeRedirect, ok := isSafeRedirectURL(r, redirectURI) | ||
| if !ok { | ||
| http.Error(w, "invalid redirect_uri", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| u, err := url.Parse(safeRedirect) | ||
| if err != nil { | ||
| http.Error(w, "invalid redirect_uri", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| q := u.Query() | ||
| q.Set("code", code) | ||
| if state != "" { |
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>
gearnode
force-pushed
the
gearnode/oauth2-openid-server
branch
from
898cb63 to
48b4b5c
Compare
Signed-off-by: Bryan Frimin <bryan@getprobo.com>
Signed-off-by: Bryan Frimin <bryan@getprobo.com>
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Signed-off-by: Bryan Frimin <bryan@getprobo.com>
Signed-off-by: Bryan Frimin <bryan@getprobo.com>
2 participants
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 OAuth 2.0 and OpenID Connect 1.0 authorization server integrated with IAM and the HTTP API. It issues RSA‑signed JWTs with round‑robin active‑key signing, publishes JWKS and discovery/metadata, supports authorization code + PKCE, device code (with Console activation UI and CLI login), and refresh rotation with replay detection.
New Features
oauth2serverwith RSA JWTs and active‑key rotation viapkg/crypto/jose; JWKS at/connect/v1/oauth2/jwks, OIDC discovery at/.well-known/openid-configuration, and OAuth 2.0 AS metadata at/.well-known/oauth-authorization-server./connect/v1/oauth2:token,device,userinfo,introspect,revoke; session‑basedauthorize+ consent UI. Consent persistsredirect_uri,code_challenge(+method),nonce,state,approved, andsession_id.authorizeDevicemutation; Probo CLI login now uses device flow with a pre‑provisioned client.pkg/uriand open‑redirect checks; scopes parsed in the HTTP layer and passed as typedcoredata.OAuth2Scopes.OAuth2Error; onlyserver_errorresponses are logged.token,introspect,revoke, anddevice.Migration
auth.oauth2-server.signing-keys(setkidandactive; at least one key must be active). Active keys sign tokens round‑robin; all keys appear in JWKS. Restart.auth.oauth2-server.access-token-duration,refresh-token-duration,authorization-code-duration, anddevice-code-duration./.well-known/openid-configuration,/.well-known/oauth-authorization-server, and/connect/v1/oauth2/jwksreturn the expected metadata and keys.Written for commit 8f218ab. Summary will update on new commits.