Claude Security Review Prompt

Run on this sample: // account-routes.js: mounted after trusted authentication middleware. // req.user.id is the authenticated user's immutable ID. import { Router } from 'express'; const router = Router(); const accounts = new Map([ ['a1', { id: 'a1', ownerId: 'u1', displayName: 'Ari', role: 'member', passwordHash: 'demo-hash-a' }], ['a2', { id: 'a2', ownerId: 'u2', displayName: 'Bo', role: 'member', passwordHash: 'demo-hash-b' }] ]); router.patch('/accounts/:id', (req, res) => { if (!req.user) return res.sendStatus(401); const account = accounts.get(req.params.id); if (!account) return res.sendStatus(404); Object.assign(account, req.body); return res.json(account); }); export default router; --- Scope: This review covers only the supplied Express route and its in-memory account store. I accept the stated authentication context: req.user.id is a trusted, immutable user ID. No other routes, authorization policies, or downstream uses of account fields were supplied. ## 1. Any authenticated user can modify another user’s account **Severity: High.** An authenticated user can directly modify another account by choosing its ID. This requires neither elevated privileges nor a race condition. **Location:** account-routes.js, lines 11–13: ```js const account = accounts.get(req.params.id); if (!account) return res.sendStatus(404); Object.assign(account, req.body); ``` **Visible evidence:** The route checks authentication and account existence, but never compares account.ownerId with req.user.id. The trusted identity is available and unused. **Trigger:** User u1 sends: ```http PATCH /accounts/a2 Content-Type: application/json {"displayName":"Changed by Ari"} ``` **Consequence:** The request changes u2’s account. The response also exposes that account’s contents, including its password hash. **Smallest fix:** Check ownership before mutation or disclosure: ```js const account = accounts.get(req.params.id); if (!account || account.ownerId !== req.user.id) { return res.sendStatus(404); } ``` Returning the same status for missing and unauthorized records avoids revealing whether another user’s account exists. **Policy dependency:** This implements owner-only account editing. Any intended administrator or delegated access needs an explicit policy; none was supplied. ## 2. Unrestricted body assignment permits changes to protected account fields **Severity: High for account integrity.** A legitimate owner can overwrite ownership, identity, role, and credential fields. Privilege escalation through role or authentication compromise through passwordHash depends on downstream code that was not supplied. **Location:** account-routes.js, line 13: ```js Object.assign(account, req.body); ``` **Visible evidence:** Every enumerable request-body property is copied into the stored account. There is no writable-field allowlist. **Trigger:** Even after adding the ownership check, u1 can submit this request to their own account: ```json { "displayName": "Ari updated", "id": "replacement", "ownerId": "u2", "role": "admin", "passwordHash": "attacker-controlled" } ``` **Consequence:** Protected values are overwritten. Changing ownerId affects subsequent ownership decisions; changing id makes the stored object inconsistent with its Map key. The security impact of role and passwordHash depends on how they are consumed elsewhere. **Smallest fix:** Allow only the demonstrated profile-edit field, displayName, and validate its type. Ignore other fields so they remain unchanged: ```js const body = req.body; if (!body || typeof body !== 'object' || Array.isArray(body)) { return res.sendStatus(400); } if (Object.hasOwn(body, 'displayName')) { if (typeof body.displayName !== 'string') { return res.sendStatus(400); } account.displayName = body.displayName; } ``` **Policy dependency:** This assumes displayName is the only field writable through this endpoint. Additional writable fields require explicit authorization and validation rules. ## 3. Successful responses disclose password hashes **Severity: Medium.** Every successful request returns credential material. Real password hashes could enable offline password guessing if exposed. The supplied demo-hash values do not establish that usable production hashes are present. **Location:** account-routes.js, line 14: ```js return res.json(account); ``` **Visible evidence:** The returned object contains passwordHash. No response filtering occurs. **Trigger:** Any successful PATCH, including an owner’s ordinary display-name update. Before the ownership fix, this also discloses other users’ hashes. **Consequence:** Credential material reaches clients and potentially their logs or monitoring systems. Authentication and ownership checks do not justify returning password hashes. **Smallest fix:** Explicitly select the existing non-credential response fields: ```js return res.json({ id: account.id, ownerId: account.ownerId, displayName: account.displayName, role: account.role }); ``` This preserves the existing response fields except passwordHash and prevents unexpected stored properties from being serialized. ## Combined minimal patch The following handler incorporates each fix while preserving the route, authentication mechanism, and account store: ```js router.patch('/accounts/:id', (req, res) => { if (!req.user) return res.sendStatus(401); const account = accounts.get(req.params.id); if (!account || account.ownerId !== req.user.id) { return res.sendStatus(404); } const body = req.body; if (!body || typeof body !== 'object' || Array.isArray(body)) { return res.sendStatus(400); } if (Object.hasOwn(body, 'displayName')) { if (typeof body.displayName !== 'string') { return res.sendStatus(400); } account.displayName = body.displayName; } return res.json({ id: account.id, ownerId: account.ownerId, displayName: account.displayName, role: account.role }); }); ``` Object.hasOwn requires a compatible Node.js runtime. For an older runtime, use Object.prototype.hasOwnProperty.call(body, 'displayName'). ## Proposed regression tests — not executed No test framework was supplied. Use Vitest with Supertest, an Express test app with express.json(), and fresh account fixtures for each test. Supply authenticated identities through test-only middleware or the established authentication test harness; do not introduce a production authentication bypass. Direct assertions about passwordHash require test-only access to the stored fixtures. 1. **Permitted owner update:** Authenticate Ari as u1. PATCH /accounts/a1 with `{"displayName":"Ari updated"}`. Expect 200, the updated displayName, and unchanged id, ownerId, role, and passwordHash in storage. 2. **Cross-owner rejection and disclosure prevention:** Authenticate Ari as u1. PATCH Bo’s /accounts/a2 with `{"displayName":"Changed by Ari"}`. Expect 404, no account data or passwordHash in the response, and no change to Bo’s stored record. Authenticate Bo as u2 and update a2 successfully. Repeat in reverse: Bo cannot modify or obtain Ari’s account through this endpoint. No separate read route was supplied. 3. **Protected fields remain unchanged:** Authenticate Ari and PATCH a1 with the multi-field payload from finding 2. Expect 200 and the allowed displayName update. Assert that id remains a1, ownerId remains u1, role remains member, and passwordHash remains demo-hash-a. Verify that a subsequent request by Ari still succeeds and a request by Bo still fails. 4. **Sensitive response filtering:** For successful requests by both users, assert the exact response keys are id, ownerId, displayName, and role. Assert passwordHash is absent, including when explicitly included in the request. 5. **Invalid input does not mutate:** Send a numeric or null displayName, and an array body. Expect 400 and unchanged stored records. An empty object should return 200 with the unchanged, filtered account response. 6. **Authentication and missing-record behavior:** Without req.user, expect 401 and no mutation. With a valid identity and a nonexistent account ID, expect 404 with no account fields. Missing context that materially limits this review: the intended writable-field and administrator-access policies, Node.js version, request-body middleware, production credential storage, downstream consumers of role and passwordHash, and existing test infrastructure. Their implementations were not supplied and are not assessed as missing controls here.