From 947d2c6e20d1f0c399d595b194a2c67d33aae79f Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 5 Sep 2026 00:29:37 +0200 Subject: [PATCH 1/4] fix(e2e): wait for the Service Worker to control the page before API calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `apiAdminCreateUser` intermittently failed with `401 {"error":"DPoP nonce required","error_type":"DpopVerificationFailed"}`, most visibly in admin.spec.ts's pagination test. Not a nonce-rotation race — the nonce pool keeps a 3-minute overlap window precisely so in-flight requests survive rotation. It is a service-worker-control race. `browserFetch` issues a raw page-context `fetch`, and the DPoP proof is attached by the Service Worker intercepting it — the helper's own doc comment says so. The SPA's proof-and-retry logic lives in `client.ts`'s `dpopFetch`, which this helper deliberately bypasses. So when the SW is not yet controlling the page, the request goes out unsigned, the middleware sees a bound session with no proof (`dpop.rs`, the `expected_jkt` match), and answers with a nonce challenge. Nothing retries it: the SW that would have signed it is what is missing, and `dpopFetch` was never in the path. The window is real on every fresh browser context. `service-worker.ts` does `skipWaiting()` + `clients.claim()`, which is correct, but claiming is asynchronous — the first navigation loads uncontrolled, then install → activate → claim. `waitForLoadState('networkidle')` says nothing about SW control, so a helper called a few lines after `apiLogin` can land inside it. Slower CI widens it, which is why this showed up there and not locally. The fix waits inside the same `page.evaluate` as the fetch, so it costs one property check once the page is controlled and needs no per-page bookkeeping. It is bounded at 10s: if the SW never claims, the request goes out as before and the resulting 401 stays the clear signal it is today rather than becoming an unexplained Playwright timeout. Worth stating because it is easy to get wrong: **`ready` is not `controlling`.** `navigator.serviceWorker.ready` resolves once a registration is active, while `controller` stays null until that worker has claimed THIS page. Awaiting only `ready` looks correct and still flakes. This is a test bug, not a product one. Real paths go through `apiFetch`, which signs in page JS; the SW is the safety net for requests that bypass it (``, downloads). This helper is the only caller relying on the SW as its primary signer. Not verified by running the suite: a flake reproduces on CI timing, so one green local run would prove nothing. The mechanism is confirmed from the code path — SW-signed proof, no fallback retry, asynchronous claim. Co-Authored-By: Claude Opus 5 (1M context) --- tests/e2e/spa/helpers.ts | 48 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/e2e/spa/helpers.ts b/tests/e2e/spa/helpers.ts index a1e2abd9..a7071b59 100644 --- a/tests/e2e/spa/helpers.ts +++ b/tests/e2e/spa/helpers.ts @@ -205,6 +205,31 @@ export async function apiLogin(page: Page, admin = TEST_ADMIN): Promise { * lives on the non-HttpOnly `oxicloud_csrf` cookie). Body is passed * as an already-serialized string so this helper works uniformly * for JSON, form, and multipart-manually-encoded payloads. + * + * ## Waiting for the Service Worker is load-bearing, not defensive + * + * Because the proof comes from the SW rather than from this fetch, + * a request issued while the page is NOT YET CONTROLLED goes out + * unsigned. The server sees a bound session with no proof and + * answers `401 DPoP nonce required` — a nonce challenge, from + * `nonce_challenge_response` in `middleware/dpop.rs`. Nothing + * retries it: the SPA's retry lives in `client.ts`'s `dpopFetch`, + * which this helper deliberately bypasses, and the SW that would + * have signed it is exactly what is missing. + * + * That window is real on every fresh browser context. The worker + * does `skipWaiting()` + `clients.claim()` (`service-worker.ts`), + * which is correct, but claiming is asynchronous: the first + * navigation loads uncontrolled, then install → activate → claim. + * `waitForLoadState('networkidle')` says nothing about SW control, + * so a helper called soon after `apiLogin` — such as + * `apiAdminCreateUser` in `admin.spec.ts`'s pagination test — can + * land inside it. Intermittently, and more often on slower CI. + * + * **`ready` is not `controlling`.** `navigator.serviceWorker.ready` + * resolves once a registration is *active*; `controller` stays null + * until that worker has claimed THIS page. Awaiting only `ready` + * looks right and still flakes. */ async function browserFetch( page: Page, @@ -217,6 +242,29 @@ async function browserFetch( ): Promise<{ ok: boolean; status: number; body: string }> { return page.evaluate( async ({ url, method, contentType, body }) => { + // Bounded wait: if the SW never claims (not registered, disabled, + // or a page that never booted the SPA) fall through and let the + // request go out as before. The resulting 401 is then the same + // clear signal it is today, rather than a Playwright timeout with + // no explanation attached. + if ('serviceWorker' in navigator && !navigator.serviceWorker.controller) { + await Promise.race([ + (async () => { + await navigator.serviceWorker.ready; + if (!navigator.serviceWorker.controller) { + await new Promise((resolve) => + navigator.serviceWorker.addEventListener( + 'controllerchange', + () => resolve(), + { once: true }, + ), + ); + } + })(), + new Promise((resolve) => setTimeout(resolve, 10_000)), + ]); + } + const csrf = document.cookie.match(/(?:^|; )oxicloud_csrf=([^;]+)/)?.[1] ?? ''; const headers: Record = {}; if (csrf) headers['x-csrf-token'] = csrf; From 99a230f3736043363599e7f0aa7eed0b1e5aa448 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 5 Sep 2026 00:54:47 +0200 Subject: [PATCH 2/4] fix(dpop): log the nonce-bootstrap challenge instead of returning a bare 401 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `None` arm of the nonce match returns a challenge — the client sent a valid proof but carried no nonce, so it gets 401 + `DPoP-Nonce` and retries. It was the only one of the three challenge paths that logged nothing. The result was an unexplainable line in the access log: WARN http::api: client_error status=401 latency_ms=0 … with no audit line saying why, and no sign of the successful retry — `main.rs` defaults the access log to `http=warn`, so the 200 that follows is never printed. It came up as "any clue why I have 401?" while reading an e2e run, which is the cost of a silent rejection. AGENTS.md is explicit that every rejection emits a structured audit line before returning the error; this path simply missed it. The line pays off immediately: it names `htu`, and the first run with it showed the caller was `GET /api/admin/plugins/{id}/logs/stream` — the admin log tail, an `EventSource`, whose proof is minted by the service worker rather than by `apiFetch`. `service-worker.ts` documents exactly this: the nonce cache is per-scope, so the page module and the SW EACH pay one round-trip challenge and then catch up from `DPoP-Nonce` response headers. The SW absorbs the 401 and retries, so `EventSource` never sees it. Hence "once per client SCOPE" rather than per session — and again whenever the browser terminates and restarts the worker, which is normal and is why one run shows several. Deliberately NOT `dpop.verify_failed`. The proof verified cleanly — nothing failed — and folding a routine bootstrap into the event operators watch for attacks would bury real signal. `nonce_stale` above keeps that event name because it is long-standing and aggregators key off it; this path is new, so it gets the accurate one: `dpop.nonce_challenged` / `reason = "nonce_missing"`. No new counter — `nonce_challenge_response` already counts every challenge centrally, so adding one here would double-count. Co-Authored-By: Claude Opus 5 (1M context) --- src/interfaces/middleware/dpop.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/interfaces/middleware/dpop.rs b/src/interfaces/middleware/dpop.rs index e6e7cbe2..b5d39dc8 100644 --- a/src/interfaces/middleware/dpop.rs +++ b/src/interfaces/middleware/dpop.rs @@ -237,6 +237,34 @@ pub async fn require_dpop_layer( // window at the verifier means this one still // succeeded, but we still want the client onto // the nonce path immediately. + // + // Logged because this returns a 401, and every + // rejection owes the operator a line saying why — + // otherwise the access log shows a bare + // `client_error status=401` with nothing to + // explain it, while the client's successful retry + // stays invisible under the default `http=warn` + // access-log filter. That combination reads like a + // real auth failure and is not one. + // + // NOT `dpop.verify_failed`: the proof verified + // cleanly, and counting a routine bootstrap as a + // verification failure would put a per-session + // event into the metric operators watch for + // attacks. `nonce_stale` above keeps that event + // name — it is long-standing and log aggregators + // key off it — but this path is new, so it gets + // the accurate one. The challenge itself is + // already counted centrally by + // `nonce_challenge_response`. + tracing::info!( + target: "audit", + event = "dpop.nonce_challenged", + reason = "nonce_missing", + method = %method, + htu = %htu, + "👮🏻‍♂️ DPoP proof carried no nonce — issuing challenge (expected once per client scope; the client harvests the nonce and retries)", + ); return nonce_challenge_response(&nonce_service); } Some(n) => n, From 9a83f8c0d152a52010c553d71e517e24459d8461 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 5 Sep 2026 08:02:23 +0200 Subject: [PATCH 3/4] feat(config): make the per-caller rate limits configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An e2e run emitted 81 × 429 in 763 log lines. The env already set LOGIN/REGISTER/REFRESH to 36000/hour, and that changed nothing, because those three are the only rate limiters with env vars — and they are the wrong ones. They key on the client IP and guard the unauthenticated front door. The limiters that fired key on the CALLER ID. The log distinguishes them: all 81 landed on target `http::api`, never `http::api::auth`, where login/register/refresh live. The likely culprit is `user_profile_rate_limiter`, 60 lookups/min/caller, guarding the visibility query behind GET /api/users/{id}. The whole suite runs as a single `admin`, so every test shares one bucket; admin views resolve an owner name per row and the run creates 34 users, so a minute of tests clears 60 easily. Nothing failed, because the SPA degrades to an unresolved name — which is exactly the problem, since that noise would hide a real rate-limit regression. Adds OXICLOUD_RATE_LIMIT_USER_PROFILE_MAX / _WINDOW_SECS and OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_MAX / _WINDOW_SECS, following the existing three exactly. Defaults are the literals they replaced (60/60 and 240/60), so an operator who sets nothing sees no change; a unit test pins that, because the failure is silent in both directions — too low and real users get 429s on listings, too high and the `access_grants` query loses the guard that stops an attacker exhausting it with random UUIDs. `tests/common/server.env` (shared by the e2e AND hurl suites) sets both to a 1-hour budget, matching the posture already used for the other three rather than a raised per-minute rate that would still burst-trip. The docs now state the IP-vs-caller split, since that is what decides which knob to reach for — and note that several actors sharing one identity (CI, a bot, a kiosk) share one caller bucket. Left alone: the four narrower env files (OIDC, webdav-drive-root) keep their existing MAX=3600 with default windows. No evidence they trip the per-caller limits, and adding config on speculation is how these files drift. Not fixed here: rate-limit rejections emit NO audit line, which is why the attribution above reads "likely" rather than "confirmed" — nothing in the log names the limiter. AGENTS.md requires one for every rejection; that is a separate change. Co-Authored-By: Claude Opus 5 (1M context) --- docs/config/env.md | 19 +++++++++++ example.env | 23 +++++++++++++ src/common/config.rs | 73 +++++++++++++++++++++++++++++++++++++++++ src/common/di.rs | 30 +++++++++++------ tests/common/server.env | 20 +++++++++++ 5 files changed, 155 insertions(+), 10 deletions(-) diff --git a/docs/config/env.md b/docs/config/env.md index 0d1e02a8..e261767e 100644 --- a/docs/config/env.md +++ b/docs/config/env.md @@ -82,9 +82,28 @@ DPoP cryptographically binds a session cookie to a browser-held ECDSA keypair (P | `OXICLOUD_RATE_LIMIT_REGISTER_WINDOW_SECS` | `3600` | Registration rate-limit window (seconds) | | `OXICLOUD_RATE_LIMIT_REFRESH_MAX` | `20` | Max token refresh attempts per IP per window | | `OXICLOUD_RATE_LIMIT_REFRESH_WINDOW_SECS` | `60` | Refresh rate-limit window (seconds) | +| `OXICLOUD_RATE_LIMIT_USER_PROFILE_MAX` | `60` | Max user-profile lookups per **caller** per window | +| `OXICLOUD_RATE_LIMIT_USER_PROFILE_WINDOW_SECS` | `60` | User-profile lookup window (seconds) | +| `OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_MAX` | `240` | Max delta-upload requests per **caller** per window | +| `OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_WINDOW_SECS` | `60` | Delta-upload window (seconds) | | `OXICLOUD_LOCKOUT_MAX_FAILURES` | `5` | Consecutive failed logins before account lockout | | `OXICLOUD_LOCKOUT_DURATION_SECS` | `900` | Account lockout duration (15 minutes) | +Login, register and refresh are keyed on the **client IP** and guard the +unauthenticated front door. User-profile and delta-upload are keyed on +the **caller id** and guard an authenticated user against exhausting a +shared resource. + +That distinction decides which knob to reach for. When several actors +share one identity — a CI suite, an integration bot, a kiosk account — +they share one caller bucket, and a ceiling sized for a single human is +easily exceeded while the IP-keyed limits sit untouched. Raising the +login/register/refresh limits does nothing in that case. + +Exceeding the user-profile limit returns `429`, which in a browser +usually shows up as owner names failing to resolve in file listings +rather than as a visible error. + ## Feature Flags | Variable | Default | Description | diff --git a/example.env b/example.env index 69de5230..649443e5 100644 --- a/example.env +++ b/example.env @@ -352,6 +352,29 @@ DATABASE_URL=postgres://postgres:postgres@localhost:5432/oxicloud # Rate-limit window for token refresh in seconds (default: 60) #OXICLOUD_RATE_LIMIT_REFRESH_WINDOW_SECS=60 +# The three limits above are keyed on the CLIENT IP and guard the +# unauthenticated front door. The two below are keyed on CALLER ID and +# guard an authenticated user against exhausting a shared resource. +# +# Raise these when several actors share one identity — a CI suite, an +# integration bot, a kiosk — because they then share one bucket and a +# ceiling sized for one human is easily exceeded. + +# Max user-profile lookups per caller per window (default: 60). +# Guards the visibility query behind GET /api/users/{id}. Exceeding it +# returns 429; in a browser this surfaces as owner names failing to +# resolve in file listings rather than as a visible error. +#OXICLOUD_RATE_LIMIT_USER_PROFILE_MAX=60 +# User-profile lookup window in seconds (default: 60) +#OXICLOUD_RATE_LIMIT_USER_PROFILE_WINDOW_SECS=60 + +# Max delta-upload requests per caller per window (default: 240). +# Generous for a real client — chunk PUTs carry up to 100 MB each — +# while stopping pin/negotiate floods. +#OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_MAX=240 +# Delta-upload window in seconds (default: 60) +#OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_WINDOW_SECS=60 + # Consecutive failed logins before account lockout (default: 5) #OXICLOUD_LOCKOUT_MAX_FAILURES=5 # Account lockout duration in seconds (default: 900 = 15 minutes) diff --git a/src/common/config.rs b/src/common/config.rs index 67001e03..cc3978fc 100644 --- a/src/common/config.rs +++ b/src/common/config.rs @@ -1600,6 +1600,33 @@ pub struct RateLimitConfig { pub lockout_max_failures: u32, /// Account lockout duration in seconds (default: 900 = 15 min) pub lockout_duration_secs: u64, + + // ── Per-caller limits ──────────────────────────────────────────── + // + // Keyed on `caller_id`, not IP: these guard an authenticated user + // against exhausting a shared resource, whereas the three above + // guard the unauthenticated front door against an attacker. + // + // The distinction matters when several actors share one identity — + // an automated test suite, a CI job, an integration bot. They then + // share one bucket, and a ceiling that is generous for one human + // is easily exceeded. That is exactly what made the e2e suite emit + // 81 × 429 in a single run, all as the same `admin`. + /// Max user-profile lookups per caller per window (default: 60). + /// + /// Guards the visibility query behind `GET /api/users/{id}`, which + /// touches `access_grants` — the rate check runs BEFORE it so an + /// attacker cannot exhaust it by hammering random UUIDs. + pub user_profile_max_requests: u32, + /// User-profile lookup window in seconds (default: 60). + pub user_profile_window_secs: u64, + /// Max delta-upload requests per caller per window (default: 240). + /// + /// Generous for a real client — chunk PUTs carry up to 100 MB each + /// — while stopping pin/negotiate floods. + pub delta_upload_max_requests: u32, + /// Delta-upload window in seconds (default: 60). + pub delta_upload_window_secs: u64, } impl Default for RateLimitConfig { @@ -1613,6 +1640,12 @@ impl Default for RateLimitConfig { refresh_window_secs: 60, lockout_max_failures: 5, lockout_duration_secs: 900, + // Unchanged from the literals these replaced in `di.rs`, so + // an operator who sets nothing sees no behaviour change. + user_profile_max_requests: 60, + user_profile_window_secs: 60, + delta_upload_max_requests: 240, + delta_upload_window_secs: 60, } } } @@ -3093,6 +3126,28 @@ impl AppConfig { { config.auth.rate_limit.refresh_window_secs = val; } + if let Ok(v) = env::var("OXICLOUD_RATE_LIMIT_USER_PROFILE_MAX").map(|v| v.parse::()) + && let Ok(val) = v + { + config.auth.rate_limit.user_profile_max_requests = val; + } + if let Ok(v) = + env::var("OXICLOUD_RATE_LIMIT_USER_PROFILE_WINDOW_SECS").map(|v| v.parse::()) + && let Ok(val) = v + { + config.auth.rate_limit.user_profile_window_secs = val; + } + if let Ok(v) = env::var("OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_MAX").map(|v| v.parse::()) + && let Ok(val) = v + { + config.auth.rate_limit.delta_upload_max_requests = val; + } + if let Ok(v) = + env::var("OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_WINDOW_SECS").map(|v| v.parse::()) + && let Ok(val) = v + { + config.auth.rate_limit.delta_upload_window_secs = val; + } if let Ok(v) = env::var("OXICLOUD_LOCKOUT_MAX_FAILURES").map(|v| v.parse::()) && let Ok(val) = v { @@ -3959,6 +4014,24 @@ pub fn default_config() -> AppConfig { mod tests { use super::*; + /// The per-caller limits moved from hardcoded literals in `di.rs` into + /// config. The whole point was to add a knob, NOT to change behaviour + /// for anyone who does not turn it — so the defaults must still be the + /// values that were compiled in before. + /// + /// Worth pinning because the failure is silent and asymmetric: too low + /// and real users get 429s on file listings, too high and the + /// `access_grants` visibility query loses the guard that stops an + /// attacker exhausting it with random UUIDs. + #[test] + fn per_caller_rate_limit_defaults_match_the_previous_literals() { + let rl = RateLimitConfig::default(); + assert_eq!(rl.user_profile_max_requests, 60); + assert_eq!(rl.user_profile_window_secs, 60); + assert_eq!(rl.delta_upload_max_requests, 240); + assert_eq!(rl.delta_upload_window_secs, 60); + } + #[test] fn startup_job_parses_name_and_flags() { let jobs = parse_startup_jobs( diff --git a/src/common/di.rs b/src/common/di.rs index 8daa9324..0ed2fced 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -2330,19 +2330,29 @@ impl AppServiceFactory { mock_email_sender: None, // populated below magic_link_invite_service: None, // populated below recipient_notification_service: None, // populated below alongside magic_link_invite_service - // 60 lookups / minute / caller; cap at 50 000 tracked - // callers to bound memory. The same limiter instance is - // shared by every clone of AppState since it lives in an - // Arc. + // Per-caller limits, configurable since the hardcoded ceilings + // had no escape hatch for deployments where several actors share + // one identity — a CI suite running as a single `admin` shares + // one bucket and trips a limit sized for one human. Defaults + // match the literals these replaced, so an operator who sets + // nothing sees no change. `OXICLOUD_RATE_LIMIT_USER_PROFILE_*` / + // `OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_*`. + // + // 50 000 tracked callers caps memory in both cases. The limiter + // lives in an Arc, so every clone of AppState shares the counts. user_profile_rate_limiter: Arc::new( - crate::interfaces::middleware::rate_limit::RateLimiter::new(60, 60, 50_000), + crate::interfaces::middleware::rate_limit::RateLimiter::new( + self.config.auth.rate_limit.user_profile_max_requests, + self.config.auth.rate_limit.user_profile_window_secs, + 50_000, + ), ), - // Delta upload: 240 requests / minute / caller. Generous for a - // real client (chunk PUTs carry up to 100 MB each) while - // stopping pin/negotiate floods; 50 000 tracked callers bound - // the memory like the other limiters. delta_upload_rate_limiter: Arc::new( - crate::interfaces::middleware::rate_limit::RateLimiter::new(240, 60, 50_000), + crate::interfaces::middleware::rate_limit::RateLimiter::new( + self.config.auth.rate_limit.delta_upload_max_requests, + self.config.auth.rate_limit.delta_upload_window_secs, + 50_000, + ), ), // PR 12 — per-sharer email-invite ceiling: caller_id-keyed. // Defends against a compromised account spamming external diff --git a/tests/common/server.env b/tests/common/server.env index a24a4127..a661242d 100644 --- a/tests/common/server.env +++ b/tests/common/server.env @@ -103,6 +103,26 @@ OXICLOUD_RATE_LIMIT_LOGIN_WINDOW_SECS=3600 OXICLOUD_RATE_LIMIT_REGISTER_MAX=36000 OXICLOUD_RATE_LIMIT_REGISTER_WINDOW_SECS=3600 +# The three above are keyed on the client IP. These two are keyed on the +# CALLER ID, which is why raising the three did nothing: the whole suite +# runs as a single `admin`, so every test shares one bucket. +# +# A 763-line e2e server log showed 81 × 429 — all on `http::api`, never +# `http::api::auth` — against the 60/min default for user-profile +# lookups. Admin views resolve an owner name per row, and the run +# creates 34 users, so a minute of tests clears 60 easily. Nothing +# failed, because the SPA degrades to an unresolved name, which is +# precisely why it went unnoticed: the noise would hide a real +# rate-limit regression. +# +# Same posture as above — a 1-hour window with a budget far beyond what +# one run consumes, rather than a raised per-minute rate that would +# still burst-trip. +OXICLOUD_RATE_LIMIT_USER_PROFILE_MAX=36000 +OXICLOUD_RATE_LIMIT_USER_PROFILE_WINDOW_SECS=3600 +OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_MAX=36000 +OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_WINDOW_SECS=3600 + # Magic-link / external-users flow (PR 9). The mock SMTP captures every # outbound message in-process so external_users.hurl can retrieve the # invitation body and follow the magic-link URL. The `SMTP_FROM` value From 289f408d239ea43b93b54bf131ea885f8d9ceeab Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 5 Sep 2026 23:01:53 +0200 Subject: [PATCH 4/4] perf(dpop): collapse the SW nonce stampede to a single challenge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A DPoP proof carries a server-issued nonce. With none cached the server answers `401 use_dpop_nonce`, the client harvests the nonce and retries. `signAndFetch` already absorbed that, so it was invisible — but it did it PER REQUEST, with no coordination. The Service Worker always cold-starts without a nonce. Both mechanisms that pre-seed the page are unreachable from worker scope: workers have no `sessionStorage`, and `seedNonceFromCookie` early-returns on `typeof document === 'undefined'`. The SW also skips requests that already carry a `DPoP` header, so it never observes the page's responses and cannot harvest from them either. Browsers terminate idle workers after ~30s, so this happens routinely, not once. Uncoordinated, every request issued in that window discovers the nonce independently: N parallel requests → N challenges → 2N requests. That is precisely the photo grid, and serving `` is the SW's main job — those requests cannot sign themselves, which is why the worker exists. Each wasted challenge also costs the server a full ECDSA P-256 verify, because `verify_proof` runs before the nonce check. Now the first request through owns the discovery and the rest await it, so N challenges collapse to 1. The wait is capped (5s) and released in a `finally`: the SW is on the critical path for every thumbnail, so a hung discovery must degrade to the old behaviour rather than stall the grid behind a promise that never settles. Measured on a 763-line e2e server log: 115 `dpop.nonce_challenged` events across 97 logins. Scope, deliberately: this does NOT remove the one challenge per worker lifetime. Doing that needs the nonce persisted where a worker can read it — IndexedDB already holds the keypair — and it can never replace the challenge path anyway, since a persisted nonce can be stale. Left out until the audit line shows it is worth it; the numbers above are now legible enough to tell. Not a correctness fix. Nothing was broken and no test failed over this. The argument is waste, plus signal: 115 challenges per run is noise that would bury a real one. `hasNonce()` is exported for the gate — deliberately "will the next proof carry a nonce", not "is it still valid", since only the server knows the latter and the challenge path already handles it. Its tests assert it agrees with what `buildDpopProof` actually emits, because a wrong answer either reinstates the stampede or stalls every request behind a bootstrap that is not happening. Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/lib/auth/dpop-proof.test.ts | 43 ++++++++++++ frontend/src/lib/auth/dpop-proof.ts | 17 +++++ frontend/src/service-worker.ts | 87 ++++++++++++++++++++++++ 3 files changed, 147 insertions(+) diff --git a/frontend/src/lib/auth/dpop-proof.test.ts b/frontend/src/lib/auth/dpop-proof.test.ts index 9e29add4..2dcab922 100644 --- a/frontend/src/lib/auth/dpop-proof.test.ts +++ b/frontend/src/lib/auth/dpop-proof.test.ts @@ -17,6 +17,7 @@ import { buildDpopProof, canonicalHtu, clearNonce, + hasNonce, isDpopNonceChallenge, updateNonceFromResponse } from './dpop-proof'; @@ -153,3 +154,45 @@ describe('updateNonceFromResponse', () => { expect(b64uDecodeJson(proof!.split('.')[1]).nonce).toBe('v2'); }); }); + +describe('hasNonce', () => { + // The Service Worker's single-flight gate keys off this: `false` + // means the next proof carries no nonce and WILL be challenged, so + // concurrent requests should let one of them discover it rather than + // each taking its own 401. A wrong answer here either reinstates the + // stampede (false negative) or stalls every request behind a + // bootstrap that is not happening (false positive). + it('is false before any nonce has been seen', () => { + expect(hasNonce()).toBe(false); + }); + + it('is true once a nonce has been harvested', () => { + updateNonceFromResponse( + new Response(null, { status: 200, headers: { 'DPoP-Nonce': 'nonce-1' } }) + ); + expect(hasNonce()).toBe(true); + }); + + it('agrees with what the proof actually carries', async () => { + expect(hasNonce()).toBe(false); + const before = await buildDpopProof('GET', '/api/x'); + expect(b64uDecodeJson(before!.split('.')[1]).nonce).toBeUndefined(); + + updateNonceFromResponse( + new Response(null, { status: 200, headers: { 'DPoP-Nonce': 'nonce-2' } }) + ); + + expect(hasNonce()).toBe(true); + const after = await buildDpopProof('GET', '/api/x'); + expect(b64uDecodeJson(after!.split('.')[1]).nonce).toBe('nonce-2'); + }); + + it('is false again after logout clears the nonce', () => { + updateNonceFromResponse( + new Response(null, { status: 200, headers: { 'DPoP-Nonce': 'nonce-3' } }) + ); + expect(hasNonce()).toBe(true); + clearNonce(); + expect(hasNonce()).toBe(false); + }); +}); diff --git a/frontend/src/lib/auth/dpop-proof.ts b/frontend/src/lib/auth/dpop-proof.ts index c55dda5a..5460d6e8 100644 --- a/frontend/src/lib/auth/dpop-proof.ts +++ b/frontend/src/lib/auth/dpop-proof.ts @@ -82,6 +82,23 @@ export function seedNonceFromCookie(): void { document.cookie = 'oxicloud_dpop_nonce=; SameSite=Strict; Path=/; Max-Age=0'; } +/** + * Whether a nonce is cached, i.e. whether the next proof will carry one. + * + * Exists for the Service Worker's single-flight gate: `false` means the + * next request WILL be answered with a nonce challenge, so concurrent + * callers should let one request discover the nonce rather than each + * discovering it separately. See `signAndFetch` in `service-worker.ts`. + * + * Deliberately not "is the nonce still valid" — only the server knows + * that, and a stale nonce is handled by the challenge/retry path. This + * answers the cheaper question that avoids a stampede. + */ +export function hasNonce(): boolean { + loadNonceOnce(); + return currentNonce !== null; +} + /** Wipe the current nonce — called on logout so a new session bootstraps fresh. */ export function clearNonce(): void { currentNonce = null; diff --git a/frontend/src/service-worker.ts b/frontend/src/service-worker.ts index 3e30141b..a51e93b0 100644 --- a/frontend/src/service-worker.ts +++ b/frontend/src/service-worker.ts @@ -43,6 +43,7 @@ import { buildDpopProof, + hasNonce, isDpopNonceChallenge, updateNonceFromResponse } from '$lib/auth/dpop-proof'; @@ -79,7 +80,93 @@ self.addEventListener('fetch', (event) => { event.respondWith(signAndFetch(req)); }); +/** + * In-flight nonce discovery, or `null` when a nonce is already cached. + * + * Single-flight gate — see [`signAndFetch`]. + */ +let nonceBootstrap: Promise | null = null; + +/** + * Cap on how long a request will wait for someone else's bootstrap. + * + * The SW sits on the critical path for every thumbnail, so a hung or + * pathologically slow discovery must not stall the grid behind it. On + * expiry the waiter proceeds unsigned-by-nonce and takes its own + * challenge — exactly the pre-gate behaviour, so the worst case is what + * we had before rather than a stall. + */ +const NONCE_BOOTSTRAP_WAIT_MS = 5_000; + +/** + * DPoP-sign one request, with a **single-flight gate on the first + * nonce**. + * + * ## Why the gate + * + * A DPoP proof carries a server-issued nonce. With none cached the + * server answers `401 use_dpop_nonce`, the client harvests the nonce + * from the response and retries — one extra round trip, absorbed here + * so the caller never sees it. + * + * The SW's nonce cache is per-worker and **in memory only**: workers + * have no `sessionStorage`, and `seedNonceFromCookie` needs `document`, + * so neither mechanism that pre-seeds the page reaches this scope. A + * worker therefore always cold-starts with no nonce — and browsers + * terminate idle workers after ~30s, so that happens often. + * + * Without a gate, every request issued in that window discovers the + * nonce *independently*: N parallel requests → N challenges → 2N + * requests. The photo grid is exactly this shape, and it is the SW's + * main job (`` thumbnails can't sign themselves). Each wasted + * challenge also costs the server a full ECDSA verify, since + * `verify_proof` runs before the nonce check. + * + * So: the first request through discovers the nonce; the rest await it + * and then sign normally. N challenges collapse to 1. + * + * ## What this is not + * + * Not a correctness fix — the retry already made this invisible. It + * removes waste, and it keeps `dpop.nonce_challenged` rare enough in + * the audit log to be worth reading. + * + * Not a cure for cold starts. One challenge per worker lifetime + * remains; removing that needs the nonce persisted somewhere the worker + * can reach (IndexedDB already holds the keypair). Deliberately left + * out — it cannot replace the challenge path anyway, since a persisted + * nonce can be stale. + */ async function signAndFetch(req: Request): Promise { + if (!hasNonce()) { + if (nonceBootstrap) { + // Someone else is already discovering it. Wait — but never + // indefinitely; on timeout fall through and take our own + // challenge. + await Promise.race([ + nonceBootstrap, + new Promise((resolve) => setTimeout(resolve, NONCE_BOOTSTRAP_WAIT_MS)) + ]); + } else { + // We own the discovery. `finally` releases on every path, + // including a thrown fetch — a waiter blocked on a promise + // that never settles would be worse than the stampede. + let release!: () => void; + nonceBootstrap = new Promise((resolve) => { + release = resolve; + }); + try { + return await signAndFetchOnce(req); + } finally { + nonceBootstrap = null; + release(); + } + } + } + return signAndFetchOnce(req); +} + +async function signAndFetchOnce(req: Request): Promise { const firstProof = await buildDpopProof(req.method, req.url).catch(() => null); // No keypair (IndexedDB blocked, SubtleCrypto missing, etc.) — pass // through unsigned. Unbound sessions still work; bound sessions in