From 8acab6f69d2dc38b19fddd3fd4fc4f721d7bfb3b Mon Sep 17 00:00:00 2001 From: Christian Schuerings Date: Tue, 4 Aug 2026 18:06:42 +0200 Subject: [PATCH 1/2] fix: prevent cross-parent IDOR via body-supplied up__ keys --- CHANGELOG.md | 6 ++ lib/generic-handlers.js | 56 +++++++------ tests/unit/cross-parent-idor-upkeys.test.js | 89 +++++++++++++++++++++ 3 files changed, 128 insertions(+), 23 deletions(-) create mode 100644 tests/unit/cross-parent-idor-upkeys.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index aa2786a5..e95ac527 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). The format is based on [Keep a Changelog](http://keepachangelog.com/). +## [Unreleased] + +### Fixed + +- `finalizePrepareAttachment` now always derives parent (`up__`) keys from the URL path, ignoring any values supplied in the request body. A 404 is returned when the URL-referenced parent record does not exist. Client-supplied attachment IDs are no longer accepted on CREATE. + ## Version 4.0.0 - 2026-08-03 **BREAKING CHANGE: The attachments plugin comes now without hyperscaler dependencies, please make sure to install them accordingly!** diff --git a/lib/generic-handlers.js b/lib/generic-handlers.js index 05728034..43e561ed 100644 --- a/lib/generic-handlers.js +++ b/lib/generic-handlers.js @@ -25,33 +25,39 @@ async function finalizePrepareAttachment(data, req, prefix) { delete attachmentData.url } - const hasUpKey = Object.keys(attachmentData).some((key) => - key.startsWith("up__"), - ) - - if (!hasUpKey) { - const parentRef = req.subject.ref.slice(0, -1) - - // Only try to populate parent keys if there is a parent reference - if (parentRef && parentRef.length > 0) { - let target + const parentRef = req.subject.ref.slice(0, -1) + + // Always derive up__ keys from the URL ref, overwriting any body-supplied values. + // Trusting body-supplied up__ keys would allow cross-parent IDOR: an attacker could + // POST to their own parent URL but claim a different parent's ID in the body. + if (parentRef && parentRef.length > 0) { + let target + if (cds.infer?.target) { + // CAP 9+: Use cds.infer.target target = cds.infer.target({ SELECT: { from: { ref: parentRef } } }) + } else { + // CAP 8 fallback: Use inferTargetCAP8 helper + target = inferTargetCAP8(req, parentRef) + } - if (target?.keys) { - LOG.info(`Populating parent keys for attachment upload`, target) - const parentKeys = Object.keys(target.keys) - const parentRecord = await SELECT.one - .from({ ref: parentRef }) - .columns(parentKeys) + if (target?.keys) { + LOG.info(`Populating parent keys for attachment upload`, target) + const parentKeys = Object.keys(target.keys) + const parentRecord = await SELECT.one + .from({ ref: parentRef }) + .columns(parentKeys) - for (const key of parentKeys) { - attachmentData[`up__${key}`] = parentRecord[key] - } - } else { - LOG.warn( - `Could not determine parent target for attachment upload. ParentRef: ${JSON.stringify(parentRef)}`, - ) + if (!parentRecord) { + return req.reject(404, "ParentRecordNotFound") + } + + for (const key of parentKeys) { + attachmentData[`up__${key}`] = parentRecord[key] } + } else { + LOG.warn( + `Could not determine parent target for attachment upload. ParentRef: ${JSON.stringify(parentRef)}`, + ) } } @@ -60,6 +66,10 @@ async function finalizePrepareAttachment(data, req, prefix) { // Generate URL for object store attachmentData.url = await attachment.createUrlForAttachment(attachmentData) } + // Reject any client-supplied ID on NEW so ??= below always assigns a server UUID. + // On subsequent invocations (e.g. before("CREATE") after before("NEW")), the ID + // is already ours and ??= preserves it correctly. + if (req.event === "NEW") delete attachmentData.ID attachmentData.ID ??= cds.utils.uuid() // On stream PUT, filename is not in req.data — fetch it from the DB record diff --git a/tests/unit/cross-parent-idor-upkeys.test.js b/tests/unit/cross-parent-idor-upkeys.test.js new file mode 100644 index 00000000..7c262891 --- /dev/null +++ b/tests/unit/cross-parent-idor-upkeys.test.js @@ -0,0 +1,89 @@ +"use strict" +require("../../lib/csn-runtime-extension") +const cds = require("@sap/cds") +const path = require("path") +const { withUser, newIncident } = require("../utils/testUtils") + +const app = path.resolve(__dirname, "../incidents-app") + +// Two users: alice creates an incident; bob tries to plant an attachment on it +const aliceTest = cds.test(app) +const { GET, POST } = withUser("alice", aliceTest) +const { POST: bobPOST, GET: bobGET } = withUser("bob", aliceTest) + +let aliceIncidentID +let bobIncidentID + +beforeAll(async () => { + aliceIncidentID = await newIncident(POST, "processor") + bobIncidentID = await newIncident(bobPOST, "processor") +}, 30000) + +afterAll(async () => { + await cds.disconnect() +}) + +describe("cross-parent IDOR via body up__ keys", () => { + it("baseline: alice can create an attachment on her own incident via navigation", async () => { + const res = await POST( + `odata/v4/processor/Incidents(ID=${aliceIncidentID},IsActiveEntity=false)/attachments`, + { filename: "legit.pdf", mimeType: "application/pdf" }, + ) + expect(res.status).toBe(201) + }) + + it("bob cannot plant an attachment on alice's incident by injecting up__ID in the body", async () => { + // Bob POSTs to his OWN incident URL but injects alice's incidentID as up__ID in the body. + // Without the fix: hasUpKey guard sees up__ID in body and skips URL-derived parent lookup, + // so the attachment is created with up__ID = aliceIncidentID (cross-parent IDOR). + // With the fix: URL-derived parent key overwrites the body-supplied up__ID, + // so the attachment goes to bob's incident (or the SELECT validates correctly). + const res = await bobPOST( + `odata/v4/processor/Incidents(ID=${bobIncidentID},IsActiveEntity=false)/attachments`, + { + up__ID: aliceIncidentID, // injected: bob's request body claims alice's incident + filename: "injected.pdf", + mimeType: "application/pdf", + }, + { validateStatus: () => true }, + ) + // Request should succeed (bob owns his incident), but the attachment must be on BOB's incident + if (res.status === 201) { + // The returned up__ID must be bob's, not alice's + expect(res.data.up__ID).not.toBe(aliceIncidentID) + } + }) + + it("alice's incident has no injected attachments after bob's attempt", async () => { + // Bob attempts the injection again and then we check alice's incident has no new attachments + await bobPOST( + `odata/v4/processor/Incidents(ID=${bobIncidentID},IsActiveEntity=false)/attachments`, + { + up__ID: aliceIncidentID, + filename: "injected2.pdf", + mimeType: "application/pdf", + }, + { validateStatus: () => true }, + ) + const res = await GET( + `odata/v4/processor/Incidents(ID=${aliceIncidentID},IsActiveEntity=false)/attachments`, + { validateStatus: () => true }, + ) + const injected = (res.data?.value ?? []).filter((a) => + a.filename?.includes("injected"), + ) + expect(injected).toHaveLength(0) + }) + + it("client-supplied attachment ID is ignored on CREATE", async () => { + const chosenID = cds.utils.uuid() + const res = await POST( + `odata/v4/processor/Incidents(ID=${aliceIncidentID},IsActiveEntity=false)/attachments`, + { ID: chosenID, filename: "chosen-id.pdf", mimeType: "application/pdf" }, + { validateStatus: () => true }, + ) + if (res.status === 201) { + expect(res.data.ID).not.toBe(chosenID) + } + }) +}) From 9da305c4cfea0d57d7feb89a772cbf8484b5df66 Mon Sep 17 00:00:00 2001 From: Christian Schuerings Date: Tue, 4 Aug 2026 18:26:07 +0200 Subject: [PATCH 2/2] fix: resolve lint errors from IDOR fix commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove undefined inferTargetCAP8 CAP 8 fallback — cds.infer.target is always available on CDS 9+. Drop unused bobGET variable in test. --- lib/generic-handlers.js | 8 +------- tests/unit/cross-parent-idor-upkeys.test.js | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/generic-handlers.js b/lib/generic-handlers.js index 43e561ed..df736eba 100644 --- a/lib/generic-handlers.js +++ b/lib/generic-handlers.js @@ -32,13 +32,7 @@ async function finalizePrepareAttachment(data, req, prefix) { // POST to their own parent URL but claim a different parent's ID in the body. if (parentRef && parentRef.length > 0) { let target - if (cds.infer?.target) { - // CAP 9+: Use cds.infer.target - target = cds.infer.target({ SELECT: { from: { ref: parentRef } } }) - } else { - // CAP 8 fallback: Use inferTargetCAP8 helper - target = inferTargetCAP8(req, parentRef) - } + target = cds.infer.target({ SELECT: { from: { ref: parentRef } } }) if (target?.keys) { LOG.info(`Populating parent keys for attachment upload`, target) diff --git a/tests/unit/cross-parent-idor-upkeys.test.js b/tests/unit/cross-parent-idor-upkeys.test.js index 7c262891..a9408eaf 100644 --- a/tests/unit/cross-parent-idor-upkeys.test.js +++ b/tests/unit/cross-parent-idor-upkeys.test.js @@ -9,7 +9,7 @@ const app = path.resolve(__dirname, "../incidents-app") // Two users: alice creates an incident; bob tries to plant an attachment on it const aliceTest = cds.test(app) const { GET, POST } = withUser("alice", aliceTest) -const { POST: bobPOST, GET: bobGET } = withUser("bob", aliceTest) +const { POST: bobPOST } = withUser("bob", aliceTest) let aliceIncidentID let bobIncidentID