Conversation
📝 WalkthroughWalkthroughUpdated OIDC authentication handling to propagate ID token and user info claims into session data, modified account resolution logic to validate OIDC subjects as UUIDs with fallback account registration, and changed session token cookie SameSite attribute from Strict to Lax. Changes
Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
philpotisk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.kt`:
- Around line 218-219: The code assigns raw ID token/UserInfo JSON into
OidcSessionAuthenticatedData via idTokenClaims = idTokenPayload and
userInfoClaims = userInfo, which later gets interpolated into logs as
sessionOidcConfig and can leak PII; to fix, stop persisting full claim
blobs—either change how OidcSessionAuthenticatedData is populated (only store
minimal bootstrap fields you actually need such as sub, exp, email) or
redact/sanitize these fields before assignment (e.g., replace claim values with
masked strings or store a hashed/summary representation) so the sensitive
payloads are not present when sessionOidcConfig is logged.
- Around line 223-230: Currently the code uses the raw subject as the local
account id (via the UUID.fromString check and returning subject), which loses
issuer context and breaks non-UUID subs; instead, generate/mint a new internal
account id (or reuse the registration amendment flow used in Web3.kt) and attach
the full OIDCIdentifier(issuer, subject) to that account; update the logic
around identifier.resolveIfExists() to only reuse existing account ids when the
identifier is already linked, call
KtorAuthnzManager.accountStore.addAccountIdentifierToAccount(accountId,
OIDCIdentifier(issuer, subject)) to link the identifier, and ensure you
explicitly create the backing account record if
EditableAccountStore.addAccountIdentifierToAccount does not guarantee account
creation (create the account first then attach identifier).
- Line 3: Remove the accidental compiler-internal import by deleting the line
importing com.sun.tools.javac.code.TypeAnnotationPosition.field from OIDC.kt;
this import is unused and unrelated to OIDC functionality, so simply remove it
(no other code changes required) to avoid depending on JDK-internal symbols.
In
`@waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/sessions/SessionTokenCookieHandler.kt`:
- Line 25: The change sets extensions = mapOf("SameSite" to "Lax") in
SessionTokenCookieHandler which relaxes SameSite for all sessions; revert the
global default to a safe value (e.g., omit SameSite or set "Strict") in
SessionTokenCookieHandler and instead add a configurable parameter or setter on
SessionTokenCookieHandler (or its factory) to allow per-auth-method overrides;
update OIDC-specific code (the OIDC callback/front-channel logout flow that
currently constructs/uses SessionTokenCookieHandler) to explicitly request
SameSite="Lax" when needed so only the OIDC flow uses the relaxed cookie policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9e83797-0b04-4ca0-a6ef-e64b9673b84e
📒 Files selected for processing (3)
waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.ktwaltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/sessiondata/OidcSessionData.ktwaltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/sessions/SessionTokenCookieHandler.kt
| @@ -1,10 +1,12 @@ | |||
| package id.walt.ktorauthnz.methods | |||
|
|
|||
| import com.sun.tools.javac.code.TypeAnnotationPosition.field | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the OIDC.kt file
fd -t f "OIDC.kt" waltid-libraries/Repository: walt-id/waltid-identity
Length of output: 158
🏁 Script executed:
# Read the imports section of the OIDC.kt file (first ~30 lines to see all imports)
head -30 waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.ktRepository: walt-id/waltid-identity
Length of output: 1427
🏁 Script executed:
# Check if TypeAnnotationPosition is used anywhere in the file
rg "TypeAnnotationPosition" waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.ktRepository: walt-id/waltid-identity
Length of output: 127
🏁 Script executed:
# Check if this import causes any issues - search for build configuration files
fd -t f -e "build.gradle.kts" -e "pom.xml" waltid-libraries/auth/waltid-ktor-authnz/ | head -5Repository: walt-id/waltid-identity
Length of output: 536
🏁 Script executed:
# Search for any other com.sun.tools imports in the codebase
rg "import com\.sun\.tools" waltid-libraries/Repository: walt-id/waltid-identity
Length of output: 219
🏁 Script executed:
# Check if field is referenced anywhere at all in this file
rg "field" waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.kt | head -20Repository: walt-id/waltid-identity
Length of output: 127
Remove the accidental javac internals import.
com.sun.tools.javac.code.TypeAnnotationPosition.field is a JDK compiler-internal symbol that is completely unused in this file. This import is unrelated to OIDC functionality and will fail resolution on many JDK implementations, breaking compilation.
Minimal fix
-import com.sun.tools.javac.code.TypeAnnotationPosition.field📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import com.sun.tools.javac.code.TypeAnnotationPosition.field |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.kt`
at line 3, Remove the accidental compiler-internal import by deleting the line
importing com.sun.tools.javac.code.TypeAnnotationPosition.field from OIDC.kt;
this import is unused and unrelated to OIDC functionality, so simply remove it
(no other code changes required) to avoid depending on JDK-internal symbols.
waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.kt
Show resolved
Hide resolved
| val accountId = identifier.resolveIfExists() ?: run { | ||
| require(runCatching { UUID.fromString(subject) }.isSuccess) { | ||
| "OIDC subject must be a UUID-compatible account id, but got: $subject" | ||
| } | ||
|
|
||
| /*if (accountId == null) { | ||
|
|
||
| // TODO: Create account if it does not exist | ||
| Account("", "") | ||
| ExampleAccountStore.registerAccount() | ||
| }*/ | ||
| KtorAuthnzManager.accountStore.addAccountIdentifierToAccount(subject, identifier) | ||
| subject | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "OIDC\.kt$" | head -20Repository: walt-id/waltid-identity
Length of output: 158
🏁 Script executed:
fd -t f "EditableAccountStore" | head -20Repository: walt-id/waltid-identity
Length of output: 175
🏁 Script executed:
fd -t f "Web3\.kt$" | head -20Repository: walt-id/waltid-identity
Length of output: 158
🏁 Script executed:
fd -t f "AccountStore" -t f | head -20Repository: walt-id/waltid-identity
Length of output: 384
🏁 Script executed:
wc -l waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.ktRepository: walt-id/waltid-identity
Length of output: 162
🏁 Script executed:
cat -n waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.kt | sed -n '200,250p'Repository: walt-id/waltid-identity
Length of output: 2464
🏁 Script executed:
fd -t f "OIDCIdentifier" -o -name "*OIDC*" | grep -v ".class" | head -20Repository: walt-id/waltid-identity
Length of output: 297
🏁 Script executed:
rg "class OIDCIdentifier" -A 10Repository: walt-id/waltid-identity
Length of output: 1934
🏁 Script executed:
rg "fun resolveIfExists" -B 5 -A 10Repository: walt-id/waltid-identity
Length of output: 2075
🏁 Script executed:
rg "fun addAccountIdentifierToAccount" -B 3 -A 15Repository: walt-id/waltid-identity
Length of output: 9217
🏁 Script executed:
rg "fun lookupAccountUuid" -B 3 -A 10Repository: walt-id/waltid-identity
Length of output: 6763
🏁 Script executed:
cat -n waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/accounts/EditableAccountStore.kt | head -80Repository: walt-id/waltid-identity
Length of output: 1663
🏁 Script executed:
cat -n waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/Web3.kt | head -300Repository: walt-id/waltid-identity
Length of output: 6834
🏁 Script executed:
rg "registration amendment" -i -B 5 -A 10Repository: walt-id/waltid-identity
Length of output: 49
Don't derive the local account id from bare sub.
The code drops the issuer context when using the raw subject as the account ID. Since OIDC only guarantees sub uniqueness per issuer, two issuers reusing the same UUID-like sub will incorrectly collapse into one account. Additionally, providers with non-UUID sub values will fail first login at the UUID.fromString(subject) requirement check.
Instead, mint a fresh internal account id (or reuse the registration amendment flow as shown in Web3.kt) and then link the full OIDCIdentifier(issuer, subject) to that account. Also verify that addAccountIdentifierToAccount(...) creates the backing account record, since the EditableAccountStore interface only promises identifier attachment without explicitly guaranteeing account creation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.kt`
around lines 223 - 230, Currently the code uses the raw subject as the local
account id (via the UUID.fromString check and returning subject), which loses
issuer context and breaks non-UUID subs; instead, generate/mint a new internal
account id (or reuse the registration amendment flow used in Web3.kt) and attach
the full OIDCIdentifier(issuer, subject) to that account; update the logic
around identifier.resolveIfExists() to only reuse existing account ids when the
identifier is already linked, call
KtorAuthnzManager.accountStore.addAccountIdentifierToAccount(accountId,
OIDCIdentifier(issuer, subject)) to link the identifier, and ensure you
explicitly create the backing account record if
EditableAccountStore.addAccountIdentifierToAccount does not guarantee account
creation (create the account first then attach identifier).
There was a problem hiding this comment.
🧩 Analysis chain
Thanks for confirming, @waltkb! Let me pull the relevant code to propose a concrete fix.
🏁 Script executed:
#!/bin/bash
echo "=== Web3.kt registration flow ==="
cat -n waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/Web3.kt | grep -A 30 "registerAccount\|createAccount\|addAccountIdentifier\|amendment\|AuthMethodFunctionAmendments"
echo ""
echo "=== EditableAccountStore interface ==="
cat -n waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/accounts/EditableAccountStore.kt
echo ""
echo "=== addAccountIdentifierToAccount implementations ==="
rg "fun addAccountIdentifierToAccount" -B 3 -A 20
echo ""
echo "=== AuthMethodFunctionAmendments ==="
rg "AuthMethodFunctionAmendments" -B 2 -A 5 --type kotlin | head -80
echo ""
echo "=== createAccount or registerAccount in account stores ==="
rg "fun createAccount\|fun registerAccount\|fun addAccount" -B 3 -A 15 --type kotlin | head -120
Length of output: 25622
@waltkb, here's the concrete fix, mirroring the pattern already established in Web3.kt:
// --- current (problematic) ---
val accountId = identifier.resolveIfExists() ?: run {
require(runCatching { UUID.fromString(subject) }.isSuccess) {
"OIDC subject must be a UUID-compatible account id, but got: $subject"
}
KtorAuthnzManager.accountStore.addAccountIdentifierToAccount(subject, identifier)
subject
}
// --- suggested ---
val accountId = identifier.resolveIfExists() ?: run {
val registrationFunction = functionAmendments?.get(AuthMethodFunctionAmendments.Registration)
?: error("No existing account found for OIDC identifier $identifier, and no registration amendment is configured.")
registrationFunction.invoke(identifier)
identifier.resolveToAccountId()
}Why this is correct:
| Problem with current code | How the fix addresses it |
|---|---|
UUID constraint blocks non-UUID sub values |
No UUID assumption; any sub format works |
Raw subject used as account ID loses issuer context |
OIDCIdentifier(issuer, subject) is passed to the amendment; account ID is minted internally |
Two issuers sharing the same UUID sub collapse into one account |
Each registration links the full OIDCIdentifier; identifiers are distinct by issuer |
addAccountIdentifierToAccount doesn't guarantee account creation per the EditableAccountStore interface (confirmed — ExampleAccountStore only updates the identifier map) |
The Registration amendment is the application owner's hook for both account creation and identifier linking, as intended by the framework design |
The functionAmendments parameter is already in scope (it's a parameter of registerAuthenticationRoutes), and resolveToAccountId() is already confirmed to exist from Web3.kt line 149.
🧠 Learnings used
Learnt from: waltkb
Repo: walt-id/waltid-identity PR: 1597
File: waltid-libraries/protocols/waltid-openid4vp-verifier/src/commonMain/kotlin/id/walt/verifier2/data/VerificationSessionSetupData.kt:327-348
Timestamp: 2026-03-18T09:25:33.787Z
Learning: In `waltid-libraries/protocols/waltid-openid4vp-verifier/src/commonMain/kotlin/id/walt/verifier2/data/VerificationSessionSetupData.kt`, waltkb intentionally keeps belt-and-suspenders validation checks in the `DcApiAnnexCFlowSetup` init blocks (e.g., verifying `coreInput?.dcqlQuery == core.dcqlQuery` and `coreInput?.policies == core.policies`), even if they appear redundant or dead code. Do not suggest removing these defensive checks.
Learnt from: waltkb
Repo: walt-id/waltid-identity PR: 1597
File: waltid-services/waltid-verifier-api2/src/test/kotlin/id/walt/verifier2/mdocs/PidBirthDateIssuerSignedIntegrityReproTest.kt:55-59
Timestamp: 2026-03-18T09:27:54.077Z
Learning: In `waltid-services/waltid-verifier-api2/src/test/kotlin/id/walt/verifier2/mdocs/PidBirthDateIssuerSignedIntegrityReproTest.kt`, `wrapped.value.elementValue` (i.e., `IssuerSignedItem.elementValue`) is of type `CborElement`, not `LocalDate` or `String`. This is because `IssuerSignedItem.elementValue` was changed from `Any` to `CborElement` as part of the CBOR overhaul in PR `#1597`. Assertions on `elementValue` should check for `CborElement`.
Learnt from: waltkb
Repo: walt-id/waltid-identity PR: 1597
File: waltid-libraries/credentials/waltid-mdoc-credentials2/src/commonMain/kotlin/id/walt/mdoc/schema/MdocsSchemaMappingFunction.kt:117-118
Timestamp: 2026-03-18T09:10:03.293Z
Learning: In `waltid-libraries/credentials/waltid-mdoc-credentials2/src/commonMain/kotlin/id/walt/mdoc/schema/MdocsSchemaMappingFunction.kt`, the `toCborElement()` extension uses `!!` on List and Map elements. Do NOT suggest `mapNotNull` or element-dropping approaches here: CBOR list element positions are significant for MSO digest/hash verification, so silently dropping nulls would corrupt hashes. If null-safety is needed, map nulls to `CborNull()` to preserve element count and positions.
Learnt from: cpatsonakis
Repo: walt-id/waltid-identity PR: 1451
File: waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/X509.kt:4-4
Timestamp: 2026-01-15T07:23:30.573Z
Learning: Do not require OptIn(ExperimentalEncodingApi::class) for Kotlin 2.2.0 or later when using kotlin.io.encoding.Base64, as it is stable in Kotlin 2.2.0+. In Kotlin files (any .kt under the repo) review for unnecessary OptIn usage related to Encoding API and remove it if the project targets Kotlin 2.2.0+. Also verify the Kotlin version in build files to ensure this stability claim holds for the codebase.
| httpOnly = true, | ||
| secure = secure, | ||
| extensions = mapOf("SameSite" to "Strict") | ||
| extensions = mapOf("SameSite" to "Lax") |
There was a problem hiding this comment.
Avoid relaxing the shared auth cookie for every flow.
SessionTokenCookieHandler is generic, so changing the default to SameSite=Lax lowers CSRF protection for every ktor-authnz-auth session, not just the enterprise OIDC callback/front-channel logout paths. If only those routes need cross-site delivery, please scope this to OIDC or make it configurable per auth method instead of changing the global default.
Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/sessions/SessionTokenCookieHandler.kt`
at line 25, The change sets extensions = mapOf("SameSite" to "Lax") in
SessionTokenCookieHandler which relaxes SameSite for all sessions; revert the
global default to a safe value (e.g., omit SameSite or set "Strict") in
SessionTokenCookieHandler and instead add a configurable parameter or setter on
SessionTokenCookieHandler (or its factory) to allow per-auth-method overrides;
update OIDC-specific code (the OIDC callback/front-channel logout flow that
currently constructs/uses SessionTokenCookieHandler) to explicitly request
SameSite="Lax" when needed so only the OIDC flow uses the relaxed cookie policy.
There was a problem hiding this comment.
@waltkb, glad we're aligned! Here's a concrete way to fix this — add a configurable sameSite property to SessionTokenCookieHandler (defaulting back to "Strict"), and have the OIDC auth method override it to "Lax" during its setup:
SessionTokenCookieHandler.kt — restore safe default, add configurable property:
object SessionTokenCookieHandler {
/** domain to set cookie on */
var domain: String? = null
/** secure value for cookie */
var secure: Boolean = true
var cookieName = "ktor-authnz-auth"
/** SameSite policy; defaults to Strict. Set to Lax for OIDC flows that require cross-site redirects. */
var sameSite: String = "Strict"
fun ApplicationCall.setCookie(token: String) {
response.cookies.append(
name = cookieName,
value = token,
domain = domain,
path = "/",
httpOnly = true,
secure = secure,
extensions = mapOf("SameSite" to sameSite)
)
}
fun RoutingCall.deleteCookie() {
response.cookies.append(
name = cookieName,
value = "",
maxAge = 0,
expires = GMTDate(),
domain = domain,
path = "/",
httpOnly = true,
secure = secure,
extensions = mapOf("SameSite" to sameSite)
)
}
}Then, wherever OIDC is configured (e.g., in your enterprise OIDC setup block), opt in to the relaxed policy explicitly:
SessionTokenCookieHandler.sameSite = "Lax"This keeps the global default safe ("Strict") and makes the OIDC relaxation intentional and visible at the call site, rather than silently affecting all auth methods.
🧠 Learnings used
Learnt from: waltkb
Repo: walt-id/waltid-identity PR: 1597
File: waltid-libraries/protocols/waltid-openid4vp-verifier/src/commonMain/kotlin/id/walt/verifier2/data/VerificationSessionSetupData.kt:327-348
Timestamp: 2026-03-18T09:25:33.787Z
Learning: In `waltid-libraries/protocols/waltid-openid4vp-verifier/src/commonMain/kotlin/id/walt/verifier2/data/VerificationSessionSetupData.kt`, waltkb intentionally keeps belt-and-suspenders validation checks in the `DcApiAnnexCFlowSetup` init blocks (e.g., verifying `coreInput?.dcqlQuery == core.dcqlQuery` and `coreInput?.policies == core.policies`), even if they appear redundant or dead code. Do not suggest removing these defensive checks.
Learnt from: cpatsonakis
Repo: walt-id/waltid-identity PR: 1451
File: waltid-libraries/crypto/waltid-x509/src/commonMain/kotlin/id/walt/x509/X509.kt:4-4
Timestamp: 2026-01-15T07:23:30.573Z
Learning: Do not require OptIn(ExperimentalEncodingApi::class) for Kotlin 2.2.0 or later when using kotlin.io.encoding.Base64, as it is stable in Kotlin 2.2.0+. In Kotlin files (any .kt under the repo) review for unnecessary OptIn usage related to Encoding API and remove it if the project targets Kotlin 2.2.0+. Also verify the Kotlin version in build files to ensure this stability claim holds for the codebase.
| @@ -1,10 +1,12 @@ | |||
| package id.walt.ktorauthnz.methods | |||
|
|
|||
| import com.sun.tools.javac.code.TypeAnnotationPosition.field | |||
| import kotlin.time.Clock | ||
| import kotlin.time.Duration.Companion.seconds | ||
| import kotlin.time.Instant | ||
| import java.util.UUID |
waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.kt
Show resolved
Hide resolved
| val accountId = identifier.resolveIfExists() ?: run { | ||
| require(runCatching { UUID.fromString(subject) }.isSuccess) { | ||
| "OIDC subject must be a UUID-compatible account id, but got: $subject" | ||
| } | ||
|
|
||
| /*if (accountId == null) { | ||
|
|
||
| // TODO: Create account if it does not exist | ||
| Account("", "") | ||
| ExampleAccountStore.registerAccount() | ||
| }*/ | ||
| KtorAuthnzManager.accountStore.addAccountIdentifierToAccount(subject, identifier) | ||
| subject | ||
| } |
| httpOnly = true, | ||
| secure = secure, | ||
| extensions = mapOf("SameSite" to "Strict") | ||
| extensions = mapOf("SameSite" to "Lax") |
| httpOnly = true, | ||
| secure = secure, | ||
| extensions = mapOf("SameSite" to "Strict") | ||
| extensions = mapOf("SameSite" to "Lax") |
2 participants
Uh oh!
There was an error while loading. Please reload this page.