Merge pull request #711 from EdouardVanbelle/fix/front-end2end-test-race

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 |

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)

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);
+	});
+});

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;

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<void> | 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 (`<img src>` 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<Response> {
+	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<void>((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<void>((resolve) => {
+				release = resolve;
+			});
+			try {
+				return await signAndFetchOnce(req);
+			} finally {
+				nonceBootstrap = null;
+				release();
+			}
+		}
+	}
+	return signAndFetchOnce(req);
+}
+
+async function signAndFetchOnce(req: Request): Promise<Response> {
 	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

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::<u32>())
+            && 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::<u64>())
+            && 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::<u32>())
+            && 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::<u64>())
+            && 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::<u32>())
             && 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(

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

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,

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

tests/e2e/spa/helpers.ts

@@ -205,6 +205,31 @@ export async function apiLogin(page: Page, admin = TEST_ADMIN): Promise<void> {
  * 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<void>((resolve) =>
+                navigator.serviceWorker.addEventListener(
+                  'controllerchange',
+                  () => resolve(),
+                  { once: true },
+                ),
+              );
+            }
+          })(),
+          new Promise<void>((resolve) => setTimeout(resolve, 10_000)),
+        ]);
+      }
+
       const csrf = document.cookie.match(/(?:^|; )oxicloud_csrf=([^;]+)/)?.[1] ?? '';
       const headers: Record<string, string> = {};
       if (csrf) headers['x-csrf-token'] = csrf;