diff --git a/js/services/r2Service.js b/js/services/r2Service.js index 9107ab37..694e3a72 100644 --- a/js/services/r2Service.js +++ b/js/services/r2Service.js @@ -45,6 +45,7 @@ import { collectVideoStorageKeys, } from "../utils/storagePointer.js"; import { safeEncodeNpub } from "../utils/nostrHelpers.js"; +import { isLikelyCorsError } from "../utils/uploadErrorHints.js"; import { getVideoNoteErrorMessage, normalizeVideoNotePayload, @@ -103,24 +104,9 @@ 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") - ); -} +// Shared CORS-error classifier (also used by s3UploadService). Re-exported here +// for backward compatibility with existing imports/tests. +export { isLikelyCorsError }; function createDefaultSettings() { return { @@ -163,6 +149,7 @@ function safeDecodeNpub(npub) { export class R2Service { constructor({ makeR2Client: makeR2ClientOverride, + makeS3Client: makeS3ClientOverride, multipartUpload: multipartUploadOverride, ensureBucketExists: ensureBucketExistsOverride, ensureBucketCors: ensureBucketCorsOverride, @@ -171,6 +158,7 @@ export class R2Service { this.listeners = new Map(); this.cloudflareSettings = null; this.makeR2Client = makeR2ClientOverride || makeR2Client; + this.makeS3Client = makeS3ClientOverride || makeS3Client; this.multipartUpload = multipartUploadOverride || multipartUpload; this.ensureBucketExists = ensureBucketExistsOverride || ensureBucketExists; this.ensureBucketCors = ensureBucketCorsOverride || ensureBucketCors; @@ -260,13 +248,29 @@ export class R2Service { let s3; try { await ensureS3SdkLoaded(); - s3 = this.makeR2Client({ - accountId: settings.accountId, - endpoint: settings.endpoint, - accessKeyId, - secretAccessKey, - region: settings.region, - }); + // Provider-aware client: Cloudflare R2 is always path-style; a generic S3 + // bucket may be virtual-hosted-style, so honor the connection's + // forcePathStyle (mirrors uploadFile). Using makeR2Client unconditionally + // here forced path-style and broke cleanup for vhost-style S3 buckets. + const useR2 = + settings.provider === "cloudflare_r2" || Boolean(settings.accountId); + if (useR2) { + s3 = this.makeR2Client({ + accountId: settings.accountId, + endpoint: settings.endpoint, + accessKeyId, + secretAccessKey, + region: settings.region, + }); + } else { + s3 = this.makeS3Client({ + endpoint: settings.endpoint, + region: settings.region, + accessKeyId, + secretAccessKey, + forcePathStyle: Boolean(settings.forcePathStyle), + }); + } } catch (err) { devLogger.warn( "[R2Service] deleteVideoStorage: failed to build storage client:", diff --git a/js/services/s3UploadService.js b/js/services/s3UploadService.js index 5e1ca9e0..fdf00e62 100644 --- a/js/services/s3UploadService.js +++ b/js/services/s3UploadService.js @@ -18,6 +18,10 @@ import { VIDEO_NOTE_ERROR_CODES, } from "./videoNotePayload.js"; import { calculateTorrentInfoHash } from "../utils/torrentHash.js"; +import { + isLikelyCorsError, + buildGenericCorsGuidance, +} from "../utils/uploadErrorHints.js"; const STATUS_VARIANTS = new Set(["info", "success", "error", "warning"]); const INFO_HASH_PATTERN = /^[a-f0-9]{40}$/; @@ -510,10 +514,15 @@ export class S3UploadService { return Boolean(published); } catch (err) { this.deps.userLogger.error("S3 upload failed:", err); - this.setUploadStatus( - 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 is an opaque "Failed to + // fetch" — attach actionable guidance (parity with the R2 path). + if (isLikelyCorsError(err)) { + message += ` ${buildGenericCorsGuidance({ endpoint: normalized?.endpoint })}`; + } + this.setUploadStatus(message, "error"); return false; } finally { this.setUploading(false); diff --git a/js/utils/uploadErrorHints.js b/js/utils/uploadErrorHints.js new file mode 100644 index 00000000..1bc38601 --- /dev/null +++ b/js/utils/uploadErrorHints.js @@ -0,0 +1,42 @@ +// Shared helpers for turning opaque upload failures into actionable guidance. +// Used by both upload paths (Cloudflare R2 via r2Service, generic S3 via +// s3UploadService) so the behavior stays in sync. + +/** + * 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") + ); +} + +/** + * Provider-neutral CORS guidance for a generic S3 bucket. (Cloudflare R2 has a + * more specific variant in r2Service that names the R2 S3 API endpoint.) + */ +export function buildGenericCorsGuidance({ endpoint } = {}) { + const origin = + typeof window !== "undefined" && + window.location && + window.location.origin && + window.location.origin !== "null" + ? window.location.origin + : ""; + const target = endpoint ? endpoint : "your bucket's S3 API endpoint"; + return [ + "This is likely a CORS issue.", + `Configure CORS on ${target} to allow origin ${origin},`, + "with AllowedMethods: GET, HEAD, PUT, POST, DELETE, OPTIONS and AllowedHeaders: *.", + ].join(" "); +} diff --git a/tests/services/r2-delete-video-storage.test.mjs b/tests/services/r2-delete-video-storage.test.mjs index d89c57ba..aeed570a 100644 --- a/tests/services/r2-delete-video-storage.test.mjs +++ b/tests/services/r2-delete-video-storage.test.mjs @@ -18,6 +18,8 @@ const { R2Service } = await import("../../js/services/r2Service.js"); const BASE = "https://pub.bitvid.network"; const CREDS = { + provider: "cloudflare_r2", + accountId: "acct", accessKeyId: "AK", secretAccessKey: "SK", bucket: "my-bucket", @@ -28,8 +30,16 @@ const CREDS = { function buildService(creds, deleteImpl) { const deleted = []; + const clients = []; // records which client builder was used (r2 vs s3) + args const svc = new R2Service({ - makeR2Client: () => ({ __mock: true }), + makeR2Client: (args) => { + clients.push({ kind: "r2", args }); + return { __mock: "r2" }; + }, + makeS3Client: (args) => { + clients.push({ kind: "s3", args }); + return { __mock: "s3" }; + }, deleteObject: deleteImpl || (async ({ key }) => { @@ -37,11 +47,11 @@ function buildService(creds, deleteImpl) { }), }); svc.resolveConnection = async () => creds; - return { svc, deleted }; + return { svc, deleted, clients }; } -test("deletes the video, its .torrent, and the thumbnail for an owned video", async () => { - const { svc, deleted } = buildService(CREDS); +test("deletes the video, its .torrent, and the thumbnail for an owned video (R2)", async () => { + const { svc, deleted, clients } = buildService(CREDS); const res = await svc.deleteVideoStorage({ npub: "npub1owner", videos: [ @@ -54,6 +64,33 @@ test("deletes the video, its .torrent, and the thumbnail for an owned video", as new Set(["u/np/h/clip.mp4", "u/np/h/clip.torrent", "u/np/h/clip.thumb.jpg"]), ); assert.equal(res.deleted.length, 3); + // R2 connection => R2 (path-style) client. + assert.equal(clients[0]?.kind, "r2"); +}); + +test("uses a forcePathStyle-aware S3 client for a generic-S3 connection", async () => { + // No accountId, non-R2 provider, virtual-hosted-style bucket. + const s3Creds = { + provider: "s3", + accessKeyId: "AK", + secretAccessKey: "SK", + bucket: "my-bucket", + publicBaseUrl: BASE, + endpoint: "https://s3.us-east-1.amazonaws.com", + region: "us-east-1", + forcePathStyle: false, + }; + const { svc, deleted, clients } = buildService(s3Creds); + const res = await svc.deleteVideoStorage({ + npub: "npub1owner", + videos: [{ url: `${BASE}/u/np/h/clip.mp4` }], + }); + assert.equal(res.skipped, false); + assert.ok(deleted.includes("u/np/h/clip.mp4"), "deletes the S3 object"); + // Generic S3 => S3 client built with the connection's forcePathStyle (not forced). + assert.equal(clients[0]?.kind, "s3"); + assert.equal(clients[0]?.args.forcePathStyle, false); + assert.equal(clients[0]?.args.endpoint, "https://s3.us-east-1.amazonaws.com"); }); test("skips entirely when storage is locked (no credentials available)", async () => { diff --git a/tests/services/s3-upload-cors-guidance.test.mjs b/tests/services/s3-upload-cors-guidance.test.mjs new file mode 100644 index 00000000..f8adb817 --- /dev/null +++ b/tests/services/s3-upload-cors-guidance.test.mjs @@ -0,0 +1,77 @@ +// Scenario (SCN-s3-upload-cors-guidance): +// Parity with the R2 path — when a generic-S3 upload fails with an opaque +// browser CORS rejection ("Failed to fetch"), the user-facing status must +// include actionable CORS guidance, not a bare "Upload failed: Failed to +// fetch". Ordinary errors must NOT get the CORS hint. + +import "../test-helpers/setup-localstorage.mjs"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { S3UploadService } from "../../js/services/s3UploadService.js"; + +function makeDeps(uploadImpl) { + return { + ensureS3SdkLoaded: async () => {}, + makeS3Client: () => ({}), + multipartUpload: uploadImpl, + buildR2Key: () => "u/np/ns/video.mp4", + computeStorageContentHash: async () => "sha256deadbeef", + buildS3ObjectUrl: () => "https://mock.example/file", + getCorsOrigins: () => ["*"], + prepareS3Connection: async (o) => ({ ...o }), + validateS3Connection: (s) => ({ + ...s, + endpoint: "https://s3.us-east-1.amazonaws.com", + region: "us-east-1", + accessKeyId: "k", + secretAccessKey: "s", + bucket: "b", + forcePathStyle: false, + publicBaseUrl: "https://mock.example", + }), + userLogger: { warn() {}, error() {} }, + buildStoragePointerValue: () => "ptr", + buildStoragePrefixFromKey: () => "prefix", + getVideoNoteErrorMessage: (c) => `Error: ${c}`, + normalizeVideoNotePayload: (p) => ({ payload: p, errors: [] }), + calculateTorrentInfoHash: async () => "a".repeat(40), + }; +} + +const fakeFile = { name: "v.mp4", type: "video/mp4", size: 10, slice() { return this; } }; + +async function uploadAndCaptureError(uploadImpl) { + const service = new S3UploadService(makeDeps(uploadImpl)); + const errors = []; + service.on("uploadStatus", (d) => { + if (d.variant === "error") errors.push(d.message); + }); + await service.uploadVideo({ + npub: "npub1x", + file: fakeFile, + metadata: { title: "T" }, + settings: { endpoint: "https://s3.mock", accessKeyId: "k", secretAccessKey: "s", bucket: "b" }, + infoHash: "a".repeat(40), + publishVideoNote: async () => true, + }); + return errors; +} + +test("a CORS-like upload failure surfaces actionable CORS guidance", async () => { + const errors = await uploadAndCaptureError(async () => { + throw new TypeError("Failed to fetch"); + }); + const msg = errors.join(" | "); + assert.match(msg, /CORS/i, `expected CORS guidance, got: ${msg}`); + assert.match(msg, /s3\.us-east-1\.amazonaws\.com/, "names the S3 endpoint to configure"); +}); + +test("an ordinary upload error does NOT get the CORS hint", async () => { + const errors = await uploadAndCaptureError(async () => { + throw new Error("Access Denied"); + }); + const msg = errors.join(" | "); + assert.ok(msg.includes("Access Denied")); + assert.doesNotMatch(msg, /CORS/i); +}); diff --git a/todo/TODO_2026-06-16_r2-followups.md b/todo/TODO_2026-06-16_r2-followups.md index f8833e2f..ad35a20c 100644 --- a/todo/TODO_2026-06-16_r2-followups.md +++ b/todo/TODO_2026-06-16_r2-followups.md @@ -44,8 +44,11 @@ Backlog from the R2 audit. Branch: `unstable` (promote down to beta/main later). - Note: this is for the *bitvid app* zone, separate from per-user R2 buckets. ## Open — follow-ups to the cleanup work we just shipped -- [ ] **Generic S3 cleanup**: `deleteVideoStorage` uses `makeR2Client` (R2/path-style). - Add a generic-S3 path so `s3UploadService` users' deletes/edits also clean up. +- [x] **Generic S3 cleanup** — DONE: `deleteVideoStorage` is now provider-aware + (uses `makeS3Client` honoring the connection's `forcePathStyle` for non-R2), + so generic-S3 deletes/edits clean up too. Also brought S3 to upload-failure + CORS-guidance parity (shared `js/utils/uploadErrorHints.js`). Generic-S3 is + now at functional parity with R2 for upload/publish/cleanup. - [ ] **All-versions delete cleanup**: delete currently cleans the target video's object; superseded versions from *past* edits (pre-`33c701aa`) are not retroactively cleaned. Consider enumerating all cached versions for a `videoRootId` on full delete.