HOMELAB-2527: fix(forgejo): assert comment identity before posting #75
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-2527-comment-identity"
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?
Closes HOMELAB-2527 (split from HOMELAB-2510 / GITS-312). Authorship of a Forgejo comment is landing-lock evidence (HOMELAB-2346); a write must be attributed to the identity it claims BEFORE it happens.
The two measured defects
forgejo_comment_on_pr(forgejo/index.ts → api.tscommentOnPR) posted with whatever token was inFORGEJO_API_KEY. It never issuedGET /user, and a caller had no way to state which identity the write must come from.verdict-cli.ts --postreadGET /useronly as a default author (args.author ?? await loginOf(...)), so an explicit--author ld-reviewershort-circuited the only identity read: the body was composed and lock-evaluated asld-reviewerwhile the token could beaaron. The post-write read-back compared the stored body, never the stored author. The 403 regression test makes this concrete — with--authorsupplied, noGET /userwas issued at all and the POST still happened.The guard
forgejo/api.ts:assertWriteIdentity(cfg, { expected, body })readsGET /userwith the same token and compares it against an explicitly configured expected identity — the tool'sexpected_identityparameter,FORGEJO_EXPECTED_IDENTITYin the env, or the helper's--author. No universal hardcoded reviewer:aaron, workers andld-reviewerall legitimately post comments, so the expectation is configuration, not a constant.GET /userfails, or answers with no login); and a landing-lock verdict body with no declared identity at all (a verdict whose authorship is undeclared is a record nobody can attribute).postVerifiedComment()requires a branded proof minted by the assertion, so a hand-rolled{ ok: true }cannot reach the POST. It then reads the comment back and verifies the body and the author (sameLogincompares case-insensitively, because Forgejo echoes stored case).verdict-cli.ts:GET /useron every run; a mismatched--authorrefuses (dry run too);--postrequires a readable identity; after the POST the stored author is verified against the token's identity and a mismatch fails loudly with "do not repost under another identity without a ruling".No historical comment is edited or deleted anywhere in this change — the guard is pre-POST only, and no code path here issues PATCH/DELETE (asserted in the tests: every refusal proves zero mutating requests, not just zero comment POSTs).
Red first
Verdict-body rule added after the first guard, red first as well (
1 failed), then implemented.Green
Baseline on the same commit with the new test file present, before implementation:
11 failed | 251 passed (262)— every pre-existing test was already green, so the red is this guard only.The suite covers: mismatch/unreadable/verdict-without-declaration → zero writes and a refusal naming both logins; matching identity → exactly one POST,
GET /userstrictly before it, and read-back authorship; ordinary comment under a sanctioned non-reviewer identity → still posts and records its real author.Scope
forgejo/api.ts,forgejo/index.ts,forgejo/verdict-cli.ts,forgejo/tests/comment-identity.test.ts.forgejo/landing.ts, any landing test, CI required-context code, merge behaviour (no landing-lock logic is changed or re-evaluated here).docs/forgejo.mdis omitted: open PR #74 (HOMELAB-2521) holds that file. The new env var and parameter are documented in the tool description and the test-file header; a doc line can follow after #74 lands.plane/is untouched: the same class of defect is plausible there (plane_commentposts under whateverPLANE_API_KEYis set), but it is not demonstrated on this ticket, so it does not belong in this PR. Flagging to the orchestrator for an owner ticket rather than widening scope.A Forgejo comment's AUTHOR is landing-lock evidence (HOMELAB-2346), but both comment write paths posted with whatever token was in FORGEJO_API_KEY and neither asked the API whose token that was. The harness once posted a verdict body as `aaron` and honestly reposted it as `ld-reviewer` (GITS-312). Two concrete defects, both measured red-first through fake-API tests: - `forgejo_comment_on_pr` never issued GET /user and had no way for a caller to state the identity the write must be attributed to. - `verdict-cli.ts --post` read GET /user only as a DEFAULT author (`args.author ?? await loginOf(...)`), so an explicit `--author ld-reviewer` short-circuited the only identity read; the post-write read-back compared the stored body and never the stored author. The 403 regression case shows it plainly: with `--author` set, no GET /user was issued at all. The guard (forgejo/api.ts): `assertWriteIdentity` reads GET /user with the same token and compares it against an EXPLICITLY configured expected identity — `expected_identity` on the tool, `FORGEJO_EXPECTED_IDENTITY` in the env, or the helper's `--author`. There is no universal hardcoded reviewer: `aaron`, workers and `ld-reviewer` all legitimately post comments, so the expectation is configuration. What is refused before any write: - an identity that differs from the declared expectation; - an identity that cannot be read (GET /user fails or answers without a login); - a landing-lock VERDICT body with no declared identity at all. `postVerifiedComment` requires a branded proof minted by the assertion (a hand-rolled `{ ok: true }` cannot reach the POST), then reads the comment back and verifies both the body and the author. The verdict helper does the same: it reads GET /user on every run, refuses a mismatched `--author` in dry run too, requires a readable identity for `--post`, and fails loudly if the stored author is not the token's identity. No historical comment is edited or deleted anywhere in this change; the guard is pre-POST only. `forgejo/landing.ts`, the landing tests and every merge behaviour are untouched, and `docs/forgejo.md` is skipped because open PR #74 holds it. Verification: forgejo suite 265/265 (17 new identity tests), tsc clean.Verdict: APPROVED
model: omniroute/deepseek-flash
head:
c682d40359Independent GENERAL review — fresh context, no edits, no merge, no other PR, no formal review submitted. Exact head re-fetched live immediately before this post and compared to the sha in the header above: unchanged. Poster identity proved first (
GET /user= ld-reviewer, id 47) via the sanctioned reviewer token; no credential on argv, none printed.Re-measured rather than read from the worker's logs:
6518158api/index/verdict-cli plus this PR's own test file → tests/comment-identity.test.ts fails 14/17 (3 pass); at this head it passes 17/17. The 11-failure log is the earlier 14-test revision of the same file, and the shapes it names reproduce.tsc --noEmitclean.Security contract, attacked with a fake API and a driver outside the repo:
{ok:true, actual}, with a forged symbol-keyed object, or with undefined throws and writes nothing; only a minted assertion reaches the POST.OBSERVATIONS, hardening and surface items only (none of these can make the lock count a verdict it should not; all reproduced, recorded in my working note):
expected_identitywins over FORGEJO_EXPECTED_IDENTITY, soexpected_identity: ""silently disables a process-wide expectation for non-verdict comments (reproduced: env ld-reviewer, token aaron, ordinary body, empty explicit → posted). Verdict-shaped bodies are still refused. One-line fix: treat "" as unset or refuse it, plus a test..catch(() => null)) and the author then falls back to the POST echo while the body comparison is skipped (reproduced with a 404 on the read-back: success reported, row never read). The identity is then asserted, not proven — consider failing loudly.commentOnPRis still exported and unguarded; postVerifiedComment is its only in-repo caller, so this is compatibility-preserving, but a deprecation note would close a future bypass.forgejo_comment_on_prdetails shape changed from the comment row to a flat refusal/success record; nothing in the repo consumes it. docs/forgejo.md does not mention expected_identity (deliberately deferred behind PR #74) and the operational note that a token without read:user can no longer comment at all is worth recording.Scope: exactly the four expected files (+796/−9); landing.ts, verdict.ts, merge behaviour, docs and plane/ untouched. Worktree note:
/home/aaron/repos/pi-extensions-2527shipped with an emptyforgejo/node_modules, so I re-installed dependencies in a throwaway archive of this head under /tmp and measured there; nothing in the repo was modified.I am approving the diff: the two measured defects are closed on both write paths at this exact head, and the items above are hardening and surface polish. forgejo is dangerous class, so this is the general lens only: a premium second opinion on a different declared model is required before this lands.
Verdict: APPROVED
model: omniroute/kimi-pool
head:
c682d40359Premium (kimi-pool) exact-head review of
c682d40359. All workclaims reproduce independently: targeted 17/17, full forgejo suite 265/265, tsc clean, CI #165
14/14 green at this head. Scope is exactly the four declared files; landing.ts is untouched and
the new api.ts -> landing.js import is acyclic.
The security contract holds on both write surfaces. The expected identity is configuration
(explicit argument, FORGEJO_EXPECTED_IDENTITY, or --author) with no universal hardcoded
reviewer. GET /user is read unconditionally before any write in both the tool and the verdict
helper; the old --author short-circuit is regression-pinned closed. Mismatch, unreadable
identity, and a verdict body with no declared identity all refuse before any mutation, and every
refusal test proves zero POST/PUT/PATCH/DELETE requests, not merely zero comment POSTs. The
branded proof is a module-private symbol checked at the POST boundary, so a hand-rolled
{ ok: true } cannot reach the write. On success there is exactly one POST followed by a read-back
that verifies the stored body verbatim and the stored author case-insensitively against the
asserted identity; a post-write authorship mismatch fails loudly with an explicit do-not-repost
instruction. No route can post under aaron while declaring ld-reviewer, no branded-proof forgery
path exists, and existing callers keep working: ordinary comments under any readable sanctioned
identity still post and now report the real author.
Deliberate behaviour changes, both consistent with the ticket contract: the tool details shape
is now a flat record carrying the read-back author, and a token whose identity is unreadable
(no read:user scope) can no longer post any comment. docs/forgejo.md documentation is
disclosed-deferred behind PR #74, and the worker's plane/ anomaly flag is a correct scope call
for an owner ticket.
No findings, no blockers. Approved for landing at this exact head.