feat: Replace Zitadel with Logto as OIDC identity provider#415
Conversation
- Adapt OIDC token validation to handle Logto's opaque and JWT access tokens
- Add claim normalization for Logto's different field names (username, name)
- Extract roles from JWT access tokens via Custom JWT claims
- Support OIDC_RESOURCE config for Logto API resource (JWT access tokens)
- Make OIDCUser fields (family_name, given_name, preferred_username) optional
- Update upsertSelf to use ctx.oidc.user instead of userinfo endpoint
- Handle Logto role objects ({name, id, ...}) in addition to string arrays
- Remove Zitadel-specific token introspection, token exchange types, and error messages
- Add Mailpit SMTP credentials for Logto email connector
- Update mock OIDC landing page with new roles claim format
- Add migration scripts for Zitadel→Logto user ID remapping
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Replace the static login information card with an interactive account management UI that deep-links to Logto's Account Center for credential changes (email, password, username, passkeys, authenticator app, backup codes). Display MFA status, connected social/SSO identities, and password state from Logto Custom JWT claims. Improve responsive layout with two-column grid on larger screens. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Update all documentation to recommend Logto instead of Zitadel, remove Zitadel-specific OIDC scopes and role claims from .env.example. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…cs to .env.example Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Logto requires a two-step impersonation flow: first create a subject token via the Management API, then exchange it at the OIDC token endpoint. Also decode the resource-scoped JWT directly instead of calling userinfo (which rejects resource-scoped tokens), and look up profile claims from the database. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
📝 WalkthroughWalkthroughReplaces ZITADEL-focused OIDC guidance with Logto, adds Logto M2M/subject-token impersonation flow and claim normalization, relaxes several profile fields to nullable, adds Prisma migration/seed scripts, a migration-notice UI/flow, i18n updates, and minor CI/config adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ManagementAPI as Logto Management API
participant OIDCProvider as OIDC Provider
participant Database
rect rgba(100,150,200,0.5)
Note over Server,ManagementAPI: New Logto M2M subject-token impersonation flow
Client->>Server: Impersonation request (impersonation JWT)
Server->>Server: jwtVerify()/decode impersonation token
Server->>Database: fetch user by sub
Server->>Server: validate actor claim vs session
Server->>ManagementAPI: request M2M access token (client_id/secret)
ManagementAPI-->>Server: m2m_access_token
Server->>ManagementAPI: create subject token (POST /api/subject-tokens with sub)
ManagementAPI-->>Server: subject_token
Server->>OIDCProvider: token exchange using subject_token (+resource if configured)
OIDCProvider-->>Server: exchanged tokens
Server->>Database: record/session for impersonation
end
sequenceDiagram
participant Client
participant Server
participant OIDCIssuer as OIDC Issuer
participant Database
rect rgba(100,200,100,0.5)
Note over Server,OIDCIssuer: Claim normalization & relaxed profile handling
Client->>Server: Login / token refresh
Server->>OIDCIssuer: verify id_token, decode access_token
OIDCIssuer-->>Server: id_token + access_token claims
Server->>Server: normalizeOIDCClaims() (username→preferred_username, name→given/family)
Server->>Server: ensure sub + email present
Server->>Database: upsert user (nullable name fields accepted)
Database-->>Server: user row created/updated
Server->>Client: session / offline user response including new fields
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (1)
src/routes/(authenticated)/my-account/+page.server.ts (1)
62-65: Build the fallback account URL viaURLinstead of string concatenation.The current concatenation can produce malformed paths in edge cases (for example, trailing slash/path variants). Using
URLkeeps this robust.♻️ Suggested refactor
- const accountCenterUrl = - configPublic.PUBLIC_OIDC_ACCOUNT_URL ?? - configPublic.PUBLIC_OIDC_AUTHORITY.replace(/\/oidc\/?$/, '') + '/account'; + const accountCenterUrl = + configPublic.PUBLIC_OIDC_ACCOUNT_URL ?? + (() => { + const authority = new URL(configPublic.PUBLIC_OIDC_AUTHORITY); + authority.pathname = authority.pathname.replace(/\/oidc\/?$/, '/'); + return new URL('account', authority).toString(); + })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/my-account/+page.server.ts around lines 62 - 65, The fallback accountCenterUrl building currently uses string concatenation which can create malformed URLs; update the logic that sets accountCenterUrl to use the URL API instead of concatenation: if configPublic.PUBLIC_OIDC_ACCOUNT_URL is present use it, otherwise construct a new URL('/account', configPublic.PUBLIC_OIDC_AUTHORITY.replace(/\/oidc\/?$/, '')) (or equivalent using new URL with eventUrl.origin as needed) so paths are joined correctly; keep the existing accountRedirectUrl (`${eventUrl.origin}/my-account`) behavior unchanged or likewise build it via new URL('/my-account', eventUrl.origin) for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 39-43: The .env.example redirect URI is incorrect: update the
documentation to instruct registering the redirect URI as
<your-domain>/auth/login-callback to match the actual callback used by the code
(startSignin() in src/api/services/OIDC.ts); change the text that currently says
<your-domain>/auth/callback to <your-domain>/auth/login-callback so operators'
provider redirect URI matches the startSignin() behavior.
In `@prisma/seed/createMissingLogtoUsers.ts`:
- Around line 37-89: The Logto user creation (the POST to
`${LOGTO_ENDPOINT}/api/users` inside the for (const user of users) loop
producing response/body and logtoId) is non-idempotent and runs before the
Prisma transaction (db.$transaction) so failures leave orphaned Logto users;
change the flow to perform local preflight and use an existing Logto user if
present or move the Logto creation until after the transaction succeeds, or if
you must create it before the DB changes implement a compensating delete of that
Logto user (using logtoId) inside the catch/failure path of the db.$transaction
block; reference the variables/functions response, body, logtoId,
db.$transaction, tx.user.create, and tx.user.delete to locate where to add the
preflight check or compensating delete.
- Line 61: The progress logs in createMissingLogtoUsers.ts currently print raw
PII (user.email and identity provider IDs) to the console (see the
console.error/console.log calls that include user.email and any provider ID
values); change those log statements to avoid emitting full emails or stable
provider IDs by replacing them with a non-PII identifier (e.g., user.id, a
truncated/masked email like user.email.replace(/(.{2}).+(@.+)/, '$1***$2'), or a
short hash of the email) and remove or mask any provider ID strings before
logging, preserving the original message context (body.message or JSON) but not
the raw PII.
- Around line 58-65: Add a runtime type-narrowing guard before reading body.id:
after parsing body from response, verify body is a non-null object and that
typeof body.id === 'string' (and optionally typeof body.message === 'string' if
you access it); if the check fails, log an error including the raw body and
continue the loop instead of assigning to logtoId or performing DB operations.
Update the block around the existing response/body logic (where response.json()
is assigned and logtoId is set) to enforce this guard and handle the
invalid-response path safely.
In `@prisma/seed/migrateOidcSubs.ts`:
- Line 57: The stdout uses raw identifiers (mapping.identifier and
mapping.zitadel_id) which may expose emails and provider subject IDs; replace
those console.log calls in migrateOidcSubs.ts with masked versions (e.g., show
only first 1–3 chars + redacted placeholder or hash) or emit counts-only
messages; update both the log that currently prints mapping.identifier and
mapping.zitadel_id and the similar log later in the file to use a
mask/obfuscation helper (e.g., maskEmail/maskId) or a summarized message so no
full email or subject ID is written to stdout.
- Around line 51-87: Before calling tx.user.create with mapping.logto_id, detect
if a target user already exists and handle it instead of blindly creating—use
db.user.findUnique or tx.user.findUnique to check for a row with id ===
mapping.logto_id; if found, either move FK references to that existing target
and delete the source (perform the same FK UPDATE loop and source delete) or
skip this mapping and increment skipped, and only create the temporary/migrating
row when no target exists; update the logic in the block around tx.user.create /
tx.$executeRawUnsafe / tx.user.delete / tx.user.update to branch on the
existence of mapping.logto_id to avoid duplicate-key failures and half-completed
transactions.
In `@README.md`:
- Line 51: Fix the typo and wording in the README string that mentions the
example compose file: change "directoy" to "directory" and capitalize/format
"docker compose" as "Docker Compose" in the sentence that reads "You can find an
example docker compose file in the [example](./example/) directoy." Ensure the
updated sentence references the example directory and uses "Docker Compose"
consistently.
In `@schema.graphql`:
- Around line 2817-2819: OfflineUser now marks family_name and given_name
nullable, but fastUserQuery spreads OfflineUser into layout data and components
may assume non-null names; update the downstream consumers to defensively handle
nulls by coalescing or conditional-rendering: locate usages of
OfflineUser/family_name and OfflineUser/given_name in fastUserQuery results and
in components receiving layout data (search for fastUserQuery, layoutData
spread, and components that read family_name/given_name), then replace direct
access with safe fallbacks (e.g., use displayName = family_name ?? given_name ??
email ?? '' or conditionally render name blocks) so no runtime errors occur when
those fields are null while leaving ImpersonatedUser/ImpersonationUser handling
unchanged.
In `@src/api/context/oidc.ts`:
- Around line 123-137: Replace the inline role-parsing blocks that push into
OIDCRoleNames (the branches that read rolesRaw and the Object.keys path) with a
shared helper (e.g., parseOidcRoles or extractOidcRoleNames) that accepts
unknown rolesRaw and the allowed oidcRoles set/array and returns a string[] of
validated role names; inside the helper treat the input as unknown, narrow types
(Array.isArray, typeof element === 'string', element is object with typeof
element.name === 'string'), do NOT use any casts, extract the name string when
present, and filter the results against the allowed oidcRoles so only
'admin'|'member'|'service_user' are returned; replace both occurrences (user and
impersonated-user paths) to call this helper and append its returned values to
OIDCRoleNames.
- Around line 154-199: The code currently uses
decodeJwt(impersonationTokenSet.data.access_token) which does no signature
verification; replace this with a cryptographic verification using jwtVerify()
against the OIDC issuer JWKS (reuse the same JWKS fetching/verification logic
used for id_token verification in this file) to validate the impersonation
access_token before trusting its claims, then extract the verified payload and
use that to look up the DB user (symbols: decodeJwt -> jwtVerify,
impersonationTokenSet.data.access_token, issuer/JWKS fetch routine,
db.user.findUnique, impersonatedUser). Also ensure the actor check uses the
verified token payload (act.sub) and on verification failure or actor mismatch
delete the impersonation cookie (impersonationTokenCookieName), abort
impersonation and return the same token response path as currently implemented.
In `@src/api/resolvers/modules/auth.ts`:
- Around line 70-89: The code currently asserts custom JWT claims on
ctx.oidc.user into claims and casts values directly into hasPassword,
mfaVerificationFactors, ssoIdentities, and socialIdentities; add runtime
validation instead of blind casts by implementing small type guards (e.g.,
isBooleanClaim, isStringArrayClaim, isSsoIdentitiesArrayClaim that checks
objects have issuer and identityId strings) and use them when reading claims
from ctx.oidc.user/claims so that invalid shapes set the GraphQL fields to null
(or a safe default) rather than casting; alternatively, if you choose the
type-level fix, update the OIDCUser type to explicitly declare password, mfa,
sso_identities, and social_identities so the compiler enforces shapes at the
source (referencing ctx.oidc.user, claims, hasPassword, mfaVerificationFactors,
ssoIdentities, socialIdentities).
In `@src/api/resolvers/modules/impersonation.ts`:
- Around line 262-266: The resolver is accepting args.scope but never forwarding
it to performTokenExchange, so callers think down-scoping occurs while it is
ignored; update the impersonation resolver to pass args.scope into
performTokenExchange (e.g., performTokenExchange(ctx.oidc.tokenSet.access_token,
args.targetUserId, args.scope)) or alternatively remove the GraphQL scope
argument from the schema and resolver signature; look for usages of
performTokenExchange and args.scope in impersonation.ts and either forward
args.scope through the call or delete the scope argument and all references to
it.
In `@src/api/resolvers/modules/user.ts`:
- Around line 432-436: The current spreads use truthy checks (e.g.,
...(issuerUserData.family_name && { family_name: issuerUserData.family_name }))
which drop empty-string values; change these to nullish checks so intentional
empty values are preserved—replace each truthy guard for
issuerUserData.family_name, issuerUserData.given_name, and
issuerUserData.preferred_username with a nullish check (e.g., use
issuerUserData.family_name != null ? { family_name: issuerUserData.family_name }
: {} ) so only null/undefined are omitted while empty strings are allowed
through.
In `@src/api/services/OIDC.ts`:
- Around line 353-364: The token exchange builds tokenExchangeParams but omits
the actor_token fields so impersonation tokens are never bound; modify the
exchange to include actor_token and actor_token_type (use the existing
actorToken variable and set actor_token_type to
'urn:ietf:params:oauth:token-type:access_token') when actorToken is present, so
the request sent from the function that creates subjectToken and performs the
token exchange includes those fields and the resulting JWT contains the act
claim expected by the verification in src/api/context/oidc.ts.
- Around line 174-180: The decodeAccessTokenClaims helper currently returns
unverified claims from decodeJwt; stop merging those into session by either
verifying the access_token signature against the issuer JWKS before returning
claims (reuse the same JWKS verification logic used for id_token) or replace
usage with a server-side call to the userinfo endpoint to obtain authoritative
claims—update callers that merge claims into session to use the
verified/userinfo result instead of decodeAccessTokenClaims. Also fix
performTokenExchange to include the provided actorToken by adding an actor_token
entry to the tokenExchangeParams body so the token exchange request binds the
actor (preserve the existing subject_token behavior and include actor_token
alongside it).
In `@src/config/public.ts`:
- Line 11: Change the optional URL/string zod schemas to first normalize empty
strings to undefined using z.preprocess so empty env values are treated as
unset; specifically update the schemas for PUBLIC_OIDC_ACCOUNT_URL,
PUBLIC_SENTRY_DSN, PUBLIC_BADGE_GENERATOR_URL, PUBLIC_DOCS_URL (and in the other
config file for OIDC_M2M_RESOURCE and SENTRY_DSN) to use z.preprocess(v => v ===
"" ? undefined : v, z.string().url().optional() or z.string().optional() as
appropriate) so the existing nullish-coalescing logic (??) works and empty
strings no longer fail validation.
In `@src/routes/`(authenticated)/my-account/+page.svelte:
- Around line 193-263: The action anchors that render only an icon need
accessible names: update each <a class="btn btn-ghost btn-sm"
href={accountUrl(...)}> (used with accountUrl('username'), accountUrl('email'),
accountUrl('password'), accountUrl(hasPasskey ? 'passkey/manage' :
'passkey/add'), accountUrl(hasTotp ? 'authenticator-app/replace' :
'authenticator-app'), and accountUrl(hasBackupCodes ? 'backup-codes/manage' :
'backup-codes/generate')) to include either an appropriate aria-label (e.g.,
"Change username", "Change email", "Change password", "Manage passkeys" / "Add
passkey", "Replace authenticator app" / "Set up authenticator app", "Manage
backup codes" / "Generate backup codes") or add visually hidden text inside the
anchor so screen readers and keyboard users get descriptive labels; ensure
labels reflect the conditional text determined by hasPasskey, hasTotp, and
hasBackupCodes.
---
Nitpick comments:
In `@src/routes/`(authenticated)/my-account/+page.server.ts:
- Around line 62-65: The fallback accountCenterUrl building currently uses
string concatenation which can create malformed URLs; update the logic that sets
accountCenterUrl to use the URL API instead of concatenation: if
configPublic.PUBLIC_OIDC_ACCOUNT_URL is present use it, otherwise construct a
new URL('/account', configPublic.PUBLIC_OIDC_AUTHORITY.replace(/\/oidc\/?$/,
'')) (or equivalent using new URL with eventUrl.origin as needed) so paths are
joined correctly; keep the existing accountRedirectUrl
(`${eventUrl.origin}/my-account`) behavior unchanged or likewise build it via
new URL('/my-account', eventUrl.origin) for consistency.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3921efe3-6860-4a20-b750-3268391b70af
📒 Files selected for processing (23)
.env.example.github/actions/setup-bun/action.yml.github/workflows/ci.ymlCLAUDE.mdREADME.mdWARP.mddev.docker-compose.ymlmessages/de.jsonmessages/en.jsonmock-oidc-landingpage.htmlprisma/seed/createMissingLogtoUsers.tsprisma/seed/migrateOidcSubs.tsschema.graphqlsrc/api/context/oidc.tssrc/api/resolvers/modules/auth.tssrc/api/resolvers/modules/impersonation.tssrc/api/resolvers/modules/user.tssrc/api/services/OIDC.tssrc/config/private.tssrc/config/public.tssrc/lib/queries/fastUserQuery.tssrc/routes/(authenticated)/my-account/+page.server.tssrc/routes/(authenticated)/my-account/+page.svelte
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 10 file(s) based on 11 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
Co-authored-by: CodeRabbit <[email protected]> Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Tade Strehk <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/routes/(authenticated)/my-account/+page.svelte (1)
32-44: Consider preventing duplicate toast notifications on re-renders.The
$effectruns wheneverdata.accountUpdateSuccesschanges. However, if the page component re-renders for other reasons whileaccountUpdateSuccessis still set (e.g., navigation back to this page), the toast may fire again unexpectedly. Consider tracking whether the toast was already shown for the current value.♻️ Suggested improvement
+ let lastToastShown = $state<string | null>(null); + // Show toast for successful account center updates $effect(() => { const successMap: Record<string, () => string> = { email: () => m.accountUpdateSuccessEmail(), password: () => m.accountUpdateSuccessPassword(), username: () => m.accountUpdateSuccessUsername(), passkey: () => m.accountUpdateSuccessPasskey(), mfa: () => m.accountUpdateSuccessMfa(), 'backup-codes': () => m.accountUpdateSuccessBackupCodes() }; - if (data.accountUpdateSuccess && successMap[data.accountUpdateSuccess]) { + if (data.accountUpdateSuccess && successMap[data.accountUpdateSuccess] && lastToastShown !== data.accountUpdateSuccess) { toast.success(successMap[data.accountUpdateSuccess]()); + lastToastShown = data.accountUpdateSuccess; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/my-account/+page.svelte around lines 32 - 44, The effect currently calls toast.success whenever data.accountUpdateSuccess is truthy which can re-fire on unrelated re-renders; add a small local tracker (e.g., a module-scoped variable like lastShownSuccess or a Set) outside the $effect to remember the last shown value, then inside the $effect compare data.accountUpdateSuccess to that tracker and only call toast.success(successMap[... ]()) and update the tracker when the value is new; ensure you still handle different string keys (email, password, username, passkey, mfa, backup-codes) and optionally clear the tracker when accountUpdateSuccess is cleared so future updates can show again.src/api/services/OIDC.ts (1)
57-67: Name splitting heuristic may produce unexpected results for some name formats.The logic splits
nameon the last space, assuming a "Given Family" order. This works for many Western names but may produce unexpected results for:
- Names with multiple middle names (e.g., "John Paul George Ringo" → given: "John Paul George", family: "Ringo")
- Single-word names or mononyms (correctly handled by the fallback)
- Names in "Family Given" order (common in some cultures)
Since
family_nameandgiven_nameare now optional, this is acceptable as a best-effort fallback when the provider only supplies a combinednameclaim.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/OIDC.ts` around lines 57 - 67, The current heuristic splits normalized.name on the last space which can misassign multi-part names; change the logic in OIDC.ts (the block that manipulates normalized.given_name and normalized.family_name) to only split when parts.length === 2 (set given_name = parts[0], family_name = parts[1]); for single-word names keep the existing fallback (set given_name = normalized.name and family_name = normalized.name or leave family_name undefined as your policy dictates), and for names with more than two parts avoid applying the "last-space" split (e.g., set given_name = normalized.name and leave family_name undefined) so you don't incorrectly map multi-part names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 249: The file .env.example is missing a trailing newline; edit the file
so that the final line "# PUBLIC_SHA=abc123" is terminated with a newline
character (i.e., add an empty line at EOF) to satisfy dotenv-linter and standard
POSIX text file conventions.
---
Nitpick comments:
In `@src/api/services/OIDC.ts`:
- Around line 57-67: The current heuristic splits normalized.name on the last
space which can misassign multi-part names; change the logic in OIDC.ts (the
block that manipulates normalized.given_name and normalized.family_name) to only
split when parts.length === 2 (set given_name = parts[0], family_name =
parts[1]); for single-word names keep the existing fallback (set given_name =
normalized.name and family_name = normalized.name or leave family_name undefined
as your policy dictates), and for names with more than two parts avoid applying
the "last-space" split (e.g., set given_name = normalized.name and leave
family_name undefined) so you don't incorrectly map multi-part names.
In `@src/routes/`(authenticated)/my-account/+page.svelte:
- Around line 32-44: The effect currently calls toast.success whenever
data.accountUpdateSuccess is truthy which can re-fire on unrelated re-renders;
add a small local tracker (e.g., a module-scoped variable like lastShownSuccess
or a Set) outside the $effect to remember the last shown value, then inside the
$effect compare data.accountUpdateSuccess to that tracker and only call
toast.success(successMap[... ]()) and update the tracker when the value is new;
ensure you still handle different string keys (email, password, username,
passkey, mfa, backup-codes) and optionally clear the tracker when
accountUpdateSuccess is cleared so future updates can show again.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e114b1ff-05e7-47b3-b36c-db81fa2742de
📒 Files selected for processing (10)
.env.exampleREADME.mdsrc/api/context/oidc.tssrc/api/resolvers/modules/auth.tssrc/api/resolvers/modules/impersonation.tssrc/api/resolvers/modules/user.tssrc/api/services/OIDC.tssrc/config/private.tssrc/config/public.tssrc/routes/(authenticated)/my-account/+page.svelte
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/config/private.ts
- src/api/resolvers/modules/user.ts
- src/api/resolvers/modules/impersonation.ts
- src/config/public.ts
…to session Previously decodeAccessTokenClaims used jose's decodeJwt which returns unverified claims. Now verifies the JWT signature against the issuer JWKS, preventing unverified claims (e.g. roles) from being trusted. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Shows an intermediate warning page before OIDC login when PUBLIC_OIDC_MIGRATION_NOTICE is enabled, informing users about the Zitadel-to-Logto migration with FAQ and instructions. Uses a version-keyed cookie so the notice can be re-shown by bumping the version. Disabled by default. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add FAQ section for first-time registrants explaining the change doesn't affect them. Add Zitadel/Logto links in the identity provider explanation using @html rendering. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/api/services/OIDC.ts (1)
379-400:⚠️ Potential issue | 🟠 Major
actor_tokenandactor_token_typeare still missing from the token exchange request.The past review flagged that
actorTokenis received but never included intokenExchangeParams. This issue persists: line 371-377 extracts the actor for logging only, but the token exchange request (lines 386-400) omitsactor_tokenandactor_token_type. Without these fields, the resulting impersonation JWT won't contain theactclaim, breaking the audit trail of who initiated impersonation. The downstream verification insrc/api/context/oidc.tsexpectsdecoded.actto verify impersonation ownership.🔧 Proposed fix to include actor_token in the exchange
const tokenExchangeParams: Record<string, string> = { grant_type: 'urn:ietf:params:oauth:grant-type:token-exchange', subject_token: subjectToken, - subject_token_type: 'urn:ietf:params:oauth:token-type:access_token' + subject_token_type: 'urn:ietf:params:oauth:token-type:access_token', + actor_token: actorToken, + actor_token_type: 'urn:ietf:params:oauth:token-type:access_token' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/OIDC.ts` around lines 379 - 400, The token exchange omits the actor token fields so the impersonation JWT lacks the `act` claim; update the code that builds tokenExchangeParams (in the same function that calls createSubjectToken and reads actorToken) to add actor_token = actorToken and actor_token_type = 'urn:ietf:params:oauth:token-type:access_token' when actorToken is present (i.e., only attach these two keys if actorToken is truthy), so the token exchange request includes the actor and downstream verification of decoded.act will work..env.example (1)
253-253:⚠️ Potential issue | 🟡 MinorAdd trailing newline at end of file.
The file is missing a trailing newline, which is flagged by dotenv-linter and is a POSIX convention for text files.
-# PUBLIC_SHA=abc123 +# PUBLIC_SHA=abc123 +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example at line 253, Add a trailing newline (LF) at the end of the .env.example file so the final line containing the commented variable "# PUBLIC_SHA=abc123" ends with a newline character; simply ensure the file terminates with a newline to satisfy dotenv-linter and POSIX text-file conventions.
🧹 Nitpick comments (1)
src/api/services/OIDC.ts (1)
57-67: Name-splitting heuristic may produce unexpected results for non-Western names.The logic assumes the last whitespace-delimited word is the family name and everything before is the given name. This works for "John Doe" but fails for:
- Single-word names (duplicates the name to both fields on line 64-65)
- East Asian name order (family name first)
- Names with particles ("Ludwig van Beethoven" → given: "Ludwig van", family: "Beethoven")
Since these fields are now optional and only used for display, consider documenting this limitation or simply preserving the original
namefield when parsing fails.♻️ Suggested simplification for single-word names
if ((!normalized.family_name || !normalized.given_name) && normalized.name) { const parts = normalized.name.trim().split(/\s+/); if (parts.length >= 2) { normalized.given_name = parts.slice(0, -1).join(' '); normalized.family_name = parts[parts.length - 1]; } else { - normalized.given_name = normalized.name; - normalized.family_name = normalized.name; + // Single-word name: leave family_name undefined, use full name as given_name + normalized.given_name = normalized.name; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/OIDC.ts` around lines 57 - 67, The current name-splitting in OIDC (logic touching normalized.name, normalized.given_name, normalized.family_name) assumes Western order and duplicates single-word names into both given and family names; change it to only split when there are at least two words and avoid duplicating the single-word case—i.e., if parts.length >= 2 set normalized.given_name = parts.slice(0, -1).join(' ') and normalized.family_name = parts[parts.length - 1], otherwise leave normalized.given_name and normalized.family_name unset (or set family_name to undefined) and rely on normalized.name for display; add a brief comment noting the heuristic limitation instead of forcing potentially incorrect family/given values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/services/OIDC.ts`:
- Around line 324-348: createSubjectToken currently derives the Management API
base URL directly from the issuer; change it to mirror getM2MAccessToken by
using configPrivate.OIDC_M2M_RESOURCE with a fallback of
`${issuer.replace(/\/oidc\/?$/, '')}/api`. Update createSubjectToken to compute
managementApiBase = configPrivate.OIDC_M2M_RESOURCE ??
`${issuer.replace(/\/oidc\/?$/, '')}/api` and then POST to the subject-tokens
endpoint relative to that base (e.g., `${managementApiBase}/subject-tokens`)
while keeping the existing headers, body, timeout, and error handling; reference
createSubjectToken, getM2MAccessToken, and configPrivate.OIDC_M2M_RESOURCE when
making the change.
---
Duplicate comments:
In @.env.example:
- Line 253: Add a trailing newline (LF) at the end of the .env.example file so
the final line containing the commented variable "# PUBLIC_SHA=abc123" ends with
a newline character; simply ensure the file terminates with a newline to satisfy
dotenv-linter and POSIX text-file conventions.
In `@src/api/services/OIDC.ts`:
- Around line 379-400: The token exchange omits the actor token fields so the
impersonation JWT lacks the `act` claim; update the code that builds
tokenExchangeParams (in the same function that calls createSubjectToken and
reads actorToken) to add actor_token = actorToken and actor_token_type =
'urn:ietf:params:oauth:token-type:access_token' when actorToken is present
(i.e., only attach these two keys if actorToken is truthy), so the token
exchange request includes the actor and downstream verification of decoded.act
will work.
---
Nitpick comments:
In `@src/api/services/OIDC.ts`:
- Around line 57-67: The current name-splitting in OIDC (logic touching
normalized.name, normalized.given_name, normalized.family_name) assumes Western
order and duplicates single-word names into both given and family names; change
it to only split when there are at least two words and avoid duplicating the
single-word case—i.e., if parts.length >= 2 set normalized.given_name =
parts.slice(0, -1).join(' ') and normalized.family_name = parts[parts.length -
1], otherwise leave normalized.given_name and normalized.family_name unset (or
set family_name to undefined) and rely on normalized.name for display; add a
brief comment noting the heuristic limitation instead of forcing potentially
incorrect family/given values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddc72029-7aa3-439a-b1ef-07ed0b0b446d
⛔ Files ignored due to path filters (1)
src/assets/undraw/moving.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
.env.examplemessages/de.jsonmessages/en.jsonsrc/api/services/OIDC.tssrc/config/public.tssrc/lib/constants/migrationNotice.tssrc/routes/(authenticated)/+layout.server.tssrc/routes/auth/migration-notice/+page.server.tssrc/routes/auth/migration-notice/+page.svelte
✅ Files skipped from review due to trivial changes (2)
- src/lib/constants/migrationNotice.ts
- messages/en.json
…tToken Align createSubjectToken with getM2MAccessToken by using the configurable OIDC_M2M_RESOURCE with fallback, instead of hardcoding the base URL derivation from the issuer. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/api/services/OIDC.ts (1)
380-401:⚠️ Potential issue | 🔴 Critical
actor_tokenstill not included in token exchange request.The
actorTokenparameter is received but never added totokenExchangeParams. Per RFC 8693 and the comment on lines 385-386, includingactor_tokenandactor_token_typeis necessary to bind theactclaim in the resulting JWT. Without this, downstream verification insrc/api/context/oidc.ts(which checksdecoded.act) cannot confirm who initiated the impersonation.🔒 Proposed fix to include actor_token
const tokenExchangeParams: Record<string, string> = { grant_type: 'urn:ietf:params:oauth:grant-type:token-exchange', subject_token: subjectToken, - subject_token_type: 'urn:ietf:params:oauth:token-type:access_token' + subject_token_type: 'urn:ietf:params:oauth:token-type:access_token', + actor_token: actorToken, + actor_token_type: 'urn:ietf:params:oauth:token-type:access_token' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/OIDC.ts` around lines 380 - 401, The token exchange builds tokenExchangeParams but never adds the actor token; update the logic in the token-exchange flow (the block that constructs tokenExchangeParams in the function that calls createSubjectToken and exchanges it) to add actor_token and actor_token_type when an actorToken argument is provided: set tokenExchangeParams.actor_token = actorToken and tokenExchangeParams.actor_token_type = 'urn:ietf:params:oauth:token-type:access_token' (matching the subject token type used) so the returned JWT includes the act claim that src/api/context/oidc.ts expects.
🧹 Nitpick comments (4)
src/api/services/OIDC.ts (4)
41-43: Type guard parameter usesany.Use
unknownfor the parameter type to enforce proper type narrowing within the guard.-export function isValidOIDCUser(user: any): user is OIDCUser { - return !!user.sub && !!user.email; +export function isValidOIDCUser(user: unknown): user is OIDCUser { + return typeof user === 'object' && user !== null && 'sub' in user && 'email' in user && !!user.sub && !!user.email; }As per coding guidelines: "NEVER use
anytype in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/OIDC.ts` around lines 41 - 43, Change the isValidOIDCUser parameter from any to unknown and implement proper runtime narrowing: update the signature to isValidOIDCUser(user: unknown): user is OIDCUser and inside the function first check that user is a non-null object, then verify it has the required properties (e.g. 'sub' and 'email') and optionally that those properties are strings before returning true; reference the OIDCUser type and the isValidOIDCUser function to locate and update the guard logic.
314-315: Consider adding runtime validation for the M2M token response.The
response.json()result is cast implicitly. Adding basic validation thatdata.access_tokenexists and is a string would improve robustness against unexpected API responses.const data = await response.json(); + if (typeof data?.access_token !== 'string') { + throw new Error('Invalid M2M token response: missing access_token'); + } return data.access_token;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/OIDC.ts` around lines 314 - 315, The code currently does await response.json() and returns data.access_token without checking shape; add runtime validation after calling response.json() to ensure the parsed object has an access_token property of type string (e.g., check typeof data?.access_token === 'string'), throw a descriptive error if missing or invalid, and only then return the token to prevent downstream failures when response.json() returns an unexpected shape; update the function in OIDC.ts that performs response.json() to include this guard around the data and access_token variables.
33-33: Index signature usesanytype.The coding guidelines prohibit
anyunless unavoidable. Consider usingunknownfor the index signature, which forces proper type narrowing when accessing dynamic claims.- [key: string]: any; + [key: string]: unknown;As per coding guidelines: "NEVER use
anytype in TypeScript - only use it when absolutely unavoidable with a// TYPE-SAFETY-EXCEPTION:comment explaining why and keeping scope narrow".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/OIDC.ts` at line 33, Replace the unsafe index signature "[key: string]: any" in OIDC.ts with a safer type (e.g., "Record<string, unknown>" or "[key: string]: unknown") and update any places that read properties from this indexed object to perform explicit type narrowing/casting (use type guards, typeof checks, or specific casts) before treating values as strings/numbers/objects; if you truly cannot avoid `any`, add a narrow-scoped "// TYPE-SAFETY-EXCEPTION:" comment explaining why and limit its use to the smallest possible symbol.
49-49: ConsiderRecord<string, unknown>for stricter type safety.While
Record<string, any>is common for dynamic OIDC claims,Record<string, unknown>would align better with the coding guidelines and force explicit type checks when accessing properties.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/services/OIDC.ts` at line 49, Change the parameter type of normalizeOIDCClaims from Record<string, any> to Record<string, unknown> to improve type safety; update the function signature normalizeOIDCClaims(claims: Record<string, unknown>): Record<string, any> and then adjust any property accesses inside the function to perform explicit type checks or narrow with type assertions (e.g., typeof checks or Array.isArray) before using claim values so compilation is satisfied and runtime behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/services/OIDC.ts`:
- Line 258: Replace the type assertion on remoteUserInfo by using the
isValidOIDCUser type guard to let TypeScript narrow the type: call
isValidOIDCUser(remoteUserInfo) and return remoteUserInfo directly from inside
that conditional branch (and handle the failure path consistently—throw or
return the appropriate error/undefined) instead of using "as OIDCUser";
reference the isValidOIDCUser function and the remoteUserInfo variable in
OIDC.ts to locate where to make this change.
- Around line 248-252: The call that falls back to fetch user info incorrectly
passes the raw access_token as the expectedSubject; update the call to pass the
actual subject (sub) or undefined instead of access_token so fetchUserInfo
receives a user ID, e.g. replace the third argument currently using "sub ??
access_token" with just "sub" (or "sub ?? undefined") and keep the surrounding
use of normalizeOIDCClaims and accessTokenClaims unchanged; check the
identifiers fetchUserInfo, normalizeOIDCClaims, access_token, sub, and
accessTokenClaims when making the change.
---
Duplicate comments:
In `@src/api/services/OIDC.ts`:
- Around line 380-401: The token exchange builds tokenExchangeParams but never
adds the actor token; update the logic in the token-exchange flow (the block
that constructs tokenExchangeParams in the function that calls
createSubjectToken and exchanges it) to add actor_token and actor_token_type
when an actorToken argument is provided: set tokenExchangeParams.actor_token =
actorToken and tokenExchangeParams.actor_token_type =
'urn:ietf:params:oauth:token-type:access_token' (matching the subject token type
used) so the returned JWT includes the act claim that src/api/context/oidc.ts
expects.
---
Nitpick comments:
In `@src/api/services/OIDC.ts`:
- Around line 41-43: Change the isValidOIDCUser parameter from any to unknown
and implement proper runtime narrowing: update the signature to
isValidOIDCUser(user: unknown): user is OIDCUser and inside the function first
check that user is a non-null object, then verify it has the required properties
(e.g. 'sub' and 'email') and optionally that those properties are strings before
returning true; reference the OIDCUser type and the isValidOIDCUser function to
locate and update the guard logic.
- Around line 314-315: The code currently does await response.json() and returns
data.access_token without checking shape; add runtime validation after calling
response.json() to ensure the parsed object has an access_token property of type
string (e.g., check typeof data?.access_token === 'string'), throw a descriptive
error if missing or invalid, and only then return the token to prevent
downstream failures when response.json() returns an unexpected shape; update the
function in OIDC.ts that performs response.json() to include this guard around
the data and access_token variables.
- Line 33: Replace the unsafe index signature "[key: string]: any" in OIDC.ts
with a safer type (e.g., "Record<string, unknown>" or "[key: string]: unknown")
and update any places that read properties from this indexed object to perform
explicit type narrowing/casting (use type guards, typeof checks, or specific
casts) before treating values as strings/numbers/objects; if you truly cannot
avoid `any`, add a narrow-scoped "// TYPE-SAFETY-EXCEPTION:" comment explaining
why and limit its use to the smallest possible symbol.
- Line 49: Change the parameter type of normalizeOIDCClaims from Record<string,
any> to Record<string, unknown> to improve type safety; update the function
signature normalizeOIDCClaims(claims: Record<string, unknown>): Record<string,
any> and then adjust any property accesses inside the function to perform
explicit type checks or narrow with type assertions (e.g., typeof checks or
Array.isArray) before using claim values so compilation is satisfied and runtime
behavior remains unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f000227-5078-4e49-ba4b-86f0a0165a72
📒 Files selected for processing (1)
src/api/services/OIDC.ts
Summary
OIDC_M2M_CLIENT_ID,OIDC_M2M_CLIENT_SECRET,OIDC_M2M_RESOURCE) for Management API accessTest plan
bun run check— no type errors🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Expanded Data
Documentation