feat: Replace Zitadel with Logto as OIDC identity provider#415
Open
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…cs to .env.example Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughMigrates default OIDC provider guidance from ZITADEL to Logto, adds Logto M2M impersonation/token-exchange flow, normalizes provider-specific claims, relaxes several user profile fields to nullable, adds migration seed scripts, and updates UI/messages, configs, and CI/deployment settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant OIDCProvider as OIDC Provider
participant ManagementAPI as Logto Management API
participant Database
rect rgba(200, 150, 100, 0.5)
Note over Server,OIDCProvider: Legacy flow (validateTokens-based)
Client->>Server: Impersonation request (impersonation token)
Server->>OIDCProvider: validateTokens()/userinfo (verified path)
OIDCProvider-->>Server: token claims
Server->>Database:_lookup_impersonated_user_by_sub
Server->>Server: validate actor claim vs session
Server->>OIDCProvider: token exchange (audience)
end
rect rgba(100, 150, 200, 0.5)
Note over Server,ManagementAPI: New Logto M2M subject-token flow
Client->>Server: Impersonation request (impersonation token)
Server->>Server: jwtVerify()/decode impersonation token
Server->>Database: fetch user by sub from token
Server->>Server: validate actor claim vs session
Server->>ManagementAPI: getM2MAccessToken(client_id, secret)
ManagementAPI-->>Server: m2m_access_token
Server->>ManagementAPI: POST /api/subject-tokens (m2m_token + sub)
ManagementAPI-->>Server: subject_token
Server->>OIDCProvider: token exchange using subject_token (include resource if configured)
OIDCProvider-->>Server: exchanged tokens
Server->>Database: establish impersonation/session
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 & flexible profile
Client->>Server: Login / token refresh
Server->>OIDCIssuer: validate id_token, decode access_token
OIDCIssuer-->>Server: id_token + access_token claims
Server->>Server: normalizeOIDCClaims() (e.g. Logto: 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 with new fields
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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
Comment on lines
+174
to
+180
| function decodeAccessTokenClaims(access_token: string): Record<string, unknown> { | ||
| try { | ||
| return decodeJwt(access_token); | ||
| } catch { | ||
| return {}; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | head -20Repository: DeutscheModelUnitedNations/munify-delegator
Length of output: 552
🏁 Script executed:
find . -type f -name "*.ts" | grep -E "(OIDC|oidc)" | head -20Repository: DeutscheModelUnitedNations/munify-delegator
Length of output: 139
🏁 Script executed:
wc -l src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 115
🏁 Script executed:
sed -n '170,230p' src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 2050
🏁 Script executed:
cat -n src/api/context/oidc.ts 2>/dev/null || echo "File not found, searching for context files"Repository: DeutscheModelUnitedNations/munify-delegator
Length of output: 11585
🏁 Script executed:
find . -type f -name "*.ts" -path "*/context/*" | head -20Repository: DeutscheModelUnitedNations/munify-delegator
Length of output: 174
🏁 Script executed:
sed -n '220,280p' src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 2069
🏁 Script executed:
sed -n '300,380p' src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 2818
🏁 Script executed:
rg "jwtVerify|decodeJwt|access_token" src/api/services/OIDC.ts -A 2 -B 2Repository: DeutscheModelUnitedNations/munify-delegator
Length of output: 1572
🏁 Script executed:
rg "actor_token|actorToken" src/api/services/OIDC.ts -A 3 -B 3Repository: DeutscheModelUnitedNations/munify-delegator
Length of output: 1166
🏁 Script executed:
sed -n '400,440p' src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 1266
🏁 Script executed:
rg "actorToken|actor_token" src/api/ -A 1 -B 1 --type tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 916
🏁 Script executed:
sed -n '380,410p' src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 935
Do not merge unverified access-token claims into the authenticated session.
The decodeJwt call on line 176 performs no signature validation. Because the id_token is verified against the issuer's JWKS (line 190) but the access_token claims are not, an attacker who controls the client-stored token_set cookie can replace the access_token with a forged JWT and inject false roles, mfa, or password claims. These unverified claims are then merged into the session state on lines 202 and 224.
Either verify the access_token JWT signature with the issuer's JWKS before merging its claims, or fetch these claims server-side from the userinfo endpoint (which already uses the access_token for authentication).
The same vulnerability exists in the impersonation token handling (src/api/context/oidc.ts, line 158).
Add actor_token to the token exchange request.
The performTokenExchange() function accepts an actorToken parameter intended to bind the act claim to the resulting JWT, but it is never included in the token exchange request body (lines 355–387). Currently only the subject_token is sent, losing the audit trail of who initiated the impersonation. Include actor_token in tokenExchangeParams so downstream systems can verify the actor.
🤖 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 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).
Comment on lines
+353
to
+364
| try { | ||
| // Step 1: Create a subject token via the Management API | ||
| const subjectToken = await createSubjectToken(subjectUserId); | ||
|
|
||
| // Step 2: Exchange the subject token for an access token | ||
| // Logto requires: grant_type, subject_token, subject_token_type, resource | ||
| // actor_token is optional (adds `act` claim to the resulting JWT) | ||
| 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' | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the OIDC.ts file and examine the context around lines 353-364
find . -name "OIDC.ts" -path "*/api/*"Repository: DeutscheModelUnitedNations/munify-delegator
Length of output: 113
🏁 Script executed:
# Get the file size first to see if we can read it
wc -l src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 115
🏁 Script executed:
# Read the specific lines and surrounding context (lines 340-380)
sed -n '340,380p' src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 1463
🏁 Script executed:
# Also check the oidc.ts context file
find . -name "oidc.ts" -path "*/api/context/*"Repository: DeutscheModelUnitedNations/munify-delegator
Length of output: 112
🏁 Script executed:
# Read the oidc.ts context file to check the impersonation logic
cat src/api/context/oidc.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 9681
🏁 Script executed:
# Read the function that contains these lines - start from line 300 to see the function signature
sed -n '300,375p' src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 2643
🏁 Script executed:
# Also search for where actor_token might be used or any conditional additions to tokenExchangeParams
rg -A 20 "tokenExchangeParams" src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 1388
🏁 Script executed:
# Check if actorToken is added anywhere else after line 370
sed -n '350,420p' src/api/services/OIDC.tsRepository: DeutscheModelUnitedNations/munify-delegator
Length of output: 2378
Include actor_token and actor_token_type in the token exchange request.
The actorToken parameter is received but only extracted for audit logging; it's never included in tokenExchangeParams sent to the OIDC token endpoint. The comment on lines 358–359 correctly notes that actor_token adds the act claim to the resulting JWT, but this field is omitted from the actual request. Downstream validation in src/api/context/oidc.ts only verifies impersonation ownership when decoded.act is present, creating a security gap where impersonation tokens lack binding to the user who initiated them.
🤖 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 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.
m1212e
approved these changes
Apr 5, 2026
Contributor
|
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 <noreply@coderabbit.ai> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Tade Strehk <git@strehk.eu>
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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
Documentation