fix(tiers 4–5): an XSS in the attribute name, x shot broken on every route, and a config page describing a framework we do not ship - #294
Merged
Conversation
…and 40 fictional config keys
Three more breaking changes for 8.0.0, two security fixes, and a documentation
page that described a framework this repo does not ship.
BREAKING — three config fields deleted: pwa.installPrompt, auth.afterSignInPath,
ai.modelEnv. Each declared, defaulted, merged, and read by nothing. modelEnv's
own doc comment already argued for its deletion: "an intention, not a behaviour
... So the exact thing this key exists to prevent — a model string baked into
the image — is what actually happens."
BREAKING — @ultimat3/manifest drops `canonical`; @ultimat3/render drops
`matchRoute` and `RouteMatch` (two exported matchers with different precedence;
http's trie is the live one and render's had zero consumers).
Security, two holes in one function. `attributePair` tested
`name.startsWith('on')` CASE-SENSITIVELY while the lines beside it folded case,
so ONERROR="alert(1)" went out on the wire; and the attribute NAME was never
validated at all, so an object key could carry a whole second attribute out of
the quotes. Both reachable by `<div {...row} />` over a JSON body or a JSONB
column. `head.ts`'s renderTag had the identical hole and now shares the
predicate — one predicate, never two.
`x shot` reported failure on every route of every app. `pageOverTarget` spread
the frame and overrode goto/screenshot but not `url`, and the frame seeds
`lastUrl` at construction — so `ScrapePage.url()` answered `about:blank` before
a navigation, after it, and after reading text. The CLI compares that as
`finalUrl`. One line. No test had ever called `ScrapePage.url()`, which is why
it shipped.
A fired wedge watchdog never quit a remote browser: shutdown() returned early on
the same flag watch() sets when it fires. On remoteBrowser() — "the PRIMARY
production path" — `browser.process()` is null, so nothing ended the remote
session. New terminal code X_SCRAPE_WATCHDOG_STOPPED covers the adjacent case
where an injected clock's sleep() rejects and leaves the run unwatched; a
separate code from X_SCRAPE_WEDGED because that one means the page stopped
answering and would send a reader to investigate a page that is fine.
The third and fourth copies of the canonical serialiser are gone, per
canonical-json.ts's own "never add a fourth copy". manifest's fed buildId AND
the contract-diff equality, so a -0/NaN/Date fold could make a breaking API
change diff as "no change" and ship silently. Verified: two contracts differing
only by -0 vs 0 hashed identically before and differ now. A FIFTH copy remains
in scripts/lib/framework-manifest.ts, routed to the gate-scripts slice.
wiki/Configuration.md documented roughly 40 config fields that exist on no
declaration — auth.providers, auth.session.*, auth.passkeys, jobs.retry.*,
jobs.retention.*, cache.redis.*, all eight seo.*, all seven budgets.*,
storage.*, otel.* — and its top-level app.config.ts example did not compile
(TS2353 on database.urlEnv, verified by extracting the fence and running tsc).
The page is now derived from the interfaces and the example compiles. The
documented session cookie name was wrong too: 'x_session' where the real one is
'__Host-x_session', whose prefix is the session-fixation defence. No checker
compiles a wiki fence — routed to the gate-scripts slice.
Also: two divergent formatBytes copies unified in core (render's had no `mb`
branch, so a 5 MiB route read 5120kb); mail's memory driver stamped
`at: new Date()`, the one unseamed clock in the package; mcp recompiled a RegExp
per validation; seven more instanceof-on-a-caught-value sites in ai and mail;
@ultimat3/testing created a temp dir in every test process that imported its
barrel, for `expect` alone, and removed none; two JSX probes kept separate depth
counters over one globalThis.React.
Refs docs/plans/2026/08/21/101-deep-dive-sweep-two/{05-tier4-render-pwa-mail-manifest-ai,06-tier5-scraping-testing}.md
Refs #293
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0135KMN4Tfq1xhMwts1FNvis
Both tracked apps set `pwa.installPrompt`, and the social demo also set `auth.afterSignInPath`. Neither key was ever read by anything, so removing them changes no behaviour — but leaving them would fail excess-property checking against the narrowed interfaces the parent commit ships, which is the whole point of deleting a declaration rather than documenting it as dead. `examples/dummy` also carried `ai.modelEnv: 'ANTHROPIC_MODEL'`, which selected no model. It is gone with the field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0135KMN4Tfq1xhMwts1FNvis
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Limit details: You’ve used the included review currently available. Your 71 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (82)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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.
Fourth of eight PRs executing
docs/plans/2026/08/21/101-deep-dive-sweep-two— slices 05 and 06, plus decision D3. Builds on #288, #291, #292. Four agents, disjoint sets, one checkout.Carries 3 more of the breaking changes batched for 8.0.0 (now 6 total, up from the plan's 4).
Security: two ways to inject a handler through an attribute NAME
{ ONERROR: 'alert(1)' }ONERROR="alert(1)"— the handler test wasname.startsWith('on'), case-sensitive, while the two lines beside it folded case{ 'q onmouseover=alert(1) r': 'ok' }q onmouseover=alert(1) r="ok"— the attribute name was never validated at all, so a key carries a second attribute out of the quotesNames now match
/^[A-Za-z_:][-A-Za-z0-9_:.]*$/and the handler test folds case. Both are reachable by spreading a JSON body or a JSONB column into JSX.head.ts'srenderTaghad the identical hole — found beyond the brief, in the same package — emitting<meta name="q" r onmouseover=alert(1) s="ok">from anattrskey. It now shares the predicate. One predicate, never two.x shotreported failure on every route of every apppageOverTargetspread the frame and overrodegoto/screenshotbut noturl, and the frame seedslastUrlat construction. SoScrapePage.url()answeredabout:blankbefore a navigation, after it, and after reading text — and the CLI compares it asfinalUrl, so every run exited non-zero with "redirected to about:blank".One line. No test anywhere had ever called
ScrapePage.url(), which is exactly why it shipped. The new parity test covers fake, fixture and the CDP code path.The config page documented a framework that does not exist
wiki/Configuration.md— the wiki is this project's only public documentation surface — documented roughly 40 config fields that exist on no declaration:auth.providers,auth.session.*,auth.passkeys,auth.trustedOrigins,jobs.retry.*,jobs.retention.*,cache.redis.*, all eightseo.*, all sevenbudgets.*,storage.driver/bucket/dir,otel.endpoint/sampling.Its top-level
app.config.tsexample did not compile — proven, not asserted: the fence was extracted verbatim and run throughtscagainst the repo's own config.This framework's stated primary developer is an agent, and an agent copies the example. The page is now derived from the interfaces; capabilities that genuinely do not exist are stated as gaps rather than documented as features. The documented session cookie name was also wrong —
'x_session'where the real one is'__Host-x_session', and that prefix is the session-fixation defence.Breaking changes
pwa.installPrompt,auth.afterSignInPath,ai.modelEnvdeleted@ultimat3/manifestdropscanonicalcanonicalJsonfrom@ultimat3/core@ultimat3/renderdropsmatchRoute,RouteMatch@ultimat3/http's trie is the live matcher; render's had zero consumersai.modelEnv's own doc comment argued for its deletion:Also fixed
shutdown()returned early on the same flagwatch()sets when it fires. OnremoteBrowser(), which the driver calls "the PRIMARY production path",browser.process()isnull, so nothing ended the remote session. Measured{ quits: 0, kills: 1 }.X_SCRAPE_WATCHDOG_STOPPED(new, terminal) — an injected clock whosesleep()rejects killed the guard's loop and left the run unwatched. Separate fromX_SCRAPE_WEDGEDdeliberately: that one means the page stopped answering and would send a reader to debug a page that is fine. Terminal where its sibling is retryable, because attempt 2 reaches the same clock identically — retrying buys five browser launches that die at the first poll, and on an authenticated target five arrivals at a login.manifest's fedbuildIdand the contract-diff equality, so a-0/NaN/Datefold could make a breaking API change diff as "no change" and ship silently. Two contracts differing only by-0vs0hashed identically before; they differ now and the diff reports it.buildIdunchanged for both tracked apps.formatBytes(render's had nombbranch:5120kbvs5mb) · mail's memory driver stampedat: new Date()· mcp recompiled aRegExpper validation · seven moreinstanceof-on-a-caught-value sites ·@ultimat3/testingcreated a temp dir in every test process importing its barrel, even forexpectalone, and removed none · two JSX probes over oneglobalThis.React.Escaped rather than absorbed
cache.tiersaccepts'memo' | 'shared' | 'isr'and@ultimat3/cachehas none of those names;'isr'appears in the package only as an error label. Two vocabularies for one set of rungs, which is the exact defectrender-modesexists to catch.scripts/lib/framework-manifest.ts, hashing the framework's own manifest → routed to the gate-scripts slice.scripts/error-render.tscannot see thecatchbinding class at all — proven: green before and after a seven-site fix. Fifteen sites across five packages have been fixed by hand this sweep and no gate found one.readme-fences.tsscanspackages/*/README.mdonly. That is why the config example rotted silently.Gate
bun run verify→ 14 of 19 passed, 5 skipped, exit 0.One red first, and an interesting one: the XSS example's attack string begins
x, sodoc-commands.tsparsed it as an invocation of thexCLI and reportedX_DOC_COMMAND_UNKNOWN. An allowance would have been the wrong fix — that entry means "this sentence is about a command not existing" — so the example key was renamed instead. A real false-positive class that only appears when documentation quotes hostile input.🤖 Generated with Claude Code
https://claude.ai/code/session_0135KMN4Tfq1xhMwts1FNvis
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.