feat: oidc#605
Conversation
📝 WalkthroughWalkthroughAdds OpenID Connect support across backend and frontend: DB schemas and migrations, sqlc queries and repository code, a full OIDC service and controllers (including /.well-known/JWKS), bootstrap/middleware wiring, frontend OIDC parsing and an /authorize UI, tests, and related tooling/i18n updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OIDC Client
participant Browser as Browser
participant AuthPage as AuthorizePage
participant API as API Server (OIDC Controller)
participant Service as OIDC Service
participant DB as Database
Client->>Browser: Redirect to /authorize?client_id=...&redirect_uri=...&scope=openid
Browser->>AuthPage: GET /authorize (load params)
AuthPage->>API: GET /api/oidc/clients/:id
API->>Service: GetClient(id)
Service-->>API: client config
API-->>AuthPage: client info
Browser->>AuthPage: User clicks "Authorize"
AuthPage->>API: POST /api/oidc/authorize {client_id, redirect_uri, scope, state}
API->>Service: ValidateAuthorizeParams(req)
Service-->>API: ok
API->>Service: StoreCode(sub, code, req)
Service->>DB: INSERT oidc_codes
DB-->>Service: inserted
API->>Service: StoreUserinfo(sub, userinfo) (optional)
Service->>DB: INSERT oidc_userinfo
DB-->>Service: inserted
API-->>Browser: 200 {redirect_uri: "...?code=...&state=..."}
Browser->>Client: Redirect back to client with code
sequenceDiagram
participant Client as OIDC Client
participant TokenAPI as API Server (Token Endpoint)
participant Service as OIDC Service
participant DB as Database
Client->>TokenAPI: POST /api/oidc/token (grant_type=authorization_code, code=...)
TokenAPI->>Service: ValidateGrantType("authorization_code")
Service-->>TokenAPI: ok
TokenAPI->>Service: GetCodeEntry(codeHash)
Service->>DB: SELECT oidc_codes WHERE code_hash=...
DB-->>Service: OidcCode row
TokenAPI->>Service: GenerateAccessToken(client, sub, scope)
Service->>Service: generateIDToken() (RS256 sign)
Service->>DB: INSERT oidc_tokens
DB-->>Service: inserted
TokenAPI->>Service: DeleteCodeEntry(codeHash)
Service->>DB: DELETE oidc_codes WHERE code_hash=...
DB-->>Service: deleted
TokenAPI-->>Client: 200 {access_token, id_token, refresh_token, expires_in}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #605 +/- ##
==========================================
- Coverage 19.35% 16.24% -3.12%
==========================================
Files 41 45 +4
Lines 2490 3379 +889
==========================================
+ Hits 482 549 +67
- Misses 1974 2776 +802
- Partials 34 54 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@frontend/src/pages/authorize-page.tsx`:
- Around line 83-91: The current check uses Object.keys(props).forEach which
returns inside the callback only, so the component never short-circuits to
<Navigate/>; change the logic in the Authorize page to detect missing required
props synchronously (for example using a for...of loop, Array.prototype.some, or
a dedicated helper) over Object.keys(props) and optionalAuthorizeProps, and when
a missing required param is found return the <Navigate to="/error" replace />
from the component render path (not from inside a callback) so the redirect
actually occurs; reference the props object, AuthorizePageProps type,
optionalAuthorizeProps array and the Navigate element when implementing the fix.
In `@internal/bootstrap/app_bootstrap.go`:
- Around line 88-92: The loop that populates app.context.oidcClients from
app.config.OIDC.Clients currently ignores ClientSecretFile; update the loop so
for each client (iterate id, client over app.config.OIDC.Clients) if
client.ClientSecretFile is non-empty read the file contents, trim
whitespace/newline, assign the value to client.ClientSecret (and handle read
errors appropriately by returning or logging), then append the resolved client
to app.context.oidcClients; reference the identifiers app.config.OIDC.Clients,
ClientSecretFile, client.ClientSecret, and app.context.oidcClients when making
the change.
In `@internal/controller/oidc_controller.go`:
- Around line 200-242: The code currently generates a random `sub` with
utils.GetRandomInt which makes the subject unstable; replace that with a stable
identifier derived from the authenticated user (e.g., userContext.ID or
userContext.Subject) or compute a pairwise subject by hashing (user ID +
client.ClientID + server secret) so the same user maps to the same `sub` per
client; update the assignments where `sub` is used (the CreateOidcCode call and
CreateOidcUserInfoParams construction) to use this new stable/pairwise value and
remove the utils.GetRandomInt call and related error handling.
- Around line 383-391: The current flow reads the authorization code and then
calls controller.queries.DeleteOidcCode(c, entry.Code), which allows a race
where two requests can both read and then mint tokens; change this to an atomic
consume operation by adding a single database call that deletes the code and
returns the row (or use a transaction that performs SELECT ... FOR UPDATE then
DELETE) so only one request can succeed; implement a new queries method (e.g.,
DeleteOidcCodeReturning or ConsumeOidcCode) that performs DELETE ... RETURNING
(or SELECT FOR UPDATE then DELETE) and use its returned result to both validate
the code and proceed to mint tokens, handling a missing/zero-rows result as
invalid/consumed. Ensure you replace the separate
read+controller.queries.DeleteOidcCode usage with this new atomic consume call
in oidc_controller.go.
- Around line 127-199: The handler currently issues authorization codes without
validating req.ResponseType and without ensuring required scopes (like "openid")
are present; update the authorization flow in the OIDC controller (where
AuthorizeRequest is bound to req and scopes are processed using SupportedScopes
and keptScopes) to first validate req.ResponseType is an allowed response (e.g.,
"code" or whatever your SupportedResponseTypes set contains) and if it's not,
build a CallbackError and redirect to the client redirect with the encoded error
(reuse the existing query.Values/CallbackError pattern). Additionally, before
issuing any code ensure the keptScopes include "openid" (or other mandatory
scopes for that response_type); if the required scope is missing, return a
descriptive invalid_request error via the same redirect mechanism instead of
proceeding to generate a code.
- Around line 329-373: After retrieving the code via GetOidcCode and before
minting the token (i.e., before calling utils.GetRandomString and
CreateOidcToken), validate that the code is still owned by the requesting client
and not expired: check entry.ClientID equals the requesting client ID
(client.ClientID) and that entry.ExpiresAt (unix) is greater than
time.Now().Unix(); if either check fails log a warning (include the mismatched
client or expiry) and return c.JSON(400, gin.H{"error":"invalid_grant"}) without
proceeding to token generation or calling CreateOidcToken.
In `@internal/middleware/context_middleware.go`:
- Line 17: The OIDC exact-path check using OIDCIgnorePaths fails for URLs with
trailing slashes and currently risks shadowing the imported path package; fix by
normalizing the request path with path.Clean (e.g., cleanedPath :=
path.Clean(r.URL.Path)) and use cleanedPath for comparisons against
OIDCIgnorePaths (or iterate and compare path.Clean on each ignored entry),
ensuring you do not reuse the identifier name "path" so the package import isn't
shadowed; update the matching logic in the middleware where OIDCIgnorePaths is
checked (the exact-path match block) to use this normalized variable.
In `@internal/utils/security_utils_test.go`:
- Around line 163-172: The test TestGetRandomInt incorrectly asserts digit
length and casts int64 to int, causing flakiness and overflow; update the test
to validate the numeric range instead of string length by calling
GetRandomInt(16) and asserting state >= 0 and state < int64(math.Pow10(16)) (use
math.Pow10 to compute the upper bound as an int64) and remove any int casts, and
keep the zero-length case asserting an error from GetRandomInt(0).
In `@internal/utils/security_utils.go`:
- Around line 126-134: GetRandomInt currently uses math.Pow which overflows for
length >= 19 and can pass a non-positive max to rand.Int; modify GetRandomInt to
check length is between 1 and 18 inclusive, compute the power of ten using
integer arithmetic (e.g., loop or precomputed int64 array) to produce a positive
int64 max = 10^length, convert that to a big.Int via big.NewInt(max) and pass it
to rand.Int, and return the resulting Int64; keep the error return for invalid
lengths and propagate rand.Int errors from within GetRandomInt.
In `@sql/oidc_schemas.sql`:
- Around line 1-8: Change the oidc_codes schema so you no longer store raw
bearer values: replace the "code" TEXT column with a non-reversible fingerprint
column (e.g., "code_hash" TEXT NOT NULL PRIMARY KEY) and store only HMAC-SHA256
(or similar) computed with a server secret; update any other plaintext token
columns (the ones mentioned at lines 10-16) the same way. In addition to the
schema change on the "oidc_codes" table, update all insert/update points to
compute and persist the HMAC hash (not the raw code) and update all
lookup/controller logic to HMAC the incoming code/token before querying by
"code_hash" (or equivalent field names) so the DB never contains plaintext
bearer values. Ensure the server secret used for HMAC is loaded securely and
remains constant across restarts so stored hashes remain verifiable.
🧹 Nitpick comments (6)
frontend/src/schemas/oidc-schemas.ts (1)
3-4: Fix typo in exported schema name for consistency.
getOidcClientInfoScehmareads like a misspelling and can ripple confusion through imports. Consider renaming togetOidcClientInfoSchemaand updating consumers accordingly.♻️ Proposed change
-export const getOidcClientInfoScehma = z.object({ +export const getOidcClientInfoSchema = z.object({ name: z.string(), });frontend/src/pages/authorize-page.tsx (3)
17-25: Alignstatetyping with optional behavior.
stateis treated as optional (viaoptionalAuthorizeProps), but the type marks it as required. This can mislead TS users and downstream logic.♻️ Proposed change
type AuthorizePageProps = { scope: string; responseType: string; clientId: string; redirectUri: string; - state: string; + state?: string; }; -const optionalAuthorizeProps = ["state"]; +const optionalAuthorizeProps: (keyof AuthorizePageProps)[] = ["state"];
78-81: Preserve OIDC params when redirecting to login.The TODO suggests the auth flow isn’t resumable after login. Consider forwarding the current query string to
/loginso the user can return to authorization seamlessly.💡 Possible tweak
- return <Navigate to="/login" replace />; + return <Navigate to={`/login?${searchParams.toString()}`} replace />;If you want, I can help wire this through the login flow.
43-50: Guard client lookup whenclient_idis missing and handle non-OK responses.The query runs unconditionally even when
props.clientIdis an empty string (from line 38), which causes requests to/api/oidc/clients/with an empty ID. Additionally,fetchshould throw on non-OK status to ensure React Query's error state is triggered cleanly rather than attempting to parse failed responses as JSON.♻️ Proposed change
const getClientInfo = useQuery({ queryKey: ["client", props.clientId], + enabled: Boolean(props.clientId), queryFn: async () => { const res = await fetch(`/api/oidc/clients/${props.clientId}`); + if (!res.ok) { + throw new Error(`Client lookup failed (${res.status})`); + } const data = await getOidcClientInfoScehma.parseAsync(await res.json()); return data; }, });internal/config/config.go (1)
134-136: Clarify thatOIDCClientConfig.IDis derived from the map key.The description currently reads like a user‑supplied OIDC client ID. Consider updating the description to avoid confusion.
✍️ Suggested wording tweak
- ID string `description:"OIDC client ID." yaml:"-"` + ID string `description:"Internal client key (map key)." yaml:"-"`internal/bootstrap/app_bootstrap.go (1)
88-92: Stabilize OIDC client ordering.Map iteration order is nondeterministic; sorting yields stable UI/serialization order.
♻️ Suggested sorting
for id, client := range app.config.OIDC.Clients { client.ID = id app.context.oidcClients = append(app.context.oidcClients, client) } + sort.Slice(app.context.oidcClients, func(i, j int) bool { + if app.context.oidcClients[i].Name == app.context.oidcClients[j].Name { + return app.context.oidcClients[i].ID < app.context.oidcClients[j].ID + } + return app.context.oidcClients[i].Name < app.context.oidcClients[j].Name + })
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/pages/continue-page.tsx (1)
55-83: Missing dependency array causes effect to run on every render.Removing the dependency array means this effect runs after every render, repeatedly clearing and re-setting the timers. This can prevent the 100ms auto-redirect from firing if the component re-renders within that window.
Add the relevant dependencies explicitly:
🐛 Proposed fix
return () => { clearTimeout(auto); clearTimeout(reveal); }; - }); + }, [ + isLoggedIn, + isValidRedirectUri, + isAllowedRedirectProto, + isTrustedRedirectUri, + isHttpsDowngrade, + disableUiWarnings, + ]);Note:
handleRedirectshould either be wrapped inuseCallbackand added to the deps, or inlined in the effect to avoid re-triggering on every render.frontend/src/pages/login-page.tsx (2)
121-141:oauthMutationin dependency array may cause repeated triggers.
useMutationreturns a new object reference on each render. IncludingoauthMutationin the dependency array can cause the effect to re-run unintentionally. Since you're only callingoauthMutation.mutate()inside the effect, consider extracting themutatefunction or using a ref to avoid this issue.💡 Suggested fix
+ const oauthMutate = oauthMutation.mutate; + useEffect(() => { if ( providers.find((provider) => provider.id === oauthAutoRedirect) && !isLoggedIn && props.redirect_uri !== "" ) { // Not sure of a better way to do this // eslint-disable-next-line react-hooks/set-state-in-effect setOauthAutoRedirectHandover(true); - oauthMutation.mutate(oauthAutoRedirect); + oauthMutate(oauthAutoRedirect); redirectButtonTimer.current = window.setTimeout(() => { setShowRedirectButton(true); }, 5000); } }, [ providers, isLoggedIn, props.redirect_uri, oauthAutoRedirect, - oauthMutation, + oauthMutate, ]);
152-159: Missing OIDC handling in navigation guard for already-logged-in users.When a user is already logged in and arrives at the login page with OIDC parameters, they should be redirected to
/authorize(similar to the post-login success flow), not/continue. Currently, an already-authenticated user in an OIDC flow would be incorrectly routed.💡 Suggested fix
if (isLoggedIn && props.redirect_uri !== "") { + if (isOidc) { + return <Navigate to={`/authorize?${compiledOIDCParams}`} replace />; + } return ( <Navigate to={`/continue?redirect_uri=${encodeURIComponent(props.redirect_uri)}`} replace /> ); }
🤖 Fix all issues with AI agents
In `@frontend/src/pages/authorize-page.tsx`:
- Around line 51-53: The hardcoded English UI strings in the AuthorizePage need
to be replaced with i18n keys: import and use the useTranslation hook (t) in the
AuthorizePage component, replace toast.info("Authorized", { description: "You
will be soon redirected to your application" }) with
toast.info(t('authorize.authorized'), { description: t('authorize.redirecting')
}), wrap the LoadingCard title/description strings with
t('authorize.loadingTitle') / t('authorize.loadingDesc'), wrap the authorization
prompt text (the copy in the Authorization prompt block) with
t('authorize.prompt.*') keys, and wrap both button labels with
t('authorize.button.confirm') / t('authorize.button.cancel'); finally add
corresponding keys and translations for authorize.authorized,
authorize.redirecting, authorize.loadingTitle, authorize.loadingDesc,
authorize.prompt.*, authorize.button.confirm, and authorize.button.cancel to the
translation resource files.
- Around line 30-37: The useQuery for getClientInfo fires even when client_id is
empty and also parses responses without checking HTTP status; update the query
call (getClientInfo / useQuery) to include enabled: missingParams.length === 0
so it only runs when client_id is present, and modify the queryFn to check
res.ok after fetch (or switch to axios for consistency with the rest of the
codebase) and throw a clear error when the response is not ok before calling
getOidcClientInfoScehma.parseAsync on the JSON; reference props.client_id,
missingParams, getClientInfo, and getOidcClientInfoScehma when making these
changes.
In `@frontend/src/pages/error-page.tsx`:
- Around line 20-31: The CardDescription block renders a hardcoded English
message with a typo ("occured") and bypasses i18n; replace the hardcoded string
inside the conditional that uses the error variable by using the translation
hook (t) and fix the spelling — e.g. use t("errorOccurred") instead of the
literal "The following error occured while processing your request:"; add the
errorOccurred key to your i18n resource files and ensure the component
imports/uses useTranslation() consistently (refer to CardDescription and the
error variable in this JSX).
In `@internal/controller/oidc_controller.go`:
- Around line 356-359: The redirect URI is being formatted with an extra slash
("%s/?%s") causing URLs like "…/callback/?…"; update the c.JSON response to
build the URI without that slash—use fmt.Sprintf("%s?%s", callback,
queries.Encode()) (or trim any trailing slash from callback with
strings.TrimRight(callback, "/") before formatting) so the redirect_uri is
correctly formed; the change should be applied where c.JSON is called and where
callback and queries.Encode() are used.
In `@internal/service/oidc_service.go`:
- Around line 161-168: The pem.Decode result for the public key (call site using
pem.Decode(fpublicKey) with variable block) is not nil-checked before accessing
block.Bytes, causing a panic on malformed PEM; update the public key parsing
branch (where x509.ParsePKCS1PublicKey is called and service.publicKey is set)
to verify block != nil (and that block.Bytes is non-empty) and return a
descriptive error when decoding fails, before calling x509.ParsePKCS1PublicKey
and assigning service.publicKey.
- Around line 207-221: The scope validation currently splits req.Scope and
checks for empty/unsupported entries but does not enforce the required "openid"
scope; update the validation (in the function that processes scopes where
variables scopes, req.Scope and SupportedScopes are used) to ensure "openid" is
present: after building the scopes slice and trimming empty entries, check
whether "openid" exists (e.g., with slices.Contains or equivalent) and return
errors.New("invalid_scope") if it's missing; keep the existing unsupported-scope
warnings but treat absence of "openid" as a hard error.
- Around line 134-141: Add a nil check for the result of pem.Decode before
accessing block.Bytes: after calling pem.Decode(fprivateKey) verify that block
!= nil and return a descriptive error if it is nil (e.g., "invalid or missing
PEM block for private key"); then proceed to call x509.ParsePKCS1PrivateKey and
assign to service.privateKey as currently done. Ensure you reference the
existing variables and functions (pem.Decode, block, x509.ParsePKCS1PrivateKey,
service.privateKey, fprivateKey) so the check is added in the same branch where
the private key is parsed.
- Around line 433-435: The current logic sets userInfo.Groups =
strings.Split(user.Groups, ",") when slices.Contains(scopes, "groups"), but
strings.Split("") yields [""] instead of an empty slice; update the block that
populates userInfo.Groups to first check if strings.TrimSpace(user.Groups) == ""
and in that case assign an empty slice (e.g., nil or []string{}), otherwise
split on "," (and optionally trim elements) so groups serialize as [] instead of
[""]. Ensure you modify the conditional that references slices.Contains(scopes,
"groups") and set userInfo.Groups accordingly.
♻️ Duplicate comments (2)
internal/controller/oidc_controller.go (2)
240-267: Code consumption is still non-atomic, allowing potential replay attacks.The code entry is fetched on line 218, the access token is generated on line 249, and the code is deleted on line 259. This sequence allows a race condition where two concurrent token requests could both successfully exchange the same authorization code. This was flagged in a previous review and appears to still be present.
Consider implementing an atomic "consume and return" operation in the repository layer (e.g.,
DELETE ... RETURNINGor a transaction withSELECT FOR UPDATE).
240-247: Missing client ID validation on authorization code.The code validates that
redirect_urimatches but doesn't verify that the authorization code was issued to the requesting client. Add a check thatentry.ClientIDequals the authenticatedclient.ClientIDto prevent cross-client code exchange.✅ Proposed fix
+ if entry.ClientID != client.ClientID { + tlog.App.Warn().Str("expected", entry.ClientID).Str("actual", client.ClientID).Msg("Code was issued to different client") + c.JSON(400, gin.H{ + "error": "invalid_grant", + }) + return + } + if entry.RedirectURI != req.RedirectURI { tlog.App.Warn().Str("redirect_uri", req.RedirectURI).Msg("Redirect URI mismatch")
🧹 Nitpick comments (4)
frontend/src/lib/hooks/oidc.ts (1)
18-53: Consider renaming:useprefix implies React hook.This function doesn't call any React hooks internally—it's a pure utility that parses
URLSearchParams. Theuseprefix conventionally signals a React hook, which can mislead developers into thinking hook rules apply (e.g., not calling it conditionally).♻️ Suggested rename
-export function useOIDCParams(params: URLSearchParams): IuseOIDCParams { +export function parseOIDCParams(params: URLSearchParams): IuseOIDCParams {Update call sites accordingly (
parseOIDCParamsorgetOIDCParams).frontend/src/pages/login-page.tsx (1)
91-95: TOTP redirect uses OIDC params unconditionally.When
isOidcis false,compiledOIDCParamswill be an empty string (per the hook logic), resulting in a redirect to/totp?with no meaningful query string. While this works, it may be cleaner to conditionally include the params only when in OIDC mode, or at minimum include theredirect_urifor non-OIDC flows so the TOTP page can continue the original redirect.Consider whether the TOTP page needs
redirect_urifor non-OIDC flows:💡 Suggested improvement
if (data.data.totpPending) { - window.location.replace(`/totp?${compiledOIDCParams}`); + window.location.replace( + isOidc + ? `/totp?${compiledOIDCParams}` + : `/totp?redirect_uri=${encodeURIComponent(props.redirect_uri)}` + ); return; }internal/service/oidc_service.go (1)
391-396: Useerrors.Isfor error comparison consistency.Line 392 uses direct equality (
err == sql.ErrNoRows) while line 295 correctly useserrors.Is(err, sql.ErrNoRows). Usingerrors.Isis the idiomatic approach and handles wrapped errors.💡 Suggested fix
if err != nil { - if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { return repository.OidcToken{}, ErrTokenNotFound } return repository.OidcToken{}, err }internal/controller/oidc_controller.go (1)
295-308: Useerrors.Isfor sentinel error comparison.Line 296 uses direct equality comparison (
err == service.ErrTokenNotFound). While this works for package-level sentinel errors, usingerrors.Isis the idiomatic approach and handles wrapped errors consistently.💡 Suggested fix
if err != nil { - if err == service.ErrTokenNotFound { + if errors.Is(err, service.ErrTokenNotFound) { tlog.App.Warn().Msg("OIDC userinfo accessed with invalid token") c.JSON(401, gin.H{ "error": "invalid_request", }) return } - tlog.App.Err(err).Msg("Failed to get token entry") + tlog.App.Error().Err(err).Msg("Failed to get token entry") c.JSON(401, gin.H{ "error": "server_error", }) return }
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@frontend/src/lib/i18n/locales/en.json`:
- Line 64: The locale string "authorizeSubtitle" currently states the app will
access "your email and other information" which may be inaccurate; update the UI
to avoid misleading users by either (a) making the message dynamic based on the
actual requested scopes/claims (detect requestedScopes/claims in the auth flow
and choose a localized key like authorizeSubtitle_email or
authorizeSubtitle_scopes with the specific scope names injected), or (b) replace
the static string for "authorizeSubtitle" with a neutral, scope-agnostic
phrasing (e.g., "This app will access information from your account.") and add
new localized keys if you implement per-scope messaging. Ensure the component
that renders authorizeSubtitle reads the appropriate localized key based on the
detected requestedScopes/claims.
In `@internal/assets/migrations/000005_oidc_session.up.sql`:
- Around line 1-17: The "sub" column in the oidc_codes and oidc_tokens tables is
marked UNIQUE which prevents multiple active codes/tokens per subject; update
the schema for tables oidc_codes and oidc_tokens by removing the UNIQUE
constraint from the "sub" column (or alternatively replace it with a composite
unique index on (sub, client_id) if you want to enforce one-per-client),
ensuring "code_hash" and "access_token_hash" remain primary/unique as defined;
modify the CREATE TABLE statements for oidc_codes and oidc_tokens to drop UNIQUE
on "sub" (or add the composite uniqueness) so multiple codes/tokens per subject
across clients/sessions are allowed.
In `@internal/bootstrap/service_bootstrap.go`:
- Around line 92-106: The OIDC service is being initialized unconditionally and
will fail if app.config.OIDC.PrivateKeyPath or app.config.OIDC.PublicKeyPath
(and generally app.config.OIDC) are empty; change the bootstrap to check whether
OIDC is configured before calling service.NewOIDCService and oidcService.Init():
only construct oidcService and assign services.oidcService if the config exists
and required fields (e.g., PrivateKeyPath and PublicKeyPath or a top-level
enabled flag) are present; otherwise log a warning and skip initialization so
startup continues when OIDC is not provided.
In `@internal/service/oidc_service.go`:
- Around line 426-433: The TokenResponse is returning the old refreshToken
instead of the newly generated one; update the TokenResponse construction in
TokenResponse (where TokenResponse is built) to set RefreshToken to
newRefreshToken (the value persisted to the DB) instead of the input parameter
refreshToken so clients receive the fresh token that matches the stored value.
- Around line 382-389: The CreateOidcToken call is missing the ClientID field on
the CreateOidcTokenParams struct, so tokens are not associated with a client;
update the call to include ClientID (e.g., ClientID: clientID or Client.ID
depending on the surrounding scope) alongside Sub, AccessTokenHash,
RefreshTokenHash, Scope, TokenExpiresAt and RefreshTokenExpiresAt in the
CreateOidcToken invocation in service.queries.CreateOidcToken so the created
token record stores the client identifier.
- Around line 586-593: The code calls the nonexistent method
service.queries.DeleteSession(ctx, expiredCode.Sub); replace that call with the
existing service-level cleanup method service.DeleteOldSession(ctx,
expiredCode.Sub) (or, if you prefer direct queries, call the appropriate
repository methods such as
queries.DeleteSessionBySubject/queries.DeleteRefreshToken by subject), and keep
the same error handling (tlog.App.Warn().Err(err).Msg("Failed to delete
session")) so the code compiles and cleanup runs correctly.
In `@sql/oidc_schemas.sql`:
- Around line 1-17: The UNIQUE constraint on "sub" in tables oidc_codes and
oidc_tokens will prevent multiple concurrent codes/tokens for the same user;
remove the UNIQUE attribute from the "sub" column in both CREATE TABLE
statements (or alternatively make the uniqueness composite by scoping to
"client_id", e.g., UNIQUE(sub, client_id)), and update the migration/DML to
match the changed schema so existing DBs and future migrations reflect the new
constraint.
♻️ Duplicate comments (4)
frontend/src/lib/i18n/locales/en-US.json (1)
64-64: Same scope/claim accuracy concern as en.json.Ensure this subtitle reflects the actual scopes/claims granted, or make it neutral; keep both locales aligned.
internal/controller/oidc_controller.go (2)
234-265: Missing validation that the authorization code was issued to the requesting client.The token exchange flow retrieves the code entry and validates redirect URI, but doesn't verify that
entry.ClientIDmatches the authenticated client (rclientId). This allows a malicious client to exchange codes issued to other clients.✅ Proposed fix
entry, err := controller.oidc.GetCodeEntry(c, controller.oidc.Hash(req.Code)) if err != nil { // ... existing error handling } + if entry.ClientID != rclientId { + tlog.App.Warn().Str("expected_client", entry.ClientID).Str("actual_client", rclientId).Msg("Code issued to different client") + c.JSON(400, gin.H{ + "error": "invalid_grant", + }) + return + } + if entry.RedirectURI != req.RedirectURI {
275-285: Authorization code consumption is not atomic, allowing potential replay attacks.The current flow reads the code (
GetCodeEntry), generates tokens, then deletes the code. Two concurrent requests could both read the same valid code and mint tokens before either deletion occurs.Consider using a
DELETE ... RETURNINGquery (already defined as a pattern in the SQL file) or wrap in a transaction to atomically consume the code.internal/service/oidc_service.go (1)
223-237: Missing validation thatopenidscope is required.Per the OpenID Connect specification, the
openidscope value MUST be present in authorization requests. The current validation warns about unsupported scopes but doesn't enforce thatopenidis included.✅ Proposed fix
// Scopes scopes := strings.Split(req.Scope, " ") if len(scopes) == 0 || strings.TrimSpace(req.Scope) == "" { return errors.New("invalid_scope") } + hasOpenID := false for _, scope := range scopes { if strings.TrimSpace(scope) == "" { return errors.New("invalid_scope") } + if scope == "openid" { + hasOpenID = true + } if !slices.Contains(SupportedScopes, scope) { tlog.App.Warn().Str("scope", scope).Msg("Unsupported OIDC scope, will be ignored") } } + + if !hasOpenID { + return errors.New("invalid_scope") + }
🧹 Nitpick comments (6)
Makefile (1)
21-23: Declareclean-dataas.PHONYand userm --for safety.If a file named
clean-dataexists, the target won’t run. Adding.PHONYprevents that. Usingrm -rf -- data/avoids treating a path starting with-as a flag.♻️ Proposed tweak
+# Clean data +.PHONY: clean-data clean-data: - rm -rf data/ + rm -rf -- data/internal/controller/oidc_controller.go (3)
117-122:errisnilwhen passed toauthorizeError.When
okisfalse,errfrom the previous successfulBindJSONcall isnil. PassingniltoauthorizeErrorand then logging it won't provide useful context.♻️ Suggested fix
_, ok := controller.oidc.GetClient(req.ClientID) if !ok { - controller.authorizeError(c, err, "Client not found", "The client ID is invalid", "", "", "") + controller.authorizeError(c, errors.New("client not found"), "Client not found", "The client ID is invalid", "", "", "") return }
352-366: Useerrors.Is()for sentinel error comparison.Direct equality comparison (
==) can fail if the error is wrapped. Useerrors.Is()for consistent error checking.♻️ Suggested fix
if err != nil { - if err == service.ErrTokenNotFound { + if errors.Is(err, service.ErrTokenNotFound) { tlog.App.Warn().Msg("OIDC userinfo accessed with invalid token") c.JSON(401, gin.H{ "error": "access_denied", }) return } - tlog.App.Err(err).Msg("Failed to get token entry") + tlog.App.Error().Err(err).Msg("Failed to get token entry") c.JSON(401, gin.H{ "error": "server_error", }) return }
377-385: Consistent logging pattern.Similar to line 361, use
Error().Err(err)for consistency.♻️ Suggested fix
if err != nil { - tlog.App.Err(err).Msg("Failed to get user entry") + tlog.App.Error().Err(err).Msg("Failed to get user entry") c.JSON(401, gin.H{ "error": "server_error", }) return }internal/service/oidc_service.go (2)
400-406: Useerrors.Is()for sentinel error comparison.Direct equality comparison may fail if errors are wrapped.
♻️ Suggested fix
if err != nil { - if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { return TokenResponse{}, ErrTokenNotFound } return TokenResponse{}, err }Apply the same pattern at lines 466 and 582.
371-371: Minor typo in variable name.
refrshTokenExpiresAtis missing an 'e' - should berefreshTokenExpiresAtfor consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/controller/oidc_controller_test.go`:
- Around line 136-143: After calling http.NewRequest to create req, add an
assertion that err is nil before using req (e.g., assert.NilError(t, err));
specifically update the test around the http.NewRequest("POST",
"/api/oidc/token", ...) invocation so the subsequent req.Header.Set,
req.SetBasicAuth and router.ServeHTTP(recorder, req) only run if err is asserted
nil.
In `@internal/controller/oidc_controller.go`:
- Around line 265-283: The authorization-code consumption is non-atomic:
GetCodeEntry and DeleteCodeEntry are separate, allowing replay; change the
service layer to perform atomic consumption (either wrap
GetCodeEntry+DeleteCodeEntry in a DB transaction that verifies and deletes the
row before returning, or replace them with a single atomic operation such as a
DeleteCodeEntryReturning or ConsumeCode method that executes DELETE ...
RETURNING the row in one statement), then call that new atomic method from the
handler instead of calling GetCodeEntry and DeleteCodeEntry separately so
GenerateAccessToken receives a single-consumed entry.
🧹 Nitpick comments (6)
internal/controller/oidc_controller.go (3)
155-163: Preferstrings.Fieldsoverstrings.Splitfor scope parsing.
strings.Split(req.Scope, " ")will produce empty strings if there are multiple consecutive spaces or leading/trailing spaces.strings.Fieldshandles these edge cases gracefully.♻️ Suggested fix
- if slices.Contains(strings.Split(req.Scope, " "), "openid") { + if slices.Contains(strings.Fields(req.Scope), "openid") {
286-303: Consider documenting the different authentication methods for grant types.The
authorization_codegrant requires Basic Auth (lines 206-214), whilerefresh_tokenuses form body credentials (lines 287-303). This is RFC 6749 compliant but inconsistent. Consider either:
- Adding a comment explaining the design choice, or
- Accepting both methods consistently across grant types
352-359: Useerrors.Isfor error comparison.Line 353 uses
==for error comparison, but other error checks in this file (lines 236, 243, 308) useerrors.Is. Useerrors.Isconsistently for sentinel error comparison.♻️ Suggested fix
- if err == service.ErrTokenNotFound { + if errors.Is(err, service.ErrTokenNotFound) {internal/controller/oidc_controller_test.go (3)
35-38: Hardcoded/tmppaths may cause test flakiness and conflicts.Using fixed paths like
/tmp/tinyauth.dband/tmp/tinyauth_oidc_keycan cause test failures when:
- Tests run in parallel
- Previous test runs left stale files
- Multiple CI jobs run on the same machine
Consider using
t.TempDir()for test isolation:♻️ Suggested fix
func TestOIDCController(t *testing.T) { tlog.NewSimpleLogger().Init() tmpDir := t.TempDir() dbPath := filepath.Join(tmpDir, "tinyauth.db") privateKeyPath := filepath.Join(tmpDir, "tinyauth_oidc_key") publicKeyPath := filepath.Join(tmpDir, "tinyauth_oidc_key.pub") serviceConfig := service.OIDCServiceConfig{ // ... other fields ... PrivateKeyPath: privateKeyPath, PublicKeyPath: publicKeyPath, // ... } // ... db, err := app.SetupDatabase(dbPath) // ... }
195-198: Address the commented-out groups assertion.The groups test is disabled with a comment indicating uncertainty about the failure cause. This should be investigated and either fixed or tracked as a known issue.
Would you like me to help investigate why the groups assertion is failing? The issue might be related to the JSON type assertion -
resJson["groups"].([]string)won't work because JSON unmarshaling intomap[string]anyproduces[]interface{}for arrays, not[]string.💡 Potential fix for the groups assertion
groupsRaw, ok := resJson["groups"].([]interface{}) assert.Assert(t, ok) var groups []string for _, g := range groupsRaw { groups = append(groups, g.(string)) } assert.DeepEqual(t, strings.Split(oidcTestContext.LdapGroups, ","), groups)
57-58: Consider adding negative test cases for robustness.The current test covers the happy path well. For production readiness, consider adding tests for:
- Invalid client ID/secret
- Expired authorization codes
- Invalid/missing scopes
- Redirect URI mismatch
- Invalid Bearer token in userinfo
This can be addressed in a follow-up PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/oidc_controller.go`:
- Around line 152-154: The scope parsing currently splits on literal spaces and
commas which can miss valid "openid" scopes; update both places where scopes are
inspected (e.g., the check around controller.oidc.StoreUserinfo where req.Scope
is used and the other usage around entry.Scope at lines ~339-345) to use
strings.Fields(req.Scope) and strings.Fields(entry.Scope) respectively so scopes
are normalized by whitespace and "openid" is reliably detected.
🧹 Nitpick comments (1)
sql/oidc_queries.sql (1)
18-22: Rename destructive “Get” queries to “Consume” for clarity.
These statements delete rows; the current name increases misuse risk. Consider renaming and regenerating sqlc output.♻️ Proposed refactor (SQL source)
- -- name: GetOidcCode :one + -- name: ConsumeOidcCode :one ... - -- name: GetOidcCodeBySub :one + -- name: ConsumeOidcCodeBySub :oneAlso applies to: 27-31
|
Let's test our luck @coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@frontend/vite.config.ts`:
- Around line 27-31: The rewrite regex in the proxy rule (the rewrite function
used in the "/.well-known" proxy config where path.replace is called) uses
/^\/.well-known/ which treats the dot as a wildcard; change the pattern to
escape the dot so it only matches the literal '.' (i.e., use a regex that
escapes the dot in the ".well-known" portion) so path.replace only strips the
exact prefix "/.well-known".
In `@internal/controller/well_known_controller.go`:
- Around line 3-8: The OpenIDConnectIssuer value can end with a slash causing
double-slashes in derived endpoints; normalize it by trimming any trailing '/'
before constructing well-known URLs. Update the code paths that use
OpenIDConnectIssuer in the well-known controller (e.g., the handler/functions
that build the .well-known endpoints) to call strings.TrimRight or
strings.TrimSuffix on OpenIDConnectIssuer once (or when reading config) so every
constructed URL uses the normalized issuer without a trailing slash. Ensure
tests/usage that concatenate suffixes (like "/.well-known/..." or "/oauth2/..."
) rely on the normalized issuer to avoid "//" in output.
- Around line 56-59: The configuration advertises "pairwise" in
SubjectTypesSupported but the code only returns public subjects; either remove
"pairwise" from the SubjectTypesSupported slice (update the
SubjectTypesSupported value to exclude "pairwise") or implement full pairwise
subject support per OIDC Core §8.1: add sector_identifier/sector_identifier_uri
handling on client registration, validate sector identifiers against redirect
URIs, and compute persistent per-client subject identifiers (e.g., HMAC/hashed
value of user id + sector_identifier + server secret) wherever sub is issued
(IDs in token response and userinfo) — modify the logic that issues subject
claims (wherever user sub is generated) to return the computed pairwise
identifier when the client’s subject_type is "pairwise".
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/oidc_controller.go`:
- Around line 232-274: The authorization code is not invalidated after a
successful exchange, allowing replay; after successfully fetching the code entry
via controller.oidc.GetCodeEntry(controller.oidc.Hash(req.Code)) and
before/after calling controller.oidc.GenerateAccessToken, call the service
method that deletes or marks the code consumed (e.g., controller.oidc.DeleteCode
or a ConsumeCode/ConsumeAuthorizationCode function) using the same hashed code
or entry.ID so the code cannot be reused; ideally perform this as an atomic
consume (DELETE ... RETURNING or a single ConsumeCode call that both validates
existence/expiry and deletes) to avoid race conditions—update the token exchange
flow to invoke that invalidate/consume method and handle its errors
appropriately.
🧹 Nitpick comments (2)
internal/service/oidc_service.go (1)
572-625: Consider adding graceful shutdown support for the cleanup routine.The cleanup goroutine has no mechanism to stop when the application shuts down. Consider accepting a
context.Contextparameter to support cancellation:♻️ Suggested improvement
-func (service *OIDCService) Cleanup() { - ctx := context.Background() +func (service *OIDCService) Cleanup(ctx context.Context) { ticker := time.NewTicker(time.Duration(30) * time.Minute) defer ticker.Stop() - for range ticker.C { + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + } // ... cleanup logic } }internal/controller/well_known_controller.go (1)
67-85: Consider using proper JSON marshaling for the JWKS response.The manual string concatenation for building the JSON response is fragile and could break if the JWK contains characters that need escaping. Consider using proper JSON marshaling:
♻️ Suggested improvement
func (controller *WellKnownController) JWKS(c *gin.Context) { jwks, err := controller.oidc.GetJWK() if err != nil { c.JSON(500, gin.H{ "status": "500", "message": "failed to get JWK", }) return } - c.Header("content-type", "application/json") - - c.Writer.WriteString(`{"keys":[`) - c.Writer.Write(jwks) - c.Writer.WriteString(`]}`) - - c.Status(http.StatusOK) + var jwk map[string]interface{} + if err := json.Unmarshal(jwks, &jwk); err != nil { + c.JSON(500, gin.H{ + "status": "500", + "message": "failed to parse JWK", + }) + return + } + + c.JSON(200, gin.H{ + "keys": []interface{}{jwk}, + }) }
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/pages/continue-page.tsx (1)
55-83: Add dependency array to useEffect to prevent repeated timer scheduling.Line 83 omits the dependency array, causing the effect to run after every render. This repeatedly schedules new 100ms and 5s timers, clearing the previous ones, which can delay the automatic redirect and create unexpected behavior.
🛠️ Proposed fix
return () => { clearTimeout(auto); clearTimeout(reveal); }; - }); + }, []);frontend/src/pages/login-page.tsx (1)
152-159: Logged-in users with OIDC params should redirect to/authorize, not/continue.When
isLoggedInis true and OIDC params are present, the current code redirects to/continue. However, for OIDC flows, the user should be redirected to/authorizeto complete the authorization consent flow, similar to the logic inloginMutation.onSuccess(lines 102-108).🐛 Proposed fix
if (isLoggedIn && props.redirect_uri !== "") { + if (isOidc) { + return <Navigate to={`/authorize?${compiledOIDCParams}`} replace />; + } return ( <Navigate to={`/continue?redirect_uri=${encodeURIComponent(props.redirect_uri)}`} replace /> ); }
🤖 Fix all issues with AI agents
In `@frontend/src/lib/hooks/oidc.ts`:
- Around line 39-45: The code currently sets compiled = new
URLSearchParams(values).toString() when isOidc is true, which rebuilds the query
string from only the validated subset and drops optional OIDC params; change
this to build the query string from the original params object (or the variable
holding the full input params) so that optional fields like nonce,
code_challenge, response_mode, prompt, etc. are preserved while still keeping
the required-parameter validation that uses missingParams/isOidc intact; locate
the block that sets isOidc and compiled (referencing missingParams, isOidc,
compiled, and values) and replace the URLSearchParams source to use the full
params object instead of values.
In `@frontend/src/pages/authorize-page.tsx`:
- Line 76: The current assignment const scopes = props.scope.split(" ") can
produce [""] when props.scope is an empty string; change the initialization of
scopes so that if props.scope is falsy or an empty string you get an empty
array, otherwise split on whitespace and filter out empty/blank segments (i.e.,
use props.scope to produce an array via splitting on whitespace and
.filter(Boolean)); update any logic that uses scopes (e.g.,
scopes.includes("openid") or mapping over scopes) to rely on this normalized
non-empty array; refer to the identifiers scopes and props.scope (originating
from useOIDCParams) when making the change.
In `@frontend/src/schemas/oidc-schemas.ts`:
- Around line 3-5: Rename the exported schema identifier getOidcClientInfoScehma
to getOidcClientInfoSchema to fix the typo; update the export declaration in the
OIDC schema file (the z.object export) and rename all references/usages of
getOidcClientInfoScehma throughout the codebase to getOidcClientInfoSchema so
imports and consumers remain consistent.
In `@internal/controller/oidc_controller_test.go`:
- Around line 177-183: Test constructs an HTTP request with http.NewRequest and
assigns the returned error to err but never asserts err is nil before using req;
update the test in internal/controller/oidc_controller_test.go to check the
error immediately after calling http.NewRequest (e.g., assert.NoError or
require.NoError on err) so you don't pass a nil/invalid req into req.Header.Set,
req.SetBasicAuth, or router.ServeHTTP (references: variables req and err from
http.NewRequest, and subsequent calls to req.Header.Set, req.SetBasicAuth, and
router.ServeHTTP(recorder, req)).
In `@internal/controller/oidc_controller.go`:
- Around line 275-294: The refresh_token branch currently calls
controller.oidc.RefreshAccessToken(c, req.RefreshToken) without verifying the
token is bound to the authenticated client; update the flow to obtain the
authenticated client ID from Basic Auth (e.g., client.ClientID) and pass it into
the refresh logic (or call a new method like RefreshAccessTokenForClient) so
RefreshAccessToken can validate the token's client binding and return an
explicit error on mismatch; on mismatch return the same invalid_grant/401
response (or a 400 server_error where appropriate) while preserving existing
ErrTokenExpired handling.
In `@internal/service/oidc_service.go`:
- Around line 214-221: GetIssuer currently returns service.config.Issuer instead
of the normalized value set in Init(), causing mismatch; update GetIssuer to
return the normalized issuer field (service.issuer) so the OIDCService uses a
single canonical issuer everywhere (reference: OIDCService.Init,
OIDCService.GetIssuer, service.issuer, service.config.Issuer).
🧹 Nitpick comments (6)
internal/middleware/ui_middleware.go (1)
43-43: Consider removing or gating this debug log.This logs every incoming request path, which could be verbose in high-traffic environments. If this was added for debugging during development, consider removing it before merging, or ensure it's only emitted at debug level (which it is) and that debug logging is disabled in production configurations.
frontend/src/pages/totp-page.tsx (1)
45-52: Minor: unnecessaryreturnandelseclause.The
returnon line 47 is unreachable afterwindow.location.replace(), and since it exits the block, theelseon line 48 is redundant. This is purely stylistic and doesn't affect behavior.✏️ Optional cleanup
redirectTimer.current = window.setTimeout(() => { if (isOidc) { window.location.replace(`/authorize?${compiledOIDCParams}`); - return; - } else { - window.location.replace( - `/continue?redirect_uri=${encodeURIComponent(props.redirect_uri)}`, - ); + } else { + window.location.replace( + `/continue?redirect_uri=${encodeURIComponent(props.redirect_uri)}`, + ); } }, 500);Or simply remove the
return:if (isOidc) { window.location.replace(`/authorize?${compiledOIDCParams}`); - return; } else {internal/assets/migrations/000005_oidc_session.up.sql (1)
3-3: Minor:PRIMARY KEYalready impliesUNIQUE.The
UNIQUEkeyword afterPRIMARY KEYoncode_hash(line 3) andaccess_token_hash(line 12) is redundant. SQLite primary keys are inherently unique.✏️ Optional cleanup
- "code_hash" TEXT NOT NULL PRIMARY KEY UNIQUE, + "code_hash" TEXT NOT NULL PRIMARY KEY, ... - "access_token_hash" TEXT NOT NULL PRIMARY KEY UNIQUE, + "access_token_hash" TEXT NOT NULL PRIMARY KEY,Also applies to: 12-12
internal/controller/oidc_controller_test.go (1)
35-36: Hardcoded/tmppaths may cause issues in parallel test runs.Using fixed paths like
/tmp/tinyauth_oidc_keyand/tmp/tinyauth.dbcould cause test interference if tests run in parallel. Consider usingt.TempDir()for test isolation.♻️ Suggested improvement
func TestOIDCController(t *testing.T) { tlog.NewSimpleLogger().Init() + tmpDir := t.TempDir() // Create an app instance app := bootstrap.NewBootstrapApp(config.Config{}) // Get db - db, err := app.SetupDatabase("/tmp/tinyauth.db") + db, err := app.SetupDatabase(filepath.Join(tmpDir, "tinyauth.db")) assert.NilError(t, err)And update the service config:
-var oidcServiceConfig = service.OIDCServiceConfig{ +func getOIDCServiceConfig(tmpDir string) service.OIDCServiceConfig { + return service.OIDCServiceConfig{ Clients: map[string]config.OIDCClientConfig{ // ... }, - PrivateKeyPath: "/tmp/tinyauth_oidc_key", - PublicKeyPath: "/tmp/tinyauth_oidc_key.pub", + PrivateKeyPath: filepath.Join(tmpDir, "tinyauth_oidc_key"), + PublicKeyPath: filepath.Join(tmpDir, "tinyauth_oidc_key.pub"), // ... + } }Also applies to: 65-65
internal/config/config.go (1)
136-142: ClarifyOIDCClientConfig.IDdescription to avoid confusion withClientID.Line 137 labels
IDas “OIDC client ID”, butIDis the config map key whileClientIDis the OIDC client_id. Consider clarifying the description.✏️ Suggested tweak
- ID string `description:"OIDC client ID." yaml:"-"` + ID string `description:"Config key (internal id) for this client." yaml:"-"`internal/repository/oidc_queries.sql.go (1)
276-352: Rename destructive “Get” queries to avoid surprises.Line 276 and Line 296 use
DELETE ... RETURNINGunderGetOidcCode*. The name implies a read-only fetch, which is easy to misuse. Consider renaming these queries toConsumeOidcCode*and reservingGet*for SELECTs.♻️ Suggested direction (sql/oidc_queries.sql)
--- name: GetOidcCode :one +-- name: ConsumeOidcCode :one DELETE FROM "oidc_codes" WHERE "code_hash" = ? RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_at --- name: GetOidcCodeBySub :one +-- name: ConsumeOidcCodeBySub :one DELETE FROM "oidc_codes" WHERE "sub" = ? RETURNING sub, code_hash, scope, redirect_uri, client_id, expires_atThen regenerate sqlc and update call sites.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/lib/i18n/locales/en.json`:
- Around line 54-78: The non-English locale files are missing 17 new translation
keys added to en.json; update each of the 29 locale JSON files to include the
following keys (using en.json as the source): authorizeCardTitle,
authorizeLoadingSubtitle, authorizeLoadingTitle, authorizeSubtitle,
authorizeSubtitleOAuth, authorizeSuccessSubtitle, authorizeSuccessTitle,
authorizeTitle, emailScopeDescription, emailScopeName, errorSubtitleInfo,
groupsScopeDescription, groupsScopeName, openidScopeDescription,
openidScopeName, profileScopeDescription, and profileScopeName; add each key
with an appropriate translated string (or placeholder copy of the English value)
into every locale file so the authorization and error messages are present
across all languages, ensuring JSON syntax and key order/structure match the
existing locale files.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@frontend/src/pages/authorize-page.tsx`:
- Around line 112-123: The OIDC parameter validation is happening after the
isLoggedIn check, causing unauthenticated users with bad requests to be
redirected to login unnecessarily; in authorize-page.tsx move the missingParams
check above the isLoggedIn branch so that if missingParams.length > 0 you
immediately return the error Navigate (the existing to={`/error?...`} logic)
before evaluating isLoggedIn/compiledOIDCParams, preserving current Navigate
behavior and avoiding a login redirect for malformed requests.
In `@frontend/src/pages/login-page.tsx`:
- Around line 92-94: The TOTP handoff currently uses compiledOIDCParams when
data.data.totpPending triggers
window.location.replace(`/totp?${compiledOIDCParams}`), but compiledOIDCParams
is empty for non-OIDC flows so the original redirect_uri is lost; update the
redirect construction to include the original non-OIDC redirect target when
isOidc is false (e.g., read the existing redirect_uri from the current URL or
the local redirect variable and append `redirect_uri=...` to the query), so that
the totp page receives a valid redirect_uri; adjust the logic around
data.data.totpPending and window.location.replace to build a query string that
uses compiledOIDCParams when isOidc is true otherwise includes the preserved
redirect_uri parameter.
In `@internal/controller/oidc_controller.go`:
- Around line 236-244: Remove the raw authorization code from the warning logs:
update the two tlog.App.Warn() calls that currently include req.Code (the "Code
not found" and "Code expired" branches) so they do not log req.Code; instead log
a generic message (and optional non-secret context like request ID or client_id
if available) or a masked/hashed representation, and ensure you update both
occurrences and any other places referencing req.Code in log messages within the
same handler.
🧹 Nitpick comments (2)
internal/controller/oidc_controller_test.go (2)
3-10: Uset.TempDirfor DB/key paths to keep tests isolated.
Hardcoded/tmppaths can collide between runs and leave artifacts. Prefer a temp dir and override config inside the test.♻️ Proposed refactor
import ( "encoding/json" "fmt" "net/http" "net/http/httptest" "net/url" + "path/filepath" "strings" "testing" @@ func TestOIDCController(t *testing.T) { tlog.NewSimpleLogger().Init() // Create an app instance app := bootstrap.NewBootstrapApp(config.Config{}) // Get db - db, err := app.SetupDatabase("/tmp/tinyauth.db") + tmpDir := t.TempDir() + db, err := app.SetupDatabase(filepath.Join(tmpDir, "tinyauth.db")) assert.NilError(t, err) @@ - oidcService := service.NewOIDCService(oidcServiceConfig, queries) + cfg := oidcServiceConfig + cfg.PrivateKeyPath = filepath.Join(tmpDir, "tinyauth_oidc_key") + cfg.PublicKeyPath = filepath.Join(tmpDir, "tinyauth_oidc_key.pub") + oidcService := service.NewOIDCService(cfg, queries)Also applies to: 23-38, 65-74
218-219: Remove the “Not sure why this is failing” comment or track it explicitly.
Stray uncertainty in tests makes failures harder to triage. Either fix the underlying issue or convert this to a TODO with a ticket reference.🛠️ Proposed fix
- // Not sure why this is failing, will look into it later
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/pages/authorize-page.tsx`:
- Around line 138-145: Replace the hardcoded error string in the
getClientInfo.isError branch with a localized translation call (e.g., use the
existing i18n hook/translator such as t) instead of the literal "Failed to load
client information"; update the Navigate URL to
encodeURIComponent(t('authorizePage.failedToLoadClientInfo')) (or the
appropriate translation key) and ensure the component imports/uses the
translation hook (e.g., useTranslation) so t is available for use in that
conditional rendering block where getClientInfo.isError is handled.
🧹 Nitpick comments (2)
frontend/src/pages/authorize-page.tsx (1)
150-154: Consider translating the "Unknown" fallback.The fallback string
"Unknown"at line 152 should use a translation key for full i18n support.🌐 Proposed fix
<CardTitle className="text-3xl"> {t("authorizeCardTitle", { - app: getClientInfo.data?.name || "Unknown", + app: getClientInfo.data?.name || t("unknownApp"), })} </CardTitle>frontend/src/pages/login-page.tsx (1)
137-143: IncludingoauthMutationin deps may cause unnecessary effect re-runs.While the current guards (
isLoggedIn,props.redirect_uri) prevent repeated mutations, including the entireoauthMutationobject in the dependency array is non-idiomatic. Consider usingoauthMutation.mutateor a ref to avoid potential issues.
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.