HOMELAB-2682: feat(forgejo): fleet reviewer posting tool — token by name, never argv, refuses Aaron #104

Merged

pi-landing merged 1 commit from plane/HOMELAB-2682-comment-as into main 2026-11-06 01:29:59 +00:00

Conversation 2 Commits 1 Files changed 4 +1351

pi-landing commented 2026-11-06 01:13:25 +00:00

Collaborator

Copy link

Why: tonight's leak

El Director's order (quoted as given):

"review seats have no route to post as the reviewer. The comment tool refuses any identity but the ambient key owner, which on mgmt is Aaron, so seats fall back to curl by hand and tokens reach argv: that caused tonight's leak. Fix for the whole fleet: a posting tool that takes a declared identity and reads its token from a mounted file or the environment by name, never argv, never echoed, with output scrubbed of token values, refusing to post as Aaron from agents. Confirm by a failing test first… GITS has a local helper to look at."

forgejo_comment_on_pr (HOMELAB-2527) can only assert that the process's ambient credential is some login. It cannot become another one. On the mgmt VM FORGEJO_API_KEY belongs to aaron, so a review seat that must post as ld-reviewer had no route through the tools: it wrote curl by hand, and a token on a command line is readable in ps by every user on the box (and lands in shell history and in any transcript that records the call).

What this adds

forgejo_comment_as — a new tool in the forgejo/ subtree (named forgejo_* like every sibling; the order said "forgego subtree", the directory is forgejo/). Parameters: repo_ref, pr_number, body, identity, optional expected_identity. No token parameter, and no credential-shaped field anywhere in the schema.

Credential resolution, by name, in order (forgejo/comment-as.ts):

#	Source	Shape
1	mounted file	${FORGEJO_IDENTITY_SECRET_DIR}/<identity>/token, default /run/secrets/ld/identities/<identity>/token — the same mount the GITS seat helper uses
2	environment	FORGEJO_TOKEN_<IDENTITY_IN_UPPER_SNAKE> (ld-reviewer → FORGEJO_TOKEN_LD_REVIEWER)
3	OpenBao KV	kv/ld/forgejo-<name> (ld-reviewer → kv/ld/forgejo-reviewer, override FORGEJO_IDENTITY_OPENBAO_PATH), read only when a Bao token is already ambient (VAULT_TOKEN/BAO_TOKEN) — nothing is minted, and the token reaches the child by environment, never argv

Rules, in the order the tool applies them:

1. a plain identity name — a token pasted into the identity slot is refused as token-shaped (checked before the name shape, because a credential is also made of name-safe characters), and ../ cannot derive a path;
2. the operator login (aaron, any case, surrounding space) is refused outright, before the credential is resolved — not even a GET /user happens;
3. a non-blank expected_identity that contradicts identity refuses before any request;
4. the ambient FORGEJO_API_KEY is never a source, and a resolved credential that is that key is refused even when staged under a seat's name;
5. a comment body containing the resolved token is refused (a PR comment is readable by everyone who can read the repository);
6. GET /user with that credential must answer the declared login — unreadable or mismatched = zero writes (reuses assertWriteIdentity + postVerifiedComment, HOMELAB-2527/2550);
7. one POST, then the stored row is read back: body and author;
8. every returned string is scrubbed (redactDeep), by value and by shape, including an API error body that echoes the credential.

Reused, not invented

• verdict-cli.ts: --token-file discipline (no --token), and redact()'s by-value masking;
• api.ts: source-name-only credential reporting (origin is a path/variable name, never a value), scrubSecretShapes, assertWriteIdentity / postVerifiedComment / UnverifiedCommentError;
• the sanctioned manual pattern (OpenBao → 0600 in 0700, GET /user == declared identity, one POST, read-back, revoke) that seats use today;
• the GITS helper below, whose mount path and 0600-header discipline are kept.

Failing test first

forgejo/tests/comment-identity-as.test.ts was written before the tool existed. On that tree, 21 of 21 tests failed (the tool was undefined):

 FAIL  tests/comment-identity-as.test.ts [ tests/comment-identity-as.test.ts ]
TypeError: Cannot read properties of undefined (reading 'execute')
 ...
 Test Files  1 failed (1)
      Tests  21 failed (21)

The cases, by the order's own letters:

• (a) no token parameter, and none smuggled in — the parameter schema carries no credential-shaped name; an extra token on the invocation is refused with zero writes; a token pasted into the identity slot is refused before any request (zero calls, not just zero writes); a body carrying the resolved token is refused;
• (b) refusing Aaron — aaron, Aaron, AARON, " aaron " all refuse with zero writes and zero requests; the ambient FORGEJO_API_KEY alone never satisfies a declared identity; a seat credential that is the ambient key refuses;
• (c) mismatch — an ld-runtime credential staged where ld-reviewer's belongs refuses with zero writes; an unreadable identity (403) refuses; a read-back attributed to someone else is unverified, not success;
• (d) output scrubbing — an API 500 whose body echoes the token is scrubbed; a stub read-back carrying the token in another field is scrubbed. Every assertion checks all strings in the result (content and details), not just the text part.

Plus the conventions themselves (FORGEJO_TOKEN_* naming, kv/ld/forgejo-* naming, mount path, operator recognition, redactToken by value and by shape) and the OpenBao leg: with an ambient Bao token a fake CLI returns the KV row and the POST carries that credential, and the child's argv is asserted to contain the KV path and not the token; a failing KV read refuses with the credential scrubbed out of the reason.

The GITS helper

Found read-only in the sandbox pod (kubectl exec -n sbx-gits sbx-gits-0 -c sandbox):

/home/ld/work/epics/gits-e1/ref/seat-post-comment.sh — "publica UN comentario de veredicto … SIN que el token toque el argv". It:

• reads the token from the mount ${SEAT_TOKEN_FILE:-/run/secrets/ld/identities/ld-reviewer/token} into a shell variable (assignment, not argv);
• materialises the Authorization header into an ephemeral 0600 file and hands it to curl via --config (so the token is never on a command line);
• packs the body with jq --rawfile (file, not argv);
• umask 077, mktemp -d, trap 'rm -rf' EXIT; prints nothing but the response row;
• hard-codes repo and identity (Homelab/ghostty-in-the-shell, ld-reviewer).

Kept: the mount path convention, the 0600/0700 discipline, "body from a file, header from a file". Added, because the helper is exactly where a seat today posts blind: the GET /user identity assertion, the mandatory read-back (body + author), the operator refusal, name-based resolution across all three sources, and output scrubbing. Nothing in the helper writes or asserts an author, so a mismatched token would post happily — that is the class of defect HOMELAB-2527/HOMELAB-2550 already fixed for the tool path.

Also checked and not a posting helper: /run/sbx-tools/gits-sbx-emu-*.sh (emulator lock/queue/proof), sbx-ci-log.sh, and /home/ld/work/epics/gits-e0r/ref/plane-373-comment.py (Plane, not Forgejo).

Suite counts

Shape	Result
checkout: npx tsc --noEmit	clean
checkout: npx vitest run	30 files / 802 tests passed (28 in the new file)
deployed: npm run test:deployed	30/30 files loaded and passed, 802/802 tests, pinned tree typebox 1.3.34

Rollout note

• Seats switch to this tool. The sandbox seats and mgmt workers stop composing curl calls and stop using seat-post-comment.sh for Forgejo comments: they pass identity and let the tool find the credential (the mount they already have is source #1, so nothing needs re-staging).
• This is part of the sandbox propagation batch — the sandbox image needs /run/secrets/ld/identities/<identity>/token mounted for each seat it runs; that mount already exists for ld-reviewer today.
• Not merged here: two DeepSeek seats follow as gate tooling.

## Why: tonight's leak El Director's order (quoted as given): > "review seats have no route to post as the reviewer. The comment tool refuses any identity but the ambient key owner, which on mgmt is Aaron, so seats fall back to curl by hand and tokens reach argv: that caused tonight's leak. Fix for the whole fleet: a posting tool that takes a declared identity and reads its token from a mounted file or the environment by name, never argv, never echoed, with output scrubbed of token values, refusing to post as Aaron from agents. Confirm by a failing test first… GITS has a local helper to look at." `forgejo_comment_on_pr` (HOMELAB-2527) can only **assert** that the process's ambient credential is some login. It cannot **become** another one. On the mgmt VM `FORGEJO_API_KEY` belongs to `aaron`, so a review seat that must post as `ld-reviewer` had no route through the tools: it wrote `curl` by hand, and a token on a command line is readable in `ps` by every user on the box (and lands in shell history and in any transcript that records the call). ## What this adds `forgejo_comment_as` — a new tool in the `forgejo/` subtree (named `forgejo_*` like every sibling; the order said "forgego subtree", the directory is `forgejo/`). Parameters: `repo_ref`, `pr_number`, `body`, `identity`, optional `expected_identity`. **No token parameter, and no credential-shaped field anywhere in the schema.** Credential resolution, by name, in order (`forgejo/comment-as.ts`): | # | Source | Shape | |---|--------|-------| | 1 | mounted file | `${FORGEJO_IDENTITY_SECRET_DIR}/<identity>/token`, default `/run/secrets/ld/identities/<identity>/token` — the same mount the GITS seat helper uses | | 2 | environment | `FORGEJO_TOKEN_<IDENTITY_IN_UPPER_SNAKE>` (`ld-reviewer` → `FORGEJO_TOKEN_LD_REVIEWER`) | | 3 | OpenBao KV | `kv/ld/forgejo-<name>` (`ld-reviewer` → `kv/ld/forgejo-reviewer`, override `FORGEJO_IDENTITY_OPENBAO_PATH`), read **only** when a Bao token is already ambient (`VAULT_TOKEN`/`BAO_TOKEN`) — nothing is minted, and the token reaches the child by environment, never argv | Rules, in the order the tool applies them: 1. a plain identity **name** — a token pasted into the identity slot is refused as token-shaped (checked before the name shape, because a credential is also made of name-safe characters), and `../` cannot derive a path; 2. the operator login (`aaron`, any case, surrounding space) is **refused outright, before the credential is resolved** — not even a `GET /user` happens; 3. a non-blank `expected_identity` that contradicts `identity` refuses before any request; 4. **the ambient `FORGEJO_API_KEY` is never a source**, and a resolved credential that *is* that key is refused even when staged under a seat's name; 5. a comment body containing the resolved token is refused (a PR comment is readable by everyone who can read the repository); 6. `GET /user` with **that** credential must answer the declared login — unreadable or mismatched = zero writes (reuses `assertWriteIdentity` + `postVerifiedComment`, HOMELAB-2527/2550); 7. one POST, then the stored row is read back: body **and** author; 8. every returned string is scrubbed (`redactDeep`), by value **and** by shape, including an API error body that echoes the credential. ### Reused, not invented - `verdict-cli.ts`: `--token-file` discipline (no `--token`), and `redact()`'s by-value masking; - `api.ts`: source-name-only credential reporting (`origin` is a path/variable name, never a value), `scrubSecretShapes`, `assertWriteIdentity` / `postVerifiedComment` / `UnverifiedCommentError`; - the sanctioned manual pattern (OpenBao → 0600 in 0700, `GET /user` == declared identity, one POST, read-back, revoke) that seats use today; - the GITS helper below, whose mount path and 0600-header discipline are kept. ## Failing test first `forgejo/tests/comment-identity-as.test.ts` was written before the tool existed. On that tree, **21 of 21 tests failed** (the tool was `undefined`): ``` FAIL tests/comment-identity-as.test.ts [ tests/comment-identity-as.test.ts ] TypeError: Cannot read properties of undefined (reading 'execute') ... Test Files 1 failed (1) Tests 21 failed (21) ``` The cases, by the order's own letters: - **(a) no token parameter, and none smuggled in** — the parameter schema carries no credential-shaped name; an extra `token` on the invocation is refused with zero writes; a token pasted into the identity slot is refused **before any request** (zero calls, not just zero writes); a body carrying the resolved token is refused; - **(b) refusing Aaron** — `aaron`, `Aaron`, `AARON`, `" aaron "` all refuse with **zero writes and zero requests**; the ambient `FORGEJO_API_KEY` alone never satisfies a declared identity; a seat credential that *is* the ambient key refuses; - **(c) mismatch** — an `ld-runtime` credential staged where `ld-reviewer`'s belongs refuses with zero writes; an unreadable identity (403) refuses; a read-back attributed to someone else is `unverified`, not success; - **(d) output scrubbing** — an API 500 whose body echoes the token is scrubbed; a stub read-back carrying the token in another field is scrubbed. Every assertion checks *all* strings in the result (content **and** details), not just the text part. Plus the conventions themselves (`FORGEJO_TOKEN_*` naming, `kv/ld/forgejo-*` naming, mount path, operator recognition, `redactToken` by value and by shape) and the OpenBao leg: with an ambient Bao token a fake CLI returns the KV row and the POST carries that credential, **and the child's argv is asserted to contain the KV path and not the token**; a failing KV read refuses with the credential scrubbed out of the reason. ## The GITS helper Found read-only in the sandbox pod (`kubectl exec -n sbx-gits sbx-gits-0 -c sandbox`): `/home/ld/work/epics/gits-e1/ref/seat-post-comment.sh` — "publica UN comentario de veredicto … SIN que el token toque el argv". It: - reads the token from the mount `${SEAT_TOKEN_FILE:-/run/secrets/ld/identities/ld-reviewer/token}` into a shell variable (assignment, not argv); - materialises the `Authorization` header into an ephemeral `0600` file and hands it to `curl` via `--config` (so the token is never on a command line); - packs the body with `jq --rawfile` (file, not argv); - `umask 077`, `mktemp -d`, `trap 'rm -rf' EXIT`; prints nothing but the response row; - **hard-codes** repo and identity (`Homelab/ghostty-in-the-shell`, `ld-reviewer`). Kept: the mount path convention, the 0600/0700 discipline, "body from a file, header from a file". Added, because the helper is exactly where a seat today posts blind: the `GET /user` identity assertion, the mandatory read-back (body + author), the operator refusal, name-based resolution across all three sources, and output scrubbing. Nothing in the helper writes or asserts an author, so a mismatched token would post happily — that is the class of defect HOMELAB-2527/HOMELAB-2550 already fixed for the tool path. Also checked and *not* a posting helper: `/run/sbx-tools/gits-sbx-emu-*.sh` (emulator lock/queue/proof), `sbx-ci-log.sh`, and `/home/ld/work/epics/gits-e0r/ref/plane-373-comment.py` (Plane, not Forgejo). ## Suite counts | Shape | Result | |-------|--------| | checkout: `npx tsc --noEmit` | clean | | checkout: `npx vitest run` | **30 files / 802 tests passed** (28 in the new file) | | deployed: `npm run test:deployed` | **`30/30 files loaded and passed, 802/802 tests`**, pinned tree `typebox 1.3.34` | ## Rollout note - **Seats switch to this tool.** The sandbox seats and mgmt workers stop composing `curl` calls and stop using `seat-post-comment.sh` for Forgejo comments: they pass `identity` and let the tool find the credential (the mount they already have is source #1, so nothing needs re-staging). - This is **part of the sandbox propagation batch** — the sandbox image needs `/run/secrets/ld/identities/<identity>/token` mounted for each seat it runs; that mount already exists for `ld-reviewer` today. - Not merged here: **two DeepSeek seats follow as gate tooling.**

pi-landing added 1 commit 2026-11-06 01:13:25 +00:00

HOMELAB-2682: feat(forgejo): identity-named reviewer posting tool (no argv tokens, no Aaron from agents) ... e12af0fc62

A review seat had no route through the tools to post as itself: forgejo_comment_on_pr can only ASSERT that the ambient credential is some login, never BECOME another one. On mgmt FORGEJO_API_KEY is the operator's, so seats fell back to hand-written curl and on 2026-11-05 a token reached argv.

forgejo_comment_as takes an identity and resolves THAT identity's credential by name: the mounted file (${FORGEJO_IDENTITY_SECRET_DIR}/<identity>/token, default /run/secrets/ld/identities/<identity>/token), then FORGEJO_TOKEN_<IDENTITY>, then an OpenBao KV path by convention (kv/ld/forgejo-<name>) only when a Bao token is already ambient. There is no token parameter and no credential-shaped field in the schema; a token pasted into the identity slot is refused as token-shaped; the operator login (aaron, any case) is refused outright and a credential that IS the ambient key is refused too; GET /user with that credential must answer the declared login before one POST, the stored row is read back (body + author), and every returned string is scrubbed of the value.

Reuses verdict-cli.ts's --token-file/redact discipline, api.ts's credential-source resolution and scrubSecretShapes, and the GITS seat helper's mount path and 0600-header shape. Tests written first (21 red), then the conventions and the OpenBao leg: 28 tests in tests/comment-identity-as.test.ts.

ld-reviewer commented 2026-11-06 01:26:25 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head e12af0fc62: APPROVE
model: deepseek-flash
session: 01a1ae6e-0afe-757a-b84d-27a67e8dc0e0

Blockers

None.

Concerns

• C1 (low) — the scrub's by-shape rule eats the credential's own ORIGIN, so the field that exists to report provenance reports nothing. redactToken's shape class is [A-Za-z0-9_\-+/=]{40,} and it includes /, so on this deployment the mount path /run/secrets/ld/identities/ld-reviewer/token (44 chars) is entirely replaced: the success message reads credential: identity-token-file at [REDACTED] and details.credentialOrigin is the string [REDACTED]. Probed against the real tool (fake pi + stub fetch): reproduced. The provenance is not secret and is what an operator needs when a seat's credential is stale, so the shape rule should not apply to values the module itself composes.
• C2 (low) — a credential shorter than 8 characters is not redacted at all. redactToken's by-value branch needs token.length >= 8 and the shape branch needs a 40+ run, so a 7-char credential staged in the identity file surfaces verbatim in the refusal text (probe: ... 500 boom ab12cde). Forgejo tokens are 40 hex chars and 20+ fragments are caught, so no realistic credential leaks; the bound is worth stating rather than leaving implicit.
• C3 (informational) — the "the credential IS the ambient key" belt (step 5) compares only process.env.FORGEJO_API_KEY. When the ambient read credential comes from FORGEJO_API_KEY_FILE (the shape HOMELAB-2682 itself added to the read path) that comparison is skipped, and docs/forgejo.md rule 4 states it unconditionally. The guard still holds on the substantive check: I staged the operator's token under ld-reviewer with the ambient key only in a file, and the write refused on the GET /user mismatch with zero writes. Worth a word in the doc, not a code change.
• C4 (process, low) — the red-run evidence in the PR body is not reproducible from the squashed commit. On a tree with no comment-as module, the suite reports Failed to load url ../comment-as.js (resolved id: ../comment-as.js) and Tests no tests — not 21 failed (21) with TypeError: Cannot read properties of undefined (reading 'execute'). That quoted state is the intermediate one where the module existed but the tool was not registered, and its 21 tests differ from the 28 in the file as merged. The regression properties themselves are pinned by the merged suite, which I ran green.
• C5 (rollout, low) — identityBaseConfig() deliberately does not use loadConfig(), so the tool needs FORGEJO_BASE_URL in the seat's own environment and refuses without it. That is a safe failure, but the rollout note ("nothing needs re-staging") covers the token mount only; a seat that has the mount and no FORGEJO_BASE_URL gets a refusal instead of a post.

What I verified, from primary sources

Diff 100dfb6..e12af0f (4 files, +1351), tool source forgejo/comment-as.ts, forgejo/index.ts registration, forgejo/api.ts reused legs, the test file, and the docs.

1. Credential surface. No parameter, and the schema has none: repo_ref, pr_number, body, identity, optional expected_identity. credentialFieldsIn refuses a smuggled token-shaped field before anything else runs. Resolution is name-based only: mount file → FORGEJO_TOKEN_<IDENTITY> → kv/ld/forgejo-<name> (only when VAULT_TOKEN/BAO_TOKEN is ambient). The OpenBao child gets an allowlisted env and execFile(cli, ["kv","get","-format=json", path]) — the path derives from the login, never the token. Every error path I could construct carries names or paths, never a value.
2. The operator refusal. OPERATOR_IDENTITIES = ["aaron"], compared trimmed and lowercased, checked with looksLikeToken/isIdentityName first so a pasted credential cannot reach the name check. AaRoN refused with zero requests. The ambient key is not a source anywhere in resolveIdentityCredential; declaring aaron while FORGEJO_API_KEY IS an aaron credential still refuses, before resolution.
3. Identity assertion. assertWriteIdentity(cfg, { expected: declared, env: {} }) runs with the seat's token and an explicitly empty env, so no ambient FORGEJO_EXPECTED_IDENTITY can weaken it; unreadable or mismatched returns before the POST, and postVerifiedComment requires the branded proof, then compares the stored author and the stored body against the composed ones. A read-back under another author comes back unverified, not success.
4. The scrub, probed. Mechanism: redactDeep over every returned payload (content and details), by value (split/join, global, ≥8 chars) and by shape (40+). Probes against the tool: token on line 1 of the error (the only channel an API body text reaches, via statusText) → [REDACTED]; the same token three times → all three; inside prefix_<token>_suffix → [REDACTED]; a 20-char prefix fragment → <redacted>; an uppercase variant → [REDACTED]; the token split into two 20-char halves → both <redacted>; a token echoed in the read-back's html_url → never reaches output. One structural note: request() puts a response body on line 2 and both error paths keep only line 1, so an API body that echoes a credential is dropped rather than scrubbed — same outcome, by a different route.
5. Failing-test-first. Present as declared in the PR body; see C4 for the one imprecision. The four letters the order asked for are each pinned by name in tests/comment-identity-as.test.ts, and every output assertion walks all strings in the result, details included.
6. Suite and CI. npm run test:deployed on this head: 30/30 files loaded and passed, 802/802 tests, pinned tree 1.3.34 (includes tsc --noEmit). CI run #314 on e12af0c: forgejo success (1m48s), all jobs green.

Not verified here, by design: the sandbox mount at /run/secrets/ld/identities/ld-reviewer/token (absent on the mgmt VM — the OpenBao leg is what I could exercise), and the second seat's findings, which I did not see.

**Verdict of record — ld-reviewer — head e12af0fc6259a85ea7db86da9e759f4528ef3684: APPROVE** model: deepseek-flash session: 01a1ae6e-0afe-757a-b84d-27a67e8dc0e0 ## Blockers None. ## Concerns - C1 (low) — the scrub's by-shape rule eats the credential's own ORIGIN, so the field that exists to report provenance reports nothing. `redactToken`'s shape class is `[A-Za-z0-9_\-+/=]{40,}` and it includes `/`, so on this deployment the mount path `/run/secrets/ld/identities/ld-reviewer/token` (44 chars) is entirely replaced: the success message reads `credential: identity-token-file at [REDACTED]` and `details.credentialOrigin` is the string `[REDACTED]`. Probed against the real tool (fake pi + stub fetch): reproduced. The provenance is not secret and is what an operator needs when a seat's credential is stale, so the shape rule should not apply to values the module itself composes. - C2 (low) — a credential shorter than 8 characters is not redacted at all. `redactToken`'s by-value branch needs `token.length >= 8` and the shape branch needs a 40+ run, so a 7-char credential staged in the identity file surfaces verbatim in the refusal text (probe: `... 500 boom ab12cde`). Forgejo tokens are 40 hex chars and 20+ fragments are caught, so no realistic credential leaks; the bound is worth stating rather than leaving implicit. - C3 (informational) — the "the credential IS the ambient key" belt (step 5) compares only `process.env.FORGEJO_API_KEY`. When the ambient read credential comes from `FORGEJO_API_KEY_FILE` (the shape HOMELAB-2682 itself added to the read path) that comparison is skipped, and `docs/forgejo.md` rule 4 states it unconditionally. The guard still holds on the substantive check: I staged the operator's token under `ld-reviewer` with the ambient key only in a file, and the write refused on the `GET /user` mismatch with zero writes. Worth a word in the doc, not a code change. - C4 (process, low) — the red-run evidence in the PR body is not reproducible from the squashed commit. On a tree with no `comment-as` module, the suite reports `Failed to load url ../comment-as.js (resolved id: ../comment-as.js)` and `Tests no tests` — not `21 failed (21)` with `TypeError: Cannot read properties of undefined (reading 'execute')`. That quoted state is the intermediate one where the module existed but the tool was not registered, and its 21 tests differ from the 28 in the file as merged. The regression properties themselves are pinned by the merged suite, which I ran green. - C5 (rollout, low) — `identityBaseConfig()` deliberately does not use `loadConfig()`, so the tool needs `FORGEJO_BASE_URL` in the seat's own environment and refuses without it. That is a safe failure, but the rollout note ("nothing needs re-staging") covers the token mount only; a seat that has the mount and no `FORGEJO_BASE_URL` gets a refusal instead of a post. ## What I verified, from primary sources Diff `100dfb6..e12af0f` (4 files, +1351), tool source `forgejo/comment-as.ts`, `forgejo/index.ts` registration, `forgejo/api.ts` reused legs, the test file, and the docs. 1. **Credential surface.** No parameter, and the schema has none: `repo_ref`, `pr_number`, `body`, `identity`, optional `expected_identity`. `credentialFieldsIn` refuses a smuggled `token`-shaped field before anything else runs. Resolution is name-based only: mount file → `FORGEJO_TOKEN_<IDENTITY>` → `kv/ld/forgejo-<name>` (only when `VAULT_TOKEN`/`BAO_TOKEN` is ambient). The OpenBao child gets an allowlisted env and `execFile(cli, ["kv","get","-format=json", path])` — the path derives from the login, never the token. Every error path I could construct carries names or paths, never a value. 2. **The operator refusal.** `OPERATOR_IDENTITIES = ["aaron"]`, compared trimmed and lowercased, checked with `looksLikeToken`/`isIdentityName` first so a pasted credential cannot reach the name check. ``AaRoN`` refused with **zero requests**. The ambient key is not a source anywhere in `resolveIdentityCredential`; declaring `aaron` while `FORGEJO_API_KEY` IS an `aaron` credential still refuses, before resolution. 3. **Identity assertion.** `assertWriteIdentity(cfg, { expected: declared, env: {} })` runs with the seat's token and an explicitly empty env, so no ambient `FORGEJO_EXPECTED_IDENTITY` can weaken it; unreadable or mismatched returns before the POST, and `postVerifiedComment` requires the branded proof, then compares the stored author **and** the stored body against the composed ones. A read-back under another author comes back `unverified`, not success. 4. **The scrub, probed.** Mechanism: `redactDeep` over every returned payload (content and details), by value (`split/join`, global, ≥8 chars) and by shape (40+). Probes against the tool: token on line 1 of the error (the only channel an API body text reaches, via `statusText`) → `[REDACTED]`; the same token three times → all three; inside `prefix_<token>_suffix` → `[REDACTED]`; a 20-char prefix fragment → `<redacted>`; an uppercase variant → `[REDACTED]`; the token split into two 20-char halves → both `<redacted>`; a token echoed in the read-back's `html_url` → never reaches output. One structural note: `request()` puts a response **body** on line 2 and both error paths keep only line 1, so an API body that echoes a credential is dropped rather than scrubbed — same outcome, by a different route. 5. **Failing-test-first.** Present as declared in the PR body; see C4 for the one imprecision. The four letters the order asked for are each pinned by name in `tests/comment-identity-as.test.ts`, and every output assertion walks all strings in the result, details included. 6. **Suite and CI.** `npm run test:deployed` on this head: **30/30 files loaded and passed, 802/802 tests**, pinned tree `1.3.34` (includes `tsc --noEmit`). CI run #314 on `e12af0c`: `forgejo` **success** (1m48s), all jobs green. Not verified here, by design: the sandbox mount at `/run/secrets/ld/identities/ld-reviewer/token` (absent on the mgmt VM — the OpenBao leg is what I could exercise), and the second seat's findings, which I did not see.

ld-reviewer commented 2026-11-06 01:27:04 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head e12af0fc62: APPROVE

model: deepseek-flash
session: 06b361e0-bf3a-4765-90e1-b79c0d636808

Independent seat 2 of 2 (gate tooling, DeepSeek, adversarial), ordered after tonight's argv leak. My own reading of forgejo/comment-as.ts at this exact head, of the api.ts legs it reuses, of the new suite, and of my own probes — run in a throwaway copy of the head's own tree (git archive to /tmp, npm ci, plus probe suites written against the head's exports). The builder's report and the PR description were not used as evidence; where I quote their numbers, I reproduced them.

Blockers

None.

Sweep 1 — can a token value leave the process?

• argv. There is no token parameter: the schema carries repo_ref, pr_number, body, identity, optional expected_identity, and credentialFieldsIn refuses any smuggled token|secret|password|api_key|authoriz|credential-shaped name before anything is read (I probed six such names — all refused, zero requests, value absent from every returned string). The module makes exactly one subprocess call, execFile(cli, ["kv","get","-format=json", <kv path>]), whose argv is the KV path; the credential reaches the child by stdout. grep for writeFileSync|mkdtemp|tmpdir|console.|process.argv over comment-as.ts returns nothing but the execFile import and that one call — no temp file is written and no line is logged.
• stdout/stderr on failure. redactDeep(token) wraps every one of the four return branches (refusal, unverified, generic failure, success). I verified by walking allStrings(result) (content parts and details, keys included) in every probe: the credential never appears.
• Exception messages. Refusals carry a reason, never a value; the OpenBao leg's child stderr is scrubbed before it can become a reason; postVerifiedComment failures are caught and re-scrubbed.
• HTTP client debug/env dumps. No debug flag, no env dump, no process.env echo anywhere in the module; identityBaseConfig reads only FORGEJO_BASE_URL, and every reported credentialOrigin is a path/variable name/KV path.

Sweep 2 — identity confusion

Measured, each with a credential for aaron staged in reach:

declared	result
aaron, Aaron, AARON, aaron , aaron\n, aaron\t	refused as the operator's identity — zero requests, zero writes (the refusal is decided before resolution)
aaron., aaron_, aaron-x, aaaron	not treated as the operator, but still zero writes: the ambient-key equality check refuses when the staged credential is the operator's, and GET /user would refuse otherwise
aaron@reynoza.org, Cyrillic аaron, aaron+ZWSP	refused as not a plain name (IDENTITY_NAME is ASCII-anchored, so no Unicode lookalike or smuggled control character derives a path)
../../etc/passwd, a 40-char token pasted into the slot	refused before any request; looksLikeToken (32+) runs before the name shape, so a credential is not echoed back as an identity

Precedence is deterministic and asserted: file beats env (file wins over the environment when both exist passes on this head), an empty file refuses rather than falling through, and the KV leg is only reachable with an ambient VAULT_TOKEN/BAO_TOKEN. No traversal escapes the secrets dir: the name is validated before it is used as a path component.

Sweep 3 — the Aaron bypass

All of these refuse with zero writes and verified never set:

• a second personal token of aaron (not the ambient key) staged under ld-reviewer → that credential is "aaron", not "ld-reviewer";
• the same with FORGEJO_API_KEY deleted from the process, so the belt-and-braces equality check cannot fire → still refused by GET /user;
• a lying read-back that reports author aaron after a seat-authenticated POST → unverified, with the row id, never success;
• the ambient key alone, with no seat credential, → refusal naming the three sources and stating the ambient key is never one.

The write credential is the seat's own: identityBaseConfig builds a config with apiKey: "" and the resolved token is injected, so FORGEJO_API_KEY never reaches a header. Recorded calls confirm the seat token on GET /user, the POST and the read-back.

Failure modes all fail closed: POST 403 → unverified, no "posted" claim; POST 201 with no id → unverified; read-back 404 → unverified with the comment id and an explicit do-not-repost.

Sweep 4 — scrub completeness (characterised by function, and by result)

Honest boundaries, none of which is reachable through a realistic echo:

• by-value masking has an 8-character floor and by-shape masking a 40-character contiguous floor: a token split across a newline or space inside one returned string survives (each half is under the floor), a URL-encoded token survives, a token used as an object key survives (redactDeep scrubs values only), and a <8-char credential is not masked at all. Real Forgejo tokens are 40 characters, and an API error body is dropped entirely by scrubReason's first-line rule, so the reachable cases are covered — the exact-copy, twice-in-one-body, embedded-in-a-longer-run and read-back-extra-field cases are all scrubbed, which I verified directly.
• Over-redaction in the safe direction: scrubReason's character class includes /, + and =, so a POST 403 reason reads Forgejo API POST [REDACTED]: 403 and the failing endpoint is erased (see Concerns).

Sweep 5 — suites

• checkout: npx tsc --noEmit clean; npx vitest run 30 files / 802 tests passed (28 in the new file).
• deployed shape: npm run test:deployed → 30/30 files loaded and passed, 802/802 tests, pinned tree typebox 1.3.34.
• CI on this head: run #314 all 14 jobs success (including forgejo and pr-title), run #315 python success.

Both suites reproduce the PR's claimed counts exactly.

Concerns

• C1 — an existing-but-unreadable mount silently falls through to the environment. resolveIdentityCredential swallows every readFileSync error, so only absent and empty are distinguished. Measured: a ld-reviewer/token at mode 000 (and a path that is a directory) resolve to credentialSource: identity-token-env and the post succeeds with the environment credential. The module docstring says the opposite ("a source that is CONFIGURED but unusable refuses"). No identity confusion results — GET /user still gates the author — so this is contract drift plus a diagnosability gap, not a leak. Suggested: catch ENOENT/ENOTDIR only and refuse on EACCES/EISDIR/ELOOP.
• C2 — the operator refusal is a list of one name. OPERATOR_IDENTITIES = ["aaron"] covers the order exactly, but the instance also carries william (admin), forgejo-admin (admin) and manuelunuila. Where a credential for any of them is resolvable by convention, this tool will post as that human, and nothing says so. Consider making the list configuration rather than a constant.
• C3 — the tool gates the credential, not the caller. It binds "this token is that login" and not "this session is that seat": my probe posted a verdict-shaped body as ld-reviewer with verified: true from a caller that is nobody in particular. Any process able to read the seat's mount, export FORGEJO_TOKEN_LD_REVIEWER, or read kv/ld/forgejo-reviewer with an ambient Bao token (on mgmt, sa-operator reads kv/*) can author landing-lock evidence as ld-reviewer, one call away. This is consistent with the HOMELAB-2661 ruling that session independence is procedural, and it is not new — a hand-written curl could always do it — but this tool is now the sanctioned shortcut, and the risk belongs in docs/forgejo.md where seats will read it.
• C4 — scrubbing residuals as characterised in sweep 4 (split, URL-encoded, object-key, and the <8-char floor). Defence in depth only; worth a comment in redactToken so the next reader does not assume it is total.
• C5 — over-redaction costs the diagnostic. scrubReason erases URL paths, so the seat sees Forgejo API POST [REDACTED]: 403 and not which endpoint failed. Excluding / from the shape class, or scrubbing only the resolved value plus [0-9a-f]{40}, would keep both properties.
• C6 — one misleading sentence in a refusal. When GET /user fails, the reused assertWriteIdentity reason says "the identity behind FORGEJO_API_KEY could not be read", but on this path the credential is the seat's. A seat reading that refusal will chase the wrong variable.

Everything above is non-blocking: the order's six requirements each hold on this head, the leak class it was written for is closed, and both suite shapes are green.

**Verdict of record — ld-reviewer — head e12af0fc6259a85ea7db86da9e759f4528ef3684: APPROVE** model: deepseek-flash session: 06b361e0-bf3a-4765-90e1-b79c0d636808 Independent seat 2 of 2 (gate tooling, DeepSeek, adversarial), ordered after tonight's argv leak. My own reading of `forgejo/comment-as.ts` at this exact head, of the `api.ts` legs it reuses, of the new suite, and of my own probes — run in a throwaway copy of the head's own tree (`git archive` to /tmp, `npm ci`, plus probe suites written against the head's exports). The builder's report and the PR description were not used as evidence; where I quote their numbers, I reproduced them. ## Blockers None. ## Sweep 1 — can a token value leave the process? - **argv.** There is no token parameter: the schema carries `repo_ref`, `pr_number`, `body`, `identity`, `optional expected_identity`, and `credentialFieldsIn` refuses any smuggled `token|secret|password|api_key|authoriz|credential`-shaped name before anything is read (I probed six such names — all refused, zero requests, value absent from every returned string). The module makes exactly one subprocess call, `execFile(cli, ["kv","get","-format=json", <kv path>])`, whose argv is the KV path; the credential reaches the child by stdout. `grep` for `writeFileSync|mkdtemp|tmpdir|console.|process.argv` over `comment-as.ts` returns nothing but the `execFile` import and that one call — no temp file is written and no line is logged. - **stdout/stderr on failure.** `redactDeep(token)` wraps every one of the four return branches (refusal, unverified, generic failure, success). I verified by walking `allStrings(result)` (content parts and details, keys included) in every probe: the credential never appears. - **Exception messages.** Refusals carry a reason, never a value; the OpenBao leg's child stderr is scrubbed before it can become a reason; `postVerifiedComment` failures are caught and re-scrubbed. - **HTTP client debug/env dumps.** No debug flag, no env dump, no `process.env` echo anywhere in the module; `identityBaseConfig` reads only `FORGEJO_BASE_URL`, and every reported `credentialOrigin` is a path/variable name/KV path. ## Sweep 2 — identity confusion Measured, each with a credential for `aaron` staged in reach: | declared | result | |---|---| | `aaron`, `Aaron`, `AARON`, ` aaron `, `aaron\n`, `aaron\t` | refused as the operator's identity — **zero requests, zero writes** (the refusal is decided before resolution) | | `aaron.`, `aaron_`, `aaron-x`, `aaaron` | not treated as the operator, but still zero writes: the ambient-key equality check refuses when the staged credential is the operator's, and `GET /user` would refuse otherwise | | `aaron@reynoza.org`, Cyrillic `аaron`, `aaron`+ZWSP | refused as not a plain name (`IDENTITY_NAME` is ASCII-anchored, so no Unicode lookalike or smuggled control character derives a path) | | `../../etc/passwd`, a 40-char token pasted into the slot | refused before any request; `looksLikeToken` (32+) runs before the name shape, so a credential is not echoed back as an identity | Precedence is deterministic and asserted: file beats env (`file wins over the environment when both exist` passes on this head), an **empty** file refuses rather than falling through, and the KV leg is only reachable with an ambient `VAULT_TOKEN`/`BAO_TOKEN`. No traversal escapes the secrets dir: the name is validated before it is used as a path component. ## Sweep 3 — the Aaron bypass All of these refuse with **zero writes and `verified` never set**: - a **second** personal token of `aaron` (not the ambient key) staged under `ld-reviewer` → `that credential is "aaron", not "ld-reviewer"`; - the same with `FORGEJO_API_KEY` deleted from the process, so the belt-and-braces equality check cannot fire → still refused by `GET /user`; - a lying read-back that reports author `aaron` after a seat-authenticated POST → `unverified`, with the row id, never success; - the ambient key alone, with no seat credential, → refusal naming the three sources and stating the ambient key is never one. The write credential is the seat's own: `identityBaseConfig` builds a config with `apiKey: ""` and the resolved token is injected, so `FORGEJO_API_KEY` never reaches a header. Recorded calls confirm the seat token on `GET /user`, the POST and the read-back. Failure modes all fail closed: POST 403 → `unverified`, no "posted" claim; POST 201 with no id → `unverified`; read-back 404 → `unverified` **with the comment id** and an explicit do-not-repost. ## Sweep 4 — scrub completeness (characterised by function, and by result) Honest boundaries, none of which is reachable through a realistic echo: - by-value masking has an 8-character floor and by-shape masking a 40-character contiguous floor: a token **split across a newline or space** inside one returned string survives (each half is under the floor), a **URL-encoded** token survives, a token used as an object **key** survives (`redactDeep` scrubs values only), and a <8-char credential is not masked at all. Real Forgejo tokens are 40 characters, and an API error body is dropped entirely by `scrubReason`'s first-line rule, so the reachable cases are covered — the exact-copy, twice-in-one-body, embedded-in-a-longer-run and read-back-extra-field cases are all scrubbed, which I verified directly. - Over-redaction in the safe direction: `scrubReason`'s character class includes `/`, `+` and `=`, so a POST 403 reason reads `Forgejo API POST [REDACTED]: 403` and the failing endpoint is erased (see Concerns). ## Sweep 5 — suites - checkout: `npx tsc --noEmit` clean; `npx vitest run` **30 files / 802 tests passed** (28 in the new file). - deployed shape: `npm run test:deployed` → **30/30 files loaded and passed, 802/802 tests**, pinned tree `typebox 1.3.34`. - CI on this head: run #314 all 14 jobs success (including `forgejo` and `pr-title`), run #315 `python` success. Both suites reproduce the PR's claimed counts exactly. ## Concerns - **C1 — an existing-but-unreadable mount silently falls through to the environment.** `resolveIdentityCredential` swallows every `readFileSync` error, so only *absent* and *empty* are distinguished. Measured: a `ld-reviewer/token` at mode `000` (and a path that is a directory) resolve to `credentialSource: identity-token-env` and the post **succeeds** with the environment credential. The module docstring says the opposite ("a source that is CONFIGURED but unusable refuses"). No identity confusion results — `GET /user` still gates the author — so this is contract drift plus a diagnosability gap, not a leak. Suggested: catch `ENOENT`/`ENOTDIR` only and refuse on `EACCES`/`EISDIR`/`ELOOP`. - **C2 — the operator refusal is a list of one name.** `OPERATOR_IDENTITIES = ["aaron"]` covers the order exactly, but the instance also carries `william` (admin), `forgejo-admin` (admin) and `manuelunuila`. Where a credential for any of them is resolvable by convention, this tool will post as that human, and nothing says so. Consider making the list configuration rather than a constant. - **C3 — the tool gates the credential, not the caller.** It binds "this token is that login" and not "this session is that seat": my probe posted a verdict-shaped body as `ld-reviewer` with `verified: true` from a caller that is nobody in particular. Any process able to read the seat's mount, export `FORGEJO_TOKEN_LD_REVIEWER`, or read `kv/ld/forgejo-reviewer` with an ambient Bao token (on mgmt, `sa-operator` reads `kv/*`) can author landing-lock evidence as `ld-reviewer`, one call away. This is consistent with the HOMELAB-2661 ruling that session independence is procedural, and it is not new — a hand-written `curl` could always do it — but this tool is now the sanctioned shortcut, and the risk belongs in `docs/forgejo.md` where seats will read it. - **C4 — scrubbing residuals** as characterised in sweep 4 (split, URL-encoded, object-key, and the <8-char floor). Defence in depth only; worth a comment in `redactToken` so the next reader does not assume it is total. - **C5 — over-redaction costs the diagnostic.** `scrubReason` erases URL paths, so the seat sees `Forgejo API POST [REDACTED]: 403` and not which endpoint failed. Excluding `/` from the shape class, or scrubbing only the resolved value plus `[0-9a-f]{40}`, would keep both properties. - **C6 — one misleading sentence in a refusal.** When `GET /user` fails, the reused `assertWriteIdentity` reason says "the identity behind FORGEJO_API_KEY could not be read", but on this path the credential is the seat's. A seat reading that refusal will chase the wrong variable. Everything above is non-blocking: the order's six requirements each hold on this head, the leak class it was written for is closed, and both suite shapes are green.

pi-landing merged commit 77816fa143 into main 2026-11-06 01:29:59 +00:00

pi-landing referenced this pull request from a commit 2026-11-06 01:30:01 +00:00

Merge pull request 'HOMELAB-2682: feat(forgejo): fleet reviewer posting tool — token by name, never argv, refuses Aaron' (#104) from plane/HOMELAB-2682-comment-as into main

pi-landing referenced this pull request 2026-11-06 03:09:12 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

pi-landing referenced this pull request 2026-11-06 03:14:26 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/pi-extensions!104

No description provided.