From 0b39c76cecbbb43e00bf0efe19966b08811abcf0 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Sun, 15 Nov 2026 18:01:12 +0800 Subject: [PATCH 1/4] feat(security): classify every mutating route and pace the worst ones An explicit route-class policy declares destructive, expensive and ordinary mutating routes route by route, a sweep test compares it against the registrations in src/app.ts in both directions, and the first four privileged routes get a limit through config so the pacing is visible to the analyzer. Part of #708 Written-by: Fledge Platform Dev-3 --- changelog.d/708-route-classes.security.md | 1 + docs/decisions/0022-mutation-route-classes.md | 95 ++++++++++++ docs/security.md | 43 ++++-- src/app.ts | 17 ++- src/lib/routeClasses.ts | 142 ++++++++++++++++++ test/routeClassSweep.test.ts | 69 +++++++++ test/routeRateLimit.test.ts | 10 +- 7 files changed, 363 insertions(+), 14 deletions(-) create mode 100644 changelog.d/708-route-classes.security.md create mode 100644 docs/decisions/0022-mutation-route-classes.md create mode 100644 src/lib/routeClasses.ts create mode 100644 test/routeClassSweep.test.ts diff --git a/changelog.d/708-route-classes.security.md b/changelog.d/708-route-classes.security.md new file mode 100644 index 00000000..cc446340 --- /dev/null +++ b/changelog.d/708-route-classes.security.md @@ -0,0 +1 @@ +Every route that changes state now names a rate-limit class — destructive, expensive or ordinary-by-decision — and a test reads `src/app.ts` and fails, naming the route, when a mutation has no class. `POST /repos/:id/role` and the provider-credential delete take the destructive budget, and `POST /repos/:id/team/stop` and `POST /repos/:id/jobs/:jobId/merge` take the expensive one. diff --git a/docs/decisions/0022-mutation-route-classes.md b/docs/decisions/0022-mutation-route-classes.md new file mode 100644 index 00000000..b069e609 --- /dev/null +++ b/docs/decisions/0022-mutation-route-classes.md @@ -0,0 +1,95 @@ +# 0022 — a mutating route carries a class, and a test enforces it + +Status: **accepted** (2026-11-15) +Date: 2026-11-15 +Approved by: **Chief, 2026-11-14** (the ordinary class is unlimited by decision, +not by omission) and the Lead's three-class ruling of the same date +Supersedes: nothing; it records the rule `#708` applies and the shape of its +remaining slices + +## Context + +`#708` found that most of the browser-facing state-changing routes in +`src/app.ts` carried neither a limiter nor a recorded decision, against eight +that carried `destructiveLimit`. The first slice built the budgets +(`src/lib/rateLimit.ts`, one `RouteLimiters` keyed by `rateLimitKey`, numbers +from config) and paced five of the hottest mutations. What it did not change is +that a route added tomorrow inherits nothing: the gap was found by counting, not +by a test, and a count rots as fast as the routes do. + +Two things were therefore still missing, and they are the same thing seen twice: +a written class for every mutating route, and a mechanism that notices when one +is missing. + +## Decision + +Every mutating route in `src/app.ts` belongs to exactly one of three classes, +declared as data in `src/lib/routeClasses.ts`: + +1. **destructive** — irreversible or privilege-changing: a role, a token, a + credential, a policy or a repository goes, or an account's standing changes. +2. **expensive** — starts, stops or merges real work: a job, a plan, a release, + a schedule, an instance, a team turn. +3. **ordinary** — everything else that changes state. + +`test/routeClassSweep.test.ts` reads the registrations out of `src/app.ts` and +compares them with that table in **both** directions. A mutating route with no +class fails as `unclassified: METHOD /path`, naming it. A class left behind for +a route that no longer exists fails as `stale classification: METHOD /path`. The +sweep reads the file; it does not pin a count, a total or a list, so an unrelated +edit cannot turn it red the way `#720` did to a suite that pinned a call total. + +The class is data rather than a decorator on the handler on purpose. A handler's +`limits.take(...)` or `config: { rateLimit }` says how a route is paced **today**; +the table says how it should be. Keeping them apart means the classifier can be +complete before every route is paced, and it means a route whose pacing is +removed stays classified — the removal is visible as a diff on the enforcement +line, not as a silent change of policy. + +## The ordinary class is unrestricted by decision + +It is listed route by route, never implied: a route is ordinary because someone +wrote it in `ORDINARY_ROUTE_LIST`, not because it was left out of the other two. +Chief approved that class on 2026-11-14 on the ground that a form repost is not +an attack surface worth a shared budget — the blast radius of an ordinary write +is one record the account already owns. + +The re-review condition: the first time an ordinary route's blast radius is not +"one record the account owns" — it starts work, moves money, or becomes +reachable with wider scope than the session — it moves to `expensive` or +`destructive` in the same change that grants it that reach. A class never +expands because nobody noticed. + +## What this slice paces, and what is still pending + +The card's five privileged routes are the reason the card exists. Two of them +(`POST /admin/users/:id/role`, `POST /repos/:id/run`) were paced by the first +slice; this one paces the remaining four: + +- `POST /repos/:id/role` and `POST /settings/providers/:id/delete` — destructive, + through `config: { rateLimit }` so the ceiling stays visible to an analyzer; +- `POST /repos/:id/team/stop` and `POST /repos/:id/jobs/:jobId/merge` — + expensive, through the class limiter with the route's own bucket. + +The eight routes that already carried `destructiveLimit` keep that window; they +gain a class and nothing else. Every other destructive or expensive route in the +table is **classified but not yet paced**, and `#708` stays open for them: the +classification is the policy, pacing the rest is the remaining work. That is a +stated gap, not an omission — the table makes the list explicit and the sweep +makes it current. + +Some ordinary routes carry a control that is not a class budget: the sign-in +routes and the session-revocation routes keep the `login` window against +credential guessing, and the machine endpoints under `/api/` keep the 120-a-minute +`apiLimiter` that paces callers rather than forms. Those controls defend a +different thing; being ordinary says they take no **class** budget. + +## Consequences + +- Adding a mutating route now costs one line in the table. Forgetting it is a red + test that names the route, not a quiet unlimited route. +- Renaming or deleting a route without the table turns the same test red, so the + table cannot silently describe routes that no longer exist. +- The classifier is complete while enforcement is staged: `routeClassOf` answers + for every route, and the paced set is smaller than the classified set until the + remaining slices land. diff --git a/docs/security.md b/docs/security.md index f2caa742..ec5f3c6a 100644 --- a/docs/security.md +++ b/docs/security.md @@ -145,17 +145,14 @@ written or audited, and it succeeds again once the window has passed. ### Rate limiting of mutating routes -#708 gives the browser-facing mutations a class and a budget instead of leaving -each one unlimited: +#708 gives the browser-facing mutations a class and, where the class earns one, a +budget: - **login**: 30 attempts a minute per route and client address — the window the sign-in routes (`/auth/token`, `/auth/github`, the callback) already carried. - **destructive**: 60 a minute by default — privilege changes and policy writes. - This slice limits `POST /admin/users/:id/role` and `POST /repos/:id/rules` - this way. - **expensive**: 60 a minute by default — routes that start real work or spend a - host slot. This slice limits `POST /repos` (it provisions an instance) and - `POST /repos/:id/run`. + host slot. The three budgets live in `src/lib/rateLimit.ts` as one `RouteLimiters`, keyed by `rateLimitKey` and prefixed with the route's own bucket, so two routes in the @@ -166,11 +163,35 @@ change rather than a source edit. A refused browser mutation answers the same bilingual `请求过多` / `Too many requests` page the sign-in routes return, never a bare status a form post cannot explain. -The rest of the state-changing routes are **not yet classified**: they are -neither limited nor exempted by a recorded decision, and #708 stays open for the -classifying sweep, the ADR and the remaining privileged routes. A route added -tomorrow inherits nothing from this section, and the eight `config.rateLimit` -handlers above are unchanged by it — same window, same key. +**Every mutating route names its class**, in `src/lib/routeClasses.ts`: one +explicit set per class, in which a route appears by being written there and never +by being left out. The rule is the one in ADR +[0022](decisions/0022-mutation-route-classes.md): *destructive* is irreversible +or privilege-changing, *expensive* starts, stops or merges real work, and +*ordinary* is everything else. Ordinary is unlimited **by decision** — Chief, +2026-11-14 — and the ADR carries the condition that forces a re-review: the first +time an ordinary route does more than write a record the account already owns, it +moves class in the same change. + +`test/routeClassSweep.test.ts` enforces it without a list or a count: it reads +the registrations out of `src/app.ts`, compares them with the table in both +directions, and fails naming the route — `unclassified: PATCH /repos/:id/schedule` +or `stale classification: POST /some/route` — when a mutation has no class or the +table describes a route that no longer exists. A route added tomorrow is red +until it names a class, so it can no longer be unlimited by accident. + +Classified is not yet the same as paced. The eight handlers that name +`config.rateLimit` above keep their window and gain a class and nothing else; +`POST /admin/users/:id/role` and `POST /repos/:id/run` were paced by an earlier +slice, and `POST /repos/:id/role`, `POST /settings/providers/:id/delete`, +`POST /repos/:id/team/stop` and `POST /repos/:id/jobs/:jobId/merge` are paced +here. The rest of the destructive and expensive routes are classified but not yet +limited, which is the remaining work on #708 and is visible as such in the table +rather than being silently unlimited. Some ordinary routes carry a control that +is not a class budget — the `login` window on the sign-in and session-revocation +routes, the 120-a-minute `apiLimiter` on the machine endpoints under `/api/` — +because those defend credential guessing and caller volume, not a class' blast +radius. ### CSRF diff --git a/src/app.ts b/src/app.ts index ca597baf..26a433fb 100644 --- a/src/app.ts +++ b/src/app.ts @@ -596,6 +596,15 @@ export async function buildApp(options: BuildOptions): Promise { keyGenerator: (request: FastifyRequest): string => rateLimitKey(request), } + // #708: the destructive class budget, read from config, for the routes that + // take the class after the eight that keep their literal window. Same key + // generator, so the ceiling is still a property of the route and the address. + const destructiveClassLimit = { + max: config.rateLimit.destructive.max, + timeWindow: config.rateLimit.destructive.windowMs, + keyGenerator: (request: FastifyRequest): string => rateLimitKey(request), + } + const app = Fastify({ logger: false, // Only the hops configured in TRUST_PROXY may add X-Forwarded-For; the @@ -2800,6 +2809,8 @@ export async function buildApp(options: BuildOptions): Promise { }) app.post('/repos/:id/team/stop', async (request, reply) => { + // #708: expensive — one team turn, paced before the session is read. + if (!limits.take('expensive', `team-stop:${rateLimitKey(request)}`)) return refuseTooMany(request, reply) const { viewer, repo } = ownedRepo(request, String((request.params as Record).id), true) requireAdvanced(viewer) const body = form(request) @@ -3322,6 +3333,8 @@ export async function buildApp(options: BuildOptions): Promise { }) app.post('/repos/:id/jobs/:jobId/merge', async (request, reply) => { + // #708: expensive — a merge writes to the upstream repository. + if (!limits.take('expensive', `job-merge:${rateLimitKey(request)}`)) return refuseTooMany(request, reply) const { viewer, repo } = ownedRepo(request, String((request.params as Record).id), true) const job = store.getJob(String((request.params as Record).jobId)) if (job === undefined || job.repo_id !== repo.id) throw httpError(404, '任务不存在。', 'JOB_NOT_FOUND') @@ -3770,7 +3783,7 @@ export async function buildApp(options: BuildOptions): Promise { return reply.redirect(`/repos/${repo.id}?notice=${encodeURIComponent(t('审核任务已排队。', 'Review job queued.'))}`) }) - app.post('/repos/:id/role', async (request, reply) => { + app.post('/repos/:id/role', { config: { rateLimit: destructiveClassLimit } }, async (request, reply) => { const { viewer, repo } = ownedRepo(request, String((request.params as Record).id), true) const body = form(request) const t = translator(request.locale ?? 'zh') @@ -4102,7 +4115,7 @@ export async function buildApp(options: BuildOptions): Promise { return reply.redirect(`/settings?${failed.length === 0 ? 'notice' : 'error'}=${encodeURIComponent(message)}#model-providers`) }) - app.post('/settings/providers/:id/delete', async (request, reply) => { + app.post('/settings/providers/:id/delete', { config: { rateLimit: destructiveClassLimit } }, async (request, reply) => { const viewer = approvedCsrfGuard(request) const id = String((request.params as Record).id) const provider = store.modelProviderById(id, viewer.user.id) diff --git a/src/lib/routeClasses.ts b/src/lib/routeClasses.ts new file mode 100644 index 00000000..58c13b47 --- /dev/null +++ b/src/lib/routeClasses.ts @@ -0,0 +1,142 @@ +// #708: every mutating route names one class here, and this table is the class +// policy the classifier test reads. A route that is not in it is unclassified, +// which is a test failure that names it — not a default. The table is explicit in +// both directions: a route removed from `src/app.ts` and left here is a stale +// entry, and the same test fails on that too. +// +// The rule (Lead, 2026-11-14; approver Chief for the ordinary class): +// 1. destructive — irreversible or privilege-changing; +// 2. expensive — starts, stops or merges real work; +// 3. ordinary — everything else that changes state, unlimited **by decision**. +// +// The classes are declared as data rather than read off the handlers because a +// handler's `limits.take(...)` says how a route is paced, not how it should be: +// classification is the policy, enforcement is the current state of it, and the +// two are compared where the limits are applied (#708's later slice). +export type RouteClass = 'destructive' | 'expensive' | 'ordinary' + +// Irreversible or privilege-changing: a role, a token, a credential, a policy or +// a repository goes, or an account's standing changes. +const DESTRUCTIVE_ROUTE_LIST = [ + 'POST /admin/users/:id/role', + 'POST /admin/users/:id/private-models', + 'POST /admin/users/:id/resend-approval', + 'POST /admin/users/:id/approve', + 'POST /admin/users/:id/reject', + 'POST /admin/users/:id/quota', + 'POST /admin/users/:id/delete', + 'POST /admin/simulation', + 'POST /settings/providers', + 'POST /settings/providers/:id/update', + 'POST /settings/providers/:id/discover', + 'POST /settings/providers/:id/models', + 'POST /settings/providers/:id/models/bulk', + 'POST /settings/providers/:id/models/:modelId/delete', + 'POST /settings/providers/:id/delete', + 'POST /settings/tokens', + 'POST /settings/tokens/:id/revoke', + 'POST /settings/sessions/:hash/revoke', + 'POST /settings/sessions/revoke-others', + 'POST /settings/sessions/rotate', + 'POST /repos/:id/delete', + 'POST /repos/:id/settings', + 'POST /repos/:id/instance/stop', + 'POST /repos/:id/role', + 'POST /repos/:id/release-readiness/tag', + 'POST /repos/:id/security-findings/:findingId/dismiss', + 'POST /admin/deployments/promote', + 'POST /admin/deployments/rollback', + 'POST /admin/platform/stop', + 'POST /admin/platform/resume', +] as const + +// Starts, stops or merges real work: a job, a plan, a release, a schedule, an +// instance or a team turn. +const EXPENSIVE_ROUTE_LIST = [ + 'POST /repos/:id/run', + 'POST /repos/:id/plan', + 'POST /repos/:id/triage', + 'POST /repos/:id/refresh', + 'POST /repos/:id/review', + 'POST /repos/:id/enable', + 'POST /repos/:id/disable', + 'POST /repos/:id/models/apply', + 'POST /repos/:id/schedule', + 'POST /repos/:id/schedule/remove', + 'POST /repos/:id/release', + 'POST /repos/:id/releases/:releaseId/check', + 'POST /repos/:id/gate/sessions', + 'POST /repos/:id/gate/sessions/:sessionId/confirm', + 'POST /repos/:id/delivery/merge-clean', + 'POST /repos/:id/work', + 'POST /repos/:id/work/:workItemId/state', + 'POST /repos/:id/work/:workItemId/run', + 'POST /repos/:id/work/:workItemId/scope', + 'POST /repos/:id/work/:workItemId/verify', + 'POST /repos/:id/team/stop', + 'POST /repos/:id/team/answer', + 'POST /api/repos/:id/team/stop', + 'POST /api/repos/:id/team/answer', + 'POST /repos/:id/chief/message', + 'POST /api/repos/:id/chief/message', + 'POST /repos/:id/jobs/:jobId/cancel', + 'POST /repos/:id/jobs/:jobId/pause', + 'POST /repos/:id/jobs/:jobId/resume', + 'POST /repos/:id/jobs/:jobId/retry', + 'POST /repos/:id/jobs/:jobId/recheck', + 'POST /repos/:id/jobs/:jobId/merge', + 'POST /repos/:id/jobs/:jobId/approve-plan', + 'POST /repos/:id/jobs/:jobId/reject-plan', + 'POST /repos/:id/jobs/:jobId/execute', + 'POST /repos/:id/security-findings/:findingId/fix', + 'POST /admin/webhooks/:id/retry', +] as const + +// Everything else that changes state. Listed rather than implied: "the rest are +// ordinary" names no route and decides no route, so an unlimited route has to +// appear here to be unlimited on purpose. +const ORDINARY_ROUTE_LIST = [ + 'POST /auth/token', + 'POST /auth/logout', + 'POST /apply', + 'POST /repos', + 'POST /repos/app', + 'POST /repos/:id/instance', + 'POST /repos/:id/memory', + 'POST /repos/:id/rules', + 'POST /repos/:id/rules/delete', + 'POST /repos/:id/team/guidance', + 'POST /repos/:id/jobs/:jobId/priority', + 'POST /repos/:id/jobs/:jobId/relations', + 'POST /repos/:id/jobs/:jobId/decision', + 'POST /repos/:id/jobs/:jobId/ack-rule-change', + 'POST /settings/notifications', + 'POST /notifications/read', + 'POST /approve/:token', + 'POST /api/jobs/:id/callback', + 'POST /api/github/webhook', + 'POST /api/inbound/email', +] as const + +export const DESTRUCTIVE_ROUTES: ReadonlySet = new Set(DESTRUCTIVE_ROUTE_LIST) +export const EXPENSIVE_ROUTES: ReadonlySet = new Set(EXPENSIVE_ROUTE_LIST) +export const ORDINARY_ROUTES: ReadonlySet = new Set(ORDINARY_ROUTE_LIST) + +// The sweep's key: `METHOD /path`, the same shape the failure message prints. +export function routeKey(method: string, path: string): string { + return `${method.toUpperCase()} ${path}` +} + +export function routeClassOf(method: string, path: string): RouteClass | undefined { + const key = routeKey(method, path) + if (DESTRUCTIVE_ROUTES.has(key)) return 'destructive' + if (EXPENSIVE_ROUTES.has(key)) return 'expensive' + if (ORDINARY_ROUTES.has(key)) return 'ordinary' + return undefined +} + +// Every key this table classifies, once: the sweep compares this set with the +// routes it reads out of `src/app.ts`. +export function classifiedRoutes(): string[] { + return [...DESTRUCTIVE_ROUTES, ...EXPENSIVE_ROUTES, ...ORDINARY_ROUTES].sort() +} diff --git a/test/routeClassSweep.test.ts b/test/routeClassSweep.test.ts new file mode 100644 index 00000000..2b604885 --- /dev/null +++ b/test/routeClassSweep.test.ts @@ -0,0 +1,69 @@ +import test from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { + DESTRUCTIVE_ROUTES, + EXPENSIVE_ROUTES, + ORDINARY_ROUTES, + classifiedRoutes, + routeClassOf, + routeKey, +} from '../src/lib/routeClasses.js' + +// #708: this file is the enforcement the card asks for. It reads the route +// registrations out of `src/app.ts` — not a list, not a count, not a number +// anyone wrote down — and compares them with the class table. A route whose +// class nobody declared fails here and is named, so "somebody will notice" is +// replaced by a red test on the day the route is added. +// +// Two directions, because a table can rot either way: a route registered and +// left unclassified is the defect the card describes, and a class left behind +// for a route that no longer exists would keep the sweep looking complete. + +const APP_SOURCE = readFileSync(new URL('../src/app.ts', import.meta.url), 'utf8') + +// Comments are removed before the scan: a commented-out registration is not a +// route, and this file's own explanations quote route shapes. +function withoutComments(source: string): string { + return source.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^[ \t]*\/\/.*$/gm, '') +} + +/** Every mutating route `src/app.ts` registers, as `METHOD /path`. */ +export function registeredMutations(source = APP_SOURCE): string[] { + const text = withoutComments(source) + const pattern = /app\.(get|post|put|patch|delete)\(\s*'([^']+)'/g + const found: string[] = [] + for (const match of text.matchAll(pattern)) { + const method = (match[1] ?? '').toUpperCase() + if (method === 'GET') continue + found.push(routeKey(method, match[2] ?? '')) + } + return found +} + +test('#708 the sweep reads the route table, not a number written down', () => { + const routes = registeredMutations() + // Not a pinned count: this only refuses a sweep that found nothing, which is + // what a broken pattern or a moved file would look like. + assert.ok(routes.length > 0, 'the sweep found no mutating route in src/app.ts; the pattern is wrong, not the code') + assert.equal(new Set(routes).size, routes.length, 'the same mutating route is registered twice, so the sweep is ambiguous') +}) + +test('#708 every mutating route in src/app.ts names a class', () => { + const unclassified = registeredMutations().filter((key) => routeClassOf(...(key.split(' ') as [string, string])) === undefined) + assert.deepEqual(unclassified, [], `unclassified: ${unclassified.join(', ')}`) +}) + +test('#708 the class table names no route that src/app.ts does not register', () => { + const registered = new Set(registeredMutations()) + const stale = classifiedRoutes().filter((key) => !registered.has(key)) + assert.deepEqual(stale, [], `stale classification: ${stale.join(', ')}`) +}) + +test('#708 a route belongs to exactly one class', () => { + const overlap = [ + ...[...DESTRUCTIVE_ROUTES].filter((key) => EXPENSIVE_ROUTES.has(key) || ORDINARY_ROUTES.has(key)), + ...[...EXPENSIVE_ROUTES].filter((key) => ORDINARY_ROUTES.has(key)), + ] + assert.deepEqual(overlap, [], `a route is in two classes: ${overlap.join(', ')}`) +}) diff --git a/test/routeRateLimit.test.ts b/test/routeRateLimit.test.ts index 4c16d183..41edcf2e 100644 --- a/test/routeRateLimit.test.ts +++ b/test/routeRateLimit.test.ts @@ -51,7 +51,7 @@ function post(built: Built, url: string, cookie: string, payload: string, locale // otherwise be answered (a redirect, a 400 or a 403), so only the limiter can // turn the last one into a 429. The routes keep separate buckets: exhausting one // destructive route must not refuse another. -test('the five state-changing POST routes refuse the request after their class budget', async () => { +test('the paced state-changing POST routes refuse the request after their class budget', async () => { await withApp(limitedConfig(), async (built, store) => { const superadmin = makeUser(store, { login: 'root-user', admin: true, superadmin: true, status: 'approved' }) const target = makeUser(store, { login: 'target-user', status: 'approved' }) @@ -67,6 +67,14 @@ test('the five state-changing POST routes refuse the request after their class b { name: 'repository add', url: '/repos', cookie: ownerCookie, payload: formBody({ csrf: ownerCsrf }) }, { name: 'rule save', url: `/repos/${repo.id}/rules`, cookie: ownerCookie, payload: formBody({ csrf: ownerCsrf, path: '../escape', content: 'x', expected_revision: '0' }) }, { name: 'job start', url: `/repos/${repo.id}/run`, cookie: ownerCookie, payload: formBody({ csrf: ownerCsrf }) }, + // #708: the remaining privileged routes the card names. The provider id + // and the job id do not exist, which is the point: the route is paced + // before it looks anything up, so a caller cannot probe for existence + // harder than it can ask. + { name: 'repository role', url: `/repos/${repo.id}/role`, cookie: ownerCookie, payload: formBody({ csrf: ownerCsrf, role: 'developer' }) }, + { name: 'provider delete', url: '/settings/providers/nope/delete', cookie: ownerCookie, payload: formBody({ csrf: ownerCsrf }) }, + { name: 'team stop', url: `/repos/${repo.id}/team/stop`, cookie: ownerCookie, payload: formBody({ csrf: ownerCsrf, target: 'chief' }) }, + { name: 'job merge', url: `/repos/${repo.id}/jobs/nope/merge`, cookie: ownerCookie, payload: formBody({ csrf: ownerCsrf }) }, ] for (const route of cases) { From 28b8f0cd0a3888a74effc564fd28fc8cb4cb05d5 Mon Sep 17 00:00:00 2001 From: HugoXK Date: Sun, 15 Nov 2026 10:52:00 +0000 Subject: [PATCH 2/4] fix(gates): classify the workspace proxy and refuse an unknown registration form (#708) --- docs/decisions/0022-mutation-route-classes.md | 10 ++++ src/lib/routeClasses.ts | 14 +++++ test/routeClassSweep.test.ts | 56 +++++++++++++++++-- 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/docs/decisions/0022-mutation-route-classes.md b/docs/decisions/0022-mutation-route-classes.md index b069e609..4da21fae 100644 --- a/docs/decisions/0022-mutation-route-classes.md +++ b/docs/decisions/0022-mutation-route-classes.md @@ -84,6 +84,16 @@ credential guessing, and the machine endpoints under `/api/` keep the 120-a-minu `apiLimiter` that paces callers rather than forms. Those controls defend a different thing; being ordinary says they take no **class** budget. +**Classification is not enforcement.** A green sweep says every mutating route +has a class; it does not say every class has a budget. Read the table as policy +and `limits.take(...)` as what is paced today — they are compared in a later +slice, and until then 28 of the 30 destructive routes and most of the +`expensive` list are classified and unpaced. The workspace proxy is the clearest +case: `app.all` on `/repos/:id/workspace/app` and `/repos/:id/workspace/app/*` +answers every method, so its mutating forms are in the table as `expensive` +because a body forwarded to the per-repo workspace app can start work inside the +instance; none of them is paced yet. + ## Consequences - Adding a mutating route now costs one line in the table. Forgetting it is a red diff --git a/src/lib/routeClasses.ts b/src/lib/routeClasses.ts index 58c13b47..4104516c 100644 --- a/src/lib/routeClasses.ts +++ b/src/lib/routeClasses.ts @@ -88,6 +88,20 @@ const EXPENSIVE_ROUTE_LIST = [ 'POST /repos/:id/jobs/:jobId/approve-plan', 'POST /repos/:id/jobs/:jobId/reject-plan', 'POST /repos/:id/jobs/:jobId/execute', + // The workspace proxy is registered with `app.all` (the two + // `/repos/:id/workspace/app…` registrations in `src/app.ts`), so it answers + // every method; the mutating ones forward their body to the per-repo workspace + // app, where they can start work inside the instance. That is why they are + // expensive rather than ordinary. Nothing paces them yet: classification is + // the policy, enforcement is the later slice. + 'POST /repos/:id/workspace/app', + 'PUT /repos/:id/workspace/app', + 'PATCH /repos/:id/workspace/app', + 'DELETE /repos/:id/workspace/app', + 'POST /repos/:id/workspace/app/*', + 'PUT /repos/:id/workspace/app/*', + 'PATCH /repos/:id/workspace/app/*', + 'DELETE /repos/:id/workspace/app/*', 'POST /repos/:id/security-findings/:findingId/fix', 'POST /admin/webhooks/:id/retry', ] as const diff --git a/test/routeClassSweep.test.ts b/test/routeClassSweep.test.ts index 2b604885..ca96fddb 100644 --- a/test/routeClassSweep.test.ts +++ b/test/routeClassSweep.test.ts @@ -19,6 +19,10 @@ import { // Two directions, because a table can rot either way: a route registered and // left unclassified is the defect the card describes, and a class left behind // for a route that no longer exists would keep the sweep looking complete. +// +// The reader understands every registration form the file uses: `app.all` is one +// registration but four mutating routes, so it is expanded, and a form the reader +// does not know is refused loudly instead of being skipped. const APP_SOURCE = readFileSync(new URL('../src/app.ts', import.meta.url), 'utf8') @@ -28,15 +32,34 @@ function withoutComments(source: string): string { return source.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^[ \t]*\/\/.*$/gm, '') } +// The registration forms this reader understands. `all` is the one that carries +// more than a single method, and a form outside this set is refused rather than +// skipped: an unread form is an unclassified route, which is the defect the sweep +// exists to catch. +const ROUTE_FORMS = new Set(['get', 'post', 'put', 'patch', 'delete', 'all']) +// `app.all` answers every method. Only the methods that can change state are +// swept; a GET through the same registration is a read and stays out. +const ALL_MUTATING_METHODS = ['POST', 'PUT', 'PATCH', 'DELETE'] as const + /** Every mutating route `src/app.ts` registers, as `METHOD /path`. */ export function registeredMutations(source = APP_SOURCE): string[] { const text = withoutComments(source) - const pattern = /app\.(get|post|put|patch|delete)\(\s*'([^']+)'/g const found: string[] = [] - for (const match of text.matchAll(pattern)) { - const method = (match[1] ?? '').toUpperCase() - if (method === 'GET') continue - found.push(routeKey(method, match[2] ?? '')) + for (const match of text.matchAll(/app\.([A-Za-z]+)\(\s*'([^']*)'/g)) { + const form = (match[1] ?? '').toLowerCase() + const path = match[2] ?? '' + if (!ROUTE_FORMS.has(form)) { + // `app.addHook('onResponse', …)` is not a route; `app.options('/x', …)` is. + // Refuse the second kind instead of letting it past unread. + if (path.startsWith('/')) throw new Error(`the sweep cannot read app.${form}('${path}'); teach registeredMutations or drop the form`) + continue + } + if (form === 'all') { + for (const method of ALL_MUTATING_METHODS) found.push(routeKey(method, path)) + continue + } + if (form === 'get') continue + found.push(routeKey(form.toUpperCase(), path)) } return found } @@ -49,6 +72,29 @@ test('#708 the sweep reads the route table, not a number written down', () => { assert.equal(new Set(routes).size, routes.length, 'the same mutating route is registered twice, so the sweep is ambiguous') }) +// `app.all` answers every method, so one registration can carry more than one +// mutating route. The reader has to see that form or the sweep misses them +// silently — the workspace proxy is registered that way and forwards a body. +test('#708 an app.all registration is read as every mutating method', () => { + const read = registeredMutations("app.all('/repos/:id/workspace/app/*', async () => {})") + assert.deepEqual(read, [ + 'POST /repos/:id/workspace/app/*', + 'PUT /repos/:id/workspace/app/*', + 'PATCH /repos/:id/workspace/app/*', + 'DELETE /repos/:id/workspace/app/*', + ]) +}) + +// The other half of the same defect: a registration form the reader does not +// know is not "not a route" — it is a route nobody classified. Refusing it keeps +// the next `app.all` or `app.options` from passing a green sweep. +test('#708 the sweep refuses a registration form it cannot read', () => { + assert.throws( + () => registeredMutations("app.options('/repos/:id/settings', async () => {})"), + /cannot read app\.options/, + ) +}) + test('#708 every mutating route in src/app.ts names a class', () => { const unclassified = registeredMutations().filter((key) => routeClassOf(...(key.split(' ') as [string, string])) === undefined) assert.deepEqual(unclassified, [], `unclassified: ${unclassified.join(', ')}`) From 49e0c7d8bf6b0fee972083a84bd0a141fa3e676b Mon Sep 17 00:00:00 2001 From: HugoXK Date: Sun, 15 Nov 2026 12:24:00 +0000 Subject: [PATCH 3/4] fix(gates): file every route that starts work under expensive (#708) --- docs/decisions/0022-mutation-route-classes.md | 2 +- src/lib/routeClasses.ts | 13 ++++-- test/routeClassSweep.test.ts | 42 +++++++++++++++++++ 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/docs/decisions/0022-mutation-route-classes.md b/docs/decisions/0022-mutation-route-classes.md index 4da21fae..44246275 100644 --- a/docs/decisions/0022-mutation-route-classes.md +++ b/docs/decisions/0022-mutation-route-classes.md @@ -87,7 +87,7 @@ different thing; being ordinary says they take no **class** budget. **Classification is not enforcement.** A green sweep says every mutating route has a class; it does not say every class has a budget. Read the table as policy and `limits.take(...)` as what is paced today — they are compared in a later -slice, and until then 28 of the 30 destructive routes and most of the +slice, and until then 18 of the 31 destructive routes and most of the `expensive` list are classified and unpaced. The workspace proxy is the clearest case: `app.all` on `/repos/:id/workspace/app` and `/repos/:id/workspace/app/*` answers every method, so its mutating forms are in the table as `expensive` diff --git a/src/lib/routeClasses.ts b/src/lib/routeClasses.ts index 4104516c..04ba0ac2 100644 --- a/src/lib/routeClasses.ts +++ b/src/lib/routeClasses.ts @@ -41,6 +41,9 @@ const DESTRUCTIVE_ROUTE_LIST = [ 'POST /repos/:id/delete', 'POST /repos/:id/settings', 'POST /repos/:id/instance/stop', + // A rule override steers every later job on the repository, and the handler + // already takes the destructive window; the class follows the window. + 'POST /repos/:id/rules', 'POST /repos/:id/role', 'POST /repos/:id/release-readiness/tag', 'POST /repos/:id/security-findings/:findingId/dismiss', @@ -53,6 +56,12 @@ const DESTRUCTIVE_ROUTE_LIST = [ // Starts, stops or merges real work: a job, a plan, a release, a schedule, an // instance or a team turn. const EXPENSIVE_ROUTE_LIST = [ + // Adding a repository provisions an instance (the handler says so and takes the + // expensive window itself), and importing an app provisions every imported + // repository: both start work, so ADR 0022 files them expensive, not ordinary. + 'POST /repos', + 'POST /repos/app', + 'POST /repos/:id/instance', 'POST /repos/:id/run', 'POST /repos/:id/plan', 'POST /repos/:id/triage', @@ -113,11 +122,7 @@ const ORDINARY_ROUTE_LIST = [ 'POST /auth/token', 'POST /auth/logout', 'POST /apply', - 'POST /repos', - 'POST /repos/app', - 'POST /repos/:id/instance', 'POST /repos/:id/memory', - 'POST /repos/:id/rules', 'POST /repos/:id/rules/delete', 'POST /repos/:id/team/guidance', 'POST /repos/:id/jobs/:jobId/priority', diff --git a/test/routeClassSweep.test.ts b/test/routeClassSweep.test.ts index ca96fddb..20b7592a 100644 --- a/test/routeClassSweep.test.ts +++ b/test/routeClassSweep.test.ts @@ -44,6 +44,10 @@ const ALL_MUTATING_METHODS = ['POST', 'PUT', 'PATCH', 'DELETE'] as const /** Every mutating route `src/app.ts` registers, as `METHOD /path`. */ export function registeredMutations(source = APP_SOURCE): string[] { const text = withoutComments(source) + // `app.route({ method, url })` keeps the method in an object, which this + // reader cannot read. Refuse it: a skipped registration is an unclassified + // mutating route passing a green sweep. + if (/app\.route\s*\(/.test(text)) throw new Error('the sweep cannot read app.route(...); teach registeredMutations or register with a verb') const found: string[] = [] for (const match of text.matchAll(/app\.([A-Za-z]+)\(\s*'([^']*)'/g)) { const form = (match[1] ?? '').toLowerCase() @@ -95,6 +99,44 @@ test('#708 the sweep refuses a registration form it cannot read', () => { ) }) +// The table is policy and the handler is what happens today. Where the two +// disagree the table must not be the lighter one: a route that takes the +// destructive window handles something irreversible, and a route that takes the +// expensive window starts work — neither can be filed under `ordinary`, which +// ADR 0022 defines as unlimited by decision. +test('#708 a route that takes a class window is not filed under a lighter class', () => { + const text = withoutComments(APP_SOURCE) + const matches = [...text.matchAll(/app\.([A-Za-z]+)\(\s*'([^']*)'/g)] + const wrong: string[] = [] + matches.forEach((match, index) => { + const form = (match[1] ?? '').toLowerCase() + if (!ROUTE_FORMS.has(form)) return + const path = match[2] ?? '' + const end = index + 1 < matches.length ? (matches[index + 1]?.index ?? text.length) : text.length + const handler = text.slice(match.index ?? 0, end) + const methods = form === 'all' ? [...ALL_MUTATING_METHODS] : form === 'get' ? [] : [form.toUpperCase()] + const takesDestructive = /rateLimit:\s*\w*destructive/i.test(handler) || /limits\.take\('destructive'/.test(handler) + const takesExpensive = /rateLimit:\s*\w*expensive/i.test(handler) || /limits\.take\('expensive'/.test(handler) + for (const method of methods) { + const key = routeKey(method, path) + const says = routeClassOf(method, path) + if (takesDestructive && says !== 'destructive') wrong.push(`${key}: takes the destructive window but is ${says ?? 'unclassified'}`) + else if (takesExpensive && says === 'ordinary') wrong.push(`${key}: takes the expensive window but is ordinary`) + } + }) + assert.deepEqual(wrong, [], `class lighter than the window: ${wrong.join('; ')}`) +}) + +// `app.route({ method, url })` carries the method in an object, so the sweep's +// shape cannot read it. Refusing it keeps the next one from being skipped. +test('#708 the sweep refuses app.route, whose method is in an object', () => { + assert.throws( + () => registeredMutations("app.route({ method: 'POST', url: '/repos/:id/settings' }, async () => {})"), + /cannot read app\.route/, + ) + assert.equal(/app\.route\s*\(/.test(withoutComments(APP_SOURCE)), false, 'src/app.ts must register with a verb, not app.route: the sweep reads verb registrations') +}) + test('#708 every mutating route in src/app.ts names a class', () => { const unclassified = registeredMutations().filter((key) => routeClassOf(...(key.split(' ') as [string, string])) === undefined) assert.deepEqual(unclassified, [], `unclassified: ${unclassified.join(', ')}`) From ede54fee3ad346ed54cc4e5f5faeda0b36bb4ba0 Mon Sep 17 00:00:00 2001 From: HugoXK Date: Sun, 15 Nov 2026 12:28:00 +0000 Subject: [PATCH 4/4] docs(adr): state the paced-route counts computed from the tree (#708) --- docs/decisions/0022-mutation-route-classes.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/decisions/0022-mutation-route-classes.md b/docs/decisions/0022-mutation-route-classes.md index 44246275..3d08810d 100644 --- a/docs/decisions/0022-mutation-route-classes.md +++ b/docs/decisions/0022-mutation-route-classes.md @@ -87,8 +87,9 @@ different thing; being ordinary says they take no **class** budget. **Classification is not enforcement.** A green sweep says every mutating route has a class; it does not say every class has a budget. Read the table as policy and `limits.take(...)` as what is paced today — they are compared in a later -slice, and until then 18 of the 31 destructive routes and most of the -`expensive` list are classified and unpaced. The workspace proxy is the clearest +slice. Counted from this tree: 13 of the 31 destructive routes and 4 of the 48 +expensive routes carry a budget, so 18 destructive and 44 expensive routes are +classified and unpaced. The workspace proxy is the clearest case: `app.all` on `/repos/:id/workspace/app` and `/repos/:id/workspace/app/*` answers every method, so its mutating forms are in the table as `expensive` because a body forwarded to the per-repo workspace app can start work inside the