fix(console): the /ui message editor gated on messages:edit alone (BACKLOG #324) - #203
Merged
Conversation
…BACKLOG #324) `GET /ui/messages/{id}/edit` and `POST /ui/messages/{id}/edit-resend` asserted `messages:edit` and nothing else, then called the JSON handler `get_message` as a plain function. Calling it that way SKIPS its own `Depends(require_phi_read( MESSAGES_VIEW_RAW))` gate, so the console stood in front of a `view_raw` route while re-asserting a different permission — and the skip is silent, because the bypassed gate never runs to disagree. `messages:edit` is not in `CUSTOM_ROLE_FORBIDDEN_PERMISSIONS`, so a custom role meaning "may resubmit, must not read" is mintable; on a deployed instance that role would have read the raw body here, exceeding its own stated scope. No shipped configuration reaches it — ADMINISTRATOR and OPERATOR both grant `view_raw` — and every such read was already audited, so this is an authorization gap, not an access gap. Both verbs now assert `messages:edit` AND `messages:view_raw` and fail closed on either. The POST is gated too, not just the GET: its `_reject` arm re-reads the origin and re-ships the PRISTINE stored body through `data_original`, so gating only the GET would have left the rejection path as an unauthorized read of exactly the body the GET refuses. `require_ui_step_up` gained a keyword-only `phi=` that forwards to `require_ui`'s existing `allow_phi_read` arm, so both verbs also charge the per-actor PHI-read budget (429 + Retry-After). It defaults False because most routes on this factory are admin writes that emit no message body. Custom-role minting is deliberately UNCHANGED per the owner ruling: no ADR 0045 D1 amendment, `messages:edit` stays mintable. Such a role simply cannot open an editor that displays the body it edits, which is the correct outcome. `ENGINE_UI_SEAM` is NOT bumped — the `_ui_seam.py` change is prose only; the seam's shape is unchanged. Tests: four in test_webui.py (refusal on both verbs asserting the synthetic needle and `data-original` are absent; a positive control that this is a tightening and not a lockout; the budget on each verb). `_MULTI_PERMISSION_ROUTES` in test_security_doc_drift.py gained both routes, and docs/SECURITY.md rides this commit because that guard reds otherwise. Falsified, not assumed: reverting the gate to the pre-#324 shape reds 3 of 4 (the positive control correctly still passes); removing ONLY `phi=` reds exactly the 2 budget tests; over-tightening the GET with a permission no custom role can hold reds the positive control; reverting the drift allow-list reds the guard naming exactly the two routes. All files restored byte-identically afterward (verified by blob hash). Scope note — three files sit outside this lane's assigned Owned list and are deliberate, not accidental: * docs/REMOTE-CONSOLE.md — its write-ops row stated `messages:edit` alone for a route that now needs two permissions; this change falsifies it. * CHANGELOG.md — entry filed under a new `### Security` section under [Unreleased] (this repo files access-control fixes there, per [0.3.1]/[0.3.0]), not `### Fixed`. * packaging/messagefoundry-webconsole/CHANGELOG.md — that distribution declares it documents all notable console changes, and two console routes gained a required permission and a 429 path. Reported, NOT fixed here (owner ruling / separate coordinator pass): * At least three further `require_ui_step_up` PHI routes still pass no `phi=` — GET /ui/messages/search, /ui/messages/search/layered, /ui/uploaded-logs/file/{id}. No backlog item existed for them at commit time, so no doc here claims they are tracked. * docs/SECURITY.md's PHI-egress census still says "nine more" console routes emit PHI and omits POST /ui/messages/{id}/edit-resend. That omission predates this change; the coordinator holds the SECURITY.md correction pass.
The console edit pair now gates on `messages:edit` AND `messages:view_raw` and charges the per-actor PHI-read budget, so #324's status line is flipped from "Filed — not started" to CLOSED. One line changed: the banner under `## 324.`, verified by number (the changed line sits under that heading, not merely near a banner that looks like it). The CENSUS WAS NOT RECOMPUTED. The four distribution lines and the ranked table are untouched by design — the coordinator is mid-pass filing several new items, and a second writer in the census or the table would collide. Whoever runs the next census pass picks this up. The banner deliberately does NOT claim the three still-unthrottled `require_ui_step_up` PHI routes are tracked: no item existed for them when this closed, so it says they were reported to the coordinator to file, and says "at least three" rather than asserting the set is exactly three.
wshallwshall
enabled auto-merge (squash)
August 5, 2026 05:37
wshallwshall
added a commit
that referenced
this pull request
Aug 5, 2026
…tion being a closed count (#204) ARCHIVE #324, closed by PR #203, moved verbatim. FILE THREE, all reported by lanes rather than fixed in them, and each re-verified against the code at e0482ae before its body was written. #1026 (6/3) -- the ASVS 12.1.1 TLS-floor probe's gate requires FOUR conditions while the comment above it names three and says "every other posture never reaches here". The undocumented fourth is `public_origin`, and the refusal for an unset `public_origin` is itself gated on `serve_ui` -- so with the console OFF a PHI instance behind a declared terminator under `enforce` starts with the probe silently inert. The same block `return 2`s when the probe's MECHANISM is unavailable, explicitly because a no-op "reports success forever afterwards": it refuses a silent no-op one level down and performs one one level up.⚠️ THE ORIGINATING REPORT'S MECHANISM WAS WRONG and the body says so. It claimed the console "auto-degrades" so `public_origin` stays unset; `public_origin` is an independent optional setting and `serve_web_console` maps to `api.serve_ui` -- neither derives the other. The conclusion held, the mechanism did not, and filing it as reported would have sent a fixer looking for a degradation path that does not exist. This is why it was held back from the previous ledger pass. #1025 (5/2) -- three `require_ui_step_up` routes emit PHI with no `phi=`, so `require_ui`'s `allow_phi_read` throttle never runs for them. A missing RATE LIMIT, not a missing authorization check; all three still gate on the right permission. Difficulty 2 is inherited: #324 already built the `phi=` seam. #1027 (5/3) -- `testpaths = ["tests"]` means the command CLAUDE.md:333 documents as the verification gate never collects the webconsole package. A FAILING test sat on main all day unseen because every quartet used the documented single path. Verified NOT a CI gap: ci.yml:250 installs the webauthn extra and runs the webconsole suite as a separate required step, so PRs merged on real coverage -- the gap is local, which is why nothing red ever reached anyone. Difficulty 3 because the naive fix (widen testpaths) makes that same failure the default local experience. STOP THE PHI-ROUTE LIST BEING A CLOSED COUNT. docs/SECURITY.md said "nine more" and omitted POST /ui/messages/{id}/edit-resend, whose `_reject` arm re-renders both the pristine core.get_message detail and the operator's raw_value. Per CLAUDE.md section 11 a completeness claim is a liability, so it is reworded to "at least ten more" with the derivation rule stated (the code is the authority; a route emits PHI if it renders a message body, and the ones charging the budget are those passing phi=True) rather than corrected to "ten" -- which the next PHI route would falsify again. Also corrected: docs/testing/master-test-plan/11-web-console.md:67 said 243 tests / 5389 lines. Measured now 247 / 5501. That citation was EXACTLY right before PR #203 and that PR made it false -- which is the useful distinction: a citation nobody has broken yet and a citation nobody has noticed is broken look identical in a grep, and only the change that breaks it can tell them apart. Census re-derived FROM the final table: 108 open, 108 rows, ranks contiguous, bijection empty both directions, all four lines summing to 108, 303 items across both files each declaring exactly one status. Frozen 2026-07-10 snapshot asserted byte-identical. NB the --min-items floor is left at 300 against a corpus of 303. Three items of slack, versus the 23 found yesterday. Raising it every pass would make each ledger commit touch ci.yml, since the floor is pinned in two places -- which is an argument for DERIVING it, not for raising it more often. Left for #1018.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Authored by the backlog-work session; rebased, verified by it, and independently re-verified by me before opening. P1 raw-PHI gate.
The fix
GET /ui/messages/{message_id}/editgated onmessages:editalone and rendered the full message detail. The catalogue's "impliesmessages:view_raw" is a built-in-role convention, not an enforced rule —messages:editis not inCUSTOM_ROLE_FORBIDDEN_PERMISSIONS, so a custom role meaning "may resubmit, must not read" could read raw PHI through that route.Per the owner's settle: gate only (no
CUSTOM_ROLE_FORBIDDEN_PERMISSIONSchange, no ADR 0045 amendment), andphi=threaded for this route only, with the other affected routes reported rather than fixed.What I verified rather than accepted
## N.heading above it: exactly 1 changed line at 2701, under heading 324, and its text is backlog: close #214 and #249 on owner-delegated rulings (declined, not shipped) #324's own banner. This is the check that matters after main's archiving pass rewrote 491 ledger lines — a byte-identical banner pasted onto a neighbour passes every well-formedness check.test_webauthn_rp_fail_closed_legiblefails because the worktree venv lacks the[webauthn]extra. Proven by a control arm, not by argument: detached to pristineorigin/mainin the same venv, none of the lane's commits present, identical single failure. CI installs[webauthn](ci.yml:250, with:245explaining it exists "so the passkey ceremony tests run realverify_*assertions"), so the required web-console step covers it.Quartet on the rebased tree: ruff clean, 1043 files formatted, mypy's 21 errors confined to the same four optional-extra modules with a passing control, and 1 failed / 10681 passed / 851 skipped with both pytest paths named —
testpaths = ["tests"]silently skips the webconsole package, and this lane editsroutes/core.pyand_auth.pydirectly.A false claim caught by its own author's verifier
The builder wrote a
require_ui_step_updocstring asserting the three unthrottled PHI routes "are tracked separately, not silently accepted" — while nothing tracked them and no item existed. Corrected before commit.That is CLAUDE.md §11's compensating-control-on-a-false-premise in its purest form: a gap correctly noticed, then described as though the noticing were the control. A reviewer reading only the diff would have seen a conscientious note.
Three out-of-brief edits, all blessed
docs/REMOTE-CONSOLE.md:240(its write-ops row saidmessages:editalone for a route that now needs two permissions — a document this change falsifies must move with it), plus oneCHANGELOG.mdentry and one webconsole packagingCHANGELOGbullet. All three named in the commit body, which is what makes them ratifiable rather than smuggled.Reported, not fixed — filed by me afterward
require_ui_step_uproutes emit PHI with nophi=, so they charge no per-actor read budget:GET /ui/messages/search,/ui/messages/search/layered,/ui/uploaded-logs/file/{file_id}.require_uideclaresphi: bool = Falseand throttles at_auth.py:260;require_ui_step_upbuilds its base withoutphi=, so the arm can never run. A missing rate limit, not a missing authorization check — those routes still gate on the right permission. Allocated #1025.docs/SECURITY.md's "nine more" PHI-route enumeration is short —POST /ui/messages/{id}/edit-resendalso emits PHI, because_rejectre-renders both the pristinecore.get_messagedetail and the operator'sraw_value. Being reworded away from a fixed count per §11 rather than corrected to "ten".docs/testing/master-test-plan/11-web-console.md:67(243 tests / 5389 lines) was exactly right before this lane and is now 247 / 5501.[webauthn]failure survived unseen. Allocated #1027.