HOMELAB-1679: fix(forgejo): lock hardening — gated knobs off the server's repo name, the server-name rule, fail-closed evidence reads (LR-20261111-017, Amendments 15/16/18) #131
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "lr-20261111-017-lockharden"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Spec 017, consolidated per AMENDMENT 18: three stacked commits, each its own fix with its own old-code-failing tests. Supersedes #130 (N6) — closed with a reference; #125's r5 merges this branch. Gate-changing: two pane seats (via
forgejo_comment_asas ld-reviewer) + homelab-93's security review + SEATSID before El Director's go. Base51daf88→ headc4e93bbf20.Commit 1 —
7e52108: gated knobs pinned, keyed off the SERVER's repo name (Amendment 15, homelab-93 (1)–(2))On the two repos named in code (
GO_GATED_REPOS), the trusted reviewer set ANDFORGEJO_DANGEROUS_PATHS/FORGEJO_LEGACY_PANEL_OWNERS/FORGEJO_PREMIUM_MODELSstay at defaults — a value a session can set is not a gate (B3 class). The gated decision keys offpr.base.repo.full_name(threaded from getPR / the fetched PR; unreadable ⇒ pinned), never the requested string. Each ignored knob is logged once per call, by name only.Old-code-failing tests:
…DANGEROUS_PATHS cannot make a deploy/** change routine,…LEGACY_PANEL_OWNERS cannot switch a gated repo to the legacy panel,…PREMIUM_MODELS is ignored,says each ignored knob once … never echoes a value,pins an unreadable server name …,a gated repo the SERVER names cannot be widened by the session env.Commit 2 —
1ddb150: the lock judges the repo the FORGE reports (review-88-pr130 B1/n1/n3/n5/n6, homelab-93 (1))Every gate choice keys off
pr.base.repo.full_name, case-normalized; a requested name that differs (one decode + case-fold) is REFUSED, naming both; an absent name refuses merges and the census.%is rejected inparseRepoRefand--repo. homelab-88's reproductions (Homelab/pr%6Fd,%48omelab/prod,Homelab/infra%2Dcore) each refuse or stay pinned.Old-code-failing tests:
refuses "Homelab/pr%6Fd","%48omelab/prod","Homelab/infra%2Dcore",…different repos, naming both,…reports no repo name at all,refuses mid-census too,refuses --repo with a percent-escape,says the line ONCE across a posting run.Commit 3 —
c4e93bb: incomplete evidence reads refuse on gated repos (Amendment 16, review B8, homelab-93 (3))A live hole in today's tool:
listAllPRCommentssilently stops at 300 comments, reviews are read 50-and-never-paged, and inline read errors are swallowed — a hold beyond the cut never reaches the ledger and the merge goes through. On gated repos: the comment cap REFUSES ("refuse, never truncate"), reviews page to the end under a hard refusing bound, inline 5xx refuses, the changed-file read bound refuses. Non-gated repos unchanged. Every pagination path fails closed — a cap only ever comes with a refusal.Old-code-failing tests:
hold as comment 301 REFUSES,51st review SEES the hold,inline 502 REFUSES,inline bound,changed-file bound.Notes for the r5 merge (homelab-93 (4))
One owner for
GO_GATED_REPOS/isGoGatedRepo;repoKeydecodes (superset of #125's);resolveLandingPolicy's second param widens tostring | ServerRepoView(string still compiles);listAllPRComments/listPRFilestake an options object ({ maxPages }) — #125's call sites adapt.Proof
56 files / 1268 pass (1 known pre-existing merge-identity ambient-token failure, identical on base);
tsc --noEmitclean; every fix's red→green shown by stash. no independent walk: implementer-run checks only.Lab-Run: LR-20261111-017
r2 round (stacked on
c4e93bb): homelab-93's B1/B2/F3/F6, verified against Forgejo v14.0.3 sourceX-Total-Count(DB rows, hidden included), read every page toceil(total/50); missing/unparseable header refuses on gated repos; an ignored-paging server is detected (page N repeats page 1's first id) and falls back to one validated read. Red→green: hidden-PENDING row 51 refuses (9 red → 19 green across B1/B2).length === X-Total-Count⇒ complete in ONE request; short of the count ⇒ count-driven paging with the repeat-id detector; the 300 cap now fires only when paging genuinely works. Inline: one GET per review validated againstcomments_count.page, DB page cut before the PENDING post-filter, count includes hidden rows); the B8 tests are re-expressed against that real behaviour.tests/landing-census-gated-policy-017.test.tskills the census mutation m2 (policy = undefined/gated: false).Pre-existing items homelab-93 listed, NOT fixed here (named for the record)
F4: combined-status 30-context cap and single-page PR-commit/verdict-cli reads; F5:
FORGEJO_BASE_URL(the largest remaining lever — every read and the merge POST go there) andFORGEJO_LANDING_IDENTITY, still env-settable on gated repos. These predate this PR; per the lead, homelab-88 runs a read-only live check (doesX-Total-Countarrive? do comment pages ignorepage?) BEFORE this lands.HEAD now
7813f704e2; base51daf8842e; patch: piext-131-diff.patch (3030 lines) in the epic folder.LD-VERDICT APPROVED head=c4e93bbf2008d29aa77e24715fab5b916bb29a5a session=LR-20261111-017-seatB-20261111T1722Z
Reviewed a read-only clone of the branch at the pinned head (git rev-parse on origin/lr-20261111-017-lockharden =
c4e93bbf20, base51daf88): the full base..head diff, all three commits, 14 files, read end to end. Full suite 1268/1269; the one failure is tests/merge-identity.test.ts (ambient landing token) and is identical on the untouched base — pre-existing, not a hold.tsc --noEmitclean.(1) homelab-93 (1) — the gate keys off the SERVER's name. All four landing call sites pass a
ServerRepoViewdecided frompr.base.repo.full_name: index.ts:361 (merge/census evidence,requireReported: true→ refuses an absent or disagreeing name), index.ts:533 (policy), index.ts:1136-1142 (census, per row, refusing rows reported not-landable with the reason), verdict-cli.ts:483 (helper, absent name PINNED gated rather than read as ungated, +990-996 fallback gated:true). I grepped every gated decision: none is fed by the requested string — the requested ref appears only in the refusal comparison (landing.ts:3096-3106) and in display (details.repoRef).%refused in parseRepoRef (index.ts:274-280) and--repo(verdict-cli.ts:157-162), which also refuses anything but a single slash.repoKeydecodes once, case-folds, last two segments — soHomelab/pr%6Fd,%48omelab/prod,Homelab/infra%2Dcorecannot address a gated repo under another name.(2) homelab-93 (2) — the four knobs cannot loosen a gated repo:
knob()(landing.ts:3195-3196) returns the default for dangerousPaths / premiumModels / legacyPanelOwners / reviewerIdentities whenever the view is gated, so a widened reviewer set, an emptied dangerous-path list, an added legacy owner or a promoted model is inert there; each ignored variable is logged once per call, by name and repo key only, never by value (landing.ts:3178-3190; dedupe sinks index.ts:1074-1078, verdict-cli.ts:897-905). Only those four env reads exist in landing.ts, so there is no fifth lever.(3) homelab-93 (3) — every pagination cap in the diff now comes WITH a refusal on a gated repo: comments (api.ts:2161-2168), reviews paged to the end under a refusing bound (api.ts:2187-2202), per-review inline comments (api.ts:2230-2247, inline read enumerated through the reviews already read to their end, and a 5xx is no longer swallowed — index.ts:465-476), changed files (api.ts:2536-2551). No silent truncation is introduced anywhere in the diff.
Red→green, measured not asserted: with the four source files (
api.ts,index.ts,landing.ts,verdict-cli.ts) stashed back to base and the new fixtures kept, 27 of the 38 tests in the four new files go red — the comment-301 hold, the 51st-review hold, the inline 502, the inline bound, the changed-file bound, all three percent-encodings, both requested-vs-reported disagreements, the absent-name refusal (merge, census and helper), and both knob pins. The other 11 stay green on base and green at head: the non-gated halves (silent comment cap of 6 pages, single 50-row review page, swallowed inline error, unenforced inline bound, silent file cap, honoured env). So non-gated behaviour is pinned unchanged, not merely left untested. The named tests exist and pin their fixes.No holds.
Two non-blocking findings, recorded so they are not lost:
paged()maxPages=10) and verdict-cli.ts:596-600 (inline read errors swallowed) keep the B8 shape on the helper's own preview path — it caps silently at 500 and swallows the inline failure. It cannot produce a wrongful merge (the merge gate refuses at 300 comments on a gated repo, so any PR where the helper's read was short is unmergeable), but the--resolve-reviewfinding check reads a possibly truncated record. Worth a follow-up, not this PR's scope.split()returns null for an empty value, so it falls back to the default (fail-safe, and pre-existing behaviour). Comment wording only.model: omniroute/deepseek-flash
session: LR-20261111-017-seatB-20261111T1722Z
LD-VERDICT APPROVED head=c4e93bbf2008d29aa77e24715fab5b916bb29a5a session=LR-20261111-017-seatA-20261111T1722Z
Independent clone, branch lr-20261111-017-lockharden, current head equals the pin; judged the full base 51daf88..c4e93bb diff (three commits) and the checks only.
What I checked
goGatedFromServerName(landing.ts:3055) andserverRepoView(landing.ts:3081) are the only producers of a gated answer, and the only production consumers are the merge gate (index.ts:361 withrequireReported: true, applied at index.ts:533), the census (index.ts:1136, resolved per row) and the verdict helper (verdict-cli.ts:483 → 646). Grepped everyresolveLandingPolicy(call site: no gate is fed a requested string. A requested-vs-reported disagreement refuses naming both, an absent name refuses on merge and census, and%is refused inparseRepoRef(index.ts:275) and in--repo(verdict-cli.ts:157).??not||), and each set-and-ignored knob is logged once, by name, never by value.7e52108→ 12 red (the three percent-encodings, absent name, different-repo naming both, mid-census,--repo%, the once-only line); commit-3 tests on parent1ddb150→ exactly the 5 gated B8 cases red (comment-301, 51st review, inline 502, inline bound, changed-file bound) with the 6 non-gated pins green.base.repo.full_nameand page-aware routes; no assertion is weakened except two verdict-694-replay expectations, which now assert the pin working and additionally assert the era policy through an explicitly built policy — noted, not a hold.tsc --noEmitclean.Holds: none.
Observations (non-blocking): (a) a legacy-era replay on Homelab/infra-core now refuses under the pinned panel — intended by the amendment and asserted; (b)
requireReported: truemakes a PR response withoutbase.repo.full_namea merge refusal — fail-closed by design, a live-shape dependency worth knowing; (c) the head-date fallback still readslistPRCommitsas one 50-row page (api.ts:2364) — pre-existing, untouched here, still fails closed to null.model: omniroute/deepseek-flash
session: LR-20261111-017-seatA-20261111T1722Z
homelab-93's mutation m2 set the census policy (index.ts ~:1138) to undefined and the whole suite still passed (1269 tests): nothing exercised the census on a GO-gated repo with a WIDENED environment. The census would then report a gated PR as landable under a session-set reviewer set — the merge still refuses, but the queue the operator reads would say "landable", which is the same class of defect as the gate being movable. The new test censuses a gated PR whose only exact-head approval comes from a reviewer identity added through FORGEJO_LANDING_REVIEWERS, and asserts BOTH that the row is not landable ("no exact-head verdict") and that the resolution really happened for that row ("FORGEJO_LANDING_REVIEWERS is set and ignored on GO-gated repo homelab/infra-core" on stderr) — so an undefined policy is caught, not only an honoured env. The control runs the same fixture on a non-gated repo, which is landable with that verdict, so the pin cannot pass by refusing everywhere. Verified against both mutations of that line: `policy = undefined` fails both tests, and `gated: false` fails the gated one. Lab-Run: LR-20261111-017LD-VERDICT APPROVED head=7813f704e2756ae11f947b1d61f773b97256dd91 session=LR-20261111-017-seatB-20261111T1810Z
Seat B, run LR-20261111-017. The PR head is exactly
7813f704e2, so the pin holds; I judged the full diff 51daf88..7813f70, with the judgment concentrated on the r2 delta c4e93bb..7813f70 (api.ts, tests/fake.ts, the B8 evidence tests, the F6 census test).What I checked (r2):
X-Total-Countfrom page 1 and reads every page toceil(total/limit), so a page shortened by a PENDING row filtered AFTER the DB cut no longer ends the read;api.tsrefuses when the header is absent (andNumber.isFiniterefuses a non-numeric one read throughparseInt). The fake cuts the DB page first and then hides PENDING rows, and its count includes the hidden rows — i.e. the fixture reproduces the measured fail-open rather than a friendlier server. The hold at review row 51 is reached and the refusal is the hold leg, not a cap.length === X-Total-Count→ ONE request (no six duplicated pages, no false 300-cap on 60 or exactly 300 comments); short of the count → count-driven paging bounded by maxPages BEFORE the loop; a page repeating page 1's first id is detected and refuses instead of looping or silently accepting a short read; the finalall.length !== totalcheck closes the comment path. Inline reads once per review and validates againstcomments_count(5000 inline read in one call, over-count refuses naming the review), and a paging-ignoring endpoint is detected rather than causing a 100-request loop.page, DB cut before the PENDING post-filter);commentsPaging/reviewsPagingare opt-in exceptions used only to pin the defensive paths.landing-census-gated-policy-017.test.tsasserts BOTH halves (pinned reviewer set on the gated repo with the "set and ignored" line, and the non-gated control still landable).pr.base.repo.full_name(serverRepoView,requireReportedon merge + census, pinned not ungated for the verdict helper), one URL-decode + case-fold with a%refusal, and the B8 fail-closed evidence reads.Red→green spot-checks (scratch copies, no tracked file touched):
listAllPRReviews→a page shortened by a hidden PENDING review (B1)goes red (merge goes through with the hold standing).policy = undefined→ both F6 tests red.listAllPRComments→ 3 B2 tests red (false 300 + hold-shaped one-request assertions).Suite on the exact head (deps symlinked as instructed): 57 files, 1278 passed, 1 failed — only
tests/merge-identity.test.ts(ambient landing token), and I confirmed the same test fails identically on the untouched base51daf88in a worktree: pre-existing, not a hold.Holds: none.
Observation, not a hold (n1):
forgejo/tests/fake.tscan omitX-Total-Count(reviewsTotalCount/commentsTotalCount: false) but cannot send a non-numeric one, so the unparseable-header refusal (NaN →Number.isFinitefalse → refuse) is correct code with no test behind it; atotalCountGarbageoption would pin it cheaply.Exercised only the diff and the checks: no other seat's verdict, no PR comments or reviews read.
model: omniroute/deepseek-flash
session: LR-20261111-017-seatB-20261111T1810Z
LD-VERDICT APPROVED head=7813f704e2756ae11f947b1d61f773b97256dd91 session=LR-20261111-017-seatA-20261111T1810Z
Head verified: the PR's current head IS
7813f704e2(base51daf88), so the pin holds and there is nothing to refuse. Judged the full 51daf88..7813f70 diff, with the r2 delta c4e93bb..7813f70 (api.ts, tests/fake.ts, the two 017 test files) read closely.WHAT I CHECKED (r2)
listAllPRReviewsreads page 1, refuses whenX-Total-Countis absent (api.ts:2297) or unparseable (the same guard:parseIntof a non-numeric header yields NaN, whichNumber.isFiniterejects), then reads every DB page up toceil(total/50)(api.ts:2306-2317). The fixture is the live shape (one other reader's PENDING row shortening page 1 to 49 visible of 51, hold in DB row 51): page 2 IS requested and the hold refuses.SPOT-CHECK m1: in a scratch copy I restored the old short-page break in that path — the B1 test goes RED in the fail-open direction (
refusalnull: the merge goes through with the hold unread). Red→green confirmed.length === X-Total-Countis accepted as complete after ONE GET, a short read is paged by the count, and the 300 cap now fires only when paging genuinely works (thecommentsPagingopt-in server). Unpaged endpoint ⇒ one request, no duplidated pages, no false cap: 60 comments and exactly 300 comments both land in 1 read; 301 against a paging server still refuses naming the cap. Inline: one GET per review validated againstcomments_count, with the repeat-id detector ending the attempt instead of walking 100 pages; a review reporting 5000 but serving 60 refuses after 2 reads, 5000 served 5000 accepts after 1.pageis sent, DB page cut BEFORE the PENDING post-filter,X-Total-Countcounting hidden rows. Both switches default to the honest server, the exceptions are opt-in.resolveLandingPolicy(process.env, undefined, policyLog)at index.ts:1136) — the gated row comes backcanMerge: trueandlanding-census-gated-policy-017goes RED; its non-gated control stays green.goGatedFromServerName/serverRepoViewseam and%refusal (r1), the fail-closed wrappers on every gated evidence read, docs/forgejo.md consistency.CHECKS
Suite at the pinned head: 1278 passed, 1 failed —
tests/merge-identity.test.ts(landing token assertion). That failure reproduces identically at base51daf88in this environment (ambient landing token), so it is pre-existing, not a hold.tsc --noEmitclean.HOLDS: none.
Non-blocking notes, not holds:
all.length === totalcheck. That asymmetry is deliberate and sound: the only unreadable rows are other readers' PENDING reviews, and reading every DB page up toceil(total/50)already covers all readable rows.X-Total-Count; the code refuses it viaNumber.isFinite, so it is a test gap, not a defect.model: omniroute/deepseek-flash
session: LR-20261111-017-seatA-20261111T1810Z