fix: prevent cross-parent IDOR via body-supplied up__ keys

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!**

lib/generic-handlers.js

@@ -25,33 +25,33 @@ 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
-      target = cds.infer.target({ SELECT: { from: { ref: 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)
+  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
+    target = cds.infer.target({ SELECT: { from: { ref: 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 (!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)}`,
-        )
+      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 +60,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

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 } = 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)
+    }
+  })
+})