diff --git a/packages/docs/content/docs/getting-started/authentication.md b/packages/docs/content/docs/getting-started/authentication.md index 7af9627..bb7c4b6 100644 --- a/packages/docs/content/docs/getting-started/authentication.md +++ b/packages/docs/content/docs/getting-started/authentication.md @@ -482,7 +482,7 @@ X-Client-Key: hvc_... X-Client-Secret: hvs_... ``` -Public clients must provide a valid DPoP proof to prove they hold the key. This revokes only the session that matches the DPoP key used in the proof — other device sessions for the same user are unaffected: +Alternatively, provide a valid DPoP proof to prove you hold the key. This revokes only the session that matches the DPoP key used in the proof — other device sessions for the same user are unaffected: ``` DELETE /oauth/sessions/did:plc:user123 @@ -491,6 +491,10 @@ Authorization: DPoP DPoP: ``` +DPoP auth is accepted here from any client, confidential or public — the same credentials that authorise `/xrpc/*` calls. A confidential client using DPoP does not have to fall back to its secret just to log out. + +The `htu` in the proof must match the URL you actually requested, byte for byte. If you percent-encode the DID in the path, sign the encoded form. + To revoke a specific device session (for either client type), use the [device management endpoints](#6-managing-device-sessions) instead. #### 6. Managing device sessions @@ -536,7 +540,7 @@ X-Client-Key: hvc_... X-Client-Secret: hvs_... ``` -For public clients, use DPoP auth instead of `X-Client-Secret`: +Or use DPoP auth instead of `X-Client-Secret` — accepted from any client type: ``` DELETE /oauth/sessions/did:plc:user123/devices/uuid-session-1 diff --git a/packages/docs/content/docs/guides/api-clients.md b/packages/docs/content/docs/guides/api-clients.md index 06e24ec..333bbd9 100644 --- a/packages/docs/content/docs/guides/api-clients.md +++ b/packages/docs/content/docs/guides/api-clients.md @@ -574,7 +574,7 @@ X-Client-Key: hvc_... X-Client-Secret: hvs_... ``` -**Public** (must prove key possession): +**With a DPoP proof** (any client type — proves key possession, revokes just that device's session): ```http DELETE /oauth/sessions/did:plc:user123 @@ -583,6 +583,8 @@ Authorization: DPoP DPoP: ``` +The proof's `htu` must equal the URL as sent, including any percent-encoding in the DID. + ### DPoP proof format If you're implementing the flow without the SDK, a DPoP proof JWT looks like this: diff --git a/packages/docs/content/docs/sdk/oauth-client-browser/overview.md b/packages/docs/content/docs/sdk/oauth-client-browser/overview.md index d22f93c..e36236b 100644 --- a/packages/docs/content/docs/sdk/oauth-client-browser/overview.md +++ b/packages/docs/content/docs/sdk/oauth-client-browser/overview.md @@ -195,6 +195,14 @@ await client.revoke(session.did); `logout()` still works as an alias for `revoke()`. +The stored session is always cleared, even when the server refuses the revocation. `404`, `401`, and `403` are treated as a completed logout — there is no live credential left to revoke. `5xx` and network errors still throw so you know revocation may not have reached the server, but they throw after the local session is gone, so the user is never left signed in by a failed logout. + +To clear a session locally without contacting the server at all: + +```typescript +await client.forgetSession(session.did); +``` + ## Resolution utilities | Property | Type | Description | diff --git a/packages/docs/content/docs/sdk/oauth-client-node/overview.md b/packages/docs/content/docs/sdk/oauth-client-node/overview.md index 8cbb38c..cd853ce 100644 --- a/packages/docs/content/docs/sdk/oauth-client-node/overview.md +++ b/packages/docs/content/docs/sdk/oauth-client-node/overview.md @@ -271,6 +271,14 @@ Or from the session itself: await session.signOut(); ``` +The stored session is always cleared, even if the server refuses the revocation. `404`, `401`, and `403` count as a completed logout — the credential is already gone or unusable. `5xx` and network errors still throw, after the local cleanup, so a failed revocation can never leave the session stored and restorable. + +To clear a session locally without contacting the server: + +```typescript +await client.forgetSession("did:plc:abc123"); +``` + ## Validate client metadata Verify that your OAuth client metadata is served correctly: diff --git a/packages/docs/content/docs/sdk/oauth-client/overview.md b/packages/docs/content/docs/sdk/oauth-client/overview.md index d67da9a..7d65604 100644 --- a/packages/docs/content/docs/sdk/oauth-client/overview.md +++ b/packages/docs/content/docs/sdk/oauth-client/overview.md @@ -118,6 +118,24 @@ await client.deleteSession("did:plc:abc123"); This deletes the session from both HappyView and local storage. +**The local cleanup always happens.** A `404`, `401`, or `403` from the server is treated as a completed logout — the session is either already gone or the credential is no longer usable, so there is nothing left to revoke. A `5xx` or a network error still throws, because the server may genuinely still hold a live session and you should know that, but it throws *after* the local session has been cleared. Either way the user ends up logged out on this device, and calling `deleteSession` again is safe. + +### Forgetting a session locally + +```typescript +await client.forgetSession("did:plc:abc123"); +``` + +Clears the stored session without contacting the server. Use it when revocation is impossible — an unreachable instance, or a credential the server has already rejected. Nothing is revoked, so the instance may still consider the session live until it expires naturally. + +### Storage keys + +`STORAGE_PREFIX` (`"happyview:session:"`) and `LAST_ACTIVE_KEY` (`"happyview:last-active-did"`) are exported, so tooling that needs to inspect or clear stored sessions directly can do so without hardcoding the format: + +```typescript +import { STORAGE_PREFIX, LAST_ACTIVE_KEY } from "@happyview/oauth-client"; +``` + ## Adapters ### CryptoAdapter diff --git a/packages/oauth-client/README.md b/packages/oauth-client/README.md index bcb66e6..c17f2b6 100644 --- a/packages/oauth-client/README.md +++ b/packages/oauth-client/README.md @@ -95,6 +95,16 @@ const session = await client.restoreSession("did:plc:abc123"); await client.deleteSession("did:plc:abc123"); ``` +The local session is always cleared, even when the server refuses the revocation. `404`, `401`, and `403` are treated as a completed logout — the credential is already gone or unusable, so there is nothing left to revoke. `5xx` and network errors still throw, so you know revocation may not have reached the server, but they throw after the local cleanup: a failed logout can never leave the user signed in. + +To clear a session locally without contacting the server: + +```typescript +await client.forgetSession("did:plc:abc123"); +``` + +`STORAGE_PREFIX` and `LAST_ACTIVE_KEY` are exported for tooling that needs to inspect stored sessions directly. + ## Adapters ### CryptoAdapter diff --git a/packages/oauth-client/src/__tests__/client.test.ts b/packages/oauth-client/src/__tests__/client.test.ts index f65bc62..619733b 100644 --- a/packages/oauth-client/src/__tests__/client.test.ts +++ b/packages/oauth-client/src/__tests__/client.test.ts @@ -1,5 +1,9 @@ import { describe, expect, mock, test } from "bun:test"; -import { HappyViewOAuthClient } from "../client"; +import { + HappyViewOAuthClient, + LAST_ACTIVE_KEY, + STORAGE_PREFIX, +} from "../client"; import { ApiError } from "../errors"; import { MemoryStorage } from "../storage"; @@ -33,6 +37,25 @@ async function generateTestJwk(): Promise { return jwk; } +async function storageWithSession( + jwk: JsonWebKey, + did = "did:plc:testuser", +): Promise { + const storage = new MemoryStorage(); + await storage.set( + `happyview:session:${did}`, + JSON.stringify({ + did, + dpopKey: jwk, + accessToken: "at_token", + clientKey: "hvc_testkey", + instanceUrl: "https://happyview.example.com", + }), + ); + await storage.set("happyview:last-active-did", did); + return storage; +} + function createClient(overrides?: { fetchFn?: typeof globalThis.fetch; clientSecret?: string; @@ -290,6 +313,157 @@ describe("HappyViewOAuthClient", () => { "did:plc:testuser", ); }); + + // A logout that leaves the session in storage is self-perpetuating: the + // next restore() signs the user straight back in, and pressing "log out" + // again repeats the same failure. Local cleanup must be unconditional. + test("clears storage when the server rejects the delete with 401", async () => { + const testJwk = await generateTestJwk(); + const { fetchFn } = createMockFetch([{ status: 401, body: {} }]); + + const storage = await storageWithSession(testJwk); + + const client = createClient({ fetchFn, storage }); + await client.deleteSession("did:plc:testuser"); + + expect( + await storage.get("happyview:session:did:plc:testuser"), + ).toBeNull(); + expect(await storage.get("happyview:last-active-did")).toBeNull(); + }); + + // 401 means the credential is already invalid — there is nothing left to + // revoke, so this is a completed logout, not a failed one. + test("does not throw on 401", async () => { + const testJwk = await generateTestJwk(); + const { fetchFn } = createMockFetch([{ status: 401, body: {} }]); + const storage = await storageWithSession(testJwk); + const client = createClient({ fetchFn, storage }); + + await expect( + client.deleteSession("did:plc:testuser"), + ).resolves.toBeUndefined(); + }); + + test("does not throw on 403", async () => { + const testJwk = await generateTestJwk(); + const { fetchFn } = createMockFetch([{ status: 403, body: {} }]); + const storage = await storageWithSession(testJwk); + const client = createClient({ fetchFn, storage }); + + await expect( + client.deleteSession("did:plc:testuser"), + ).resolves.toBeUndefined(); + }); + + // A 5xx may mean the server still holds a live session, so the caller has + // to hear about it — but they are still logged out locally either way. + test("still throws on 500, after clearing storage", async () => { + const testJwk = await generateTestJwk(); + const { fetchFn } = createMockFetch([{ status: 500, body: {} }]); + const storage = await storageWithSession(testJwk); + const client = createClient({ fetchFn, storage }); + + await expect( + client.deleteSession("did:plc:testuser"), + ).rejects.toThrow(ApiError); + + expect( + await storage.get("happyview:session:did:plc:testuser"), + ).toBeNull(); + expect(await storage.get("happyview:last-active-did")).toBeNull(); + }); + + // restoreSession JSON.parses the stored blob, so a corrupt entry throws + // before any request is made. That must not strand the user either. + test("clears storage when the stored session is corrupt", async () => { + const { fetchFn } = createMockFetch([]); + const storage = new MemoryStorage(); + await storage.set("happyview:session:did:plc:testuser", "{not json"); + await storage.set("happyview:last-active-did", "did:plc:testuser"); + + const client = createClient({ fetchFn, storage }); + await client.deleteSession("did:plc:testuser").catch(() => {}); + + expect( + await storage.get("happyview:session:did:plc:testuser"), + ).toBeNull(); + expect(await storage.get("happyview:last-active-did")).toBeNull(); + }); + + test("fires onSessionDelete even when the server returns 401", async () => { + const testJwk = await generateTestJwk(); + const { fetchFn } = createMockFetch([{ status: 401, body: {} }]); + const storage = await storageWithSession(testJwk); + + const deleted: string[] = []; + const client = new HappyViewOAuthClient({ + instanceUrl: "https://happyview.example.com", + clientKey: "hvc_testkey", + storage, + fetch: fetchFn, + sessionHooks: { onSessionDelete: (did) => deleted.push(did) }, + }); + await client.deleteSession("did:plc:testuser"); + + expect(deleted).toEqual(["did:plc:testuser"]); + }); + }); + + // Anyone recovering a stuck session by hand has to know these strings, so + // they are API whether or not they are exported. Pin them. + describe("storage keys", () => { + test("exports the key format it writes", async () => { + const testJwk = await generateTestJwk(); + const storage = await storageWithSession(testJwk); + + expect(STORAGE_PREFIX).toBe("happyview:session:"); + expect(LAST_ACTIVE_KEY).toBe("happyview:last-active-did"); + expect( + await storage.get(`${STORAGE_PREFIX}did:plc:testuser`), + ).not.toBeNull(); + }); + }); + + describe("forgetSession", () => { + test("clears local session state without contacting the server", async () => { + const testJwk = await generateTestJwk(); + const { fetchFn, calls } = createMockFetch([]); + const storage = await storageWithSession(testJwk); + + const client = createClient({ fetchFn, storage }); + await client.forgetSession("did:plc:testuser"); + + expect(calls).toHaveLength(0); + expect( + await storage.get("happyview:session:did:plc:testuser"), + ).toBeNull(); + expect(await storage.get("happyview:last-active-did")).toBeNull(); + }); + + test("preserves last active DID when forgetting a different session", async () => { + const testJwk = await generateTestJwk(); + const storage = new MemoryStorage(); + await storage.set( + "happyview:session:did:plc:other", + JSON.stringify({ + did: "did:plc:other", + dpopKey: testJwk, + accessToken: "at_token", + clientKey: "hvc_testkey", + instanceUrl: "https://happyview.example.com", + }), + ); + await storage.set("happyview:last-active-did", "did:plc:testuser"); + + const client = createClient({ storage }); + await client.forgetSession("did:plc:other"); + + expect(await storage.get("happyview:session:did:plc:other")).toBeNull(); + expect(await storage.get("happyview:last-active-did")).toBe( + "did:plc:testuser", + ); + }); }); describe("restoreSession", () => { diff --git a/packages/oauth-client/src/client.ts b/packages/oauth-client/src/client.ts index f9bf900..7187755 100644 --- a/packages/oauth-client/src/client.ts +++ b/packages/oauth-client/src/client.ts @@ -14,7 +14,10 @@ import type { StoredSession, } from "./types"; -const STORAGE_PREFIX = "happyview:session:"; +/// The storage keys this client reads and writes. Exported because a consumer +/// recovering a wedged session by hand would otherwise have to hardcode them, +/// which silently breaks the day they are renamed. +export const STORAGE_PREFIX = "happyview:session:"; export const LAST_ACTIVE_KEY = "happyview:last-active-did"; export interface FetchMetadataOptions { @@ -192,24 +195,55 @@ export class HappyViewOAuthClient { return session; } + /** + * Revoke the session server-side and clear it locally. + * + * The local cleanup runs unconditionally. If it did not, a server that + * refuses the revocation would leave the session in storage, `restore()` + * would sign the user back in on the next load, and the next logout would + * fail identically — a loop with no exit through this API. + */ async deleteSession(did: string): Promise { - const session = await this.restoreSession(did); - if (session) { - const resp = await session.fetchHandler( - `${this.instanceUrl}/oauth/sessions/${did}`, - { method: "DELETE" }, - ); - - if (!resp.ok && resp.status !== 404) { - const body = await resp.json().catch(() => ({})); - throw new ApiError( - `Failed to delete session: ${resp.status} ${(body as any).error ?? (body as any).message ?? resp.statusText}`, - resp.status, - body, + try { + const session = await this.restoreSession(did); + if (session) { + const resp = await session.fetchHandler( + `${this.instanceUrl}/oauth/sessions/${did}`, + { method: "DELETE" }, ); + + // 404: already gone. 401/403: the credential is itself invalid, so + // there is no live session left to revoke. None of these is a logout + // that failed, so none of them should be reported as one. + const alreadyUnusable = + resp.status === 404 || resp.status === 401 || resp.status === 403; + + if (!resp.ok && !alreadyUnusable) { + const body = await resp.json().catch(() => ({})); + throw new ApiError( + `Failed to delete session: ${resp.status} ${(body as any).error ?? (body as any).message ?? resp.statusText}`, + resp.status, + body, + ); + } } + } finally { + // 5xx and network errors still throw — the server may genuinely still + // hold a live session and the caller deserves to know. They throw from + // here, after the local state is gone, so the user is logged out either + // way. + await this.forgetSession(did); } + } + /** + * Clear a session locally without contacting the server. + * + * The escape hatch for a session that cannot be revoked remotely — a revoked + * or expired credential, an unreachable instance. Nothing is revoked, so the + * server may still consider the session live until it expires. + */ + async forgetSession(did: string): Promise { await this.storage.delete(`${STORAGE_PREFIX}${did}`); const lastActive = await this.storage.get(LAST_ACTIVE_KEY); diff --git a/packages/oauth-client/src/index.ts b/packages/oauth-client/src/index.ts index 4263eb1..ecd91ad 100644 --- a/packages/oauth-client/src/index.ts +++ b/packages/oauth-client/src/index.ts @@ -1,7 +1,11 @@ export * from "@atproto/jwk"; export * from "@atproto/jwk-webcrypto"; -export { HappyViewOAuthClient, LAST_ACTIVE_KEY } from "./client"; +export { + HappyViewOAuthClient, + LAST_ACTIVE_KEY, + STORAGE_PREFIX, +} from "./client"; export type { FetchMetadataOptions } from "./client"; export { importJwk } from "./import-jwk"; export { diff --git a/src/oauth/routes.rs b/src/oauth/routes.rs index 75198c1..4dcc46d 100644 --- a/src/oauth/routes.rs +++ b/src/oauth/routes.rs @@ -1,4 +1,4 @@ -use axum::extract::{FromRequest, Path, State}; +use axum::extract::{FromRequest, OriginalUri, Path, State}; use axum::http::StatusCode; use axum::routing::{get, post}; use axum::{Json, Router}; @@ -25,6 +25,31 @@ pub fn routes() -> Router { ) } +/// The request path exactly as the client sent it. +/// +/// Two things make this different from `req.uri().path()`. This router is +/// nested under `/oauth`, so the handler's own URI has that prefix stripped; +/// and percent-encoding must survive, because a DPoP `htu` is whatever the +/// client signed, which is whatever it put on the wire. Rebuilding the path +/// from an already-decoded `Path` segment silently rejects any client that +/// encodes the DID — a mismatch the caller has no way to fix from their side. +fn original_request_path(req: &axum::extract::Request) -> String { + req.extensions() + .get::() + .map(|uri| uri.path().to_string()) + .unwrap_or_else(|| req.uri().path().to_string()) +} + +/// Build the `htu` a DPoP proof is validated against. +fn dpop_htu(state: &AppState, host: &str, path: &str) -> String { + let scheme = if state.config.public_url.starts_with("https") { + "https" + } else { + "http" + }; + format!("{}://{}{}", scheme, host, path) +} + // --- Request / response types --- #[derive(Deserialize)] @@ -331,8 +356,9 @@ async fn register_session( /// GET /oauth/sessions/:did — retrieve session info (scopes). /// -/// Same auth as DELETE: confidential clients use `X-Client-Key` + `X-Client-Secret`, -/// public clients use `X-Client-Key` + `Authorization: DPoP ` + `DPoP` proof. +/// Same auth as DELETE: `X-Client-Key` + `X-Client-Secret` looks the session up +/// by (client, user); otherwise `X-Client-Key` + `Authorization: DPoP ` + +/// a `DPoP` proof identifies the specific device session. async fn get_session( State(state): State, Path(did): Path, @@ -378,29 +404,21 @@ async fn get_session( let resolved = client_auth::resolve_client_by_key(&state.db, state.db_backend, &client_key).await?; - if resolved.client_type != "public" { - return Err(AppError::Auth( - "non-public clients must provide X-Client-Secret".into(), - )); - } - let auth_header = req .headers() .get("authorization") .and_then(|v| v.to_str().ok()) .ok_or_else(|| { - AppError::Auth("public clients must provide Authorization: DPoP ".into()) + AppError::Auth("DPoP auth requires Authorization: DPoP ".into()) })?; let access_token = auth_header.strip_prefix("DPoP ").ok_or_else(|| { - AppError::Auth("public clients must use DPoP authorization scheme".into()) + AppError::Auth("DPoP auth requires the DPoP authorization scheme".into()) })?; let dpop_proof = req .headers() .get("dpop") .and_then(|v| v.to_str().ok()) - .ok_or_else(|| { - AppError::Auth("public clients must provide DPoP proof header".into()) - })?; + .ok_or_else(|| AppError::Auth("DPoP auth requires a DPoP proof header".into()))?; let thumbprint = crate::oauth::dpop_proof::extract_proof_thumbprint(dpop_proof)?; let dpop_key_id = keys::get_dpop_key_id_by_thumbprint( @@ -411,17 +429,12 @@ async fn get_session( ) .await?; - let scheme = if state.config.public_url.starts_with("https") { - "https" - } else { - "http" - }; let host = req .headers() .get("host") .and_then(|v| v.to_str().ok()) .unwrap_or("localhost"); - let request_url = format!("{}://{}/oauth/sessions/{}", scheme, host, did); + let request_url = dpop_htu(&state, host, &original_request_path(&req)); crate::oauth::dpop_proof::validate_dpop_proof( dpop_proof, @@ -452,8 +465,18 @@ async fn get_session( /// DELETE /oauth/sessions/:did — logout / revoke a session. /// -/// Confidential clients authenticate with `X-Client-Key` + `X-Client-Secret`. -/// Public clients authenticate with `X-Client-Key` + `Authorization: DPoP ` + `DPoP` proof. +/// With `X-Client-Secret`, every session for this user+client is revoked. +/// Otherwise the caller authenticates with `X-Client-Key` + `Authorization: +/// DPoP ` + a `DPoP` proof, and only that device's session is revoked. +/// +/// DPoP auth is accepted regardless of `client_type`, matching what `/xrpc/*` +/// already accepts. Requiring the client secret here — but not for the calls +/// the same credentials make everywhere else — meant a confidential client +/// using DPoP could never log out: the 401 left the session in local storage, +/// the next restore signed the user back in, and the next logout failed the +/// same way. The DPoP proof is possession of the session key, which is the +/// same thing that authorises using the session; revoking it is strictly less +/// dangerous than continuing to use it. async fn delete_session( State(state): State, Path(did): Path, @@ -499,29 +522,21 @@ async fn delete_session( let resolved = client_auth::resolve_client_by_key(&state.db, state.db_backend, &client_key).await?; - if resolved.client_type != "public" { - return Err(AppError::Auth( - "non-public clients must provide X-Client-Secret".into(), - )); - } - let auth_header = req .headers() .get("authorization") .and_then(|v| v.to_str().ok()) .ok_or_else(|| { - AppError::Auth("public clients must provide Authorization: DPoP ".into()) + AppError::Auth("DPoP auth requires Authorization: DPoP ".into()) })?; let access_token = auth_header.strip_prefix("DPoP ").ok_or_else(|| { - AppError::Auth("public clients must use DPoP authorization scheme".into()) + AppError::Auth("DPoP auth requires the DPoP authorization scheme".into()) })?; let dpop_proof = req .headers() .get("dpop") .and_then(|v| v.to_str().ok()) - .ok_or_else(|| { - AppError::Auth("public clients must provide DPoP proof header".into()) - })?; + .ok_or_else(|| AppError::Auth("DPoP auth requires a DPoP proof header".into()))?; let thumbprint = crate::oauth::dpop_proof::extract_proof_thumbprint(dpop_proof)?; let dpop_key_id = keys::get_dpop_key_id_by_thumbprint( @@ -532,17 +547,12 @@ async fn delete_session( ) .await?; - let scheme = if state.config.public_url.starts_with("https") { - "https" - } else { - "http" - }; let host = req .headers() .get("host") .and_then(|v| v.to_str().ok()) .unwrap_or("localhost"); - let request_url = format!("{}://{}/oauth/sessions/{}", scheme, host, did); + let request_url = dpop_htu(&state, host, &original_request_path(&req)); crate::oauth::dpop_proof::validate_dpop_proof( dpop_proof, @@ -627,7 +637,7 @@ async fn list_device_sessions( Path(did): Path, req: axum::extract::Request, ) -> Result>, AppError> { - let request_path = req.uri().path().to_string(); + let request_path = original_request_path(&req); let headers = SessionAuthHeaders::from_request(&req); let client = resolve_session_client(&state, &headers, &request_path, "GET").await?; @@ -654,7 +664,7 @@ async fn delete_device_session( Path((did, session_id)): Path<(String, String)>, req: axum::extract::Request, ) -> Result { - let request_path = req.uri().path().to_string(); + let request_path = original_request_path(&req); let headers = SessionAuthHeaders::from_request(&req); let client = resolve_session_client(&state, &headers, &request_path, "DELETE").await?; @@ -702,34 +712,24 @@ async fn resolve_session_client( client_auth::resolve_client_by_key(&state.db, state.db_backend, &headers.client_key) .await?; - if resolved.client_type != "public" { - return Err(AppError::Auth( - "non-public clients must provide X-Client-Secret".into(), - )); - } - - let auth_header = headers.auth_header.as_deref().ok_or_else(|| { - AppError::Auth("public clients must provide Authorization: DPoP ".into()) - })?; - let access_token = auth_header.strip_prefix("DPoP ").ok_or_else(|| { - AppError::Auth("public clients must use DPoP authorization scheme".into()) - })?; + let auth_header = headers + .auth_header + .as_deref() + .ok_or_else(|| AppError::Auth("DPoP auth requires Authorization: DPoP ".into()))?; + let access_token = auth_header + .strip_prefix("DPoP ") + .ok_or_else(|| AppError::Auth("DPoP auth requires the DPoP authorization scheme".into()))?; let dpop_proof = headers .dpop_proof .as_deref() - .ok_or_else(|| AppError::Auth("public clients must provide DPoP proof header".into()))?; + .ok_or_else(|| AppError::Auth("DPoP auth requires a DPoP proof header".into()))?; let thumbprint = crate::oauth::dpop_proof::extract_proof_thumbprint(dpop_proof)?; let _dpop_key_id = keys::get_dpop_key_id_by_thumbprint(&state.db, state.db_backend, &resolved.id, &thumbprint) .await?; - let scheme = if state.config.public_url.starts_with("https") { - "https" - } else { - "http" - }; - let request_url = format!("{}://{}{}", scheme, headers.host, request_path); + let request_url = dpop_htu(state, &headers.host, request_path); crate::oauth::dpop_proof::validate_dpop_proof( dpop_proof, diff --git a/tests/dpop_auth.rs b/tests/dpop_auth.rs index ac0bf65..e344d7d 100644 --- a/tests/dpop_auth.rs +++ b/tests/dpop_auth.rs @@ -977,3 +977,173 @@ async fn test_public_client_dpop_delete_session() { let get_resp = app.router.clone().oneshot(get_req).await.unwrap(); assert_ne!(get_resp.status(), StatusCode::OK); } + +/// A confidential client can provision a DPoP key and use it against `/xrpc/*` +/// without ever presenting its secret, because `resolve_dpop_claims` does not +/// look at `client_type`. Revoking the session it just used must not be the one +/// operation that demands more — otherwise logout 401s forever while every +/// other call succeeds, and the client cannot get out of the loop. +#[tokio::test] +#[serial] +async fn test_confidential_client_dpop_delete_session_without_secret() { + common::require_db!(); + let app = common::app::TestApp::new_with_encryption().await; + let (client_key, client_secret, _id) = app.create_api_client("confidential", None).await; + + let key_req = post_json_with_headers( + "/oauth/dpop-keys", + &json!({}), + vec![ + ("x-client-key", &client_key), + ("x-client-secret", &client_secret), + ], + ); + let key_resp = app.router.clone().oneshot(key_req).await.unwrap(); + assert_eq!(key_resp.status(), StatusCode::CREATED); + let key_body = response_json(key_resp).await; + let provision_id = key_body["provision_id"].as_str().unwrap(); + let dpop_key = &key_body["dpop_key"]; + + let did = "did:plc:confidentialdelete"; + let access_token = "confidential-delete-token"; + + app.mock_session_verification(did, did).await; + + let session_req = post_json_with_headers( + "/oauth/sessions", + &json!({ + "provision_id": provision_id, + "did": did, + "access_token": access_token, + "scopes": "atproto", + "pds_url": "https://pds.example.com", + }), + vec![ + ("x-client-key", &client_key), + ("x-client-secret", &client_secret), + ], + ); + let session_resp = app.router.clone().oneshot(session_req).await.unwrap(); + assert_eq!(session_resp.status(), StatusCode::CREATED); + + // DPoP proof only — no X-Client-Secret, exactly what the JS SDK sends. + let request_url = format!("http://127.0.0.1/oauth/sessions/{}", did); + let proof = generate_dpop_proof(dpop_key, "DELETE", &request_url, access_token, None) + .expect("failed to generate DPoP proof"); + + let del_req = delete_with_headers( + &format!("/oauth/sessions/{}", did), + vec![ + ("x-client-key", &client_key), + ("authorization", &format!("DPoP {}", access_token)), + ("dpop", &proof), + ], + ); + let del_resp = app.router.clone().oneshot(del_req).await.unwrap(); + assert_eq!(del_resp.status(), StatusCode::NO_CONTENT); +} + +/// The device-session route builds its htu from the request path rather than +/// from a format string, and this router is nested under `/oauth` — so the +/// handler's own `req.uri()` has that prefix stripped. The client signs the +/// full URL it requested, so the server has to reconstruct the full one too. +/// Every existing test of this route authenticates with the client secret and +/// so never reaches the proof check. +#[tokio::test] +#[serial] +async fn test_dpop_delete_device_session_htu_includes_router_prefix() { + common::require_db!(); + let app = common::app::TestApp::new_with_encryption().await; + let (client_key, client_secret, _id) = app.create_api_client("confidential", None).await; + let did = "did:plc:devicedpop"; + + let (_prov, dpop_key, session_id) = + provision_and_register(&app, &client_key, &client_secret, did, "device-dpop-token").await; + + let path = format!("/oauth/sessions/{}/devices/{}", did, session_id); + let request_url = format!("http://127.0.0.1{}", path); + let proof = generate_dpop_proof(&dpop_key, "DELETE", &request_url, "device-dpop-token", None) + .expect("failed to generate DPoP proof"); + + let del_req = delete_with_headers( + &path, + vec![ + ("x-client-key", &client_key), + ("authorization", "DPoP device-dpop-token"), + ("dpop", &proof), + ], + ); + let del_resp = app.router.clone().oneshot(del_req).await.unwrap(); + assert_eq!(del_resp.status(), StatusCode::NO_CONTENT); +} + +/// The htu is whatever the client actually signed, which is whatever it put on +/// the wire. A client that percent-encodes the DID signs the encoded form, so +/// rebuilding the URL from a decoded path segment produces a mismatch that no +/// caller can fix from their side. +#[tokio::test] +#[serial] +async fn test_public_client_dpop_delete_session_percent_encoded_did() { + common::require_db!(); + let app = common::app::TestApp::new_with_encryption().await; + let (client_key, _secret, _id) = app + .create_api_client("public", Some(vec!["http://localhost:3000".to_string()])) + .await; + + use base64::Engine; + use base64::engine::general_purpose::URL_SAFE_NO_PAD; + use sha2::{Digest, Sha256}; + + let verifier = "test-verifier-for-encoded-delete"; + let challenge = URL_SAFE_NO_PAD.encode(Sha256::digest(verifier.as_bytes())); + + let key_req = post_json_with_headers( + "/oauth/dpop-keys", + &json!({ "pkce_challenge": challenge }), + vec![ + ("x-client-key", &client_key), + ("origin", "http://localhost:3000"), + ], + ); + let key_resp = app.router.clone().oneshot(key_req).await.unwrap(); + assert_eq!(key_resp.status(), StatusCode::CREATED); + let key_body = response_json(key_resp).await; + let provision_id = key_body["provision_id"].as_str().unwrap(); + let dpop_key = &key_body["dpop_key"]; + + let did = "did:plc:encodeddelete"; + let encoded_did = "did%3Aplc%3Aencodeddelete"; + let access_token = "encoded-delete-token"; + + app.mock_session_verification(did, did).await; + + let session_req = post_json_with_headers( + "/oauth/sessions", + &json!({ + "provision_id": provision_id, + "pkce_verifier": verifier, + "did": did, + "access_token": access_token, + "scopes": "atproto", + "pds_url": "https://pds.example.com", + }), + vec![("x-client-key", &client_key)], + ); + let session_resp = app.router.clone().oneshot(session_req).await.unwrap(); + assert_eq!(session_resp.status(), StatusCode::CREATED); + + let request_url = format!("http://127.0.0.1/oauth/sessions/{}", encoded_did); + let proof = generate_dpop_proof(dpop_key, "DELETE", &request_url, access_token, None) + .expect("failed to generate DPoP proof"); + + let del_req = delete_with_headers( + &format!("/oauth/sessions/{}", encoded_did), + vec![ + ("x-client-key", &client_key), + ("authorization", &format!("DPoP {}", access_token)), + ("dpop", &proof), + ], + ); + let del_resp = app.router.clone().oneshot(del_req).await.unwrap(); + assert_eq!(del_resp.status(), StatusCode::NO_CONTENT); +}