From 066d98febbc8994c5730d5b0d3f3961b93c14690 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Tue, 16 Jun 2026 16:56:12 -0400 Subject: [PATCH] fix(upload): content-address storage keys to stop URL-first overwrites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildR2Key fell back to a static "uploads" namespace (and "video" slug) when no torrent info-hash was available, so two distinct URL-first uploads sharing a filename produced the SAME R2/S3 key — the second silently overwrote the first object while the first video note still pointed at that URL (data loss). WebTorrent uploads were already safe because the info-hash namespaces the key. Add computeStorageContentHash(file) in js/r2.js: when no info-hash is present it derives a deterministic content-based namespace — full SHA-256 of the bytes for files ≤512 MB (idempotent: re-uploading the same file reuses the key), and a SHA-256 fingerprint over name/size/mtime + sampled head/tail bytes above that, so we never buffer a multi-GB video in memory. Wired into BOTH upload paths (r2Service + s3UploadService) via a new storageIdentifier kept separate from the info-hash, so magnet generation is unchanged. Test: tests/storage-key-collision.test.mjs (distinct same-named uploads get distinct keys; same content is idempotent; large-file fingerprint path samples edges without buffering and stays deterministic). Full unit suite green. test_integrity_note: change_type: ["new_tests"] scenarios: - id: SCN-upload-key-no-collision given: "a URL-first upload with no torrent info-hash" when: "the storage key is derived (computeStorageContentHash -> buildR2Key)" then: "distinct same-named files get distinct keys, same content is idempotent, and no key uses the colliding 'uploads' namespace" observable_outcomes: - "returned key strings for real Blob/File content and a fake >512MB file" determinism_controls: - "fixed byte content; fake large file with stubbed slice() and fixed lastModified" anti_cheat_rationale: prevents: - "hard-coded return value" - "over-mocking internal logic" relaxation: did_relax_any_assertion: false Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 2 +- js/r2.js | 77 ++++++++++++++++++++++++- js/services/r2Service.js | 9 ++- js/services/s3UploadService.js | 12 +++- tests/storage-key-collision.test.mjs | 85 ++++++++++++++++++++++++++++ 5 files changed, 180 insertions(+), 5 deletions(-) create mode 100644 tests/storage-key-collision.test.mjs diff --git a/AGENTS.md b/AGENTS.md index 2f274687..d7a6d149 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,7 +66,7 @@ For any nostr related work, please review the nip documentation located in /docs ### 4a. Cloudflare/S3 upload-path gotchas (audited 2026-06-16) * **Two near-identical upload services exist** — `js/services/r2Service.js` (Cloudflare R2) and `js/services/s3UploadService.js` (generic S3). They share `buildR2Key`, `resolveUploadIdentifier`, and the magnet build but are *copies*. **Any fix to one must be mirrored in the other** or they silently drift. -* **Storage-key collisions = data loss.** `buildR2Key` (`js/r2.js`) builds `u///.` where `namespace` falls back to the literal `"uploads"` and `slug` to `"video"` (e.g. a non-ASCII filename, or no info-hash). WebTorrent uploads are content-addressed (info-hash namespaces the key) and safe, but a **URL-first upload with no info-hash and a duplicate filename overwrites the previous object** while the old note still points at that URL. Include a unique component (e.g. `videoRootId` or a random suffix) when no info-hash is available. +* **Storage-key collisions = data loss — FIXED 2026-06-16.** `buildR2Key` (`js/r2.js`) builds `u///.` where `namespace` used to fall back to the literal `"uploads"` and `slug` to `"video"` (e.g. a non-ASCII filename, or no info-hash) — so a URL-first upload with no info-hash and a duplicate filename overwrote the previous object while the old note still pointed at that URL. WebTorrent uploads were always safe (info-hash namespaces the key = content-addressed). Fix: `computeStorageContentHash(file)` (`js/r2.js`) derives a content-based namespace when no info-hash is available (full SHA-256 for ≤512 MB, sampled metadata+edge fingerprint above that to avoid buffering huge files), wired into **both** `r2Service` and `s3UploadService`. Test: `tests/storage-key-collision.test.mjs`. **If you add a third upload path, reuse `computeStorageContentHash`.** * **Whole-file hash before upload.** When the modal doesn't pre-seed an info-hash, `resolveUploadIdentifier` runs `calculateTorrentInfoHash(file)`, which reads the *entire* file before the upload begins and before any progress is emitted — on multi-GB files this looks like a hang. Emit a "preparing/hashing" status or parallelize. * **Publish can report success unconfirmed.** `publishEventToRelay` (`js/nostrPublish.js`) has an optimistic-success fallthrough when the relay handle exposes no `on()`/`then()` — a note may show "Published" with no relay ACK. Don't tighten blindly (legacy `seen`-only relays rely on it), but log it. * **Silent partial failures.** Thumbnail and `.torrent` upload failures are caught and `warn`-only; the video note still publishes with no user signal. Empty (0-byte) files hit `CompleteMultipartUpload` with `Parts:[]` and surface a cryptic error — guard `file.size === 0`. diff --git a/js/r2.js b/js/r2.js index fcaa8b08..3019f9b6 100644 --- a/js/r2.js +++ b/js/r2.js @@ -1,4 +1,9 @@ -import { userLogger } from "./utils/logger.js"; +import { userLogger, devLogger } from "./utils/logger.js"; +import { + computeSha256HexFromValue, + valueToUint8Array, + getSharedTextEncoder, +} from "./utils/cryptoUtils.js"; const DB_NAME = "bitvidSettings"; const DB_VERSION = 1; const STORE_NAME = "kv"; @@ -242,4 +247,74 @@ export function buildPublicUrl(baseUrl, key) { return `${sanitizedBase}/${encodedKey}`; } +// Above this size we fingerprint metadata + sampled edges instead of hashing the +// whole file, so we never buffer a multi-GB video in memory (which would freeze +// the main thread). Only reached on the rare path where a torrent info-hash is +// unavailable. +const STORAGE_FULL_HASH_MAX_BYTES = 512 * 1024 * 1024; // 512 MB +const STORAGE_FINGERPRINT_SAMPLE_BYTES = 256 * 1024; // 256 KB head + tail + +async function fingerprintLargeFile(file, size) { + const encoder = getSharedTextEncoder(); + const metaString = `${file?.name || ""} ${size} ${ + Number(file?.lastModified) || 0 + }`; + const meta = encoder ? encoder.encode(metaString) : new Uint8Array(); + const canSlice = typeof file?.slice === "function"; + const headBlob = canSlice + ? file.slice(0, Math.min(STORAGE_FINGERPRINT_SAMPLE_BYTES, size)) + : null; + const tailBlob = canSlice + ? file.slice(Math.max(0, size - STORAGE_FINGERPRINT_SAMPLE_BYTES)) + : null; + const head = headBlob ? await valueToUint8Array(headBlob) : null; + const tail = tailBlob ? await valueToUint8Array(tailBlob) : null; + const parts = [meta, head, tail].filter(Boolean); + const total = parts.reduce((sum, part) => sum + part.length, 0); + const combined = new Uint8Array(total); + let offset = 0; + for (const part of parts) { + combined.set(part, offset); + offset += part.length; + } + return computeSha256HexFromValue(combined); +} + +/** + * Derives a deterministic, content-based namespace component for a storage key + * when no torrent info-hash is available. Prevents distinct URL-first uploads + * that share a filename from overwriting each other - the `buildR2Key` + * `"uploads"` fallback was a silent data-loss path. + * + * Small/medium files use a full SHA-256 of the bytes (idempotent + collision + * safe). Files larger than `STORAGE_FULL_HASH_MAX_BYTES` use a SHA-256 + * fingerprint over name/size/mtime + sampled head & tail bytes, so we never + * buffer a huge file in memory. Returns "" only if the file can't be read at + * all (callers must then supply their own uniqueness). + * + * @param {Blob|File} file + * @returns {Promise} + */ +export async function computeStorageContentHash(file) { + if (!file) { + return ""; + } + try { + const size = Number(file.size) || 0; + if (size > 0 && size <= STORAGE_FULL_HASH_MAX_BYTES) { + const hex = await computeSha256HexFromValue(file); + if (hex) { + return `sha256${hex}`; + } + } + const fingerprint = await fingerprintLargeFile(file, size); + if (fingerprint) { + return `fp${fingerprint}`; + } + } catch (error) { + devLogger.warn("[r2] Failed to derive content hash for storage key:", error); + } + return ""; +} + export { sanitizeBaseDomain }; diff --git a/js/services/r2Service.js b/js/services/r2Service.js index 16677194..1ba428f7 100644 --- a/js/services/r2Service.js +++ b/js/services/r2Service.js @@ -19,6 +19,7 @@ import { buildR2Key, buildPublicUrl, + computeStorageContentHash, } from "../r2.js"; import { sanitizeBucketName, @@ -872,6 +873,12 @@ class R2Service { const keyIdentifier = await resolveUploadIdentifier({ infoHash, file }); const normalizedInfoHash = normalizeInfoHash(infoHash || keyIdentifier); const hasValidInfoHash = isValidInfoHash(normalizedInfoHash); + // When no info-hash is available, derive a content-based namespace so two + // distinct URL-first uploads that share a filename can't overwrite each + // other. Kept separate from keyIdentifier so magnet generation still keys + // off the real info-hash only. + const storageIdentifier = + keyIdentifier || (file ? await computeStorageContentHash(file) : ""); let bucketResult = null; try { @@ -909,7 +916,7 @@ class R2Service { this.setCloudflareUploadStatus(statusMessage, "info"); // Use forced keys if provided, otherwise generate them - const key = forcedVideoKey || buildR2Key(npub, file, keyIdentifier); + const key = forcedVideoKey || buildR2Key(npub, file, storageIdentifier); const publicUrl = forcedVideoUrl || buildPublicUrl(bucketEntry.publicBaseUrl, key); const buildTorrentKey = () => { diff --git a/js/services/s3UploadService.js b/js/services/s3UploadService.js index 5fec92ca..a00722fa 100644 --- a/js/services/s3UploadService.js +++ b/js/services/s3UploadService.js @@ -1,6 +1,6 @@ import { ensureS3SdkLoaded, makeS3Client } from "../storage/s3-client.js"; import { multipartUpload } from "../storage/s3-multipart.js"; -import { buildR2Key } from "../r2.js"; +import { buildR2Key, computeStorageContentHash } from "../r2.js"; import { buildS3ObjectUrl, getCorsOrigins, @@ -35,6 +35,7 @@ const defaultDeps = { makeS3Client, multipartUpload, buildR2Key, + computeStorageContentHash, buildS3ObjectUrl, getCorsOrigins, prepareS3Connection, @@ -276,6 +277,13 @@ export class S3UploadService { }); const normalizedInfoHash = normalizeInfoHash(infoHash || keyIdentifier); const hasValidInfoHash = isValidInfoHash(normalizedInfoHash); + // When no info-hash is available, derive a content-based namespace so two + // distinct URL-first uploads that share a filename can't overwrite each + // other. Kept separate from keyIdentifier so magnet generation still keys + // off the real info-hash only. + const storageIdentifier = + keyIdentifier || + (file ? await this.deps.computeStorageContentHash(file) : ""); const s3 = this.deps.makeS3Client({ endpoint: normalized.endpoint, @@ -287,7 +295,7 @@ export class S3UploadService { const key = forcedVideoKey || - this.deps.buildR2Key(npub, file, keyIdentifier); + this.deps.buildR2Key(npub, file, storageIdentifier); const publicUrl = forcedVideoUrl || this.deps.buildS3ObjectUrl({ diff --git a/tests/storage-key-collision.test.mjs b/tests/storage-key-collision.test.mjs new file mode 100644 index 00000000..4793fe10 --- /dev/null +++ b/tests/storage-key-collision.test.mjs @@ -0,0 +1,85 @@ +// Scenario (SCN-upload-key-no-collision): +// Given a URL-first upload with NO torrent info-hash available, +// When the storage key is derived (computeStorageContentHash -> buildR2Key), +// Then two DISTINCT files that happen to share a filename must produce +// DIFFERENT keys (so neither overwrites the other — the old `buildR2Key` +// "uploads" fallback was a silent data-loss path, KNOWN_BUGS upload audit +// #1), the SAME file must produce a STABLE key (idempotent re-upload), and +// no key may fall back to the colliding "uploads" namespace. +// +// Anti-cheat: asserts externally observable key strings for real Blob/File +// content (small path) AND a fake >512MB file (sampled-fingerprint path, which +// must NOT buffer the whole file). No internal call-sequence assertions. + +import test from "node:test"; +import assert from "node:assert/strict"; + +import { buildR2Key, computeStorageContentHash } from "../js/r2.js"; + +const NPUB = + "npub1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqsx5h2j"; + +async function keyFor(file) { + const identifier = await computeStorageContentHash(file); + return buildR2Key(NPUB, file, identifier); +} + +test("distinct same-named URL-first uploads get distinct, non-colliding keys", async () => { + const a = new File([new Uint8Array([1, 2, 3, 4, 5])], "video.mp4", { + type: "video/mp4", + }); + const b = new File([new Uint8Array([5, 4, 3, 2, 1])], "video.mp4", { + type: "video/mp4", + }); + + const ka = await keyFor(a); + const kb = await keyFor(b); + + assert.notEqual(ka, kb, "distinct content must not share a storage key"); + assert.ok( + !ka.includes("/uploads/") && !kb.includes("/uploads/"), + "must not fall back to the colliding 'uploads' namespace", + ); +}); + +test("same content yields a stable key (idempotent re-upload, no churn)", async () => { + const bytes = new Uint8Array([7, 7, 7, 9, 9, 9, 1, 2]); + const a1 = new File([bytes], "clip.mp4", { type: "video/mp4" }); + const a2 = new File([bytes], "clip.mp4", { type: "video/mp4" }); + + assert.equal( + await keyFor(a1), + await keyFor(a2), + "identical content (even as separate File objects) must map to one key", + ); +}); + +test("large files use a deterministic sampled fingerprint without buffering the whole file", async () => { + // Fake a >512MB file: slice() returns only small edge blobs, so nothing close + // to the reported size is ever allocated. Proves the fingerprint branch is + // both bounded-memory and deterministic. + const makeBig = (headByte, tailByte) => { + let bytesRead = 0; + return { + name: "big.mp4", + size: 600 * 1024 * 1024, + lastModified: 111, + slice(start) { + const byte = start === 0 ? headByte : tailByte; + const chunk = new Uint8Array([byte, byte, byte]); + bytesRead += chunk.length; + // Guard against accidental whole-file reads in the implementation. + assert.ok(bytesRead < 1024 * 1024, "must only sample edges, not buffer the file"); + return new Blob([chunk]); + }, + }; + }; + + const k1 = await keyFor(makeBig(1, 2)); + const k1dup = await keyFor(makeBig(1, 2)); + const k2 = await keyFor(makeBig(1, 9)); // differing tail sample + + assert.equal(k1, k1dup, "same metadata + edge samples => stable fingerprint"); + assert.notEqual(k1, k2, "differing content edges => distinct key"); + assert.ok(!k1.includes("/uploads/"), "fingerprint path must not collide on 'uploads'"); +});