OAUTH-001B9 — Refuse stale Strava disconnect cleanup

SECURITY.md

@@ -66,7 +66,7 @@ The findings below describe the repository at the start of the 2026-07-12 assess
 | RISK-021 | Admin state transitions accept impossible combinations: cancel paid without refund decision, fulfill unpaid/cancelled orders, substitute without new waiver, or comp without proven waiver. | Financial, legal, fulfillment, and waiver records can be inconsistent. | Centralize allowed state transitions and authorization; require reason/evidence and test every forbidden edge. |
 | RISK-022 | Existing rate-limit documents contain raw IP or email keys and values; the profile-thumbnail slice instead uses a domain-separated SHA-256 account-key pseudonym, which is stable and not anonymous. Fixed windows are distributed only by those identifiers and cleanup depends on an externally configured TTL. The profile-thumbnail upload limiter charges every image-processing invocation, including an exact domain-command retry, because request ID alone cannot prove unchanged image bytes without retaining more image-derived data. | Additional PII or linkable pseudonymous storage, unbounded growth if TTL is absent, identifier-targeted denial of service, retry quota consumption, and easy distributed abuse. | HMAC identifiers with a rotating secret, verify TTL, add per-user/business budgets, monitor rejects/cost, and keep retry side effects limited to abuse-control bookkeeping rather than profile/photo/audit state. |
 | RISK-023 | The legacy `functions.config().api.key` endpoint uses a static shared key with no replay control, source restriction, App Check, authenticated operator, or request audit. The API is also scheduled for Firebase decommissioning in March 2027. | Key theft permits bulk role changes and future deployment failure. | Retire it in favor of authenticated admin workflow or narrowly authenticated scheduled import; migrate any remaining config to Secret Manager. |
-| RISK-024 | OAuth tokens remain plaintext. Repository Rules deny browser access, but protected deployment and live behavior are unproven; least-privilege runtime IAM and an encryption decision remain open. OAUTH-001A1H [#606](https://github.com/Run-MPRC/Run-MPRC.github.io/issues/606) source transactionally rereads the exact access-token, refresh-token, and expiry tuple before persisting a refresh response. A retry rereads the tuple; changed, missing, or malformed state returns one fixed failure without a stale write or downstream bearer call. Synthetic tests use no real token, account, or provider call. Provider refresh calls are still not serialized; identical-tuple replacement, post-commit acknowledgement reconciliation, deployment, and live behavior remain unproven. | Token theft exposes member activity data; an undeployed boundary or an unhandled provider/acknowledgement race can still lose availability or leave local and provider state ambiguous. | Deploy and read back server/IAM-only secret access, decide encryption, serialize or otherwise reconcile refresh operations and lost acknowledgements, minimize scopes, audit access, and verify only with approved made-up staged accounts. |
+| RISK-024 | OAuth tokens remain plaintext. Repository Rules deny browser access, but protected deployment and live behavior are unproven; least-privilege runtime IAM and an encryption decision remain open. OAUTH-001A1H [#606](https://github.com/Run-MPRC/Run-MPRC.github.io/issues/606) source transactionally rereads the exact access-token, refresh-token, and expiry tuple before persisting a refresh response. A retry rereads the tuple; changed, missing, or malformed state returns one fixed failure without a stale write or downstream bearer call. OAUTH-001B9 [#608](https://github.com/Run-MPRC/Run-MPRC.github.io/issues/608) source records the disconnect secret's Firestore update time before optional revocation, then transactionally rereads it and deletes the local secret/connection pair only when the secret is absent or still has that exact version. A newer version, including byte-identical replacement, is preserved with one fixed unknown result; a transaction conflict rereads without repeating provider work. Synthetic tests use no real token, account, or provider call. Provider calls are still not serialized; provider/local and lost-acknowledgement reconciliation, a trusted connection-only write that does not also version the secret, client confirmation admission, deployment, and live behavior remain unproven. | Token theft exposes member activity data; an undeployed boundary or an unhandled provider/acknowledgement race can still lose availability or leave local and provider state ambiguous. | Deploy and read back server/IAM-only secret access, decide encryption, serialize or otherwise reconcile refresh and disconnect operations and lost acknowledgements, minimize scopes, audit access, and verify only with approved made-up staged accounts. |
 | RISK-025 | Admin authentication relies on password Auth plus a long-lived role token; no repository evidence of MFA, recent-auth checks, re-auth for refunds/role grants, or rapid revocation workflow. | A stolen admin session has broad durable impact. | Require MFA for privileged accounts, recent-auth for sensitive actions, capability roles, short sessions/forced refresh, and break-glass procedures. |
 | RISK-042 | Profile photos and an officer people finder have no approved privacy notice, retention/backup treatment, scoped capability, or production release path. The existing Admin Members page downloads complete account documents and cannot enforce an opt-out promise. #505/#506 source now supplies the private processed thumbnail, default-off preference, minimum server-only projection, and bounded audited verified-admin name search, but none is deployed. | A photo, name, query, preference, prefix digest, or stable entry reference could be exposed, retained, enumerated, correlated, or mistaken for membership evidence; automated face matching would add unapproved biometric processing. | Keep the feature default-hidden and separate from the official roster. Preserve current-source revalidation, direct Rules denials, fixed query/result bounds, no-store responses, pseudonymous rate keys, query-free audit, and synthetic race/opt-out/removal proof. Never store the original, persist raw queries, or add face recognition. Complete #110 policy inputs, AUTH-003 scoped authorization follow-up, #133 protected authority, and #507 backend-first release/readback before live use. |
 | RISK-026 | Firestore Admin SDK bypasses rules and the Functions runtime IAM scope is not documented. #135 removes the long-lived service-account JSON path from release source, but the least-privilege short-lived deploy identity is not configured yet. | Server or CI compromise may expose the entire project; missing provider configuration also blocks release. | Complete the private IAM inventory and #133 OIDC/WIF configuration; keep runtime and deploy identities separate; never restore a JSON key shortcut. |

functions/strava.js

@@ -1126,12 +1126,18 @@ exports.stravaDisconnect = functions
     }
     const { uid } = context.auth;
     let secretExists = false;
+    let secretUpdateTime = null;
     let accessToken = null;
     try {
       const secretSnap = await secretDocRef(uid).get();
       secretExists = secretSnap.exists;
       if (secretExists) {
-        accessToken = snapshotStoredDisconnectAccessToken(secretSnap.data());
+        const secretData = secretSnap.data();
+        secretUpdateTime = secretSnap.updateTime;
+        if (!secretUpdateTime || typeof secretUpdateTime.isEqual !== 'function') {
+          throw new Error('invalid disconnect secret version');
+        }
+        accessToken = snapshotStoredDisconnectAccessToken(secretData);
       }
     } catch (_error) {
       console.warn('strava_disconnect_secret_read_failed');
@@ -1168,10 +1174,25 @@ exports.stravaDisconnect = functions
     }
     try {
       const db = admin.firestore();
-      const cleanup = db.batch();
-      cleanup.delete(secretDocRef(uid, db));
-      cleanup.delete(connectionDocRef(uid, db));
-      await cleanup.commit();
+      const secretRef = secretDocRef(uid, db);
+      const connectionRef = connectionDocRef(uid, db);
+      const cleaned = await db.runTransaction(async (transaction) => {
+        const currentSecret = await transaction.get(secretRef);
+        if (currentSecret.exists) {
+          if (!secretExists) return false;
+          const currentUpdateTime = currentSecret.updateTime;
+          if (
+            !currentUpdateTime
+            || secretUpdateTime.isEqual(currentUpdateTime) !== true
+          ) {
+            return false;
+          }
+        }
+        transaction.delete(secretRef);
+        transaction.delete(connectionRef);
+        return true;
+      });
+      if (cleaned !== true) throw new Error('stale disconnect cleanup');
     } catch (_error) {
       console.warn('strava_disconnect_cleanup_failed');
       throw stravaDisconnectError();