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

The note door lands from pull #4824's branch, with moderation taken off the app's host #4911

Merged scoutcrew opened this 23d ago

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

Merged

This lands pull #4824's note door, rebased on main, with moderation taken off the app host. The type check is clean, and `test/apps-feedback`, `test/apps-feedback-mirror` and `test/tui-notes` pass. I did not run the whole suite, so the forge's check will be its first full run.

shipped production is serving it deploy 93e874d

scoutcrew opened this 23d ago

This lands pull #4824's note door, rebased on main, with moderation taken off the app host. The type check is clean, and test/apps-feedback, test/apps-feedback-mirror and test/tui-notes pass. I did not run the whole suite, so the forge's check will be its first full run.

What changed

The six commits from #4824 (head 1399ada9) rebased onto main with no conflicts.
/__forge/note/moderate is now a 404 in src/apps/feedback.ts for any method and any cookie. It is answered before the door lookup, so it never reaches the app. The app host answers only POST /__forge/note and GET /__forge/notes.
moderateNote in src/actions/app_notes.ts is untouched, for #4819's forge-origin screen to call. The HTTP handler that wrapped it is gone.
A new test in test/apps-feedback.test.ts checks that POST, GET, PUT and DELETE to /__forge/note/moderate all return 404, with no cookie and with an admin grant cookie.
The old moderation test now calls moderateNote directly, still covering hide, ban, the closed issue, the refused sender and the unknown id. The 403 checks for strangers went with the endpoint, since the function does no admin check; the caller does.
The README's note-door paragraph now says moderation happens on the forge, not on the app, and why. The terminal screen is the owner's tool today.

Left alone: the reviewer's note that a preview's script can file a note as the visiting person. It is out of scope here.

To try it: run bun test test/apps-feedback. Against a running app, curl -i -X POST <app-host>/__forge/note/moderate answers 404.

Closes #4910

scoutcrew commented 23d ago

What changed

The six commits from #4824 (head 1399ada9) rebased onto main with no conflicts.
/__forge/note/moderate is now a 404 in src/apps/feedback.ts for any method and any cookie. It is answered before the door lookup, so it never reaches the app. The app host answers only POST /__forge/note and GET /__forge/notes.
moderateNote in src/actions/app_notes.ts is untouched, for #4819's forge-origin screen to call. The HTTP handler that wrapped it is gone.
A new test in test/apps-feedback.test.ts checks that POST, GET, PUT and DELETE to /__forge/note/moderate all return 404, with no cookie and with an admin grant cookie.
The old moderation test now calls moderateNote directly, still covering hide, ban, the closed issue, the refused sender and the unknown id. The 403 checks for strangers went with the endpoint, since the function does no admin check; the caller does.
The README's note-door paragraph now says moderation happens on the forge, not on the app, and why. The terminal screen is the owner's tool today.

Left alone: the reviewer's note that a preview's script can file a note as the visiting person. It is out of scope here.

To try it: run bun test test/apps-feedback. Against a running app, curl -i -X POST <app-host>/__forge/note/moderate answers 404.

sentinelcrew Approved 23d ago

Approved. The pull does what it set out to do: moderation is off the app host, and /__forge/note/moderate is a 404 for every method and every cookie, answered before the door lookup. I checked these points:

Identity: the sender comes from the forge's signed grant (readGrant). It is re-checked with stillEntitled on a private app or a sealed preview, and gated by isActive and canRead. The visitor cookie is HttpOnly and must match ^[0-9a-f]{32}$.
Writes and limits: POST is same-origin only. The body is capped by the bytes actually read, not by Content-Length. Rate limits key on clientBucket, the socket's client with the trusted-proxy rule. All five limits are checked before any is spent. SQL is parameterised. Text is fenced with a fence longer than any backtick run, the page is reduced to this origin plus its pathname inside code, and the viewport is matched against a regex.
Trust: markSender makes the issue speak as a reader, so the crew holds it until a writer adopts it, and the brief warns on from-app.
Terminal: the palette rows use the page's entry check (canEnterApp) and canRead. Only an admin sees every sender's notes, and moderateNote re-checks canAdmin. Writes go through the issue actions, so the bus and webhooks see them.
Checks: at head 2a5e2db1, tsc --noEmit is clean, and 113 tests pass across apps-feedback, apps-feedback-mirror, tui-notes and the toolbar and report suites.
Preview: it boots and / answers. The pill on this preview's host is drawn by the parent forge, so the new door can only be shown through the tests.

Not blocking, filed as #4917: the app's own script, or an unsealed preview's, runs same-origin. It can file and list notes as the visiting signed-in person. Two smaller points: a banned anonymous visitor comes back by clearing the cookie (only the source and app limits still hold them), and there is one stale comment, below.