From e33249d849977732a3e98690c2d0f692939456ea Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Thu, 16 Jul 2026 09:50:18 +0200 Subject: [PATCH 01/11] test(copy-move): reproduce .da-versions destination forge The generic /copy and /move routes read their destination from the request body. The finding-16 router guard runs on daCtx.key. That key is the copy/move source, which has no .da-versions segment, so the guard passes. The destination is not checked against the reserved namespace. A writer can plant objects under {repo}/.da-versions/{fileId}/... and forge a document's version history and audit log. These tests drive a .da-versions destination through copyHandler, moveRoute, and copyFile. Each expects a 400 with no write. They fail against current code: the copy proceeds and returns 200. Companion tests assert a segment that only contains 'da-versions' (my-da-versions-notes) still dispatches, pinning segment-exact matching. --- test/routes/copy.test.js | 57 ++++++++++++++++++++++++++++++++ test/routes/move.test.js | 57 ++++++++++++++++++++++++++++++++ test/storage/object/copy.test.js | 39 ++++++++++++++++++++++ 3 files changed, 153 insertions(+) diff --git a/test/routes/copy.test.js b/test/routes/copy.test.js index 97d54d90..b8a3a4a4 100644 --- a/test/routes/copy.test.js +++ b/test/routes/copy.test.js @@ -181,4 +181,61 @@ describe('Copy Route', () => { assert.strictEqual(400, resp.status); assert.strictEqual(copyCalled.length, 0); }); + + it('Test copyHandler returns 400 when destination is in the reserved .da-versions folder', async () => { + const copyCalled = []; + const copyObject = (e, c, d, m) => { + copyCalled.push({ + e, c, d, m, + }); + return { status: 200 }; + }; + + const copyHandler = await esmock('../../src/routes/copy.js', { + '../../src/storage/object/copy.js': { + default: copyObject, + }, + '../../src/utils/auth.js': { hasPermission: () => true }, + }); + + const formdata = new Map(); + formdata.set('destination', '/myorg/my/.da-versions/1234/audit-9999999999.txt'); + const req = { + formData: () => formdata, + }; + + const resp = await copyHandler({ req, env: {}, daCtx: { org: 'myorg', key: 'my/decoy.html' } }); + assert.strictEqual(resp.status, 400); + assert.strictEqual(copyCalled.length, 0); + const body = JSON.parse(resp.body); + assert.match(body.error, /da-versions/i); + }); + + it('Test copyHandler allows a destination segment that merely contains da-versions', async () => { + const copyCalled = []; + const copyObject = (e, c, d, m) => { + copyCalled.push({ + e, c, d, m, + }); + return { status: 200 }; + }; + + const copyHandler = await esmock('../../src/routes/copy.js', { + '../../src/storage/object/copy.js': { + default: copyObject, + }, + '../../src/utils/auth.js': { hasPermission: () => true }, + }); + + const formdata = new Map(); + formdata.set('destination', '/myorg/my/my-da-versions-notes.html'); + const req = { + formData: () => formdata, + }; + + const resp = await copyHandler({ req, env: {}, daCtx: { org: 'myorg', key: 'my/src.html' } }); + assert.strictEqual(resp.status, 200); + assert.strictEqual(copyCalled.length, 1); + assert.strictEqual(copyCalled[0].d.destination, 'my/my-da-versions-notes.html'); + }); }); diff --git a/test/routes/move.test.js b/test/routes/move.test.js index 5cf8d096..b3666f3c 100644 --- a/test/routes/move.test.js +++ b/test/routes/move.test.js @@ -123,4 +123,61 @@ describe('Move Route', () => { assert.strictEqual(1, moCalled.length); assert.strictEqual('somedest', moCalled[0].d.destination); }); + + it('Test moveRoute returns 400 when destination is in the reserved .da-versions folder', async () => { + const moCalled = []; + const moveObject = (e, c, d) => { + moCalled.push({ e, c, d }); + return { status: 200 }; + }; + + const moveRoute = await esmock('../../src/routes/move.js', { + '../../src/storage/object/move.js': { + default: moveObject, + }, + '../../src/utils/auth.js': { + hasPermission: () => true, + }, + }); + + const formdata = new Map(); + formdata.set('destination', '/someorg/my/.da-versions/1234/audit-9999999999.txt'); + const req = { + formData: () => formdata, + }; + + const resp = await moveRoute({ req, env: {}, daCtx: { org: 'someorg', key: 'my/decoy.html' } }); + assert.strictEqual(resp.status, 400); + assert.strictEqual(0, moCalled.length); + const body = JSON.parse(resp.body); + assert.match(body.error, /da-versions/i); + }); + + it('Test moveRoute allows a destination segment that merely contains da-versions', async () => { + const moCalled = []; + const moveObject = (e, c, d) => { + moCalled.push({ e, c, d }); + return { status: 204 }; + }; + + const moveRoute = await esmock('../../src/routes/move.js', { + '../../src/storage/object/move.js': { + default: moveObject, + }, + '../../src/utils/auth.js': { + hasPermission: () => true, + }, + }); + + const formdata = new Map(); + formdata.set('destination', '/someorg/my/my-da-versions-notes.html'); + const req = { + formData: () => formdata, + }; + + const resp = await moveRoute({ req, env: {}, daCtx: { org: 'someorg', key: 'abc.html' } }); + assert.strictEqual(resp.status, 204); + assert.strictEqual(1, moCalled.length); + assert.strictEqual(moCalled[0].d.destination, 'my/my-da-versions-notes.html'); + }); }); diff --git a/test/storage/object/copy.test.js b/test/storage/object/copy.test.js index 5a73a0a7..4b890da8 100644 --- a/test/storage/object/copy.test.js +++ b/test/storage/object/copy.test.js @@ -89,6 +89,45 @@ describe('Object copy', () => { assert.strictEqual(resp.$metadata.httpStatusCode, 403); }); + it('returns 400 without sending a copy when the destination Key is in the reserved .da-versions folder', async () => { + const s3Sent = []; + const mockS3Client = class { + // eslint-disable-next-line class-methods-use-this + send(command) { + s3Sent.push(command); + return { $metadata: { httpStatusCode: 200 } }; + } + + middlewareStack = { add: () => {} }; + }; + + const mockGetObject = async () => ({ contentType: 'text/html', status: 200 }); + + // eslint-disable-next-line no-shadow + const { copyFile } = await esmock('../../../src/storage/object/copy.js', { + '@aws-sdk/client-s3': { + S3Client: mockS3Client, + }, + '../../../src/storage/object/get.js': { + default: mockGetObject, + }, + '../../../src/utils/auth.js': { hasPermission: () => true }, + }); + + const env = { dacollab: { fetch: () => ({ body: { cancel: () => {} } }) } }; + const daCtx = { + bucket: 'root-bucket', + org: 'myorg', + origin: 'https://test.com', + users: [{ email: 'test@example.com' }], + }; + const details = { source: 'mysrc', destination: 'my/.da-versions/1234' }; + + const resp = await copyFile({}, env, daCtx, 'mysrc/audit-9999999999.txt', details, false); + assert.strictEqual(resp.$metadata.httpStatusCode, 400); + assert.strictEqual(s3Sent.length, 0); + }); + it('Copy to location with permission', async () => { const pathLookup = new Map(); pathLookup.set('aaa@bbb.ccc', [ From 436d05dc07845bf833212ee0292d0cafcd4f8000 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Thu, 16 Jul 2026 09:53:05 +0200 Subject: [PATCH 02/11] fix: reject copy/move destinations in the reserved .da-versions folder Version bodies and audit logs live at the reserved key {org}/{repo}/.da-versions/{fileId}/.... The generic /copy and /move routes read their destination from the request body. They did not check it against that namespace. A writer could copy or move an object into {repo}/.da-versions/.... That plants attacker-controlled bodies where version and audit storage lives. It forges a document's version history and audit log. #301 rejects a different-org destination. It does not reject a same-org .da-versions destination, so the vector was open. The .da-versions router guard added in #302 inspects daCtx.key, which is the copy/move source, not the destination. So it did not cover this either. Add hasReservedSegment to the version paths module. It is true when any segment of a path is .da-versions. copyHelper and moveHelper reject a destination in that folder, next to the existing cross-org check. copyFile applies the same check to each derived Key, so a folder copy cannot slip a reserved key through. The check does not rely on hasPermission, which grants .da-versions writes in no-config and broad-grant orgs. The ACL-aware version routes build the physical key internally. Their daCtx.key does not carry a .da-versions segment, so they are unaffected. A segment that only contains 'da-versions' stays allowed. --- src/helpers/copy.js | 11 +++++++++++ src/helpers/move.js | 11 +++++++++++ src/storage/object/copy.js | 7 +++++++ src/storage/version/paths.js | 11 +++++++++++ 4 files changed, 40 insertions(+) diff --git a/src/helpers/copy.js b/src/helpers/copy.js index d984278f..9d5bbf8b 100644 --- a/src/helpers/copy.js +++ b/src/helpers/copy.js @@ -9,6 +9,8 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ +import { hasReservedSegment } from '../storage/version/paths.js'; + const NO_DEST_ERROR = { body: JSON.stringify({ error: 'No destination provided.' }), status: 400, @@ -24,6 +26,11 @@ const CROSS_ORG_ERROR = { status: 400, }; +const RESERVED_DEST_ERROR = { + body: JSON.stringify({ error: 'Destination cannot be inside the reserved .da-versions folder.' }), + status: 400, +}; + export default async function copyHelper(req, daCtx) { let formData; try { @@ -43,6 +50,10 @@ export default async function copyHelper(req, daCtx) { if (destOrg !== daCtx.org) return { error: CROSS_ORG_ERROR }; const destination = destParts.join('/'); + + // Reject destinations inside the reserved .da-versions folder + if (hasReservedSegment(destination)) return { error: RESERVED_DEST_ERROR }; + const source = daCtx.key; return { source, destination, continuationToken }; } diff --git a/src/helpers/move.js b/src/helpers/move.js index 8f3c6f01..4c598a43 100644 --- a/src/helpers/move.js +++ b/src/helpers/move.js @@ -10,6 +10,8 @@ * governing permissions and limitations under the License. */ +import { hasReservedSegment } from '../storage/version/paths.js'; + const NO_DEST_ERROR = { body: JSON.stringify({ error: 'No destination provided.' }), status: 400, @@ -25,6 +27,11 @@ const CROSS_ORG_ERROR = { status: 400, }; +const RESERVED_DEST_ERROR = { + body: JSON.stringify({ error: 'Destination cannot be inside the reserved .da-versions folder.' }), + status: 400, +}; + export default async function moveHelper(req, daCtx) { try { const formData = await req.formData(); @@ -39,6 +46,10 @@ export default async function moveHelper(req, daCtx) { if (destOrg !== daCtx.org) return { error: CROSS_ORG_ERROR }; let destination = destParts.join('/'); + + // Reject destinations inside the reserved .da-versions folder + if (hasReservedSegment(destination)) return { error: RESERVED_DEST_ERROR }; + const source = daCtx.key; // Ensure destination is not child of source diff --git a/src/storage/object/copy.js b/src/storage/object/copy.js index fb4fc83f..0aa31e28 100644 --- a/src/storage/object/copy.js +++ b/src/storage/object/copy.js @@ -21,12 +21,19 @@ import { putObjectWithVersion } from '../version/put.js'; import { getUsersForMetadata } from '../utils/version.js'; import { listCommand } from '../utils/list.js'; import { hasPermission } from '../../utils/auth.js'; +import { hasReservedSegment } from '../version/paths.js'; const MAX_KEYS = 900; export const copyFile = async (config, env, daCtx, sourceKey, details, isRename) => { const Key = sourceKey.replace(details.source, details.destination); + // A folder copy derives many keys from one request. Never let any of them + // land in the reserved .da-versions folder, whatever the caller's grants. + if (hasReservedSegment(Key)) { + return { $metadata: { httpStatusCode: 400 } }; + } + if (!hasPermission(daCtx, sourceKey, 'read') || !hasPermission(daCtx, Key, 'write')) { return { $metadata: { diff --git a/src/storage/version/paths.js b/src/storage/version/paths.js index fd30d191..10ef161d 100644 --- a/src/storage/version/paths.js +++ b/src/storage/version/paths.js @@ -91,3 +91,14 @@ export function auditArchiveKey(repo, fileId, timestamp) { export function auditDirPrefix(repo, fileId) { return `${repo}/.da-versions/${fileId}/audit`; } + +/** + * True when any path segment is the reserved .da-versions folder. Keeps + * user-controlled destinations and keys out of version storage, which is + * only reachable through the ACL-aware version routes. + * @param {string} path org-stripped key or copy/move destination + * @returns {boolean} + */ +export function hasReservedSegment(path) { + return typeof path === 'string' && path.split('/').includes('.da-versions'); +} From c5193be74c2ce7035a069a426c7a85fc820b15c3 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Thu, 16 Jul 2026 09:58:01 +0200 Subject: [PATCH 03/11] test(it): assert copy and move are blocked from the .da-versions folder Adds an integration assertion after the cross-org tests. A copy and a move whose destination is inside {repo}/.da-versions/... both return 400 from the destination guard. The blocked move leaves its source in place. Against pre-fix code the copy returns 204 and plants the object in version storage. --- test/it/it-tests.js | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/it/it-tests.js b/test/it/it-tests.js index 5dd71401..6a156fc5 100644 --- a/test/it/it-tests.js +++ b/test/it/it-tests.js @@ -613,6 +613,44 @@ export default (ctx) => describe('Integration Tests: it tests', function () { assert.strictEqual(resp.status, 200, `Expected 200 OK, got ${resp.status} - user: ${superUser.email}`); }); + it('[super user] cannot copy or move into the reserved .da-versions folder', async () => { + // The generic copy/move routes take their destination from the request body. + // A destination inside {repo}/.da-versions/... lands in version and audit + // storage and forges a document's history. The destination guard must reject + // it with 400, whatever the caller's grants. + const { + serverUrl, org, repo, superUser, + } = ctx; + + const copyForm = new FormData(); + copyForm.append('destination', `/${org}/${repo}/.da-versions/forge-target/audit-9999999999.txt`); + let resp = await fetch(`${serverUrl}/copy/${org}/${repo}/test-folder/page1.html`, { + method: 'POST', + body: copyForm, + headers: { Authorization: `Bearer ${superUser.accessToken}` }, + }); + assert.strictEqual(resp.status, 400, `Expected 400 from the destination guard on copy, got ${resp.status} - user: ${superUser.email}`); + let body = await resp.json(); + assert.match(body.error, /da-versions/i, `Expected reserved-folder error, got ${body.error}`); + + const moveForm = new FormData(); + moveForm.append('destination', `/${org}/${repo}/.da-versions/forge-target/0000.html`); + resp = await fetch(`${serverUrl}/move/${org}/${repo}/test-folder/page1-copy.html`, { + method: 'POST', + body: moveForm, + headers: { Authorization: `Bearer ${superUser.accessToken}` }, + }); + assert.strictEqual(resp.status, 400, `Expected 400 from the destination guard on move, got ${resp.status} - user: ${superUser.email}`); + body = await resp.json(); + assert.match(body.error, /da-versions/i, `Expected reserved-folder error, got ${body.error}`); + + // the blocked move must leave the source in place + resp = await fetch(`${serverUrl}/source/${org}/${repo}/test-folder/page1-copy.html`, { + headers: { Authorization: `Bearer ${superUser.accessToken}` }, + }); + assert.strictEqual(resp.status, 200, `Expected 200 OK, got ${resp.status} - user: ${superUser.email}`); + }); + it('[anonymous] cannot delete an object', async () => { const { serverUrl, org, repo, key, From f88b87455890b104d92fe28b6e7e95fe380d1d90 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Thu, 16 Jul 2026 10:36:41 +0200 Subject: [PATCH 04/11] test(copy): reproduce .da-versions carry-along over-block on repo rename A repo-level move lists the repo's own version storage under {repo}/.da-versions/.... copyFile must carry those keys to the new repo. The broad reserved-Key guard rejects them with 400, so moveObject skips the delete and strands the history at the old repo name. This test drives copyFile with a source already under .da-versions and expects the copy to proceed. It fails against the broad guard: it returns 400 and sends nothing. --- test/storage/object/copy.test.js | 42 ++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/storage/object/copy.test.js b/test/storage/object/copy.test.js index 4b890da8..60f7c8c3 100644 --- a/test/storage/object/copy.test.js +++ b/test/storage/object/copy.test.js @@ -128,6 +128,48 @@ describe('Object copy', () => { assert.strictEqual(s3Sent.length, 0); }); + it('carries a source-side .da-versions object along on a repo-level rename', async () => { + // A repo-level move/copy lists the repo's own version storage. copyFile must + // carry those keys along, not reject them. The reserved guard only blocks a + // destination that introduces .da-versions, not a source that already has it. + const s3Sent = []; + const mockS3Client = class { + // eslint-disable-next-line class-methods-use-this + send(command) { + s3Sent.push(command); + return { $metadata: { httpStatusCode: 200 } }; + } + + middlewareStack = { add: () => {} }; + }; + + const mockGetObject = async () => ({ contentType: 'text/html', status: 200 }); + + // eslint-disable-next-line no-shadow + const { copyFile } = await esmock('../../../src/storage/object/copy.js', { + '@aws-sdk/client-s3': { + S3Client: mockS3Client, + }, + '../../../src/storage/object/get.js': { + default: mockGetObject, + }, + '../../../src/utils/auth.js': { hasPermission: () => true }, + }); + + const env = { dacollab: { fetch: () => ({ body: { cancel: () => {} } }) } }; + const daCtx = { + bucket: 'root-bucket', + org: 'myorg', + origin: 'https://test.com', + users: [{ email: 'test@example.com' }], + }; + const details = { source: 'oldrepo', destination: 'newrepo' }; + + const resp = await copyFile({}, env, daCtx, 'oldrepo/.da-versions/fid1/v1.html', details, true); + assert.notStrictEqual(resp?.$metadata?.httpStatusCode, 400, 'carry-along must not be blocked'); + assert.strictEqual(s3Sent.length, 1, 'the version object must be copied to the new repo'); + }); + it('Copy to location with permission', async () => { const pathLookup = new Map(); pathLookup.set('aaa@bbb.ccc', [ From 34e90d173c8370a1e22bb6fd75d14563005ff2d9 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Thu, 16 Jul 2026 10:37:26 +0200 Subject: [PATCH 05/11] fix: allow repo-level copy/move to carry .da-versions along The copyFile reserved-key guard rejected every derived key under .da-versions. A repo-level move lists the repo's own version storage, so the guard 400-skipped those keys and moveObject left them at the old repo name, stranding the version history. Reject only when the destination introduces the segment: a source that is already under .da-versions carries along. The forge is still blocked, because its source is ordinary content and the destination is what adds .da-versions. --- src/storage/object/copy.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/storage/object/copy.js b/src/storage/object/copy.js index 0aa31e28..04fc5ab9 100644 --- a/src/storage/object/copy.js +++ b/src/storage/object/copy.js @@ -28,9 +28,11 @@ const MAX_KEYS = 900; export const copyFile = async (config, env, daCtx, sourceKey, details, isRename) => { const Key = sourceKey.replace(details.source, details.destination); - // A folder copy derives many keys from one request. Never let any of them - // land in the reserved .da-versions folder, whatever the caller's grants. - if (hasReservedSegment(Key)) { + // Block a destination that pushes a key into the reserved .da-versions folder, + // whatever the caller's grants. A repo-level copy or move lists the repo's own + // version storage, so allow keys whose source is already under .da-versions to + // carry along; only reject when the destination is what introduces the segment. + if (!hasReservedSegment(sourceKey) && hasReservedSegment(Key)) { return { $metadata: { httpStatusCode: 400 } }; } From 020af630aa4445b2169f03a554c069d45fbfeac4 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Thu, 16 Jul 2026 10:38:16 +0200 Subject: [PATCH 06/11] test(paths): lock hasReservedSegment segment-exact matching hasReservedSegment is the shared reserved-name check for the copy/move guard. It had no direct unit test, so a substring form would pass the suite. Lock the contract: it matches .da-versions as a whole path segment, ignores a segment that only contains the name (my-da-versions-notes, .da-versions-backup), and is safe for non-string input. --- test/storage/version/paths.test.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/storage/version/paths.test.js b/test/storage/version/paths.test.js index d7dd6075..00ce068d 100644 --- a/test/storage/version/paths.test.js +++ b/test/storage/version/paths.test.js @@ -17,6 +17,7 @@ import { auditDirPrefix, isValidId, isSafeId, + hasReservedSegment, } from '../../../src/storage/version/paths.js'; describe('Version Paths', () => { @@ -136,4 +137,24 @@ describe('Version Paths', () => { assert.strictEqual(isSafeId(null), false); }); }); + + describe('hasReservedSegment', () => { + it('matches the reserved folder as any path segment', () => { + assert.strictEqual(hasReservedSegment('repo/.da-versions/fid/audit.txt'), true); + assert.strictEqual(hasReservedSegment('.da-versions/fid/v1.html'), true); + assert.strictEqual(hasReservedSegment('a/b/.da-versions'), true); + }); + + it('does not match a segment that merely contains the name', () => { + assert.strictEqual(hasReservedSegment('repo/my-da-versions-notes.html'), false); + assert.strictEqual(hasReservedSegment('repo/.da-versions-backup/x'), false); + assert.strictEqual(hasReservedSegment('repo/foo.da-versions'), false); + assert.strictEqual(hasReservedSegment('repo/page1.html'), false); + }); + + it('is safe for non-string input', () => { + assert.strictEqual(hasReservedSegment(undefined), false); + assert.strictEqual(hasReservedSegment(null), false); + }); + }); }); From 9e15d3ffaa836af7c02df73b0f300bfbb78be4c6 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Thu, 16 Jul 2026 10:47:55 +0200 Subject: [PATCH 07/11] test(copy): require .da-versions destination block with no source exemption The refined guard skipped the check when the source was already under .da-versions, so a repo-level copy or move could relocate a .da-versions object into another .da-versions location. Chained with a separate write hole that plants crafted content in some .da-versions, that reopens the forge. This test drives copyFile with a source under .da-versions and expects 400 with no send. It fails against the exemption: it returns 200 and sends the copy. --- test/storage/object/copy.test.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/storage/object/copy.test.js b/test/storage/object/copy.test.js index 60f7c8c3..d755b408 100644 --- a/test/storage/object/copy.test.js +++ b/test/storage/object/copy.test.js @@ -128,10 +128,12 @@ describe('Object copy', () => { assert.strictEqual(s3Sent.length, 0); }); - it('carries a source-side .da-versions object along on a repo-level rename', async () => { - // A repo-level move/copy lists the repo's own version storage. copyFile must - // carry those keys along, not reject them. The reserved guard only blocks a - // destination that introduces .da-versions, not a source that already has it. + it('blocks a .da-versions destination even when the source is already under .da-versions', async () => { + // No source-side exemption. A repo-level copy or move must not relocate a + // .da-versions object into another .da-versions location. Otherwise an + // attacker who gets crafted content into any .da-versions (through a separate + // write hole) could plant it in a victim's version history. The guard rejects + // the reserved destination whatever the source looks like. const s3Sent = []; const mockS3Client = class { // eslint-disable-next-line class-methods-use-this @@ -166,8 +168,8 @@ describe('Object copy', () => { const details = { source: 'oldrepo', destination: 'newrepo' }; const resp = await copyFile({}, env, daCtx, 'oldrepo/.da-versions/fid1/v1.html', details, true); - assert.notStrictEqual(resp?.$metadata?.httpStatusCode, 400, 'carry-along must not be blocked'); - assert.strictEqual(s3Sent.length, 1, 'the version object must be copied to the new repo'); + assert.strictEqual(resp.$metadata.httpStatusCode, 400); + assert.strictEqual(s3Sent.length, 0); }); it('Copy to location with permission', async () => { From 8bfca4d4cc1b4e8768622aff07db1e4ae9e042ff Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Thu, 16 Jul 2026 10:48:59 +0200 Subject: [PATCH 08/11] fix: block all .da-versions copy/move destinations without exemption An earlier revision exempted a source already under .da-versions. That let a repo-level rename carry its version storage along. The exemption is a hole. An attacker who plants crafted content in any .da-versions object could relocate it into a victim's version history through a repo-level copy or move. Drop the exemption: reject every derived key under .da-versions. Documented copy and move act on a path below the repo. That path does not list the repo-root .da-versions folder, so this changes nothing for them. Only an undocumented repo-root move or copy reaches version storage, and it no longer carries or duplicates it. --- src/storage/object/copy.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/storage/object/copy.js b/src/storage/object/copy.js index 04fc5ab9..322bfa24 100644 --- a/src/storage/object/copy.js +++ b/src/storage/object/copy.js @@ -28,11 +28,11 @@ const MAX_KEYS = 900; export const copyFile = async (config, env, daCtx, sourceKey, details, isRename) => { const Key = sourceKey.replace(details.source, details.destination); - // Block a destination that pushes a key into the reserved .da-versions folder, - // whatever the caller's grants. A repo-level copy or move lists the repo's own - // version storage, so allow keys whose source is already under .da-versions to - // carry along; only reject when the destination is what introduces the segment. - if (!hasReservedSegment(sourceKey) && hasReservedSegment(Key)) { + // A folder copy derives many keys from one request. Reject any that lands in + // the reserved .da-versions folder, whatever the caller's grants and whatever + // the source looks like. Version storage is only reachable through the + // ACL-aware version routes, so no copy or move may write into it. + if (hasReservedSegment(Key)) { return { $metadata: { httpStatusCode: 400 } }; } From d887a3c230104766f486896de0fe9efa4f41d937 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Fri, 17 Jul 2026 13:00:56 +0200 Subject: [PATCH 09/11] Update src/helpers/move.js Co-authored-by: Alexandre Capt --- src/helpers/move.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers/move.js b/src/helpers/move.js index 4c598a43..ff7ae004 100644 --- a/src/helpers/move.js +++ b/src/helpers/move.js @@ -28,7 +28,7 @@ const CROSS_ORG_ERROR = { }; const RESERVED_DEST_ERROR = { - body: JSON.stringify({ error: 'Destination cannot be inside the reserved .da-versions folder.' }), + body: JSON.stringify({ error: 'Invalid or reserved destination.' }), status: 400, }; From ef661815d2d1707fb695d7835fcb7c15d5b4cedf Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Fri, 17 Jul 2026 13:01:27 +0200 Subject: [PATCH 10/11] Update src/helpers/copy.js Co-authored-by: Alexandre Capt --- src/helpers/copy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers/copy.js b/src/helpers/copy.js index 9d5bbf8b..13d8ab8c 100644 --- a/src/helpers/copy.js +++ b/src/helpers/copy.js @@ -27,7 +27,7 @@ const CROSS_ORG_ERROR = { }; const RESERVED_DEST_ERROR = { - body: JSON.stringify({ error: 'Destination cannot be inside the reserved .da-versions folder.' }), + body: JSON.stringify({ error: 'Invalid or reserved destination.' }), status: 400, }; From d6ec91fe1d56641ba7b4649f09081c96d386d48d Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Fri, 17 Jul 2026 13:49:03 +0200 Subject: [PATCH 11/11] test: match the generic reserved-destination error message --- test/it/it-tests.js | 4 ++-- test/routes/copy.test.js | 2 +- test/routes/move.test.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/it/it-tests.js b/test/it/it-tests.js index 6a156fc5..f5eda1a8 100644 --- a/test/it/it-tests.js +++ b/test/it/it-tests.js @@ -631,7 +631,7 @@ export default (ctx) => describe('Integration Tests: it tests', function () { }); assert.strictEqual(resp.status, 400, `Expected 400 from the destination guard on copy, got ${resp.status} - user: ${superUser.email}`); let body = await resp.json(); - assert.match(body.error, /da-versions/i, `Expected reserved-folder error, got ${body.error}`); + assert.match(body.error, /invalid or reserved/i, `Expected reserved-destination error, got ${body.error}`); const moveForm = new FormData(); moveForm.append('destination', `/${org}/${repo}/.da-versions/forge-target/0000.html`); @@ -642,7 +642,7 @@ export default (ctx) => describe('Integration Tests: it tests', function () { }); assert.strictEqual(resp.status, 400, `Expected 400 from the destination guard on move, got ${resp.status} - user: ${superUser.email}`); body = await resp.json(); - assert.match(body.error, /da-versions/i, `Expected reserved-folder error, got ${body.error}`); + assert.match(body.error, /invalid or reserved/i, `Expected reserved-destination error, got ${body.error}`); // the blocked move must leave the source in place resp = await fetch(`${serverUrl}/source/${org}/${repo}/test-folder/page1-copy.html`, { diff --git a/test/routes/copy.test.js b/test/routes/copy.test.js index b8a3a4a4..6b2c8684 100644 --- a/test/routes/copy.test.js +++ b/test/routes/copy.test.js @@ -208,7 +208,7 @@ describe('Copy Route', () => { assert.strictEqual(resp.status, 400); assert.strictEqual(copyCalled.length, 0); const body = JSON.parse(resp.body); - assert.match(body.error, /da-versions/i); + assert.match(body.error, /invalid or reserved/i); }); it('Test copyHandler allows a destination segment that merely contains da-versions', async () => { diff --git a/test/routes/move.test.js b/test/routes/move.test.js index b3666f3c..f85fc6e1 100644 --- a/test/routes/move.test.js +++ b/test/routes/move.test.js @@ -150,7 +150,7 @@ describe('Move Route', () => { assert.strictEqual(resp.status, 400); assert.strictEqual(0, moCalled.length); const body = JSON.parse(resp.body); - assert.match(body.error, /da-versions/i); + assert.match(body.error, /invalid or reserved/i); }); it('Test moveRoute allows a destination segment that merely contains da-versions', async () => {