From 7f5dcd7e50ef0dd9577b2ba94fb01b213122dc5d Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Wed, 17 Jun 2026 05:47:12 -0400 Subject: [PATCH] fix(upload): attach CORS guidance to direct upload failures A new user's top first-upload failure is a missing R2 CORS policy (browser PUT/POST blocked) -- and object-scoped R2 tokens often cannot auto-set CORS. The connection test already surfaces buildCorsGuidance on the opaque "Failed to fetch", but a user who skips the test and uploads directly got a bare "Upload failed: Failed to fetch". The upload error handler now detects the browser CORS/network rejection (isLikelyCorsError -- Chrome "Failed to fetch", Firefox "NetworkError", Safari "Load failed") and appends the same actionable guidance. Updated the R2 follow-up TODO (credential audit done; CORS reviewed). Test: tests/services/r2-cors-error-classifier.test.mjs. Full suite green. Co-Authored-By: Claude Opus 4.8 --- js/services/r2Service.js | 33 ++++++++++++++++--- .../r2-cors-error-classifier.test.mjs | 28 ++++++++++++++++ todo/TODO_2026-06-16_r2-followups.md | 33 ++++++++++++------- 3 files changed, 79 insertions(+), 15 deletions(-) create mode 100644 tests/services/r2-cors-error-classifier.test.mjs diff --git a/js/services/r2Service.js b/js/services/r2Service.js index 0c74e7b9..9107ab37 100644 --- a/js/services/r2Service.js +++ b/js/services/r2Service.js @@ -103,6 +103,25 @@ function buildCorsGuidance({ accountId } = {}) { ].join(" "); } +/** + * A browser CORS preflight/rejection surfaces as an opaque network error + * ("Failed to fetch" / "NetworkError" / Safari's "Load failed") with no HTTP + * status — indistinguishable from a real network drop. We treat these as + * likely-CORS so the upload path can attach actionable guidance instead of a + * bare "Failed to fetch". + */ +export function isLikelyCorsError(err) { + const message = ( + (err && (err.message || err.toString && err.toString())) || + "" + ).toLowerCase(); + return ( + message.includes("failed to fetch") || + message.includes("networkerror") || + message.includes("load failed") + ); +} + function createDefaultSettings() { return { accountId: "", @@ -1251,10 +1270,16 @@ export class R2Service { return publishOutcome; } catch (err) { userLogger.error("Cloudflare upload failed:", err); - this.setCloudflareUploadStatus( - err?.message ? `Upload failed: ${err.message}` : "Upload failed.", - "error" - ); + let message = err?.message + ? `Upload failed: ${err.message}` + : "Upload failed."; + // A CORS rejection during the browser PUT/POST looks like an opaque + // "Failed to fetch" — attach actionable guidance so users who skipped the + // connection test still know what to fix (the #1 first-upload failure). + if (isLikelyCorsError(err)) { + message += ` ${buildCorsGuidance({ accountId })}`; + } + this.setCloudflareUploadStatus(message, "error"); return false; } finally { this.setCloudflareUploading(false); diff --git a/tests/services/r2-cors-error-classifier.test.mjs b/tests/services/r2-cors-error-classifier.test.mjs new file mode 100644 index 00000000..913fc2b1 --- /dev/null +++ b/tests/services/r2-cors-error-classifier.test.mjs @@ -0,0 +1,28 @@ +// Scenario (SCN-cors-error-classifier): +// A browser CORS rejection during upload surfaces as an opaque network error +// with no HTTP status. isLikelyCorsError() must recognize the messages the +// major browsers actually produce (so the upload path can attach CORS guidance +// instead of a bare "Failed to fetch"), without false-positiving on ordinary +// errors (so we don't wrongly tell users to fix CORS). + +import "../test-helpers/setup-localstorage.mjs"; +import test from "node:test"; +import assert from "node:assert/strict"; + +globalThis.__BITVID_DISABLE_NETWORK_IMPORTS__ = true; +const { isLikelyCorsError } = await import("../../js/services/r2Service.js"); + +test("recognizes the browsers' CORS/network rejection messages", () => { + assert.equal(isLikelyCorsError(new TypeError("Failed to fetch")), true); // Chrome + assert.equal(isLikelyCorsError(new Error("NetworkError when attempting to fetch resource.")), true); // Firefox + assert.equal(isLikelyCorsError(new TypeError("Load failed")), true); // Safari +}); + +test("does NOT false-positive on ordinary upload errors", () => { + assert.equal(isLikelyCorsError(new Error("Access Denied")), false); + assert.equal(isLikelyCorsError(new Error("File is empty (0 bytes).")), false); + assert.equal(isLikelyCorsError(new Error("NoSuchBucket")), false); + assert.equal(isLikelyCorsError(null), false); + assert.equal(isLikelyCorsError(undefined), false); + assert.equal(isLikelyCorsError({}), false); +}); diff --git a/todo/TODO_2026-06-16_r2-followups.md b/todo/TODO_2026-06-16_r2-followups.md index a2d4207d..f8833e2f 100644 --- a/todo/TODO_2026-06-16_r2-followups.md +++ b/todo/TODO_2026-06-16_r2-followups.md @@ -13,18 +13,29 @@ Backlog from the R2 audit. Branch: `unstable` (promote down to beta/main later). ## Open — R2 areas not yet audited/built -### 1. Credential security review (highest priority) -- [ ] Audit how browser-held S3/R2 keys are stored in IndexedDB (the README's own warning). - - Files: `js/services/storageService.js`, `js/r2.js` (legacy settings), `js/ui/profileModal/ProfileStorageController.js`. - - Check: encryption at rest, unlock/lock model (`storageService.isUnlocked`), in-memory exposure window, what `resolveConnection` returns when locked, whether secrets can leak to logs/telemetry. - - Confirm the "trusted operator only" assumption is actually enforced/communicated in the UI. +### 1. Credential security review — DONE (2026-06-17, `200e5cec`) +- [x] Audited browser-held S3/R2 key storage. Verdict: **sound** — envelope encryption + (AES-GCM-256 payload + fresh IV; master key encrypted to-self via the Nostr signer, + NIP-44/NIP-04), in-memory master key cleared on logout, no secret logging, legacy + plaintext migrated then cleared, secrets never in plaintext `meta`. Locked-in by a + scan-the-whole-record regression test. +- [ ] Hardening (optional, post-launch): **idle auto-lock** — master key currently stays + in memory the whole session after unlock. Add an idle/timeout auto-`lock(pubkey)`. +- [ ] Confirm the production CSP is strict (the real defense for in-memory keys / XSS). -### 2. Bucket / custom-domain provisioning -- [ ] Audit `ensureBucketConfigForNpub` (`r2Service.js`) + `js/storage/r2-mgmt.js` - (`ensureBucket`, `putCors`, `attachCustomDomainAndWait`, `setManagedDomain`, - `deriveShortSubdomain`) for correctness + failure handling. - - Check: Cloudflare management **API token** scope/usage, partial-failure recovery, - CORS rule correctness, idempotency, what happens when the user pre-created the bucket. +### 2. Bucket / custom-domain provisioning — partially audited (2026-06-17) +- [x] CORS provisioning reviewed: `ensureBucketConfigForNpub` auto-creates the bucket + + sets CORS best-effort (S3 `PutBucketCors`); the connection test (`verifyPublicAccess`) + does a real browser upload+fetch and surfaces `buildCorsGuidance` on the opaque + "Failed to fetch". Gap fixed: the **direct upload** path now also attaches CORS + guidance via `isLikelyCorsError` (``), for users who skip the test. +- [ ] Still to check: `r2-mgmt.js` (`attachCustomDomainAndWait`, `setManagedDomain`, + `deriveShortSubdomain`) — the managed-domain path; Cloudflare API-token scope; + idempotency; partial-failure recovery. Note: token-based bucket creation/domain + mgmt is marked deprecated in `ensureBucketConfigForNpub` — confirm it's fully unused. +- [ ] CORS replace-not-merge: `PutBucketCors` overwrites the whole config with only the + current origin, so a bucket used from multiple bitvid origins keeps only the last. + Consider merging existing origins. ### 3. CDN purge integration - [ ] `scripts/purge-cloudflare-changed.mjs` uses `CLOUDFLARE_ZONE_ID` + `CLOUDFLARE_API_TOKEN`