security: log redaction, magic-link enumeration, federation doc cap
Three Low/Info hardening items from the 2026-06-10 security review.
OAuth/credential log redaction:
- oauth-flow.server.ts logged the raw exception on code-exchange
failure; a provider error can embed the auth code / token response,
which would land in logs + Sentry. Log only e.message now.
- manager.markNeedsRelink bounded the provider-supplied reason string
to 200 chars before logging.
Magic-link account enumeration:
- createMagicToken now returns null (instead of throwing "No account
found for this email") when no account matches; the login route
always responds { step: "magic-link-sent" }, minting a token and
sending mail only for a real account. The public login form can no
longer be used to probe which emails are registered. Registration's
email/username "already in use/taken" messages are intentionally
unchanged — standard signup UX, and the passkey ceremony can't be
made to fake-succeed.
Federation remote-document size cap:
- assertRemoteDocSize rejects an actor/outbox document over 4 MB once
serialized, applied in the ingest fetchJson seam. Fedify owns the
transfer (with its own SSRF + redirect limits) and our poll uses the
authenticated loader for secure-mode instances, so this is a
downstream guard on iteration/persistence, complementing the existing
per-poll item cap.
Tests: assertRemoteDocSize unit cases; a gated (FEDERATION_INTEGRATION=1)
integration test asserting an unsigned POST to /users/:u/inbox is
rejected with 401 — a regression guard for Fedify's signature
verification.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
55f8758c70
commit
769d1b5d31
7 changed files with 147 additions and 14 deletions
|
|
@ -277,12 +277,16 @@ function generateLoginCode(): string {
|
|||
return String(num).padStart(6, "0");
|
||||
}
|
||||
|
||||
export async function createMagicToken(email: string): Promise<{ token: string; code: string }> {
|
||||
export async function createMagicToken(
|
||||
email: string,
|
||||
): Promise<{ token: string; code: string } | null> {
|
||||
const db = getDb();
|
||||
|
||||
// Check user exists
|
||||
// Returns null (not an error) when no account matches, so the caller
|
||||
// can respond identically whether or not the email is registered —
|
||||
// closing the account-enumeration oracle on the public login form.
|
||||
const [user] = await db.select().from(users).where(eq(users.email, email));
|
||||
if (!user) throw new Error("No account found for this email");
|
||||
if (!user) return null;
|
||||
|
||||
const token = randomBytes(32).toString("base64url");
|
||||
const code = generateLoginCode();
|
||||
|
|
|
|||
|
|
@ -165,7 +165,10 @@ export async function markNeedsRelink(
|
|||
.set({ status: "needs_relink" })
|
||||
.where(eq(connectedServices.id, serviceId));
|
||||
// Reason is logged but not persisted yet — add a column if/when we surface it in UI.
|
||||
console.warn(`[connected-services] ${serviceId} flipped to needs_relink: ${reason}`);
|
||||
// Bounded: `reason` can carry provider-side error text, so cap its length
|
||||
// to avoid dumping a large/sensitive blob into logs.
|
||||
const safeReason = reason.length > 200 ? `${reason.slice(0, 200)}…` : reason;
|
||||
console.warn(`[connected-services] ${serviceId} flipped to needs_relink: ${safeReason}`);
|
||||
}
|
||||
|
||||
// Provider-side revocation (e.g. a Garmin deregistration notification):
|
||||
|
|
|
|||
|
|
@ -110,7 +110,13 @@ export async function completeOAuthFlow(
|
|||
});
|
||||
return { status: "linked", state };
|
||||
} catch (e) {
|
||||
console.error(`OAuth callback failed for ${manifest.id}:`, e);
|
||||
// Log only the error message, never the raw exception object — a
|
||||
// provider's thrown error can embed the authorization code or token
|
||||
// exchange response, which would then land in logs / Sentry.
|
||||
console.error(
|
||||
`OAuth callback failed for ${manifest.id}:`,
|
||||
e instanceof Error ? e.message : String(e),
|
||||
);
|
||||
const code =
|
||||
typeof (e as { code?: string }).code === "string"
|
||||
? (e as { code: string }).code
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ const {
|
|||
isHostBackingOff,
|
||||
resetHostBackoff,
|
||||
pollRemoteActor,
|
||||
assertRemoteDocSize,
|
||||
} = await import("./federation-ingest.server.ts");
|
||||
|
||||
function trailsCreate(overrides: Record<string, unknown> = {}, noteOverrides: Record<string, unknown> = {}) {
|
||||
|
|
@ -129,3 +130,15 @@ describe("429 backoff (7.4)", () => {
|
|||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("assertRemoteDocSize", () => {
|
||||
it("accepts a normal-sized document", () => {
|
||||
expect(() => assertRemoteDocSize({ type: "Person", name: "Alice" })).not.toThrow();
|
||||
expect(() => assertRemoteDocSize(null)).not.toThrow();
|
||||
});
|
||||
|
||||
it("rejects a document over the 4 MB cap", () => {
|
||||
const huge = { type: "OrderedCollection", orderedItems: ["x".repeat(5 * 1024 * 1024)] };
|
||||
expect(() => assertRemoteDocSize(huge)).toThrow(/too large/);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -28,6 +28,22 @@ const HOST_PACE_MS = 5_000;
|
|||
const CONFLICT_STREAK_LIMIT = 5;
|
||||
/** Backoff after a 429 without a usable Retry-After (7.4). */
|
||||
const DEFAULT_BACKOFF_MS = 15 * 60 * 1000;
|
||||
/**
|
||||
* Reject a fetched actor/outbox document larger than this once serialized.
|
||||
* Fedify owns the transfer (and applies its own SSRF + redirect limits),
|
||||
* so this is a downstream guard: it stops us iterating/persisting an
|
||||
* absurdly large document from a hostile remote, on top of the existing
|
||||
* per-poll item cap. A legitimate actor doc or 50-item page is a few KB.
|
||||
*/
|
||||
const MAX_REMOTE_DOC_BYTES = 4 * 1024 * 1024; // 4 MB
|
||||
|
||||
/** Throws if a remote document's serialized size exceeds the cap. */
|
||||
export function assertRemoteDocSize(document: unknown): void {
|
||||
const size = JSON.stringify(document ?? null).length;
|
||||
if (size > MAX_REMOTE_DOC_BYTES) {
|
||||
throw new Error(`remote document too large: ${size} bytes (cap ${MAX_REMOTE_DOC_BYTES})`);
|
||||
}
|
||||
}
|
||||
/**
|
||||
* Cap an honored Retry-After at the poll interval — anything longer is
|
||||
* equivalent to "skip until a later sweep", and an absurd header from
|
||||
|
|
@ -228,6 +244,7 @@ function defaultDeps(): PollDeps {
|
|||
const ctx = federation.createContext(new URL(getOrigin()), undefined);
|
||||
const loader = await ctx.getDocumentLoader({ identifier: signerUsername });
|
||||
const { document } = await loader(url);
|
||||
assertRemoteDocSize(document);
|
||||
return document;
|
||||
},
|
||||
async pace(host) {
|
||||
|
|
|
|||
|
|
@ -69,16 +69,20 @@ export async function action({ request }: Route.ActionArgs) {
|
|||
});
|
||||
if (!perIp.allowed) return tooManyRequests(perIp.resetMs);
|
||||
|
||||
const { token, code: loginCode } = await createMagicToken(email);
|
||||
const origin = getOrigin();
|
||||
const link = `${origin}/auth/verify?token=${token}`;
|
||||
// In dev, return the link and code directly
|
||||
if (process.env.NODE_ENV !== "production") {
|
||||
console.log(`[Magic Link] ${email}: ${link} (code: ${loginCode})`);
|
||||
return data({ step: "magic-link-sent", devLink: link, code: loginCode });
|
||||
// Always respond "sent" regardless of whether the account exists,
|
||||
// so the response can't probe which emails are registered. A token
|
||||
// is only minted (and mail only sent) for a real account.
|
||||
const minted = await createMagicToken(email);
|
||||
if (minted) {
|
||||
const origin = getOrigin();
|
||||
const link = `${origin}/auth/verify?token=${minted.token}`;
|
||||
// In dev, return the link and code directly for a real account.
|
||||
if (process.env.NODE_ENV !== "production") {
|
||||
console.log(`[Magic Link] ${email}: ${link} (code: ${minted.code})`);
|
||||
return data({ step: "magic-link-sent", devLink: link, code: minted.code });
|
||||
}
|
||||
await sendMagicLink(email, link, minted.code);
|
||||
}
|
||||
|
||||
await sendMagicLink(email, link, loginCode);
|
||||
return data({ step: "magic-link-sent" });
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,86 @@
|
|||
import { describe, it, expect, beforeAll, afterEach } from "vitest";
|
||||
import { eq } from "drizzle-orm";
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { getDb } from "~/lib/db";
|
||||
import { users } from "@trails-cool/db/schema/journal";
|
||||
|
||||
// Regression guard for the inbox's HTTP Signature requirement. Fedify
|
||||
// verifies signatures on inbox listeners by default; this fails loudly
|
||||
// if a future config change or Fedify upgrade silently stops rejecting
|
||||
// unsigned activities — the property the whole federation trust model
|
||||
// rests on.
|
||||
//
|
||||
// Talks to real Postgres + builds the Fedify instance, so it's gated
|
||||
// behind FEDERATION_INTEGRATION=1 like the sibling federation
|
||||
// integration tests (the recipient actor must exist for Fedify to get
|
||||
// as far as verifying the signature, rather than 404ing).
|
||||
const runIntegration = process.env.FEDERATION_INTEGRATION === "1";
|
||||
const ORIGIN = process.env.ORIGIN ?? "http://localhost:3000";
|
||||
|
||||
const createdUserIds: string[] = [];
|
||||
|
||||
async function makePublicUser(username: string): Promise<void> {
|
||||
const db = getDb();
|
||||
const id = randomUUID();
|
||||
await db.insert(users).values({
|
||||
id,
|
||||
email: `${username}@example.test`,
|
||||
username,
|
||||
domain: new URL(ORIGIN).host,
|
||||
profileVisibility: "public",
|
||||
});
|
||||
createdUserIds.push(id);
|
||||
}
|
||||
|
||||
function unsignedInboxRequest(username: string): Request {
|
||||
return new Request(`${ORIGIN}/users/${username}/inbox`, {
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/activity+json" },
|
||||
// Deliberately NO Signature header.
|
||||
body: JSON.stringify({
|
||||
"@context": "https://www.w3.org/ns/activitystreams",
|
||||
id: "https://attacker.example/activities/1",
|
||||
type: "Follow",
|
||||
actor: "https://attacker.example/users/mallory",
|
||||
object: `${ORIGIN}/users/${username}`,
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
function routeArgs(request: Request, username: string): any {
|
||||
return {
|
||||
request,
|
||||
params: { username },
|
||||
context: {},
|
||||
unstable_url: new URL(request.url),
|
||||
unstable_pattern: "/users/:username/inbox",
|
||||
};
|
||||
}
|
||||
|
||||
describe.runIf(runIntegration)("POST /users/:username/inbox — signature enforcement", () => {
|
||||
beforeAll(() => {
|
||||
process.env.FEDERATION_ENABLED = "true";
|
||||
process.env.ORIGIN ??= ORIGIN;
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
const db = getDb();
|
||||
for (const id of createdUserIds.splice(0)) {
|
||||
await db.delete(users).where(eq(users.id, id));
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects an unsigned activity with 401 (never 2xx)", async () => {
|
||||
const username = `inbox-unsigned-${Date.now()}`;
|
||||
await makePublicUser(username);
|
||||
|
||||
const { action } = await import("./users.$username.inbox.ts");
|
||||
const resp = (await action(routeArgs(unsignedInboxRequest(username), username))) as Response;
|
||||
|
||||
// The contract that matters: an unsigned activity is never accepted.
|
||||
expect(resp.status).toBeGreaterThanOrEqual(400);
|
||||
expect(resp.status).toBeLessThan(500);
|
||||
expect(resp.status).toBe(401);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue