diff --git a/.env.example b/.env.example index c8d03d5..168a371 100644 --- a/.env.example +++ b/.env.example @@ -32,3 +32,14 @@ WS_INTERNAL_URL= # --- App --- NEXT_PUBLIC_APP_URL=http://localhost:3000 + +# --- Rate limiting --- +# Per-bucket ceilings per 60s, as RATE_LIMIT_. Leave unset in +# production: the shipped defaults (boards.create = 10/min) assume a proxy that +# sets x-forwarded-for, so every abuser gets their own bucket. +# +# Locally there is no such header, so ALL callers share one bucket — and +# `pnpm test:e2e` self-seeds a board per spec across 4 workers, which blows a +# 10/min budget and fails with 429. This raises the local ceiling so the suite +# can run; the limiter still applies, just far above one developer's pace. +RATE_LIMIT_BOARDS_CREATE=200 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fbe13c1..71b63ab 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,9 +11,12 @@ jobs: steps: - uses: actions/checkout@v4 + # No `version:` on purpose. action-setup@v4 throws "Multiple versions of + # pnpm specified" when its input disagrees with package.json's + # packageManager pin, and "9" !== "9.15.4". Reading the pin is also the + # behaviour we want: CI resolves the exact pnpm the lockfile was written + # with, and there is one place to bump it. - uses: pnpm/action-setup@v4 - with: - version: 9 - uses: actions/setup-node@v4 with: diff --git a/apps/web/app/(marketing)/page.tsx b/apps/web/app/(marketing)/page.tsx index cfe4a23..d179a7b 100644 --- a/apps/web/app/(marketing)/page.tsx +++ b/apps/web/app/(marketing)/page.tsx @@ -1,11 +1,15 @@ import Link from "next/link"; +import { configuredProviders } from "@/auth"; import { NewBoardButton } from "@/components/marketing/NewBoardButton"; import { resolveViewer } from "@/lib/auth/viewer"; /** Minimal, calm landing: the one-liner, one button, a sign-in affordance. */ export default async function LandingPage() { const viewer = await resolveViewer(); + // No provider means nothing can sign you in, so offering it would send + // people to a page that can only apologise. + const canSignIn = configuredProviders().length > 0; return (
@@ -22,25 +26,27 @@ export default async function LandingPage() { -

- {viewer ? ( - <> - You're in as {viewer.name} - {viewer.kind === "guest" ? ( - <> - {" · "} - - Sign in to keep your boards - - - ) : null} - - ) : ( - - Sign in - - )} -

+ {viewer || canSignIn ? ( +

+ {viewer ? ( + <> + You're in as {viewer.name} + {viewer.kind === "guest" && canSignIn ? ( + <> + {" · "} + + Sign in to keep your boards + + + ) : null} + + ) : ( + + Sign in + + )} +

+ ) : null}
); } diff --git a/apps/web/app/(marketing)/signin/page.tsx b/apps/web/app/(marketing)/signin/page.tsx new file mode 100644 index 0000000..fa785f5 --- /dev/null +++ b/apps/web/app/(marketing)/signin/page.tsx @@ -0,0 +1,77 @@ +import Link from "next/link"; + +import { configuredProviders, signIn } from "@/auth"; +import { NewBoardButton } from "@/components/marketing/NewBoardButton"; +import { Button } from "@/components/ui/button"; +import { Card, CardContent, CardDescription, CardHeader, CardTitle } from "@/components/ui/card"; +import { safeCallbackUrl } from "@/lib/auth/safe-callback"; + +interface SignInPageProps { + searchParams: Promise<{ callbackUrl?: string; error?: string }>; +} + +/** + * /signin — Auth.js `pages.signIn`, replacing the built-in page. + * + * Two states, because a bare setup ships with no OAuth keys (zero-setup + * principle) and the built-in page renders an empty card in that case: + * providers configured → a button each; none → say so plainly and offer the + * guest path, which already works and is what most people want anyway. + */ +export default async function SignInPage({ searchParams }: SignInPageProps) { + const { callbackUrl, error } = await searchParams; + const providers = configuredProviders(); + const redirectTo = safeCallbackUrl(callbackUrl); + + return ( +
+ + + + {providers.length > 0 ? "Sign in to Quorum" : "Sign-in isn’t set up"} + + + {providers.length > 0 + ? "Signing in keeps your boards across devices. You can also just start a board as a guest." + : "This deployment has no sign-in provider configured, so there’s nothing to sign in with yet. Guest boards work fine without one."} + + + + {error ? ( +

+ That sign-in attempt didn’t go through. Try again, or start a board as a guest. +

+ ) : null} + + {providers.map((provider) => ( +
{ + "use server"; + await signIn(provider.id, { redirectTo }); + }} + > + +
+ ))} + + {providers.length > 0 ? ( + + Back to Quorum + + ) : ( + <> + + + Back to Quorum + + + )} + + +
+ ); +} diff --git a/apps/web/auth.ts b/apps/web/auth.ts index da3652c..58bf1ff 100644 --- a/apps/web/auth.ts +++ b/apps/web/auth.ts @@ -36,6 +36,27 @@ function buildProviders(): NextAuthConfig["providers"] { return providers; } +/** What /signin renders a button for. Empty in a bare setup — see below. */ +export interface ConfiguredProvider { + id: string; + name: string; +} + +/** + * The providers actually wired up, derived from the SAME list Auth.js gets so + * the two can never disagree. + * + * Callers use this to decide whether signing in is possible at all: with no + * OAuth keys the list is empty, and anything that offers a sign-in affordance + * would otherwise send people to a page that cannot sign them in. + */ +export function configuredProviders(): ConfiguredProvider[] { + return buildProviders().map((provider) => { + const config = typeof provider === "function" ? provider() : provider; + return { id: config.id, name: config.name }; + }); +} + export const { handlers, auth, signIn, signOut } = NextAuth(() => ({ // Lazy config: getDb() is called per request, not at import, so importing // this module never opens a database connection by itself. @@ -50,6 +71,10 @@ export const { handlers, auth, signIn, signOut } = NextAuth(() => ({ session: { strategy: "database" }, secret: env.AUTH_SECRET, trustHost: true, + // Auth.js's built-in page renders an empty card when no provider is + // configured, which is the documented default here — a dead end in the + // product's own light theme. Ours explains itself and always offers a way on. + pages: { signIn: "/signin", error: "/signin" }, providers: buildProviders(), callbacks: { session({ session, user }) { diff --git a/apps/web/lib/auth/safe-callback.test.ts b/apps/web/lib/auth/safe-callback.test.ts new file mode 100644 index 0000000..ce34c4b --- /dev/null +++ b/apps/web/lib/auth/safe-callback.test.ts @@ -0,0 +1,41 @@ +/** + * Regression: ISSUE-006 — /api/auth/signin rendered a blank page with no + * provider configured. The replacement /signin page accepts Auth.js's + * attacker-controlled `callbackUrl`, so it must never redirect off-origin. + * Found by /qa on 2026-08-01 + * Report: .gstack/qa-reports/qa-report-quorum-2026-08-01-signin.md + */ +import { describe, expect, it } from "vitest"; + +import { safeCallbackUrl } from "./safe-callback"; + +describe("safeCallbackUrl", () => { + it("keeps a same-origin path so sign-in returns you where you were", () => { + expect(safeCallbackUrl("/b/brd_abc123")).toBe("/b/brd_abc123"); + }); + + it("keeps the query string and hash on that path", () => { + expect(safeCallbackUrl("/b/brd_abc?tab=chat#msg-3")).toBe("/b/brd_abc?tab=chat#msg-3"); + }); + + it.each([undefined, null, ""])("falls back home for %j", (value) => { + expect(safeCallbackUrl(value)).toBe("/"); + }); + + it.each([ + "https://evil.example/steal", + "http://evil.example", + "javascript:alert(1)", + "data:text/html,", + "evil.example", + ])("refuses the absolute or scheme-bearing target %j", (value) => { + expect(safeCallbackUrl(value)).toBe("/"); + }); + + it.each(["//evil.example", "//evil.example/path", "/\\evil.example"])( + "refuses %j, which a browser reads as protocol-relative despite the leading slash", + (value) => { + expect(safeCallbackUrl(value)).toBe("/"); + }, + ); +}); diff --git a/apps/web/lib/auth/safe-callback.ts b/apps/web/lib/auth/safe-callback.ts new file mode 100644 index 0000000..ca4cf64 --- /dev/null +++ b/apps/web/lib/auth/safe-callback.ts @@ -0,0 +1,19 @@ +/** + * Sanitiser for the `callbackUrl` Auth.js appends when it bounces someone to + * the sign-in page. The value arrives in the query string, so it is entirely + * attacker-controlled: a link like + * `/signin?callbackUrl=https://evil.example` would otherwise let a phisher use + * our own domain to redirect people after a real, successful sign-in. + * + * Only same-origin paths survive. A leading "/" is NOT enough on its own — + * browsers read "//evil.example" as protocol-relative and "/\evil.example" the + * same way, both absolute despite the leading slash. + */ +export function safeCallbackUrl(raw: string | undefined | null): string { + if (typeof raw !== "string" || raw === "") return "/"; + if (!raw.startsWith("/")) return "/"; + // Reject every scheme-ish second character; "/" and "\" are the two a + // browser treats as the start of an authority. + if (raw.startsWith("//") || raw.startsWith("/\\")) return "/"; + return raw; +} diff --git a/apps/web/lib/server/rate-limit.regression-1.test.ts b/apps/web/lib/server/rate-limit.regression-1.test.ts new file mode 100644 index 0000000..490862d --- /dev/null +++ b/apps/web/lib/server/rate-limit.regression-1.test.ts @@ -0,0 +1,82 @@ +import { afterEach, describe, expect, it } from "vitest"; + +import { enforceRateLimit, type RateLimitStore } from "./rate-limit"; + +/** + * Regression: ISSUE-004 — the shipped 10/min boards.create ceiling applies to + * one shared `ip:unknown` bucket when no proxy header is present, so the + * self-seeding e2e suite failed 8/18 specs with 429. + * Found by /qa on 2026-08-01 + * Report: .gstack/qa-reports/qa-report-quorum-2026-08-01.md + */ + +function fakeStore(): RateLimitStore { + const entries = new Map(); + return { + async hit(key, windowSec) { + const count = (entries.get(key) ?? 0) + 1; + entries.set(key, count); + return { count, ttlMs: windowSec * 1_000 }; + }, + }; +} + +function req(): Request { + return new Request("http://localhost:3000/api/boards", { method: "POST" }); +} + +const ENV_KEY = "RATE_LIMIT_BOARDS_CREATE"; +const original = process.env[ENV_KEY]; + +afterEach(() => { + if (original === undefined) delete process.env[ENV_KEY]; + else process.env[ENV_KEY] = original; +}); + +/** Requests allowed before the first 429, capped so a broken override can't hang. */ +async function allowedBefore429(store: RateLimitStore, ceiling = 40): Promise { + for (let sent = 0; sent < ceiling; sent += 1) { + if (await enforceRateLimit(req(), { bucket: "boards.create", limit: 10, windowSec: 60, store })) + return sent; + } + return ceiling; +} + +describe("per-bucket limit override", () => { + it("raises the ceiling so a self-seeding suite is not blocked at 10", async () => { + process.env[ENV_KEY] = "25"; + expect(await allowedBefore429(fakeStore())).toBe(25); + }); + + it("keeps the route default when unset — production is unchanged", async () => { + delete process.env[ENV_KEY]; + expect(await allowedBefore429(fakeStore())).toBe(10); + }); + + it.each(["0", "-5", "abc", "10.5", " "])( + "ignores the unusable override %j rather than disabling the limit", + async (value) => { + process.env[ENV_KEY] = value; + expect(await allowedBefore429(fakeStore())).toBe(10); + }, + ); + + it("scopes the override to its own bucket", async () => { + process.env[ENV_KEY] = "25"; + const store = fakeStore(); + // A different bucket must not inherit boards.create's raised ceiling. + let allowed = 0; + while ( + allowed < 40 && + !(await enforceRateLimit(req(), { + bucket: "agents.connect", + limit: 10, + windowSec: 60, + store, + })) + ) { + allowed += 1; + } + expect(allowed).toBe(10); + }); +}); diff --git a/apps/web/lib/server/rate-limit.ts b/apps/web/lib/server/rate-limit.ts index 3090852..5e0e8eb 100644 --- a/apps/web/lib/server/rate-limit.ts +++ b/apps/web/lib/server/rate-limit.ts @@ -147,16 +147,52 @@ export interface EnforceRateLimitOptions { store?: RateLimitStore; } +/** `boards.create` → `RATE_LIMIT_BOARDS_CREATE`. */ +function envNameForBucket(bucket: string): string { + return `RATE_LIMIT_${bucket.replace(/[^a-zA-Z0-9]+/g, "_").toUpperCase()}`; +} + +/** + * Per-bucket ceiling, overridable per environment. + * + * The shipped defaults are tuned for a public deployment behind a proxy that + * sets x-forwarded-for, where each abuser gets their own bucket. Without that + * header every caller shares the `ip:unknown` bucket — which is exactly the + * local case, and why the e2e suite (18 self-seeding specs, 4 workers, one + * apparent client) exhausted a 10/min budget and failed with 429. + * + * Read off process.env for the same reason as REDIS_URL above: lib/env's eager + * boot parse would make this module untestable. A missing, non-numeric, or + * non-positive value keeps the caller's default, so a typo can never silently + * disable a limit. + */ +function resolveLimit(bucket: string, fallback: number): number { + const raw = process.env[envNameForBucket(bucket)]; + if (raw === undefined || raw.trim() === "") return fallback; + const parsed = Number(raw); + if (!Number.isInteger(parsed) || parsed <= 0) { + log.warn("rate_limit.bad_override", { bucket, value: raw, using: fallback }); + return fallback; + } + return parsed; +} + /** null when the request may proceed, otherwise the 429 to return as-is. */ export async function enforceRateLimit( request: Request, { bucket, limit, windowSec, scope, store }: EnforceRateLimitOptions, ): Promise { const subject = scope ?? `ip:${clientIpFromRequest(request)}`; - const result = await checkRateLimit({ key: `rl:${bucket}:${subject}`, limit, windowSec, store }); + const effectiveLimit = resolveLimit(bucket, limit); + const result = await checkRateLimit({ + key: `rl:${bucket}:${subject}`, + limit: effectiveLimit, + windowSec, + store, + }); if (result.allowed) return null; - log.warn("rate_limit.blocked", { bucket, subject, limit, windowSec }); + log.warn("rate_limit.blocked", { bucket, subject, limit: effectiveLimit, windowSec }); const response = jsonError( 429, "rate_limited", diff --git a/apps/ws/src/internal-api.ts b/apps/ws/src/internal-api.ts index f89daaa..e0ecd9d 100644 --- a/apps/ws/src/internal-api.ts +++ b/apps/ws/src/internal-api.ts @@ -24,7 +24,12 @@ import { agentStatusSchema } from "@quorum/shared/schemas/agent"; import type { ConsensusPolicy } from "@quorum/shared/schemas/policy"; import type { Proposal, ProposalInput } from "@quorum/shared/schemas/proposal"; import { agentOperationSchema, agentMessageSchema } from "@quorum/shared/schemas/turn"; -import { appendMessage, createProposalInDoc, setAgentStatus } from "@quorum/shared/yjs/doc"; +import { + appendMessage, + createProposalInDoc, + readAgentStatuses, + setAgentStatus, +} from "@quorum/shared/yjs/doc"; import { messageSchema } from "@quorum/shared/schemas/message"; import { serializeError, type Logger } from "./log"; @@ -46,6 +51,28 @@ export const agentStatusBodySchema = z.object({ status: agentStatusSchema, }); +type AgentStatus = z.infer; + +/** + * The board-chat notice for an agent status change, or null when the change + * needs none. + * + * A degraded agent stops answering @mentions and dispatch drops those turns + * with nothing but a server log, so someone keeps typing at an agent that went + * quiet. The roster pill alone is not enough: the chat is where they are + * talking, and §7 forbids the silent refusal. Only the transition INTO + * degraded speaks, so one outage costs one message rather than one per dropped + * mention (the same restraint as the rate-limit notice in turns.ts). + */ +export function degradedNotice( + previous: AgentStatus | undefined, + next: AgentStatus, + agentName: string, +): string | null { + if (next !== "degraded" || previous === "degraded") return null; + return `${agentName} stopped responding and is marked degraded — @mentions will not reach it until it recovers.`; +} + /** Load a board doc (via a Hocuspocus direct connection) and mutate it. */ export interface DocGateway { withDoc(boardId: string, fn: (doc: Y.Doc) => T): Promise; @@ -332,7 +359,20 @@ export function createInternalApi(deps: InternalApiDeps): InternalApi { if (agent === null) throw new HttpError(404, "agent is not on this board"); await deps.boards.setAgentStatus(body.agentId, body.status); await deps.docs.withDoc(body.boardId, (doc) => { + const previous = readAgentStatuses(doc)[body.agentId]?.status; setAgentStatus(doc, body.agentId, body.status, now()); + const notice = degradedNotice(previous, body.status, agent.name); + if (notice !== null) { + appendMessage(doc, { + id: newId("message"), + role: "system", + authorId: "system", + name: "Quorum", + content: notice, + createdAt: now(), + mentions: [], + }); + } }); deps.log.info("agent status updated", { boardId: body.boardId, diff --git a/apps/ws/test/internal-api.regression-1.test.ts b/apps/ws/test/internal-api.regression-1.test.ts new file mode 100644 index 0000000..0d4e880 --- /dev/null +++ b/apps/ws/test/internal-api.regression-1.test.ts @@ -0,0 +1,36 @@ +/** + * Regression: ISSUE-005 — an agent that degraded kept silently swallowing + * @mentions. Dispatch skipped 11 turns with only a server log while the board + * chat showed nothing, so the sender had no idea the agent had stopped. + * Found by /qa on 2026-08-01 + * Report: .gstack/qa-reports/qa-report-quorum-2026-08-01.md + */ +import { describe, expect, it } from "vitest"; + +import { degradedNotice } from "../src/internal-api"; + +describe("degradedNotice", () => { + it("announces the agent by name when it first degrades", () => { + const notice = degradedNotice("ready", "degraded", "Researcher"); + expect(notice).toContain("Researcher"); + expect(notice).toContain("degraded"); + }); + + it("announces a degrade seen for the first time (no prior status in the doc)", () => { + expect(degradedNotice(undefined, "degraded", "Researcher")).not.toBeNull(); + }); + + it("stays silent while the agent is already degraded, so one outage is one message", () => { + expect(degradedNotice("degraded", "degraded", "Researcher")).toBeNull(); + }); + + it("speaks again only after the agent recovers and degrades a second time", () => { + expect(degradedNotice("degraded", "ready", "Researcher")).toBeNull(); + expect(degradedNotice("ready", "degraded", "Researcher")).not.toBeNull(); + }); + + it.each(["ready", "running"] as const)("says nothing about a healthy %s agent", (status) => { + expect(degradedNotice("degraded", status, "Researcher")).toBeNull(); + expect(degradedNotice("ready", status, "Researcher")).toBeNull(); + }); +});