fix: eliminate TOCTOU race in scan-status update

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
+
+- TOCTOU race in scan-status update: a per-scan UUID token (`scanToken`) is now generated at scan-start and required in the completion `WHERE` clause, replacing the `hash IS NULL` disjunct that could match a concurrently-started scan and stamp the wrong verdict onto a replaced file. A new `scanToken` column has been added to the `Attachment` type.
+
 ## Version 4.0.0 - 2026-08-03
 
 **BREAKING CHANGE: The attachments plugin comes now without hyperscaler dependencies, please make sure to install them accordingly!**

db/index.cds

@@ -19,6 +19,7 @@ context sap.attachments {
     hash      : String                                    @UI.Hidden                  @Core.Computed;
     status    : String default 'Unscanned'                @title: '{i18n>ScanStatus}' @readonly;
     lastScan  : Timestamp                                 @title: '{i18n>LastScan}'   @Core.Computed  @readonly;
+    scanToken : UUID                                      @UI.Hidden                  @Core.Computed;
   }
 
   aspect MediaData : Attachment {

srv/malware-scanner/malwareScanner-mocked.js

@@ -10,6 +10,13 @@ class MockedMalwareScanner extends require("./malwareScanner") {
   async scanFile(req) {
     const { file } = req.data
 
+    // Widen the race window in tests. Set SCAN_DELAY_MS to a positive number of
+    // milliseconds to make the mock scanner pause before returning its verdict.
+    // This makes concurrent-upload TOCTOU scenarios reproducible without timing luck.
+    if (process.env.SCAN_DELAY_MS) {
+      await new Promise((r) => setTimeout(r, Number(process.env.SCAN_DELAY_MS)))
+    }
+
     LOG.info(`Setting scan status to Clean (development mode)!`)
 
     let fileSize = 0

srv/malware-scanner/malwareScanner.js

@@ -74,7 +74,7 @@ class MalwareScanner extends cds.ApplicationService {
       return
     }
 
-    await this.updateStatus(
+    const scanToken = await this.updateStatus(
       _target,
       keys,
       "Scanning",
@@ -122,48 +122,26 @@ class MalwareScanner extends cds.ApplicationService {
       LOG.debug(`Malware scan completed for ${target}, ${keys} - file is clean`)
     }
 
-    // Assign hash as another condition to ensure the correct file is marked as fine
+    // Use the per-scan token as the WHERE condition so that only the scan whose
+    // token is still in the DB row wins. If a concurrent upload has already started
+    // a new scan (overwriting scanToken), this WHERE silently matches zero rows and
+    // the stale verdict is safely discarded — eliminating the TOCTOU race.
+    const tokenField = prefix ? `${prefix}_scanToken` : "scanToken"
     await this.updateStatus(
       _target,
-      prefix
-        ? [
-            "(",
-            { ref: [`${prefix}_hash`] },
-            "=",
-            { val: hash },
-            "or",
-            { ref: [`${prefix}_hash`] },
-            "is",
-            "null",
-            ")",
-            "and",
-            {
-              xpr: Object.keys(keys).reduce((acc, key) => {
-                if (acc.length) acc.push("and")
-                acc.push({ ref: [key] }, "=", { val: keys[key] })
-                return acc
-              }, []),
-            },
-          ]
-        : [
-            "(",
-            { ref: ["hash"] },
-            "=",
-            { val: hash },
-            "or",
-            { ref: ["hash"] },
-            "is",
-            "null",
-            ")",
-            "and",
-            {
-              xpr: Object.keys(keys).reduce((acc, key) => {
-                if (acc.length) acc.push("and")
-                acc.push({ ref: [key] }, "=", { val: keys[key] })
-                return acc
-              }, []),
-            },
-          ],
+      [
+        { ref: [tokenField] },
+        "=",
+        { val: scanToken },
+        "and",
+        {
+          xpr: Object.keys(keys).reduce((acc, key) => {
+            if (acc.length) acc.push("and")
+            acc.push({ ref: [key] }, "=", { val: keys[key] })
+            return acc
+          }, []),
+        },
+      ],
       status,
       hash,
       ...(prefix ? [prefix] : []),
@@ -250,29 +228,61 @@ class MalwareScanner extends cds.ApplicationService {
 
   /**
    * Updates the scan status, lastScan timestamp, and optionally the hash on an attachment entity.
-   * When status is "Scanning", clears the hash to ensure the subsequent clean/infected update matches.
    * For draft-enabled entities, updates both the active and draft tables.
+   *
+   * When `status` is "Scanning":
+   *   - Generates a fresh UUID token, stores it in `scanToken` (or `<prefix>_scanToken`), and
+   *     returns it to the caller so it can be used as the WHERE condition on scan completion.
+   *   - Clears `hash` for inline/prefix attachments (preserves existing behaviour).
+   *   - Does NOT set `lastScan` (scan has not completed yet).
+   *   - Returns the generated token string.
+   *
+   * When `status` is "Clean", "Infected", or "Failed":
+   *   - Sets `lastScan` to now and, when `hash` is provided, writes it.
+   *   - Returns `undefined`.
+   *
+   * NOTE: The rescan path (generic-handlers.js) discards this token — it fires a fresh
+   * ScanAttachmentsFile event immediately, which overwrites scanToken before completion runs.
+   *
    * @param {import('@sap/cds').entity} target - The entity definition to update
    * @param {object|Array} where - WHERE condition: plain keys object or CQL expression array
    * @param {string} status - The new scan status ("Scanning", "Clean", "Infected", "Failed")
    * @param {string} [hash] - The SHA-256 hash of the scanned file; omitted when setting to "Scanning"
    * @param {string} [prefix] - Field prefix for inline attachments (e.g. "myAttachment")
+   * @returns {Promise<string|undefined>} The generated scanToken when status is "Scanning"; otherwise undefined
    */
   async updateStatus(target, where, status, hash, prefix) {
     const statusField = prefix ? `${prefix}_status` : "status"
     const lastScanField = prefix ? `${prefix}_lastScan` : "lastScan"
     const hashField = prefix ? `${prefix}_hash` : "hash"
-
-    const updateObject = { [statusField]: status }
-    if (status !== "Scanning") {
-      updateObject[lastScanField] = new Date()
-    } else if (prefix) {
-      // Clear hash for inline attachments so the subsequent clean/infected update
-      // always matches via IS NULL when the hash changes between scans (e.g. re-upload or re-scan)
-      updateObject[hashField] = null
+    const tokenField = prefix ? `${prefix}_scanToken` : "scanToken"
+
+    if (status === "Scanning") {
+      const scanToken = cds.utils.uuid()
+      const updateObject = {
+        [statusField]: "Scanning",
+        [tokenField]: scanToken,
+        ...(prefix ? { [hashField]: null } : {}),
+      }
+      if (target.drafts) {
+        await Promise.all([
+          UPDATE.entity(target).where(where).set(updateObject),
+          UPDATE.entity(target.drafts).where(where).set(updateObject),
+        ])
+      } else {
+        await UPDATE.entity(target).where(where).set(updateObject)
+      }
+      LOG.info(
+        `Updated scan status to Scanning for ${target.name}, ${JSON.stringify(where)}`,
+      )
+      return scanToken
     }
-    if (hash) {
-      updateObject[hashField] = hash
+
+    // Non-Scanning status update (Clean / Infected / Failed)
+    const updateObject = {
+      [statusField]: status,
+      [lastScanField]: new Date(),
+      ...(hash ? { [hashField]: hash } : {}),
     }
     if (target.drafts) {
       await Promise.all([

tests/unit/toctou-scan-status.test.js

@@ -0,0 +1,191 @@
+"use strict"
+require("../../lib/csn-runtime-extension")
+const cds = require("@sap/cds")
+const crypto = require("crypto")
+const path = require("path")
+const app = path.resolve(__dirname, "../incidents-app")
+cds.test(app)
+
+const MalwareScanner = require("../../srv/malware-scanner/malwareScanner")
+
+// ---------------------------------------------------------------------------
+// Unit-level TOCTOU test: exercises updateStatus + _scanAttachmentsFile
+// directly against the real SQLite DB, bypassing the HTTP layer.
+// ---------------------------------------------------------------------------
+
+let scanner
+
+beforeEach(() => {
+  jest.clearAllMocks()
+  cds.env.requires.attachments = { scan: true }
+  cds.env.requires.malwareScanner = {
+    credentials: { uri: "host", certificate: "C", key: "K" },
+  }
+  jest.spyOn(cds, "context", "get").mockReturnValue({ model: cds.model })
+  scanner = new MalwareScanner()
+  scanner.retryConfig = {
+    enabled: false,
+    maxAttempts: 1,
+    initialDelay: 0,
+    maxDelay: 0,
+  }
+})
+
+describe("TOCTOU race in scan-status update", () => {
+  const target = "ProcessorService.Incidents.attachments"
+
+  function hashOf(buf) {
+    return crypto.createHash("sha256").update(buf).digest("hex")
+  }
+
+  it("baseline: updateStatus(Scanning) then updateStatus(Clean) leaves correct hash", async () => {
+    const _target = cds.model.definitions[target]
+    const keys = { up__ID: cds.utils.uuid(), ID: cds.utils.uuid() }
+    await INSERT.into(_target).entries({
+      ...keys,
+      status: "Unscanned",
+      filename: "baseline.txt",
+    })
+
+    const hashA = hashOf(Buffer.from("FILE-A"))
+
+    const scanToken = await scanner.updateStatus(_target, keys, "Scanning")
+    expect(typeof scanToken).toBe("string")
+    expect(scanToken).toHaveLength(36)
+
+    // Verify row shows Scanning and has scanToken set
+    const scanning = await SELECT.one.from(_target).where(keys)
+    expect(scanning.status).toBe("Scanning")
+    expect(scanning.scanToken).toBe(scanToken)
+
+    await scanner.updateStatus(_target, keys, "Clean", hashA)
+
+    const row = await SELECT.one.from(_target).where(keys)
+    expect(row.status).toBe("Clean")
+    expect(row.hash).toBe(hashA)
+  })
+
+  it("TOCTOU race: stale scan-1 verdict must not overwrite scan-2 result", async () => {
+    const _target = cds.model.definitions[target]
+    const keys = { up__ID: cds.utils.uuid(), ID: cds.utils.uuid() }
+    await INSERT.into(_target).entries({
+      ...keys,
+      status: "Unscanned",
+      filename: "race.txt",
+    })
+
+    const hashA = hashOf(Buffer.from("FILE-A-CONTENT"))
+    const hashB = hashOf(Buffer.from("FILE-B-CONTENT"))
+
+    // scan-1 starts: sets scanToken-1
+    const tokenScan1 = await scanner.updateStatus(_target, keys, "Scanning")
+
+    // scan-2 starts: overwrites scanToken with tokenScan2
+    const tokenScan2 = await scanner.updateStatus(_target, keys, "Scanning")
+
+    expect(tokenScan1).not.toBe(tokenScan2)
+
+    // scan-2 completes first with tokenField WHERE
+    const tokenField = "scanToken"
+    await scanner.updateStatus(
+      _target,
+      [
+        { ref: [tokenField] },
+        "=",
+        { val: tokenScan2 },
+        "and",
+        {
+          xpr: Object.keys(keys).reduce((acc, key) => {
+            if (acc.length) acc.push("and")
+            acc.push({ ref: [key] }, "=", { val: keys[key] })
+            return acc
+          }, []),
+        },
+      ],
+      "Clean",
+      hashB,
+    )
+
+    const afterScan2 = await SELECT.one.from(_target).where(keys)
+    expect(afterScan2.status).toBe("Clean")
+    expect(afterScan2.hash).toBe(hashB)
+
+    // Now scan-1 completes with its (now-stale) token — must NOT overwrite
+    await scanner.updateStatus(
+      _target,
+      [
+        { ref: [tokenField] },
+        "=",
+        { val: tokenScan1 },
+        "and",
+        {
+          xpr: Object.keys(keys).reduce((acc, key) => {
+            if (acc.length) acc.push("and")
+            acc.push({ ref: [key] }, "=", { val: keys[key] })
+            return acc
+          }, []),
+        },
+      ],
+      "Clean",
+      hashA,
+    )
+
+    const afterScan1LateArrival = await SELECT.one.from(_target).where(keys)
+    // Bug (IS NULL): would have matched, overwriting hashB with hashA
+    // Fix (token): tokenScan1 no longer matches → row unchanged
+    expect(afterScan1LateArrival.hash).toBe(hashB)
+    expect(afterScan1LateArrival.status).toBe("Clean")
+  })
+
+  it("_scanAttachmentsFile: scan completion uses token so stale scan cannot overwrite", async () => {
+    const _target = cds.model.definitions[target]
+    const keys = { up__ID: cds.utils.uuid(), ID: cds.utils.uuid() }
+    await INSERT.into(_target).entries({
+      ...keys,
+      status: "Unscanned",
+      filename: "token-test.txt",
+    })
+
+    const expectedHash = hashOf(Buffer.from("CONTENT-FOR-TOKEN-TEST"))
+
+    // Simulate the attachment service getting the file
+    const { Readable } = require("stream")
+    const attachmentsSvc = {
+      get: jest.fn().mockResolvedValue(Readable.from([])),
+      emit: jest.fn().mockResolvedValue(undefined),
+    }
+    cds.connect.to = jest.fn().mockResolvedValue(attachmentsSvc)
+
+    // Mock scan (called as this.scan(stream) by _scanWithRetry) to return a fixed hash
+    scanner.scan = jest
+      .fn()
+      .mockResolvedValue({ isMalware: false, hash: expectedHash })
+
+    // Intercept updateStatus to capture the token returned from the Scanning call
+    let capturedToken = null
+    const originalUpdateStatus = scanner.updateStatus.bind(scanner)
+    let callCount = 0
+    scanner.updateStatus = jest.fn(async (...args) => {
+      callCount++
+      const result = await originalUpdateStatus(...args)
+      if (callCount === 1) {
+        // First call = "Scanning": capture the returned token
+        capturedToken = result
+      }
+      return result
+    })
+
+    await scanner._scanAttachmentsFile({ data: { target, keys } })
+
+    const row = await SELECT.one.from(_target).where(keys)
+    expect(row.status).toBe("Clean")
+    expect(row.hash).toBe(expectedHash)
+
+    // The Scanning call must have returned a token
+    expect(typeof capturedToken).toBe("string")
+    expect(capturedToken).toHaveLength(36)
+
+    // Token is not cleared on completion — the DB retains which scan last wrote the verdict.
+    expect(row.scanToken).toBeTruthy()
+  })
+})