From ed0480f43c91a00cd6280bc136425e7e45aaf990 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Fri, 10 Jul 2026 17:57:24 +0200 Subject: [PATCH 1/2] fix: reject cross-org copy/move destinations instead of misdelivering The copy and move helpers parsed the destination form field with split('/').slice(1), silently discarding the destination's org segment and re-anchoring the path under the request org. A cross-org copy returned 204 and created a phantom copy inside the source org; nothing reached the intended destination org (confirmed against production admin.da.live on 2026-07-10). Under a restrictive source-org ACL the same request surfaced as a misleading 403. Compare the destination's org segment to the request org and return an explicit 400 on mismatch. Both endpoints are documented as operating within an organization, and 400 is already a documented response code for both. The underlying feature ask is adobe/da-live#623. Relates to adobe/da-live#34. --- src/helpers/copy.js | 9 +++++- src/helpers/move.js | 12 ++++++- test/it/it-tests.js | 69 ++++++++++++++++++++++++++++++++++++++++ test/routes/copy.test.js | 65 ++++++++++++++++++++++++++++++++++--- test/routes/move.test.js | 64 ++++++++++++++++++++++++++++++++++--- 5 files changed, 209 insertions(+), 10 deletions(-) diff --git a/src/helpers/copy.js b/src/helpers/copy.js index 39fd4eb2..3c624cb2 100644 --- a/src/helpers/copy.js +++ b/src/helpers/copy.js @@ -19,6 +19,11 @@ const BAD_CONTENT_TYPE_ERROR = { status: 400, }; +const CROSS_ORG_ERROR = { + body: JSON.stringify({ error: 'Destination must be in the same org as the source.' }), + status: 400, +}; + export default async function copyHelper(req, daCtx) { let formData; try { @@ -32,7 +37,9 @@ export default async function copyHelper(req, daCtx) { const continuationToken = formData.get('continuation-token'); const lower = fullDest.slice(1).toLowerCase(); const sanitized = lower.endsWith('/') ? lower.slice(0, -1) : lower; - const destination = sanitized.split('/').slice(1).join('/'); + const [destOrg, ...destParts] = sanitized.split('/'); + if (destOrg !== daCtx.org) return { error: CROSS_ORG_ERROR }; + const destination = destParts.join('/'); const source = daCtx.key; return { source, destination, continuationToken }; } diff --git a/src/helpers/move.js b/src/helpers/move.js index 62604770..8f3c6f01 100644 --- a/src/helpers/move.js +++ b/src/helpers/move.js @@ -20,6 +20,11 @@ const NO_PARENT_ERROR = { status: 400, }; +const CROSS_ORG_ERROR = { + body: JSON.stringify({ error: 'Destination must be in the same org as the source.' }), + status: 400, +}; + export default async function moveHelper(req, daCtx) { try { const formData = await req.formData(); @@ -28,7 +33,12 @@ export default async function moveHelper(req, daCtx) { if (!fullDest) return { error: NO_DEST_ERROR }; const lower = fullDest.slice(1).toLowerCase(); const sanitized = lower.endsWith('/') ? lower.slice(0, -1) : lower; - let destination = sanitized.split('/').slice(1).join('/'); + + // Reject cross-org destinations + const [destOrg, ...destParts] = sanitized.split('/'); + if (destOrg !== daCtx.org) return { error: CROSS_ORG_ERROR }; + + let destination = destParts.join('/'); const source = daCtx.key; // Ensure destination is not child of source diff --git a/test/it/it-tests.js b/test/it/it-tests.js index f37b988d..55883f39 100644 --- a/test/it/it-tests.js +++ b/test/it/it-tests.js @@ -451,6 +451,75 @@ export default (ctx) => describe('Integration Tests: it tests', function () { assert.ok(fileNames.includes('page2'), 'Should list page2'); }); + it('[super user] should copy a page within the org', async () => { + const { + serverUrl, org, repo, superUser, + } = ctx; + const formData = new FormData(); + formData.append('destination', `/${org}/${repo}/test-folder/page1-copy.html`); + + let resp = await fetch(`${serverUrl}/copy/${org}/${repo}/test-folder/page1.html`, { + method: 'POST', + body: formData, + headers: { Authorization: `Bearer ${superUser.accessToken}` }, + }); + assert.strictEqual(resp.status, 204, `Expected 204 No Content, got ${resp.status} - user: ${superUser.email}`); + + // validate the copy exists + 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}`); + const body = await resp.text(); + assert.strictEqual(body, '

Page 1

'); + }); + + it('[super user] cannot copy a page to another org', async () => { + const { + serverUrl, org, repo, superUser, + } = ctx; + const formData = new FormData(); + formData.append('destination', `/other-${org}/${repo}/test-folder/page1-xorg.html`); + + let resp = await fetch(`${serverUrl}/copy/${org}/${repo}/test-folder/page1.html`, { + method: 'POST', + body: formData, + headers: { Authorization: `Bearer ${superUser.accessToken}` }, + }); + assert.strictEqual(resp.status, 400, `Expected 400 Bad Request, got ${resp.status} - user: ${superUser.email}`); + const body = await resp.json(); + assert.match(body.error, /same org/i, `Expected cross-org error, got ${body.error}`); + + // validate no phantom copy was re-anchored into the source org + resp = await fetch(`${serverUrl}/source/${org}/${repo}/test-folder/page1-xorg.html`, { + headers: { Authorization: `Bearer ${superUser.accessToken}` }, + }); + assert.strictEqual(resp.status, 404, `Expected 404 Not Found, got ${resp.status} - user: ${superUser.email}`); + }); + + it('[super user] cannot move a page to another org', async () => { + const { + serverUrl, org, repo, superUser, + } = ctx; + const formData = new FormData(); + formData.append('destination', `/other-${org}/${repo}/test-folder/page1-moved.html`); + + let resp = await fetch(`${serverUrl}/move/${org}/${repo}/test-folder/page1-copy.html`, { + method: 'POST', + body: formData, + headers: { Authorization: `Bearer ${superUser.accessToken}` }, + }); + assert.strictEqual(resp.status, 400, `Expected 400 Bad Request, got ${resp.status} - user: ${superUser.email}`); + const body = await resp.json(); + assert.match(body.error, /same org/i, `Expected cross-org error, got ${body.error}`); + + // validate the source was not touched + 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, diff --git a/test/routes/copy.test.js b/test/routes/copy.test.js index ff25ed9f..97d54d90 100644 --- a/test/routes/copy.test.js +++ b/test/routes/copy.test.js @@ -44,11 +44,11 @@ describe('Copy Route', () => { formData: () => formdata, }; - const resp = await copyHandler({ req, env: {}, daCtx: { key: 'my/src.html' } }); + const resp = await copyHandler({ req, env: {}, daCtx: { org: 'myorg', key: 'my/src.html' } }); assert.strictEqual(403, resp.status); assert.strictEqual(copyCalled.length, 0); - const resp2 = await copyHandler({ req, env: {}, daCtx: { key: 'my/src2.html' } }); + const resp2 = await copyHandler({ req, env: {}, daCtx: { org: 'myorg', key: 'my/src2.html' } }); assert.strictEqual(403, resp2.status); assert.strictEqual(copyCalled.length, 0); @@ -58,11 +58,11 @@ describe('Copy Route', () => { formData: () => formdata2, }; - const resp3 = await copyHandler({ req: req2, env: {}, daCtx: { key: 'my/src.html' } }); + const resp3 = await copyHandler({ req: req2, env: {}, daCtx: { org: 'myorg', key: 'my/src.html' } }); assert.strictEqual(403, resp3.status); assert.strictEqual(copyCalled.length, 0); - const resp4 = await copyHandler({ req: req2, env: {}, daCtx: { key: 'my/src2.html' } }); + const resp4 = await copyHandler({ req: req2, env: {}, daCtx: { org: 'myorg', key: 'my/src2.html' } }); assert.strictEqual(200, resp4.status); assert.strictEqual(copyCalled.length, 1); assert.strictEqual('my/src2.html', copyCalled[0].d.source); @@ -99,6 +99,63 @@ describe('Copy Route', () => { assert.match(body.error, /Content-Type/i); }); + it('Test copyHandler returns 400 when destination org differs from request org', 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', '/otherorg/my/dest.html'); + const req = { + formData: () => formdata, + }; + + const resp = await copyHandler({ req, env: {}, daCtx: { org: 'myorg', key: 'my/src.html' } }); + assert.strictEqual(resp.status, 400); + assert.strictEqual(copyCalled.length, 0); + const body = JSON.parse(resp.body); + assert.match(body.error, /same org/i); + }); + + it('Test copyHandler accepts a same-org destination regardless of case', 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/dest.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/dest.html'); + }); + it('Test copyHandler - no destination provided', async () => { const copyCalled = []; const copyObject = (e, c, d, m) => { diff --git a/test/routes/move.test.js b/test/routes/move.test.js index 9f0b58fd..5cf8d096 100644 --- a/test/routes/move.test.js +++ b/test/routes/move.test.js @@ -44,11 +44,11 @@ describe('Move Route', () => { formData: () => formdata, }; - const resp = await moveRoute({ req, env: {}, daCtx: { key: 'abc.html' } }); + const resp = await moveRoute({ req, env: {}, daCtx: { org: 'someorg', key: 'abc.html' } }); assert.strictEqual(403, resp.status); assert.strictEqual(0, moCalled.length); - const resp2 = await moveRoute({ req, env: {}, daCtx: { key: 'zzz.html' } }); + const resp2 = await moveRoute({ req, env: {}, daCtx: { org: 'someorg', key: 'zzz.html' } }); assert.strictEqual(403, resp2.status); assert.strictEqual(0, moCalled.length); @@ -58,13 +58,69 @@ describe('Move Route', () => { formData: () => formdata2, }; - const resp3 = await moveRoute({ req: req2, env: {}, daCtx: { key: 'abc.html' } }); + const resp3 = await moveRoute({ req: req2, env: {}, daCtx: { org: 'someorg', key: 'abc.html' } }); assert.strictEqual(403, resp3.status); assert.strictEqual(0, moCalled.length); - await moveRoute({ req: req2, env: {}, daCtx: { key: 'zzz.html' } }); + await moveRoute({ req: req2, env: {}, daCtx: { org: 'someorg', key: 'zzz.html' } }); assert.strictEqual(1, moCalled.length); assert.strictEqual('zzz.html', moCalled[0].d.source); assert.strictEqual('someotherdest', moCalled[0].d.destination); }); + + it('Test moveRoute returns 400 when destination org differs from request org', async () => { + const moCalled = []; + const moveObject = (e, c, d) => { + moCalled.push({ e, c, d }); + }; + + 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', '/otherorg/somedest/'); + const req = { + formData: () => formdata, + }; + + const resp = await moveRoute({ req, env: {}, daCtx: { org: 'someorg', key: 'abc.html' } }); + assert.strictEqual(resp.status, 400); + assert.strictEqual(0, moCalled.length); + const body = JSON.parse(resp.body); + assert.match(body.error, /same org/i); + }); + + it('Test moveRoute accepts a same-org destination regardless of case', 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/somedest/'); + 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('somedest', moCalled[0].d.destination); + }); }); From 3eadb262f019a36701a96381ee5c8c3f7c60c00e Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Tue, 14 Jul 2026 12:52:22 +0200 Subject: [PATCH 2/2] style(copy): align cross-org guard formatting with move helper --- src/helpers/copy.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/helpers/copy.js b/src/helpers/copy.js index 3c624cb2..d984278f 100644 --- a/src/helpers/copy.js +++ b/src/helpers/copy.js @@ -37,8 +37,11 @@ export default async function copyHelper(req, daCtx) { const continuationToken = formData.get('continuation-token'); const lower = fullDest.slice(1).toLowerCase(); const sanitized = lower.endsWith('/') ? lower.slice(0, -1) : lower; + + // Reject cross-org destinations const [destOrg, ...destParts] = sanitized.split('/'); if (destOrg !== daCtx.org) return { error: CROSS_ORG_ERROR }; + const destination = destParts.join('/'); const source = daCtx.key; return { source, destination, continuationToken };