feat(security): classify every mutating route and pace the worst ones

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.

docs/decisions/0022-mutation-route-classes.md

@@ -0,0 +1,109 @@
+# 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.
+
+**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. Counted from this tree: 13 of the 31 destructive routes and 4 of the 49
+expensive routes carry a budget, so 18 destructive and 45 expensive routes are
+classified and unpaced. That includes the gate routes main added while this
+branch was open — applying a confirmed session writes a branch and opens one
+pull request, so `POST /repos/:id/gate/sessions/:sessionId/create` is
+`expensive`, and like the rest of the list it stays 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
+  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.

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
 

src/app.ts

@@ -602,6 +602,15 @@ export async function buildApp(options: BuildOptions): Promise<BuiltApp> {
     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
@@ -2965,6 +2974,8 @@ export async function buildApp(options: BuildOptions): Promise<BuiltApp> {
   })
 
   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<string, string>).id), true)
     requireAdvanced(viewer)
     const body = form(request)
@@ -3487,6 +3498,8 @@ export async function buildApp(options: BuildOptions): Promise<BuiltApp> {
   })
 
   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<string, string>).id), true)
     const job = store.getJob(String((request.params as Record<string, string>).jobId))
     if (job === undefined || job.repo_id !== repo.id) throw httpError(404, '任务不存在。', 'JOB_NOT_FOUND')
@@ -3935,7 +3948,7 @@ export async function buildApp(options: BuildOptions): Promise<BuiltApp> {
     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<string, string>).id), true)
     const body = form(request)
     const t = translator(request.locale ?? 'zh')
@@ -4283,7 +4296,7 @@ export async function buildApp(options: BuildOptions): Promise<BuiltApp> {
     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<string, string>).id)
     const provider = store.modelProviderById(id, viewer.user.id)