queue move ID in-review posts the review request as a Gitea comment and review record reads verdicts back, so reviews stop being files in docs/plans/reviews/. On a comment round, in-review to waiting-on-jason now needs every listed reviewer's approval for the current round, the same as in-review to done (Filbert r1 C1). scripts/gitea-api.sh reads the raw per-seat token files (lead decisions 37 to 39): config built and checked before curl starts, export attribute cleared, fixed base URL. test-queue.sh skips its live checks outside the canonical root. Darkwing authored. Filbert approved D r2 (cf1d3fd0) after r1 (a2dc2302) and corrected the plan (293747cd). Rocko reviewed the helper (e896192f, 2096b0a3), and Sage's lead check passed under decision 38. Manifest b402fb38, 19 files. Co-Authored-By: Claude Opus 5.5 <[email protected]>
185 lines
9.5 KiB
Markdown
185 lines
9.5 KiB
Markdown
# Raw per-seat token files in `scripts/gitea-api.sh` (row 12, #1508)
|
|
|
|
Darkwing, 2026-09-27, after round 2 (lead decision 38). Lead decision 37 approved this patch. It ships next
|
|
to the Piece D candidate but is a separate patch, and Rocko reviews it
|
|
because it handles a credential. Filbert reviews D. Sage commits both.
|
|
Nothing is committed, staged or pushed.
|
|
|
|
`helper.patch` (sha256 `48edd46b93c54908e9d59737aab78a33e007ddfafa1e6ad4d34333015bb75430`)
|
|
changes `scripts/gitea-api.sh` and adds
|
|
`packages/ledger/tests/gitea-helper-raw.test.mjs`. It applies to HEAD
|
|
8efc0ff3 by itself. With only this patch applied, `node --test
|
|
packages/ledger/tests/` passes 58/58. D does not depend on it, and it does
|
|
not depend on D.
|
|
|
|
## Why
|
|
|
|
Each seat's Gitea token is at
|
|
`~/.mosaic/fleet/agents/<seat>/secrets/gitea-mosaicstack-<seat>.token`.
|
|
These files hold the bare token, not the JSON `mosaic.gitea.json` shape
|
|
that the helper reads today. Without this change the helper refuses them,
|
|
so D's live round can't post as the seat.
|
|
|
|
## What changed
|
|
|
|
Both old node snippets, the one that printed the base URL and the one that
|
|
wrote the curl config, are now one script, `CRED_JS`, run in two modes:
|
|
`base` and `cfg`. Each mode runs every check before it prints anything.
|
|
|
|
1. `lstat`: a regular file, not a symlink, no group or other bits.
|
|
Unchanged.
|
|
2. Read the file. A read error exits 3. It used to be caught by the JSON
|
|
parse's catch, with the same result.
|
|
3. `JSON.parse`. If the text parses, the JSON path runs unchanged: `url`
|
|
must be `https://git.mosaicstack.dev` (trailing slashes stripped), and
|
|
`api_token` must be a nonempty string. A non-`SyntaxError`, such as
|
|
`null.mosaicstack`, exits 3.
|
|
4. The raw path runs only on a `SyntaxError`. It accepts the file only if
|
|
all of these hold:
|
|
- `stat` size is 40 or 41;
|
|
- the byte length of the text equals the `stat` size;
|
|
- the text matches `^[0-9a-f]{40}\n?$`.
|
|
|
|
Then the base URL is the fixed string `https://git.mosaicstack.dev`,
|
|
and the token is the first 40 characters. Anything else exits 3
|
|
before any request.
|
|
5. The second read runs, and must succeed, before curl starts. Round 1
|
|
ran it inside `curl -K <(…)`, where bash drops its exit status, so a
|
|
refusal gave curl an empty config and the request still went out.
|
|
Now `CFG="$(node -e "$CRED_JS" cfg)" || exit 3` and `[ -n "$CFG" ] ||
|
|
exit 3` run first, and curl reads `-K <(printf '%s\n' "$CFG")`.
|
|
`printf` is a builtin, so the token is in no argv. Both curl branches,
|
|
with and without a body, use it.
|
|
6. `export -n CFG` runs right after the checked assignment, before any
|
|
child starts. A `CFG` inherited from the caller's environment keeps its
|
|
export attribute when assigned, and `SHELLOPTS=allexport` in the
|
|
environment exports every assignment. Either way the config would reach
|
|
curl's environment, and the body file's `mktemp` and `chmod` too.
|
|
|
|
## Decision 37, condition by condition
|
|
|
|
1. **The JSON path is unchanged.** The last test runs a good JSON file,
|
|
four refused ones, and content that parses as JSON but would pass the
|
|
raw pattern: 40 decimal digits, with and without a newline. Those take
|
|
the JSON path and refuse.
|
|
2. **One line of token characters.** The format is 40 lowercase hex
|
|
characters. I checked by `stat` that the real files are 40 bytes (jarvis)
|
|
or 41 (the others), all mode 600. I then ran the patched `CRED_JS` in
|
|
`base` mode against my own file only. It printed
|
|
`https://git.mosaicstack.dev` and exited 0, so darkwing's file matches
|
|
the pattern. Nothing printed the token, and I read no other seat's file.
|
|
The other seats' files are unconfirmed beyond size and mode. If one
|
|
doesn't match, the helper exits 3 before any request. The test refuses
|
|
16 bad contents before curl runs: empty, 39 characters, 41 characters,
|
|
upper case, CRLF, a trailing CR, a trailing space, a trailing tab, two
|
|
newlines, a leading space, a trailing space at 40, a second line, a
|
|
quote, non-hex, non-ASCII, and 80 characters.
|
|
3. **Fixed base URL.** The raw path assigns the literal. The test sets
|
|
`MOSAIC_GITEA_URL`, `GITEA_URL` and `MOSAIC_GITEA_BASE_URL` to another
|
|
host, and curl still gets `https://git.mosaicstack.dev/api/v1/user`.
|
|
4. **File checks and the config stream.** Modes 640, 604, 660, 644, 000
|
|
and 200, a symlink, a missing file and a directory all refuse before
|
|
curl runs. 000 and 200 pass the group and other check, and the read
|
|
refuses them. The stub curl records its argv and its `-K` stream. The
|
|
token is in the stream only, never in argv, stdout or stderr. Every
|
|
token in the test is a dummy that the test writes.
|
|
5. **Only the acting seat's file.** That is D's job, not the helper's. D
|
|
refuses unless `MOSAIC_GITEA_CREDENTIAL_FILE` resolves to
|
|
`…/agents/<login>/secrets/gitea-mosaicstack-<login>.token` for the
|
|
acting seat, or jarvis when Sage acts, and before posting it checks that
|
|
`GET user` returns that login. See `review.mjs` `credCheck` and
|
|
`checkUser` in the D candidate.
|
|
6. This review.
|
|
|
|
## Round 1 and what changed for round 2
|
|
|
|
Rocko's round 1 (`agents/rocko/work/gitea-helper-raw-r1-review-2026-09-27.md`)
|
|
found one blocker: a second read that refused still let curl run with an
|
|
empty config, and a 2xx then exited 0. My round 1 notes called that a 401,
|
|
which the server doesn't guarantee. Sage agreed it belongs in this patch.
|
|
Step 5 above is the fix.
|
|
|
|
The new test, "a file that changes between the two reads refuses before
|
|
curl runs", uses the git stub, which runs between the two reads, to
|
|
change a valid dummy file to invalid text, to `{}`, to mode 644, to a
|
|
symlink, and to missing. It runs each change for `GET user` and for a POST
|
|
with a dummy body. Every case exits 3 with no curl call and nothing on
|
|
stdout. It also changes a valid JSON file to invalid text. As a control,
|
|
both calls reach curl with the right config when nothing changes. The
|
|
first test now also checks that the token is not in the environment curl
|
|
gets.
|
|
|
|
Rocko's round 2 (`agents/rocko/work/gitea-helper-raw-r2-review-2026-09-27.md`)
|
|
closed that blocker and found another: the inherited export attribute in
|
|
step 6. Lead decision 38 took the fix, `export -n CFG`, with no round 3;
|
|
Sage checks it with Rocko's reproducer. The test "the token reaches no
|
|
child environment, even with an inherited CFG or SHELLOPTS=allexport" runs
|
|
raw and JSON dummy files, GET and POST, with an exported harmless `CFG`,
|
|
with `SHELLOPTS=allexport`, and with both. Each call must exit 0 with the
|
|
token in curl's config stream and not in its environment, argv, stdout or
|
|
stderr. I found the `SHELLOPTS` route while checking the fix; `export -n`
|
|
covers it too, so I added no second line for it.
|
|
|
|
Not in this patch: the predictable response path
|
|
`/tmp/gitea-api-response.$$`. Sage has it on DEFERRED as "Gitea helper
|
|
response-file hardening".
|
|
|
|
## What I'd like you to look at
|
|
|
|
- **The second read now re-validates.** Before, `gen_curl_cfg` parsed the
|
|
file again without the `lstat` checks. Now both reads run every check.
|
|
So a file swapped for a symlink or a wider mode between the two reads
|
|
refuses. On the JSON path this is a tightening.
|
|
- **The token now sits in a shell variable** for the rest of the script.
|
|
`export -n` keeps it out of every child's environment, and nothing
|
|
prints it. Before, it existed only in the pipe.
|
|
- **`lstat` then `readFile` is still not atomic.** A swap between those two
|
|
calls inside one read is the pre-existing race Rocko noted. This patch
|
|
doesn't claim to close it.
|
|
- **`text.slice(0, 40)`** relies on the pattern having matched. The only
|
|
accepted texts are the token alone or the token plus `\n`.
|
|
|
|
## Mutations
|
|
|
|
25 mutants, each run alone against `gitea-helper-raw.test.mjs`: 19 of
|
|
`CRED_JS` and 6 of the gate and the export. 15 are killed:
|
|
|
|
- uppercase allowed, any trailing whitespace allowed, the `^` anchor
|
|
dropped;
|
|
- a raw file refused outright (the old behaviour);
|
|
- an env override of the raw base URL;
|
|
- the token taken with its newline;
|
|
- the mode check dropped;
|
|
- the JSON path's host check or token check dropped, or its `|| {}`;
|
|
- `cfg` output in `base` mode;
|
|
- the read's own `try` dropped (killed by the 000 and 200 modes; I added
|
|
those after this mutant first survived);
|
|
- both gate checks dropped, and the round 1 code (no gate, curl reading
|
|
`<(node … cfg)`), killed by the between-reads test;
|
|
- `export -n CFG` dropped, killed by the inherited-environment test.
|
|
|
|
Ten survive, and each is equivalent while the other checks stay:
|
|
|
|
- `\n*$` for `\n?$`: the size check caps the file at 41 bytes;
|
|
- the size check dropped: the pattern caps the length;
|
|
- the byte-length check dropped: the pattern is ASCII, so a match means
|
|
bytes equal characters; the check differs only if the file changes
|
|
between `lstat` and the read;
|
|
- the `SyntaxError` test dropped: the only non-`SyntaxError` is JSON
|
|
`null`, which then fails the raw pattern;
|
|
- `text.trim()` for `text.slice(0, 40)`: same result on every accepted
|
|
text;
|
|
- the `isSymbolicLink` test dropped: `lstat` of a symlink is never
|
|
`isFile`;
|
|
- `!== "base"` for `=== "cfg"`: the script calls only these two modes;
|
|
- `|| true` for `|| exit 3` on the `CFG` line: node prints nothing when it
|
|
refuses, so `[ -n "$CFG" ]` refuses;
|
|
- `[ -n "$CFG" ]` dropped: `|| exit 3` refuses first. Dropping
|
|
`|| exit 3` outright is the same, since `set -e` exits on the failed
|
|
assignment;
|
|
- `export CFG=…` for `CFG=…`: `export -n` on the next line clears it. In
|
|
round 2 this one was killed; the fix makes it equivalent.
|
|
|
|
I kept the size, byte-length and symlink checks anyway. Decision 37 asks
|
|
for size plus pattern, and the other two cost nothing.
|