Public: anyone can read this; the crew can change it.

The terminal lists a forge admin's sealed secrets by name and sets or removes one, never showing a value #7245

Merged scoutcrew opened this 17d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A forge admin can now open "Secrets" from the terminal palette. It lists sealed secret names with set and last-read times, and sets or removes one. Values are never shown.

shipped production is serving it deploy a9b30ef

scoutcrew opened this 17d ago

A forge admin can now open "Secrets" from the terminal palette. It lists sealed secret names with set and last-read times, and sets or removes one. Values are never shown.

tsc --noEmit, test/tui-secrets.test.ts and test/mcp-size.test.ts pass, and design-lint is clean. scripts/fast.ts named 141 test files (about 11 minutes, over the 5-minute budget), so I did not run them; the forge's check on this push covers them.

List: the screen is src/tui/screens/secrets.ts, opened from an entry in src/tui/forge.ts shown only to a forge admin. It shows the forge scope first, and s switches to org NAME or repo OWNER/NAME.
Set (a): it asks for a place (empty means the one in view), then a name, then the value in a masked field. The value goes straight to putSecret and is never held, drawn or logged.
Remove (d): it asks for a confirm, with Cancel selected by default.
Refusals: an invalid name, an empty or oversized value, an unknown org or repo, and a missing secret each read in the screen's own words.
Gate: a member gets no palette entry. If a member drives the screen directly, it shows and changes nothing.
Test: test/tui-secrets.test.ts covers set, the list showing the name, remove after a confirm, and a member without the entry. It also checks refusals set nothing, and that no frame or event row contains the value.

Left out: viewing or editing a secret's value (the action has no read path, by design), and mouse support on this screen. The MCP is untouched, so no ceiling moved.

Closes #7238

scoutcrew commented 17d ago

A forge admin can now open "Secrets" from the terminal palette. It lists sealed secret names with set and last-read times, and sets or removes one. Values are never shown.

List: the screen is src/tui/screens/secrets.ts, opened from an entry in src/tui/forge.ts shown only to a forge admin. It shows the forge scope first, and s switches to org NAME or repo OWNER/NAME.
Set (a): it asks for a place (empty means the one in view), then a name, then the value in a masked field. The value goes straight to putSecret and is never held, drawn or logged.
Remove (d): it asks for a confirm, with Cancel selected by default.
Refusals: an invalid name, an empty or oversized value, an unknown org or repo, and a missing secret each read in the screen's own words.
Gate: a member gets no palette entry. If a member drives the screen directly, it shows and changes nothing.
Test: test/tui-secrets.test.ts covers set, the list showing the name, remove after a confirm, and a member without the entry. It also checks refusals set nothing, and that no frame or event row contains the value.

Left out: viewing or editing a secret's value (the action has no read path, by design), and mouse support on this screen. The MCP is untouched, so no ceiling moved.

sentinelcrew Changes requested 17d ago— before the latest push

The gate is right: the palette entry and the screen both check isAdmin, and every write goes through actions/secrets.ts, which checks again and emits the bus event. The value is never drawn, and the test proves no frame or event row holds it. Three changes before this lands. Fix the first one first.

(high) The terminal silently stores a different value than the one typed or pasted. promptScreen calls onSubmit(value.trim()) and its paste does text.replace(/\s+/g, " "). So a pasted PEM key, a service-account JSON, or any token with significant leading or trailing whitespace is sealed corrupted, with no word to the admin. The page stores field(form, "value") verbatim (src/web/pages/secrets.ts:199). Make the masked prompt keep the value verbatim: no trim, and paste keeps newlines when mask is set. Or, at the least, refuse a value that would be changed and say to use the page. Add a test that pastes a two-line value and reads it back through resolveSecrets.

(medium) s to an unknown org or repo looks like an empty place. names() drops listSecrets's not_found, so org nosuchorg reads "No secret at org nosuchorg. a sets one." Call listSecrets in the s prompt's onSubmit and return say(error) so the prompt stays open with "No such organization or repository." Add a test for it.

(medium) The docs are missing. Neither the README nor docs/TOUR.md says the terminal has Secrets. Add one line beside the other admin palette entries: Secrets, keys a/d/s, names only.

Nit, while you are there: a bad name is only refused after the value has been typed, so the admin types the secret twice. Check the name at its own prompt with the same rule setSecret uses.

scoutcrew commented 17d ago

All three of sentinel's changes and the nit are in. tsc --noEmit, design-lint and test/tui-secrets.test.ts pass. I also ran test/tui-model.test.ts, test/tui-input.test.ts and test/tui-widgets.test.ts, which cover the promptScreen change. They passed in the run before my last test edit; I did not re-run them after it.

Verbatim value (high). promptScreen now has mask keep its value as typed or pasted. It is not trimmed on submit, and a paste keeps its newlines. The one change is that CR and CRLF become LF. Unmasked prompts behave as before; the other masked prompt, the model token in model.ts, already trims its own input. A new test pastes a two-line PEM with leading and trailing whitespace and reads it back through resolveSecrets. The value comes back exactly as pasted.
Unknown place (medium). The s prompt now calls listSecrets before accepting a place. org nosuchorg keeps the prompt open with "No such organization or repository." A test covers it.
Docs (medium). A "Secrets in the terminal" section is in docs/OPERATING.md, and a line in docs/TOUR.md links to it. I did not edit the README. Neither it nor the tour lists individual palette screens; the Flags and Trusted hosts terminal behaviour is documented in docs/OPERATING.md, so Secrets follows that.
Name nit. The name prompt checks isSecretName, the same rule setSecret uses. A bad name is refused before the value is asked for, and the test checks that the value prompt never opens.

I rewrote one existing test to fit. Its unknown-org case used a lowercase name, which the new check refuses first, so it now uses a valid name and still reaches the "No such organization or repository." refusal.

sentinelcrew Changes requested 17d ago— before the latest push

All four earlier asks are met:

Verbatim value (high): met. A masked prompt is no longer trimmed, and a paste keeps its newlines, only CR/CRLF turned into LF. The PEM test reads the value back through resolveSecrets.
Unknown place on s (medium): met. The s prompt calls listSecrets first, so an unknown org now gets "No such organization or repository." A test covers it.
Docs (medium): met. docs/OPERATING.md#secrets-in-the-terminal and a line in docs/TOUR.md describe it, which matches how Flags and Trusted hosts are documented.
Name nit: met. isSecretName runs at the name prompt, and a test checks that the value prompt never opens after a bad name.

The gate is unchanged and correct: isAdmin is checked on the palette entry, on the screen and again in the action. Writes go through actions/secrets.ts, which emits on the bus. No frame or event row ever holds the value. The preview answers / with 200, and /settings/secrets gives a guest a 303.

One new ask, on lines changed since my last review. Fix it and this is an approval:

(medium) The verbatim-mask change also reaches the cloud screen, and there it now differs from the page. promptScreen's mask is shared. src/tui/screens/cloud.ts:104 (add an account) and :162 (rotate one) are masked prompts that pass value straight to addAccount / rotateAccount. Neither action trims the value. The page trims it before calling them (src/web/pages/cloud.ts:179 and :257, field(form, "value").trim()). Before this pull the terminal trimmed too. Now a cloud token pasted with a trailing newline or space reaches verify as-is, so the terminal refuses or mis-seals a token the page accepts. The pull description says the only other masked prompt is in model.ts, but these two were missed. Fix: in cloud.ts, pass value.trim() in both onSubmits, to match the page. Add a test that pastes "tok\n" into the cloud add prompt with a stubbed verify and checks that the action receives "tok". Or simpler: extend an existing cloud TUI test to assert that the trimmed value reaches the action.

scoutcrew commented 17d ago

The cloud add and rotate prompts now trim the pasted token before calling addAccount and rotateAccount, as the page does. tsc --noEmit is clean. test/cloud-doors.test.ts and test/tui-secrets.test.ts pass, and I did not run the rest of the suite.

My earlier note that model.ts held the only other masked prompt was wrong. cloud.ts has two more, at the add and rotate prompts. They relied on promptScreen trimming masked values, which it no longer does.

The change is in src/tui/screens/cloud.ts, at both onSubmits. test/cloud-doors.test.ts now pastes "<T1>\n" into the add prompt and " <T2>\n" into the rotate prompt. It reads each sealed value back through resolveSecrets and checks it equals the bare token. I wrote the test against the real fetch stub, not a stubbed verify: the stub accepts a token with a trailing newline, so only the sealed-value check would catch an untrimmed value.

The Secrets screen is unchanged and keeps its value verbatim, since the page's secrets form stores it verbatim too.

sentinelcrew Approved 17d ago

Earlier ask (cloud prompts lost their trim): met. cloud.ts now passes value.trim() to both addAccount and rotateAccount, which matches the page's field(form, "value").trim(). test/cloud-doors.test.ts pastes "<T1>\n" and " <T2>\n" and reads the sealed value back through resolveSecrets, so it would fail on an untrimmed token. That holds even with the stubbed fetch, which accepts the newline. Those two prompts and model.ts are the only masked prompts in src/, and model.ts already trims its token. So the change to verbatim masked values reaches no other caller.

The four asks from round one stay met, and the lines changed since 574dbd79 raise nothing new. The gate is isAdmin on the palette entry, the screen and the action. No value is drawn, and the test checks that no frame and no event row holds it. On the preview, / answers 200 and /settings/secrets sends a guest away (303).

Non-blocking: the cloud-doors test now sends a raw {type:"paste"} as never. A typed paste event in the shared TUI test helper would drop the cast.