From c803a364da80364c6670fb64becc809bdc3b69d5 Mon Sep 17 00:00:00 2001 From: coozgan Date: Thu, 20 Aug 2026 20:53:44 +0800 Subject: [PATCH] fix(cloud): build and inject source-bound frontend, fail closed on missing dist The cloud launch flow builds the stock frontend from the exact filtered source archive in an isolated temporary root and appends only its admitted byte snapshot. A residual gitignored checkout bundle is never read. Every build or admission failure -- including an npm that resolves but cannot be executed, whose spawn error would otherwise escape as a traceback -- leaves the original archive unchanged so the EC2 box uses its required npm-build fallback. install.sh skips the on-box npm build only when the shipped bundle is COMPLETE (index.html plus every /assets/ chunk it references, the same completeness signal frontend._incomplete_bundle_reason uses) and fails closed when no frontend can be produced; a torn or partial staging falls through to the npm rebuild. The CloudFormation health check rejects the dashboard-less HTTP 200 guidance page, and cloud connect probes the forwarded dashboard before opening it. Dist admission scans text assets for credential-shaped content. The generic bare-secret entropy heuristic is exempted only for a data-URI payload that actually DECODES to a recognized media container, because inlined media is the one shape built assets false-positive on. Keying that exemption on content rather than position matters: `data:image/png;base64,` must not become a laundering prefix for a raw key. Every other bare-secret run is actionable -- including runs longer than 40 characters, which is what a genuine key glued to adjacent base64 characters looks like and precisely what security._contains_bare_secret exists to catch. Distinctive and encoded-credential matches stay actionable everywhere. Baselined files (cli_cloud.py, cloud/ec2.py, frontend.py, test_frontend_dist_resolve.py) keep their pre-existing formatting so the black baseline is untouched and the PR carries no .github/ change. Original author: Joshyfruit (coozgan). Co-authored-by: Bolin Chen Co-authored-by: Claude Co-authored-by: Kiro Crew --- docs/guides/cloud-instance-ssm-vs-ssh.md | 3 + docs/guides/remote-crew-on-ec2.md | 95 +++ docs/system-specs/modules/cloud.md | 22 +- install.sh | 35 +- src/kiro_crew/cli_cloud.py | 23 +- src/kiro_crew/cli_server.py | 3 +- src/kiro_crew/cloud/ec2.py | 5 + src/kiro_crew/cloud/source.py | 354 +++++++++++- .../cloud/templates/kirocrew-ec2.yaml | 22 +- src/kiro_crew/frontend.py | 129 ++++- src/kiro_crew/security_posture.py | 4 + test/test_cloud_cli.py | 64 +++ test/test_cloud_ec2.py | 9 + test/test_cloud_source.py | 544 +++++++++++++++++- test/test_frontend_dist_resolve.py | 45 ++ test/test_spawn_audit.py | 10 +- 16 files changed, 1313 insertions(+), 54 deletions(-) diff --git a/docs/guides/cloud-instance-ssm-vs-ssh.md b/docs/guides/cloud-instance-ssm-vs-ssh.md index 47f8f006503..9b92f0e78a4 100644 --- a/docs/guides/cloud-instance-ssm-vs-ssh.md +++ b/docs/guides/cloud-instance-ssm-vs-ssh.md @@ -11,6 +11,9 @@ older SSH-over-`ProxyCommand` workaround. > inbound port, and no `~/.ssh/config` edits**. Reachability is an IAM decision > (grant/revoke `ssm:StartSession`, CloudTrail-audited), not a networking decision > gated by a key someone holds. +> +> New here? [remote-crew-on-ec2.md](remote-crew-on-ec2.md#deploy-your-first-kiro-crew-on-ec2-step-by-step) +> is the step-by-step first-launch walkthrough; this page is the transport deep-dive. ## The two transports diff --git a/docs/guides/remote-crew-on-ec2.md b/docs/guides/remote-crew-on-ec2.md index 6aeb1810c6e..bf96911c213 100644 --- a/docs/guides/remote-crew-on-ec2.md +++ b/docs/guides/remote-crew-on-ec2.md @@ -24,6 +24,63 @@ plus the EC2-specific gotchas people actually hit (from `kirocrew doctor`). Both end at the same loopback gateway and both are managed identically once registered in **Settings → Remote Instances**. +## Deploy your first Kiro Crew on EC2 (step by step) + +This is the shortest path from a fresh laptop to a working cloud Kiro Crew, using +the one-command launcher (`kirocrew cloud`). Plan on ~10 minutes plus one browser +sign-in. + +**Before you start (laptop prerequisites):** + +1. **AWS CLI installed and configured.** `aws sts get-caller-identity` must print + your account — if it errors, run `aws configure` (or `aws sso login`) first. +2. **Session Manager plugin** — `kirocrew cloud doctor` offers to install it, or + see [cloud-instance-ssm-vs-ssh.md](cloud-instance-ssm-vs-ssh.md). +3. **npm on your laptop (recommended)** — with `npm` installed (Node.js 18+), + the launcher builds the dashboard frontend automatically from the packaged + source (`npm ci && npm run build` in an isolated temporary tree — your + checkout's own `website/dist` and `static/dist` are never read or touched) + and ships the result, so the box skips the slow, failure-prone on-box npm + build. `kirocrew cloud doctor` reports whether the frontend will ship. + Without npm — or if the local build fails — the box builds the frontend + itself. +4. **A git checkout of this repo** with Kiro Crew installed from it — the launcher + packages your local source and uploads it to S3 (private-repo safe), it does + not clone GitHub on the box. + +Then run the checks and launch: + +```bash +kirocrew cloud doctor # verifies AWS CLI, credentials, plugin, frontend build +kirocrew cloud launch # interactive: picks the recommended tier, region, etc. +``` + +- Accept the recommended **balanced** tier. The **light** tier is sized under + Kiro Crew's working set — fine for a quick look, but expect pressure under + real workloads. (`kirocrew cloud launch` prints each tier's current instance + type and memory; the source of truth is `src/kiro_crew/cloud/sizes.py`.) +- Launch blocks until the box is **actually serving the dashboard** (the stack's + health check verifies the SPA loads, not just that something answers on the + port). Typical wait: 10–20 minutes on a cold box. +- When it finishes it prints a dashboard URL and opens it. **Sign in to Kiro in + the browser window** that appears (device-code flow) — chats don't work until + this is done. If you launched with `--yes` (non-interactive), run + `kirocrew cloud login` afterwards to do this step. + +**Coming back later:** + +```bash +kirocrew cloud connect # reopens the dashboard tunnel (alias: tunnel) +kirocrew cloud status # instance state +kirocrew cloud stop # stop the box (EBS kept; cheap) +kirocrew cloud start # start it again +kirocrew cloud destroy # tear everything down (stack + uploaded source) +``` + +If `connect` ever prints a "stale dashboard" warning, the box is missing its +frontend assets — see +[Dashboard HTML not found](#dashboard-html-not-found-after-launch) below. + ## Way 1 — SSH tunnel 1. On your laptop, forward the gateway's port over SSH (use the **real** gateway @@ -59,6 +116,44 @@ port, no SSH key. These map to warnings in `kirocrew doctor`. +### "Dashboard HTML not found" after launch + +The gateway is up but has no frontend assets (`src/kiro_crew/static/dist` is +missing) — `kirocrew cloud connect` also prints a "stale dashboard" warning in +this state. Fixes, cheapest first: + +1. On the box: `sudo systemctl restart kirocrew` — picks up assets staged after + the gateway started. +2. Still missing? The on-box npm build failed at install time. On a cloud box + the install is fail-closed (`KIROCREW_REQUIRE_FRONTEND=1` plus the stack's + own dist check), so re-running `kirocrew cloud launch` (with Node/npm on + your laptop, so the frontend ships pre-built) is the reliable fix. +3. To inspect by hand: `kirocrew cloud connect` keeps working for SSH/SSM access + (`aws ssm start-session --target i-…`); the install log is + `/var/log/kirocrew-install.log` on the box. + +### Launch fails during package install / pip output scrolls by + +What looks like "python testing" is pip's dependency resolver on a cold box. The +template already retries the install once for transient mirror flakes; if the +stack still rolls back, just re-run `kirocrew cloud launch` — a second run reuses +the warm dnf/pip caches and almost always gets through. Persistent failures point +at the instance tier: use **balanced**, not light. + +### Stack rolls back with a WaitCondition timeout + +The box didn't report healthy within 25 minutes. Read the actual failure reason: + +```bash +aws cloudformation describe-stack-events \ + --stack-name kirocrew- \ + --query 'StackEvents[?ResourceStatus==`CREATE_FAILED`].ResourceStatusReason' +``` + +The reason carries the last lines of the on-box install log, which names the +failing step (package install, npm build, gateway start). Fix that step, then +re-run `kirocrew cloud launch`. + ### MCP tools all fail: "Sandbox backend unavailable … `allow_unsandboxed_exec` is not set" On Linux, agent subprocesses run inside a **user-namespace sandbox**. Many hardened diff --git a/docs/system-specs/modules/cloud.md b/docs/system-specs/modules/cloud.md index a053750e9e2..660de80c5e3 100644 --- a/docs/system-specs/modules/cloud.md +++ b/docs/system-specs/modules/cloud.md @@ -73,7 +73,7 @@ claim that a hostile in-process agent is fully contained. | `ssm.py` | SSM `send-command` run-and-poll (base64-wrapped remote scripts) + `start-session` port-forward; `open_port_forward()` directly spawns the streaming `aws ssm start-session` child because `run_aws` captures output, and calls `aws.assert_human_action()` before doing so; `port_is_free` / `wait_for_local_port`. | | `login.py` | `kiro-cli` device-code / social sign-in on the box over SSM, plus `logout` — the account switch. `login` short-circuits on an existing session, so `logout` is what makes a different Kiro account reachable without a hand-run SSM command. It kills any still-polling background `kiro-cli login` **and** any live `kiro-cli acp` runtime **before** signing out (otherwise the login re-authenticates the old account, and an ACP runtime keeps serving the old account's in-memory credential until its next 401), removes the login log/PID/FIFO (they hold the previous device-code URL + code, which must never be re-shown as a fresh prompt), and confirms the result with `is_logged_in` rather than the exit code — `kiro-cli logout` exits non-zero when there was no session to drop, which is still the requested state. That confirmation fails CLOSED: it requires a positive signed-out sentinel (`__NOAUTH__`), so an SSM timeout or transport error — where the session may still be active — reports failure rather than a false "signed out". The same fail-closed applies to the cleanup command itself: if that SSM invocation doesn't return `Success`, the kills it was meant to do can't be trusted and logout reports failure without probing. The CLI warns the operator that in-flight chats/cron sessions are stopped (their runtimes are killed). | | `connect.py` | SSM port-forward + token mint + open browser; Instances-registry integration; `redact_token`. `is_launched_instance()` prevents the generic instance PATCH endpoint from rewriting a correlated launch’s connection method, SSM target, AWS profile, or region, so Stop/Start/Delete retain the stack address and a running billable instance is not stranded. | -| `source.py` | Detect and package an editable local checkout (`git archive`, tarfile fallback) and upload it to a per-account S3 bucket; packaged installs instead use the template's public-repo clone fallback. The secret-excluding filter is shared by both packaging paths. Also **`ensure_instance_boundary`** — creates the shared, immutable `kirocrew-ec2-boundary` managed policy once (create-if-not-exists, never re-versioned) and returns its ARN; `delete_instance_boundary` for admin cleanup. | +| `source.py` | Detect and package an editable local checkout (`git archive`, tarfile fallback) and upload it to a per-account S3 bucket; packaged installs instead use the template's public-repo clone fallback. The secret-excluding filter is shared by both packaging paths. `_inject_dist` extracts only `website/` from that already-filtered archive into an isolated temporary root, runs lockfile-exact `npm ci --ignore-scripts` plus the stock build there, and appends the resulting frontend. It never reads the gitignored checkout `static/dist`, so a bundle left by another branch cannot override the source being shipped; edition composition variables are removed so an external composition root cannot enter the build. The built dist must be a real tree contained by the isolated root. Members are then admitted through four gates: the secret filter (`allow_dist_under` exempts only the literal destination `dist` component for that exact prefix — every other excluded dir still refuses the member; `.env`/`.pem`/credential-name checks still apply), a **type allowlist** (`_DIST_ALLOWED_SUFFIXES` — only build-artifact extensions ship), the hardened nolink read gate (`hooks.safe_read_file_bytes_nolink`: O_NOFOLLOW + same-fd fstat rejects symlinks/hardlinks/non-regular files race-free, fd-real-path containment), and a bounded text-content scan using the shared credential redactor as a detector. The content scan covers UTF-8 HTML, JavaScript, CSS, source maps, JSON/manifests, and SVG; it blocks distinctive/plaintext and encoded credentials plus exact 40-character bare AWS secret keys, while ignoring the redactor's bare-secret warning for longer base64-shaped runs because real minified bundles and data URIs trigger that heuristic. Shipping is **atomic over one hardened-read byte snapshot**: the scanner and index-reference check inspect the same bytes written to the archive, and any build, containment, read/content, or referenced-chunk rejection aborts injection entirely — the original archive ships unchanged and the box uses its required npm-build fallback, so a partial dist never rides. Diagnostics repr-escape untrusted filenames and index references before logging or terminal display. Also **`ensure_instance_boundary`** — creates the shared, immutable `kirocrew-ec2-boundary` managed policy once (create-if-not-exists, never re-versioned) and returns its ARN; `delete_instance_boundary` for admin cleanup. | | `config.py` | Persisted profile / region / tag (**never credentials**); `load()` tolerates a hand-edited/corrupt `cloud.json` — bad JSON *or* a non-object shape falls back to defaults rather than crashing every cloud command. | | `sizes.py` | arm64/Graviton size tiers (16 GB default `t4g.xlarge`). | | `ui.py` / `wizard.py` | Terminal UI + the interactive launch flow. `_deploy_with_progress` runs the blocking deploy on a daemon thread and captures the `aws cloudformation deploy` child via a `proc_sink`, so a Ctrl+C on the main (poll) thread terminates it instead of orphaning it (~1800s). An unknown `--size`/`size_key` on the public `launch()` entrypoint yields a clean rc=1 + message, not an uncaught `KeyError`. Resuming a saved stack (`launch` after `stop`) first calls `_ensure_running_and_ssm_ready` — starts a `stopped` instance and waits for SSM `Online` before sign-in/tunnel (which are SSM-only and would otherwise fail); a `terminated` instance fails clean pointing at `--new`. `last_tag` is persisted (`cfg.save()`) **only after** a deploy confirms healthy — a failed first launch leaves no saved pointer, so the next `launch` retries clean instead of resuming a rolled-back/instance-less stack; `_saved_launch_is_usable` additionally ignores a stale saved tag (from an older build) whose stack is in a `_FAILED_STATES` status or has no instance. | @@ -84,8 +84,12 @@ claim that a hostile in-process agent is fully contained. CloudFormation stack, one `aws cloudformation deploy` (change-set based), atomic rollback, one-command `delete-stack` teardown. AMI resolves from the public `resolve:ssm` Amazon-Linux-2023 alias per arch (no hardcoded AMI ids). A -`WaitCondition` + `cfn-signal` blocks the deploy until the gateway is healthy; a -failed bootstrap folds the on-box setup-log tail into the signal reason so the cause survives the rollback. Bootstrap failure reasons are normalized to printable ASCII before CloudFormation receives them; otherwise CloudFormation replaces the setup error with a charset error and masks it during rollback (`test_cloud_ec2.py::test_failure_reason_is_filtered_to_printable_ascii`). +`WaitCondition` + `cfn-signal` blocks the deploy until the gateway is healthy — +where healthy means the root URL answers **and** its body lacks the gateway's +"Dashboard HTML not found" marker (the dashboard-less fallback page is still +HTTP 200, so a bare status check would bless a frontend-less box). A failed +bootstrap folds the on-box setup-log tail into the signal reason so the cause +survives the rollback. Bootstrap failure reasons are normalized to printable ASCII before CloudFormation receives them; otherwise CloudFormation replaces the setup error with a charset error and masks it during rollback (`test_cloud_ec2.py::test_failure_reason_is_filtered_to_printable_ascii`). Bootstrap is reboot-resilient. UserData performs no package or build work directly: it first copies its already-rendered script to @@ -113,7 +117,17 @@ role (`s3:GetObject` scoped to the single object). Wheel and desktop installs have no checkout to package, so `ec2.deploy` omits `SourceBucket` by default and the template clones the public repository/ref instead. An explicit `ship_source=True` remains fail-closed rather than packaging an unrelated -`site-packages` ancestor. +`site-packages` ancestor. Packaging builds the stock frontend from the exact +filtered source archive in a temporary root, with dependency lifecycle scripts +disabled, and injects only the admitted result. It never reads or mutates the +checkout's live `website/dist` or gitignored `static/dist`; this binds shipped +same-origin JavaScript to the source tarball even after a branch switch. If that +isolated build fails, the original archive ships unchanged and the box builds +with npm. `install.sh` skips the on-box npm build only when the source-bound +bundle is present. A cloud box that still ends up without a dist is failed by +the template's existing gates (`KIROCREW_REQUIRE_FRONTEND=1` makes an on-box +build failure fatal, and the bootstrap's dist check fails the stack before the +gateway starts); local installs stay non-fatal. `discover_network` is **egress-kind-aware**, not just "has a default route": `_subnet_egress_kinds` classifies each subnet's effective route table (explicit diff --git a/install.sh b/install.sh index 26f483f6d9a..fb5ae94c1c7 100755 --- a/install.sh +++ b/install.sh @@ -411,7 +411,40 @@ cd "$KIROCREW_APP_DIR" # ── Frontend (npm + vite) ── # Vite emits to website/dist; we stage it into src/kiro_crew/static/dist # where setup.py copies it into the package at install time. -if has node && [ -d "$KIROCREW_APP_DIR/website" ]; then +# A launch that ships the prebuilt bundle (kirocrew cloud launch injects it +# into the source tarball) skips this step entirely — the on-box npm build is +# the fallback path. The skip is gated on KIROCREW_REQUIRE_FRONTEND=1 (exported +# by the CFn template) so it only fires on cloud boxes: a LOCAL checkout also +# has a staged static/dist (a symlink on source installs), and skipping there +# would freeze the dashboard on whatever was built last. index.html alone does +# not prove a usable bundle: a torn staging or interrupted earlier build can +# leave an index whose hashed chunks are missing, and an install RETRY that +# trusted it would skip the rebuild and serve a shell whose every chunk 404s. +# So the skip additionally requires every /assets/ chunk the index references +# to exist (the same completeness signal frontend._incomplete_bundle_reason +# uses); an incomplete tree falls through to the npm rebuild below. A cloud box +# that ends up with no dist is failed by the existing gates: +# KIROCREW_REQUIRE_FRONTEND=1 makes a build failure fatal below, and the +# template's own DIST_INDEX check fails the stack before the gateway starts. +# Local installs stay non-fatal. +_dist_bundle_complete() { + # Complete = index.html exists and every /assets/*.js|.css it references + # is present on disk. Gateway-served routes (/manifest.js) are not under + # /assets/ and are deliberately not matched. + local _dist_dir _refs _ref + _dist_dir="$1" + [ -f "$_dist_dir/index.html" ] || return 1 + _refs="$(grep -oE '(src|href)="/assets/[^"?#]+\.(js|css)"' \ + "$_dist_dir/index.html" 2>/dev/null | sed -E 's/^[^"]*"//; s/"$//')" + for _ref in $_refs; do + [ -f "$_dist_dir$_ref" ] || return 1 + done + return 0 +} +if [ "${KIROCREW_REQUIRE_FRONTEND:-0}" = "1" ] \ + && _dist_bundle_complete "$KIROCREW_APP_DIR/src/kiro_crew/static/dist"; then + ok "Frontend shipped pre-built — skipping npm build" +elif has node && [ -d "$KIROCREW_APP_DIR/website" ]; then info "Building frontend (website/)…" _fe_log="$(mktemp)" ( diff --git a/src/kiro_crew/cli_cloud.py b/src/kiro_crew/cli_cloud.py index d72cc03d563..1af3d1a7cf2 100644 --- a/src/kiro_crew/cli_cloud.py +++ b/src/kiro_crew/cli_cloud.py @@ -18,7 +18,7 @@ from kiro_crew.cloud import connect as connect_mod from kiro_crew.cloud import ec2, iam from kiro_crew.cloud import login as login_mod -from kiro_crew.cloud import sizes, ssm, ui, wizard +from kiro_crew.cloud import sizes, source, ssm, ui, wizard from kiro_crew.cloud.aws import AWSError, CloudActionDenied from kiro_crew.cloud.config import DEFAULT_REGION, CloudConfig from kiro_crew.deploy.engine import resolve_aws_bin @@ -118,6 +118,14 @@ def _cloud_connect(args: argparse.Namespace) -> int: ui.fail(str(exc)) return 1 if conn.ready and conn.url: + # Warn early if the box serves the "Dashboard HTML not found" page (no + # static/dist) instead of letting the user open a broken dashboard. + # Deferred import: cli_server pulls the gateway's heavy module chain + # (vector_memory → numpy), which must not load at CLI import time + # (test_cli_lazy_imports, issue #3504). + from kiro_crew.cli_server import _probe_dashboard_health + + _probe_dashboard_health(conn.local_port) if not conn.token: # Tunnel is up but the token mint failed — the URL will hit the # dashboard's login wall. Say so instead of implying it's ready. @@ -402,6 +410,19 @@ def _cloud_doctor(args: argparse.Namespace) -> int: else: ui.fail("AWS not reachable") ui.detail(reach.get("note", "")) + # A launch builds the stock frontend from the exact source archive in an + # isolated temporary tree, so a residual gitignored static/dist can never + # influence what ships. Doctor checks only the local prerequisites; the + # packaging path remains fail-closed and falls back to the box build. + try: + reason = source.dist_ineligible_reason(source.repo_root()) + except Exception as exc: + reason = f"could not inspect the checkout ({exc})" + if not reason: + ui.ok("frontend source ready (launch builds an isolated prebuilt bundle)") + else: + ui.warn(f"frontend cannot be prebuilt for launch: {reason}") + ui.detail("The box will use its required npm-build fallback.") return 0 diff --git a/src/kiro_crew/cli_server.py b/src/kiro_crew/cli_server.py index 8c8fd494797..aa4cdd4c75c 100644 --- a/src/kiro_crew/cli_server.py +++ b/src/kiro_crew/cli_server.py @@ -134,7 +134,8 @@ def _probe_dashboard_health(port: int) -> None: print( "⚠️ Warning: gateway is serving a stale dashboard " "(assets missing — likely an update pruned the " - "running install). Restart the gateway to fix.", + "running install). Restart the gateway to fix " + "(on a cloud box: sudo systemctl restart kirocrew).", file=sys.stderr, ) except Exception: diff --git a/src/kiro_crew/cloud/ec2.py b/src/kiro_crew/cloud/ec2.py index efd7392c4e0..7373b1a8f4d 100644 --- a/src/kiro_crew/cloud/ec2.py +++ b/src/kiro_crew/cloud/ec2.py @@ -668,6 +668,11 @@ def deploy( # access needed). Fall back to a git clone only if source shipping is off. source_bucket = source_key = "" if ship_source: + # Build the stock frontend from website/ inside the exact filtered source + # archive, then inject its admitted bytes. The temporary build never + # mutates the checkout's live website/dist or trusts its ignored + # static/dist; failure leaves the source archive unchanged so the box + # uses its required npm-build fallback. source_bucket, source_key = source_mod.upload_source(tag, profile, region) def _cleanup_uploaded_source() -> None: diff --git a/src/kiro_crew/cloud/source.py b/src/kiro_crew/cloud/source.py index 213d1bcd866..329e6c672e8 100644 --- a/src/kiro_crew/cloud/source.py +++ b/src/kiro_crew/cloud/source.py @@ -12,14 +12,20 @@ from __future__ import annotations +import base64 +import binascii +import io import logging import os +import re +import shutil import subprocess import tarfile import tempfile from pathlib import Path from typing import Optional +from kiro_crew import frontend, hooks, security from kiro_crew.cloud import aws from kiro_crew.sel import sel from kiro_crew.subprocess_utf8 import UTF8_TEXT @@ -123,10 +129,154 @@ def repo_root() -> Path: ) -def _exclude_filter(ti: tarfile.TarInfo) -> Optional[tarfile.TarInfo]: - """Shared tar member filter: drop excluded dirs + credential-shaped files.""" +# Repo-relative destination for the source-bound frontend bundle. ``static/dist`` +# is git-ignored (and "dist" is in ``_EXCLUDE_DIRS``), so the source archive never +# carries a residual checkout bundle; ``_inject_dist`` builds from the archived +# ``website/`` bytes and appends only the admitted result. +_DIST_PREFIX = "src/kiro_crew/static/dist" + +# Extension allowlist for injected dist members. Vite/Rollup output (and the +# ``website/public/`` files it copies verbatim) is entirely static web assets, +# so admission is type-level: only build-artifact-shaped files ship. A stray +# ``secrets.yaml`` / ``id_rsa`` / extensionless credential dropped into +# ``website/public/`` is refused by TYPE, independent of the name/suffix +# denylists in :func:`_exclude_filter` — an allowlist cannot be extended by an +# attacker naming a file cleverly, where a denylist can be sidestepped. +_DIST_ALLOWED_SUFFIXES = frozenset( + { + # documents + code + ".html", + ".js", + ".mjs", + ".cjs", + ".css", + ".map", + ".json", + ".webmanifest", + ".wasm", + # images + ".svg", + ".png", + ".jpg", + ".jpeg", + ".gif", + ".webp", + ".ico", + ".avif", + # fonts + ".woff", + ".woff2", + ".ttf", + } +) + +# Allowed asset types whose bytes must be valid UTF-8 and credential-free. Binary +# formats bypass text decoding but still pass the name, type and hardened-read +# gates. SVG is XML text and source maps/manifests are JSON text. +# A `data:/;base64,` URI is an embedded resource that +# DECLARES itself as one in its own syntax. Vite inlines small images and fonts +# this way, and those payloads are long base64 runs that trip the generic +# bare-secret sliding-window heuristic (a real 231-char PNG run from this +# project's built editor CSS does; pinned by +# test_real_bundled_data_uri_does_not_false_positive). +# +# Masking the payload before the credential scan removes the false-positive +# SOURCE, which is what lets every remaining bare-secret warning be treated as +# actionable. The alternative -- exempting the warning whenever the run is not +# exactly 40 chars -- also discarded the true positives, because +# `security._contains_bare_secret` exists precisely to catch a genuine 40-char +# secret glued to adjacent base64 characters ("X" + key, key + "ABC", +# key + "X" + key), all of which report a 41+ char run. +# +# Only the payload is masked, and only for the scan: the bytes shipped in the +# tarball are always the unmodified originals. A credential baked into a bundle +# by a build lands in ordinary JS/CSS, not inside a data: URI, so the leak this +# gate exists to stop is still caught. +_DATA_URI_B64_RE = re.compile(r"data:[\w.+-]+/[\w.+-]+;base64,([A-Za-z0-9+/=]+)") + +#: Prefix of the generic entropy-heuristic warning `redact_credentials` emits. +_BARE_SECRET_WARNING = "Redacted bare secret key" + +#: Container magic numbers for the asset types a bundler inlines as data URIs. +#: The exemption below is keyed on CONTENT, not position: a payload is only +#: treated as media if it actually decodes to one of these. Masking purely by +#: position would let a bare key parked where a payload goes escape the scan. +_MEDIA_MAGIC = ( + b"\x89PNG\r\n\x1a\n", # png + b"\xff\xd8\xff", # jpeg + b"GIF87a", + b"GIF89a", + b"RIFF", # webp + b"wOFF", + b"wOF2", + b"\x00\x01\x00\x00", # ttf + b"OTTO", + b"true", + b"ttcf", + b"\x00\x00\x01\x00", # ico + b" bool: + """True if a base64 data-URI payload decodes to a recognized media container.""" + for pad in ("", "=", "=="): + try: + raw = base64.b64decode(payload + pad, validate=True) + except (binascii.Error, ValueError): + continue + return raw.startswith(_MEDIA_MAGIC) + return False + + +def _mask_inlined_media(text: str) -> str: + """Blank out data-URI payloads that are genuinely inlined media.""" + + def replace(match: "re.Match[str]") -> str: + return "data:masked" if _data_uri_payload_is_media(match.group(1)) else match.group(0) + + return _DATA_URI_B64_RE.sub(replace, text) + + +_DIST_TEXT_SUFFIXES = frozenset( + {".html", ".js", ".mjs", ".cjs", ".css", ".map", ".json", ".webmanifest", ".svg"} +) + + +def _dist_suffix_allowed(arcname: str) -> bool: + """Whether an injected dist member's extension is an allowed asset type.""" + suffix = Path(arcname).suffix.lower() + return suffix in _DIST_ALLOWED_SUFFIXES + + +def _exclude_filter( + ti: tarfile.TarInfo, allow_dist_under: Optional[str] = None +) -> Optional[tarfile.TarInfo]: + """Shared tar member filter: drop excluded dirs + credential-shaped files. + + ``allow_dist_under`` names the ONE repo-relative prefix whose members + exempt only the ``dist`` component of ``_EXCLUDE_DIRS`` — used by + :func:`_inject_dist` for the prebuilt ``static/dist`` bundle, which + otherwise trips on the ``dist`` dir name. Every other excluded dir (``.aws``, + ``.ssh``, …) and the suffix / ``.env`` / credential-name checks still apply + to every member, so a ``static/dist/.aws/credentials`` or + ``static/dist/.env`` is still refused. Default ``None`` keeps every other + caller byte-identical. + """ + # Pure-posix join: injected arcnames are already posix and the allow-prefix + # comparison must not vary with the host separator. + name = "/".join(Path(ti.name).parts) parts = set(Path(ti.name).parts) - if parts & _EXCLUDE_DIRS: # excluded / secret-bearing directory anywhere in path + excluded = parts & _EXCLUDE_DIRS + if allow_dist_under is not None and ( + name == allow_dist_under or name.startswith(allow_dist_under + "/") + ): + # Only the prefix's own `dist` component is exempt; every other excluded + # dir (secret-bearing or not) still refuses the member — a staged + # ``static/dist/.aws/credentials`` must never ship. + excluded -= {"dist"} + if excluded: # excluded / secret-bearing dir anywhere return None if ti.name.endswith(_EXCLUDE_SUFFIXES): # credential file suffixes return None @@ -330,24 +480,214 @@ def _excluded(rel: str) -> bool: return Path(out.name) +class _AbortInjection(Exception): + """Internal: discard the injected tarball and ship the original archive.""" + + +def _read_dist_bundle(dist: Path, dist_resolved: Path) -> list[tuple[str, bytes]]: + """Return one policy-checked, immutable snapshot of the dist bundle. + + Every candidate is read exactly once through the no-link gate. Text assets + are decoded strictly and scanned for credential-shaped content without + rewriting their bytes. ``index.html`` is parsed from that same pinned byte + snapshot, so completeness checks cannot follow a swapped path or inspect + content different from what the tarball receives. + """ + admitted: list[tuple[str, bytes]] = [] + admitted_arcnames: set[str] = set() + index_html: Optional[str] = None + + for file_path in sorted(dist.rglob("*")): + if not file_path.is_file() or file_path.is_symlink(): + continue + arcname = f"{_DIST_PREFIX}/{file_path.relative_to(dist).as_posix()}" + ti = tarfile.TarInfo(arcname) + if _exclude_filter(ti, allow_dist_under=_DIST_PREFIX) is None: + logger.info("excluding %r from source tarball", arcname) + continue + if not _dist_suffix_allowed(arcname): + logger.info( + "excluding %r (extension not in the dist asset allowlist)", + arcname, + ) + continue + + try: + data = hooks.safe_read_file_bytes_nolink(str(file_path), within_root=str(dist_resolved)) + except hooks.FileTooLargeError as exc: + raise _AbortInjection(f"{arcname!r} exceeds the hardened read limit") from exc + if data is None: + raise _AbortInjection( + f"{arcname!r} rejected by the hardened read gate " + "(hardlink/symlink/non-regular or escaped the tree)" + ) + + suffix = Path(arcname).suffix.lower() + if suffix in _DIST_TEXT_SUFFIXES: + try: + decoded = data.decode("utf-8") + except UnicodeDecodeError as exc: + raise _AbortInjection(f"text asset {arcname!r} is not valid UTF-8") from exc + # The data-URI exemption applies to the GENERIC bare-secret + # heuristic ONLY -- that is the single check with a false-positive + # problem on built assets -- and only to a payload that actually + # DECODES to media (see _mask_inlined_media). Exempting by position + # alone would let a bare key parked where a payload goes escape. Distinctive and encoded-credential + # matches have reliable token structure, so they stay actionable + # everywhere, including inside a data: URI payload (an attacker + # cannot launder a key by base64-ing it into an inlined image). + # + # Every bare-secret warning on the masked text is actionable, + # including runs longer than 40 chars: that is what a genuine key + # glued to adjacent base64 characters looks like, and it is + # precisely what `security._contains_bare_secret` exists to catch. + _, full_warnings = security.redact_credentials(decoded) + _, masked_warnings = security.redact_credentials(_mask_inlined_media(decoded)) + actionable = [ + warning for warning in full_warnings if not warning.startswith(_BARE_SECRET_WARNING) + ] + [warning for warning in masked_warnings if warning.startswith(_BARE_SECRET_WARNING)] + if actionable: + raise _AbortInjection(f"text asset {arcname!r} contains credential-shaped content") + if arcname == f"{_DIST_PREFIX}/index.html": + index_html = decoded + + admitted.append((arcname, data)) + admitted_arcnames.add(arcname) + + if index_html is None: + raise _AbortInjection("index.html was not admitted by the hardened read gate") + + unshipped = [ + ref + for ref in frontend._index_asset_refs(index_html) + if f"{_DIST_PREFIX}{ref}" not in admitted_arcnames + ] + if unshipped: + raise _AbortInjection( + f"index.html references {unshipped[0]!r} which was refused admission " + "— a partial dist must never ship" + ) + return admitted + + +def _build_archived_dist(archive: Path) -> list[tuple[str, bytes]]: + """Build and admit a stock frontend from the exact archived source bytes.""" + with tempfile.TemporaryDirectory(prefix="kirocrew-frontend-") as temp: + build_root = Path(temp).resolve(strict=True) + website_root = build_root / "website" + with tarfile.open(archive, "r:gz") as src: + for member in src: + try: + rel = Path(member.name).relative_to("website") + except ValueError: + continue + if not member.isreg() or not rel.parts or ".." in rel.parts: + continue + target = website_root / rel + target.parent.mkdir(parents=True, exist_ok=True) + source_fh = src.extractfile(member) + if source_fh is None: + raise _AbortInjection(f"could not read archived {member.name!r}") + with source_fh: + target.write_bytes(source_fh.read()) + + messages: list[str] = [] + try: + dist = frontend.build_stock_frontend(build_root, log=messages.append) + except OSError as exc: + # `build_stock_frontend` resolves npm with shutil.which and reports a + # MISSING one by returning None, but the spawn itself is unguarded: + # an npm that resolves yet cannot be executed (non-executable, broken + # symlink, unlinked between the check and the spawn, EPERM) raises + # here. Without this branch it would reach `except BaseException: + # raise` below and abort the launch with a traceback -- exactly the + # outcome the archive-unchanged fallback exists to prevent. Converted + # to a refusal so the box builds its own frontend, which is what + # every other build failure already does. + raise _AbortInjection(f"could not run the frontend build ({exc!r})") from exc + if dist is None: + reason = messages[-1].strip() if messages else "frontend build failed" + raise _AbortInjection(reason) + try: + dist.lstat() + dist_resolved = dist.resolve(strict=True) + except OSError as exc: + raise _AbortInjection(f"could not resolve built frontend ({exc!r})") from exc + if dist.is_symlink() or build_root not in dist_resolved.parents: + raise _AbortInjection("built frontend resolves outside the isolated source tree") + return _read_dist_bundle(dist, dist_resolved) + + +def dist_ineligible_reason(root: Path) -> str: + """Why a source-bound frontend cannot be built for a cloud launch ("" = eligible).""" + website = root / "website" + if not website.is_dir(): + return "website source is not present in this checkout" + if not (website / "package-lock.json").is_file(): + return "website/package-lock.json is missing" + if not shutil.which("npm"): + return "npm is not installed" + return "" + + +def _inject_dist(archive: Path, root: Path) -> Path: + """Build from archived ``website/`` source and append the admitted dist. + + The ignored checkout ``static/dist`` is never trusted: build scripts can + produce arbitrary same-origin JavaScript, so no marker can safely bind a + residual bundle to the source being shipped. Instead the stock frontend is + rebuilt in a temporary root populated only from the already-filtered source + archive. Any build or admission refusal returns that original archive + unchanged so the box builds with npm. + """ + injected = tempfile.NamedTemporaryFile( # noqa: SIM115 - handed to caller + prefix="kirocrew-src-", suffix=".tar.gz", delete=False + ) + injected.close() + try: + admitted = _build_archived_dist(archive) + with tarfile.open(archive, "r:gz") as src, tarfile.open(injected.name, "w:gz") as dst: + for member in src: + fh = src.extractfile(member) if member.isreg() else None + dst.addfile(member, fh) + for arcname, data in admitted: + ti = tarfile.TarInfo(arcname) + ti.size = len(data) + dst.addfile(ti, io.BytesIO(data)) + except _AbortInjection as exc: + logger.warning("not shipping %s: %s — the box will build with npm", _DIST_PREFIX, exc) + Path(injected.name).unlink(missing_ok=True) + return archive + except BaseException: + Path(injected.name).unlink(missing_ok=True) + raise + archive.unlink(missing_ok=True) + logger.info("built and injected frontend %s from archived source", _DIST_PREFIX) + return Path(injected.name) + + def build_source_tarball(root: Optional[Path] = None) -> Path: """Package the local source tree into a gzip tarball; return its path. Uses ``git archive HEAD`` (fast) for a clean checkout, but if the tracked working tree is DIRTY (uncommitted edits to tracked files) it uses the ``git ls-files`` tar path instead — otherwise the launch would silently ship - stale last-commit code. Both paths ship only tracked files. + stale last-commit code. Both paths ship only tracked files. ``_inject_dist`` + then builds the stock frontend from that exact archive in an isolated + temporary tree and appends only its admitted byte snapshot. A residual + gitignored checkout bundle is never read; a failed build leaves the original + archive unchanged so the box builds its own frontend. """ root = root or repo_root() if _tracked_tree_is_dirty(root): logger.info("working tree has uncommitted tracked changes; packaging the working tree") - return _tar_fallback(root) + return _inject_dist(_tar_fallback(root), root) archive = _use_git_archive(root) if archive is not None: logger.info("packaged source via git archive: %s", archive) - return archive + return _inject_dist(archive, root) logger.info("git archive unavailable; using tracked-file tarfile fallback") - return _tar_fallback(root) + return _inject_dist(_tar_fallback(root), root) def _account_id(profile: str, region: str) -> str: diff --git a/src/kiro_crew/cloud/templates/kirocrew-ec2.yaml b/src/kiro_crew/cloud/templates/kirocrew-ec2.yaml index 8036b78caa5..a883953d2a1 100644 --- a/src/kiro_crew/cloud/templates/kirocrew-ec2.yaml +++ b/src/kiro_crew/cloud/templates/kirocrew-ec2.yaml @@ -283,6 +283,14 @@ Resources: # EXPANDED size is guarded by test_cloud_ec2.py's UserData size test; # when it trips, trim the script or move knowledge here, never delete it. # + # "Dashboard health": the WaitCondition gate accepts the box only when + # the root URL answers AND its body lacks the gateway's + # DASHBOARD_HTML_NOT_FOUND_MARKER (dashboard/handlers/core.py). The + # gateway serves that guidance page with HTTP 200 when static/dist is + # missing, so a bare status check would bless a dashboard-less box. + # install.sh fails closed on a missing/incomplete bundle first; this is + # the backstop at the serving layer, for a box whose assets vanish + # after install already succeeded. # "Reboot resume": State Manager runs wildcard associations when a new # managed node first comes online. An account-level # AWS-RunPatchBaseline association can therefore patch and reboot the @@ -680,13 +688,19 @@ Resources: systemctl enable kirocrew.service || fail "could not enable kirocrew.service" systemctl start kirocrew.service || fail "could not start kirocrew.service" - echo "--- waiting for the gateway to answer on 127.0.0.1:${DashboardPort} ---" + echo "--- waiting for the gateway to serve the dashboard on 127.0.0.1:${DashboardPort} ---" + # see "Dashboard health" ok=0 for i in $(seq 1 60); do - if curl -fsS -o /dev/null http://127.0.0.1:${DashboardPort}/ 2>/dev/null; then ok=1; break; fi + if curl -fsS http://127.0.0.1:${DashboardPort}/ 2>/dev/null \ + | grep -q 'Dashboard HTML not found'; then + : + elif curl -fsS -o /dev/null http://127.0.0.1:${DashboardPort}/ 2>/dev/null; then + ok=1; break + fi sleep 5 done - [ "$ok" = "1" ] || fail "gateway did not become healthy within 5 minutes" + [ "$ok" = "1" ] || fail "gateway did not serve the dashboard within 5 minutes" echo "=== KiroCrew bootstrap complete $(date -u) ===" signal_ok=0 @@ -717,6 +731,8 @@ Resources: Properties: Handle: !Ref WaitHandle # 25 min: cold boot + dnf + NodeSource + npm build (vite) + pip install. + # The vite build time only applies when the launch shipped no prebuilt + # static/dist (the usual path skips it); keep the budget for that fallback. Timeout: "1500" Count: 1 diff --git a/src/kiro_crew/frontend.py b/src/kiro_crew/frontend.py index 206266b43e7..7ab5119092b 100644 --- a/src/kiro_crew/frontend.py +++ b/src/kiro_crew/frontend.py @@ -21,7 +21,7 @@ import subprocess import tempfile from pathlib import Path -from typing import Callable, Iterator, Optional +from typing import Callable, Iterator, List, Optional from kiro_crew import platform_compat from kiro_crew.executors import subprocess_executor @@ -240,6 +240,18 @@ def ensure_dev_dist_symlink() -> Optional[Path]: return candidate +def _index_asset_refs(html: str) -> List[str]: + """The ``/assets/`` chunk paths ``index.html`` references. + + Vite emits the content-hashed chunks under ``/assets/``, so these are the + completeness signal shared by :func:`_incomplete_bundle_reason` (chunks + exist on disk) and the source-shipping injector (chunks made it into the + tarball). Gateway-served routes (``/manifest.js``) are deliberately not + matched. + """ + return re.findall(r'(?:src|href)="(/assets/[^"?#]+\.(?:js|css))', html) + + def _incomplete_bundle_reason(tree: Path) -> str: """Why ``tree`` is not a complete built frontend, or ``""`` if it is. @@ -260,7 +272,7 @@ def _incomplete_bundle_reason(tree: Path) -> str: html = index.read_text(encoding="utf-8", errors="replace") except OSError as exc: return f"index.html is unreadable ({exc})" - refs = re.findall(r'(?:src|href)="(/assets/[^"?#]+\.(?:js|css))', html) + refs = _index_asset_refs(html) missing = [ref for ref in refs if not (tree / ref.lstrip("/")).is_file()] if missing: return f"{len(missing)} referenced asset(s) missing, e.g. {missing[0]}" @@ -295,62 +307,125 @@ def _staging_lock(static_parent: Path) -> Iterator[None]: yield -def _npm_build_and_stage_locked( +def _run_npm_step( website_dir: Path, - proj_path: Path, npm: str, + args: list[str], + timeout: int, log: Callable[[str], None], + label: str, + *, + env: Optional[dict[str, str]] = None, ) -> bool: - """Run ``npm run build`` then stage it. Caller holds the staging lock. - - The build is spawned in its own process group and the whole tree is reaped - on timeout. ``npm run build`` is ``tsc -b && vite build``, so killing only - npm would leave vite writing ``website/dist`` after this function returns - and the lock releases — a surviving writer makes the lock's exclusion - meaningless, since a peer could then stage a tree vite is still rewriting. - """ + """Run one npm step and reap its whole process tree on timeout.""" proc = subprocess.Popen( - [npm, "run", "build"], - env=_edition_build_env(), + [npm, *args], + env=env, cwd=str(website_dir), # DEVNULL, not PIPE: nothing reads the build's output, and pipes would # make the post-kill drain block until every grandchild closes its - # inherited write handle — inside the lock holder, which would then - # never release it. + # inherited write handle. stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, start_new_session=platform_compat.IS_POSIX, creationflags=platform_compat.CREATE_NEW_PROCESS_GROUP, ) try: - proc.wait(timeout=_BUILD_TIMEOUT) + proc.wait(timeout=timeout) except subprocess.TimeoutExpired: # Enumerate BEFORE killing: the kill reparents survivors to init and # erases the PPID links that identify them. The group kill alone misses - # a descendant that started its own session, and such an escapee keeps - # rewriting website/dist after this holder releases the staging lock — - # the mixed-bundle publication this lock exists to prevent. + # a descendant that started its own session. descendants = platform_compat.process_descendants(proc.pid) try: platform_compat.kill_process_tree(proc.pid, platform_compat.SIGKILL) except (ProcessLookupError, OSError, ValueError) as exc: - log(f" ⚠️ Could not reap the timed-out frontend build: {exc}") + log(f" ⚠️ Could not reap the timed-out frontend {label}: {exc}") for child in descendants: try: platform_compat.kill_process_tree(child, platform_compat.SIGKILL) except (ProcessLookupError, OSError, ValueError): - # Already reaped by the group kill, or no longer signalable. continue - # Reap the direct child so it is not left a zombie. Bounded, so a - # survivor cannot hold the staging lock open indefinitely. try: proc.wait(timeout=_BUILD_KILL_GRACE) except subprocess.TimeoutExpired: - log(" ⚠️ Frontend build did not die after SIGKILL") - log(" ⚠️ Frontend build timed out — dashboard may be stale") + log(f" ⚠️ Frontend {label} did not die after SIGKILL") + log(f" ⚠️ Frontend {label} timed out — dashboard may be stale") return False if proc.returncode != 0: - log(" ⚠️ Frontend build failed — dashboard may be stale") + log(f" ⚠️ Frontend {label} failed — dashboard may be stale") + return False + return True + + +def build_stock_frontend( + proj_path: Path, + npm: str | None = None, + log: Callable[[str], None] = print, +) -> Optional[Path]: + """Build a stock dist from an isolated source tree without staging it. + + Cloud source packaging extracts ``website/`` from the exact source archive + into a temporary root and calls this helper. Edition variables are removed: + an inherited composition root points outside that archive and would break + the guarantee that the shipped JavaScript derives only from packaged source. + """ + website_dir = proj_path / _DIR_NAME + lockfile = website_dir / "package-lock.json" + if not website_dir.is_dir() or not lockfile.is_file(): + log(" ⚠️ Archived website source or package-lock.json is missing") + return None + npm_bin = npm or shutil.which("npm") + if not npm_bin: + log(" ⚠️ npm not found — cannot build the archived frontend") + return None + env = dict(os.environ) + env.pop(_EDITION_DIR_ENV, None) + env.pop(_EDITION_OPT_IN_ENV, None) + if not _run_npm_step( + website_dir, + npm_bin, + ["ci", "--ignore-scripts", "--no-audit", "--no-fund"], + _INSTALL_TIMEOUT, + log, + "npm install", + env=env, + ): + return None + if not _run_npm_step( + website_dir, + npm_bin, + ["run", "build"], + _BUILD_TIMEOUT, + log, + "build", + env=env, + ): + return None + dist = website_dir / "dist" + reason = _incomplete_bundle_reason(dist) + if reason: + log(f" ⚠️ Archived frontend build is incomplete ({reason})") + return None + return dist + + +def _npm_build_and_stage_locked( + website_dir: Path, + proj_path: Path, + npm: str, + log: Callable[[str], None], +) -> bool: + """Run ``npm run build`` then stage it. Caller holds the staging lock.""" + if not _run_npm_step( + website_dir, + npm, + ["run", "build"], + _BUILD_TIMEOUT, + log, + "build", + env=_edition_build_env(), + ): return False static_dist = proj_path / "src" / "kiro_crew" / "static" / "dist" return _stage_dist_locked(website_dir / "dist", static_dist, log) diff --git a/src/kiro_crew/security_posture.py b/src/kiro_crew/security_posture.py index 4789cd7d955..4cab3a8c92c 100644 --- a/src/kiro_crew/security_posture.py +++ b/src/kiro_crew/security_posture.py @@ -1424,6 +1424,10 @@ class PostureControl: # run twice. Nothing redacted here is written or shown — the ledger and # the transcript keep the original text. "dashboard/steer_settle.py", + # DETECTOR, not a redactor: scans candidate frontend bytes and discards the + # redacted output. Admission either preserves those original bytes unchanged + # or aborts the dist injection, so this module owns no redacted egress. + "cloud/source.py", # DETECTOR, not a redactor: the pre-push content scan calls both scanners only # to COUNT findings and then refuses the push. It deliberately discards the # cleaned text — rewriting a code diff would corrupt the very fix the gate diff --git a/test/test_cloud_cli.py b/test/test_cloud_cli.py index fdb184df413..ebb06d28375 100644 --- a/test/test_cloud_cli.py +++ b/test/test_cloud_cli.py @@ -205,6 +205,70 @@ def test_connect_not_ready_returns_failure(self, monkeypatch, capsys): assert rc == 1 assert "Dashboard tunnel did not become ready" in capsys.readouterr().out + def test_connect_probes_dashboard_health_on_local_port(self, monkeypatch): + # When the tunnel is ready, connect warns (via cli_server's probe) if the + # box serves "Dashboard HTML not found" instead of a working SPA. + monkeypatch.setattr(cli_cloud.ssm, "session_manager_plugin_installed", lambda: True) + monkeypatch.setattr( + ec2, "describe", lambda *a, **k: {"exists": True, "instance_id": "i-0abc"} + ) + monkeypatch.setattr( + connect_mod, + "connect", + lambda *a, **k: connect_mod.Connection( + instance_id="i-0abc", + local_port=5599, + remote_port=5476, + token="tok", + url="http://127.0.0.1:5599/?token=tok", + ready=True, + process=None, # .wait() skipped + ), + ) + probed = {} + # cli_cloud defers the probe import into _cloud_connect (CLI lazy-import + # ratchet, issue #3504), so patch it at its source module. + import kiro_crew.cli_server as cli_server + + monkeypatch.setattr( + cli_server, + "_probe_dashboard_health", + lambda port: probed.setdefault("port", port), + ) + rc = cli_cloud._cloud_connect(_args(profile="", region="", tag="kc-1")) + assert rc == 0 + assert probed.get("port") == 5599 + + def test_doctor_reports_frontend_build_prerequisites(self, monkeypatch, capsys, tmp_path): + # Doctor reports whether the launcher can build from archived source; an + # ignored checkout dist is deliberately irrelevant to provenance. + monkeypatch.setattr(cli_cloud, "_resolve", lambda a: ("", "us-east-1")) + monkeypatch.setattr( + "shutil.which", lambda name, **kw: "/usr/bin/npm" if name == "npm" else None + ) + monkeypatch.setattr(cli_cloud.ssm, "session_manager_plugin_installed", lambda: False) + monkeypatch.setattr(cli_cloud.ssm, "session_manager_plugin_install_hint", lambda: "") + monkeypatch.setattr( + cli_cloud.iam, + "reachability_check", + lambda *a, **k: {"reachable": False, "note": "offline"}, + ) + import kiro_crew.cloud.source as source_mod + + monkeypatch.setattr(source_mod, "repo_root", lambda: tmp_path) + assert cli_cloud._cloud_doctor(_args(profile="", region="")) == 0 + assert "cannot be prebuilt" in capsys.readouterr().out + + website = tmp_path / "website" + website.mkdir() + (website / "package-lock.json").write_text('{"lockfileVersion": 3}\n') + assert cli_cloud._cloud_doctor(_args(profile="", region="")) == 0 + assert "frontend source ready" in capsys.readouterr().out + + (website / "package-lock.json").unlink() + assert cli_cloud._cloud_doctor(_args(profile="", region="")) == 0 + assert "package-lock.json is missing" in capsys.readouterr().out + class TestDestroy: def test_destroy_dry_run(self, monkeypatch, capsys): diff --git a/test/test_cloud_ec2.py b/test/test_cloud_ec2.py index 605fb84d4ab..e8221ed406a 100644 --- a/test/test_cloud_ec2.py +++ b/test/test_cloud_ec2.py @@ -715,9 +715,18 @@ def test_dry_run_defaults_to_public_clone_without_checkout(self, monkeypatch): class TestDeployShipsSource: def test_deploy_uploads_source_and_passes_params(self, monkeypatch): import kiro_crew.cloud.source as source_mod + from kiro_crew import frontend as frontend_mod monkeypatch.setattr(ec2, "find_stack", lambda *a, **k: None) monkeypatch.setattr(source_mod, "ensure_instance_boundary", lambda *a, **k: _BOUNDARY_ARN) + + # Launch must NEVER rebuild the frontend: on a source install + # static/dist is the LIVE tree the local gateway serves, and an + # in-launch npm build would empty/replace it mid-serve. + def _never_build(*a, **k): + raise AssertionError("ec2.deploy must not call frontend.build_and_stage") + + monkeypatch.setattr(frontend_mod, "build_and_stage", _never_build) monkeypatch.setattr( source_mod, "upload_source", diff --git a/test/test_cloud_source.py b/test/test_cloud_source.py index 420e9c2f7c8..1b42bddd29b 100644 --- a/test/test_cloud_source.py +++ b/test/test_cloud_source.py @@ -2,6 +2,9 @@ from __future__ import annotations +import base64 +import io +import os import tarfile from pathlib import Path @@ -304,12 +307,543 @@ def _archive_must_not_run(root): # pragma: no cover - must not be called tarball.unlink() def test_clean_tree_prefers_git_archive(self, monkeypatch, tmp_path): - # The fast path (git archive) is still used when the tree is clean. + # The fast path (git archive) is still used when the tree is clean — + # the tarball is the git-archive output, passed through _inject_dist + # (a no-op without a local static/dist, which tmp_path lacks). monkeypatch.setattr(source, "_tracked_tree_is_dirty", lambda root: False) - sentinel = tmp_path / "archive.tar.gz" - sentinel.write_bytes(b"x") - monkeypatch.setattr(source, "_use_git_archive", lambda root: sentinel) - assert source.build_source_tarball(tmp_path) == sentinel + + def _fake_archive(root): + out = tmp_path / "archive.tar.gz" + with tarfile.open(out, "w:gz") as tf: + payload = tmp_path / "app.py" + payload.write_text("x=1\n") + tf.add(payload, arcname="app.py") + return out + + monkeypatch.setattr(source, "_use_git_archive", _fake_archive) + tarball = source.build_source_tarball(tmp_path) + try: + with tarfile.open(tarball) as tf: + assert "app.py" in tf.getnames() + finally: + tarball.unlink() + + +class TestBuildArchivedDist: + @staticmethod + def _archive(tmp_path: Path, members: dict[str, bytes]) -> Path: + archive = tmp_path / "source.tar.gz" + with tarfile.open(archive, "w:gz") as tf: + for name, data in members.items(): + ti = tarfile.TarInfo(name) + ti.size = len(data) + tf.addfile(ti, io.BytesIO(data)) + return archive + + def test_build_uses_archived_source_not_residual_checkout(self, tmp_path, monkeypatch): + archive = self._archive( + tmp_path, + { + "website/package-lock.json": b'\n{"lockfileVersion": 3}\n', + "website/source-marker.txt": b"archived source\n", + }, + ) + residual = tmp_path / "src" / "kiro_crew" / "static" / "dist" + (residual / "assets").mkdir(parents=True) + (residual / "index.html").write_text("residual\n") + (residual / "assets" / "app.js").write_text("// malicious residual\n") + + def _build(build_root, **_kwargs): + website = build_root / "website" + assert (website / "source-marker.txt").read_text( + encoding="utf-8" + ) == "archived source\n" + dist = website / "dist" + (dist / "assets").mkdir(parents=True) + (dist / "index.html").write_bytes(b'\n') + (dist / "assets" / "app.js").write_bytes(b"// archive-bound bundle\n") + return dist + + monkeypatch.setattr(source.frontend, "build_stock_frontend", _build) + + admitted = dict(source._build_archived_dist(archive)) + + assert admitted[f"{source._DIST_PREFIX}/assets/app.js"] == (b"// archive-bound bundle\n") + assert b"malicious residual" not in b"".join(admitted.values()) + + def test_build_refuses_output_outside_isolated_tree(self, tmp_path, monkeypatch): + archive = self._archive( + tmp_path, {"website/package-lock.json": b'{"lockfileVersion": 3}\n'} + ) + outside = tmp_path / "outside-dist" + (outside / "assets").mkdir(parents=True) + (outside / "index.html").write_text("outside\n") + (outside / "assets" / "app.js").write_text("// outside\n") + monkeypatch.setattr(source.frontend, "build_stock_frontend", lambda *_a, **_kw: outside) + + with pytest.raises(source._AbortInjection, match="outside the isolated"): + source._build_archived_dist(archive) + + def test_unexecutable_npm_refuses_instead_of_raising(self, tmp_path, monkeypatch): + """A resolvable-but-unrunnable npm must refuse, not raise. + + ``build_stock_frontend`` reports a MISSING npm by returning None, but + the spawn itself is unguarded: an npm that resolves via ``shutil.which`` + yet cannot be executed (non-executable, broken symlink, unlinked between + the check and the spawn, EPERM) raises OSError. That has to become an + ``_AbortInjection`` so the caller ships the original archive and the box + builds its own frontend -- otherwise the launch dies with a traceback, + which is exactly what the fallback exists to prevent. + """ + archive = self._archive( + tmp_path, {"website/package-lock.json": b'{"lockfileVersion": 3}\n'} + ) + # A real file that resolves but cannot be executed -> Popen raises. + fake_npm = tmp_path / "npm" + fake_npm.write_text("not executable\n") + fake_npm.chmod(0o644) + monkeypatch.setattr("shutil.which", lambda name, **kw: str(fake_npm)) + + with pytest.raises(source._AbortInjection, match="could not run the frontend build"): + source._build_archived_dist(archive) + + +class TestInjectDist: + """``_inject_dist`` ships only a frontend built from archived source.""" + + @staticmethod + def _make_archive(tmp_path: Path, names: list[str]) -> Path: + out = tmp_path / "in.tar.gz" + with tarfile.open(out, "w:gz") as tf: + for name in names: + payload = tmp_path / "payload.txt" + payload.write_text("x=1\n") + tf.add(payload, arcname=name) + return out + + @staticmethod + def _make_dist(root: Path) -> Path: + dist = root / "src" / "kiro_crew" / "static" / "dist" + (dist / "assets").mkdir(parents=True) + (dist / "index.html").write_text("spa\n") + (dist / "assets" / "app.js").write_text("// bundle\n") + return dist + + @pytest.fixture(autouse=True) + def _build_local_dist(self, monkeypatch, tmp_path): + """Keep admission tests focused; provenance has dedicated tests below.""" + + def _build(archive): + dist = archive.parent / "src" / "kiro_crew" / "static" / "dist" + if not dist.exists(): + raise source._AbortInjection("frontend build failed") + return source._read_dist_bundle(dist, dist.resolve(strict=True)) + + monkeypatch.setattr(source, "_build_archived_dist", _build) + + def test_dist_members_injected(self, tmp_path): + self._make_dist(tmp_path) + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + try: + with tarfile.open(out) as tf: + names = tf.getnames() + assert "install.sh" in names + assert f"{source._DIST_PREFIX}/index.html" in names + assert f"{source._DIST_PREFIX}/assets/app.js" in names + finally: + out.unlink(missing_ok=True) + + def test_no_dist_returns_archive_unchanged(self, tmp_path): + archive = self._make_archive(tmp_path, ["install.sh"]) + assert source._inject_dist(archive, tmp_path) == archive + assert archive.exists() # not rewritten/removed + + def test_dist_symlinks_never_shipped(self, tmp_path): + dist = self._make_dist(tmp_path) + outside = tmp_path / "outside.txt" + outside.write_text("secret beyond the tree\n") + link = dist / "linked.txt" + try: + link.symlink_to(outside) + except OSError: + pytest.skip("symlink creation not permitted here") + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + try: + with tarfile.open(out) as tf: + names = tf.getnames() + assert not any("linked.txt" in n for n in names) + assert not any("outside.txt" in n for n in names) + finally: + out.unlink(missing_ok=True) + + def test_dist_secret_files_still_filtered(self, tmp_path): + # allow_dist_under skips only the _EXCLUDE_DIRS ("dist") check — the + # credential checks (.env, .npmrc, .pem) still apply inside the bundle. + dist = self._make_dist(tmp_path) + (dist / ".env").write_text("TOKEN=x\n") + (dist / ".npmrc").write_text("//registry/:_authToken=x\n") + (dist / "cert.pem").write_text("-----BEGIN-----\n") + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + try: + with tarfile.open(out) as tf: + names = tf.getnames() + assert f"{source._DIST_PREFIX}/index.html" in names + assert not any(n.endswith(".env") for n in names) + assert not any(n.endswith(".npmrc") for n in names) + assert not any(n.endswith(".pem") for n in names) + finally: + out.unlink(missing_ok=True) + + def test_dist_secret_dirs_still_filtered(self, tmp_path): + # The allow exempts only the literal "dist" component: a secret-bearing + # DIRECTORY staged inside the bundle (e.g. a copied static/dist/.aws) + # must still be refused — the credential checks are name/suffix-based + # and would not catch .aws/credentials on their own. + dist = self._make_dist(tmp_path) + aws_dir = dist / ".aws" + aws_dir.mkdir() + (aws_dir / "credentials").write_text("[default]\naws_access_key_id=x\n") + ssh_dir = dist / ".ssh" + ssh_dir.mkdir() + (ssh_dir / "id_ed25519").write_text("OPENSSH PRIVATE KEY\n") + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + try: + with tarfile.open(out) as tf: + names = tf.getnames() + assert f"{source._DIST_PREFIX}/index.html" in names + assert not any("/.aws/" in n for n in names) + assert not any("/.ssh/" in n for n in names) + finally: + out.unlink(missing_ok=True) + + def test_exclude_filter_default_still_drops_dist(self): + # The prefix allow is opt-in: without allow_dist_under, "dist" members + # (e.g. node_modules/pkg/dist/junk.js) stay excluded as before. + ti = tarfile.TarInfo("node_modules/pkg/dist/junk.js") + assert source._exclude_filter(ti) is None + assert source._exclude_filter(ti, allow_dist_under=source._DIST_PREFIX) is None + ok = tarfile.TarInfo(f"{source._DIST_PREFIX}/index.html") + assert source._exclude_filter(ok) is None # default unchanged + assert source._exclude_filter(ok, allow_dist_under=source._DIST_PREFIX) is ok + + def test_allow_dist_under_does_not_allow_sibling_prefix(self): + # The allow is an exact prefix: a path under "dist-evil" is NOT under + # "dist", so the allow must not rescue it. This one has no literal "dist" + # part so it passes either way — the discriminating case is below. + not_under = tarfile.TarInfo("src/kiro_crew/static/dist-evil/x.js") + assert source._exclude_filter(not_under, allow_dist_under=source._DIST_PREFIX) is not_under + # A member that IS under a "dist" dir but NOT the exact allowed prefix + # must stay excluded even when the allow is active. + other_dist = tarfile.TarInfo("website/node_modules/pkg/dist/junk.js") + assert source._exclude_filter(other_dist, allow_dist_under=source._DIST_PREFIX) is None + + def test_hardlinked_credential_in_dist_refused(self, tmp_path): + # A HARDLINK planted in dist aliasing a credential file has a path + # inside the checkout and is not a symlink — only the nolink read gate + # (fstat st_nlink > 1 on the opened descriptor) catches it. A planted + # filesystem object has no legitimate shape inside a Vite build tree, + # so ONE rejection aborts the WHOLE injection: nothing ships, the box + # builds with npm. + dist = self._make_dist(tmp_path) + secret = tmp_path / "outside-secret.txt" + secret.write_text("AKIAIOSFODNN7EXAMPLE\n") + try: + os.link(secret, dist / "assets" / "vendor-abc123.js") + except (OSError, NotImplementedError): + pytest.skip("hardlinks not supported on this filesystem") + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + assert out == archive # aborted: original archive, no dist at all + with tarfile.open(archive) as tf: + assert tf.getnames() == ["install.sh"] + + def test_oversized_asset_aborts_atomically(self, tmp_path, monkeypatch): + dist = self._make_dist(tmp_path) + archive = self._make_archive(tmp_path, ["install.sh"]) + original = archive.read_bytes() + real_read = source.hooks.safe_read_file_bytes_nolink + created: list[str] = [] + real_ntf = source.tempfile.NamedTemporaryFile + + def _oversized(path, *args, **kwargs): + if Path(path) == dist / "assets" / "app.js": + raise source.hooks.FileTooLargeError("over the per-file cap") + return real_read(path, *args, **kwargs) + + def _capturing_ntf(*args, **kwargs): + temp = real_ntf(*args, **kwargs) + created.append(temp.name) + return temp + + monkeypatch.setattr(source.hooks, "safe_read_file_bytes_nolink", _oversized) + monkeypatch.setattr(source.tempfile, "NamedTemporaryFile", _capturing_ntf) + + assert source._inject_dist(archive, tmp_path) == archive + assert archive.read_bytes() == original + assert created and not any(Path(path).exists() for path in created) + + def test_credential_in_text_asset_aborts_injection(self, tmp_path): + dist = self._make_dist(tmp_path) + (dist / "assets" / "config.js").write_text( + 'window.config={accessKey:"AKIAIOSFODNN7EXAMPLE"};\n' + ) + archive = self._make_archive(tmp_path, ["install.sh"]) + original = archive.read_bytes() + + out = source._inject_dist(archive, tmp_path) + + assert out == archive + assert archive.read_bytes() == original + with tarfile.open(archive) as tf: + assert tf.getnames() == ["install.sh"] + + def test_exact_bare_aws_secret_in_text_asset_aborts_injection(self, tmp_path): + dist = self._make_dist(tmp_path) + (dist / "assets" / "config.js").write_text( + 'window.secret="wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY";\n' + ) + archive = self._make_archive(tmp_path, ["install.sh"]) + + assert source._inject_dist(archive, tmp_path) == archive + + @pytest.mark.parametrize( + "run", + [ + "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEYA", + "XwJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", + "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEYABC", + "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEYXwJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", + ], + ids=["trailing", "leading", "trailing-multi", "two-keys"], + ) + def test_glued_bare_aws_secret_in_text_asset_aborts_injection(self, tmp_path, run): + """A real secret glued to adjacent base64 chars must not ship. + + ``security._contains_bare_secret`` exists to catch exactly this: a + genuine 40-char key with no delimiter on one side reports a 41+ char + run. An admission gate that only acted on the exact ``(40 chars)`` + warning discarded that signal and shipped the key verbatim into an + unauthenticated ``/assets/`` file. + """ + dist = self._make_dist(tmp_path) + (dist / "assets" / "config.js").write_text(f'window.s="{run}";\n') + archive = self._make_archive(tmp_path, ["install.sh"]) + original = archive.read_bytes() + + assert source._inject_dist(archive, tmp_path) == archive + assert archive.read_bytes() == original + with tarfile.open(archive) as tf: + assert tf.getnames() == ["install.sh"] + + def test_data_uri_payload_cannot_hide_a_credential_elsewhere(self, tmp_path): + """Masking a data: URI must not blind the scan to the rest of the file.""" + secret = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + dist = self._make_dist(tmp_path) + (dist / "assets" / "editor.css").write_text( + '.i{background:url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUg")}\n' + f'.j{{content:"{secret}"}}\n' + ) + archive = self._make_archive(tmp_path, ["install.sh"]) + + assert source._inject_dist(archive, tmp_path) == archive + + @pytest.mark.parametrize( + "payload", + [ + "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", + "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEYABC", + ], + ids=["exact-40", "glued"], + ) + def test_bare_secret_parked_as_a_data_uri_payload_aborts(self, tmp_path, payload): + """A key where a payload goes is not media, so it is not exempt. + + The exemption exists for genuinely inlined assets, which decode to a + recognized container. Keying it on POSITION instead of CONTENT would + turn ``data:image/png;base64,`` into a laundering prefix: writing the + raw key there would discard the bare-secret warning and ship it. + """ + dist = self._make_dist(tmp_path) + (dist / "assets" / "editor.css").write_text( + f'.i{{background:url("data:image/png;base64,{payload}")}}\n' + ) + archive = self._make_archive(tmp_path, ["install.sh"]) + original = archive.read_bytes() + + assert source._inject_dist(archive, tmp_path) == archive + assert archive.read_bytes() == original + + def test_encoded_credential_inside_data_uri_aborts_injection(self, tmp_path): + """The data-URI exemption covers the entropy heuristic, not real keys. + + Only the generic bare-secret heuristic is exempted inside a ``data:`` + payload, because that is the one check built assets false-positive on. + A distinctive credential base64'd into an inlined image must still + abort -- otherwise the exemption would be a laundering channel. + """ + payload = base64.b64encode(b'{"accessKey":"AKIAIOSFODNN7EXAMPLE"}').decode() + dist = self._make_dist(tmp_path) + (dist / "assets" / "editor.css").write_text( + f'.i{{background:url("data:image/png;base64,{payload}")}}\n' + ) + archive = self._make_archive(tmp_path, ["install.sh"]) + original = archive.read_bytes() + + assert source._inject_dist(archive, tmp_path) == archive + assert archive.read_bytes() == original + + def test_real_bundled_data_uri_does_not_false_positive(self, tmp_path): + dist = self._make_dist(tmp_path) + # This 231-character PNG data run comes from Kiro Crew's built editor + # CSS and trips the generic bare-secret sliding-window heuristic. Long + # minified runs are not an asset-safe signal; distinctive credentials + # and exact 40-character bare AWS secrets remain blocked. + data = ( + "iVBORw0KGgoAAAANSUhEUgAAAAQAAAAECAYAAACp8Z5+AAAAAXNSR0IArs4c6Q" + "AAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsMAAA7DAcdvqGQAAAAZdEVYdFNv" + "ZnR3YXJlAHBhaW50Lm5ldCA0LjAuMTZEaa/1AAAAHUlEQVQYV2PYvXu3JAi7u" + "LiAMaYAjAGTQBPYLQkAa/0Zef3qRswAAAAASUVORK5CYII" + ) + (dist / "assets" / "editor.css").write_text( + f'.icon{{background:url("data:image/png;base64,{data}")}}\n' + ) + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + try: + with tarfile.open(out) as tf: + assert f"{source._DIST_PREFIX}/assets/editor.css" in tf.getnames() + finally: + out.unlink(missing_ok=True) + + def test_symlinked_index_is_not_read(self, tmp_path): + dist = tmp_path / "src" / "kiro_crew" / "static" / "dist" + (dist / "assets").mkdir(parents=True) + outside = tmp_path / "outside-index.html" + outside.write_text("outside\n") + try: + (dist / "index.html").symlink_to(outside) + except OSError: + pytest.skip("symlink creation not permitted here") + archive = self._make_archive(tmp_path, ["install.sh"]) + + assert source._inject_dist(archive, tmp_path) == archive + + def test_control_char_in_ref_is_escaped_in_diagnostic(self, tmp_path): + dist = tmp_path / "src" / "kiro_crew" / "static" / "dist" + dist.mkdir(parents=True) + (dist / "index.html").write_text('\n') + + with pytest.raises(source._AbortInjection) as exc_info: + source._read_dist_bundle(dist, dist.resolve(strict=True)) + reason = str(exc_info.value) + + assert "\x1b" not in reason + assert r"\x1b" in reason + + def test_non_asset_extension_refused_by_allowlist(self, tmp_path): + # Vite copies website/public/* into dist verbatim, so an untracked + # secrets.yaml (or an extensionless credential) parked there would + # ship to S3. The type allowlist refuses it independent of the + # name/suffix denylists, while the legit members still ship. + dist = self._make_dist(tmp_path) + (dist / "secrets.yaml").write_text("aws_secret_access_key: hunter2\n") + (dist / "id_rsa_backup").write_text("PRIVATE KEY\n") + (dist / "notes.txt").write_text("internal deploy notes\n") + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + try: + with tarfile.open(out) as tf: + names = tf.getnames() + assert f"{source._DIST_PREFIX}/index.html" in names + assert f"{source._DIST_PREFIX}/assets/app.js" in names + assert not any(n.endswith("secrets.yaml") for n in names) + assert not any(n.endswith("id_rsa_backup") for n in names) + assert not any(n.endswith("notes.txt") for n in names) + finally: + out.unlink(missing_ok=True) + + def test_allowlist_admits_typical_vite_output(self, tmp_path): + # The allowlist must not refuse real build output: hashed chunks, + # sourcemaps, the webmanifest, fonts, images and wasm all ship. + dist = self._make_dist(tmp_path) + for name in ( + "assets/chunk-ab12cd34.js", + "assets/style-ef56ab78.css", + "assets/app.js.map", + "manifest.json", + "site.webmanifest", + "icon-192.png", + "logo.svg", + "fonts/inter.woff2", + "vendor/react.mjs", + "pcm.wasm", + ): + p = dist / name + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text("x\n") + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + try: + with tarfile.open(out) as tf: + names = set(tf.getnames()) + for name in ( + "assets/chunk-ab12cd34.js", + "assets/style-ef56ab78.css", + "assets/app.js.map", + "manifest.json", + "site.webmanifest", + "icon-192.png", + "logo.svg", + "fonts/inter.woff2", + "vendor/react.mjs", + "pcm.wasm", + ): + assert f"{source._DIST_PREFIX}/{name}" in names + finally: + out.unlink(missing_ok=True) + + def test_refused_referenced_chunk_aborts_injection(self, tmp_path): + # Atomicity: if a chunk index.html references is refused admission + # (here: the chunk on disk is a SYMLINK, which the walk skips), the + # bundle must not ship AT ALL — shipping the index without its chunk + # would make install.sh skip the on-box build and the box serve a + # shell whose chunk 404s. + dist = tmp_path / "src" / "kiro_crew" / "static" / "dist" + (dist / "assets").mkdir(parents=True) + (dist / "index.html").write_text( + '\n' + ) + real = tmp_path / "elsewhere.js" + real.write_text("// real chunk kept outside the tree\n") + try: + (dist / "assets" / "app-deadbeef.js").symlink_to(real) + except OSError: + pytest.skip("symlink creation not permitted here") + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + assert out == archive # aborted: partial dist never rides + with tarfile.open(archive) as tf: + assert tf.getnames() == ["install.sh"] + + def test_incomplete_dist_not_shipped(self, tmp_path): + # An index whose referenced hashed chunks are missing (torn tree from a + # failed/partial build) must NOT ship: install.sh would skip the on-box + # npm build because index.html exists, and the box would serve a shell + # whose every chunk 404s. + dist = tmp_path / "src" / "kiro_crew" / "static" / "dist" + dist.mkdir(parents=True) + (dist / "index.html").write_text( + '\n' + ) + archive = self._make_archive(tmp_path, ["install.sh"]) + out = source._inject_dist(archive, tmp_path) + assert out == archive # unchanged: incomplete bundle refused + with tarfile.open(archive) as tf: + assert tf.getnames() == ["install.sh"] class TestBucketNaming: diff --git a/test/test_frontend_dist_resolve.py b/test/test_frontend_dist_resolve.py index 3d6ed6903c3..9c2405d3549 100644 --- a/test/test_frontend_dist_resolve.py +++ b/test/test_frontend_dist_resolve.py @@ -720,6 +720,51 @@ def test_stage_built_dist_sweeps_residue_even_when_refusing(tmp_path): assert not orphan.exists(), "residue survived a refused stage" +def test_build_stock_frontend_uses_lockfile_without_lifecycle_scripts(tmp_path, monkeypatch): + website = tmp_path / "website" + website.mkdir() + (website / "package-lock.json").write_text('{"lockfileVersion": 3}\n') + monkeypatch.setenv("KIROCREW_EDITION_DIR", "/private/edition") + monkeypatch.setenv("KIROCREW_ALLOW_EDITION", "1") + calls: list[tuple[list[str], dict[str, str]]] = [] + + def _run(website_dir, npm, args, timeout, log, label, *, env=None): + assert website_dir == website + assert npm == "/usr/bin/npm" + assert env is not None + calls.append((args, env)) + if args == ["run", "build"]: + _make_dist(website / "dist") + return True + + monkeypatch.setattr(frontend, "_run_npm_step", _run) + + assert frontend.build_stock_frontend(tmp_path, npm="/usr/bin/npm") == (website / "dist") + assert [args for args, _env in calls] == [ + ["ci", "--ignore-scripts", "--no-audit", "--no-fund"], + ["run", "build"], + ] + for _args, env in calls: + assert "KIROCREW_EDITION_DIR" not in env + assert "KIROCREW_ALLOW_EDITION" not in env + + +def test_build_stock_frontend_stops_when_install_fails(tmp_path, monkeypatch): + website = tmp_path / "website" + website.mkdir() + (website / "package-lock.json").write_text('{"lockfileVersion": 3}\n') + calls: list[list[str]] = [] + + def _run(_website, _npm, args, *_rest, **_kwargs): + calls.append(args) + return False + + monkeypatch.setattr(frontend, "_run_npm_step", _run) + + assert frontend.build_stock_frontend(tmp_path, npm="/usr/bin/npm") is None + assert calls == [["ci", "--ignore-scripts", "--no-audit", "--no-fund"]] + + def test_build_and_stage_holds_the_lock_across_the_build(tmp_path): """The lock must be held while `npm run build` runs, not just while copying. diff --git a/test/test_spawn_audit.py b/test/test_spawn_audit.py index 8e6f21db749..3671df76997 100644 --- a/test/test_spawn_audit.py +++ b/test/test_spawn_audit.py @@ -1077,11 +1077,11 @@ def _is_bundled_skill_asset(path: Path) -> bool: # module's own location, never agent input) when no node resolves. Same # class as cli.py::_ensure_node, which invokes the identical script. "env.py::ensure_node", - # Fixed argv (`npm run build`) in the operator's own checkout. The npm - # binary and project path arrive from the caller: the Dev Fleet sync - # resolves npm via its trusted-bin allowlist and the path from the - # operator-registered worktree, never from agent input. - "frontend.py::_npm_build_and_stage_locked", + # Fixed argv (`npm ci`, `npm run build`) in the operator's own checkout + # or isolated archive-build tree. The npm binary and project path arrive + # from the caller: the Dev Fleet sync and cloud source packager resolve + # npm via its trusted-bin allowlist / which, never from agent input. + "frontend.py::_run_npm_step", "frontend.py::build_frontend_async", "frontend.py::build_frontend_sync", # _write_build_source_fingerprint stamps the built bundle's source