From 6e85c2a32b1768c44abfc69595e5b262a395bada Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Tue, 30 Jun 2026 16:02:37 -0400 Subject: [PATCH] =?UTF-8?q?[storage]=20fix=20B2=20/=20generic-S3=20uploads?= =?UTF-8?q?=20("S3=20endpoint=20is=20required")=20=E2=80=94=20prepareUploa?= =?UTF-8?q?d=20contract?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Uploading to a Backblaze B2 (or any generic-S3) bucket failed with "S3 endpoint is required." even though the connection tested fine. Root cause: the two upload services have different prepareUpload contracts — r2Service.prepareUpload(npub, { credentials }) vs s3UploadService.prepareUpload(settings). MediaUploader calls both the r2 way, so for S3/B2 the npub STRING became the settings → validateS3Connection saw no endpoint. Compounding it, a storageService.getConnection() result keeps bucket/endpoint/region under `.meta`, which validateS3Connection reads at the top level. (R2 worked because its service already uses the (npub, {credentials}) contract.) Fix: - s3UploadService.prepareUpload now accepts (npub, { credentials }) AND a direct settings object (back-compat), flattening meta → top-level via flattenS3Settings. - The storage Test now derives/passes the B2 public URL (else the prefix), so it stops falsely warning "Public Bucket URL is missing. Uploads will fail." Tests: tests/services/s3-upload-prepare-contract.test.mjs (3 — meta flatten, npub ignored when credentials given, direct-settings back-compat). Build + lint clean; upload/storage suites 46/46. Co-Authored-By: Claude Opus 4.8 --- js/services/s3UploadService.js | 35 ++++++- .../profileModal/ProfileStorageController.js | 6 ++ .../s3-upload-prepare-contract.test.mjs | 98 +++++++++++++++++++ todo/TODO_2026-06-20_pre-launch.md | 13 +++ 4 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 tests/services/s3-upload-prepare-contract.test.mjs diff --git a/js/services/s3UploadService.js b/js/services/s3UploadService.js index fdf00e62..8c42bbcd 100644 --- a/js/services/s3UploadService.js +++ b/js/services/s3UploadService.js @@ -26,6 +26,30 @@ import { const STATUS_VARIANTS = new Set(["info", "success", "error", "warning"]); const INFO_HASH_PATTERN = /^[a-f0-9]{40}$/; +// A storageService.getConnection() result keeps bucket/region/endpoint/forcePathStyle +// under `.meta`; validateS3Connection reads them at the top level. Flatten so a saved +// connection (e.g. Backblaze B2) can be uploaded to without "S3 endpoint is required". +function flattenS3Settings(raw = {}) { + const meta = raw && typeof raw.meta === "object" && raw.meta ? raw.meta : {}; + const pickBool = (a, b) => + typeof a === "boolean" ? a : typeof b === "boolean" ? b : false; + return { + ...raw, + endpoint: raw.endpoint || meta.endpoint || "", + bucket: raw.bucket || meta.bucket || "", + region: raw.region || meta.region || "auto", + accessKeyId: raw.accessKeyId || "", + secretAccessKey: raw.secretAccessKey || "", + forcePathStyle: pickBool(raw.forcePathStyle, meta.forcePathStyle), + publicBaseUrl: + raw.publicBaseUrl || + meta.publicBaseUrl || + raw.baseDomain || + meta.baseDomain || + "", + }; +} + function normalizeInfoHash(value) { return typeof value === "string" ? value.trim().toLowerCase() : ""; } @@ -155,7 +179,16 @@ export class S3UploadService { return prepared; } - async prepareUpload(settings = {}, { createBucketIfMissing = true } = {}) { + async prepareUpload(settingsOrNpub = {}, options = {}) { + const { createBucketIfMissing = true, credentials } = options || {}; + // Two call styles must both work: + // - direct: prepareUpload(settings, { createBucketIfMissing }) + // - mediaUploader: prepareUpload(npub, { credentials }) ← like r2Service + // When credentials come from storageService.getConnection, the bucket/region/ + // endpoint live under `.meta`, so flatten them to the top level that + // validateS3Connection reads. (npub is ignored when credentials are supplied.) + const raw = credentials || settingsOrNpub || {}; + const settings = flattenS3Settings(raw); const prepared = await this.verifyConnection({ settings, createBucketIfMissing, diff --git a/js/ui/profileModal/ProfileStorageController.js b/js/ui/profileModal/ProfileStorageController.js index e1d791af..4a770de2 100644 --- a/js/ui/profileModal/ProfileStorageController.js +++ b/js/ui/profileModal/ProfileStorageController.js @@ -4,6 +4,7 @@ import { prepareS3Connection, getCorsOrigins, deriveB2Endpoint, + derivePublicBaseUrl, } from "../../services/s3Service.js"; import { getActiveSigner } from "../../nostr/client.js"; import { DEFAULT_NIP07_ENCRYPTION_METHODS } from "../../nostr/nip07Permissions.js"; @@ -952,6 +953,11 @@ export class ProfileStorageController { } else { config.endpoint = endpointOrAccount; config.forcePathStyle = forcePathStyle; + // Explicit Public Access URL, else derive it (so the test doesn't falsely warn). + config.publicBaseUrl = + publicBaseUrl || + derivePublicBaseUrl({ endpoint: endpointOrAccount, bucket, forcePathStyle }); + config.baseDomain = config.publicBaseUrl; } try { diff --git a/tests/services/s3-upload-prepare-contract.test.mjs b/tests/services/s3-upload-prepare-contract.test.mjs new file mode 100644 index 00000000..da6e3486 --- /dev/null +++ b/tests/services/s3-upload-prepare-contract.test.mjs @@ -0,0 +1,98 @@ +// Bug: B2 (and generic-S3) uploads failed with "S3 endpoint is required." MediaUploader +// calls service.prepareUpload(npub, { credentials }) (the r2Service contract), but +// s3UploadService.prepareUpload used to take (settings, ...) — so the npub string became +// the settings (no endpoint). Also, a storageService.getConnection() result keeps +// bucket/endpoint under `.meta`, which validateS3Connection reads at the top level. +// +// Fix: s3UploadService.prepareUpload accepts (npub, { credentials }) AND flattens meta. + +import "../test-helpers/setup-localstorage.mjs"; +import { test, mock } from "node:test"; +import assert from "node:assert/strict"; +import { S3UploadService } from "../../js/services/s3UploadService.js"; + +function createDeps() { + const seen = {}; + return { + deps: { + ensureS3SdkLoaded: mock.fn(async () => {}), + makeS3Client: mock.fn(() => ({})), + multipartUpload: mock.fn(async () => {}), + buildR2Key: mock.fn(() => "k"), + buildS3ObjectUrl: mock.fn(() => "https://u/file"), + getCorsOrigins: mock.fn(() => ["*"]), + prepareS3Connection: mock.fn(async (opts) => ({ ...opts })), + validateS3Connection: mock.fn((settings) => { + seen.settings = settings; + return { ...settings }; + }), + userLogger: { warn: mock.fn(), error: mock.fn() }, + buildStoragePointerValue: mock.fn(() => "p"), + buildStoragePrefixFromKey: mock.fn(() => "pre"), + getVideoNoteErrorMessage: mock.fn((c) => `Error: ${c}`), + normalizeVideoNotePayload: mock.fn((p) => ({ payload: p, errors: [] })), + calculateTorrentInfoHash: mock.fn(async () => "h"), + }, + seen, + }; +} + +// A B2 connection as storageService.getConnection() returns it: keys + endpoint at the +// top level, but bucket/region/forcePathStyle/publicBaseUrl under `.meta`. +const B2_CONNECTION = { + provider: "backblaze_b2", + accessKeyId: "b2key", + secretAccessKey: "b2secret", + endpoint: "https://s3.us-west-004.backblazeb2.com", + forcePathStyle: false, + meta: { + provider: "backblaze_b2", + bucket: "bitvid", + region: "us-west-004", + publicBaseUrl: "https://bitvid.s3.us-west-004.backblazeb2.com", + forcePathStyle: false, + }, +}; + +test("prepareUpload(npub, { credentials }) flattens the connection's meta to top-level settings", async () => { + const { deps, seen } = createDeps(); + const service = new S3UploadService(deps); + + const { settings, bucketEntry } = await service.prepareUpload("npub1abc", { + credentials: B2_CONNECTION, + }); + + // validateS3Connection must have received endpoint + bucket (the prior bug passed the + // npub string, so endpoint was undefined). + assert.equal(seen.settings.endpoint, "https://s3.us-west-004.backblazeb2.com"); + assert.equal(seen.settings.bucket, "bitvid", "bucket flattened out of meta"); + assert.equal(seen.settings.region, "us-west-004"); + assert.equal(seen.settings.forcePathStyle, false); + assert.equal( + seen.settings.publicBaseUrl, + "https://bitvid.s3.us-west-004.backblazeb2.com", + "public URL flattened (so uploads don't get a blank base URL)", + ); + assert.equal(bucketEntry.bucket, "bitvid"); +}); + +test("the npub is ignored when credentials are supplied (not treated as settings)", async () => { + const { deps, seen } = createDeps(); + const service = new S3UploadService(deps); + await service.prepareUpload("npub1ignored", { credentials: B2_CONNECTION }); + assert.notEqual(seen.settings, "npub1ignored"); + assert.equal(typeof seen.settings, "object"); +}); + +test("still accepts a direct settings object (back-compat)", async () => { + const { deps, seen } = createDeps(); + const service = new S3UploadService(deps); + await service.prepareUpload({ + endpoint: "https://s3.mock", + bucket: "flat-bucket", + accessKeyId: "k", + secretAccessKey: "s", + }); + assert.equal(seen.settings.bucket, "flat-bucket"); + assert.equal(seen.settings.endpoint, "https://s3.mock"); +}); diff --git a/todo/TODO_2026-06-20_pre-launch.md b/todo/TODO_2026-06-20_pre-launch.md index 0e7c37fc..d46934ba 100644 --- a/todo/TODO_2026-06-20_pre-launch.md +++ b/todo/TODO_2026-06-20_pre-launch.md @@ -949,6 +949,19 @@ Reported 2026-06-25. Relates to #17 (NIP-71 interop) / the bitvid→NIP-71 mirro - [ ] **Follow-up (cosmetic):** the upload modal's summary provider label can still show a stale provider until reloaded — refresh `loadFromStorage` when the storage default changes in the Storage pane (functional routing is already correct). +- [x] **B2 / generic-S3 upload contract mismatch FIXED 2026-06-30.** Uploading to a + Backblaze B2 (or any generic-S3) bucket failed with **"S3 endpoint is required."** + Root cause: `MediaUploader` calls `service.prepareUpload(npub, { credentials })` + (the `r2Service` contract), but `s3UploadService.prepareUpload` took `(settings, …)` + — so the npub string became the settings (no endpoint). Also a + `storageService.getConnection()` result keeps `bucket`/`endpoint`/`region` under + `.meta`, which `validateS3Connection` reads at the top level. Fix: + `s3UploadService.prepareUpload` now accepts `(npub, { credentials })` and flattens + meta → top-level via `flattenS3Settings`. (R2 was unaffected because its service + already used that contract.) Also: the storage **Test** now derives/passes the B2 + public URL so it stops falsely warning "Public Bucket URL is missing." Tests: + `tests/services/s3-upload-prepare-contract.test.mjs` (3 — flatten + npub-ignored + + back-compat). Build + lint clean; upload/storage suites 46/46. ### 39. Image uploads should prefer uploading to configured storage (UX) - [x] **Profile image + banner upload — FIXED 2026-06-30.** The UI was already wired