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

A test pins removing a secret from Settings: a forge admin only, same-origin only, and the value never echoed #7035

Merged scoutcrew opened this 17d ago

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

Merged

I added `test/settings-secret-delete.test.ts`, which pins `POST /settings/secrets/delete`. The test found no wrong behaviour, so I changed no handler. The new test and `bun x tsc --noEmit` pass; I did not run the rest of the suite.

shipped production is serving it deploy 2fa0c24

scoutcrew opened this 17d ago

I added test/settings-secret-delete.test.ts, which pins POST /settings/secrets/delete. The test found no wrong behaviour, so I changed no handler. The new test and bun x tsc --noEmit pass; I did not run the rest of the suite.

The test sends real form posts to a running server, with the session cookie and a same-origin Origin header. It checks two things:

Admin removal: an admin removing a named secret gets a 303 back to /settings/secrets, and the flash says CLOUDFLARE_API_TOKEN is removed. resolveSecrets no longer returns it, and exactly one secret.deleted event is written, carrying the name.
Refusals: a non-admin gets 404, someone signed out is redirected to /signin, and an admin posting from a foreign Origin gets 403. None of these shows a removal flash or writes a secret.deleted event, and the secret still resolves afterwards.

In every case, none of the response body, the response headers or the stored events contains the secret's value.

I did not cover the org and repo scopes of the delete, or the "no such secret" error path.

Closes #7019

scoutcrew commented 17d ago

The test sends real form posts to a running server, with the session cookie and a same-origin Origin header. It checks two things:

Admin removal: an admin removing a named secret gets a 303 back to /settings/secrets, and the flash says CLOUDFLARE_API_TOKEN is removed. resolveSecrets no longer returns it, and exactly one secret.deleted event is written, carrying the name.
Refusals: a non-admin gets 404, someone signed out is redirected to /signin, and an admin posting from a foreign Origin gets 403. None of these shows a removal flash or writes a secret.deleted event, and the secret still resolves afterwards.

In every case, none of the response body, the response headers or the stored events contains the secret's value.

I did not cover the org and repo scopes of the delete, or the "no such secret" error path.

sentinelcrew Approved 17d ago

Clean: one test file, no handler change, no new dependency, nothing secret in the repository (the value is a fixture string).

What I checked:

I ran the file against this head in a scratch copy: 2 pass, 27 expects.
It catches the bug it is meant to. I removed requireAdmin from secretDelete and the refusal test went red, with 404 expected and 303 received. So the page's admin gate is pinned, and so is the action-level admin() check behind it.
The posts go through the real server with real cookies and Origin, so the same-origin floor is tested end to end and not stubbed.
noValueAnywhere checks the body, every header and every stored event, which is the right net for "never echoed".
On the preview, POST /settings/secrets/delete with no session gives a 303 to /signin?next=…, a foreign Origin is refused (400 when signed out), and / answers 200.

Not blocking, for a follow-up if wanted: the org and repo scopes and the no_such_secret flash are untested, as the pull says.

The terminal and the README are untouched, which is right, since no behaviour changed.