From fef9591b5d61814f70cdfc9ccd13774e787c4b6c Mon Sep 17 00:00:00 2001 From: robbond Date: Mon, 9 Nov 2026 09:31:41 +0000 Subject: [PATCH] fix(security): protect representation blob deletion against CSRF --- actions/services/documentDirectService.js | 2 +- pages/api/file/deleteblobrep.js | 52 +++++++- tests/phase21/file-handler-contract.test.cjs | 127 ++++++++++++++++++- 3 files changed, 169 insertions(+), 12 deletions(-) diff --git a/actions/services/documentDirectService.js b/actions/services/documentDirectService.js index 4a5e986a..db6dd0b4 100644 --- a/actions/services/documentDirectService.js +++ b/actions/services/documentDirectService.js @@ -134,7 +134,7 @@ export const deleteMyRepresentationsFromBlob = ( repfile }); - return getSignedFileJson(queryUrl).catch((error) => { + return postSignedFileJson(queryUrl).catch((error) => { consoleLogger(error); }); }; diff --git a/pages/api/file/deleteblobrep.js b/pages/api/file/deleteblobrep.js index c129444b..0ad7c394 100644 --- a/pages/api/file/deleteblobrep.js +++ b/pages/api/file/deleteblobrep.js @@ -5,10 +5,17 @@ import { getServerSession } from "next-auth/next"; import { authOptions } from "../auth/[...nextauth]"; import nextConnect from "next-connect"; -import middleware from "../middleware/middleware"; -const ApiProxy = nextConnect(); -ApiProxy.use(middleware); +const methodNotAllowed = (req, res) => { + res.setHeader("Allow", "POST"); + return respondError(res, { + status: 405, + code: "METHOD_NOT_ALLOWED", + message: "Method not allowed" + }); +}; + +const ApiProxy = nextConnect({ onNoMatch: methodNotAllowed }); const forbidden = (res) => respondError(res, { @@ -26,7 +33,44 @@ const isValidPathSegment = (value) => value !== ".." && !/[\\/\u0000-\u001f\u007f]/.test(value); -ApiProxy.get(async (req, res) => { +const hasTrustedOrigin = (req) => { + const requestOrigin = req?.headers?.origin; + + if (typeof requestOrigin !== "string" || requestOrigin === "null") { + return false; + } + + try { + const requestUrl = new URL(requestOrigin); + if (requestOrigin !== requestUrl.origin) { + return false; + } + + const normalizedRequestOrigin = requestUrl.origin; + const trustedOrigins = [ + process.env.NEXTAUTH_URL, + process.env.CY_API_ROOT + ] + .filter((configuredUrl) => + typeof configuredUrl === "string" && configuredUrl.length > 0 + ) + .map((configuredUrl) => new URL(configuredUrl).origin); + + return trustedOrigins.includes(normalizedRequestOrigin); + } catch (_error) { + return false; + } +}; + +ApiProxy.post(async (req, res) => { + if (req.method !== "POST") { + return methodNotAllowed(req, res); + } + + if (!hasTrustedOrigin(req)) { + return forbidden(res); + } + const containerName = req.query.container; const casefolderID = req.query.casefolderID; const repfile = req.query.repfile; diff --git a/tests/phase21/file-handler-contract.test.cjs b/tests/phase21/file-handler-contract.test.cjs index c9b0b057..d93c8ca5 100644 --- a/tests/phase21/file-handler-contract.test.cjs +++ b/tests/phase21/file-handler-contract.test.cjs @@ -33,6 +33,18 @@ const validCaseDeletionRequest = (overrides = {}) => ({ ...overrides }); +const validRepDeletionRequest = (overrides = {}) => ({ + method: "POST", + headers: { origin: TRUSTED_EN_ORIGIN }, + query: { + container: "c1", + casefolderID: "CASE-1", + repfile: "representation.json", + hash: "expected" + }, + ...overrides +}); + const loadDocumentDirectService = (injected = {}) => { const sourcePath = path.resolve( __dirname, @@ -586,6 +598,7 @@ test("deleteblobrep handler accepts encoded hash variant", async () => { : "&hash=other", respondError: respondErrorMock, respondSuccess: respondSuccessMock, + process: deletionTestProcess, deleteBlobRep: async (_container, repPath) => { deletedRepPath = repPath; return { deleted: true }; @@ -593,6 +606,8 @@ test("deleteblobrep handler accepts encoded hash variant", async () => { }); const req = { + method: "POST", + headers: { origin: TRUSTED_EN_ORIGIN }, query: { container: "c1", casefolderID: "CASE-1", @@ -624,6 +639,7 @@ test("deleteblobrep rejects missing or unsafe folder segments before deletion", hashAPIPath: () => "&hash=expected", respondError: respondErrorMock, respondSuccess: respondSuccessMock, + process: deletionTestProcess, deleteBlobRep: async () => { deletionCalls += 1; } @@ -632,6 +648,8 @@ test("deleteblobrep rejects missing or unsafe folder segments before deletion", await mod.default.handler( { + method: "POST", + headers: { origin: TRUSTED_EN_ORIGIN }, query: { container: "c1", hash: "expected", @@ -656,12 +674,15 @@ test("deleteblobrep handler dependency failure returns DELETE_BLOB_REP_FAILED", hashAPIPath: () => "&hash=expected", respondError: respondErrorMock, respondSuccess: respondSuccessMock, + process: deletionTestProcess, deleteBlobRep: async () => { throw new Error("delete rep failed"); } }); const req = { + method: "POST", + headers: { origin: TRUSTED_EN_ORIGIN }, query: { container: "c1", casefolderID: "f1", @@ -676,6 +697,94 @@ test("deleteblobrep handler dependency failure returns DELETE_BLOB_REP_FAILED", assert.strictEqual(res.state.jsonBody.error.code, "DELETE_BLOB_REP_FAILED"); }); +const loadRepDeletionSecurity = (overrides = {}) => { + let deletionCalls = 0; + let sessionCalls = 0; + const mod = loadModule("pages/api/file/deleteblobrep.js", { + nextConnect: createNextConnectMock(), + getServerSession: async () => { + sessionCalls += 1; + return { user: { id: "c1" } }; + }, + authOptions: () => ({}), + hashAPIPath: () => "&hash=expected", + respondError: respondErrorMock, + respondSuccess: respondSuccessMock, + process: deletionTestProcess, + deleteBlobRep: async () => { + deletionCalls += 1; + return { deleted: true }; + }, + ...overrides + }); + + return { + mod, + getDeletionCalls: () => deletionCalls, + getSessionCalls: () => sessionCalls + }; +}; + +for (const method of ["GET", "DELETE"]) { + test(`deleteblobrep rejects ${method} with 405 before deletion`, async () => { + const loaded = loadRepDeletionSecurity(); + const res = createRes(); + + await loaded.mod.default.handler( + validRepDeletionRequest({ method }), + res + ); + + assert.strictEqual(res.state.statusCode, 405); + assert.strictEqual(res.state.jsonBody.error.code, "METHOD_NOT_ALLOWED"); + assert.strictEqual(res.state.headers.allow, "POST"); + assert.strictEqual(loaded.getSessionCalls(), 0); + assert.strictEqual(loaded.getDeletionCalls(), 0); + }); +} + +for (const [name, origin] of [ + ["cross-origin", "https://attacker.example.test"], + ["missing Origin", undefined], + ["malformed Origin", "not a valid origin"], + ["Origin containing a path", `${TRUSTED_EN_ORIGIN}/delete`], + ["null Origin", "null"], + ["subdomain spoof", "https://portal.example.test.attacker.test"], + ["hostname suffix spoof", "https://attacker-portal.example.test"] +]) { + test(`deleteblobrep rejects ${name} before session or deletion`, async () => { + const loaded = loadRepDeletionSecurity(); + const headers = origin === undefined ? {} : { origin }; + const res = createRes(); + + await loaded.mod.default.handler( + validRepDeletionRequest({ headers }), + res + ); + + assert.strictEqual(res.state.statusCode, 403); + assert.strictEqual(res.state.jsonBody.error.code, "FORBIDDEN"); + assert.strictEqual(loaded.getSessionCalls(), 0); + assert.strictEqual(loaded.getDeletionCalls(), 0); + }); +} + +test("deleteblobrep accepts the configured Welsh portal Origin", async () => { + const loaded = loadRepDeletionSecurity(); + const res = createRes(); + + await loaded.mod.default.handler( + validRepDeletionRequest({ + headers: { origin: TRUSTED_CY_ORIGIN } + }), + res + ); + + assert.strictEqual(res.state.statusCode, 200); + assert.strictEqual(loaded.getSessionCalls(), 1); + assert.strictEqual(loaded.getDeletionCalls(), 1); +}); + const ordinaryDeletionCases = [ { name: "deleteblobcase", @@ -694,7 +803,10 @@ const ordinaryDeletionCases = [ { name: "deleteblobrep", route: "pages/api/file/deleteblobrep.js", - request: {}, + request: { + method: "POST", + headers: { origin: TRUSTED_EN_ORIGIN } + }, query: { container: "user-container", casefolderID: "CASE-1", @@ -1022,7 +1134,7 @@ test("admin storage caller uses dedicated POST deletion while ordinary caller re assert.doesNotMatch(adminSource, /deleteMyRepresentationsFromBlob/); assert.match( ordinarySource, - /buildFileQuery\("\/api\/file\/deleteblobrep"[\s\S]*getSignedFileJson\(queryUrl\)/ + /buildFileQuery\("\/api\/file\/deleteblobrep"[\s\S]*postSignedFileJson\(queryUrl\)/ ); }); @@ -1209,7 +1321,7 @@ test("admin case deletion dependency failure returns DELETE_BLOB_CASE_FAILED", a assert.strictEqual(loaded.calls.deletion.length, 1); }); -test("case deletion uses signed POST while representation deletion remains signed GET", async () => { +test("case and representation deletion use signed POST", async () => { const calls = []; const mod = loadDocumentDirectService({ buildFileQuery: (route, params) => { @@ -1233,12 +1345,9 @@ test("case deletion uses signed POST while representation deletion remains signe await mod.deleteMyRepresentationsFromBlob("c1", "CASE-1", "REP-1"); assert.deepStrictEqual(JSON.parse(JSON.stringify(calls)), [ + ["post", "/api/file/deleteblobcase?container=c1&casefolderID=CASE-1"], [ "post", - "/api/file/deleteblobcase?container=c1&casefolderID=CASE-1" - ], - [ - "get", "/api/file/deleteblobrep?container=c1&casefolderID=CASE-1&repfile=REP-1" ] ]); @@ -5933,6 +6042,10 @@ test("whole-representation deletion remains cross-user protected with zero Azure await loaded.mod.default.handler( { + method: "POST", + headers: { + origin: "http://localhost:3000" + }, query: { container: "organisation-container", casefolderID: "CASE-1",