From 1f6aa3c9f20149e052107a4d5fab7919c5617af1 Mon Sep 17 00:00:00 2001 From: UnknownJoe796 Date: Tue, 7 Jul 2026 00:46:04 -0600 Subject: [PATCH 1/2] Add PKCE and single-use state CSRF protection to OAuth client flow The OAuth client flow previously round-tripped a caller-supplied `state` through the provider without storing or validating it (TODO acknowledged in-code), and used no PKCE. - state/CSRF: the value sent to the provider is now an opaque 256-bit single-use nonce. At flow start a FlowRecord (caller state + PKCE verifier) is stored in the cache keyed by the nonce with a short TTL; the callback validates and consumes it via getAndRemove BEFORE any token exchange, so unknown/expired/replayed callbacks are rejected. The caller's app STATE is preserved server-side and still delivered to onAccess; it no longer leaks to the provider. - PKCE (RFC 7636): S256 code_challenge on the auth redirect and code_verifier on token exchange, gated by a per-provider supportsPkce flag (default true). Verified against the RFC Appendix B test vector. - Confirmed the redirect_uri (fixed callback URL) and final UI redirect (driven only by a server-signed Proof) are not attacker-controllable; documented rather than adding an unnecessary whitelist. Also fixes two latent bugs uncovered here: a NPE on code.state!! and an onError result that was computed then discarded (execution fell through). Requires a cache: OauthCallbackEndpoint/OauthProofEndpoints take a Runtime and loginUrl is now suspend (source-breaking on the 5.x line). Docs note the shared-cache requirement for multi-instance deployments and the residual login-CSRF limitation of cookie-less state. Co-Authored-By: Claude Fable 5 (cherry picked from commit d0bc38c27e25590fcfe9428d214acbce459b30cd) (cherry picked from commit 72e2eed22bb3460c39193507701293f68a5fedb3) --- .../lightningserver/demo/Server.kt | 1 + .../sessions/proofs/oauth/models.kt | 6 + .../sessions/proofs/OauthProofEndpoints.kt | 15 +- .../proofs/oauth/OauthCallbackEndpoint.kt | 99 +++++++++++-- .../proofs/oauth/OauthProviderInfo.kt | 9 ++ .../sessions/proofs/oauth/Pkce.kt | 31 ++++ .../proofs/OauthCallbackSecurityTest.kt | 135 ++++++++++++++++++ 7 files changed, 279 insertions(+), 17 deletions(-) create mode 100644 sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/Pkce.kt create mode 100644 sessions-oauth/src/test/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthCallbackSecurityTest.kt diff --git a/demo/src/main/kotlin/com/lightningkite/lightningserver/demo/Server.kt b/demo/src/main/kotlin/com/lightningkite/lightningserver/demo/Server.kt index c4307e034..3441794e2 100644 --- a/demo/src/main/kotlin/com/lightningkite/lightningserver/demo/Server.kt +++ b/demo/src/main/kotlin/com/lightningkite/lightningserver/demo/Server.kt @@ -249,6 +249,7 @@ object Server : ServerBuilder() { val proofDevices = path.path("proof").path("devices") module KnownDeviceProofEndpoints(database, cache) val proofOauth = path.path("proof").path("github") module OauthProofEndpoints( provider = OauthProviderInfo.github, + cache = cache, credentials = githubOauth, continueUiAuthUrl = { autosignIn.location.path.resolved().fullUrl() + "?proof=" + serverRuntime.externalSerialization.json.encodeToString(Proof.serializer(), it).encodeURLQueryComponent() + "&backend=" + generalSettings().publicUrl.encodeURLQueryComponent() } ) diff --git a/sessions-oauth-shared/src/commonMain/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/models.kt b/sessions-oauth-shared/src/commonMain/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/models.kt index 20c68ca3f..14417433d 100644 --- a/sessions-oauth-shared/src/commonMain/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/models.kt +++ b/sessions-oauth-shared/src/commonMain/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/models.kt @@ -79,6 +79,8 @@ public data class OauthTokenRequest( val client_secret: String, val redirect_uri: String? = null, val grant_type: String = "authorization_code", + /** PKCE (RFC 7636) code verifier proving this client started the flow. Omitted when PKCE is disabled. */ + val code_verifier: String? = null, ) @Serializable @@ -101,6 +103,10 @@ public data class OauthCodeRequest( val prompt: OauthPromptType? = null, val login_hint: String? = null, val sessionExpiration: Instant? = null, + /** PKCE (RFC 7636) code challenge = BASE64URL-NOPAD(SHA256(code_verifier)). Omitted when PKCE is disabled. */ + val code_challenge: String? = null, + /** PKCE transformation method; always "S256" when [code_challenge] is present. */ + val code_challenge_method: String? = null, ) @Serializable diff --git a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthProofEndpoints.kt b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthProofEndpoints.kt index 87e978be0..60766e1d4 100644 --- a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthProofEndpoints.kt +++ b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthProofEndpoints.kt @@ -18,6 +18,7 @@ import com.lightningkite.lightningserver.typed.ApiHttpHandler import com.lightningkite.lightningserver.typed.sdk.SdkModule import com.lightningkite.lightningserver.typed.sdk.SdkModule.Companion.defaultInfo import com.lightningkite.lightningserver.typed.sdk.sdkSettings +import com.lightningkite.services.cache.Cache import com.lightningkite.services.database.HasId import io.ktor.http.* import kotlin.time.Duration @@ -51,8 +52,13 @@ import kotlin.uuid.Uuid * ) * ``` * + * **Security:** The flow is protected against CSRF via a single-use `state` nonce and against + * authorization-code interception via PKCE (RFC 7636). Both are held in [cache] for the duration of + * the redirect round-trip and validated/consumed on the callback. + * * @param proofSigner The signer used to create cryptographic proofs (defaults to derived from secretBasis) * @param provider The OAuth provider configuration (Google, Apple, Microsoft, GitHub, or custom) + * @param cache Cache holding the transient CSRF `state` and PKCE verifier between redirect and callback * @param credentials Function that returns the OAuth client credentials (ID and secret) * @param continueUiAuthUrl Function that returns the UI URL to redirect to after successful authentication * @@ -61,6 +67,7 @@ import kotlin.uuid.Uuid */ public class OauthProofEndpoints( private val provider: OauthProviderInfo, + private val cache: Runtime, override val proofSigner: RuntimeDeferred = secretBasis.signer("proof"), override val proofExpiration: Duration = 1.hours, private val credentials: Runtime, @@ -88,9 +95,14 @@ public class OauthProofEndpoints( stateSerializer = serializerOrContextual(), oauthProviderInfo = provider, credentials = credentials, + cache = cache, ) { response: OauthResponse, _: Uuid -> val profile = provider.getProfile(response, credentials()) val email = profile.email ?: throw BadRequestException("No email was found for this profile.") + // Open-redirect note: the final destination is produced entirely by the app-supplied + // `continueUiAuthUrl` from a server-generated, signed Proof. No user- or attacker-controllable + // value (query param or `state`) feeds into it, so the redirect target is app-controlled and + // does not require redirect-URI whitelisting here. HttpResponse.redirectToGet( continueUiAuthUrl( proofSigner.await().makeProof( @@ -161,7 +173,4 @@ public class OauthProofEndpoints( * * 5. Consider adding telemetry/metrics for OAuth login attempts, successes, and failures * to help diagnose provider-specific issues. - * - * 6. The UUID state parameter in callback is generated but not validated. Consider using the - * state parameter for CSRF protection by storing and validating it. */ \ No newline at end of file diff --git a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthCallbackEndpoint.kt b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthCallbackEndpoint.kt index 7ed1e6fc8..ed3ec7f56 100644 --- a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthCallbackEndpoint.kt +++ b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthCallbackEndpoint.kt @@ -1,5 +1,6 @@ package com.lightningkite.lightningserver.sessions.proofs.oauth +import com.lightningkite.lightningserver.BadRequestException import com.lightningkite.lightningserver.definition.Runtime import com.lightningkite.lightningserver.definition.builder.ServerBuilder import com.lightningkite.lightningserver.http.* @@ -10,44 +11,115 @@ import com.lightningkite.lightningserver.runtime.ServerRuntime import com.lightningkite.lightningserver.runtime.location import com.lightningkite.lightningserver.serialization.* import com.lightningkite.lightningserver.sessions.proofs.oauth.path +import com.lightningkite.services.cache.Cache +import com.lightningkite.services.cache.getAndRemove +import com.lightningkite.services.cache.set import kotlinx.serialization.KSerializer +import kotlinx.serialization.Serializable import kotlinx.serialization.json.Json +import kotlin.time.Duration +import kotlin.time.Duration.Companion.minutes public class OauthCallbackEndpoint( path: PathSpec0, public val stateSerializer: KSerializer, public val oauthProviderInfo: OauthProviderInfo, public val credentials: Runtime, + /** + * Cache used to hold transient per-flow data (the CSRF `state` marker and the PKCE code verifier) + * between the authorization redirect and the callback. Entries are single-use and short-lived. + * + * In multi-instance or serverless deployments this MUST be a shared cache (e.g. Redis, DynamoDB), + * not the in-memory `"ram"` cache: the callback can land on a different instance than the one that + * started the flow, and an unshared cache would fail to find the flow record, breaking login. + */ + public val cache: Runtime, public val defaultScope: String = oauthProviderInfo.scopeForProfile, public val defaultAccessType: OauthAccessType = OauthAccessType.online, + /** + * How long an in-progress OAuth flow may sit in the cache before its `state`/verifier expire. + * Must comfortably cover the user's time at the provider (login, consent, MFA). + */ + public val flowExpiration: Duration = 10.minutes, + /** + * Invoked when the provider redirects back with an error. NOTE: this runs before `state` is + * validated, so the [OauthCode] passed here is unauthenticated and fully attacker-controllable — + * do not trust its fields for anything security-sensitive. The default simply throws. + */ public val onError: suspend context(ServerRuntime) (OauthCode) -> HttpResponse = { throw Exception("Got Oauth error from ${oauthProviderInfo.niceName}: ${it}") }, public val onAccess: suspend context(ServerRuntime) (OauthResponse, STATE) -> HttpResponse, ) : ServerBuilder() { + /** + * Transient record for a single in-progress OAuth flow, keyed in the cache by the opaque `state` + * nonce that the provider echoes back. Storing both values together means one cache lookup on the + * callback validates the CSRF `state` and retrieves the PKCE verifier and caller state. + * + * `internal` (not `public`) so in-module tests can inspect stored flows; not part of the public API. + */ + @Serializable + internal data class FlowRecord( + /** The caller's `STATE` value, serialized with [stateSerializer], preserved across the round-trip. */ + val state: String, + /** The PKCE code verifier for this flow, or null when the provider does not support PKCE. */ + val codeVerifier: String?, + ) + + internal fun flowKey(nonce: String): String = "oauth-flow-${oauthProviderInfo.identifierName}-$nonce" + context(runtime: ServerRuntime) public suspend fun handle(code: OauthCode): HttpResponse { - code.error?.let { onError(code) } - val response = oauthProviderInfo.accessToken(credentials, callback.location.path.resolved().fullUrl(), code) - return onAccess(response, runtime.externalSerialization.json.decodeFromString(stateSerializer, code.state!!)) + code.error?.let { return onError(code) } + // CSRF protection (and PKCE): the `state` the provider echoed back must match a stored, + // unconsumed flow we issued. getAndRemove enforces single use, so a replayed callback fails. + // Limitation: the nonce is stored server-side but not bound to the initiating browser (this is + // a cookie-less proof design). That still stops the classic forged-callback CSRF, but not + // "login CSRF" where an attacker completes their own flow and hands the victim the resulting + // callback URL. Bind the flow to a browser cookie if that threat is in scope. + val nonce = code.state ?: throw BadRequestException("Missing OAuth state parameter.") + val record = cache().getAndRemove(flowKey(nonce)) + ?: throw BadRequestException("Invalid, expired, or already-used OAuth state.") + val response = oauthProviderInfo.accessToken( + credentials, + callback.location.path.resolved().fullUrl(), + code, + codeVerifier = record.codeVerifier, + ) + return onAccess(response, runtime.externalSerialization.json.decodeFromString(stateSerializer, record.state)) } context(runtime: ServerRuntime) - public fun loginUrl( + public suspend fun loginUrl( state: STATE, scope: String = defaultScope, accessType: OauthAccessType = defaultAccessType, loginHint: String? = null, - ): String = oauthProviderInfo.loginUrl( - credentials = credentials, - redirectUri = callback.location.path.resolved().fullUrl(), - scope = scope, - state = runtime.externalSerialization.json.encodeToString(stateSerializer, state), - accessType = accessType, - loginHint = loginHint, - prompt = OauthPromptType.select_account - ) + ): String { + // The `state` we send to the provider is an opaque, single-use nonce (not the caller state), + // so nothing sensitive leaks through the provider and callbacks can be validated for CSRF. + val nonce = randomUrlToken() + val codeVerifier = if (oauthProviderInfo.supportsPkce) generatePkceCodeVerifier() else null + cache().set( + flowKey(nonce), + FlowRecord( + state = runtime.externalSerialization.json.encodeToString(stateSerializer, state), + codeVerifier = codeVerifier, + ), + flowExpiration, + ) + return oauthProviderInfo.loginUrl( + credentials = credentials, + redirectUri = callback.location.path.resolved().fullUrl(), + scope = scope, + state = nonce, + accessType = accessType, + loginHint = loginHint, + prompt = OauthPromptType.select_account, + codeChallenge = codeVerifier?.let { pkceCodeChallengeS256(it) }, + ) + } public val callback: HttpHandler = when (oauthProviderInfo.mode) { OauthResponseMode.form_post -> { @@ -67,4 +139,3 @@ public class OauthCallbackEndpoint( public suspend fun accessToken(refreshToken: String): OauthResponse = oauthProviderInfo.accessToken(credentials, refreshToken) } - diff --git a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthProviderInfo.kt b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthProviderInfo.kt index 173e234bf..20dc9bb46 100644 --- a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthProviderInfo.kt +++ b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthProviderInfo.kt @@ -61,6 +61,9 @@ import kotlin.uuid.Uuid * @property mode How the OAuth provider sends the authorization code (form_post or query) * @property settings Configuration for credentials serialization (standard or provider-specific) * @property scopeForProfile OAuth scopes required to retrieve user profile information + * @property supportsPkce Whether this provider accepts PKCE (RFC 7636) parameters on the authorization + * and token requests. Defaults to `true`; all major providers support (and recommend) PKCE. Set to + * `false` only for a non-compliant provider that rejects unknown `code_challenge`/`code_verifier` params. * @property getProfile Async function that retrieves user profile from the provider */ public class OauthProviderInfo( @@ -73,6 +76,7 @@ public class OauthProviderInfo( public val mode: OauthResponseMode = OauthResponseMode.form_post, public val settings: SettingInfo<*> = SettingInfo.standard, public val scopeForProfile: String, + public val supportsPkce: Boolean = true, public val getProfile: suspend context(ServerRuntime) (OauthResponse, OauthProviderCredentials?) -> ExternalProfile, ) { public data class SettingInfo( @@ -96,6 +100,7 @@ public class OauthProviderInfo( accessType: OauthAccessType = OauthAccessType.online, prompt: OauthPromptType? = if (accessType == OauthAccessType.offline) OauthPromptType.consent else null, loginHint: String? = null, + codeChallenge: String? = null, ): String { val params = OauthCodeRequest( response_type = "code", @@ -107,6 +112,8 @@ public class OauthProviderInfo( access_type = accessType, prompt = prompt, login_hint = loginHint, + code_challenge = codeChallenge, + code_challenge_method = codeChallenge?.let { "S256" }, ).let { FormDataFormat(EmptySerializersModule()).encodeToString(OauthCodeRequest.serializer(), it) } return "$loginUrl?$params" } @@ -116,6 +123,7 @@ public class OauthProviderInfo( credentials: Runtime, redirectUri: String, oauth: OauthCode, + codeVerifier: String? = null, ): OauthResponse { oauth.error?.let { throw BadRequestException("Got error code '${it}' from $niceName.") @@ -130,6 +138,7 @@ public class OauthProviderInfo( client_secret = credentials().secret, redirect_uri = redirectUri, grant_type = OauthGrantTypes.authorizationCode, + code_verifier = codeVerifier, ) ) ) diff --git a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/Pkce.kt b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/Pkce.kt new file mode 100644 index 000000000..a908575dd --- /dev/null +++ b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/Pkce.kt @@ -0,0 +1,31 @@ +package com.lightningkite.lightningserver.sessions.proofs.oauth + +import java.security.MessageDigest +import java.security.SecureRandom +import kotlin.io.encoding.Base64 + +/** + * Helpers implementing PKCE (Proof Key for Code Exchange, RFC 7636) and the CSRF `state` nonce + * used to secure the OAuth authorization-code flow. + * + * All values are drawn from a cryptographically secure random source and encoded with the + * URL-safe, unpadded BASE64URL alphabet (which is a subset of the RFC 3986 unreserved charset), + * so they are safe to place directly in URLs. + */ +private val secureRandom = SecureRandom() + +/** BASE64URL encoding without padding, per RFC 7636 Appendix A. */ +private fun base64UrlNoPad(bytes: ByteArray): String = Base64.UrlSafe.encode(bytes).trimEnd('=') + +/** Generates an opaque, high-entropy token (43 chars) suitable for a `state` nonce or cache key. */ +internal fun randomUrlToken(): String = base64UrlNoPad(ByteArray(32).also(secureRandom::nextBytes)) + +/** + * Generates a PKCE `code_verifier`: a 43-character high-entropy string from the unreserved + * charset, satisfying RFC 7636's 43-128 character requirement. + */ +internal fun generatePkceCodeVerifier(): String = randomUrlToken() + +/** Computes the PKCE S256 `code_challenge` = BASE64URL-NOPAD(SHA256(verifier)). */ +internal fun pkceCodeChallengeS256(verifier: String): String = + base64UrlNoPad(MessageDigest.getInstance("SHA-256").digest(verifier.encodeToByteArray())) diff --git a/sessions-oauth/src/test/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthCallbackSecurityTest.kt b/sessions-oauth/src/test/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthCallbackSecurityTest.kt new file mode 100644 index 000000000..381e1f8fd --- /dev/null +++ b/sessions-oauth/src/test/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthCallbackSecurityTest.kt @@ -0,0 +1,135 @@ +package com.lightningkite.lightningserver.sessions.proofs + +import com.lightningkite.lightningserver.BadRequestException +import com.lightningkite.lightningserver.definition.Runtime +import com.lightningkite.lightningserver.definition.builder.ServerBuilder +import com.lightningkite.lightningserver.http.HttpResponse +import com.lightningkite.lightningserver.plainText +import com.lightningkite.lightningserver.runtime.test.test +import com.lightningkite.lightningserver.serialization.serializerOrContextual +import com.lightningkite.lightningserver.sessions.proofs.oauth.* +import com.lightningkite.services.cache.Cache +import com.lightningkite.services.cache.get +import com.lightningkite.services.cache.getAndRemove +import io.ktor.http.* +import kotlinx.coroutines.runBlocking +import org.junit.Test +import kotlin.test.* +import kotlin.uuid.Uuid + +/** + * Security tests for the OAuth callback: CSRF `state` validation/consumption and PKCE (RFC 7636). + * + * These exercise the parts of the flow that do not require a live provider. The token exchange and + * profile fetch (outgoing HTTP) are never reached: every negative case is rejected during state + * validation, which happens before any network call. + */ +class OauthCallbackSecurityTest { + + private fun testServer(supportsPkce: Boolean = true) = object : ServerBuilder() { + val cache = setting("cache", Cache.Settings("ram")) + val provider = OauthProviderInfo( + niceName = "TestProvider", + loginUrl = "https://provider.example/authorize", + tokenUrl = "https://provider.example/token", + scopeForProfile = "email", + mode = OauthResponseMode.query, + supportsPkce = supportsPkce, + getProfile = { _, _ -> ExternalProfile(email = "user@example.com") }, + ) + val callback: OauthCallbackEndpoint = path.path("cb") include OauthCallbackEndpoint( + path = path, + stateSerializer = serializerOrContextual(), + oauthProviderInfo = provider, + credentials = Runtime.Constant(OauthProviderCredentials("client-id", "client-secret")), + cache = cache, + onAccess = { _, _ -> HttpResponse.plainText("ok") }, + ) + } + + @Test + fun `login url carries pkce challenge and stored verifier hashes to it`() = runBlocking { + val server = testServer() + server.test({}) { + val state = Uuid.random() + val url = server.callback.loginUrl(state) + val params = Url(url).parameters + + assertEquals("S256", params["code_challenge_method"]) + val challenge = params["code_challenge"] + assertNotNull(challenge, "authorization URL must include a code_challenge") + + val nonce = params["state"]!! + val record = server.cache().get(server.callback.flowKey(nonce)) + assertNotNull(record, "loginUrl must persist the flow keyed by the state nonce") + val verifier = assertNotNull(record.codeVerifier) + assertEquals(challenge, pkceCodeChallengeS256(verifier), "stored verifier must hash to the challenge") + } + } + + @Test + fun `pkce can be disabled per provider`() = runBlocking { + val server = testServer(supportsPkce = false) + server.test({}) { + val url = server.callback.loginUrl(Uuid.random()) + val params = Url(url).parameters + assertNull(params["code_challenge"]) + assertNull(params["code_challenge_method"]) + val record = server.cache().get(server.callback.flowKey(params["state"]!!)) + assertNull(assertNotNull(record).codeVerifier) + } + } + + @Test + fun `callback with unknown state is rejected`() = runBlocking { + val server = testServer() + server.test({}) { + assertFailsWith { + server.callback.handle(OauthCode(code = "any", state = "never-issued")) + } + } + } + + @Test + fun `callback with missing state is rejected`() = runBlocking { + val server = testServer() + server.test({}) { + assertFailsWith { + server.callback.handle(OauthCode(code = "any", state = null)) + } + } + } + + @Test + fun `state is single-use`() = runBlocking { + val server = testServer() + server.test({}) { + val url = server.callback.loginUrl(Uuid.random()) + val nonce = Url(url).parameters["state"]!! + // Simulate the first callback consuming the flow (getAndRemove is the single-use mechanism). + assertNotNull(server.cache().getAndRemove(server.callback.flowKey(nonce))) + // A second callback with the same state must be rejected before any token exchange. + assertFailsWith { + server.callback.handle(OauthCode(code = "any", state = nonce)) + } + } + } + + @Test + fun `pkce challenge matches rfc 7636 test vector`() { + // RFC 7636 Appendix B worked example. + assertEquals( + "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + pkceCodeChallengeS256("dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"), + ) + } + + @Test + fun `generated verifier meets rfc 7636 length and charset`() { + repeat(50) { + val v = generatePkceCodeVerifier() + assertTrue(v.length in 43..128, "verifier length ${v.length} out of range") + assertTrue(v.all { it.isLetterOrDigit() || it in "-._~" }, "verifier has non-unreserved chars: $v") + } + } +} From 7e998872083f7e90d3c44fc4b5685e8d8d8984f0 Mon Sep 17 00:00:00 2001 From: UnknownJoe796 Date: Wed, 29 Jul 2026 17:08:56 -0600 Subject: [PATCH 2/2] Add a real token-exchange test for the OAuth PKCE flow --- .../proofs/oauth/OauthProviderInfo.kt | 19 +- .../sessions/proofs/OauthTokenExchangeTest.kt | 181 ++++++++++++++++++ 2 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 sessions-oauth/src/test/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthTokenExchangeTest.kt diff --git a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthProviderInfo.kt b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthProviderInfo.kt index 20dc9bb46..4e41c5b44 100644 --- a/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthProviderInfo.kt +++ b/sessions-oauth/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/oauth/OauthProviderInfo.kt @@ -128,7 +128,7 @@ public class OauthProviderInfo( oauth.error?.let { throw BadRequestException("Got error code '${it}' from $niceName.") } ?: oauth.code?.let { code -> - return client.post(tokenUrl) { + val httpResponse = client.post(tokenUrl) { setBody( FormDataFormat(EmptySerializersModule()).encodeToString( OauthTokenRequest.serializer(), @@ -144,7 +144,22 @@ public class OauthProviderInfo( ) contentType(ContentType.Application.FormUrlEncoded) accept(ContentType.Application.Json) - }.internalBody() + } + // Providers report token-exchange failures (expired/invalid code, PKCE mismatch, etc.) + // as a non-2xx status with an RFC 6749 §5.2 error body, e.g. {"error":"invalid_grant"}. + // That body doesn't satisfy OauthResponse's required fields, so decoding it directly would + // surface a raw MissingFieldException instead of a clean, expected BadRequestException. + // Check the status first and surface only the standardized `error` code - the provider's + // free-text `error_description` (or any other response content) is not relayed, since it's + // not meant for the end user and could contain provider-internal detail. + val bodyText = httpResponse.bodyAsText() + if (!httpResponse.status.isSuccess()) { + val errorCode = runCatching { + runtime.externalSerialization.json.parseToJsonElement(bodyText).jsonObject["error"]?.jsonPrimitive?.content + }.getOrNull() + throw BadRequestException("Token exchange with $niceName failed" + (errorCode?.let { " ($it)" } ?: ".")) + } + return runtime.externalSerialization.json.decodeFromString(OauthResponse.serializer(), bodyText) } throw BadRequestException("Code is empty") } diff --git a/sessions-oauth/src/test/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthTokenExchangeTest.kt b/sessions-oauth/src/test/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthTokenExchangeTest.kt new file mode 100644 index 000000000..39ad93c41 --- /dev/null +++ b/sessions-oauth/src/test/kotlin/com/lightningkite/lightningserver/sessions/proofs/OauthTokenExchangeTest.kt @@ -0,0 +1,181 @@ +package com.lightningkite.lightningserver.sessions.proofs + +import com.lightningkite.lightningserver.BadRequestException +import com.lightningkite.lightningserver.definition.Runtime +import com.lightningkite.lightningserver.definition.builder.ServerBuilder +import com.lightningkite.lightningserver.http.HttpResponse +import com.lightningkite.lightningserver.plainText +import com.lightningkite.lightningserver.runtime.test.test +import com.lightningkite.lightningserver.serialization.serializerOrContextual +import com.lightningkite.lightningserver.sessions.proofs.oauth.* +import com.lightningkite.services.cache.Cache +import com.lightningkite.services.cache.get +import com.sun.net.httpserver.HttpServer +import io.ktor.http.* +import kotlinx.coroutines.runBlocking +import org.junit.Test +import java.net.InetSocketAddress +import java.net.URLDecoder +import java.nio.charset.StandardCharsets +import kotlin.test.* +import kotlin.uuid.Uuid + +/** + * Exercises the OAuth token exchange itself: the outbound `client.post(tokenUrl)` call inside + * [OauthProviderInfo.accessToken], driven through [OauthCallbackEndpoint.handle]. + * + * [OauthCallbackSecurityTest] covers the CSRF `state` / PKCE bookkeeping (registration, single-use + * consumption, forgery/replay rejection), but every one of its cases is rejected during state + * validation, before `accessToken` ever runs. So the actual HTTP request sent to the token + * endpoint - and in particular whether the `code_verifier` PKCE proves anything - was never + * exercised, not even against a mock. This file closes that gap using a real loopback HTTP server + * standing in for the provider, so the genuine `HttpClient` used in production round-trips over a + * real socket exactly as it would against a live provider. + * + * NOT covered here: a real external OAuth server (e.g. Testcontainers + Keycloak) verifying our + * request against actual RFC 7636/6749 server-side validation logic. That was investigated + * separately and found infeasible in this sandbox/CI: no Docker available, and the macOS CI + * runner doesn't support Testcontainers on Apple Silicon. A future PR adding a Linux/Docker-capable + * CI job would need to add that end-to-end suite; tracked in plans/architecture-review-2026-07.md. + */ +class OauthTokenExchangeTest { + + /** Minimal loopback HTTP server standing in for the provider's token endpoint. */ + private class FakeTokenEndpoint( + private val respond: (body: String) -> Pair, + ) : AutoCloseable { + data class Captured(val path: String, val contentType: String?, val body: String) + + val capturedRequests: MutableList = mutableListOf() + + private val server: HttpServer = HttpServer.create(InetSocketAddress("127.0.0.1", 0), 0).apply { + createContext("/") { exchange -> + val body = exchange.requestBody.readBytes().toString(StandardCharsets.UTF_8) + capturedRequests += Captured( + path = exchange.requestURI.path, + contentType = exchange.requestHeaders.getFirst("Content-Type"), + body = body, + ) + val (status, responseBody) = respond(body) + val bytes = responseBody.toByteArray(StandardCharsets.UTF_8) + exchange.responseHeaders.add("Content-Type", "application/json") + exchange.sendResponseHeaders(status, bytes.size.toLong()) + exchange.responseBody.use { it.write(bytes) } + } + start() + } + + val tokenUrl: String get() = "http://127.0.0.1:${server.address.port}/token" + + override fun close() { + server.stop(0) + } + } + + /** Parses `application/x-www-form-urlencoded` body content into a lookup map for assertions. */ + private fun parseFormBody(body: String): Map = + body.split("&").filter { it.isNotEmpty() }.associate { + val key = it.substringBefore('=') + val value = it.substringAfter('=', "") + URLDecoder.decode(key, "UTF-8") to URLDecoder.decode(value, "UTF-8") + } + + private fun testServer(tokenUrl: String) = object : ServerBuilder() { + val cache = setting("cache", Cache.Settings("ram")) + + /** Captures the [OauthResponse] `onAccess` (and thus the completed login flow) actually saw. */ + var lastAccessResponse: OauthResponse? = null + + val provider = OauthProviderInfo( + niceName = "TestProvider", + loginUrl = "https://provider.example/authorize", + tokenUrl = tokenUrl, + scopeForProfile = "email", + mode = OauthResponseMode.query, + getProfile = { _, _ -> ExternalProfile(email = "user@example.com") }, + ) + val callback: OauthCallbackEndpoint = path.path("cb") include OauthCallbackEndpoint( + path = path, + stateSerializer = serializerOrContextual(), + oauthProviderInfo = provider, + credentials = Runtime.Constant(OauthProviderCredentials("test-client-id", "test-client-secret")), + cache = cache, + onAccess = { response, _ -> + lastAccessResponse = response + HttpResponse.plainText("welcome:${response.access_token}") + }, + ) + } + + @Test + fun `successful token exchange sends code, credentials, redirect uri and matching pkce verifier, and completes login`() = + runBlocking { + FakeTokenEndpoint({ _ -> + 200 to """{"access_token":"at-123","token_type":"Bearer","scope":"email"}""" + }).use { fake -> + val server = testServer(fake.tokenUrl) + server.test({}) { + // Register a real state + PKCE pair the way OauthProofEndpoints does at flow-start. + val callerState = Uuid.random() + val loginUrl = server.callback.loginUrl(callerState) + val params = Url(loginUrl).parameters + val nonce = params["state"]!! + val challengeSent = params["code_challenge"]!! + val record = server.cache().get(server.callback.flowKey(nonce))!! + val verifier = record.codeVerifier!! + assertEquals( + challengeSent, + pkceCodeChallengeS256(verifier), + "sanity check: the stored verifier must actually hash to the sent challenge", + ) + + val httpResponse = server.callback.handle(OauthCode(code = "auth-code-xyz", state = nonce)) + + assertEquals("welcome:at-123", httpResponse.body!!.text()) + assertEquals("at-123", server.lastAccessResponse?.access_token, "onAccess must complete the login flow with the exchanged token") + + assertEquals(1, fake.capturedRequests.size) + val request = fake.capturedRequests.single() + assertEquals("/token", request.path) + assertEquals(ContentType.Application.FormUrlEncoded.toString(), request.contentType) + val form = parseFormBody(request.body) + assertEquals("auth-code-xyz", form["code"]) + assertEquals("test-client-id", form["client_id"]) + assertEquals("test-client-secret", form["client_secret"]) + assertEquals("authorization_code", form["grant_type"]) + assertEquals( + verifier, + form["code_verifier"], + "token request must carry the code_verifier matching the code_challenge registered at flow-start - this is the PKCE guarantee", + ) + } + } + } + + @Test + fun `provider token error is surfaced as a clean BadRequestException, not a raw crash`() = runBlocking { + FakeTokenEndpoint({ _ -> + 400 to """{"error":"invalid_grant","error_description":"The authorization code is invalid or expired."}""" + }).use { fake -> + val server = testServer(fake.tokenUrl) + server.test({}) { + val loginUrl = server.callback.loginUrl(Uuid.random()) + val nonce = Url(loginUrl).parameters["state"]!! + + val error = assertFailsWith( + "a provider token-endpoint error must surface as a clean BadRequestException, not an unrelated internal exception", + ) { + server.callback.handle(OauthCode(code = "auth-code-xyz", state = nonce)) + } + assertTrue( + error.message.orEmpty().contains("invalid_grant"), + "error message should communicate the standard OAuth error code: ${error.message}", + ) + // The provider's free-text error_description (and any other raw response body content) + // must not be relayed verbatim - only the standardized `error` code is safe to surface. + assertFalse(error.message.orEmpty().contains("error_description")) + assertFalse(error.message.orEmpty().contains("invalid or expired")) + } + } + } +}