HTTP error answers carry fixed text, never an error's message
Soul pass 3 (R1): with no error handler, Fastify answered a 5xx with the
error's message. A store failure under a misbound database URL reached an
anonymous caller as `getaddrinfo ENOTFOUND <host>` or `database "<db>" does
not exist`, and a misbound URL puts part of the password in either. The same
default quoted the request body in a JSON parse error (Node's JSON.parse
message carries the input), the Content-Type header in a 415, and the path
in a 404.
buildApp now sets both handlers. A 5xx answers `{ error: "internal_error" }`
and writes one stderr line naming the method, the route pattern and the
error code. A schema failure answers the failed rules by schema path and
Ajv message, which come from the route's schema. Any other 4xx answers
`invalid_request` with the error code. An unknown route answers `not_found`.
The app's own answers had two more echoes: `unknown_app` quoted the slug,
and a failed provider callback put the caught error's text in the browser
page and the backend handoff (the JSON answer was already fixed). Both are
fixed text now; the full callback error stays in the audit event.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
376dfbd2c2
commit
33afcbf364
2 changed files with 207 additions and 11 deletions
49
src/app.ts
49
src/app.ts
|
|
@ -1,6 +1,6 @@
|
|||
import { createHmac, randomUUID } from "node:crypto";
|
||||
import cors from "@fastify/cors";
|
||||
import Fastify, { type FastifyInstance } from "fastify";
|
||||
import Fastify, { type FastifyError, type FastifyInstance } from "fastify";
|
||||
import { type AppCaller, isAppCaller, resolveAppCaller } from "./app-caller.js";
|
||||
import { deliverBackendHandoff, type BackendHandoffPayload } from "./backend-handoff.js";
|
||||
import {
|
||||
|
|
@ -532,7 +532,7 @@ export async function startOAuthFlow(
|
|||
const { config, keys } = ctx;
|
||||
const profile = getAppProfile(input.appSlug);
|
||||
if (!profile) {
|
||||
return { statusCode: 404, body: { error: "unknown_app", detail: `No app '${input.appSlug}' is registered.` } };
|
||||
return { statusCode: 404, body: { error: "unknown_app", detail: "No app is registered under that slug." } };
|
||||
}
|
||||
|
||||
if (!supportsProvider(profile, provider)) {
|
||||
|
|
@ -736,6 +736,31 @@ export async function buildApp(options: BuildAppOptions = {}): Promise<FastifyIn
|
|||
...(options.oauthRuntimes ?? {}),
|
||||
};
|
||||
const app = Fastify({ logger: false });
|
||||
// Fastify's default handlers answer with the error's message, and those
|
||||
// messages carry input: pg and Node name the database host or the database
|
||||
// (which a misbound URL fills with part of a secret), JSON.parse quotes the
|
||||
// request body, and the media-type and not-found errors quote a header and
|
||||
// the path. So every framework answer here is built from fixed text, the
|
||||
// error code and the route's own schema, never from the message.
|
||||
app.setErrorHandler((error: FastifyError, request, reply) => {
|
||||
const status = error.statusCode !== undefined && error.statusCode >= 400 && error.statusCode < 600 ? error.statusCode : 500;
|
||||
const code = typeof error.code === "string" ? error.code : "error";
|
||||
reply.code(status);
|
||||
if (status >= 500) {
|
||||
console.error(`Heimdall ${request.method} ${request.routeOptions.url ?? "(no route)"} failed (${code}).`);
|
||||
return { error: "internal_error" };
|
||||
}
|
||||
if (error.validation) {
|
||||
// Ajv's messages and schema paths come from the route's schema, not the request.
|
||||
const failures = error.validation.map((failure) => `${failure.schemaPath} ${failure.message ?? failure.keyword}`);
|
||||
return { error: "invalid_request", detail: `${error.validationContext ?? "request"}: ${failures.join("; ")}` };
|
||||
}
|
||||
return { error: "invalid_request", code };
|
||||
});
|
||||
app.setNotFoundHandler((_request, reply) => {
|
||||
reply.code(404);
|
||||
return { error: "not_found" };
|
||||
});
|
||||
app.decorate("heimdallContext", { config, keys, store, oauthRuntimes, tokenCustody });
|
||||
app.addHook("onClose", async () => {
|
||||
await store.close();
|
||||
|
|
@ -788,7 +813,7 @@ export async function buildApp(options: BuildAppOptions = {}): Promise<FastifyIn
|
|||
const profile = getAppProfile(request.params.appSlug);
|
||||
if (!profile) {
|
||||
reply.code(404);
|
||||
return { error: "unknown_app", detail: `No app '${request.params.appSlug}' is registered.` };
|
||||
return { error: "unknown_app", detail: "No app is registered under that slug." };
|
||||
}
|
||||
return serializeAppProfile(profile);
|
||||
}
|
||||
|
|
@ -1289,6 +1314,11 @@ export async function buildApp(options: BuildAppOptions = {}): Promise<FastifyIn
|
|||
};
|
||||
} catch (error) {
|
||||
const message = error instanceof Error ? error.message : "OAuth callback failed.";
|
||||
// What leaves Heimdall is fixed text. This path catches store and
|
||||
// provider failures: a Postgres error names a constraint, the host or
|
||||
// the database, and JSON.parse quotes the provider's response body.
|
||||
// The full text stays in the audit event.
|
||||
const failure = "The provider callback could not be completed.";
|
||||
const nowIso = new Date().toISOString();
|
||||
await denyBrowserAttempt("oauth_callback_failed");
|
||||
await store.createAuditEvent({
|
||||
|
|
@ -1316,7 +1346,7 @@ export async function buildApp(options: BuildAppOptions = {}): Promise<FastifyIn
|
|||
returnTo: statePayload.return_to,
|
||||
connection: statePayload.connection,
|
||||
error: "oauth_callback_failed",
|
||||
errorDescription: message,
|
||||
errorDescription: failure,
|
||||
});
|
||||
} catch {}
|
||||
}
|
||||
|
|
@ -1333,7 +1363,7 @@ export async function buildApp(options: BuildAppOptions = {}): Promise<FastifyIn
|
|||
returnTo: statePayload.return_to,
|
||||
...(handoff.kind === "backend_callback" ? { attemptId: handoff.attemptId } : {}),
|
||||
error: "oauth_callback_failed",
|
||||
errorDescription: message,
|
||||
errorDescription: failure,
|
||||
})
|
||||
);
|
||||
}
|
||||
|
|
@ -1341,10 +1371,7 @@ export async function buildApp(options: BuildAppOptions = {}): Promise<FastifyIn
|
|||
reply.code(502);
|
||||
return {
|
||||
error: "oauth_callback_failed",
|
||||
// Deliberately not `message`: this path catches store failures, and a
|
||||
// Postgres unique violation would otherwise hand the caller a
|
||||
// constraint name. The full text is in the audit event above.
|
||||
detail: "The provider callback could not be completed.",
|
||||
detail: failure,
|
||||
provider: request.params.provider,
|
||||
appSlug: statePayload.app_slug,
|
||||
returnTo: statePayload.return_to,
|
||||
|
|
@ -1544,7 +1571,7 @@ export async function buildApp(options: BuildAppOptions = {}): Promise<FastifyIn
|
|||
const profile = getAppProfile(request.params.appSlug);
|
||||
if (!profile) {
|
||||
reply.code(404);
|
||||
return { error: "unknown_app", detail: `No app '${request.params.appSlug}' is registered.` };
|
||||
return { error: "unknown_app", detail: "No app is registered under that slug." };
|
||||
}
|
||||
if (!resolveAppCaller(config, profile.slug, request.headers)) {
|
||||
reply.code(401);
|
||||
|
|
@ -1695,7 +1722,7 @@ export async function buildApp(options: BuildAppOptions = {}): Promise<FastifyIn
|
|||
const profile = getAppProfile(request.params.appSlug);
|
||||
if (!profile) {
|
||||
reply.code(404);
|
||||
return { error: "unknown_app", detail: `No app '${request.params.appSlug}' is registered.` };
|
||||
return { error: "unknown_app", detail: "No app is registered under that slug." };
|
||||
}
|
||||
if (!resolveAppCaller(config, profile.slug, request.headers)) {
|
||||
reply.code(401);
|
||||
|
|
|
|||
|
|
@ -1221,3 +1221,172 @@ describe("Heimdall service", () => {
|
|||
expect(tokenSet.scope).toEqual(["user:read:email"]);
|
||||
});
|
||||
});
|
||||
|
||||
// An answer never carries an input value. The canary stands in for whatever a
|
||||
// failure could quote: part of a misbound database URL in a pg or resolver
|
||||
// error, a request body in a JSON parse error, a header or a path.
|
||||
describe("error answers never echo an input", () => {
|
||||
const CANARY = "CANARY";
|
||||
|
||||
function failingStore(): InMemoryStore {
|
||||
const store = new InMemoryStore();
|
||||
store.consumeAuthCompletion = async () => {
|
||||
throw Object.assign(new Error(`getaddrinfo ENOTFOUND ${CANARY}host.invalid`), {
|
||||
code: "ENOTFOUND",
|
||||
hostname: `${CANARY}host.invalid`,
|
||||
});
|
||||
};
|
||||
return store;
|
||||
}
|
||||
|
||||
it("answers a store failure with a fixed 500 body and logs only the route and code", async () => {
|
||||
const app = await buildApp({ config: createTestConfig(), store: failingStore() });
|
||||
apps.push(app);
|
||||
const logged: string[] = [];
|
||||
const originalError = console.error;
|
||||
console.error = (...args: unknown[]) => void logged.push(args.map(String).join(" "));
|
||||
try {
|
||||
const response = await app.inject({
|
||||
method: "POST",
|
||||
url: "/v1/apps/bifrost/auth-completions/redeem",
|
||||
payload: { completionCode: "x" },
|
||||
});
|
||||
|
||||
expect(response.statusCode).toBe(500);
|
||||
expect(response.json()).toEqual({ error: "internal_error" });
|
||||
} finally {
|
||||
console.error = originalError;
|
||||
}
|
||||
expect(logged).toEqual(["Heimdall POST /v1/apps/:appSlug/auth-completions/redeem failed (ENOTFOUND)."]);
|
||||
});
|
||||
|
||||
it("answers a body that is not JSON without quoting it", async () => {
|
||||
const app = await buildApp({ config: createTestConfig() });
|
||||
apps.push(app);
|
||||
|
||||
const response = await app.inject({
|
||||
method: "POST",
|
||||
url: "/v1/apps/bifrost/auth-completions/redeem",
|
||||
headers: { "content-type": "application/json" },
|
||||
payload: `${CANARY}notjson`,
|
||||
});
|
||||
|
||||
expect(response.statusCode).toBe(400);
|
||||
expect(response.json().error).toBe("invalid_request");
|
||||
expect(response.body).not.toContain(CANARY);
|
||||
});
|
||||
|
||||
it("answers an unsupported media type without quoting the header", async () => {
|
||||
const app = await buildApp({ config: createTestConfig() });
|
||||
apps.push(app);
|
||||
|
||||
const response = await app.inject({
|
||||
method: "POST",
|
||||
url: "/v1/apps/bifrost/auth-completions/redeem",
|
||||
headers: { "content-type": `application/${CANARY}` },
|
||||
payload: "x",
|
||||
});
|
||||
|
||||
expect(response.statusCode).toBe(415);
|
||||
expect(response.body).not.toContain(CANARY);
|
||||
});
|
||||
|
||||
it("names the failed schema rule without quoting the request", async () => {
|
||||
const app = await buildApp({ config: createTestConfig() });
|
||||
apps.push(app);
|
||||
|
||||
const response = await app.inject({
|
||||
method: "POST",
|
||||
url: "/v1/apps/bifrost/auth-completions/redeem",
|
||||
payload: { [`${CANARY}extra`]: `${CANARY}value` },
|
||||
});
|
||||
|
||||
expect(response.statusCode).toBe(400);
|
||||
expect(response.json().error).toBe("invalid_request");
|
||||
expect(response.json().detail).toMatch(/^body: #\//);
|
||||
expect(response.body).not.toContain(CANARY);
|
||||
});
|
||||
|
||||
it("answers an unknown route or app without quoting the path", async () => {
|
||||
const app = await buildApp({ config: createTestConfig() });
|
||||
apps.push(app);
|
||||
|
||||
const route = await app.inject({ method: "GET", url: `/v1/${CANARY}route` });
|
||||
expect(route.statusCode).toBe(404);
|
||||
expect(route.json()).toEqual({ error: "not_found" });
|
||||
|
||||
const unknownApp = await app.inject({ method: "GET", url: `/v1/apps/${CANARY}app` });
|
||||
expect(unknownApp.statusCode).toBe(404);
|
||||
expect(unknownApp.json().error).toBe("unknown_app");
|
||||
expect(unknownApp.body).not.toContain(CANARY);
|
||||
|
||||
const start = await app.inject({
|
||||
method: "POST",
|
||||
url: "/v1/oauth/discord/start",
|
||||
payload: { appSlug: `${CANARY}app`, mode: "sign_in", returnTo: "https://repixelizer.gamecult.org/app/" },
|
||||
});
|
||||
expect(start.statusCode).toBe(400);
|
||||
expect(start.body).not.toContain(CANARY);
|
||||
});
|
||||
|
||||
function failingDiscordRuntime(): OAuthProviderRuntime {
|
||||
return {
|
||||
...createMockDiscordRuntime(),
|
||||
async exchangeAuthorizationCode() {
|
||||
throw new SyntaxError(`Unexpected token 'a', "access_token=${CANARY}token" is not valid JSON`);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
it("renders a failed provider callback page without the failure's text", async () => {
|
||||
const app = await buildApp({ config: createTestConfig(), oauthRuntimes: { discord: failingDiscordRuntime() } });
|
||||
apps.push(app);
|
||||
|
||||
const stateToken = await startDiscordSignIn(app);
|
||||
const response = await app.inject({
|
||||
method: "GET",
|
||||
url: `/v1/oauth/discord/callback?code=test-code&state=${encodeURIComponent(stateToken)}`,
|
||||
headers: { accept: "text/html" },
|
||||
});
|
||||
|
||||
expect(response.headers["content-type"]).toContain("text/html");
|
||||
expect(response.body).toContain("oauth_callback_failed");
|
||||
expect(response.body).not.toContain(CANARY);
|
||||
});
|
||||
|
||||
it("delivers a failed provider callback to the app backend without the failure's text", async () => {
|
||||
const deliveries: string[] = [];
|
||||
globalThis.fetch = async (_input, init) => {
|
||||
deliveries.push(String(init?.body));
|
||||
return new Response(null, { status: 204 });
|
||||
};
|
||||
const app = await buildApp({ config: createTestConfig(), oauthRuntimes: { discord: failingDiscordRuntime() } });
|
||||
apps.push(app);
|
||||
|
||||
const start = await app.inject({
|
||||
method: "POST",
|
||||
url: "/v1/oauth/discord/start",
|
||||
payload: {
|
||||
appSlug: "repixelizer",
|
||||
mode: "sign_in",
|
||||
returnTo: "https://repixelizer.gamecult.org/app/",
|
||||
handoff: {
|
||||
kind: "backend_callback",
|
||||
attemptId: "attempt-123",
|
||||
callbackUrl: "https://repixelizer.gamecult.org/api/auth/heimdall/callback",
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(start.statusCode).toBe(201);
|
||||
const response = await app.inject({
|
||||
method: "GET",
|
||||
url: `/v1/oauth/discord/callback?code=test-code&state=${encodeURIComponent(start.json().stateToken as string)}`,
|
||||
});
|
||||
|
||||
expect(response.statusCode).toBe(502);
|
||||
expect(response.body).not.toContain(CANARY);
|
||||
expect(deliveries).toHaveLength(1);
|
||||
expect(JSON.parse(deliveries[0]!)).toMatchObject({ status: "error", error: "oauth_callback_failed" });
|
||||
expect(deliveries[0]).not.toContain(CANARY);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue