From 0daeb783b9f77f52fd63b04c965d0680641420fa Mon Sep 17 00:00:00 2001 From: robbond Date: Mon, 26 Oct 2026 14:39:22 +0000 Subject: [PATCH] Stage 3: enforce blob deletion ownership and add admin representation deletion Also repairs two stale Phase 21 fixtures: - Supplies a valid PDF to exercise the upload dependency-failure path. - Injects getPersonalAccount into the representation-involvement VM fixture. --- components/admin/tabs/storage.js | 24 +- docs/administrative-authentication.md | 32 +- pages/api/admin/deleteblobrep.js | 78 +++++ pages/api/file/deleteblobcase.js | 17 ++ pages/api/file/deleteblobrep.js | 17 ++ tests/phase21/file-handler-contract.test.cjs | 304 ++++++++++++++++++- 6 files changed, 458 insertions(+), 14 deletions(-) create mode 100644 pages/api/admin/deleteblobrep.js diff --git a/components/admin/tabs/storage.js b/components/admin/tabs/storage.js index de680619..30ca3412 100644 --- a/components/admin/tabs/storage.js +++ b/components/admin/tabs/storage.js @@ -3,12 +3,10 @@ import { useState } from "react"; import Link from "next/link"; import { bytesToSize } from "../../../components/utils"; import { - deleteMyRepresentationsFromBlob, deleteAwaitingSubmissionsFromBlob, getRepsFromBlobProxy } from "../../../actions/services/documentService"; import { sendRepCompleteMessage } from "../../../actions/services/portalService"; -import { parseCookies } from "nookies"; import { consoleLogger } from "../../../actions/core/logger"; import { formatBlobDates } from "../utils/adminHelper"; @@ -25,10 +23,21 @@ export default function StorageTab({ data, repCompleteCount }) { }; const deleteRepItem = (containerID, caseID, repfile) => { - let cookies = parseCookies(); - //console.log("sssss", containerID, caseID, repfile); - - deleteMyRepresentationsFromBlob(containerID, caseID, repfile) + fetch("/api/admin/deleteblobrep", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + container: containerID, + casefolderID: caseID, + repfile + }) + }) + .then((response) => { + if (!response.ok) { + throw new Error("Unable to delete representation"); + } + return response.json(); + }) .then((data) => { return data; }) @@ -48,6 +57,9 @@ export default function StorageTab({ data, repCompleteCount }) { "viewKey": "myRepresentations" }); }); + }) + .catch((error) => { + consoleLogger(error); }); }; diff --git a/docs/administrative-authentication.md b/docs/administrative-authentication.md index e9f8c871..bc212df9 100644 --- a/docs/administrative-authentication.md +++ b/docs/administrative-authentication.md @@ -70,10 +70,9 @@ independent administrative-session authorization. A later authorised stage must enforce the administrative session on privileged API routes and account for both browser calls and the existing server-side bootstrap callers. -Storage download, representation editing, queue actions, and draft deletion -also retain their existing authorization behaviour in this stage. In -particular, the original cross-container deletion risk is not remediated by the -administrative page login and remains a separate mitigation boundary. +Storage download, representation editing, and queue actions retain their +existing authorization behaviour. The other administrative API and storage +limitations described in this section remain open. Directly observed storage routes include: @@ -83,8 +82,29 @@ Directly observed storage routes include: visible administrative-session or HMAC check. - `/api/file/downloadblob` and `/api/file/createrepcompletemessage_api`, which enforce existing path-HMAC checks but not administrative authorization. -- `/api/file/deleteblobcase` and `/api/file/deleteblobrep`, whose existing - cross-container deletion behaviour is intentionally unchanged in this stage. + +## Stage 3 deletion controls + +Ordinary case and representation deletion through `/api/file/deleteblobcase` +and `/api/file/deleteblobrep` now requires a valid NextAuth session whose user +ID matches the requested Azure container. The existing signed-query/HMAC check +also remains required. Requests that fail ownership or HMAC validation are +rejected before the Azure storage deletion operation is called. + +Administrative cross-container representation deletion uses the dedicated +`POST /api/admin/deleteblobrep` route. That endpoint independently validates +the administrative session and requires a trusted same-origin request. It also +validates the requested container, case-folder segment, and representation-file +segment before performing deletion. + +Administrative case deletion was not investigated or remediated as part of +Stage 3 and remains a separate, unverified security boundary. The other +administrative API and storage limitations documented above also remain open. + +Automated contract verification for the implemented deletion controls has +passed. Live Azure behaviour, browser-level behaviour, and deployment +configuration have not been verified by that automated coverage. These changes +do not establish closure of the overall ITHC finding. ## Deployment requirements before production use diff --git a/pages/api/admin/deleteblobrep.js b/pages/api/admin/deleteblobrep.js new file mode 100644 index 00000000..78c56cca --- /dev/null +++ b/pages/api/admin/deleteblobrep.js @@ -0,0 +1,78 @@ +import { deleteBlobRep } from "../../../actions/azurestorage"; +import { + hasTrustedSameOrigin, + readAdminSessionCookie, + setAdminAuthNoStore, + validateAdminSession +} from "../../../lib/admin/adminAuth"; +import { respondError, respondSuccess } from "../middleware/apiResponse"; + +const forbidden = (res) => + respondError(res, { + status: 403, + code: "FORBIDDEN", + message: "Request not permitted" + }); + +const isValidContainer = (value) => + typeof value === "string" && + value.length >= 3 && + value.length <= 63 && + /^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$/.test(value); + +const isValidPathSegment = (value) => + typeof value === "string" && + value.length > 0 && + value.length <= 255 && + value === value.trim() && + value !== "." && + value !== ".." && + !/[\\/\u0000-\u001f\u007f]/.test(value); + +export default async function handler(req, res) { + setAdminAuthNoStore(res); + res.setHeader("Allow", "POST"); + + if (req.method !== "POST") { + return respondError(res, { + status: 405, + code: "METHOD_NOT_ALLOWED", + message: "Method not allowed" + }); + } + + const adminSession = readAdminSessionCookie(req); + if ( + !validateAdminSession(adminSession) || + !hasTrustedSameOrigin(req) + ) { + return forbidden(res); + } + + const { container, casefolderID, repfile } = req.body ?? {}; + if ( + !isValidContainer(container) || + !isValidPathSegment(casefolderID) || + !isValidPathSegment(repfile) + ) { + return respondError(res, { + status: 400, + code: "INVALID_REQUEST", + message: "Invalid request" + }); + } + + try { + const data = await deleteBlobRep( + container, + `${casefolderID}/${repfile}` + ); + return respondSuccess(res, { data }); + } catch (_error) { + return respondError(res, { + status: 400, + code: "DELETE_BLOB_REP_FAILED", + message: "Unable to delete blob representation" + }); + } +} \ No newline at end of file diff --git a/pages/api/file/deleteblobcase.js b/pages/api/file/deleteblobcase.js index 62764132..9c969db7 100644 --- a/pages/api/file/deleteblobcase.js +++ b/pages/api/file/deleteblobcase.js @@ -1,6 +1,7 @@ import { hashAPIPath } from "../../../actions/core/hash"; import { deleteBlobCase } from "../../../actions/azurestorage"; import { respondError, respondSuccess } from "../middleware/apiResponse"; +import { getSession } from "next-auth/react"; import nextConnect from "next-connect"; import middleware from "../middleware/middleware"; @@ -8,6 +9,13 @@ import middleware from "../middleware/middleware"; const ApiProxy = nextConnect(); ApiProxy.use(middleware); +const forbidden = (res) => + respondError(res, { + status: 403, + code: "FORBIDDEN", + message: "Request not permitted" + }); + ApiProxy.get(async (req, res) => { const containerName = req.query.container; const casefolderID = req.query.casefolderID; @@ -30,6 +38,15 @@ ApiProxy.get(async (req, res) => { const casefolderIDTrimmed = casefolderID.trim(); + const session = await getSession({ req }); + if ( + typeof session?.user?.id !== "string" || + session.user.id.length === 0 || + session.user.id !== containerName + ) { + return forbidden(res); + } + const hashCandidatePaths = [ "/api/file/deleteblobcase?container=" + containerName + diff --git a/pages/api/file/deleteblobrep.js b/pages/api/file/deleteblobrep.js index b327837c..405f0bab 100644 --- a/pages/api/file/deleteblobrep.js +++ b/pages/api/file/deleteblobrep.js @@ -1,6 +1,7 @@ import { hashAPIPath } from "../../../actions/core/hash"; import { deleteBlobRep } from "../../../actions/azurestorage"; import { respondError, respondSuccess } from "../middleware/apiResponse"; +import { getSession } from "next-auth/react"; import nextConnect from "next-connect"; import middleware from "../middleware/middleware"; @@ -8,6 +9,13 @@ import middleware from "../middleware/middleware"; const ApiProxy = nextConnect(); ApiProxy.use(middleware); +const forbidden = (res) => + respondError(res, { + status: 403, + code: "FORBIDDEN", + message: "Request not permitted" + }); + ApiProxy.get(async (req, res) => { const containerName = req.query.container; const casefolderID = req.query.casefolderID; @@ -34,6 +42,15 @@ ApiProxy.get(async (req, res) => { const casefolderIDTrimmed = casefolderID.trim(); const repfileTrimmed = repfile.trim(); + const session = await getSession({ req }); + if ( + typeof session?.user?.id !== "string" || + session.user.id.length === 0 || + session.user.id !== containerName + ) { + return forbidden(res); + } + const hashCandidatePaths = [ "/api/file/deleteblobrep?container=" + containerName + diff --git a/tests/phase21/file-handler-contract.test.cjs b/tests/phase21/file-handler-contract.test.cjs index 3d976800..1eeab609 100644 --- a/tests/phase21/file-handler-contract.test.cjs +++ b/tests/phase21/file-handler-contract.test.cjs @@ -1,4 +1,6 @@ const assert = require("assert"); +const fs = require("fs"); +const path = require("path"); const { loadModule, createRes, @@ -171,6 +173,7 @@ test("deleteblobcase handler accepts encoded hash variant", async () => { const mod = loadModule("pages/api/file/deleteblobcase.js", { nextConnect: createNextConnectMock(), middleware: () => {}, + getSession: async () => ({ user: { id: "c1" } }), hashAPIPath: (queryPath) => queryPath.includes("casefolderID=CASE%2FSUB") ? "&hash=expected" @@ -201,6 +204,7 @@ test("deleteblobcase handler dependency failure returns DELETE_BLOB_CASE_FAILED" const mod = loadModule("pages/api/file/deleteblobcase.js", { nextConnect: createNextConnectMock(), middleware: () => {}, + getSession: async () => ({ user: { id: "c1" } }), hashAPIPath: () => "&hash=expected", respondError: respondErrorMock, respondSuccess: respondSuccessMock, @@ -231,6 +235,7 @@ test("deleteblobrep handler accepts encoded hash variant", async () => { const mod = loadModule("pages/api/file/deleteblobrep.js", { nextConnect: createNextConnectMock(), middleware: () => {}, + getSession: async () => ({ user: { id: "c1" } }), hashAPIPath: (queryPath) => queryPath.includes("repfile=Rep%20Name") ? "&hash=expected" @@ -262,6 +267,7 @@ test("deleteblobrep handler dependency failure returns DELETE_BLOB_REP_FAILED", const mod = loadModule("pages/api/file/deleteblobrep.js", { nextConnect: createNextConnectMock(), middleware: () => {}, + getSession: async () => ({ user: { id: "c1" } }), hashAPIPath: () => "&hash=expected", respondError: respondErrorMock, respondSuccess: respondSuccessMock, @@ -285,6 +291,288 @@ test("deleteblobrep handler dependency failure returns DELETE_BLOB_REP_FAILED", assert.strictEqual(res.state.jsonBody.error.code, "DELETE_BLOB_REP_FAILED"); }); +const ordinaryDeletionCases = [ + { + name: "deleteblobcase", + route: "pages/api/file/deleteblobcase.js", + query: { + container: "user-container", + casefolderID: "CASE-1", + hash: "expected" + }, + dependency: "deleteBlobCase" + }, + { + name: "deleteblobrep", + route: "pages/api/file/deleteblobrep.js", + query: { + container: "user-container", + casefolderID: "CASE-1", + repfile: "representation.json", + hash: "expected" + }, + dependency: "deleteBlobRep" + } +]; + +for (const deletionCase of ordinaryDeletionCases) { + test(`${deletionCase.name} forbids a missing NextAuth session before deletion`, async () => { + let deletionCalls = 0; + const mod = loadModule(deletionCase.route, { + nextConnect: createNextConnectMock(), + middleware: () => {}, + getSession: async () => null, + hashAPIPath: () => "&hash=expected", + respondError: respondErrorMock, + respondSuccess: respondSuccessMock, + [deletionCase.dependency]: async () => { + deletionCalls += 1; + } + }); + const res = createRes(); + + await mod.default.handler({ query: deletionCase.query }, res); + + assert.strictEqual(res.state.statusCode, 403); + assert.strictEqual(res.state.jsonBody.error.code, "FORBIDDEN"); + assert.strictEqual(deletionCalls, 0); + }); + + test(`${deletionCase.name} forbids a session without user id before deletion`, async () => { + let deletionCalls = 0; + const mod = loadModule(deletionCase.route, { + nextConnect: createNextConnectMock(), + middleware: () => {}, + getSession: async () => ({ user: {} }), + hashAPIPath: () => "&hash=expected", + respondError: respondErrorMock, + respondSuccess: respondSuccessMock, + [deletionCase.dependency]: async () => { + deletionCalls += 1; + } + }); + const res = createRes(); + + await mod.default.handler({ query: deletionCase.query }, res); + + assert.strictEqual(res.state.statusCode, 403); + assert.strictEqual(deletionCalls, 0); + }); + + test(`${deletionCase.name} forbids cross-container deletion before HMAC or Azure deletion`, async () => { + let hashCalls = 0; + let deletionCalls = 0; + const mod = loadModule(deletionCase.route, { + nextConnect: createNextConnectMock(), + middleware: () => {}, + getSession: async () => ({ user: { id: "different-user" } }), + hashAPIPath: () => { + hashCalls += 1; + return "&hash=expected"; + }, + respondError: respondErrorMock, + respondSuccess: respondSuccessMock, + [deletionCase.dependency]: async () => { + deletionCalls += 1; + } + }); + const res = createRes(); + + await mod.default.handler({ query: deletionCase.query }, res); + + assert.strictEqual(res.state.statusCode, 403); + assert.strictEqual(hashCalls, 0); + assert.strictEqual(deletionCalls, 0); + }); + + test(`${deletionCase.name} rejects invalid HMAC for the owning user without deletion`, async () => { + let deletionCalls = 0; + const mod = loadModule(deletionCase.route, { + nextConnect: createNextConnectMock(), + middleware: () => {}, + getSession: async () => ({ user: { id: "user-container" } }), + hashAPIPath: () => "&hash=different", + respondError: respondErrorMock, + respondSuccess: respondSuccessMock, + [deletionCase.dependency]: async () => { + deletionCalls += 1; + } + }); + const res = createRes(); + + await mod.default.handler({ query: deletionCase.query }, res); + + assert.strictEqual(res.state.statusCode, 400); + assert.strictEqual(res.state.jsonBody.error.code, "INVALID_HASH"); + assert.strictEqual(deletionCalls, 0); + }); + + test(`${deletionCase.name} preserves valid same-user deletion`, async () => { + const deletionCalls = []; + const mod = loadModule(deletionCase.route, { + nextConnect: createNextConnectMock(), + middleware: () => {}, + getSession: async () => ({ user: { id: "user-container" } }), + hashAPIPath: () => "&hash=expected", + respondError: respondErrorMock, + respondSuccess: respondSuccessMock, + [deletionCase.dependency]: async (...args) => { + deletionCalls.push(args); + return { deleted: true }; + } + }); + const res = createRes(); + + await mod.default.handler({ query: deletionCase.query }, res); + + assert.strictEqual(res.state.statusCode, 200); + assert.strictEqual(deletionCalls.length, 1); + assert.strictEqual(deletionCalls[0][0], "user-container"); + }); +} + +const loadAdminDeletion = (overrides = {}) => { + let deletionCalls = 0; + const mod = loadModule("pages/api/admin/deleteblobrep.js", { + deleteBlobRep: async () => { + deletionCalls += 1; + return { deleted: true }; + }, + readAdminSessionCookie: () => "admin-token", + validateAdminSession: () => true, + hasTrustedSameOrigin: () => true, + setAdminAuthNoStore: () => {}, + respondError: respondErrorMock, + respondSuccess: respondSuccessMock, + ...overrides + }); + + return { mod, getDeletionCalls: () => deletionCalls }; +}; + +const validAdminDeletionRequest = () => ({ + method: "POST", + headers: { origin: "https://portal.example.test" }, + body: { + container: "ck-user-container", + casefolderID: "CASE-1", + repfile: "representation.json" + } +}); + +test("admin representation deletion forbids a missing administrative session", async () => { + const loaded = loadAdminDeletion({ + readAdminSessionCookie: () => null, + validateAdminSession: () => false + }); + const res = createRes(); + + await loaded.mod.default(validAdminDeletionRequest(), res); + + assert.strictEqual(res.state.statusCode, 403); + assert.strictEqual(res.state.jsonBody.error.code, "FORBIDDEN"); + assert.strictEqual(loaded.getDeletionCalls(), 0); +}); + +test("admin representation deletion forbids an invalid or expired administrative session", async () => { + const loaded = loadAdminDeletion({ validateAdminSession: () => false }); + const res = createRes(); + + await loaded.mod.default(validAdminDeletionRequest(), res); + + assert.strictEqual(res.state.statusCode, 403); + assert.strictEqual(loaded.getDeletionCalls(), 0); +}); + +for (const originCase of ["missing", "cross-origin"]) { + test(`admin representation deletion forbids ${originCase} Origin`, async () => { + const loaded = loadAdminDeletion({ hasTrustedSameOrigin: () => false }); + const req = validAdminDeletionRequest(); + if (originCase === "missing") delete req.headers.origin; + if (originCase === "cross-origin") { + req.headers.origin = "https://attacker.example"; + } + const res = createRes(); + + await loaded.mod.default(req, res); + + assert.strictEqual(res.state.statusCode, 403); + assert.strictEqual(loaded.getDeletionCalls(), 0); + }); +} + +test("ordinary user HMAC cannot authorize administrative representation deletion", async () => { + const loaded = loadAdminDeletion({ + readAdminSessionCookie: () => null, + validateAdminSession: () => false + }); + const req = validAdminDeletionRequest(); + req.body.hash = "ordinary-user-hmac"; + const res = createRes(); + + await loaded.mod.default(req, res); + + assert.strictEqual(res.state.statusCode, 403); + assert.strictEqual(loaded.getDeletionCalls(), 0); +}); + +test("admin representation deletion rejects malformed paths without deletion", async () => { + for (const malformedBody of [ + { casefolderID: "../CASE-1" }, + { repfile: "folder/representation.json" }, + { repfile: ".." } + ]) { + const loaded = loadAdminDeletion(); + const req = validAdminDeletionRequest(); + req.body = { ...req.body, ...malformedBody }; + const res = createRes(); + + await loaded.mod.default(req, res); + + assert.strictEqual(res.state.statusCode, 400); + assert.strictEqual(res.state.jsonBody.error.code, "INVALID_REQUEST"); + assert.strictEqual(loaded.getDeletionCalls(), 0); + } +}); + +test("valid administrative session and origin authorize cross-container representation deletion", async () => { + const deletionCalls = []; + const loaded = loadAdminDeletion({ + deleteBlobRep: async (...args) => { + deletionCalls.push(args); + return { deleted: args[1] }; + } + }); + const res = createRes(); + + await loaded.mod.default(validAdminDeletionRequest(), res); + + assert.strictEqual(res.state.statusCode, 200); + assert.deepStrictEqual(JSON.parse(JSON.stringify(deletionCalls)), [ + ["ck-user-container", "CASE-1/representation.json"] + ]); +}); + +test("admin storage caller uses dedicated POST deletion while ordinary caller remains signed GET", () => { + const rootDir = path.resolve(__dirname, "..", ".."); + const adminSource = fs.readFileSync( + path.join(rootDir, "components/admin/tabs/storage.js"), + "utf8" + ); + const ordinarySource = fs.readFileSync( + path.join(rootDir, "actions/services/documentDirectService.js"), + "utf8" + ); + + assert.match(adminSource, /fetch\("\/api\/admin\/deleteblobrep"/); + assert.match(adminSource, /method:\s*"POST"/); + assert.doesNotMatch(adminSource, /deleteMyRepresentationsFromBlob/); + assert.match( + ordinarySource, + /buildFileQuery\("\/api\/file\/deleteblobrep"[\s\S]*getSignedFileJson\(queryUrl\)/ + ); +}); + test("deleteawaitingsubmissionfromblob accepts legacy deleteblob hash variant", async () => { let deletedPath = null; const mod = loadModule( @@ -467,13 +755,24 @@ test("uploadsinglefile handler dependency failure returns UPLOAD_SINGLE_FILE_FAI throw new Error("upload failed"); }, fileTypeFromBuffer: async () => ({ mime: "application/pdf" }), - fs: { readFileSync: () => Buffer.from("x") } + fs: { readFileSync: () => Buffer.from("x") }, + path: { basename: (value) => value } }); const req = { query: { hash: "expected" }, body: { containerID: ["c1"], casefolderID: ["f1"] }, - files: {} + files: { + document: [ + { + fieldName: "document.pdf", + originalFilename: "document.pdf", + path: "/tmp/document.pdf", + size: 1, + headers: { "content-type": "application/pdf" } + } + ] + } }; const res = createRes(); await mod.default.handler(req, res); @@ -820,6 +1119,7 @@ test("createrepinvolvement dependency failure returns CREATE_REP_INVOLVEMENT_FAI respondError: respondErrorMock, respondSuccess: respondSuccessMock, getToken: async () => ({ access_token: "tok" }), + getPersonalAccount: async () => ({}), hashAPIPath: () => "?hash=abc", consoleLogger: () => {}, process: {