fix(#470): make the C8 plugin-UI host actually work — same-origin frame, scoped ids, emitted CSS (C8b) - #793
Merged
Merged
Conversation
Three defects found while porting the first real plugin SPA (byte5ai/omadia-dev-platform#3). Each failed silently rather than loudly, which is why C8 shipped green. 1. The iframe sandbox made every authenticated call cross-origin. `allow-same-origin` was withheld deliberately, so the document got an opaque origin: `fetch('/bot-api/...')` left with `Origin: null`, the `SameSite=Lax` session cookie was never attached, `EventSource` failed the same way and `localStorage` threw. A data-driven plugin UI opens every screen with a GET, so the host rendered a correctly-styled, correctly- themed shell showing an error state on all four screens. Neither repo's suite caught it: both stub `fetch`, and a stub has no origin. Decision: sandboxed but SAME-ORIGIN — `allow-same-origin allow-scripts allow-forms`. The sandbox bought no privilege it did not already have, as the plugin's server half runs in-process with the operator's authority; what confines the bundle is the response, not the attribute, and that CSP (`default-src 'none'`, `script-src 'self'`, `connect-src 'self'`, `frame-ancestors 'self'`, `base-uri 'none'`, `form-action 'none'`) is unchanged. `allow-popups` is dropped — nothing uses it. Rejected: a postMessage/CORS proxy (`Access-Control-Allow-Origin: null` matches every opaque origin, and the alternative loosens the session cookie to `SameSite=None` product-wide) and a separate origin (the clean answer, recorded as the upgrade path; needs a second hostname and TLS on every install target). Reasoning in plan.md §4.3a addendum. 2. The host page rejected every scoped plugin id. Its regex claimed to mirror `manifestLoader.ts` and omitted the optional `@scope/`, so every `@omadia/*` id — which is all of them — hit `notFound()`. The gate now lives in `web-ui/app/_lib/pluginId.ts` and a test reads `manifestLoader.ts` and asserts both the pattern and the 214-char cap are character-identical, turning the "mirrors" comment into a check. Traversal safety is asserted, not assumed. 3. `border`, `divide-*` and `transition*` emitted no CSS at all. `@source inline()` expands BRACES; a top-level comma is not a list separator, so `@source inline("border,border-{0,2,4}")` asked Tailwind for a class literally named `border,border-0` and produced nothing — while `plugin-ui-vocabulary.md` listed all three groups as available. Worse than a missing utility: Tailwind's reset is `border: 0 solid`, so `class="border border-border"` set a colour on a zero-width edge and rendered invisible. Fixed at source, artifact regenerated (69,559 → 72,659 B raw, 12,105 → 12,486 B gzip). `hover:`/`focus:` on `bg-transparent` were promised by the doc and not emitted either; the source line now declares them. Tests, all of which fail against the parent commit: - `pluginUiVocabulary.test.ts` checks parity in both directions — every class the source declares and every class the document promises must exist in the committed sheet. It refuses un-expandable notation rather than skipping it, so a promise cannot go unchecked; the doc's `xs…7xl` ellipsis is written out and the Responsive prose became a table for the same reason. - `PluginUiFrame.test.tsx` pins the sandbox attribute to its exact string and the src to core's own origin. - `pluginUiFrameCredentials.test.ts` proves the posture the decision rests on: the session cookie is `SameSite=Lax; Path=/; HttpOnly` and not `SameSite=None`; a same-origin request replaying it authenticates while the same request without it 401s. Core-decoupling ratchet held at 3300 — the tests use a neutral `@omadia/ example-ui` id rather than naming the package being extracted.
…remises under test The sandbox change in this branch is right and stays. Its stated reasoning was not, and a wrong rationale next to a security attribute is the exact defect class this PR exists to fix. Three corrections, all narrative plus tests — no behaviour change: 1. "It gives the plugin no privilege it does not already hold" was false. The server-side plugin contract is genuinely deny-by-default: pluginServiceGrants throws ServiceNotDeclaredError for undeclared services, pluginContext gates ctx.http / ctx.net / ctx.secrets / ctx.memory / ctx.llm / ctx.subAgent / ctx.knowledgeGraph / ctx.mcp / ctx.events.emit / ctx.flows on manifest permissions, operatorAuthAccessor exposes a verifier rather than a bearer, and CORE_RESERVED_ROOTS refuses /api/v1/admin to a plugin even on operator consent. A same-origin bundle riding the Path=/ admin session reaches all of it, and no CSRF layer exists to stop it. The decision is still correct, for a narrower reason now written down: plugin server code is loaded by a bare in-process dynamic import with no vm, worker, child process or permission wall, sharing globalThis and a process.env that holds the session signing key. Against a malicious plugin author the grant model was never a security boundary — it is a consent-and-contract seam — so withholding same-origin from the UI defended nothing while breaking every honest plugin. The residual cost is a different attacker: a compromised browser-only dependency or an XSS in the plugin UI, which now inherits the operator's admin surface. §4.3a says so plainly instead of claiming the trade is free. 2. "The sandbox is retained for what it still denies" was unenforceable. With allow-same-origin plus allow-scripts a bundle reaches window.frameElement, strips the attribute and reloads, regaining top-level navigation, downloads and modals. The attribute is an intent marker, not isolation, and the test that pins those tokens now says which of the two it is. 3. "ingest-scanned ZIP" overstated the scan. The only ingest pass over a UI bundle reads .js / .mjs for arbitrary Tailwind values, never opens ui/index.html, is bounded at 200 files / 8 MB, and does not run at all for built-in or local-dev catalog packages. Inline script, javascript: URLs and <base> are blocked by the runtime CSP, not by ingest. New middleware/test/pluginUiTrustModel.test.ts is the tripwire for the premise itself: it pins the bare in-process plugin load, keeps /api/v1/admin core-reserved with the accepted exposure recorded next to it, exercises the real ServiceNotDeclaredError gate, and pins the session JWT's role: 'admin' plus Path=/. Each failure message names the decision to reopen rather than the line that changed. A documented trade with no test is just a comment. pluginUiStaticServing.test.ts now table-drives every CONTENT_TYPES entry — Content-Type, nosniff, a non-empty CSP, the SVG sandboxing policy versus the standard one for the other ten extensions — plus the 304 branch. CSP was already branch-free in the handler but asserted on only three of eleven servable types. CONTENT_TYPES is exported so the table cannot drift from what it claims to cover. Verified: middleware plugin/auth/manifest surface 438/438; web-ui 778/778 across 91 files; web-ui typecheck, build and lint (0 errors, 47 pre-existing warnings); plugin-ui css drift up to date at 72,659 bytes; typecheck:test ratchet held at 406; core-decoupling ratchet held at 3300. Mutation-checked: removing /api/v1/admin from CORE_RESERVED_ROOTS, replacing the dynamic import with a vm-based load, and flipping role: 'admin' each redden one tripwire; deleting the CSP res.set reddens 15 of the table-driven cases.
Weegy
enabled auto-merge (squash)
August 20, 2026 17:58
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
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.
Fixes three defects in the C8 plugin-UI host (#784), all found while porting the first real plugin SPA in byte5ai/omadia-dev-platform#3. Each fails silently rather than loudly, which is why C8 shipped green.
Part of epic #470, capability gap G7.
1. The sandbox made every authenticated call cross-origin
PluginUiFrame.tsxsetsandbox="allow-scripts allow-forms allow-popups"—allow-same-originwithheld deliberately, to keep "third-party code" out of the operator's cookies, with the note that a plugin needing authenticated calls does them from its own backend router.The first half was sound. The second half does not follow. A sandbox without
allow-same-origingives the document an opaque origin, and the plugin's own backend router is still reached over HTTP from inside that document:fetch('/bot-api/v1/...')leaves withOrigin: nulland is a cross-site request;SameSite=Lax, so it is not attached;EventSource(url, { withCredentials: true })— the live job-event tail — fails identically;localStoragethrows outright.The ported SPA is entirely data-driven: all four screens open with a
GET. So the host page rendered a correctly-styled, correctly-themed, correctly-translated shell showing an error state on every screen. Neither repo's suite caught it — both stubfetch, and a stub has no origin. This is a property of the browser, not of the client.Decision: sandboxed but same-origin
Three arguments, in order of weight:
ctx.services, its own router and the operator's authority. A plugin that wanted the operator's session could read it server-side today. Withholding it from the plugin's UI removed function, not capability.default-src 'none'; script-src 'self'; connect-src 'self'; form-action 'none'; base-uri 'none'; frame-ancestors 'self', served from an extension allowlist with no.cssin it.allow-popupsis dropped — nothing in a plugin UI opens a window, and a granted capability nothing uses is only a surface.The familiar objection ("
allow-scripts+allow-same-originlets the frame remove its own sandbox") is true and, here, empty: the frame is already same-origin and already has the session. There is nothing left to escalate to. The sandbox is retained for what it still denies — top-level navigation, downloads, modals, pointer lock, presentation.The alternatives, and why not
Access-Control-Allow-Origin: nullmatches every opaque origin on the internet — a weaker boundary than the one it replaces. The other route to the same place,SameSite=None; Secure, weakens authentication product-wide to serve one iframe.Written up in
specs/470-dev-platform-plugin/plan.md§4.3a addendum.2. The host page rejected every scoped plugin id
It did not mirror it.
manifestLoader.tsblesses an optional@scope/, and every omadia plugin id is scoped — so/plugin-ui/@omadia/...callednotFound()on the only id shape those packages can have. The nav entry a plugin registers landed on a 404. The comment was the whole enforcement, and a comment cannot fail.The gate now lives in
web-ui/app/_lib/pluginId.tsand covers both halves of the middleware rule (214-char cap, then charset). It is not imported from middleware — web-ui is a separate Next build that depends on no middleware package, and the epic's constraint 2 pushes dependencies the other way — so instead it is pinned by test:pluginId.test.tsreadsmanifestLoader.tsand asserts the pattern and the cap are character-identical. Drift now fails a test instead of a screen. A re-introduced local regex inpage.tsxfails too.Path-traversal safety is asserted rather than assumed: a valid id's only
/is the scope separator and no segment may begin with., so..is structurally unreachable — covered by tests, alongside the percent-encoded round-trip (%40omadia%2F…→ decoded by Next → accepted).3.
border,divide-*andtransition*emitted nothing@source inline()expands braces. A top-level comma is not a list separator, soasked Tailwind for classes literally named
border,border-0— and produced zero CSS. Three declarations were written that way; the neighbours use the empty-alternative brace form (rounded{,-none,-sm}), which is what made it invisible on review. Meanwhileplugin-ui-vocabulary.mdlisted all three groups as available, so the document and the sheet disagreed.Worse than a missing utility: Tailwind's reset is
border: 0 solid, soclass="border border-border"— the most common pairing in the ported pages, 27 occurrences — set a colour on a zero-width border and rendered invisible. No error, no warning, nothing in any build. Exactly the silent-unstyled failure the no-arbitrary-values contract exists to prevent, sitting inside the artifact that enforces it.A fourth disagreement surfaced from the new parity test: the doc promises
hover:/focus:onbg-transparent, and the source declared it without variants. The source line now declares them.Artifact regenerated: 69,559 → 72,659 B raw, 12,105 → 12,486 B gzip (+381 B), 9,208 → 9,556 B brotli.
Tests — every one of them fails against the parent commit
web-ui/scripts/__tests__/pluginUiVocabulary.test.ts— parity in both directions: every class the source declares, and every class the document promises, must exist in the committed sheet. The source→artifact half is generic: it catches any declaration that silently emits nothing, not just this comma bug. The expander refuses notation it cannot expand rather than skipping it, so a promise cannot go unchecked — which is why the doc'sxs…7xlellipsis is now written out and the Responsive prose became a table.PluginUiFrame.test.tsx— pins the sandbox to its exact string (so both droppingallow-same-originagain and re-addingallow-popupsgo red) and pins thesrcto core's own origin, since same-origin is a property of the URL as much as of the attribute.middleware/test/pluginUiFrameCredentials.test.ts— proves the posture the decision rests on: the session cookie isSameSite=Lax; Path=/; HttpOnlyand notSameSite=None; a same-origin request replaying it authenticates, while the same request without it 401s (mutation guard); and the served document still carriesconnect-src 'self',frame-ancestors 'self',base-uri 'none',form-action 'none'.pluginId.test.ts— the pin tomanifestLoader.ts, the scoped/unscoped/FQDN cases, the traversal shapes, the 214-char boundary, the encode/decode round-trip.vitest.config.tsnow includesscripts/**so the artifact-parity test can live next to the source it guards;vitest.setup.tsgains amatchMediastub (jsdom declares the property but leaves it uncallable, so'matchMedia' in windowwas true and the value was not a function).Verification
web-uilintweb-uitypecheckweb-uivitestweb-uibuildplugin-ui:css:check(drift)web-uii18n:checkmiddlewarebuildmiddlewaretypecheckmiddlewaretypecheck:testmiddlewarelintmiddlewaretests (auth + plugin + manifest + plugin-ui)scripts/check-core-decoupling.mjsThe decoupling ratchet earned its keep here: the first draft of these tests hardcoded the extracted plugin's own id and pushed the count 3300 → 3318. They now use a neutral
@omadia/example-ui, which tests the same property — "a scoped id resolves" — without core naming the package being extracted.What is still not verified
Rendering in a real browser. That row stayed open in the plugin repo's own honesty table and it stays open here: this PR removes the two core-side blockers, but only a real browser against a real host page closes it. That belongs with the plugin PR once this lands.
Cross-family review (Forge)
Adversarial review by an OpenAI-family model (GPT-5.4,
reasoning_effort=high) against theAnthropic-family work in this PR, to surface shared-lineage blind spots. Scope: the four questions
the frame change raises — same-origin reach, CSP/ingest confinement, encoded-slash routing, and
whether the new tests kill their mutants.
Verdict: the code is right; the rationale was wrong. Nothing in the behaviour of this PR is
reverted. Three of its load-bearing security claims were false or overstated, and the corrections
plus their tripwires are pushed onto this branch.
Findings
pluginServiceGrants.tsthrowsServiceNotDeclaredError;pluginContext.tsgatesctx.http/ctx.net/ctx.secrets/ctx.memory/ctx.llm/ctx.subAgent/ctx.knowledgeGraph/ctx.mcp/ctx.events.emit/ctx.flowson manifest permissions;operatorAuthAccessorexposes a verifier, not a bearer; andCORE_RESERVED_ROOTSrefuses/api/v1/adminto a plugin even with operator consent. A same-origin bundle riding thePath=/session (JWT hardcodesrole: 'admin') reaches all of it, with no CSRF layer anywhere in the product to stop it.allow-same-origin+allow-scriptsthe bundle reacheswindow.frameElement, stripssandboxand reloads, regaining every one of them..js/.mjsonly —ui/index.htmlis never opened — is bounded at 200 files / 8 MB, and does not run at all for built-in/local-dev catalog packages. Inline<script>,onclick=,javascript:and<base>are blocked by the runtime CSP, not by ingest.nosniffon 1.CONTENT_TYPESentry + the 304 branch%2Finside a segment survives Next intact andmiddlewareProxy.tsre-encodes it byte-identically, so@omadia/example-uireaches Express as one segment. The plugin id is a catalogMaplookup, never a filesystem join; asset paths go throughsafeRelativePath+ realpath containment.%40omadia%2F..%2F..%2Fetc%2Fpasswdand..%2F..%2Fmanifest.yamlboth dead-end. No traversal..border{}from the artifact reddens 3 tests; a doc promising a non-existent class reddens the doc↔artifact check; restoring the top-level comma in the source reddens the source↔artifact check; dropping@scope/from the pattern reddens 4; addingallow-popupsand droppingallow-same-origineach redden 2. Shrinking the doc is deliberately unguarded — the source→artifact direction and the pinned-by-name test cover the three regression groups.localStorage/sessionStorage, andomadia_sessionisHttpOnly— the frame can ride the cookie but not read it. The delta is capability, not secrets. One further undocumented delta:allow-same-originmakeswindow.top.documentwritable (UI-redress inside the real operator chrome).The honest trust model, as now recorded
The same-origin grant is defensible not because the plugin already had that authority, but because
plugin server code is loaded by a bare in-process dynamic import (
toolPluginRuntime.ts) with novm,worker, child process or permission wall — sharing
globalThisand aprocess.envthat holds thesession signing key. Against a malicious plugin author the grant model was never a security
boundary; it is a consent-and-contract seam, and such an author can forge a session outright.
Withholding same-origin from the UI defended nothing against that actor while breaking every honest
plugin.
The residual cost is a different attacker: a compromised browser-only dependency or an XSS in the
plugin UI, which controls the bundle but not the server half. That attacker previously got an opaque
origin and nothing; it now inherits the operator's admin surface. That is the trade this PR makes, and
§4.3a now says so in those words rather than claiming the trade is free.
Because that reasoning rests on a premise that a future hardening pass is explicitly planned to
invalidate (
pluginContext.tsrecords the intent to sandbox plugins), the premise itself is nowunder test:
middleware/test/pluginUiTrustModel.test.tsfails the day plugin server code becomesisolated, and tells the reader to reopen the §4.3a decision and move plugin UIs to a distinct origin.
A documented trade with no test is just a comment.
Verification of the review commit
web-uivitestweb-uitypecheck · buildweb-uilintplugin-ui:css:checkmiddlewarelintmiddleware typecheck:testratchetcheck-core-decoupling.mjsMutation evidence for the new suites — each mutant reddens the test that claims to guard it:
'/api/v1/admin'fromCORE_RESERVED_ROOTSawait import(...)with anode:vmloadrole: 'viewer'res.set('Content-Security-Policy', responseCsp)The PR's own suites were mutation-checked too, before any change: deleting
.border{}from theartifact reddens 3; a doc promising a class the sheet lacks reddens the doc↔artifact check;
restoring the top-level comma in the source reddens the source↔artifact check; dropping
@scope/from the pattern reddens 4; adding
allow-popupsand droppingallow-same-originredden 2 each.7/7 killed.
What remains unverified
Unchanged from the original honesty note: rendering in a real browser. What this review did close
is the routing half of that risk — the
%2Fround-trip was probed end-to-end against a real Next 16server, so the scoped-id fix is known to reach Express intact rather than assumed to.
Verdict: MERGE.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.