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/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 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/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, 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 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;