fix(webconsole): /ui path-segment encoding and fetch-metadata on the static mount (BACKLOG #1370, #1371) - #636
Open
wshallwshall wants to merge 2 commits into
Open
fix(webconsole): /ui path-segment encoding and fetch-metadata on the static mount (BACKLOG #1370, #1371)#636wshallwshall wants to merge 2 commits into
wshallwshall wants to merge 2 commits into
Conversation
added 2 commits
August 27, 2026 03:41
…t (BACKLOG #1370)
Connection names and channel ids are interpolated into `/ui` URL paths. They are
unconstrained free text -- the registry checks only for a duplicate and no charset gate
exists -- so a name carrying `/` splits into two path segments and addresses a different
route.
MAIN WAS IN TWO DIFFERENT STATES, AND THE HALF-PROTECTED ONE IS THE MORE INTERESTING:
connections.py:64 quote(r.name) PARTIAL -- quote defaults to safe="/"
connections.py:200 quote(name) PARTIAL -- same
admin.py:363,381 {role.id} RAW
messages.py:618,628 {ch}/{dest} RAW
A BARE quote() READS AS PROTECTION AND PROVIDES NONE AGAINST THE ONE CHARACTER THAT MATTERS.
Measured rather than reasoned: quote("IB/ACME") returns it UNCHANGED, while quote("a?b") and
quote("a#b") are both encoded -- so the call looks correct at every site anyone checks by
hand. `safe=""` is the whole fix.
`_seg()` lands in pages/_common.py and the six sites route through it.
NOT A BLANKET SWEEP, AND THE EXCLUSION IS THE LOAD-BEARING PART. `_auth`'s re-auth `next`
carries a whole PATH inside a QUERY parameter, where safe="/" is CORRECT; routing it through
_seg would break every re-auth redirect. connections.py:59 and :384 keep a bare quote() for
the same reason -- they build `?channel_id=` query values, not path segments. A sweep of
"every quote() call" would ship a broken login, so a test pins the re-auth encoding against
exactly that shape.
THE SEVEN TESTS COME FROM THE ABANDONED BRANCH AND PASS UNMODIFIED AGAINST THIS
RE-IMPLEMENTATION. They were written for a different implementation of the same contract, so
they corroborate rather than restate -- the strongest evidence available that this rebuild
matches what the dead lane intended, without cherry-picking a branch 31 commits behind whose
_security.py has moved underneath it.
Five mutants, ALL KILLED, EACH BY A DISTINCT RED SET: _seg reverted to the default safe (3
red); _seg over-encoding, which fires the benign-name negative control (2 red); one site
reverted to a raw interpolation (2 red); ONE SITE REVERTED TO A BARE quote(), caught by the
structural scan ALONE (1 red); the custom-role site reverted (2 red). Every mutant asserted a
unique anchor and a changed hash before scoring; every restore byte-identical.
Verified: ruff check + format clean; mypy clean over 33 files; the FULL webconsole suite at
385 passed, 3 skipped -- not just the new file.
SCOPE: this is gap 2 of the two the Lander scoped on PR 530. The sibling -- fetch-metadata
does not cover the /ui/static mount, `_is_ui_fetch_scope` absent from main -- is NOT in this
commit and still wants building.
Per the dispatch: zero deployments, so this is a condition a FIRST deployment would meet
rather than an exposure anyone has today, and whatever lands from it should not be armed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ACKLOG #1371)
Main DOES check fetch-metadata -- `_auth`'s per-route helper refuses any request whose
`Sec-Fetch-Site` says cross-site, and it is used across five modules. But `/ui/static` is a
Starlette Mount, not an APIRoute, so NO ROUTE DEPENDENCY EVER RUNS FOR IT. The asset tier is
the one /ui surface that check cannot reach, and the console looks covered precisely because
the helper exists everywhere else.
Middleware is the only tier that sees a Mount. `UiFetchMetadataMiddleware` plus
`_is_ui_fetch_scope`, deliberately WIDER than the existing `_is_ui_html_path` -- that
predicate excludes /ui/static correctly, because CSP headers only apply to HTML. Collapsing
the two would remove this check's only purpose, and a mutant that does exactly that is in the
proof below.
THREE CARVE-OUTS, EACH OF WHICH LOOKS LIKE A WEAKNESS AND IS NOT:
* A cross-site top-level NAVIGATION passes. An intranet link into the console is one; so is
the OIDC callback, cross-site by construction. Without reading Sec-Fetch-Mode as well,
EVERY REAL SSO LOGIN WOULD 403 WHILE EVERY HERMETIC TEST STILL PASSED. Method is part of
safe -- a cross-site navigation carrying a POST is a CSRF submission -- and object/embed
are refused because that is framing, not navigation.
* An ABSENT header passes. Sec-Fetch-Site is browser-populated; old browsers, reporting
agents and every non-browser client omit it. Failing closed would refuse the shipped
Windows tray's own GET /ui probe, which builds its client with no headers at all.
* 403, NEVER 404. The tray classifies 404 as DISABLED and every other status as ENABLED, so
a "do not disclose the route" pass would make a healthy console report as switched off.
THE EIGHT TESTS COME FROM THE ABANDONED BRANCH AND PASS UNMODIFIED AGAINST THIS
RE-IMPLEMENTATION -- written for a different implementation of the same contract, so they
corroborate rather than restate.
Five mutants, ALL KILLED, EACH BY A DISTINCT RED SET, and every one is a change someone would
plausibly propose as an improvement: narrow the scope to the HTML predicate (2 red); fail
closed on an absent header (1 red); return 404 rather than disclose the route (4 red); drop
the navigation carve-out (1 red); let object/embed keep it (1 red). Mutant C initially scored
ANCHOR 0 because ruff had rewrapped the line after I wrote the anchor -- the guard refused to
score rather than report a false kill, which is the whole reason it asserts uniqueness first.
Verified: ruff check + format clean; mypy clean over 33 files; FULL webconsole suite 393
passed, 3 skipped -- 385 before these 8, so the arithmetic confirms they ran and nothing
regressed.
SCOPE: this is gap 1 of the two the Lander scoped on PR 530; #1370 in the previous commit is
gap 2. Both are now built. PR 530's own branch stays superseded and should not be
cherry-picked -- it is 31 commits behind and _security.py has moved underneath it.
Per the dispatch: zero deployments, so this is a first-deployment condition rather than a live
exposure, and whatever lands from it should not be armed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wshallwshall
force-pushed
the
claude/builder-2-530-security-gaps
branch
from
August 27, 2026 08:44
0617fbd to
b97db98
Compare
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.
The two real gaps behind the stranded PR 530, re-implemented against current main
rather than cherry-picked. PR 530's own branch stays superseded and must not be
cherry-picked.
#1370 -- operator-supplied names escape their /ui path segment. A bare
quote()READS AS PROTECTION AND PROVIDES NONE against the one character that matters:
quote("IB/ACME")returns it unchanged whilequote("a?b")is encoded, so everycall site looks right when checked by hand. Two sites were half-protected that way
and four were raw.
safe=""is the fix.The exclusion is load-bearing:
_auth's re-authnextcarries a PATH inside aQUERY parameter, so
safe="/"is correct there. A sweep of "every quote() call"WOULD SHIP A BROKEN LOGIN. Two connections.py sites keep a bare
quote()for thesame reason -- they build
?channel_id=values. A test pins it.#1371 -- fetch-metadata never reached the /ui/static mount. Main DOES check
fetch-metadata per-route across five modules, which is exactly why it looks covered.
/ui/staticis a Mount, so no dependency ever runs for it, and middleware is theonly tier that sees a Mount.
Three carve-outs that each look like a weakness and are not, each pinned by a test:
a cross-site top-level NAVIGATION passes (without it every real SSO login 403s while
every hermetic test passes); an ABSENT header passes (the shipped tray probe sends
none); and it returns 403 and never 404 (the tray reads 404 as DISABLED).
Verification worth knowing: the dead branch's own tests pass unmodified against both
rebuilds -- 7 for #1370, 8 for #1371. They were written for a DIFFERENT implementation
of the same contract, so they corroborate rather than restate. Ten mutants across the
two, all killed, each by a distinct red set. Full webconsole suite 393 passed, 3
skipped, up from 385 before these 15 tests. ruff and mypy clean.
Zero engine files. Both are first-deployment conditions under CLAUDE.md section 0,
not live exposures.
NOT ARMED and should not be: this is a browser-security path, and the builder that
wrote it recommended the same hold.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com