Skip to content

Commit bbf6688

Browse files
hanapotskiclaude
andcommitted
fix(security): require auth on GET /accounts/:email (enumeration)
GET /api/accounts/:email had no auth middleware, so it could be called anonymously. An earlier fix (finding #1) stopped it leaking the bcrypt password hash, but left the endpoint reachable without a session -- so it could still be used to confirm which emails have an account (200 vs 404) and to read that account's id, name, confirmation status and creation date (security audit finding #10). Server: - Gate the route with jwtSession.validateUserHasRequiredRoles, using the same role set as GET "/" (admin, security_admin, data_entry, global_admin). GET "/" already exposes strictly more account data to those roles, so this adds no new exposure for authenticated staff and removes all anonymous access. Client: - The only public caller was ForgotPassword's per-keystroke existence check, which is removed. It relied on the endpoint being anonymous and is redundant: the "account not found, please register" feedback is already surfaced on submit from the forgotPassword response (FORGOT_PASSWORD_ACCOUNT_NOT_FOUND). - The admin Features screen (isAdmin) is unaffected; it sends the jwt cookie automatically. Tests: - Add controller-level tests asserting the route's role guard rejects an unauthenticated caller (401), rejects an authenticated caller without a permitted role (401), and lets an admin through. tsc and lint pass clean on both workspaces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent a207cba commit bbf6688

3 files changed

Lines changed: 88 additions & 28 deletions

File tree

client/src/components/Account/ForgotPassword.tsx

Lines changed: 7 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import {
1010
} from "@mui/material";
1111
import Label from "components/Admin/ui/Label";
1212
import { Formik, FormikHelpers } from "formik";
13-
import debounce from "lodash.debounce";
1413
import { useNavigate, useParams } from "react-router-dom";
1514
import { palette } from "theme/palette";
1615
import * as Yup from "yup";
@@ -37,24 +36,12 @@ const ForgotPassword: React.FC<ForgotPasswordProps> = () => {
3736
const { email } = useParams<{ email?: string }>();
3837
const navigate = useNavigate();
3938

40-
const debouncedEmailValidation = debounce(
41-
async (
42-
value: string,
43-
setFieldError: (field: string, message: string) => void
44-
) => {
45-
try {
46-
await accountService.getByEmail(value);
47-
return;
48-
} catch (e) {
49-
console.error(e);
50-
setFieldError(
51-
"email",
52-
"Account not found. If you want to create a new account with this email, please register."
53-
);
54-
}
55-
},
56-
500
57-
);
39+
// Note: we intentionally do NOT probe whether the account exists as the user
40+
// types. Looking an email up by GET /api/accounts/:email requires an
41+
// authenticated staff role (security audit finding #10), and doing so would
42+
// also leak account existence to anonymous visitors. The "account not found"
43+
// feedback is surfaced on submit instead, from the forgotPassword response
44+
// (FORGOT_PASSWORD_ACCOUNT_NOT_FOUND) below.
5845

5946
return (
6047
<PageWrapper>
@@ -134,15 +121,8 @@ const ForgotPassword: React.FC<ForgotPasswordProps> = () => {
134121
handleBlur,
135122
handleSubmit,
136123
isSubmitting,
137-
setFieldError,
138124
isValid,
139125
}) => {
140-
const handleEmailChange = (
141-
e: React.ChangeEvent<HTMLInputElement>
142-
) => {
143-
handleChange(e);
144-
debouncedEmailValidation(e.target.value, setFieldError);
145-
};
146126
return (
147127
<form
148128
noValidate
@@ -172,7 +152,7 @@ const ForgotPassword: React.FC<ForgotPasswordProps> = () => {
172152
autoComplete="email"
173153
autoFocus
174154
value={values.email}
175-
onChange={handleEmailChange}
155+
onChange={handleChange}
176156
onBlur={handleBlur}
177157
helperText={touched.email ? errors.email : ""}
178158
error={touched.email && Boolean(errors.email)}

server/__test__/account.test.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
1+
import jwt from "jsonwebtoken";
12
import accountController from "../app/controllers/account-controller";
23
import accountService from "../app/services/account-service";
34
import loginService from "../app/services/logins-service";
5+
import jwtSession from "../middleware/jwt-session";
46
import { mockResponse, mockRequest, mockNext } from "./utils";
57

68
jest.mock("../app/services/account-service");
@@ -775,3 +777,65 @@ describe("Account", () => {
775777
expect(res.status).toHaveBeenCalledWith(403);
776778
});
777779
});
780+
781+
// GET /api/accounts/:email must not be reachable without an authenticated staff
782+
// role. Previously it was public, letting anyone enumerate accounts by email and
783+
// harvest their id/name/confirmation/creation date (security audit finding #10).
784+
// These exercise the role-check middleware the route is wired with.
785+
describe("GET /accounts/:email authorization (finding #10)", () => {
786+
const jwtSecret = process.env.JWT_SECRET || "mark it zero";
787+
const guard = jwtSession.validateUserHasRequiredRoles([
788+
"admin",
789+
"security_admin",
790+
"data_entry",
791+
"global_admin",
792+
]);
793+
794+
const signToken = (payload: object) =>
795+
jwt.sign(payload, jwtSecret, { algorithm: "HS256" });
796+
797+
it("rejects an unauthenticated request with 401", async () => {
798+
const res = mockResponse();
799+
const req = mockRequest({
800+
headers: {},
801+
cookies: {},
802+
params: { email: "victim@test.com" },
803+
});
804+
const next = mockNext();
805+
806+
await guard(req, res, next);
807+
808+
expect(next).not.toHaveBeenCalled();
809+
expect(res.status).toHaveBeenCalledWith(401);
810+
});
811+
812+
it("rejects an authenticated caller lacking a permitted role with 401", async () => {
813+
const res = mockResponse();
814+
const req = mockRequest({
815+
headers: {},
816+
cookies: { jwt: signToken({ email: "vol@test.com", sub: "coordinator" }) },
817+
params: { email: "victim@test.com" },
818+
});
819+
const next = mockNext();
820+
821+
await guard(req, res, next);
822+
823+
expect(next).not.toHaveBeenCalled();
824+
expect(res.status).toHaveBeenCalledWith(401);
825+
});
826+
827+
it("allows an authenticated admin through to the handler", async () => {
828+
const res = mockResponse();
829+
const req = mockRequest({
830+
headers: {},
831+
cookies: { jwt: signToken({ email: "admin@test.com", sub: "admin" }) },
832+
params: { email: "someone@test.com" },
833+
});
834+
const next = mockNext();
835+
836+
await guard(req, res, next);
837+
838+
expect(next).toHaveBeenCalledTimes(1);
839+
expect(res.status).not.toHaveBeenCalled();
840+
});
841+
});

server/app/routes/account-router.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,22 @@ router.put(
5151
accountController.updateUserProfile
5252
);
5353

54-
router.get("/:email", accountController.getByEmail);
54+
// Requires an authenticated staff role. Previously this was public, which let
55+
// any anonymous caller enumerate which emails have accounts (200 vs 404) and
56+
// harvest the account id, name, confirmation status and creation date for any
57+
// known email (security audit finding #10). The only legitimate caller is the
58+
// admin Features screen (isAdmin), which sends the jwt cookie automatically;
59+
// the role set mirrors GET "/" (which already exposes strictly more account
60+
// data to the same roles).
61+
router.get(
62+
"/:email",
63+
jwtSession.validateUserHasRequiredRoles([
64+
"admin",
65+
"security_admin",
66+
"data_entry",
67+
"global_admin",
68+
]),
69+
accountController.getByEmail
70+
);
5571

5672
export default router;

0 commit comments

Comments
 (0)