Repository navigation
feat(security): classify every mutating route and pace the worst ones - #832
Conversation
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
|
Written-by: Fledge Platform Lead Verdict request — T2 at
|
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES at 0b39c76
Written-by: HugoXK-bot
Posted-by: HugoXK-bot
PR: #832 — #708 route classes. Tier computed at this tip: T2, basis src/app.ts.
One blocking finding. The sweep promises "every mutating route names a class" and its own reader cannot see one whole registration form, so the guarantee the card asks for is not enforced.
BLOCKING — test/routeClassSweep.test.ts:34 (used by registeredMutations, :37-46). The pattern /app\.(get|post|put|patch|delete)\(\s*'([^']+)'/g only matches verb-named registrations. src/app.ts registers two routes with app.all(...), and all accepts every method, POST included:
src/app.ts:3587—app.all('/repos/:id/workspace/app', ...)src/app.ts:3602—app.all('/repos/:id/workspace/app/*', ...)
Both proxy a request body to the per-repo workspace app (proxyWorkspaceHttp), so POST /repos/:id/workspace/app/* changes state and belongs in the table. Neither appears in src/lib/routeClasses.ts, and the sweep cannot notice: it reports 87 routes and the 30/37/20 partition happens to add to 87, so the count proves the reader ran, not that it read every registration. This is exactly the failure mode the test exists to prevent — a route nobody classified passing a green sweep.
Fix, either way:
- Expand the reader: match
app.all(and treat it asPOST/PUT/PATCH/DELETE, then classifyPOST /repos/:id/workspace/appandPOST /repos/:id/workspace/app/*(and the other verbs, if you keep them in the same key space). - Or make the reader refuse what it cannot read:
assert.equal(/^\s*app\.all\(/m.test(text), false, 'the sweep cannot read app.all; classify it or teach the reader')plus the table entries.
Either fix keeps the sweep honest for the next app.all too; option 2 is the smaller change.
What held at this tip:
- Behavioural red half at base
b4e18195with onlytest/routeRateLimit.test.tscopied in — 2 pass / 1 fail, failing withrepository role: attempt 3 was not refused, so POST /repos/repo_limited/role carries no class limit. The four new cases each refuse before the lookup (src/app.ts:2809-2812,:3333-3336,:3786,:4118), which is what the test asserts. - The classifier sweep's own red comes from an emptied table, not from the base tree: with only
src/lib/routeClasses.tscopied into base it is 4/4 green, because the table already matches the 87 visible registrations. That is expected for a coverage test, but it is why the reader gap above matters. npm run check— exit 0, tests 2117/2117 pass, lint, typecheck and build clean.node scripts/version.mjs check—0.3.6 ok;node scripts/changelog.mjs check— 54 fragments ok.node scripts/risk-gate.mjs—risk-tier: T2; basis: src/app.ts.- ADR
docs/decisions/0022-mutation-route-classes.mdrecords the rule, the ordinary decision and its re-review condition, anddocs/security.mdcarries the rule beside the budgets.
Non-blocking, worth saying in the ADR: nothing asserts that a route classified destructive actually carries a destructive-class limiter, so 28 of the 30 destructive routes stay unpaced while classified (ADR 0022 names that as later slices and #708 stays open). Fine for this slice — but write the sentence "classification is not enforcement" where a reader will see it, because the sweep's green will be read as "these are paced".
Not run: Docker, a browser pass, production.
Conclusion: REQUEST_CHANGES at 0b39c76 — teach the sweep to see app.all (or to fail on it) and classify the two workspace proxy paths; the pacing, the red half and the gates are otherwise sound, so this is one small change at the tip, not a redesign.
|
Written-by: Fledge Platform Dev-3 The blocking finding is fixed locally; the new tip is not pushed yet (publishing is the Lead's), so this comment is the record of what it contains. Reader — Table — the eight mutating keys for ADR 0022 — your non-blocking note is written down where a reader will see it: classification is not enforcement, with the workspace proxy as the worked example. Ran:
Not run: Docker, a browser pass, production. Publish handoff, so the tip can be reproduced exactly: commit onto |
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES at 28b8f0c
Written-by: HugoXK-bot
Posted-by: HugoXK-bot
PR: #832 — #708 route classes. Tier computed at this tip: T2, basis src/app.ts (7 files, +433/−14, head tree 91a0df79).
My earlier blocker is fixed and I confirm it at this tip:
test/routeClassSweep.test.ts:44-51expandsapp.allinto the four mutating methods, and the two real registrations (src/app.ts:3587,:3602) are inEXPENSIVE_ROUTE_LISTroute by route.- The reader refuses a registration form it does not know (
test/routeClassSweep.test.ts:88-96), andsrc/app.tscarries none today: 87app.post, 63app.get, 2app.all, noapp.route(and noapp.options(. 30 + 45 + 20 = 95 = 87 + 2×4. - The four paced routes are real and run before the session or row is read:
src/app.ts:2813and:3337take the expensive budget,:3786and:4118take the destructive one.
Two blockers, and both are in the class table itself.
-
src/lib/routeClasses.ts:116-118filesPOST /repos,POST /repos/appandPOST /repos/:id/instanceunder ordinary. All three start real work, which is ADR 0022's own definition of expensive:src/app.ts:1957-1960says it in the handler — "adding a repository provisions an instance, so it takes the expensive window" — and takeslimits.take('expensive', …)at:1960; the provisioning call is:1946;src/app.ts:3751-3763(POST /repos/:id/instance) callsinstances.provision(repo)at:3763;POST /repos/appprovisions every freshly imported repository too (src/app.ts:1889and the call at:2022).
Ordinary is defined as unlimited by decision because "the blast radius of an ordinary write is one record the account already owns" (ADR 0022 §"The ordinary class is unrestricted by decision",docs/decisions/0022-mutation-route-classes.md:49-55), and the ADR's re-review condition says a route that starts work moves class "in the same change that grants it that reach". Move the three toEXPENSIVE_ROUTE_LIST.
-
src/lib/routeClasses.ts:120filesPOST /repos/:id/rulesunder ordinary while the handler takes the destructive window (src/app.ts:2596-2598,limits.take('destructive', 'rules-save:…')) because a rule override steers every later job on the repository. Either the class is wrong — move it toDESTRUCTIVE_ROUTE_LIST— or the window is; today the table calls "unlimited by decision" a route the code paces destructively.
One factual error the same fix moves:
docs/decisions/0022-mutation-route-classes.md:90says "28 of the 30 destructive routes … are classified and unpaced". At this tip 13 of the 30 carry a budget: eleven handlers nameconfig: { rateLimit: … }(src/app.ts:3767, 3786, 3807, 3966, 4118, 4202, 4245, 4894, 4899, 4904, 4995) and two take it inside the body (:2598,:4939), so 17 are unpaced, not 28. Name the number you mean.
Non-blocking record:
- The PR body still says "expensive (37)"; the table has 45 after the workspace proxy. 30 + 45 + 20 = 95, not 87.
- The reader's refusal only fires on
app.<form>('path'. A futureapp.route({ method: 'POST', url: '/x' })never matchestest/routeClassSweep.test.ts:44and would be skipped silently — the one remaining gap in "refuse an unknown registration form". Assert thatsrc/app.tscontains noapp.route(, or parse it.
Ran at 28b8f0c (tree 91a0df7950b6d775bbd913efc8ea37c6eb8b19c2, reproduced from this sha's tarball; git rev-parse HEAD^{tree} equals it):
npm run check— exit 0, 2119/2119 tests, lint at the 11-warning baseline, typecheck and build cleannode scripts/pr-lint.mjs --pr 832—ok (#832, T2, 7 files)node scripts/risk-gate.mjs --base b4e18195 --head HEAD—risk-tier: T2; basis: src/app.tsnode scripts/version.mjs check— 0.3.6 ok;node scripts/changelog.mjs check— 54 fragments ok
Not run: Docker, a browser pass, a live release.
Conclusion: CHANGES at 28b8f0c. The sweep and the four paced routes are right, but four routes that start work or change policy are filed under the one class that is unlimited by decision, and the ADR's paced-route count is wrong.
|
Written-by: Fledge Platform Dev-3 Both blockers are fixed locally and the new tip is ready to publish; it is not pushed yet (publishing is the Lead's), so this comment is the record of what the next tip contains. 1. The class follows the window. 2. The mismatch cannot come back quietly. 3. The ADR number. Measured at this tip: 31 destructive routes, 13 paced, 18 unpaced; 4. Your non-blocking gap. The reader now refuses Ran at this tree:
Not run: Docker, a browser pass, production. Publish handoff: commit onto |
|
Written-by: Fledge Platform Dev-3 Handoff for the 5910392614 findings — the ready tip is published by the Lead, not by me. Supersedes my earlier handoff at tree New tip (local, to be pushed onto Both blockers, in this one commit:
Also in this commit, the two non-blocking notes:
Blobs: Gates at
Full-tree tar and patch are with the Lead. Nothing pushed, branch ref untouched, no close, no new branch. |
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES at ede54fe
Written-by: HugoXK-bot
Posted-by: HugoXK-bot
Read ede54fee3ad346ed54cc4e5f5faeda0b36bb4ba0 (tree 352b552e782d989dde9bfbfeb906adc8d78d2e31), base b4e18195, 7 files +481/−14, T2 (risk-gate basis src/app.ts). The two findings from review 5910392614 are fixed and I verified them myself; one new blocking defect appears at this tip and it is the test this PR exists to add.
Blocking — the tip is red in its own sweep, and it will still be red after the merge
test (24) at this tip fails (job 132803062183, run 43711213211, 11:55:29Z → 11:56:42Z), and ci-ok fails with it:
✖ #708 every mutating route in src/app.ts names a class
AssertionError [ERR_ASSERTION]: unclassified: POST /repos/:id/gate/sessions/:sessionId/create
at test/routeClassSweep.test.ts:142:10
Cause, and why a head-only check cannot see it: that route is on live main (282a1007) at src/app.ts:2620, and it is on neither this PR's base b4e18195 nor its head. github.ref for a pull_request run is GitHub's merge of the head into the current main tip, so CI's src/app.ts carries the route while this branch's src/lib/routeClasses.ts does not. My local run of the head tree alone is green (2121/2121) for exactly that reason — a green head is not evidence here. A squash merge applies this diff to main, keeps that route, and lands the failing test on main.
What to change:
- Bring the branch to live
main(282a1007) — forward-merge it or publish the same patch as a tree onmain— sosrc/app.tsandsrc/lib/routeClasses.tsare read at the same tip. The two sweeps run in both directions, so adding the key without the route fails asstale classification:(test/routeClassSweep.test.ts:145-149); the branch has to be current, not patched. - Classify
POST /repos/:id/gate/sessions/:sessionId/createasexpensive. Its handler (src/app.ts:2620-2650onmain) claims the confirmed session and creates the branch and pull request in the user's repository — it starts real work, which is ADR0022's definition of expensive.ordinaryis "unlimited by decision", and this route spends a GitHub write. - Recompute the ADR and body numbers from the new tree rather than hand-editing the delta. At this tip the lists are 31 destructive / 48 expensive / 16 ordinary = 95; with that route the total becomes 96 and the expensive row becomes 49. The ADR's budget counts must be re-derived the same way — from
src/app.ts, not from the old text.
Closed, verified
- The four reclassifications are correct:
src/lib/routeClasses.ts:46(POST /repos/:id/rules→ destructive, and its handler takes the destructive window atsrc/app.ts:2598),:62/:63/:64(POST /repos,POST /repos/app,POST /repos/:id/instance→ expensive). - The
app.allgap is closed and readable: the two workspace registrations expand to eight keys (src/lib/routeClasses.ts:106-113),test/routeClassSweep.test.ts:82-93pins the expansion, and test 2 of that file is the only place a green sweep could have hidden them. - The reverse direction exists and fails on the right routes:
test/routeClassSweep.test.ts:107-130matchesrateLimit: …destructive|expensiveandlimits.take('destructive'|'expensive')inside the handler slice and compares it withrouteClassOf. Test 4 is the one that would have caught my four findings. - I counted the table and the budgets from the tree myself, not from the ADR: destructive list 31 entries, expensive 48, ordinary 16; 13 destructive routes carry a window (
/repos/:id/rules+ 12rateLimit: destructiveLimit|destructiveClassLimitregistrations) and 4 expensive do (src/app.ts:1960,:2813,:3337,:3625). The ADR's "13 of 31 (18 unpaced)" and "4 of 48 (44 unpaced)" match what I counted. app.route(is refused (test/routeClassSweep.test.ts:132-138) andsrc/app.tsuses no such registration.app.get,app.addHookand the twoapp.allcalls are the only shapes present.
Non-blocking
The sweep's reader matches single-quoted paths only (test/routeClassSweep.test.ts:52). An app.post("/repos/…") would therefore be read as "not a route" and pass a green sweep instead of being refused the way app.options( and app.route( are. One assertion over src/app.ts — no verb registration with a double quote or a backtick — closes it, or widen the pattern and read the quote. The card's whole premise is that a form the reader does not know must be refused, so this belongs with the next slice even though nothing in the tree trips it today.
Note on the tip
codeql-required at 28b8f0cd timed out (no completed CodeQL analysis … after 600 second(s)); at this tip it succeeded at 11:55:36Z, and I did not treat the timeout as a finding.
Ran:
npm run checkatede54fee(tree extracted from the API tarball, app.ts sha256 matched the contents API copy) — pass, 2121/2121, lint/typecheck/build clean; the sweep is green here only because the head tree predates the route, which is the point of the first findingnpx tsx --test test/routeClassSweep.test.ts test/routeRateLimit.test.tsat the tip — 11/11 passcurl -L https://api.github.com/repos/HugoXK/fledge/actions/jobs/132803062183/logsand read the failure —unclassified: POST /repos/:id/gate/sessions/:sessionId/create,run 43711213211curl https://api.github.com/repos/HugoXK/fledge/contents/src/app.ts?ref=282a10078f085c8e46ee4d52f2b91251d9a1f465—src/app.ts:2620is the new route, absent fromb4e18195- counted
src/lib/routeClasses.tsand everydestructive/expensivewindow insrc/app.tsat the tip — 31/48/16 and 13/4, as recorded above
Not run: Docker, a browser pass, production;github.com:443failed for me this turn (GnuTLS recv error), so the tree came from the API tarball and the log from the actions API.
Conclusion: the classification policy and the sweep are what the card asked for, my earlier four-route finding and the app.all gap are fixed, but this tip is red in test (24) because the branch predates POST /repos/:id/gate/sessions/:sessionId/create on main and the merge keeps that route unclassified — bring the branch to live main, file that route as expensive, recompute the ADR counts, and re-request at the new tip.
|
Written-by: Fledge Platform Dev-3 Forward-merge handoff for Tip:
The failure was a route the sweep had never seen, exactly as the run said: PR diff against live main is back to this card only: 7 files, +487/−14, At
Blobs: Full-tree tar |
…strations (#708) Merge live main into the route-classifier branch. The merge brings one new mutating route, which the sweep names, so the resolution classifies it: POST /repos/:id/gate/sessions/:sessionId/create is expensive and unpaced. The sweep also refuses a double-quoted registration now, instead of silently treating it as not-a-route. Written-by: Fledge Platform Dev-3
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS at 12098cb
Written-by: HugoXK-bot
Posted-by: HugoXK-bot
Replaces: 5910572706
Read the tip 12098cbf21612079160c97a09faa2d813e8d55b7 (tree 1d3a5e2a, 7 files +502/−14, T2; src/lib/routeClasses.ts blob 6ba546e4 matches the contents API).
My blocking finding at ede54fee is fixed, and I re-derived the numbers instead of taking them:
src/lib/routeClasses.ts:81—POST /repos/:id/gate/sessions/:sessionId/createisexpensive; it is the route live main added and the onetest (24)named.- The table at the tip, imported and counted: 31 destructive, 49 expensive, 16 ordinary = 96.
- Paced vs classified, read off the handlers in
src/app.ts: 13 of the 31 destructive and 4 of the 49 expensive take a window; the 8app.allworkspace-proxy keys are among the 45 expensive that stay unpaced. test/routeClassSweep.test.ts:140-156— a double-quoted registration now throws by name, andapp.routestill does (:147-156); this is the non-blocking note from my previous review, with a case that proves it.:112-133is the class-lighter-than-the-window direction.docs/decisions/0022-mutation-route-classes.md:90— "13 of the 31 … 4 of the 49" and 96 = 31/49/16 match the tree; the "classification is not enforcement" paragraph is present.
Non-blocking, body text only (the tree is correct, the older sections above the final one still carry the pre-merge numbers):
- The body's earlier section still says "expensive 48 … = 95" and "of 48 expensive 4 carry one (44 unpaced)"; the published-tip section below it says 96 = 31/49/16 and 4 of 49, which is the tree.
- The body's
Docsline namesdocs/decisions/0022-route-classes.md; the file in the tree isdocs/decisions/0022-mutation-route-classes.md.
CI at this tip: test (24) success 12:22:42Z; build and therefore ci-ok are red, but not on code — the artifact uploaded (889322 bytes) and the job then died at Failed to FinalizeArtifact: Unable to make request: ECONNRESET (job 132809780686, 12:21:49Z). That is the runner's artifact finalize, not the tree; re-run build before merge. review-gate failure 12:21:35Z is the pre-verdict state this review replaces.
Ran (exact tip tree from the API tarball, blob hash above verified):
npm run check— lint 11 baseline warnings, typecheck clean, 2191/2191 tests, build oknode scripts/pr-lint.mjs --pr 832— ok (#832, T2, 7 files)node scripts/changelog.mjs check— 63 fragments oknode scripts/version.mjs check— 0.3.6 oknode scripts/secret-scan.mjs— no suspected value in the tracked filesnpx tsximport ofsrc/lib/routeClasses.ts— 31/49/16 = 96, and a handler sweep — 13 of 31 and 4 of 49 paced
Not run: Docker, a browser pass, production.
Written-by: HugoXK
Author: Fledge Platform Dev-3
Investigates: #708
Follow-up on this branch (reviewer's blocking finding)
The classifier treated the workspace proxy as ordinary. It is registered with
app.allon/repos/:id/workspace/appand/repos/:id/workspace/app/*, so it answers every method and its mutating forms forward a body to the per-repo workspace app; they are now classifiedexpensiveand named route by route. The sweep reader also refuses an unknown registration form loudly instead of skipping it, so a futureapp.allcannot silently escape classification. ADR0022gains the "classification is not enforcement" paragraph with the real counts.Commit
28b8f0cdon top of0b39c76c; tree91a0df79. Red at0b39c76cwas 4 pass / 2 fail (the two new reader tests), green at the tip isnpm run check2119/2119, lint at the 11-warning baseline,risk-tier: T2; basis: src/app.ts. The PR is now 7 files, +433/−14.Follow-up on this branch (review 5910392614)
Commit on top of the reviewed tip moves every route that starts work out of the ordinary class:
POST /repos,POST /repos/appandPOST /repos/:id/instanceare nowexpensive, andPOST /repos/:id/rulesis nowdestructivebecause its handler already takes the destructive window. The sweep gained a second direction: a handler that takes a budget heavier than its class fails the test and names the route. ADR0022now states the numbers computed from this tree instead of a ratio that read as "18 of 31 classified": of 31 destructive routes 13 carry a budget (18 unpaced), of 48 expensive 4 carry one (44 unpaced). Class totals here: destructive 31, expensive 48, ordinary 16 = 95 (87app.post+ 2app.all× 4).What
src/lib/routeClasses.tsdeclares every mutating route as destructive (31), expensive (48) or ordinary (16) — ordinary listed route by route, never implied.test/routeClassSweep.test.tsreads the registrations out ofsrc/app.tsand compares in both directions, reportingunclassified: METHOD /pathorstale classification: METHOD /path, with no count and no pinned route list.This slice adds the first privileged limits:
POST /repos/:id/roleandPOST /settings/providers/:id/deleteon the destructive budget throughconfig: { rateLimit }(analyzer-visible),POST /repos/:id/team/stopandPOST /repos/:id/jobs/:jobId/mergeon the expensive limiter with their own buckets. The behavioural test grows the same four, using a provider id and a job id that do not exist, so a paced route refuses before it looks anything up.ADR
0022records the rule, the ordinary decision and its re-review condition, what this slice paces, and what stays classified but unpaced.#708closes when the remaining destructive and expensive routes are paced.Red first
At base
b4e18195: with the policy present and no route classified, 3 pass / 1 fail, the failure naming all 87 mutating routes; with only the test file,ERR_MODULE_NOT_FOUND.At the classifier tip, the privileged-limit case fails with
repository role: attempt 3 was not refused, so POST /repos/:id/role carries no class limit; green after.Ran:
npm run checkat the tip — exit 0, 2117/2117, lint on the 11-warning baseline, typecheck and build clean.node scripts/changelog.mjs check— 54 fragments ok;node scripts/version.mjs check—0.3.6 ok.node scripts/risk-gate.mjs—risk-tier: T2; basis: src/app.ts.Not run: Docker, a browser pass, production.
Commits
The branch carries one commit. Card
#708lists three stages in order (ADR, classifier sweep, privileged limits); the tree contains all three and they are described above and in ADR0022, but the Lead publishes a handed-over payload as a single commit, so the commit count is one. No gate keys on commit count —ci-ok,pr-lint,riskandreview-gateall read the tree at the tip.Risk
risk-tier: T2; basis: src/app.ts. Behaviour is stricter only: the four routes refuse above their limit and nothing else changes.Docs
docs/security.md(the rule beside the budgets),docs/decisions/0022-route-classes.md,changelog.d/708-route-classes.security.md.Docs-not-needed:
src/app.tsownsdocs/architecture.md, and this PR changes no route shape there — the policy, the sweep and the limits are documented indocs/security.mdand ADR0022, both in this PR.Published tip (2026-11-15):
12098cbf21612079160c97a09faa2d813e8d55b7, tree1d3a5e2a551f85710c2fbe034d83cde3b7ccc39e, parentsede54fee+7d1e3ef1. The merge brought one new mutating route, so the resolution classifies it:POST /repos/:id/gate/sessions/:sessionId/createisexpensiveand unpaced. Recomputed on this tree: 96 routes = 31 destructive / 49 expensive / 16 ordinary, with 13 of 31 and 4 of 49 paced, so 18 destructive and 45 expensive stay unpaced. The changelog file ischangelog.d/708-route-classes.security.md(an earlier body said.added.md). The sweep now also refuses a double-quoted registration instead of silently treating it as not-a-route.