diff --git a/web/api/delete-account/index.ts b/web/api/delete-account/index.ts index 827c9d1991..4570433edc 100644 --- a/web/api/delete-account/index.ts +++ b/web/api/delete-account/index.ts @@ -1,13 +1,32 @@ +import { isSameOriginRequest } from "@/api/helpers/csrf"; import { errorResponse } from "@/api/helpers/errors"; -import { logger } from "@/lib/logger"; -import { urls } from "@/lib/urls"; import { auth0 } from "@/lib/auth0"; +import { logger } from "@/lib/logger"; import { ManagementClient } from "auth0"; -import { NextRequest } from "next/server"; -import { getAppUrlFromRequest } from "../helpers/utils"; +import { NextRequest, NextResponse } from "next/server"; +/** + * Irreversibly deletes the caller's Auth0 identity. + * + * Mounted on POST only, and rejects any request the browser reports as cross-site. + * The session cookie is `SameSite=Lax`, so a GET mount here would let any attacker + * page destroy a logged-in developer's identity with a single top-level navigation + * (``, `window.open`, meta-refresh) — a valid session cookie alone does not + * prove the request came from our own UI. See `api/helpers/csrf.ts`. + * + * The caller deletes the Hasura user row first, POSTs here, then navigates to + * `/api/auth/logout` to clear the (still-valid, stateless) session cookie. + */ export const deleteAccount = async (req: NextRequest) => { - const appUrl = await getAppUrlFromRequest(req); + if (!(await isSameOriginRequest(req))) { + return errorResponse({ + statusCode: 403, + code: "cross_origin_request", + detail: "Account deletion must be initiated from the developer portal", + req, + }); + } + if ( !process.env.AUTH0_CLIENT_ID || !process.env.AUTH0_CLIENT_SECRET || @@ -58,5 +77,8 @@ export const deleteAccount = async (req: NextRequest) => { }); } - return Response.redirect(new URL(urls.logout(), appUrl), 307); + // No redirect: the caller is a `fetch()`, and returning a 307 here would make it + // replay the POST against `/api/auth/logout`. The client navigates to logout + // itself, passing the host it is on so the sibling-domain return lands correctly. + return new NextResponse(null, { status: 204 }); }; diff --git a/web/api/helpers/csrf.ts b/web/api/helpers/csrf.ts new file mode 100644 index 0000000000..f926ce9460 --- /dev/null +++ b/web/api/helpers/csrf.ts @@ -0,0 +1,53 @@ +import { getAllowedAppBaseUrls } from "@/lib/app-base-url"; +import { NextRequest } from "next/server"; +import "server-only"; +import { getAppUrlFromRequest } from "./utils"; + +const toOrigin = (value: string | undefined): string | undefined => { + if (!value) return undefined; + try { + return new URL(value).origin; + } catch { + return undefined; + } +}; + +/** + * Cross-site request guard for cookie-authenticated, state-changing endpoints. + * + * The Auth0 session cookie is `SameSite=Lax` (SDK default — `lib/auth0.ts` sets no + * `session.cookie` override), so the browser attaches it to any cross-site + * *top-level navigation*. "A valid session cookie is present" is therefore not + * evidence that the request came from our own UI, and a destructive handler gated + * on the session alone is CSRF-able by a plain `` on an attacker's page. + * + * Endpoints using this guard must also be mounted on a non-idempotent method: + * `Lax` never sends the cookie on a cross-site POST, so the method restriction is + * the primary defence and this check is the layer that survives a future change to + * the cookie's `SameSite` attribute. + * + * `Sec-Fetch-Site` is set by the browser and cannot be forged from page JS. Only + * `same-origin` is accepted — `same-site` would let a takeover of any sibling + * subdomain reach the endpoint, and our own callers always use a relative URL. + * + * Fail-closed: a request carrying neither `Sec-Fetch-Site` nor `Origin` cannot be + * shown to be same-origin. Every browser that can render the portal sends at least + * one of them on a `fetch()`, so rejecting costs no real client. + */ +export const isSameOriginRequest = async ( + req: NextRequest, +): Promise => { + const secFetchSite = req.headers.get("sec-fetch-site"); + if (secFetchSite) return secFetchSite === "same-origin"; + + const origin = toOrigin(req.headers.get("origin") ?? undefined); + if (!origin) return false; + + const configured = getAllowedAppBaseUrls(); + const allowed = [ + await getAppUrlFromRequest(req), + ...(Array.isArray(configured) ? configured : [configured]), + ].flatMap((value) => toOrigin(value) ?? []); + + return allowed.includes(origin); +}; diff --git a/web/app/api/auth/delete-account/route.ts b/web/app/api/auth/delete-account/route.ts index 97fe096f0c..35bc0de00d 100644 --- a/web/app/api/auth/delete-account/route.ts +++ b/web/app/api/auth/delete-account/route.ts @@ -1 +1,4 @@ -export { deleteAccount as GET } from "@/api/delete-account"; +// POST only. Deleting the Auth0 identity is irreversible and authenticated by the +// `SameSite=Lax` session cookie alone, which the browser would attach to a +// cross-site top-level GET navigation. Next returns 405 for every other method. +export { deleteAccount as POST } from "@/api/delete-account"; diff --git a/web/proxy.ts b/web/proxy.ts index 6329a3274d..906c9b959b 100644 --- a/web/proxy.ts +++ b/web/proxy.ts @@ -367,6 +367,8 @@ export async function proxy(request: NextRequest) { // Auth SDK routes pass straight through: login/logout/callback/profile under // `/api/auth/*`, plus our custom login-callback / delete-account handlers. + // These carry their own request-origin checks — `delete-account` is POST-only + // and same-origin gated (`api/helpers/csrf.ts`) because nothing here does it. if (pathname.startsWith("/api/auth/")) { return authRes; } diff --git a/web/scenes/Portal/Profile/DangerZone/DeleteAccountDialog/index.tsx b/web/scenes/Portal/Profile/DangerZone/DeleteAccountDialog/index.tsx index c7005d4798..f123debb95 100644 --- a/web/scenes/Portal/Profile/DangerZone/DeleteAccountDialog/index.tsx +++ b/web/scenes/Portal/Profile/DangerZone/DeleteAccountDialog/index.tsx @@ -17,6 +17,7 @@ import { useForm } from "react-hook-form"; import { toast } from "react-toastify"; import * as yup from "yup"; import { useMutation } from "@apollo/client/react"; +import { deleteAuth0Identity } from "@/scenes/common/Profile/DangerZone/DeleteAccountDialog/delete-auth0-identity"; import { DeleteAccountDocument } from "@/scenes/common/Profile/DangerZone/DeleteAccountDialog/graphql/client/delete-account.generated"; const DELETE_WORD = "DELETE"; @@ -60,8 +61,9 @@ export const DeleteAccountDialog = (props: DialogProps) => { user_id: user.hasura.id, }, }); + await deleteAuth0Identity(); toast.success("Account Deleted!"); - window.location.href = urls.api.authDeleteAccount(); + window.location.href = urls.logout(window.location.origin); } catch (e) { console.error("Delete Account Dialog: ", e); toast.error("Error deleting account"); diff --git a/web/scenes/PortalV3/Profile/DangerZone/DeleteAccountDialog/index.tsx b/web/scenes/PortalV3/Profile/DangerZone/DeleteAccountDialog/index.tsx index 1113b2c376..c1d9100141 100644 --- a/web/scenes/PortalV3/Profile/DangerZone/DeleteAccountDialog/index.tsx +++ b/web/scenes/PortalV3/Profile/DangerZone/DeleteAccountDialog/index.tsx @@ -6,6 +6,7 @@ import { FormDialog } from "@/components/FormDialog"; import { AlertIcon } from "@/components/Icons/AlertIcon"; import { Auth0SessionUser } from "@/lib/types"; import { urls } from "@/lib/urls"; +import { deleteAuth0Identity } from "@/scenes/common/Profile/DangerZone/DeleteAccountDialog/delete-auth0-identity"; import { DeleteAccountDocument } from "@/scenes/common/Profile/DangerZone/DeleteAccountDialog/graphql/client/delete-account.generated"; import { useUser } from "@auth0/nextjs-auth0/client"; import { useMutation } from "@apollo/client/react"; @@ -58,8 +59,9 @@ export const DeleteAccountDialog = (props: DialogProps) => { user_id: user.hasura.id, }, }); + await deleteAuth0Identity(); toast.success("Account Deleted!"); - window.location.href = urls.api.authDeleteAccount(); + window.location.href = urls.logout(window.location.origin); } catch (e) { console.error("Delete Account Dialog: ", e); toast.error("Error deleting account"); diff --git a/web/scenes/common/Profile/DangerZone/DeleteAccountDialog/delete-auth0-identity.ts b/web/scenes/common/Profile/DangerZone/DeleteAccountDialog/delete-auth0-identity.ts new file mode 100644 index 0000000000..da2337717d --- /dev/null +++ b/web/scenes/common/Profile/DangerZone/DeleteAccountDialog/delete-auth0-identity.ts @@ -0,0 +1,22 @@ +import { urls } from "@/lib/urls"; + +/** + * Deletes the caller's Auth0 identity after the Hasura user row is gone. + * + * Must be a POST: the endpoint is authenticated by the `SameSite=Lax` session + * cookie, which the browser would also attach to a cross-site top-level GET + * navigation, so a navigable delete is CSRF-able from any attacker page. The + * response is a 204 — logout navigation is the caller's job. + */ +export const deleteAuth0Identity = async (): Promise => { + const response = await fetch(urls.api.authDeleteAccount(), { + method: "POST", + credentials: "same-origin", + }); + + if (!response.ok) { + throw new Error( + `Failed to delete Auth0 identity: ${response.status} ${response.statusText}`, + ); + } +}; diff --git a/web/tests/integration/auth/delete-account.test.ts b/web/tests/integration/auth/delete-account.test.ts index e79336f502..006cfc28de 100644 --- a/web/tests/integration/auth/delete-account.test.ts +++ b/web/tests/integration/auth/delete-account.test.ts @@ -1,9 +1,9 @@ import { deleteAccount } from "@/api/delete-account"; -import { Auth0User } from "@/lib/types"; -import { urls } from "@/lib/urls"; import { auth0 } from "@/lib/auth0"; +import { Auth0User } from "@/lib/types"; import { NextRequest } from "next/server"; +// #region Mocks const validSessionUser = { email: "test@world.org", email_verified: true, @@ -23,52 +23,134 @@ jest.mock("@/lib/auth0", () => ({ })); const getSession = auth0.getSession as jest.Mock; -const updateSession = auth0.updateSession as jest.Mock; jest.mock("../../../lib/logger", () => ({ logger: { + info: jest.fn(), error: jest.fn(), warn: jest.fn(), }, })); +const usersDelete = jest.fn(); + jest.mock("auth0", () => ({ ManagementClient: jest.fn().mockImplementation(() => ({ - users: { - delete: jest.fn(() => Promise.resolve()), - }, + users: { delete: (...args: unknown[]) => usersDelete(...args) }, })), })); +// #endregion -describe("test /delete-account", () => { - beforeEach(() => { - // Reset mocks before each test - (getSession as jest.Mock).mockReset(); - (updateSession as jest.Mock).mockReset(); +// #region Test Data +const APP_ORIGIN = "http://localhost:3000"; + +const createMockRequest = (headers: Record = {}) => + new NextRequest(`${APP_ORIGIN}/api/auth/delete-account`, { + method: "POST", + headers, }); - it("should return 401 if session user id is not found", async () => { - const mockReq = {} as unknown as NextRequest; - (getSession as jest.Mock).mockResolvedValue(null); - const response = await deleteAccount(mockReq); - const body = await response.json(); - expect(getSession).toHaveReturned(); - expect(response.status).toEqual(401); - expect(body.code).toEqual("unauthorized"); +const sameOriginRequest = () => + createMockRequest({ "sec-fetch-site": "same-origin" }); +// #endregion + +beforeEach(() => { + jest.clearAllMocks(); + usersDelete.mockResolvedValue(undefined); + getSession.mockResolvedValue({ user: validSessionUser }); +}); + +// #region Cross-site request rejection +// The session cookie is SameSite=Lax, so it rides along on any cross-site +// top-level navigation. These branches are what stops an attacker page from +// destroying a logged-in developer's Auth0 identity. +describe("/api/auth/delete-account [cross-site requests]", () => { + it("rejects a request the browser reports as cross-site", async () => { + const response = await deleteAccount( + createMockRequest({ "sec-fetch-site": "cross-site" }), + ); + + expect(response.status).toEqual(403); + expect((await response.json()).code).toEqual("cross_origin_request"); + expect(usersDelete).not.toHaveBeenCalled(); }); - it("Should successfully delete account", async () => { - const mockReq = { - json: () => Promise.resolve(), - } as unknown as NextRequest; - - (getSession as jest.Mock).mockResolvedValue({ user: validSessionUser }); - const response = await deleteAccount(mockReq); - expect(getSession).toHaveReturned(); - expect(response.status).toEqual(307); - console.log(response.headers.get("location")); - expect(response.headers.get("location")).toEqual( - new URL(urls.logout(), process.env.NEXT_PUBLIC_APP_URL).toString(), + it("rejects a same-site request from a sibling subdomain", async () => { + const response = await deleteAccount( + createMockRequest({ "sec-fetch-site": "same-site" }), ); + + expect(response.status).toEqual(403); + expect(usersDelete).not.toHaveBeenCalled(); + }); + + it("rejects a user-initiated top-level navigation", async () => { + // `none` means the URL was typed/bookmarked rather than issued by our UI — + // the shape a meta-refresh or `window.open` delivery lands as. + const response = await deleteAccount( + createMockRequest({ "sec-fetch-site": "none" }), + ); + + expect(response.status).toEqual(403); + expect(usersDelete).not.toHaveBeenCalled(); + }); + + it("fails closed when the request carries no origin metadata", async () => { + const response = await deleteAccount(createMockRequest()); + + expect(response.status).toEqual(403); + expect(usersDelete).not.toHaveBeenCalled(); + }); + + it("rejects a foreign Origin when Sec-Fetch-Site is absent", async () => { + const response = await deleteAccount( + createMockRequest({ origin: "https://evil.example" }), + ); + + expect(response.status).toEqual(403); + expect(usersDelete).not.toHaveBeenCalled(); + }); + + it("accepts a matching Origin when Sec-Fetch-Site is absent", async () => { + const response = await deleteAccount( + createMockRequest({ origin: APP_ORIGIN }), + ); + + expect(response.status).toEqual(204); + expect(usersDelete).toHaveBeenCalledWith({ id: validSessionUser.sub }); + }); +}); +// #endregion + +// #region Session and deletion outcomes +describe("/api/auth/delete-account [same-origin requests]", () => { + it("returns 401 if session user id is not found", async () => { + getSession.mockResolvedValue(null); + + const response = await deleteAccount(sameOriginRequest()); + + expect(response.status).toEqual(401); + expect((await response.json()).code).toEqual("unauthorized"); + expect(usersDelete).not.toHaveBeenCalled(); + }); + + it("deletes the Auth0 identity and returns 204", async () => { + const response = await deleteAccount(sameOriginRequest()); + + expect(usersDelete).toHaveBeenCalledWith({ id: validSessionUser.sub }); + expect(response.status).toEqual(204); + // The client, not the server, drives the logout navigation: a redirect here + // would make the caller's fetch replay the POST against /api/auth/logout. + expect(response.headers.get("location")).toBeNull(); + }); + + it("surfaces a 500 when Auth0 rejects the deletion", async () => { + usersDelete.mockRejectedValue(new Error("auth0 down")); + + const response = await deleteAccount(sameOriginRequest()); + + expect(response.status).toEqual(500); + expect((await response.json()).code).toEqual("internal_server_error"); }); }); +// #endregion