Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an embedded OpenID Connect (OIDC) provider with DB-backed storage/models, OIDC routes and login form, Bearer-token validation in middleware, htpasswd/Kratos credential integrations, a CLI OIDC login (PKCE) flow, scheduled OIDC cleanup, tests, CLI flags, asset handling, and dependency/tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Middleware as Auth Middleware
participant Validator as OIDC Validator
participant DB as Database
participant PersonDB as Person DB
Client->>Middleware: HTTP request with Authorization: Bearer <token>
Middleware->>Validator: authenticateOIDCToken(token)
Validator->>DB: loadOIDCPublicKeys() (cached)
DB-->>Validator: RSA public keys
Validator->>Validator: Parse & validate JWT (iss, aud, exp, etc.)
alt Valid JWT
Validator->>PersonDB: Lookup person by sub
PersonDB-->>Validator: Person record
Validator->>Middleware: Inject token & set user context
Middleware->>Client: forward to next handler
else Not valid / no key
Middleware->>Middleware: Fall back to access-token lookup or return 401/500
end
sequenceDiagram
participant CLI as CLI Command
participant Browser as Browser/User
participant Provider as OIDC Provider
participant LocalCB as Local Callback Server
participant TokenEP as Token Endpoint
CLI->>Provider: Discover /.well-known (get endpoints)
CLI->>CLI: Generate PKCE (verifier, challenge), state, nonce
CLI->>LocalCB: Start loopback server /callback
CLI->>Browser: Open authorization URL (client_id=mc-cli, code_challenge, state, nonce)
Browser->>Provider: GET /authorize
Provider->>Browser: Serve login form (/oidc/login)
Browser->>Provider: POST credentials
Provider->>Provider: Validate credentials, set auth_request subject
Provider->>Browser: Redirect to loopback with code & state
Browser->>LocalCB: GET /callback?code=...&state=...
LocalCB->>LocalCB: Validate state, return success page
LocalCB->>CLI: Send authorization code
CLI->>TokenEP: POST /oauth/token (code, verifier, redirect_uri)
TokenEP-->>CLI: access_token, refresh_token, id_token
CLI->>CLI: Validate ID token nonce, persist tokens locally
CLI->>User: Print success and token info
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
auth/basic.go (1)
110-129: Clarify the Bearer token authentication flow.The logic attempts OIDC authentication first, then falls through to access token lookup if OIDC doesn't authenticate. This is reasonable for supporting both OIDC JWTs and legacy access tokens using the same
Bearerscheme.However, consider adding a brief comment explaining this intentional fallback behavior for future maintainers.
📝 Suggested documentation
if token, ok := extractBearerAuthToken(c.Request().Header); ok { + // Try OIDC JWT first, then fall back to access token lookup. + // Both authentication methods use the Bearer scheme. if authenticated, err := authenticateOIDCToken(c, token); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/basic.go` around lines 110 - 129, Add a short inline comment explaining that the Bearer handling intentionally tries OIDC JWT authentication first via extractBearerAuthToken and authenticateOIDCToken and, if that fails without error, falls back to legacy access token lookup using getAccessToken and InjectToken to support both OIDC and legacy tokens; place the comment just above the if token, ok := extractBearerAuthToken(...) block or immediately before the authenticateOIDCToken call so future maintainers understand the deliberate fallback flow.auth/oidc/provider.go (2)
45-48: Document the security implications ofWithAllowInsecure().The
op.WithAllowInsecure()option allows non-HTTPS issuer URLs. While useful for development, this should be documented and potentially gated behind a flag for production deployments.📝 Suggested documentation
oidcProvider, err := op.NewProvider(config, storage, op.StaticIssuer(issuerURL), - op.WithAllowInsecure(), // allow http:// for dev + // WithAllowInsecure allows http:// issuer URLs for local development. + // In production, ensure issuerURL uses https:// for security. + op.WithAllowInsecure(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/provider.go` around lines 45 - 48, The call to op.NewProvider currently passes op.WithAllowInsecure(), which permits non-HTTPS issuer URLs (issuerURL) and poses production security risks; update documentation and the code to clearly state the implications of op.WithAllowInsecure() near the NewProvider instantiation, and gate its use behind an explicit configuration flag (e.g., a boolean in config or an environment variable like ALLOW_INSECURE_OIDC) so the default behavior is secure (flag defaults to false); update README/dev docs to describe that op.WithAllowInsecure() is strictly for local/dev use and must not be enabled in production, and add a short code comment by the NewProvider call referencing the secure default and the config flag.
60-68: Consider making the default key path configurable or absolute.The default key path
oidc-signing-key.pemis relative to the working directory. In containerized deployments, this may result in the key being written to unexpected locations or failing if the directory is read-only.Consider:
- Using an absolute path under a well-known directory (e.g.,
/var/lib/mission-control/)- Failing explicitly if no path is provided and auto-generation is not desired in production
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/provider.go` around lines 60 - 68, The function loadOrGenerateKey currently falls back to the relative filename "oidc-signing-key.pem"; change it to use a configurable absolute default and safer behavior: read a default absolute path like "/var/lib/mission-control/oidc-signing-key.pem" when keyPath is empty (or derive it from a config/env var), ensure the directory exists (create with proper permissions) before writing, and if running in a production mode flag or if a new env/config (e.g., DISABLE_KEY_AUTO_GENERATION) is set then return an explicit error instead of auto-generating; update references to keyPath handling in loadOrGenerateKey to use the config/env variable and the new failure path.cmd/auth_login.go (2)
274-282: Missing Windows browser support.The
openBrowserfunction handles Darwin and Linux (viaxdg-open) but silently falls through for Windows. Consider adding Windows support withcmd /c start.♻️ Suggested fix
func openBrowser(url string) { - var cmd string + var cmd string + var args []string switch runtime.GOOS { case "darwin": cmd = "open" + args = []string{url} + case "windows": + cmd = "cmd" + args = []string{"/c", "start", url} default: cmd = "xdg-open" + args = []string{url} } - _ = exec.Command(cmd, url).Start() + _ = exec.Command(cmd, args...).Start() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_login.go` around lines 274 - 282, The openBrowser function currently only handles "darwin" and defaults to "xdg-open" for other OSes; add explicit Windows support by handling runtime.GOOS == "windows" in openBrowser and invoking the Windows shell with cmd, "/c", "start" plus the URL as an argument via exec.Command (e.g., exec.Command("cmd", "/c", "start", url).Start()), and propagate or log the returned error instead of ignoring it to catch failures when launching the browser.
189-193: Error fromrand.Readis silently ignored.While
rand.Readrarely fails, silently ignoring the error for security-sensitive randomness (state parameter) is not ideal. Consider propagating the error or at minimum panicking on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_login.go` around lines 189 - 193, The function randomBase64 currently ignores the error from rand.Read; change it to handle and propagate the error by updating its signature to return (string, error) (e.g., func randomBase64(n int) (string, error)), check the error from rand.Read and return "", err on failure, and update all callers to handle the error (or, if callers expect a fatal failure, document and use panic on rand.Read error instead); ensure the function still encodes b with base64.RawURLEncoding.EncodeToString when no error occurs.auth/oidc/storage.go (2)
275-289: Duplicate function:parseRSAPublicKeyis nearly identical toparseRSAPublicKeyPEMinoidc_validate.go.Both functions decode a PEM block and parse a PKIX public key. Consider extracting this to a shared utility to avoid duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/storage.go` around lines 275 - 289, The parseRSAPublicKey function duplicates logic already present in parseRSAPublicKeyPEM; refactor by extracting the common PEM-to-*rsa.PublicKey parsing into a single helper (e.g., parsePKIXRSAPublicKey or ParseRSAPublicKeyFromPEM) and replace both parseRSAPublicKey and parseRSAPublicKeyPEM to call this new utility; ensure the helper performs pem.Decode, x509.ParsePKIXPublicKey, type asserts to *rsa.PublicKey and returns clear errors so callers (parseRSAPublicKey and parseRSAPublicKeyPEM) can be removed or become simple wrappers delegating to the new function.
291-301:generateKeyIDmay produce non-deterministic output.Using
json.Marshalon*rsa.PublicKeyrelies on the struct's JSON marshaling behavior, which may not be stable across Go versions or produce consistent output. Consider using a canonical representation like the DER-encoded public key bytes.♻️ Suggested fix using DER encoding
func generateKeyID(pub *rsa.PublicKey) (string, error) { - b, err := json.Marshal(pub) + b, err := x509.MarshalPKIXPublicKey(pub) if err != nil { - // fallback return uuid.New().String(), nil } h := crypto.SHA256.New() h.Write(b) return fmt.Sprintf("%x", h.Sum(nil))[:16], nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/storage.go` around lines 291 - 301, The current generateKeyID uses json.Marshal on *rsa.PublicKey which can be non-deterministic; replace the JSON step with a canonical DER encoding: call x509.MarshalPKIXPublicKey(pub) to get canonical bytes, handle any error from that call (return an error instead of silently falling back), then compute SHA256 over the DER bytes and return the first 16 hex chars as the ID; update imports to include crypto/x509 and adjust error handling in generateKeyID accordingly so the function remains deterministic.auth/oidc_validate.go (1)
33-36: Errors from key loading are silently swallowed.When
loadOIDCPublicKeysreturns an error, the function returns(false, nil), hiding potential database or parsing issues. Consider logging the error or distinguishing between "no keys available" and "failed to load keys."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc_validate.go` around lines 33 - 36, The code currently swallows errors from loadOIDCPublicKeys; change the block in the function that calls loadOIDCPublicKeys so that errors are propagated instead of returning (false, nil): if loadOIDCPublicKeys(ctx) returns an error, return (false, err) (or log and return the error) and only treat an empty keys slice as the benign case (false, nil); update the call site around loadOIDCPublicKeys to return the error when err != nil and keep the existing behavior only when err == nil but len(keys) == 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@auth/middleware.go`:
- Around line 80-82: The OIDC check inside the case Basic branch is unreachable
because within case Basic vars.AuthMode is already Basic; move the guard out of
the switch: remove the current if (OIDCEnabled && (vars.AuthMode == Kratos ||
vars.AuthMode == Clerk)) block from the case Basic branch and add a pre-switch
check that runs before the switch, e.g. if OIDCEnabled && (vars.AuthMode ==
Kratos || vars.AuthMode == Clerk) return fmt.Errorf("--oidc is only supported
with --auth basic"), so the code validates OIDCEnabled against vars.AuthMode for
Kratos/Clerk modes before entering the switch.
In `@auth/oidc_validate.go`:
- Line 31: The direct type assertion ctx :=
c.Request().Context().(context.Context) can panic if the request context isn't
the expected type; change it to a safe assertion such as ctx, ok :=
c.Request().Context().(context.Context); if !ok { ctx = context.Background() }
(or call a provided helper to extract a duty/context.Context) and then use that
ctx variable—update the usage at the same site (the expression
c.Request().Context()) to use the safe-checked ctx instead of the unchecked
assertion.
- Around line 66-79: The loop over claims["aud"] uses an unsafe type assertion
a.(string) that can panic; update the iteration in the audience-checking block
(where claims["aud"] is inspected and compared to oidcmodels.ClientID) to first
type-assert each element with a safe check (e.g., s, ok := a.(string)) and only
compare s to oidcmodels.ClientID when ok is true, skipping non-string elements
to avoid panics and preserve the existing found/continue logic.
In `@auth/oidc/login.go`:
- Around line 53-59: The auth_request_id is reflected into HTML unescaped in
ShowForm and HandleSubmit (used with loginFormHTML) creating a reflected XSS;
fix by escaping the id before interpolating (e.g., call html.EscapeString on the
id or switch rendering to a templating engine with auto-escaping) and ensure
both ShowForm and HandleSubmit use the escaped value when formatting
loginFormHTML so no raw query/form value is inserted into the response.
- Line 62: Replace the unsafe type assertion on the request context in login.go
(ctx := c.Request().Context().(context.Context)) with a safe comma-ok assertion
against the expected duty/context type (e.g., dutycontext.Context) and provide a
fallback to the standard context.Context when the assertion fails; specifically,
change to something like "if dutyCtx, ok :=
c.Request().Context().(dutycontext.Context); ok { ctx = dutyCtx } else { ctx =
c.Request().Context() }" (use the actual duty context type name used elsewhere,
and ensure the variable name ctx and any downstream usages remain consistent).
In `@auth/oidc/models.go`:
- Around line 18-41: Replace the fragile custom StringList Value() and Scan()
implementations with pq.StringArray-backed behavior: change the StringList type
to alias pq.StringArray and update the StringList.Value and (*StringList).Scan
methods to delegate to pq.StringArray's Value and Scan (use
pq.StringArray(s).Value() and (*pq.StringArray)(s).Scan(src)); ensure you import
github.com/lib/pq and remove the manual join/trim logic from StringList.Value
and StringList.Scan while keeping the same method names so existing callers of
StringList, StringList.Value, and (*StringList).Scan continue to work.
In `@auth/oidc/provider.go`:
- Line 111: Replace the direct call to time.Now() used for the database
timestamp with duty.Now() to follow the project convention for DB
timestamps/soft deletes; locate the record construction that sets CreatedAt (the
line with CreatedAt: time.Now()) in provider.go and change it to use duty.Now()
instead, ensuring the duty package is imported if not already.
- Around line 132-137: The aesKeyFromIssuer function currently copies the issuer
bytes directly into a 32-byte array which yields predictable/weak keys for short
issuers; replace this with a proper KDF (e.g., SHA-256 or HKDF-SHA256) to
deterministically derive a full 32-byte key from the issuer string. Locate
aesKeyFromIssuer and compute a 32-byte digest (sha256.Sum256(issuer)) or run
issuer through crypto/hkdf with an application-specific salt/info to produce 32
bytes, then return that as the [32]byte key to ensure uniform entropy regardless
of issuer length.
In `@auth/oidc/storage.go`:
- Around line 68-69: Replace all uses of time.Now() for DB timestamps with
duty.Now() to follow guidelines: update CreatedAt and ExpiresAt in
CreateAuthRequest, replace expiry/now calculations in CreateAccessToken and
CreateAccessAndRefreshTokens, update token expiration update logic, and switch
timestamp fields on RefreshToken creation/updates and the now value in
SetAuthRequestSubject to duty.Now(); locate these spots by the function names
CreateAuthRequest, CreateAccessToken, CreateAccessAndRefreshTokens, the token
expiration update block, RefreshToken handling, and SetAuthRequestSubject and
swap time.Now() -> duty.Now() so tests and soft-deletes use the duty clock.
In `@cmd/auth_login.go`:
- Around line 164-176: The discoverOIDC function uses http.Get with the default
client that has no timeout; replace that call by creating an http.Client with a
reasonable Timeout (e.g., a few seconds), use client.Get(discoveryURL), and keep
the existing response body handling (defer resp.Body.Close()) and JSON decode
logic; update error handling to return errors as before. Ensure the change is
applied inside discoverOIDC so the timeout-targeted client is used instead of
http.Get.
- Around line 23-41: The file defines a duplicate AuthCmd and adds it to Root
causing a conflict; remove the AuthCmd variable and the Root.AddCommand(AuthCmd)
call, and instead attach authLoginCmd (with its flags like loginServer and
MarkFlagRequired) to the existing Auth command declared in cmd/token.go by
referencing Auth and calling Auth.AddCommand(authLoginCmd) in init; ensure the
package imports/reference makes Auth visible so only authLoginCmd is registered
under the single Auth command.
- Around line 195-212: The exchangeCode function uses http.PostForm (default
client without timeout); replace that call with an explicit http.Client having a
timeout (e.g., time.Second * N) and use client.PostForm or client.Do with the
same form to perform the token exchange, ensuring the response body is closed
and errors propagated as before; update exchangeCode to create the client, build
the request if needed, and call client.PostForm (or client.Do) instead of
http.PostForm to add the timeout protection.
In `@jobs/jobs.go`:
- Line 114: Fix the typo in the error log string used in jobs.go: update the
logger.Errorf call (the one logging "failed to start notificatino status update
queue: %v") to spell "notification" correctly; locate the logger.Errorf
invocation that reports failure to start the notification status update queue
and change the message text to "failed to start notification status update
queue: %v".
---
Nitpick comments:
In `@auth/basic.go`:
- Around line 110-129: Add a short inline comment explaining that the Bearer
handling intentionally tries OIDC JWT authentication first via
extractBearerAuthToken and authenticateOIDCToken and, if that fails without
error, falls back to legacy access token lookup using getAccessToken and
InjectToken to support both OIDC and legacy tokens; place the comment just above
the if token, ok := extractBearerAuthToken(...) block or immediately before the
authenticateOIDCToken call so future maintainers understand the deliberate
fallback flow.
In `@auth/oidc_validate.go`:
- Around line 33-36: The code currently swallows errors from loadOIDCPublicKeys;
change the block in the function that calls loadOIDCPublicKeys so that errors
are propagated instead of returning (false, nil): if loadOIDCPublicKeys(ctx)
returns an error, return (false, err) (or log and return the error) and only
treat an empty keys slice as the benign case (false, nil); update the call site
around loadOIDCPublicKeys to return the error when err != nil and keep the
existing behavior only when err == nil but len(keys) == 0.
In `@auth/oidc/provider.go`:
- Around line 45-48: The call to op.NewProvider currently passes
op.WithAllowInsecure(), which permits non-HTTPS issuer URLs (issuerURL) and
poses production security risks; update documentation and the code to clearly
state the implications of op.WithAllowInsecure() near the NewProvider
instantiation, and gate its use behind an explicit configuration flag (e.g., a
boolean in config or an environment variable like ALLOW_INSECURE_OIDC) so the
default behavior is secure (flag defaults to false); update README/dev docs to
describe that op.WithAllowInsecure() is strictly for local/dev use and must not
be enabled in production, and add a short code comment by the NewProvider call
referencing the secure default and the config flag.
- Around line 60-68: The function loadOrGenerateKey currently falls back to the
relative filename "oidc-signing-key.pem"; change it to use a configurable
absolute default and safer behavior: read a default absolute path like
"/var/lib/mission-control/oidc-signing-key.pem" when keyPath is empty (or derive
it from a config/env var), ensure the directory exists (create with proper
permissions) before writing, and if running in a production mode flag or if a
new env/config (e.g., DISABLE_KEY_AUTO_GENERATION) is set then return an
explicit error instead of auto-generating; update references to keyPath handling
in loadOrGenerateKey to use the config/env variable and the new failure path.
In `@auth/oidc/storage.go`:
- Around line 275-289: The parseRSAPublicKey function duplicates logic already
present in parseRSAPublicKeyPEM; refactor by extracting the common
PEM-to-*rsa.PublicKey parsing into a single helper (e.g., parsePKIXRSAPublicKey
or ParseRSAPublicKeyFromPEM) and replace both parseRSAPublicKey and
parseRSAPublicKeyPEM to call this new utility; ensure the helper performs
pem.Decode, x509.ParsePKIXPublicKey, type asserts to *rsa.PublicKey and returns
clear errors so callers (parseRSAPublicKey and parseRSAPublicKeyPEM) can be
removed or become simple wrappers delegating to the new function.
- Around line 291-301: The current generateKeyID uses json.Marshal on
*rsa.PublicKey which can be non-deterministic; replace the JSON step with a
canonical DER encoding: call x509.MarshalPKIXPublicKey(pub) to get canonical
bytes, handle any error from that call (return an error instead of silently
falling back), then compute SHA256 over the DER bytes and return the first 16
hex chars as the ID; update imports to include crypto/x509 and adjust error
handling in generateKeyID accordingly so the function remains deterministic.
In `@cmd/auth_login.go`:
- Around line 274-282: The openBrowser function currently only handles "darwin"
and defaults to "xdg-open" for other OSes; add explicit Windows support by
handling runtime.GOOS == "windows" in openBrowser and invoking the Windows shell
with cmd, "/c", "start" plus the URL as an argument via exec.Command (e.g.,
exec.Command("cmd", "/c", "start", url).Start()), and propagate or log the
returned error instead of ignoring it to catch failures when launching the
browser.
- Around line 189-193: The function randomBase64 currently ignores the error
from rand.Read; change it to handle and propagate the error by updating its
signature to return (string, error) (e.g., func randomBase64(n int) (string,
error)), check the error from rand.Read and return "", err on failure, and
update all callers to handle the error (or, if callers expect a fatal failure,
document and use panic on rand.Read error instead); ensure the function still
encodes b with base64.RawURLEncoding.EncodeToString when no error occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 293eb264-89fe-48a2-b4d3-2ed83466c947
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
auth/basic.goauth/middleware.goauth/oidc/cleanup.goauth/oidc/login.goauth/oidc/models.goauth/oidc/provider.goauth/oidc/routes.goauth/oidc/storage.goauth/oidc_test.goauth/oidc_validate.gocmd/auth_login.gocmd/root.gogo.modjobs/jobs.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
auth/oidc/login.go (3)
68-70:⚠️ Potential issue | 🔴 CriticalXSS vulnerability in
renderFormwith unescapedid.The
idform value is interpolated into HTML without escaping. Apply the samehtml.EscapeString()fix here.🔒 Proposed fix
renderForm := func(msg string) error { - return c.HTML(http.StatusOK, fmt.Sprintf(loginFormHTML, id, "<p style='color:red'>"+msg+"</p>")) + return c.HTML(http.StatusOK, fmt.Sprintf(loginFormHTML, html.EscapeString(id), "<p style='color:red'>"+msg+"</p>")) },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/login.go` around lines 68 - 70, The renderForm closure interpolates the unescaped variable id into loginFormHTML causing an XSS risk; update renderForm (used where loginFormHTML and id are referenced) to pass html.EscapeString(id) instead of id so the id value is properly escaped before formatting into the HTML response.
53-59:⚠️ Potential issue | 🔴 CriticalReflected XSS vulnerability via
auth_request_id.The
idparameter from the query string is directly interpolated into HTML without escaping. An attacker could craft a URL with malicious JavaScript in theauth_request_idparameter. Usehtml.EscapeString()to sanitize the value.🔒 Proposed fix
+import "html" + func (h *LoginHandler) ShowForm(c echo.Context) error { id := c.QueryParam("auth_request_id") if id == "" { return c.String(http.StatusBadRequest, "missing auth_request_id") } - return c.HTML(http.StatusOK, fmt.Sprintf(loginFormHTML, id, "")) + return c.HTML(http.StatusOK, fmt.Sprintf(loginFormHTML, html.EscapeString(id), "")) },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/login.go` around lines 53 - 59, The ShowForm handler currently inserts the raw query param id into HTML (via loginFormHTML) causing reflected XSS; sanitize id by applying html.EscapeString(c.QueryParam("auth_request_id")) before including it in fmt.Sprintf so the rendered HTML uses the escaped value (keep using loginFormHTML and the same fmt.Sprintf call but pass the escaped id).
62-62:⚠️ Potential issue | 🟡 MinorUnsafe context type assertion may panic.
If the request context is not a
duty/context.Context, this will panic. Use a comma-ok assertion with a fallback.🛡️ Proposed fix
- ctx := c.Request().Context().(context.Context) + ctx, ok := c.Request().Context().(context.Context) + if !ok { + return c.String(http.StatusInternalServerError, "internal error") + },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/login.go` at line 62, The direct type assertion ctx := c.Request().Context().(context.Context) can panic; change it to a safe comma-ok assertion and provide a fallback: do ctx, ok := c.Request().Context().(context.Context); if !ok { ctx = context.Background() } (or use c.Request().Context() directly if appropriate). Update the code in auth/oidc/login.go where ctx is created so it uses the comma-ok pattern to avoid a runtime panic.
🧹 Nitpick comments (3)
auth/oidc_test.go (1)
485-527: Inline DDL for test tables is acceptable for now.The comment indicates these tables will be in duty's migrations in production. Consider adding a TODO with a tracking reference to ensure this is cleaned up once the schema dependency (flanksource/duty#1845) is merged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc_test.go` around lines 485 - 527, The inline DDL in ensureOIDCTables currently exists only for tests but should be marked for future removal once the schema migration is merged; add a clear TODO comment near the ensureOIDCTables function (or above the ddl variable) that mentions tracking issue flanksource/duty#1845 and states that these tables will be removed in favor of duty migrations, so reviewers and future maintainers know to delete this inline schema when the migration lands.auth/kratos.go (1)
238-241: Consider passing request context instead ofgocontext.Background().Using
gocontext.Background()loses the request's tracing span and cancellation signal. If the caller has access to a request context, it would be preferable to accept it as a parameter or capture it when constructing the checker.That said, since
kratosLoginWithCacheprimarily uses the context for the Kratos API call and includes its own caching layer, the practical impact is limited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/kratos.go` around lines 238 - 241, The Match method currently calls kratosLoginWithCache with gocontext.Background(), losing request tracing and cancellation; change Match to accept a context (e.g., func (k *KratosCredentialChecker) Match(ctx context.Context, user, pass string) bool) and pass that ctx into k.middleware.kratosLoginWithCache(ctx, user, pass), or alternatively store a request-scoped context on KratosCredentialChecker at construction and use that instead; update all callers of KratosCredentialChecker.Match accordingly so tracing/cancellation and deadlines propagate to kratosLoginWithCache.auth/basic.go (1)
110-129: Silent error handling in access token fallback path may hide issues.When OIDC authentication returns
(false, nil), the code falls through to the access token check. However, errors fromgetAccessTokenand the DB lookup are silently swallowed—if either fails, the request gets a generic 401 rather than surfacing whether there was a transient database error vs. an actually invalid token.Consider logging errors before falling through to the 401 response:
🔧 Suggested improvement
// fall through to access token check - if accessToken, err := getAccessToken(c.Request().Context().(context.Context), token); err == nil && accessToken != nil { + ctx := c.Request().Context().(context.Context) + accessToken, err := getAccessToken(ctx, token) + if err != nil { + ctx.GetLogger().V(4).Infof("access token lookup failed: %v", err) + } + if accessToken != nil { - ctx := c.Request().Context().(context.Context) var person models.Person if err := ctx.DB().Where("id = ?", accessToken.PersonID).First(&person).Error; err == nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/basic.go` around lines 110 - 129, The access-token fallback path silently swallows errors from getAccessToken and the DB lookup, making it impossible to distinguish transient failures from invalid tokens; update the bearer-auth block (around extractBearerAuthToken, authenticateOIDCToken, getAccessToken, and the DB lookup using ctx.DB().Where(...).First) to log returned errors and return an appropriate 500 when getAccessToken or the DB query returns a non-nil error (while still returning 401 for a genuinely missing/invalid access token); also include contextual info (token or person id) in the logs and ensure InjectToken behavior is unchanged when no errors occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@auth/oidc/login.go`:
- Around line 68-70: The renderForm closure interpolates the unescaped variable
id into loginFormHTML causing an XSS risk; update renderForm (used where
loginFormHTML and id are referenced) to pass html.EscapeString(id) instead of id
so the id value is properly escaped before formatting into the HTML response.
- Around line 53-59: The ShowForm handler currently inserts the raw query param
id into HTML (via loginFormHTML) causing reflected XSS; sanitize id by applying
html.EscapeString(c.QueryParam("auth_request_id")) before including it in
fmt.Sprintf so the rendered HTML uses the escaped value (keep using
loginFormHTML and the same fmt.Sprintf call but pass the escaped id).
- Line 62: The direct type assertion ctx :=
c.Request().Context().(context.Context) can panic; change it to a safe comma-ok
assertion and provide a fallback: do ctx, ok :=
c.Request().Context().(context.Context); if !ok { ctx = context.Background() }
(or use c.Request().Context() directly if appropriate). Update the code in
auth/oidc/login.go where ctx is created so it uses the comma-ok pattern to avoid
a runtime panic.
---
Nitpick comments:
In `@auth/basic.go`:
- Around line 110-129: The access-token fallback path silently swallows errors
from getAccessToken and the DB lookup, making it impossible to distinguish
transient failures from invalid tokens; update the bearer-auth block (around
extractBearerAuthToken, authenticateOIDCToken, getAccessToken, and the DB lookup
using ctx.DB().Where(...).First) to log returned errors and return an
appropriate 500 when getAccessToken or the DB query returns a non-nil error
(while still returning 401 for a genuinely missing/invalid access token); also
include contextual info (token or person id) in the logs and ensure InjectToken
behavior is unchanged when no errors occur.
In `@auth/kratos.go`:
- Around line 238-241: The Match method currently calls kratosLoginWithCache
with gocontext.Background(), losing request tracing and cancellation; change
Match to accept a context (e.g., func (k *KratosCredentialChecker) Match(ctx
context.Context, user, pass string) bool) and pass that ctx into
k.middleware.kratosLoginWithCache(ctx, user, pass), or alternatively store a
request-scoped context on KratosCredentialChecker at construction and use that
instead; update all callers of KratosCredentialChecker.Match accordingly so
tracing/cancellation and deadlines propagate to kratosLoginWithCache.
In `@auth/oidc_test.go`:
- Around line 485-527: The inline DDL in ensureOIDCTables currently exists only
for tests but should be marked for future removal once the schema migration is
merged; add a clear TODO comment near the ensureOIDCTables function (or above
the ddl variable) that mentions tracking issue flanksource/duty#1845 and states
that these tables will be removed in favor of duty migrations, so reviewers and
future maintainers know to delete this inline schema when the migration lands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 125bedb3-ae1f-400e-acdc-829f0edcd2a3
📒 Files selected for processing (7)
auth/basic.goauth/kratos.goauth/middleware.goauth/oidc/login.goauth/oidc/models.goauth/oidc/routes.goauth/oidc_test.go
✅ Files skipped from review due to trivial changes (1)
- auth/oidc/routes.go
🚧 Files skipped from review as they are similar to previous changes (1)
- auth/middleware.go
Member
This could just be because I'm running v1.0.0 Kratos server. Let me downgrade and tes. EditDowngrading doesn't work. But I can confirm that this issue is in fact caused by the drift in kratos server and client version. We use Bumping kratos to a much newer version directly fails due to migration errors. For me, |
Member
|
@codex[agent] review |
|
@adityathebe I've opened a new pull request, #2903, to work on those changes. Once the pull request is ready, I'll request review from you. |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
auth/oidc/login.go (1)
63-63:⚠️ Potential issue | 🟡 MinorUnsafe context type assertion may panic.
If the request context is not a
duty/context.Context, this line will panic. While this may be guaranteed by middleware, a safe assertion would be more defensive.🛡️ Proposed fix
- ctx := c.Request().Context().(context.Context) + ctx, ok := c.Request().Context().(context.Context) + if !ok { + return c.String(http.StatusInternalServerError, "internal error") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/login.go` at line 63, The current forced type assertion ctx := c.Request().Context().(context.Context) can panic; replace it with a safe approach by either using the context directly (ctx := c.Request().Context()) or performing a checked assertion (val, ok := c.Request().Context().(context.Context); if !ok { handle error/return }) so the login handler in login.go does not crash when extracting ctx from c.Request().Context().auth/oidc/models.go (1)
18-40:⚠️ Potential issue | 🟠 Major
StringListis still hand-parsing PostgreSQL arrays.
Scanonly acceptsstring, and the manual join/split logic still breaks quoted or comma-containing values. Please switch this to a proventext[]scanner/valuer instead of maintaining a custom codec here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/models.go` around lines 18 - 40, The custom PostgreSQL array parsing in StringList.Value and StringList.Scan is fragile; replace it by delegating to a proven text[] codec (e.g., github.com/lib/pq's pq.StringArray). Update StringList.Value to call pq.StringArray(s).Value() (handling nil/empty appropriately) and update StringList.Scan to call (&pq.StringArray{}).Scan(src) and then convert the pq.StringArray result back into your StringList (or make StringList a type alias of pq.StringArray). Also add the pq import and remove the manual join/split logic so quoted or comma-containing values are handled correctly.
🧹 Nitpick comments (5)
auth/oidc/provider.go (1)
46-49: Consider conditionally enablingWithAllowInsecure()based on issuer URL scheme.
WithAllowInsecure()is always enabled, which allowshttp://issuers. While useful for development, this could pose a security risk if accidentally deployed with an HTTP issuer URL in production.🛡️ Proposed fix
+import "strings" + func NewProvider(ctx context.Context, issuerURL, signingKeyPath string) (*Provider, error) { // ... key loading ... + opts := []op.Option{op.StaticIssuer(issuerURL)} + if strings.HasPrefix(issuerURL, "http://") { + logger.Warnf("OIDC: using insecure http:// issuer URL - not recommended for production") + opts = append(opts, op.WithAllowInsecure()) + } + oidcProvider, err := op.NewProvider(config, storage, - op.StaticIssuer(issuerURL), - op.WithAllowInsecure(), // allow http:// for dev + opts..., )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/provider.go` around lines 46 - 49, The provider is always created with op.WithAllowInsecure(), which permits http issuers; change NewProvider call in the oidc provider initialization to conditionally include op.WithAllowInsecure() only when the issuerURL uses the "http://" scheme (or when running in a recognized dev environment). Concretely, inspect issuerURL before calling op.NewProvider (the oidcProvider creation block) and build the options slice so op.WithAllowInsecure() is appended only if issuerURL has the "http://" prefix (or an explicit dev flag), leaving production HTTPS issuers to be created without WithAllowInsecure.auth/basic.go (1)
110-129: Complex nested logic could be simplified.The bearer token handling has deeply nested conditionals that are hard to follow. Consider extracting the access token fallback into a helper function.
♻️ Proposed refactor
if token, ok := extractBearerAuthToken(c.Request().Header); ok { if authenticated, err := authenticateOIDCToken(c, token); err != nil { return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal error"}) } else if authenticated { return next(c) } - // fall through to access token check - if accessToken, err := getAccessToken(c.Request().Context().(context.Context), token); err == nil && accessToken != nil { - ctx := c.Request().Context().(context.Context) - var person models.Person - if err := ctx.DB().Where("id = ?", accessToken.PersonID).First(&person).Error; err == nil { - if err := InjectToken(ctx, c, &person, ""); err == nil { - ctx = ctx.WithUser(&person) - c.SetRequest(c.Request().WithContext(ctx)) - return next(c) - } - } - } + // Fall through to access token check + if authenticated := authenticateAccessToken(c, token); authenticated { + return next(c) + } return c.JSON(http.StatusUnauthorized, map[string]string{"error": "unauthorized"}) }With a helper:
func authenticateAccessToken(c echo.Context, token string) bool { ctx, ok := c.Request().Context().(context.Context) if !ok { return false } accessToken, err := getAccessToken(ctx, token) if err != nil || accessToken == nil { return false } var person models.Person if err := ctx.DB().Where("id = ?", accessToken.PersonID).First(&person).Error; err != nil { return false } if err := InjectToken(ctx, c, &person, ""); err != nil { return false } ctx = ctx.WithUser(&person) c.SetRequest(c.Request().WithContext(ctx)) return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/basic.go` around lines 110 - 129, The bearer token handling in the middleware (inside the extractBearerAuthToken branch) is deeply nested; extract the access-token fallback into a helper (e.g., authenticateAccessToken) that accepts echo.Context and the token string and returns bool, moving the getAccessToken, DB lookup of models.Person, InjectToken call, ctx.WithUser and c.SetRequest logic into that helper and ensuring the context type assertion (c.Request().Context().(context.Context)) is checked safely; then simplify the original block to call authenticateOIDCToken first and, on false, call authenticateAccessToken and return next(c) if it returns true or a 401 otherwise.auth/kratos.go (1)
158-167: Consider logging OIDC authentication errors for debugging.When
authenticateOIDCTokenreturns an error, the middleware returns a generic 500 but doesn't log the actual error. This could make production debugging difficult.♻️ Proposed fix
if OIDCEnabled { if token, ok := extractBearerAuthToken(c.Request().Header); ok { if authenticated, err := authenticateOIDCToken(c, token); err != nil { + ctx.GetSpan().RecordError(err) + logger.Errorf("OIDC token authentication error: %v", err) return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal error"}) } else if authenticated { return next(c) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/kratos.go` around lines 158 - 167, The OIDC error path swallows the real error and only returns a generic 500; update the middleware branch that calls authenticateOIDCToken (the block using OIDCEnabled, extractBearerAuthToken and authenticateOIDCToken) to log the returned error before returning the generic response so debugging is possible; specifically, capture the err from authenticateOIDCToken and call the appropriate logger (e.g., c.Logger().Errorf or your package logger) with a clear message including the error value, then continue to return the same HTTP 500 JSON response.auth/oidc/storage.go (1)
188-206: Consider logging keys that fail to parse in KeySet.Silently skipping keys that fail parsing could hide configuration issues or corrupted key data in the database. Adding a warning log would help with debugging.
♻️ Proposed improvement
for _, k := range keys { pub, err := parseRSAPublicKey(k.PublicKey) if err != nil { + s.ctx.Warnf("Failed to parse public key %s: %v", k.ID, err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/storage.go` around lines 188 - 206, In KeySet, don’t silently skip keys that fail parsing; when parseRSAPublicKey(k.PublicKey) returns an error, log a warning that includes the key identifier (k.ID) and the parsing error so operators can debug corrupted/invalid key data. Locate the KeySet method in Storage and add a warning via the existing context/logger (e.g., s.ctx.Logger or equivalent) inside the error branch before the continue, referencing PublicKey, parseRSAPublicKey, and publicKey so the log clearly ties the failed parse to the DB record.cmd/auth_login.go (1)
44-44: Add doc comments to the new top-level helpers.Most of the new functions in this file are undocumented, while
validateNoncealready is.As per coding guidelines, "Add function comments in Go, even if self-explanatory, as they are considered good practice."
Also applies to: 160-160, 174-174, 185-185, 191-191, 249-249, 270-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_login.go` at line 44, Add Go doc comments to all new top-level helper functions in this file (e.g., runAuthLogin and the helpers at the other locations referenced) following Go conventions: for each exported or non-exported function add a single-line comment that begins with the function name and briefly describes its purpose and behavior (note validateNonce already has a comment so leave it). Place the comment immediately above each function declaration (for example above runAuthLogin, and the functions at the other referenced spots) and ensure phrasing is concise and uses proper punctuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@auth/kratos.go`:
- Around line 238-241: The Match method currently returns the raw error from
middleware.kratosLoginWithCache which can leak internal authentication details
to callers; change KratosCredentialChecker.Match to call
k.middleware.kratosLoginWithCache(ctx, user, pass), log or record the original
err internally (e.g. at debug), but always return a generic authentication error
such as errors.New("invalid credentials") or a wrapped sentinel error (without
including err.Error()) so callers/handlers (like the OIDC login handler) don't
expose internal messages.
In `@auth/middleware.go`:
- Around line 65-76: skipAuthPathsExact currently contains "/end_session"
(underscore) but the OIDC routes register "/endsession" (no underscore), causing
the logout endpoint to be protected; update the skipAuthPathsExact slice (the
symbol skipAuthPathsExact in auth/middleware.go) to include the correct
"/endsession" entry (and remove or optionally keep the incorrect "/end_session"
if unused) so the OIDC logout route matches the exact-path exclusion and does
not require auth.
In `@auth/oidc/login.go`:
- Around line 77-79: The handler currently returns the raw error from
h.checker.Match directly in renderForm ("Invalid credentials: %v"), which can
leak internal details; change the login flow in login.go (the code around
h.checker.Match and renderForm) to return a generic user-facing message like
"Invalid credentials" via renderForm while sending the original err to an
internal logger (e.g., using server logger, h.logger, or context logger) for
debugging; ensure you do not expose err text in the response body or UI but
still preserve the error in logs for operators.
In `@auth/oidc/models.go`:
- Around line 155-161: Replace the permissive scope passthroughs with an
explicit whitelist: define a package-level set of the four allowed scopes and
update RestrictAdditionalIdTokenScopes and RestrictAdditionalAccessTokenScopes
to return functions that filter the input slice to only those whitelisted
scopes, and change IsScopeAllowed to return whether a given scope is in that
set; use the existing function names RestrictAdditionalIdTokenScopes,
RestrictAdditionalAccessTokenScopes and IsScopeAllowed to locate and update the
logic so the CLI only permits the specific allowed scopes.
- Around line 44-60: The AuthRequest struct is redeclaring core ID/timestamp
fields instead of reusing the shared duty model types; replace the explicit
ID/CreatedAt/ExpiresAt (and similar core fields) by embedding the central model
type from github.com/flanksource/duty/models (e.g., the package's Base/Model
struct) so GORM tags are preserved via the embedded type, remove the duplicated
fields (ID, CreatedAt, ExpiresAt, and any other core fields) from AuthRequest
and the other structs at the referenced ranges (lines 96-106 and 121-127) and
update imports to include github.com/flanksource/duty/models; ensure any custom
column tags or JSON names still match by applying tags on the embedded field or
adding struct tag overrides if needed.
In `@cmd/auth_login.go`:
- Around line 145-147: The current login prints the sensitive tokens.AccessToken
to stdout (via fmt.Fprintf to cmd.OutOrStdout()), which leaks secrets; change
the behavior so token printing is explicit/opt-in: remove unconditional prints
of tokens.AccessToken and the curl example with the real token, instead only
print a generic success message and a curl example that uses a placeholder, and
add a boolean flag (e.g., --print-token or --show-token) on the command that,
when true, prints the real AccessToken and the curl example with serverURL and
tokens.AccessToken; ensure the conditional uses the command flag value (from
cmd.Flags()) so existing callers keep the safe default.
- Around line 59-60: randomBase64 currently ignores errors from rand.Read and
can return a predictable zero-filled string; update randomBase64 to return
(string, error) and check/handle that error at each call site (e.g., where state
and nonce are set in cmd/auth_login.go and the other calls later in the file),
failing closed by aborting the flow (returning the error, logging and exiting,
or returning an HTTP error) rather than proceeding with a deterministic value;
ensure callers of randomBase64 propagate or handle the error appropriately and
remove any silent ignores.
- Around line 45-52: Ensure the provided loginServer is parsed and validated
before performing OIDC discovery: parse loginServer (the variable used to build
serverURL), require URL scheme "https" for remote hosts, and allow non-HTTPS
only when the host is loopback/local (e.g., "localhost", "127.0.0.1", "::1") or
equivalent; if validation fails return an explicit error (e.g., "insecure server
URL: only loopback may use http"). Add this check right after
trimming/normalizing serverURL and before calling discoverOIDC so
discoverOIDC/browser/token exchange never run over plain HTTP for remote hosts.
Use url.Parse and inspect u.Scheme and the hostname to implement the guard.
---
Duplicate comments:
In `@auth/oidc/login.go`:
- Line 63: The current forced type assertion ctx :=
c.Request().Context().(context.Context) can panic; replace it with a safe
approach by either using the context directly (ctx := c.Request().Context()) or
performing a checked assertion (val, ok :=
c.Request().Context().(context.Context); if !ok { handle error/return }) so the
login handler in login.go does not crash when extracting ctx from
c.Request().Context().
In `@auth/oidc/models.go`:
- Around line 18-40: The custom PostgreSQL array parsing in StringList.Value and
StringList.Scan is fragile; replace it by delegating to a proven text[] codec
(e.g., github.com/lib/pq's pq.StringArray). Update StringList.Value to call
pq.StringArray(s).Value() (handling nil/empty appropriately) and update
StringList.Scan to call (&pq.StringArray{}).Scan(src) and then convert the
pq.StringArray result back into your StringList (or make StringList a type alias
of pq.StringArray). Also add the pq import and remove the manual join/split
logic so quoted or comma-containing values are handled correctly.
---
Nitpick comments:
In `@auth/basic.go`:
- Around line 110-129: The bearer token handling in the middleware (inside the
extractBearerAuthToken branch) is deeply nested; extract the access-token
fallback into a helper (e.g., authenticateAccessToken) that accepts echo.Context
and the token string and returns bool, moving the getAccessToken, DB lookup of
models.Person, InjectToken call, ctx.WithUser and c.SetRequest logic into that
helper and ensuring the context type assertion
(c.Request().Context().(context.Context)) is checked safely; then simplify the
original block to call authenticateOIDCToken first and, on false, call
authenticateAccessToken and return next(c) if it returns true or a 401
otherwise.
In `@auth/kratos.go`:
- Around line 158-167: The OIDC error path swallows the real error and only
returns a generic 500; update the middleware branch that calls
authenticateOIDCToken (the block using OIDCEnabled, extractBearerAuthToken and
authenticateOIDCToken) to log the returned error before returning the generic
response so debugging is possible; specifically, capture the err from
authenticateOIDCToken and call the appropriate logger (e.g., c.Logger().Errorf
or your package logger) with a clear message including the error value, then
continue to return the same HTTP 500 JSON response.
In `@auth/oidc/provider.go`:
- Around line 46-49: The provider is always created with op.WithAllowInsecure(),
which permits http issuers; change NewProvider call in the oidc provider
initialization to conditionally include op.WithAllowInsecure() only when the
issuerURL uses the "http://" scheme (or when running in a recognized dev
environment). Concretely, inspect issuerURL before calling op.NewProvider (the
oidcProvider creation block) and build the options slice so
op.WithAllowInsecure() is appended only if issuerURL has the "http://" prefix
(or an explicit dev flag), leaving production HTTPS issuers to be created
without WithAllowInsecure.
In `@auth/oidc/storage.go`:
- Around line 188-206: In KeySet, don’t silently skip keys that fail parsing;
when parseRSAPublicKey(k.PublicKey) returns an error, log a warning that
includes the key identifier (k.ID) and the parsing error so operators can debug
corrupted/invalid key data. Locate the KeySet method in Storage and add a
warning via the existing context/logger (e.g., s.ctx.Logger or equivalent)
inside the error branch before the continue, referencing PublicKey,
parseRSAPublicKey, and publicKey so the log clearly ties the failed parse to the
DB record.
In `@cmd/auth_login.go`:
- Line 44: Add Go doc comments to all new top-level helper functions in this
file (e.g., runAuthLogin and the helpers at the other locations referenced)
following Go conventions: for each exported or non-exported function add a
single-line comment that begins with the function name and briefly describes its
purpose and behavior (note validateNonce already has a comment so leave it).
Place the comment immediately above each function declaration (for example above
runAuthLogin, and the functions at the other referenced spots) and ensure
phrasing is concise and uses proper punctuation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7115d21e-64be-4908-821c-a33700f6a736
📒 Files selected for processing (13)
.gitignoreauth/basic.goauth/kratos.goauth/middleware.goauth/oidc/login.goauth/oidc/models.goauth/oidc/provider.goauth/oidc/routes.goauth/oidc/storage.goauth/oidc_test.goauth/oidc_validate.gocmd/auth_login.gojobs/jobs.go
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- auth/oidc_validate.go
🚧 Files skipped from review as they are similar to previous changes (1)
- auth/oidc/routes.go
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 29085375 | Triggered | RSA Private Key | 077e179 | tests/oidc_e2e/oidc-signing-key.pem | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
cmd/auth_login.go (2)
133-136:⚠️ Potential issue | 🟠 MajorAvoid printing tokens to stdout by default.
Printing the access and refresh tokens to stdout risks leaking credentials via terminal logs, scripts, or transcripts. Consider making token output opt-in via a
--show-tokensflag.🔒 Proposed change
+var showTokens bool + func init() { authLoginCmd.Flags().StringVar(&loginServer, "server", "", "Mission Control server URL (required)") + authLoginCmd.Flags().BoolVar(&showTokens, "show-tokens", false, "Print tokens to stdout") _ = authLoginCmd.MarkFlagRequired("server") Auth.AddCommand(authLoginCmd) }fmt.Fprintf(cmd.OutOrStdout(), "\nLogin successful!\n\n") fmt.Fprintf(cmd.OutOrStdout(), "Tokens saved to: %s\n\n", tokenPath) - fmt.Fprintf(cmd.OutOrStdout(), "Access token (expires %s):\n%s\n\n", tokens.ExpiresAt.Format("15:04:05"), tokens.AccessToken) - fmt.Fprintf(cmd.OutOrStdout(), "Refresh token:\n%s\n\n", tokens.RefreshToken) + if showTokens { + fmt.Fprintf(cmd.OutOrStdout(), "Access token (expires %s):\n%s\n\n", tokens.ExpiresAt.Format("15:04:05"), tokens.AccessToken) + fmt.Fprintf(cmd.OutOrStdout(), "Refresh token:\n%s\n\n", tokens.RefreshToken) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_login.go` around lines 133 - 136, The code prints sensitive tokens to stdout (the fmt.Fprintf calls that output tokens, using variables tokens and tokenPath); change this so tokens are not printed by default: add a boolean CLI flag (e.g., --show-tokens mapped to a variable like showTokens) to the auth login command, leave the "Tokens saved to: tokenPath" message intact, and wrap the two fmt.Fprintf lines that print tokens (tokens.AccessToken, tokens.RefreshToken and tokens.ExpiresAt) in a conditional if showTokens block so tokens are only printed when the flag is explicitly set.
36-42:⚠️ Potential issue | 🟠 MajorRequire HTTPS for
--serverunless targeting loopback.Discovery, browser auth, and token exchange can run over plain HTTP to a remote host. Credentials could be intercepted. Fail early unless the URL is loopback or uses HTTPS.
🔒 Proposed guard
func runAuthLogin(cmd *cobra.Command, _ []string) error { serverURL := strings.TrimRight(loginServer, "/") + parsed, err := url.Parse(serverURL) + if err != nil { + return fmt.Errorf("invalid server URL: %w", err) + } + host := parsed.Hostname() + if parsed.Scheme != "https" && host != "127.0.0.1" && host != "localhost" && host != "::1" { + return fmt.Errorf("server URL must use https unless targeting loopback") + } endpoints, err := oidcclient.Discover(serverURL + "/.well-known/openid-configuration")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_login.go` around lines 36 - 42, In runAuthLogin, validate loginServer before calling oidcclient.Discover: parse loginServer (used as serverURL) and reject it with a clear error unless the scheme is "https" or the host is a loopback address/name (e.g., "localhost", "127.0.0.1", "::1"); if the URL is missing a scheme, treat it as invalid. Use the parsed URL to detect loopback via the Host or IP, and return an error early (before oidcclient.Discover) when the check fails so discovery/browser/token exchange are prohibited over plain HTTP to non-loopback hosts.auth/oidc/login.go (2)
47-47:⚠️ Potential issue | 🟠 MajorUnsafe context type assertion may panic.
The type assertion
c.Request().Context().(context.Context)will panic if the request context is not aduty/context.Context. This could happen if middleware doesn't properly set up the context.🛡️ Suggested fix: Use comma-ok assertion
- ctx := c.Request().Context().(context.Context) + ctx, ok := c.Request().Context().(context.Context) + if !ok { + return c.String(http.StatusInternalServerError, "internal error") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/login.go` at line 47, The direct type assertion ctx := c.Request().Context().(context.Context) can panic; change it to use a safe comma-ok assertion and fallback to context.Background() (or the original c.Request().Context()) if the assertion fails: retrieve rawCtx := c.Request().Context(), perform ctx, ok := rawCtx.(context.Context) and if !ok set ctx to a safe default, then use ctx for downstream operations (update the assignment in login.go where ctx is created).
62-64:⚠️ Potential issue | 🟡 MinorAvoid exposing internal error details to users.
The error from
h.checker.Matchis displayed directly to the user viafmt.Sprintf("Invalid credentials: %v", err). This could leak internal details about the authentication system (e.g., connection errors, configuration issues). Log the error internally and return a generic message.🛡️ Proposed fix
if err := h.checker.Match(ctx, username, password); err != nil { - return renderForm(fmt.Sprintf("Invalid credentials: %v", err)) + // Log the actual error for debugging + ctx.GetLogger().Warnf("login failed for user %s: %v", username, err) + return renderForm("Invalid credentials") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/login.go` around lines 62 - 64, Do not return internal error text from h.checker.Match to users; instead log the actual err internally and return a generic error string via renderForm. Find the authentication check where h.checker.Match(ctx, username, password) is called and replace the direct fmt.Sprintf usage with an internal log call (e.g., h.logger.Errorf or standard log.Printf) that records the err, then call renderForm with a generic message like "Invalid credentials" (no error details).
🧹 Nitpick comments (7)
tests/oidc_e2e/oidc_login_test.go (1)
113-116: Use HTTP client with timeout in tests.
http.DefaultClienthas no timeout. If the server hangs, the test will block indefinitely. Use a client with a reasonable timeout.♻️ Proposed fix
+var testClient = &http.Client{Timeout: 10 * time.Second} + // In the first test: - resp, err := http.DefaultClient.Do(req) + resp, err := testClient.Do(req) // In the second test: - resp, err := http.DefaultClient.Do(req) + resp, err := testClient.Do(req)Also applies to: 137-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/oidc_e2e/oidc_login_test.go` around lines 113 - 116, Replace uses of http.DefaultClient.Do(req) in the test with a client that has a timeout: create an http.Client (e.g., client := &http.Client{Timeout: 5 * time.Second}) and call client.Do(req) instead of http.DefaultClient.Do(req) both where resp, err := http.DefaultClient.Do(req) appears and the similar occurrence later; also add the time import if missing. This ensures tests use a bounded timeout and won't hang indefinitely.Makefile (1)
142-143: Consider adding checksum verification for downloaded asset.The Tailwind JS is downloaded from CDN without integrity verification. For reproducible builds and supply-chain security, consider pinning a SHA256 checksum.
♻️ Example with checksum
TAILWIND_VERSION ?= 3.4.17 TAILWIND_JS = auth/oidc/static/tailwind.min.js +TAILWIND_SHA256 ?= <expected-sha256-here> $(TAILWIND_JS): curl -sL "https://cdn.tailwindcss.com/$(TAILWIND_VERSION)" -o $(TAILWIND_JS) + echo "$(TAILWIND_SHA256) $(TAILWIND_JS)" | sha256sum -c -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 142 - 143, The Makefile rule that downloads $(TAILWIND_JS) from the CDN using $(TAILWIND_VERSION) lacks integrity verification; add a pinned checksum variable (e.g., TAILWIND_SHA256) and verify the downloaded file's SHA256 before accepting it: download to a temporary file, compute its SHA256 (with sha256sum or openssl dgst -sha256), compare against TAILWIND_SHA256, and only move/rename the temp file to $(TAILWIND_JS) if the checksum matches, otherwise print an error and exit non‑zero; update the target that references TAILWIND_JS/Tailwind to use this verification flow so builds fail on checksum mismatch.auth/oidc/login.go (1)
71-73: Consider logging the internal error before returning generic message.When
SetAuthRequestSubjectfails, only "Internal error" is shown to the user (which is correct for security), but the actual error should be logged for debugging purposes.🔍 Suggested improvement
if err := h.storage.SetAuthRequestSubject(id, personID); err != nil { + ctx.GetLogger().Errorf("failed to set auth request subject for %s: %v", id, err) return renderForm("Internal error") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/login.go` around lines 71 - 73, When h.storage.SetAuthRequestSubject(id, personID) returns an error, log the actual error before returning the generic renderForm response; update the handler around the SetAuthRequestSubject call to capture the error value (err) and call your logger (e.g. h.logger or standard log) with a clear message including the function name and variables (SetAuthRequestSubject, id, personID) for debugging, then still return renderForm("Internal error") to the user.auth/oidc/provider.go (2)
51-54:op.WithAllowInsecure()enables HTTP for development but should be guarded in production.This option allows non-HTTPS issuers, which is appropriate for local development but poses a security risk in production. Consider making this conditional based on environment or configuration.
♻️ Suggested conditional insecure mode
+import "strings" + func NewProvider(ctx context.Context, issuerURL, signingKeyPath string) (*Provider, error) { // ... key loading ... + opts := []op.Option{op.StaticIssuer(issuerURL)} + if strings.HasPrefix(issuerURL, "http://") { + opts = append(opts, op.WithAllowInsecure()) + } + oidcProvider, err := op.NewProvider(config, storage, - op.StaticIssuer(issuerURL), - op.WithAllowInsecure(), // allow http:// for dev + opts..., )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/provider.go` around lines 51 - 54, The call to op.NewProvider currently unconditionally passes op.WithAllowInsecure(), which permits HTTP issuers in production; change NewProvider invocation in provider.go (the oidcProvider creation) to build the options slice conditionally: create a []op.ProviderOption (or similar) containing op.StaticIssuer(issuerURL) and, only if a configurable flag/env (e.g., config.AllowInsecure || env == "development") is true, append op.WithAllowInsecure(); then pass that slice (spread or variadic) into op.NewProvider. Ensure the flag is exposed in your config struct and default is false so production never enables insecure mode.
122-128: Private key parsing only supports PKCS#1 format.
parseRSAPrivateKeyusesx509.ParsePKCS1PrivateKey, which only handles PKCS#1 (RSA PRIVATE KEY) format. Keys generated by tools like OpenSSL may use PKCS#8 (PRIVATE KEY) format, which would fail to parse with a confusing error.♻️ Suggested fix: Support both PKCS#1 and PKCS#8
func parseRSAPrivateKey(data []byte) (*rsa.PrivateKey, error) { block, _ := pem.Decode(data) if block == nil { return nil, fmt.Errorf("no PEM block found") } - return x509.ParsePKCS1PrivateKey(block.Bytes) + // Try PKCS#1 first + if key, err := x509.ParsePKCS1PrivateKey(block.Bytes); err == nil { + return key, nil + } + // Fall back to PKCS#8 + key, err := x509.ParsePKCS8PrivateKey(block.Bytes) + if err != nil { + return nil, fmt.Errorf("failed to parse private key (tried PKCS#1 and PKCS#8): %w", err) + } + rsaKey, ok := key.(*rsa.PrivateKey) + if !ok { + return nil, fmt.Errorf("private key is not RSA") + } + return rsaKey, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/provider.go` around lines 122 - 128, The parseRSAPrivateKey function currently only parses PKCS#1 keys; update it to try parsing PKCS#1 first with x509.ParsePKCS1PrivateKey and if that fails attempt x509.ParsePKCS8PrivateKey on the same PEM block, then assert the returned interface{} to *rsa.PrivateKey and return it; ensure you still handle nil pem.Decode (block == nil) and return a clear error if neither parse succeeds. Reference parseRSAPrivateKey, pem.Decode, x509.ParsePKCS1PrivateKey and x509.ParsePKCS8PrivateKey and include type assertion to *rsa.PrivateKey when handling the PKCS#8 result.auth/oidc/storage.go (2)
200-203: Invalid PEM entries are silently skipped inKeySet.When
parseRSAPublicKeyfails, the key is ignored without logging. This could make it difficult to diagnose issues with corrupted or incorrectly formatted public keys in the database.🔍 Suggested improvement: Log parse failures
for _, k := range keys { pub, err := parseRSAPublicKey(k.PublicKey) if err != nil { + logger.Warnf("failed to parse public key %s: %v", k.ID, err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/storage.go` around lines 200 - 203, KeySet currently skips invalid PEM entries silently when parseRSAPublicKey(k.PublicKey) returns an error; update KeySet to log the parse failures (including the error and some key identifier like k.ID or k.KeyID) instead of only continuing so malformed or corrupted public keys can be diagnosed; locate the loop in KeySet that calls parseRSAPublicKey and replace the silent continue with a logging call (using the package's logger or standard logger) that includes the error and identifying info, then continue to the next key.
163-174:RevokeTokensilently discards non-matching deletes.When the
DELETEquery affects zero rows (token not found), no error is returned. This is acceptable for idempotent revocation, but consider returningop.ErrInvalidRefreshTokenwhenuserID != ""and the token doesn't exist—this helps clients distinguish between "already revoked" and "never existed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/oidc/storage.go` around lines 163 - 174, RevokeToken currently deletes RefreshToken records but ignores zero-row deletes; update Storage.RevokeToken to capture the result of query.Delete(&RefreshToken{}) (e.g., res := query.Delete(&RefreshToken{})) and check res.Error and res.RowsAffected; if res.Error != nil return oidc.ErrServerError(), and if userID != "" and res.RowsAffected == 0 return op.ErrInvalidRefreshToken to signal a non-existent token, otherwise return nil to preserve idempotent behavior for the hashed-token branch; reference Storage.RevokeToken, RefreshToken, and op.ErrInvalidRefreshToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@auth/oidc/storage.go`:
- Around line 298-302: In generateKeyID, json.Marshal(pub) failure is currently
swallowed and a UUID fallback is returned; update the error handling in
generateKeyID to log the marshal error (using the package logger or the
surrounding context logger) before returning the fallback ID so the failure is
visible, and preserve the existing fallback behavior to maintain compatibility;
reference the json.Marshal(pub) call and the generateKeyID function when making
the change.
- Around line 95-98: The AuthRequest.code column needs a uniqueness guarantee:
update the AuthRequest model to add a unique index tag on the code field (so
GORM will know it's unique) and add a DB migration to create a unique
constraint/index on the oidc_auth_requests.code column in the database schema;
ensure the lookup function (e.g., AuthRequestByCode) can rely on uniqueness and
that SaveAuthCode (Storage.SaveAuthCode) continues to update the hashed code,
leaving existing logic intact.
In `@auth/oidcclient/oidcclient.go`:
- Around line 30-42: In Discover, after calling HTTPClient.Get(discoveryURL)
check resp.StatusCode for a 2xx range before attempting json.NewDecoder on
resp.Body; if the status is not OK, read or snapshot a small portion of
resp.Body (or include resp.Status and resp.StatusCode) and return a clear error
instead of proceeding to json decode. Update the Discover function to validate
resp.StatusCode, close resp.Body via defer as now, and return an error
containing the status (and optionally truncated body text) when non-2xx is
returned so callers get a meaningful message instead of a JSON decode error.
- Around line 55-59: RandomBase64 currently discards errors from rand.Read and
returns a predictable zero-filled string; change RandomBase64 to return (string,
error), call crypto/rand.Read and if it returns an error propagate it (return
"", err), otherwise return the base64.RawURLEncoding.EncodeToString(b) and nil.
Update its callers (e.g., in cmd/auth_login.go and
tests/oidc_e2e/oidc_login_test.go) to handle the error: call RandomBase64, check
the error and fail/propagate as appropriate (abort the login flow or fail the
test) instead of assuming a valid string.
In `@cmd/auth_login.go`:
- Around line 161-169: The openBrowser function lacks Windows support and will
fail on Windows because it always uses "open" or "xdg-open"; update openBrowser
to handle runtime.GOOS == "windows" by using the Windows shell invocation (e.g.,
"cmd", "/c", "start" with the URL) when building the exec.Command invocation,
and ensure you pass the correct arguments to exec.Command instead of a single
concatenated string so Start() works across platforms.
In `@Makefile`:
- Around line 145-146: The new Makefile target named static (with dependency
$(TAILWIND_JS)) is shadowing the existing static target (which depends on
manifests, generate, fmt, ginkgo); rename the new target (e.g., tailwind or
assets) so it no longer overrides the original, give that new target the
$(TAILWIND_JS) dependency, and update the original static target to also depend
on the new target name (so static still runs manifests, generate, fmt, ginkgo
and the tailwind/assets step).
---
Duplicate comments:
In `@auth/oidc/login.go`:
- Line 47: The direct type assertion ctx :=
c.Request().Context().(context.Context) can panic; change it to use a safe
comma-ok assertion and fallback to context.Background() (or the original
c.Request().Context()) if the assertion fails: retrieve rawCtx :=
c.Request().Context(), perform ctx, ok := rawCtx.(context.Context) and if !ok
set ctx to a safe default, then use ctx for downstream operations (update the
assignment in login.go where ctx is created).
- Around line 62-64: Do not return internal error text from h.checker.Match to
users; instead log the actual err internally and return a generic error string
via renderForm. Find the authentication check where h.checker.Match(ctx,
username, password) is called and replace the direct fmt.Sprintf usage with an
internal log call (e.g., h.logger.Errorf or standard log.Printf) that records
the err, then call renderForm with a generic message like "Invalid credentials"
(no error details).
In `@cmd/auth_login.go`:
- Around line 133-136: The code prints sensitive tokens to stdout (the
fmt.Fprintf calls that output tokens, using variables tokens and tokenPath);
change this so tokens are not printed by default: add a boolean CLI flag (e.g.,
--show-tokens mapped to a variable like showTokens) to the auth login command,
leave the "Tokens saved to: tokenPath" message intact, and wrap the two
fmt.Fprintf lines that print tokens (tokens.AccessToken, tokens.RefreshToken and
tokens.ExpiresAt) in a conditional if showTokens block so tokens are only
printed when the flag is explicitly set.
- Around line 36-42: In runAuthLogin, validate loginServer before calling
oidcclient.Discover: parse loginServer (used as serverURL) and reject it with a
clear error unless the scheme is "https" or the host is a loopback address/name
(e.g., "localhost", "127.0.0.1", "::1"); if the URL is missing a scheme, treat
it as invalid. Use the parsed URL to detect loopback via the Host or IP, and
return an error early (before oidcclient.Discover) when the check fails so
discovery/browser/token exchange are prohibited over plain HTTP to non-loopback
hosts.
---
Nitpick comments:
In `@auth/oidc/login.go`:
- Around line 71-73: When h.storage.SetAuthRequestSubject(id, personID) returns
an error, log the actual error before returning the generic renderForm response;
update the handler around the SetAuthRequestSubject call to capture the error
value (err) and call your logger (e.g. h.logger or standard log) with a clear
message including the function name and variables (SetAuthRequestSubject, id,
personID) for debugging, then still return renderForm("Internal error") to the
user.
In `@auth/oidc/provider.go`:
- Around line 51-54: The call to op.NewProvider currently unconditionally passes
op.WithAllowInsecure(), which permits HTTP issuers in production; change
NewProvider invocation in provider.go (the oidcProvider creation) to build the
options slice conditionally: create a []op.ProviderOption (or similar)
containing op.StaticIssuer(issuerURL) and, only if a configurable flag/env
(e.g., config.AllowInsecure || env == "development") is true, append
op.WithAllowInsecure(); then pass that slice (spread or variadic) into
op.NewProvider. Ensure the flag is exposed in your config struct and default is
false so production never enables insecure mode.
- Around line 122-128: The parseRSAPrivateKey function currently only parses
PKCS#1 keys; update it to try parsing PKCS#1 first with
x509.ParsePKCS1PrivateKey and if that fails attempt x509.ParsePKCS8PrivateKey on
the same PEM block, then assert the returned interface{} to *rsa.PrivateKey and
return it; ensure you still handle nil pem.Decode (block == nil) and return a
clear error if neither parse succeeds. Reference parseRSAPrivateKey, pem.Decode,
x509.ParsePKCS1PrivateKey and x509.ParsePKCS8PrivateKey and include type
assertion to *rsa.PrivateKey when handling the PKCS#8 result.
In `@auth/oidc/storage.go`:
- Around line 200-203: KeySet currently skips invalid PEM entries silently when
parseRSAPublicKey(k.PublicKey) returns an error; update KeySet to log the parse
failures (including the error and some key identifier like k.ID or k.KeyID)
instead of only continuing so malformed or corrupted public keys can be
diagnosed; locate the loop in KeySet that calls parseRSAPublicKey and replace
the silent continue with a logging call (using the package's logger or standard
logger) that includes the error and identifying info, then continue to the next
key.
- Around line 163-174: RevokeToken currently deletes RefreshToken records but
ignores zero-row deletes; update Storage.RevokeToken to capture the result of
query.Delete(&RefreshToken{}) (e.g., res := query.Delete(&RefreshToken{})) and
check res.Error and res.RowsAffected; if res.Error != nil return
oidc.ErrServerError(), and if userID != "" and res.RowsAffected == 0 return
op.ErrInvalidRefreshToken to signal a non-existent token, otherwise return nil
to preserve idempotent behavior for the hashed-token branch; reference
Storage.RevokeToken, RefreshToken, and op.ErrInvalidRefreshToken.
In `@Makefile`:
- Around line 142-143: The Makefile rule that downloads $(TAILWIND_JS) from the
CDN using $(TAILWIND_VERSION) lacks integrity verification; add a pinned
checksum variable (e.g., TAILWIND_SHA256) and verify the downloaded file's
SHA256 before accepting it: download to a temporary file, compute its SHA256
(with sha256sum or openssl dgst -sha256), compare against TAILWIND_SHA256, and
only move/rename the temp file to $(TAILWIND_JS) if the checksum matches,
otherwise print an error and exit non‑zero; update the target that references
TAILWIND_JS/Tailwind to use this verification flow so builds fail on checksum
mismatch.
In `@tests/oidc_e2e/oidc_login_test.go`:
- Around line 113-116: Replace uses of http.DefaultClient.Do(req) in the test
with a client that has a timeout: create an http.Client (e.g., client :=
&http.Client{Timeout: 5 * time.Second}) and call client.Do(req) instead of
http.DefaultClient.Do(req) both where resp, err := http.DefaultClient.Do(req)
appears and the similar occurrence later; also add the time import if missing.
This ensures tests use a bounded timeout and won't hang indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36a8d98c-e20b-42fa-917a-31b24ca046ab
⛔ Files ignored due to path filters (3)
auth/oidc/static/logo.svgis excluded by!**/*.svggo.sumis excluded by!**/*.sumtests/oidc_e2e/oidc-signing-key.pemis excluded by!**/*.pem
📒 Files selected for processing (18)
.gitignoreMakefileauth/middleware.goauth/oidc/login.goauth/oidc/provider.goauth/oidc/routes.goauth/oidc/static/callback_success.htmlauth/oidc/static/embed.goauth/oidc/static/login.htmlauth/oidc/storage.goauth/oidc_test.goauth/oidc_validate.goauth/oidcclient/oidcclient.gocmd/auth_login.gogo.modspecs/Authentication.mdtests/oidc_e2e/oidc_login_test.gotests/oidc_e2e/suite_test.go
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- auth/oidc/static/callback_success.html
- auth/oidc/static/login.html
- specs/Authentication.md
- auth/oidc_validate.go
🚧 Files skipped from review as they are similar to previous changes (4)
- auth/oidc/routes.go
- auth/oidc_test.go
- auth/middleware.go
- go.mod
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
33-44: Standardize Go/cache version tokens across workflows for cache reuse.Line 33 uses
v1.26.x, while other workflows use1.26.x; Lines 42–44 also use a different cache prefix format (1.26vs1.26.x). Unifying these avoids accidental cache fragmentation and keeps CI config easier to maintain.Suggested normalization
- go-version: v1.26.x + go-version: 1.26.x ... - key: cache-go-1.26-${{ hashFiles('**/go.sum') }}-${{ hashFiles('.bin/*') }} + key: cache-go-1.26.x-${{ hashFiles('**/go.sum') }}-${{ hashFiles('.bin/*') }} restore-keys: | - cache-go-1.26- + cache-go-1.26.x-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 33 - 44, The workflow mixes Go version tokens and cache prefixes which fragments caches; update the go-version entry (symbol: go-version) to use "1.26.x" (matching other workflows) and normalize the cache key and restore-keys strings used with the actions/cache step (the actions/cache@... block and its key/restore-keys values) to consistently include the same token "1.26.x" (e.g., change any "1.26" occurrences in cache key and restore-keys to "1.26.x") so all workflows use the identical cache namespace.Dockerfile (1)
11-18: Consider running the container as a non-root user.The static analysis tool flags that the runtime image does not specify a non-root user (DS-0002). Running containers as root increases the attack surface and potential impact of container escapes.
🛡️ Suggested fix to add a non-root user
Check whether
flanksource/base-image:0.6.0already includes a non-root user, then add aUSERdirective:COPY --from=builder /app/.bin/incident-commander /app COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ +USER 1000:1000 RUN /app/incident-commander go-offline ENTRYPOINT ["/app/incident-commander"]Alternatively, if the base image provides a specific user (e.g.,
appuser), use that:+USER appuser RUN /app/incident-commander go-offline🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 11 - 18, The Dockerfile currently runs the runtime image as root; add and switch to a non-root user to satisfy DS-0002: create or reuse a non-root user and group, ensure /app and any copied files are owned by that user (references: COPY --from=builder ... /app, RUN /app/incident-commander go-offline, ENTRYPOINT ["/app/incident-commander"]), then add a USER directive at the end of the Dockerfile to run the container as that non-root user; if the base image already exposes a user name, simply switch to it, otherwise create the user and chown the necessary paths before switching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 33-44: The workflow mixes Go version tokens and cache prefixes
which fragments caches; update the go-version entry (symbol: go-version) to use
"1.26.x" (matching other workflows) and normalize the cache key and restore-keys
strings used with the actions/cache step (the actions/cache@... block and its
key/restore-keys values) to consistently include the same token "1.26.x" (e.g.,
change any "1.26" occurrences in cache key and restore-keys to "1.26.x") so all
workflows use the identical cache namespace.
In `@Dockerfile`:
- Around line 11-18: The Dockerfile currently runs the runtime image as root;
add and switch to a non-root user to satisfy DS-0002: create or reuse a non-root
user and group, ensure /app and any copied files are owned by that user
(references: COPY --from=builder ... /app, RUN /app/incident-commander
go-offline, ENTRYPOINT ["/app/incident-commander"]), then add a USER directive
at the end of the Dockerfile to run the container as that non-root user; if the
base image already exposes a user name, simply switch to it, otherwise create
the user and chown the necessary paths before switching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e418a63-8ef3-4af3-9397-9960ca35ca3f
📒 Files selected for processing (6)
.github/workflows/codeql.yml.github/workflows/lint.yml.github/workflows/release.yml.github/workflows/test.yml.gitignoreDockerfile
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/codeql.yml
- .github/workflows/lint.yml
- .gitignore
Cover storage CRUD (auth requests, refresh tokens, revocation, cleanup), JWT validation (valid, wrong key, expired, wrong audience, unknown subject), and login handler form rendering/credential rejection.
- Export CredentialChecker and PersonLookup interfaces from oidc package - Decouple MountRoutes from htpasswd: accept CredentialChecker directly - Add KratosCredentialChecker that validates credentials via Kratos API - Add LookupKratosPersonByUsername for Kratos identity-to-person mapping - Add OIDC Bearer token validation to Kratos Session middleware - Fix unique constraint violation: use *string for AuthRequest.Code - Clean up OIDC test data in AfterEach to prevent constraint conflicts
- Escape auth_request_id in login form HTML to prevent XSS - Remove duplicate AuthCmd, add login subcommand to existing Auth - Use SHA-256 for AES key derivation instead of raw byte copy - Add 30s timeout to HTTP client in CLI auth login - Use safe type assertion for JWT audience array - Fix "notificatino" typo in jobs error message
Set done=true in SetAuthRequestSubject when persisting the authenticated subject. Zitadel's /authorize/callback requires AuthRequest.Done() to be true before issuing an authorization code. Without this, the flow fails with interaction_required even after valid credentials.
Return identity scope-filter functions for the CLI OIDC client instead of nil from RestrictAdditionalIdTokenScopes and RestrictAdditionalAccessTokenScopes. Zitadel calls these hooks during token creation. Returning nil caused a nil function dereference in pkg/op/token.go (CreateJWT/CreateIDToken), leading to panic during code exchange.
… hashing Implements a complete OIDC provider for CLI authentication with token security enhancements. Introduces cryptographically secure token storage using SHA-256 hashing for short-lived auth codes and refresh tokens, and separate AES-256 key generation for OIDC state encryption. Adds professionally styled login and callback success pages with Tailwind CSS. Includes CLI tools for PKCE-based OAuth flows and comprehensive security documentation.
Remove /oidc suffix from issuer URL construction so that the provider uses the base public URL (e.g., http://localhost:8080) instead of appending /oidc. Add static asset serving for embedded resources at /oidc/static/.
Adds Tailwind-styled login and success pages with embedded assets, proper token hashing in storage, crypto key management, and refresh token grant support. Extracts OIDC client logic to reusable package and adds browser-based e2e tests.
…uild targets golangci-lint v2.6.0 was built with Go 1.25 and cannot target Go 1.26. Add tailwind.min.js download as dependency for ci-test, e2e, and linux targets so the go:embed in auth/oidc/static/embed.go resolves in CI.
The TAILWIND_JS variable and download rule were defined after the e2e and ci-test targets. GNU Make 3.81 doesn't resolve forward-referenced file target prerequisites, causing the curl download to be skipped.
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
4 participants
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.
Summary
zitadel/oidc/v3for CLI authentication via Authorization Code + PKCE flowop.Storageinterface backed by PostgreSQL (auth requests, refresh tokens, public keys)auth loginCLI command with PKCE browser flow, local callback server, and token storageSchema dependency
oidc_public_keys,oidc_auth_requests,oidc_refresh_tokens)New files
auth/oidc/— storage, provider, models, login handler, routes, cleanup jobauth/oidc_validate.go— JWT validation against DB public keyscmd/auth_login.go— CLIauth logincommand with PKCE flowTest plan
make lintpasses with 0 issuesSummary by CodeRabbit
New Features
Maintenance
Tests
Chores