fix(security): require auth + ownership on profile update (IDOR)

[hanapotski]

Summary

Fixes security audit finding #2 (Critical) — PUT /api/accounts/:userid had no auth middleware, so anyone could overwrite any user's profile (firstName, lastName, email, tenantId) just by hitting the endpoint with a target user id. Overwriting the email tied to a victim's login could be chained with the password-reset flow toward account takeover.

Fix

• Authenticate the route — add jwtSession.validateUser to PUT /:userid so it requires a valid session (401 otherwise).
• Enforce ownership in the controller — a user may only update their own profile (req.user.id === :userid), unless they hold an account-management admin role (admin / security_admin / global_admin). Returns 403 otherwise. Roles are read from the JWT sub claim, matching the existing stakeholder-access helper convention.

Compatibility

The only client caller — Profile.tsx self-edit — is unaffected: it edits the logged-in user's own id, and the jwt cookie is sent automatically on these same-origin (/api/...) requests (same mechanism the other protected admin services rely on).

Tests

Added controller tests covering:

• owner updating their own profile → allowed (200)
• non-owner, non-admin → blocked (403), service never called (the IDOR case)
• admin updating another user's profile → allowed (200)

tsc --noEmit passes. Note: account.test.ts has a pre-existing Object { vs { Jest-serializer snapshot mismatch (10 snapshots, identical on develop) unrelated to this change.

---

🤖 This PR was written by Claude on behalf of @hanapotski.

🤖 Generated with Claude Code

[VirginiaWu11]

Thanks for closing the unauthenticated IDOR here — the jwtSession.validateUser + ownership check is the right shape, and the added controller tests (owner self-edit / non-owner blocked / admin override) cover the intended cases well.

One gap I think needs to be closed before merge, plus a smaller cleanup suggestion:

🔴 Cross-tenant privilege escalation (blocking)

isAccountAdmin in account-controller.ts grants override access to anyone whose JWT sub contains admin or security_admin:

const isAccountAdmin =
  roles.has("admin") ||
  roles.has("security_admin") ||
  roles.has("global_admin");

But admin/security_admin are per-tenant roles — they come from the login_tenant join table, scoped to whichever tenant the user authenticated into (see account-service.ts's authenticate(), ~L330-372). The JWT itself never encodes which tenant those roles were granted for (jwt-session.ts's JwtPayload is just {email, sub}). And accountService.updateUserProfile does update login ... where id=$<userid> with no tenant comparison against the caller.

Net effect: a user who is admin/security_admin for one tenant only (e.g. mck.foodoasis.net) can still call PUT /api/accounts/:userid for a :userid belonging to a completely different tenant (e.g. LA County) and overwrite that user's profile, including their email — which chains into the same account-takeover path this PR is meant to close, just gated behind "admin of some tenant" instead of "unauthenticated."

Closing this cleanly needs either a tenant claim on the JWT (checked against the target user's actual tenant) or an explicit lookup of the target user's tenant_id in the controller, compared against the caller's tenant, before allowing the isAccountAdmin bypass. global_admin is fine as-is since it's a login-level (non-tenant-scoped) flag.

🟡 Minor: duplicated role-parsing

const roles = new Set(
  (req.user?.sub || req.user?.role || "").split(",").filter(Boolean)
);

This is a verbatim copy of getUserRoles in stakeholder-access.ts — worth exporting and importing that instead so the two authorization checks (this one and the stakeholder one) can't drift apart later, especially since the PR description already calls out matching that convention.

---

🤖 Review generated with Claude Code

[hanapotski]

Thanks for the careful review, @VirginiaWu11 — both points addressed in a1b8309.

🔴 Cross-tenant escalation

Fixed by narrowing the override to global_admin only and dropping admin/security_admin from it. As you noted, admin/security_admin are per-tenant roles from the login_tenant join and the JWT ({email, id, sub}) doesn't encode which tenant they were granted for, so trusting them here is unsafe. global_admin is a login-level, non-tenant-scoped flag, so it remains safe to honor.

I went with narrowing rather than adding a tenant claim + target-tenant lookup because there is currently no feature where a per-tenant admin edits another user's profile — the only client caller of PUT /accounts/:userid is the self-service Profile.tsx edit. So owner + global_admin fully covers the real use case, and adding tenant plumbing to the JWT would be speculative for a capability nothing uses (and JWT/auth changes carry deployment implications per the repo's security guidance). If per-tenant admin editing is ever needed, that's a deliberate follow-up: add a tenant claim to the JWT and compare it against the target user's tenant_id.

Added a regression test: a per-tenant admin (sub: "admin,security_admin" without global_admin) is now blocked with 403; global_admin can still edit another user.

🟡 Duplicated role-parsing

getUserRoles is now exported from stakeholder-access.ts and imported in the controller, so both authorization checks share one implementation.

tsc, lint, and the account + stakeholder test suites pass.

---

🤖 This reply was written by Claude on behalf of @hanapotski.

[VirginiaWu11]

Re-reviewed the update — both issues from the previous round are resolved:

• Cross-tenant escalation: the override is now scoped to global_admin only (admin/security_admin no longer bypass ownership), with a clear comment explaining why those two are excluded (per-tenant roles, no tenant claim on the JWT). New regression test blocks a per-tenant admin from updating another user's profile (cross-tenant escalation) covers exactly this case.
• Duplicated role-parsing: replaced with an import of the now-exported getUserRoles from stakeholder-access.ts — no more copy.

Verified independently in a clean worktree checked out from this PR's head (a1b83096):

• tsc --noEmit — clean
• npm run lint — clean
• npx jest --ci __test__/account.test.ts — all 4 new tests pass (lets a user update their own profile, blocks a user from updating another user's profile (IDOR), lets a global_admin update another user's profile, blocks a per-tenant admin from updating another user's profile (cross-tenant escalation)); the 10 failing snapshots are the same pre-existing Object {} vs {} formatting mismatch that exists on develop, unrelated to this change.
• No other importers of getUserRoles/stakeholder-access.ts are affected by the export change.

Looks good — approving.

---

🤖 Review generated with Claude Code