diff --git a/.github/workflows/security-audit.yml b/.github/workflows/security-audit.yml index f3f03c9c5d..ecdd9794d9 100644 --- a/.github/workflows/security-audit.yml +++ b/.github/workflows/security-audit.yml @@ -1103,13 +1103,9 @@ jobs: BASE_SHA="${{ github.event.pull_request.base.sha }}" git show "$BASE_SHA:studio/frontend/package-lock.json" \ > /tmp/base-package-lock.json - # Pull the TRUSTED allowlist from the base ref so a PR cannot - # allowlist its own new postinstall dependency in the same diff - # the checker scans. If the file does NOT exist on base, REMOVE - # the temp file -- the checker treats a missing base allowlist - # as bootstrap mode (the PR that introduces the file is allowed - # to populate it; once it lands, subsequent PRs must respect - # the head-only rejection rule). + # Pull TRUSTED allowlist from base so a PR cannot self-approve + # a new postinstall in the same diff. Missing-on-base = bootstrap + # mode (gate accepts head allowlist for THAT PR only). if ! git show "$BASE_SHA:studio/frontend/.install-script-allowlist" \ > /tmp/base-install-script-allowlist 2>/dev/null; then rm -f /tmp/base-install-script-allowlist diff --git a/.github/workflows/studio-frontend-ci.yml b/.github/workflows/studio-frontend-ci.yml index f22b5909c8..515dffcd2a 100644 --- a/.github/workflows/studio-frontend-ci.yml +++ b/.github/workflows/studio-frontend-ci.yml @@ -110,10 +110,8 @@ jobs: run: npm run typecheck - name: Frontend unit tests (vitest) - # New vitest suite covers the HtmlSvgRenderer iframe sandbox / - # CSP / sanitizer contract. Run it before the build so a - # sanitizer regression fails the gate even if the bundle still - # builds clean. + # Covers HtmlSvgRenderer sandbox / CSP / sanitizer. Runs before + # build so a regression fails the gate even if the bundle is clean. run: npm run test - name: Build diff --git a/scripts/check_new_install_scripts.py b/scripts/check_new_install_scripts.py index b216d70068..1779bf4ba4 100644 --- a/scripts/check_new_install_scripts.py +++ b/scripts/check_new_install_scripts.py @@ -4,32 +4,21 @@ """Diff two `package-lock.json` files and flag NEW install-script deps. -A package with `"hasInstallScript": true` runs `preinstall` / `install` / -`postinstall` lifecycle hooks every time `npm ci` lays it down. Every -npm supply-chain compromise of the last 18 months (Shai-Hulud, -TanStack, axios-style, ArmorCode hijacks) leveraged exactly this lever: -the attacker publishes a new malicious version of a dep we already -trust, and the post-install hook runs the next time CI installs. +Packages with `hasInstallScript: true` run preinstall/install/postinstall +on every `npm ci`. Every npm supply-chain compromise of the last 18 months +(Shai-Hulud, TanStack, axios, ArmorCode) used this lever: malicious +version of a trusted dep, hook runs on next install. -This scanner refuses to allow a newly-introduced install-script dep to -land without a maintainer eyeball on the lifecycle script body. -Existing install-script deps are NOT re-flagged -- if `node-gyp` has -been in the lockfile since day one, it's not part of this PR's threat -model. Only new entries are surfaced. +Existing install-script deps are NOT re-flagged; only NEW entries surface. -Supports lockfileVersion 1 (`dependencies` key, recursive), 2 and 3 -(flat `packages` key with `node_modules//node_modules/` nesting -for transitive entries). For each NEW install-script package we -attempt a stdlib-only fetch of -`https://registry.npmjs.org//` to recover the actual -postinstall command body. If the network is blocked we still emit the -finding -- the lifecycle command body is informational, not -load-bearing. +Supports lockfileVersion 1 (`dependencies`, recursive) and 2/3 (flat +`packages` with `node_modules//node_modules/` nesting). For each +finding we try a stdlib fetch of `https://registry.npmjs.org//` +to recover the lifecycle body; network failure is non-fatal. -Exit codes -========== +Exit codes: 0 no newly-added install-script deps - 1 one or more newly-added install-script deps; listed on stderr + 1 newly-added install-script deps listed on stderr 2 internal error (missing lockfile, malformed JSON, etc.) """ @@ -76,15 +65,11 @@ class Finding: def _strip_nm_prefix(key: str) -> str: - """Convert a v2/v3 `packages` key into a bare package name. - - `node_modules/foo` -> `foo`; `node_modules/foo/node_modules/bar` -> - `bar`. The empty key (`""`) is the project root and returns "". - """ + """v2/v3 `packages` key -> bare leaf name. ``""`` is the project root.""" if not key: return "" - # Use the LAST `node_modules/` segment so transitives map to their - # leaf name, matching how npm install resolves a postinstall. + # Last `node_modules/` segment: transitives map to leaf name (matches + # how npm resolves the postinstall). marker = "node_modules/" idx = key.rfind(marker) if idx == -1: @@ -97,10 +82,8 @@ def _collect_install_script_entries(lock: dict) -> dict[str, str]: every entry with `hasInstallScript: true` (v2/v3) OR a non-empty `scripts.preinstall|install|postinstall` (v1). - The same package may appear at multiple versions in a single - lockfile (de-duplicated copies under different parents); we key by - `name@version` so we don't lose either copy. Returns a dict keyed - by `name@version` -> the same string for convenience. + Keyed by `name@version` so duplicate copies at different versions + are not collapsed. Returns {`name@version`: name}. """ seen: dict[str, str] = {} version = lock.get("lockfileVersion") @@ -120,10 +103,8 @@ def _collect_install_script_entries(lock: dict) -> dict[str, str]: ver = entry.get("version") or "" seen[f"{name}@{ver}"] = name - # v1 also embeds a `dependencies` tree; v2/v3 carry both for - # backwards-compat but `packages` is canonical for them. For v1 - # there is no `hasInstallScript` flag, so look for a non-empty - # `scripts.preinstall|install|postinstall` directly. + # v1 has no `hasInstallScript` flag -- detect via non-empty + # scripts.{preinstall,install,postinstall}. def _walk_v1(deps: dict, depth: int = 0) -> None: if depth > 64 or not isinstance(deps, dict): return @@ -135,8 +116,6 @@ def _collect_install_script_entries(lock: dict) -> dict[str, str]: isinstance(scripts, dict) and scripts.get(hook) for hook in ("preinstall", "install", "postinstall") ) - # v1 also sets `requires` only on the parent, no flag, so - # the lifecycle-script presence is the only signal. if lifecycle: ver = entry.get("version") or "" seen[f"{name}@{ver}"] = name @@ -163,12 +142,9 @@ def _load_lockfile(path: Path) -> dict: def _fetch_registry_scripts(name: str, version: str) -> dict[str, str] | None: - """Return {hook: command} for any of preinstall / install / - postinstall published in the registry metadata for this name@ver. + """Return {hook: command} from registry metadata, or None on any failure. - Returns None on any error (network blocked, 404, malformed JSON). - Never raises; the caller treats absence as "could not enrich, emit - finding anyway". + Never raises; absence means "emit finding without enrichment". """ safe_name = urllib.parse.quote(name, safe = "@/") url = f"{REGISTRY_BASE}{safe_name}/{urllib.parse.quote(version)}" @@ -203,9 +179,8 @@ def diff_new_install_scripts(base_lock: dict, head_lock: dict) -> list[Finding]: findings: list[Finding] = [] for key in sorted(head): if key in base: - continue # pre-existing install-script dep; not in scope + continue # pre-existing dep, out of scope name = head[key] - # key is "name@version"; rsplit("@", 1) handles scoped names. version = ( key[len(name) + 1 :] if key.startswith(name + "@") else "" ) @@ -215,8 +190,7 @@ def diff_new_install_scripts(base_lock: dict, head_lock: dict) -> list[Finding]: else: detail = ( "newly added with hasInstallScript=true; registry " - "metadata unreachable -- inspect the package's " - "scripts.{preinstall,install,postinstall} manually" + "unreachable -- inspect scripts.{preinstall,install,postinstall}" ) findings.append( Finding( @@ -236,14 +210,12 @@ def diff_new_install_scripts(base_lock: dict, head_lock: dict) -> list[Finding]: def _load_allowlist(path: Path) -> set[str]: - """Read a file of newline-separated ``name@version`` entries to skip. + """Newline-separated `name@version` entries to skip. - Each entry MUST be pinned to an exact version (``esbuild@0.21.5``, - ``@scope/pkg@1.2.3``). Bare names are rejected so allowlisting - ``esbuild`` cannot silently approve a later malicious - ``esbuild@99.0.0`` published by a compromised maintainer -- every - new version requires its own review. Lines starting with ``#`` are - comments; blank lines are ignored. Missing file = empty allowlist. + Each entry MUST be pinned to an exact version; bare names rejected so + a compromised maintainer cannot ship a malicious later version under + an existing allowlist line. `#` comments and blank lines ignored. + Missing file = empty set. """ if not path.exists(): return set() @@ -301,9 +273,8 @@ def main(argv: list[str] | None = None) -> int: default = None, help = ( "Path to the TRUSTED BASE allowlist. Defaults to " - "'/.install-script-allowlist'. Entries that exist " - "only on HEAD fail the gate so a PR cannot allowlist its " - "own new postinstall dependency." + "'/.install-script-allowlist'. Head-only entries that " + "self-approve a new postinstall fail the gate." ), ) args = parser.parse_args(argv) @@ -335,13 +306,9 @@ def main(argv: list[str] | None = None) -> int: raw_findings = diff_new_install_scripts(base_lock, head_lock) raw_findings_keys = {_finding_allowlist_key(f) for f in raw_findings} - # Bootstrap: if the BASE ref has no allowlist file at all, this is - # the PR that creates it. There is no prior allowlist to diff - # against, and refusing every head entry here would make the gate - # unlandable. The workflow signals "missing on base" by NOT writing - # the temp file (rm -f after a failed ``git show``), which is what - # we detect here. Once the file exists on base, future PRs must - # respect the head-only / deletion rules below. + # Bootstrap: PR that creates the file. Workflow signals "missing on base" + # by `rm -f` after a failed `git show`. After landing, head-only / delete + # rules below kick in. if not base_allowlist_path.exists(): print( f"[install-script-diff] bootstrap: {base_allowlist_path} " @@ -357,13 +324,9 @@ def main(argv: list[str] | None = None) -> int: return 2 added_head_only = head_allowlist - base_allowlist - # Refuse the bypass shape where a PR introduces a new postinstall - # dep AND allowlists it in the same diff -- head-only allowlist - # entries that match install-script findings in the same PR are - # rejected. Entries that match no current finding are allowed: - # they prepare the trust list for a follow-up PR that actually - # adds the dependency, and the human-review path still sees the - # allowlist diff before that follow-up can land. + # Refuse "introduce a new postinstall AND allowlist it in the same + # diff". Head-only entries that match no current finding are fine + # (prepare trust for a follow-up PR; reviewer still sees the diff). self_approving = sorted(added_head_only & raw_findings_keys) if self_approving: print( @@ -380,16 +343,9 @@ def main(argv: list[str] | None = None) -> int: ) return 1 - # Refuse a PR that DROPS trusted base entries. Without this, - # an attacker could land a two-step bypass: - # 1. PR A removes ``.install-script-allowlist`` from base - # (no new lockfile findings -> passes today). - # 2. PR B then hits the bootstrap path (base allowlist - # missing) and self-allowlists a newly introduced - # install-script dependency. - # Allowlist deletions are rare; doing them via an - # admin-override / branch-protection bypass keeps the - # bootstrap path closed for the everyday gate. + # Refuse PRs that DROP trusted base entries; otherwise a two-step + # bypass works (PR A removes file -> PR B hits bootstrap and + # self-allowlists). Deletions go via admin override. removed_from_head = sorted(base_allowlist - head_allowlist) if removed_from_head: print( @@ -406,10 +362,8 @@ def main(argv: list[str] | None = None) -> int: ) return 1 - # Only the trusted base allowlist participates in the skip set. - # Head-only entries that survived the self-approval check above - # are deliberately NOT honored here -- they wait for the next - # PR (running against this PR's merged base) to take effect. + # Only base allowlist applies. Head-only entries that survived + # the self-approval check wait for the next PR to take effect. allowlist = base_allowlist findings = raw_findings @@ -439,10 +393,9 @@ def main(argv: list[str] | None = None) -> int: print(str(f), file = sys.stderr) print(file = sys.stderr) print( - "[install-script-diff] Refusing to proceed. Every new " - "install-script dep is a postinstall lifecycle hook that " - "would run on the next `npm ci`. Review each finding above, " - "confirm the maintainer + version, and re-run.", + "[install-script-diff] Refusing to proceed. Each new install-script " + "dep ships a postinstall hook that runs on next `npm ci`. Review, " + "confirm maintainer + version, and re-run.", file = sys.stderr, ) return 1 diff --git a/studio/backend/main.py b/studio/backend/main.py index 1d23d04a4a..6f01064c01 100644 --- a/studio/backend/main.py +++ b/studio/backend/main.py @@ -330,14 +330,9 @@ def _build_csp(script_nonce: "str | None" = None) -> str: "style-src 'self' 'unsafe-inline'; " f"{script_src}; " "font-src 'self' data:; " - # Restrict iframe sources to same-origin only. SVG previews still - # use a sandboxed ``srcdoc`` iframe (no URL fetch); interactive - # HTML previews go through the same-origin ``/api/preview/html/{id}`` - # route in routes/html_preview.py, which serves the snippet with - # its own overriding ``script-src 'unsafe-inline'`` response CSP. - # Without the same-origin route, Chromium would inherit THIS - # ``script-src 'self'`` for srcdoc / data: / blob: iframes per - # HTML / CSP3 and inline scripts would be silently dead. + # Same-origin only. SVG uses srcdoc; interactive HTML goes through + # /api/preview/html/{id} which carries its own overriding response + # CSP (srcdoc / data: / blob: inherit THIS one per CSP3). "frame-src 'self'; " "frame-ancestors 'none'; " "form-action 'self'; " diff --git a/studio/backend/routes/html_preview.py b/studio/backend/routes/html_preview.py index f30064c4c3..5d4ba8c1fa 100644 --- a/studio/backend/routes/html_preview.py +++ b/studio/backend/routes/html_preview.py @@ -3,34 +3,21 @@ """HTML preview route for assistant ```html fences. -Reason this route exists: when the chat renderer embeds the assistant's -HTML via a ``srcdoc`` iframe, Chromium inherits the embedder CSP -(``script-src 'self'``), so inline scripts and ``onclick`` handlers are -silently blocked. Serving the same HTML from a same-origin URL with an -overriding response-header CSP is the only way to let assistant-generated -interactive HTML actually run while keeping the surrounding Studio CSP -strict. +srcdoc / data: / blob: iframes inherit the host CSP (script-src 'self') +per CSP3, so inline scripts and onclick handlers are silently blocked. +Serving the snippet from a same-origin URL with an overriding response +CSP is the only way to unlock interactive HTML. -Security shape: - -* POST is auth-gated (``get_current_subject``). Only an authenticated - caller can stash HTML into the in-memory store. -* GET is intentionally NOT auth-gated -- browsers do not attach the - Authorization bearer to iframe subresource loads, so we instead make - the URL itself the secret: ``secrets.token_urlsafe(24)`` (192 bits of - entropy). The token leaves the server only in the POST response and - is then placed into the iframe ``src`` by the requesting page. It is - never persisted to disk, never logged, and is wiped on TTL expiry. -* The response CSP is ``default-src 'none'`` + ``script-src - 'unsafe-inline'`` so the preview is sandboxed from the network but - inline scripts and event-handler attributes execute as intended. -* The iframe still has ``sandbox="allow-scripts allow-modals - allow-popups"`` (no ``allow-same-origin``), so even though the URL - is same-origin the document is treated as a unique opaque origin - for SOP purposes -- script in the preview cannot reach - ``window.parent`` storage, cookies, or DOM. -* A size cap and TTL cap bound the in-memory footprint per Studio - process. +Security: +- POST is auth-gated. +- GET is NOT auth-gated -- iframe subresource loads do not carry the + Authorization bearer, so the URL token (192 bits, secrets.token_urlsafe(24)) + is the authorisation. Never persisted, wiped on TTL expiry. +- Response CSP: default-src 'none' + script-src 'unsafe-inline'. + Scripts run; network egress (connect-src, frame-src, worker-src) blocked. +- iframe sandbox stays "allow-scripts allow-modals allow-popups" with NO + allow-same-origin, so preview JS cannot reach window.parent. +- Size and live-entry caps bound per-process memory. """ from __future__ import annotations @@ -44,8 +31,7 @@ from fastapi import APIRouter, Depends, HTTPException from fastapi.responses import HTMLResponse from pydantic import BaseModel, Field -# Backend root on sys.path so ``auth`` imports resolve when this module -# is loaded standalone (matches the pattern used by routes/export.py). +# Backend on sys.path for standalone load (matches routes/export.py). _BACKEND_ROOT = Path(__file__).resolve().parents[1] if str(_BACKEND_ROOT) not in sys.path: sys.path.insert(0, str(_BACKEND_ROOT)) @@ -55,30 +41,14 @@ from auth.authentication import get_current_subject # noqa: E402 router = APIRouter() -# --------------------------------------------------------------------------- # Knobs (module-level so tests can monkeypatch). -# --------------------------------------------------------------------------- - -# 1 MiB cap; assistant HTML previews are short snippets, not full SPAs. -MAX_HTML_PREVIEW_BYTES = 1_000_000 - -# 10 minutes. Long enough for a user to interact with the preview, short -# enough that a forgotten tab does not pin the entry. -PREVIEW_TTL_SECONDS = 10 * 60 - -# Defensive cap so a runaway producer cannot exhaust the worker. New POSTs -# evict the oldest entries past this watermark. Per-process, in-memory only. -MAX_LIVE_PREVIEWS = 256 - +MAX_HTML_PREVIEW_BYTES = 1_000_000 # 1 MiB; snippets, not full SPAs. +PREVIEW_TTL_SECONDS = 10 * 60 # 10 min interaction window. +MAX_LIVE_PREVIEWS = 256 # Per-process cap; oldest evicted first. _PREVIEWS: dict[str, tuple[float, str]] = {} -# --------------------------------------------------------------------------- -# Internal helpers. -# --------------------------------------------------------------------------- - - def _sweep_expired(now: float | None = None) -> None: now = time.monotonic() if now is None else now expired = [k for k, (t, _) in _PREVIEWS.items() if now - t > PREVIEW_TTL_SECONDS] @@ -89,31 +59,24 @@ def _sweep_expired(now: float | None = None) -> None: def _evict_overflow() -> None: if len(_PREVIEWS) <= MAX_LIVE_PREVIEWS: return - # Evict oldest first. sorted_keys = sorted(_PREVIEWS, key = lambda k: _PREVIEWS[k][0]) for k in sorted_keys[: len(_PREVIEWS) - MAX_LIVE_PREVIEWS]: _PREVIEWS.pop(k, None) def _build_html_doc(source: str) -> str: - # ```` mirrors the srcdoc fallback so any ```` - # without an explicit target opens in a new tab rather than navigating - # the iframe (which would be UX-confusing). + # so links open a new tab instead of + # navigating the iframe away from the preview. return "" '' + source _PREVIEW_CSP = "; ".join( ( "default-src 'none'", - # ``script-src 'unsafe-inline'`` enables BOTH ``'; -// Fallback for when the same-origin preview route is unreachable (offline, -// 404, transport error). srcdoc inherits the host page's ``script-src -// 'self'`` per HTML / CSP3, so this path is static-layout-only -- inline -// ``