From 76401da543a363f6e3aa61bd9cdd92d81eff3cb3 Mon Sep 17 00:00:00 2001 From: kptdobe Date: Fri, 29 May 2026 10:04:39 +0200 Subject: [PATCH 1/3] fix(version): create labelled versions for non-html/json content types POST /versionsource returned a silent 500 for legacy imports whose source object was stored with ContentType: application/octet-stream. shouldCreateVersion gates only text/html and application/json, so even when an explicit label was supplied the version write was skipped and postObjectVersionWithLabel returned { error: 'Version was not created' }. The diagnostic added in #284 confirmed 9 occurrences/24h with an identical fingerprint (octet-stream, hadLabel, currentStatus=200). When the caller passes an explicit label, treat the version as requested-by-name and create it regardless of contentType. Auto-version on plain PUT still gates to html/json, so storage cost is bounded to the labelled call rate. Refs: COR-55, COR-46 Co-Authored-By: Paperclip --- src/storage/version/put.js | 5 +- test/storage/version/put.test.js | 119 +++++++++++++++++++++---------- 2 files changed, 86 insertions(+), 38 deletions(-) diff --git a/src/storage/version/put.js b/src/storage/version/put.js index 2b432844..b94607a4 100644 --- a/src/storage/version/put.js +++ b/src/storage/version/put.js @@ -112,9 +112,10 @@ export async function putObjectWithVersion( return { status: 409, metadata: { id: ID } }; } - // Only create versions for HTML and JSON files const contentType = update.type || current.contentType; - const createVersion = shouldCreateVersion(contentType); + // For named versions (POST /versionsource) we always create, even when the source + // object lacks a versionable contentType (legacy imports stored as octet-stream). + const createVersion = shouldCreateVersion(contentType) || update.label != null; const Version = current.metadata?.version || crypto.randomUUID(); const Users = JSON.stringify(getUsersForMetadata(daCtx.users)); diff --git a/test/storage/version/put.test.js b/test/storage/version/put.test.js index df42459b..18612c0e 100644 --- a/test/storage/version/put.test.js +++ b/test/storage/version/put.test.js @@ -3162,28 +3162,35 @@ describe('Version Put', () => { assert.strictEqual(resp.status, 201, 'must return 201 when version already exists (concurrent 412)'); }); - it('logs diagnostics when versionCreated is false (silent 500 path)', async () => { - // Regression: legacy HTML imports can land in storage with no S3 - // ContentType metadata. shouldCreateVersion(undefined) returns false -> - // shouldCreateVersionObject is false -> putObjectWithVersion returns - // { status: 200, versionCreated: false } -> postObjectVersionWithLabel - // returns a silent 500 with empty Cloudflare Logs (no console.error). - // The fix adds a single diagnostic log capturing contentType / hadLabel / - // currentStatus so the next daily review can confirm root cause. + it('returns 201 when source contentType is application/octet-stream and a label is provided (COR-55)', async () => { + // Regression for COR-55: legacy imports with missing/octet-stream ContentType metadata + // returned a silent 500 because shouldCreateVersion gated out non-html/json. When the + // caller passes an explicit label (POST /versionsource), the version MUST be created + // regardless of contentType, and the audit entry MUST be written. + const versionWrites = []; + const auditCalls = []; + const mockGetObject = async () => ({ - body: 'doc content', - contentType: undefined, - contentLength: 200, + body: 'binary content', + contentType: 'application/octet-stream', + contentLength: 14, status: 200, - metadata: { id: 'doc-id', version: 'v1' }, + metadata: { + id: 'octet-id', version: 'v1', timestamp: '123', users: '[]', path: '/legacy/file', + }, etag: '"etag1"', }); - const mainClient = { - async send() { return { $metadata: { httpStatusCode: 200 } }; }, - }; const versionClient = { - async send() { return { $metadata: { httpStatusCode: 200 } }; }, + async send(cmd) { + versionWrites.push(cmd.input); + return { $metadata: { httpStatusCode: 200 } }; + }, + }; + const mainClient = { + async send() { + return { $metadata: { httpStatusCode: 200 } }; + }, }; const { postObjectVersionWithLabel } = await esmock('../../../src/storage/version/put.js', { @@ -3193,35 +3200,75 @@ describe('Version Put', () => { ifMatch: () => mainClient, }, '../../../src/storage/version/audit.js': { - writeAuditEntry: async () => ({ status: 200 }), + writeAuditEntry: async (env, ctx, repo, fileId, entry) => { + auditCalls.push({ repo, fileId, entry }); + }, }, '../../../src/storage/utils/config.js': { default: () => ({}) }, }); const daCtx = { - bucket: 'b', org: 'o', key: 'doc.html', ext: 'html', users: [], + bucket: 'b', org: 'o', site: 'mysite', key: 'mysite/legacy/file', ext: 'dat', users: [], }; + const resp = await postObjectVersionWithLabel('Pre-import snapshot', {}, daCtx); + + assert.strictEqual(resp.status, 201, 'labelled version must succeed for octet-stream content'); + assert.strictEqual(versionWrites.length, 1, 'version snapshot must be written even for octet-stream'); + assert.strictEqual(versionWrites[0].Metadata.Label, 'Pre-import snapshot'); + assert.strictEqual(auditCalls.length, 1, 'audit entry must be written for labelled non-html/json version'); + assert.strictEqual(auditCalls[0].entry.versionLabel, 'Pre-import snapshot'); + assert.ok(auditCalls[0].entry.versionId, 'audit entry must include versionId for labelled version'); + }); - const errors = []; - const origError = console.error; - console.error = (...args) => { - errors.push(args); + it('plain PUT (no label) still skips auto-version for non-html/json contentType', async () => { + // Companion regression to confirm the COR-55 fix only widens the LABELED path. + // Auto-versioning on plain PUT must still gate to html/json (no extra storage cost). + const versionWrites = []; + const mainWrites = []; + + const mockGetObject = async () => ({ + body: 'binary content', + contentType: 'application/octet-stream', + contentLength: 14, + status: 200, + metadata: { + id: 'octet-id', version: 'v1', timestamp: '123', users: '[]', path: '/legacy/file', + }, + etag: '"etag1"', + }); + + const versionClient = { + async send(cmd) { + versionWrites.push(cmd.input); + return { $metadata: { httpStatusCode: 200 } }; + }, + }; + const mainClient = { + async send(cmd) { + mainWrites.push(cmd.input); + return { $metadata: { httpStatusCode: 200 } }; + }, }; - let resp; - try { - resp = await postObjectVersionWithLabel('My Label', {}, daCtx); - } finally { - console.error = origError; - } - assert.strictEqual(resp.status, 500); - assert.strictEqual(resp.error, 'Version was not created'); - assert(errors.length > 0, 'silent-500 path must emit a console.error so Cloudflare Logs is non-empty'); - const payload = errors[0].find((a) => a && typeof a === 'object'); - assert(payload, 'log must include a structured diagnostic payload'); - assert.strictEqual(payload.contentType, undefined, 'payload must record contentType (undefined when source metadata is missing)'); - assert.strictEqual(payload.hadLabel, true, 'payload must record whether a label was supplied'); - assert.strictEqual(payload.currentStatus, 200, 'payload must record the source-object status'); + const { putObjectWithVersion } = await esmock('../../../src/storage/version/put.js', { + '../../../src/storage/object/get.js': { default: mockGetObject }, + '../../../src/storage/utils/version.js': { + ifNoneMatch: () => versionClient, + ifMatch: () => mainClient, + }, + }); + + const daCtx = { + org: 'o', ext: 'bin', site: 'mysite', users: [{ email: 'u@x.com' }], + }; + const update = { + org: 'o', key: 'mysite/legacy/file', body: 'new', type: 'application/octet-stream', + }; + const resp = await putObjectWithVersion({}, daCtx, update, true); + + assert.strictEqual(resp.status, 200); + assert.strictEqual(versionWrites.length, 0, 'no version snapshot for plain octet-stream PUT without label'); + assert.strictEqual(mainWrites.length, 1, 'main object updated once'); }); it('returns 201 when version client body is a ReadableStream that would be disturbed by SDK retry', async () => { From fc74b2867563817ffe5a5e65d2ed3e7c9a83901b Mon Sep 17 00:00:00 2001 From: kptdobe Date: Fri, 29 May 2026 10:22:16 +0200 Subject: [PATCH 2/3] test(version): drop ticket-id references from test names and comments Project convention: ticket IDs belong in commit messages and PR descriptions, not source code (they rot as tickets are renumbered or deleted). Co-Authored-By: Paperclip --- test/storage/version/put.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/storage/version/put.test.js b/test/storage/version/put.test.js index 18612c0e..4b99c036 100644 --- a/test/storage/version/put.test.js +++ b/test/storage/version/put.test.js @@ -3162,8 +3162,8 @@ describe('Version Put', () => { assert.strictEqual(resp.status, 201, 'must return 201 when version already exists (concurrent 412)'); }); - it('returns 201 when source contentType is application/octet-stream and a label is provided (COR-55)', async () => { - // Regression for COR-55: legacy imports with missing/octet-stream ContentType metadata + it('returns 201 when source contentType is application/octet-stream and a label is provided', async () => { + // Regression: legacy imports with missing/octet-stream ContentType metadata // returned a silent 500 because shouldCreateVersion gated out non-html/json. When the // caller passes an explicit label (POST /versionsource), the version MUST be created // regardless of contentType, and the audit entry MUST be written. @@ -3221,7 +3221,7 @@ describe('Version Put', () => { }); it('plain PUT (no label) still skips auto-version for non-html/json contentType', async () => { - // Companion regression to confirm the COR-55 fix only widens the LABELED path. + // Companion regression to confirm the fix only widens the LABELED path. // Auto-versioning on plain PUT must still gate to html/json (no extra storage cost). const versionWrites = []; const mainWrites = []; From bbded36020e897869a8dbd828d34e34636eb3d59 Mon Sep 17 00:00:00 2001 From: kptdobe Date: Fri, 29 May 2026 10:50:17 +0200 Subject: [PATCH 3/3] fix(version): infer ContentType from extension instead of widening the gate Pivot from the gate-widening approach (createVersion || label != null) to healing the underlying metadata on the labelled-version path. postObjectVersionWithLabel now derives a versionable mime from daCtx.ext when the stored ContentType is missing or application/octet-stream: html -> text/html json -> application/json The inferred type is passed via update.type. shouldCreateVersion sees the healed type, the version snapshot stores ContentType: text/html (or json), and the main object's PUT overwrites the stale ContentType in S3 metadata so the file is self-healed for all future requests. Binary files (jpg/pdf/etc.) still cannot be labelled-versioned, matching the project's "binaries do not version" semantics. The diagnostic log from #284 is retained and extended with inferredType + ext, so the unhealed path is observable. Tests updated: - new: legacy octet-stream HTML labelled version heals snapshot + main - new: labelled version on non-versionable ext still 500s with diagnostic - companion: plain PUT auto-version gate intact (no leak from labelled path) Co-Authored-By: Paperclip --- src/storage/version/put.js | 26 +++++++-- test/storage/version/put.test.js | 92 ++++++++++++++++++++++++++------ 2 files changed, 97 insertions(+), 21 deletions(-) diff --git a/src/storage/version/put.js b/src/storage/version/put.js index b94607a4..a4716a3a 100644 --- a/src/storage/version/put.js +++ b/src/storage/version/put.js @@ -78,6 +78,16 @@ function shouldCreateVersion(contentType) { return type.startsWith('text/html') || type.startsWith('application/json'); } +// Infer a versionable contentType from the file extension when the stored +// ContentType is missing or octet-stream (legacy imports). Returns the original +// contentType when it is already versionable, or when we cannot map the extension. +function inferVersionableType(contentType, ext) { + if (shouldCreateVersion(contentType)) return contentType; + if (ext === 'html') return 'text/html'; + if (ext === 'json') return 'application/json'; + return contentType; +} + function buildInput({ bucket, org, key, body, type, contentLength, }) { @@ -112,10 +122,9 @@ export async function putObjectWithVersion( return { status: 409, metadata: { id: ID } }; } + // Only create versions for HTML and JSON files const contentType = update.type || current.contentType; - // For named versions (POST /versionsource) we always create, even when the source - // object lacks a versionable contentType (legacy imports stored as octet-stream). - const createVersion = shouldCreateVersion(contentType) || update.label != null; + const createVersion = shouldCreateVersion(contentType); const Version = current.metadata?.version || crypto.randomUUID(); const Users = JSON.stringify(getUsersForMetadata(daCtx.users)); @@ -226,7 +235,7 @@ export async function putObjectWithVersion( Repo: daCtx.site || undefined, Body: (body || storeBody ? current.body : ''), ContentLength: (body || storeBody ? current.contentLength : undefined), - ContentType: current.contentType, + ContentType: contentType, ID, Version, Ext: daCtx.ext, @@ -323,8 +332,13 @@ export async function postObjectVersionWithLabel(label, env, daCtx) { const bodyBuffer = body instanceof ReadableStream ? await new Response(body).arrayBuffer() : body; const { bucket, org, key } = daCtx; + // Legacy imports may have lost their ContentType metadata (stored as + // application/octet-stream). Recover the correct mime from the file + // extension so the version write + main-object PUT both heal. + const inferredType = inferVersionableType(contentType, daCtx.ext); + const resp = await putObjectWithVersion(env, daCtx, { - bucket, org, key, body: bodyBuffer, contentLength, type: contentType, label, + bucket, org, key, body: bodyBuffer, contentLength, type: inferredType, label, }, true); if (resp.status !== 200) return { status: resp.status }; @@ -335,6 +349,8 @@ export async function postObjectVersionWithLabel(label, env, daCtx) { // eslint-disable-next-line no-console console.error('Failed to version (no version created)', { contentType, + inferredType, + ext: daCtx.ext, hadLabel: label != null, currentStatus, }); diff --git a/test/storage/version/put.test.js b/test/storage/version/put.test.js index 4b99c036..0e675333 100644 --- a/test/storage/version/put.test.js +++ b/test/storage/version/put.test.js @@ -3162,21 +3162,22 @@ describe('Version Put', () => { assert.strictEqual(resp.status, 201, 'must return 201 when version already exists (concurrent 412)'); }); - it('returns 201 when source contentType is application/octet-stream and a label is provided', async () => { - // Regression: legacy imports with missing/octet-stream ContentType metadata - // returned a silent 500 because shouldCreateVersion gated out non-html/json. When the - // caller passes an explicit label (POST /versionsource), the version MUST be created - // regardless of contentType, and the audit entry MUST be written. + it('heals legacy octet-stream HTML on labelled version: snapshot + main object both repaired', async () => { + // Regression: legacy imports with missing or octet-stream ContentType could not be + // labelled-versioned because shouldCreateVersion gated out non-html/json. Recover the + // correct mime from the file extension so the version snapshot AND the main-object PUT + // both heal to text/html. const versionWrites = []; + const mainWrites = []; const auditCalls = []; const mockGetObject = async () => ({ - body: 'binary content', + body: 'legacy', contentType: 'application/octet-stream', - contentLength: 14, + contentLength: 18, status: 200, metadata: { - id: 'octet-id', version: 'v1', timestamp: '123', users: '[]', path: '/legacy/file', + id: 'legacy-id', version: 'v1', timestamp: '123', users: '[]', path: '/mysite/legacy.html', }, etag: '"etag1"', }); @@ -3188,7 +3189,8 @@ describe('Version Put', () => { }, }; const mainClient = { - async send() { + async send(cmd) { + mainWrites.push(cmd.input); return { $metadata: { httpStatusCode: 200 } }; }, }; @@ -3208,21 +3210,79 @@ describe('Version Put', () => { }); const daCtx = { - bucket: 'b', org: 'o', site: 'mysite', key: 'mysite/legacy/file', ext: 'dat', users: [], + bucket: 'b', org: 'o', site: 'mysite', key: 'mysite/legacy.html', ext: 'html', users: [], }; const resp = await postObjectVersionWithLabel('Pre-import snapshot', {}, daCtx); - assert.strictEqual(resp.status, 201, 'labelled version must succeed for octet-stream content'); - assert.strictEqual(versionWrites.length, 1, 'version snapshot must be written even for octet-stream'); + assert.strictEqual(resp.status, 201); + assert.strictEqual(versionWrites.length, 1, 'version snapshot must be written'); + assert.strictEqual(versionWrites[0].ContentType, 'text/html', 'snapshot ContentType must heal to text/html'); assert.strictEqual(versionWrites[0].Metadata.Label, 'Pre-import snapshot'); - assert.strictEqual(auditCalls.length, 1, 'audit entry must be written for labelled non-html/json version'); + assert.strictEqual(mainWrites.length, 1, 'main object PUT must run'); + assert.strictEqual(mainWrites[0].ContentType, 'text/html', 'main object ContentType must heal in storage'); + assert.strictEqual(auditCalls.length, 1, 'audit entry must be written'); assert.strictEqual(auditCalls[0].entry.versionLabel, 'Pre-import snapshot'); - assert.ok(auditCalls[0].entry.versionId, 'audit entry must include versionId for labelled version'); + assert.ok(auditCalls[0].entry.versionId); }); + it('logs diagnostics and returns 500 when labelled version requested on non-versionable ext', async () => { + // Preserves the binary-never-version semantics: when the file extension does not map + // to a versionable mime (and the stored contentType is also not versionable), the + // labelled-version request still 500s and the diagnostic log fires with inferredType + ext + // captured so we can spot future legacy patterns in Cloudflare Logs. + const mockGetObject = async () => ({ + body: 'binary content', + contentType: 'application/octet-stream', + contentLength: 14, + status: 200, + metadata: { id: 'bin-id', version: 'v1' }, + etag: '"etag1"', + }); + + const s3Client = { + async send() { return { $metadata: { httpStatusCode: 200 } }; }, + }; + + const { postObjectVersionWithLabel } = await esmock('../../../src/storage/version/put.js', { + '../../../src/storage/object/get.js': { default: mockGetObject }, + '../../../src/storage/utils/version.js': { + ifNoneMatch: () => s3Client, + ifMatch: () => s3Client, + }, + '../../../src/storage/version/audit.js': { writeAuditEntry: async () => ({ status: 200 }) }, + '../../../src/storage/utils/config.js': { default: () => ({}) }, + }); + + const daCtx = { + bucket: 'b', org: 'o', site: 'mysite', key: 'mysite/data.bin', ext: 'bin', users: [], + }; + + const errors = []; + const origError = console.error; + console.error = (...args) => { + errors.push(args); + }; + let resp; + try { + resp = await postObjectVersionWithLabel('My Label', {}, daCtx); + } finally { + console.error = origError; + } + + assert.strictEqual(resp.status, 500); + assert.strictEqual(resp.error, 'Version was not created'); + assert(errors.length > 0, 'diagnostic log must fire for the unhealed octet-stream path'); + const payload = errors[0].find((a) => a && typeof a === 'object'); + assert(payload, 'log must include a structured diagnostic payload'); + assert.strictEqual(payload.contentType, 'application/octet-stream'); + assert.strictEqual(payload.inferredType, 'application/octet-stream', 'inference must fall through for unknown ext'); + assert.strictEqual(payload.ext, 'bin'); + assert.strictEqual(payload.hadLabel, true); + assert.strictEqual(payload.currentStatus, 200); + }); it('plain PUT (no label) still skips auto-version for non-html/json contentType', async () => { - // Companion regression to confirm the fix only widens the LABELED path. - // Auto-versioning on plain PUT must still gate to html/json (no extra storage cost). + // Companion regression: the labelled-path mime inference must NOT bleed into plain + // PUTs. Auto-versioning on plain PUT must still gate to html/json (no extra storage cost). const versionWrites = []; const mainWrites = [];