mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-10 13:22:14 +00:00
fix(mcp): connecting an assistant asks for a recent sign-in or re-proof
Allowing an AI assistant hands it a standing credential to the record that outlives the session, the same class of act as minting a token, but the consent took a live session alone. A stolen session could connect its own assistant and keep reading after the owner signed everything out. The consent page now shows the Allow form only when the session signed in or re-proved within five minutes (with the second factor on an account that has one). Otherwise it links to a new confirm page that runs the same re-proof dialog as the rest of the app and returns to the exact consent request. The allow decision checks again and mints nothing without it; deny needs no proof. The confirm page only ever returns to the consent endpoint.
This commit is contained in:
1 parent
a294d7f684
commit
1900792756
14 files changed
+358
-3
No files matched your search
@@ -1,7 +1,7 @@
|
||||
/**
|
||||
* Authorize endpoint — consent gate + audience + PKCE + redirect-URI binding.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
|
||||
process.env.API_TOKEN_HMAC_KEY = "x".repeat(48);
|
||||
process.env.APP_URL = "https://health.example";
|
||||
@@ -21,6 +21,22 @@ vi.mock("@/lib/rate-limit", () => ({
|
||||
})),
|
||||
rateLimitHeaders: vi.fn(() => ({})),
|
||||
}));
|
||||
// The consent reads the session's own proof stamps. By default the session
|
||||
// signed in a moment ago, which is a recent proof on an account without a
|
||||
// second factor; the cases below age it.
|
||||
vi.mock("@/lib/db", () => ({
|
||||
prisma: {
|
||||
session: {
|
||||
findUnique: vi.fn(async () => ({
|
||||
createdAt: new Date(),
|
||||
mfaVerifiedAt: null,
|
||||
reproofAt: null,
|
||||
})),
|
||||
},
|
||||
webauthnMfaCredential: { count: vi.fn(async () => 0) },
|
||||
passkey: { count: vi.fn(async () => 0) },
|
||||
},
|
||||
}));
|
||||
vi.mock("@/lib/app-settings", () => ({
|
||||
isApiGloballyEnabled: vi.fn(async () => true),
|
||||
}));
|
||||
@@ -30,6 +46,7 @@ import { getSession } from "@/lib/auth/session";
|
||||
import { isApiGloballyEnabled } from "@/lib/app-settings";
|
||||
import { registerDcrClient } from "@/lib/mcp/oauth/clients";
|
||||
import { s256Challenge } from "@/lib/mcp/oauth/pkce";
|
||||
import { prisma } from "@/lib/db";
|
||||
|
||||
const REDIRECT = "https://claude.ai/api/mcp/auth_callback";
|
||||
const VERIFIER = "a".repeat(64);
|
||||
@@ -327,6 +344,82 @@ describe("POST /authorize — decision", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("authorize — allowing a connection needs a recent proof", () => {
|
||||
// A queued stale stamp a case did not read must not leak into the next one.
|
||||
afterEach(() => {
|
||||
vi.mocked(prisma.session.findUnique).mockReset();
|
||||
});
|
||||
|
||||
function staleSession() {
|
||||
vi.mocked(prisma.session.findUnique).mockResolvedValueOnce({
|
||||
createdAt: new Date(Date.now() - 60 * 60 * 1000),
|
||||
mfaVerifiedAt: null,
|
||||
reproofAt: null,
|
||||
} as never);
|
||||
}
|
||||
const allow = {
|
||||
response_type: "code",
|
||||
client_id: CLIENT.clientId,
|
||||
redirect_uri: REDIRECT,
|
||||
code_challenge: CHALLENGE,
|
||||
code_challenge_method: "S256",
|
||||
scope: "health:read",
|
||||
state: "xyz",
|
||||
resource: RESOURCE,
|
||||
decision: "allow",
|
||||
};
|
||||
|
||||
it("GET shows a confirm link instead of the form when the session is an hour old", async () => {
|
||||
signedIn();
|
||||
staleSession();
|
||||
const res = await GET(getReq(authorizeUrl()) as never);
|
||||
expect(res.status).toBe(200);
|
||||
const body = await res.text();
|
||||
expect(body).toMatch(/Confirm it is you/);
|
||||
expect(body).not.toMatch(/name="decision"/);
|
||||
const href = body.match(/href="([^"]+)"/)?.[1] ?? "";
|
||||
const next =
|
||||
new URL(href.replace(/&/g, "&"), "https://x").searchParams.get(
|
||||
"next",
|
||||
) ?? "";
|
||||
expect(href.startsWith("/confirm-access?next=")).toBe(true);
|
||||
// Back to this exact consent request.
|
||||
expect(next).toBe(
|
||||
new URL(authorizeUrl()).pathname + new URL(authorizeUrl()).search,
|
||||
);
|
||||
});
|
||||
|
||||
it("POST allow on an hour-old session mints nothing", async () => {
|
||||
signedIn();
|
||||
staleSession();
|
||||
const res = await POST(postReq(allow) as never);
|
||||
expect(res.status).toBe(401);
|
||||
expect(res.headers.get("location")).toBeNull();
|
||||
expect(await res.text()).toMatch(/confirm-access\?next=/);
|
||||
});
|
||||
|
||||
it("an account with a second factor needs the factor, not a young session", async () => {
|
||||
vi.mocked(getSession).mockResolvedValue({
|
||||
session: {
|
||||
id: "s1",
|
||||
expiresAt: new Date(Date.now() + 1e6),
|
||||
actingAsUserId: null,
|
||||
recordEpoch: 0,
|
||||
},
|
||||
user: { id: "user-1", totpConfirmedAt: new Date("2026-01-01") } as never,
|
||||
});
|
||||
const res = await POST(postReq(allow) as never);
|
||||
expect(res.status).toBe(401);
|
||||
});
|
||||
|
||||
it("deny needs no proof", async () => {
|
||||
signedIn();
|
||||
staleSession();
|
||||
const res = await POST(postReq({ ...allow, decision: "deny" }) as never);
|
||||
expect(res.status).toBe(303);
|
||||
});
|
||||
});
|
||||
|
||||
describe("authorize CSP — the consent form may lead only to the validated redirect", () => {
|
||||
function csp(res: Response): string {
|
||||
return res.headers.get("content-security-policy") ?? "";
|
||||
|
||||
@@ -11,7 +11,9 @@
|
||||
* a GET (confused-deputy mitigation: a third party cannot silently
|
||||
* obtain a grant — an authenticated human must click "Allow").
|
||||
* POST — the consent decision. Requires a session, re-validates every
|
||||
* parameter, and on "Allow" mints a single-use, short-lived,
|
||||
* parameter, and on "Allow" asks for a recent sign-in or re-proof
|
||||
* (the GET shows a confirm link instead of the form without one),
|
||||
* then mints a single-use, short-lived,
|
||||
* self-describing authorization code bound to this user + client +
|
||||
* redirect URI + PKCE challenge + scope + audience, then 303s back.
|
||||
*
|
||||
@@ -30,6 +32,8 @@ import { annotate } from "@/lib/logging/context";
|
||||
import { withBackgroundEvent } from "@/lib/logging/background";
|
||||
import { auditLog } from "@/lib/auth/audit";
|
||||
import { getSession } from "@/lib/auth/session";
|
||||
import { assertRecentCookieProof } from "@/lib/api-handler";
|
||||
import { StepUpRequiredError } from "@/lib/api-errors";
|
||||
import { checkAuthSurfaceRateLimit, rateLimitHeaders } from "@/lib/rate-limit";
|
||||
import { isApiGloballyEnabled } from "@/lib/app-settings";
|
||||
import {
|
||||
@@ -88,6 +92,42 @@ function html(body: string, status = 200): Response {
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Has this session signed in or re-proved recently enough to grant access?
|
||||
*
|
||||
* Allowing a connection hands a standing credential to the record to a third
|
||||
* party, one that outlives the session: the same class of act as minting a
|
||||
* token or making a share link, and it takes the same gate
|
||||
* (`assertRecentCookieProof`: a sign-in or re-proof within five minutes, with
|
||||
* the second factor on an account that has one). Without it, whoever held a
|
||||
* stolen session could connect their own assistant and keep reading the
|
||||
* record after the owner signed everything out.
|
||||
*/
|
||||
async function hasRecentProof(
|
||||
session: NonNullable<Awaited<ReturnType<typeof getSession>>>,
|
||||
): Promise<boolean> {
|
||||
try {
|
||||
await assertRecentCookieProof(session.user, session.session.id);
|
||||
return true;
|
||||
} catch (err) {
|
||||
if (err instanceof StepUpRequiredError) return false;
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The page shown instead of the consent form while the session carries no
|
||||
* recent proof. It links to the confirm page, which runs the same re-proof
|
||||
* dialog as the rest of the app and then comes back to this exact request.
|
||||
*/
|
||||
function reproofPage(clientName: string, returnTo: string, status = 200) {
|
||||
const confirmHref = `/confirm-access?next=${encodeURIComponent(returnTo)}`;
|
||||
return html(
|
||||
`<main><h1>Confirm it is you</h1><p>Before <strong>${htmlEscape(clientName)}</strong> can connect to your HealthLog data, confirm it is you. This is asked when you have not signed in or confirmed in the last five minutes.</p><p><a href="${htmlEscape(confirmHref)}">Confirm and continue</a></p></main>`,
|
||||
status,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* The scopes this surface grants for a request. `health:read` is always
|
||||
* granted (the floor). `health:write` is granted ONLY when explicitly
|
||||
@@ -359,6 +399,11 @@ async function authorizeGet(request: NextRequest): Promise<Response> {
|
||||
);
|
||||
}
|
||||
|
||||
if (!(await hasRecentProof(session))) {
|
||||
annotate({ action: { name: "mcp.oauth.authorize.reproof_required" } });
|
||||
return reproofPage(v.clientName, url.pathname + url.search);
|
||||
}
|
||||
|
||||
annotate({ action: { name: "mcp.oauth.authorize.consent_shown" } });
|
||||
// Consent screen — a plain POST form (no inline script/style, CSP-safe).
|
||||
const hidden = (name: string, value: string) =>
|
||||
@@ -499,6 +544,23 @@ async function authorizePost(request: NextRequest): Promise<Response> {
|
||||
});
|
||||
}
|
||||
|
||||
// The same gate the consent page asked for, checked again on the decision
|
||||
// itself: the page may have been left open past the window, or the POST
|
||||
// built by hand. Denying needs no proof, so only "allow" is gated.
|
||||
if (!(await hasRecentProof(session))) {
|
||||
annotate({ action: { name: "mcp.oauth.authorize.reproof_required" } });
|
||||
const back = new URL("/api/mcp/oauth/authorize", request.url);
|
||||
back.searchParams.set("response_type", "code");
|
||||
back.searchParams.set("client_id", v.clientId);
|
||||
back.searchParams.set("redirect_uri", v.redirectUri);
|
||||
back.searchParams.set("code_challenge", v.codeChallenge);
|
||||
back.searchParams.set("code_challenge_method", "S256");
|
||||
back.searchParams.set("scope", v.scope);
|
||||
back.searchParams.set("resource", v.resource);
|
||||
if (v.state !== undefined) back.searchParams.set("state", v.state);
|
||||
return reproofPage(v.clientName, back.pathname + back.search, 401);
|
||||
}
|
||||
|
||||
const code = signArtifact(
|
||||
"authCode",
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user