Commit Graph
3 Commits
Author SHA1 Message Date
shadow-testandClaude Opus 5 ab2c75d0b2 Ship a firewall backend, and correct three claims review disproved
Build App (Preview) / compute-version (pull_request) Successful in 5s
Build App (Preview) / create-release (pull_request) Successful in 3s
Build App (Preview) / build-macos (pull_request) Successful in 2m46s
Build App (Preview) / build-linux (pull_request) Successful in 7m22s
Build App (Preview) / build-windows (pull_request) Successful in 7m43s
Build App (Preview) / prune-previews (pull_request) Successful in 4s
Build Container / build-container (pull_request) Successful in 11m20s
Review of #28 found the iptables exclusion was justified by a false premise,
and I confirmed it: `wireguard-tools` declares `Recommends: nftables | iptables`,
`--no-install-recommends` strips it, and `wg-quick`'s add_default() shells out
to a firewall backend with no `type -p` guard. Measured on the image as this PR
shipped it:

    [#] iptables-restore -n
    /usr/bin/wg-quick: line 32: iptables-restore: command not found
    wg-quick EXIT=127

That fires for `AllowedIPs = 0.0.0.0/0` — every stock full-tunnel config from
every provider — not for a desktop client's killswitch as the comment claimed.
Split tunnels are unaffected.

Ship `nftables` rather than `iptables`: wg-quick prefers it (`type -p nft`, so
with both installed iptables is dead weight), it is first in the package's own
Recommends, and it is half the size.

The review's proposed fix stopped there; it does not hold. Adding nftables does
not make wg-quick work on this host, and neither does iptables:

    Warning: Extension CONNMARK revision 0 not supported, missing kernel module?

`Table=auto` routes by fwmark and needs xt_CONNMARK from the *host* kernel.
WSL2 has none and containers have no /lib/modules to load one from. So this
fixes native Linux and Docker Desktop for Mac — which other WHP users are on —
and cannot fix Docker Desktop for Windows, where the answer is to add routes
with `ip route` directly. Documented rather than left to be rediscovered.

Also from review:

- "`ip` and `wg` are always present" was false. A project keeps the base image
  it was first built from, so this reaches new projects only. Reworded to match
  the wording already used for the Playwright libraries, and `/usr/bin/wg` added
  to FEATURE_PROBES so an existing project is *told* it is missing VPN tooling
  and prompted to migrate, rather than finding out via `wg: command not found`.
- "no client is installed" contradicted shipping `wg` four lines earlier. The
  true claim is that no tunnel is configured or started.
- The size figure measured against bare ubuntu:24.04, which over-counts by the
  ~209 kB of libelf1t64 the real base already has, and covered one arch. Now
  measured against the current base on amd64 and stated for arm64 too, per the
  standard CLAUDE.md sets for the Playwright layer.
- `/run` persistence conflated two mechanisms: same-container files on a
  stop/start, `docker commit` on a recreation. Both stated, plus the corollary
  that key material written to /run ends up inside a snapshot image — observed,
  a `wg.priv` was already sitting in one.
- The DNS bullet presented a Docker Desktop address as the general case. Now
  leads with the mechanism, notes 127.0.0.11 on a user-defined network is
  unaffected, and adds the two things the advice omitted: a resolver the tunnel
  can reach (or it leaks every query), and pinning the endpoint via the old
  gateway (or the tunnel routes through itself).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-17 13:55:44 -07:00
shadow-testandClaude Opus 5 2de00b3c55 Fix review findings: secrets in snapshots, URL spoofing, migration data loss
Adversarial review of the branch produced findings across four areas.
This addresses them, plus the Windows CI environment.

Secrets. commit_container_snapshot baked the container's full env into
the per-project snapshot image, so the shared OAuth token — and the AWS
keys, git token and gateway master key — outlived revocation and were
readable via docker inspect. Verified against Engine 29.6 that a commit
body's config merges over the container's: keys cannot be dropped but
can be overwritten, so all of them now commit as KEY=. clear_claude_token
additionally rewrites images from earlier builds and reports honestly
when a tag could not be rewritten.

The recommendation to move the token out of env entirely was not taken,
with reasoning: apiKeyHelper is a different auth method that outranks
CLAUDE_CODE_OAUTH_TOKEN rather than a transport for it, and no
file-based delivery exists. The durable exposure — the image — is what
is closed here. Separately noted, not fixed: entrypoint.sh captures the
token into the scheduler's .env inside the persisted volume.

URL spoofing. Three call sites reached openUrl with container-controlled
strings, one of which the review missed (the WebLinksAddon handler).
The sign-in URL was scraped from container output with a longest-match
tie-break and no userinfo check, so claude.ai@evil.tld rendered as
"claude.ai…" in a truncating element. There is now one sanitizer in
front of every sink — scheme allowlist, no userinfo, C0/C1 and quote
rejection, host allowlist for the sign-in case, first-match — and the
origin renders un-truncated. The toast is keyed so a changed URL
remounts, closing a bait-and-switch where the user read one URL and
clicked another.

Migration. The rollback pin was best-effort: a tag failure was logged
and the migration continued past remove_container, after which the
final commit overwrote the only copy of the old system layer. It now
aborts before anything destructive and reads the tag back. /var was
destroyed while the ordinary recreate path preserves it — making the
"safe" alternative to Reset more destructive than Reset's alternative;
data-bearing subtrees are now detected and disclosed in the pre-flight
rather than copied, since tarring a live database onto a different
base's packages is a corruption risk. resume_migration now verifies the
migration-state label instead of reporting success for a container that
never swapped. dismiss actually resolves the record rather than leaving
the feature permanently refusing to migrate. Start and Reset are guarded
while a migration is live.

Lifecycle. The gateway no longer publishes on 0.0.0.0 — bind address and
advertised URL are derived together so they cannot drift. Disabling it
now stops it. App exit runs teardown concurrently under a budget with a
visible shutting-down state instead of blocking for minutes. Auto-starts
retry when Docker is not up yet, and the polling-recovery path now
reconciles, so interrupted migrations are still recovered. Auth-bridge
forwards are capped, closing a container-driven fd exhaustion.

Windows CI. build-windows failed on this branch with "linker link.exe
not found". The runner had no MSVC build tools and the workflow assumed
a hand-provisioned machine, so a bare runner registers, accepts jobs and
fails at link time after downloading the whole crate graph. The job now
installs the VC++ workload when vswhere cannot find it, matching how it
already conditionally installs Rust and Node.

192 Rust tests, 274 frontend tests, both builds clean, zero warnings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-09 19:35:39 -07:00
shadow-testandClaude Opus 5 d42b741337 Migrate a project onto a new base image without losing its volumes
Projects were pinned to the image they were first created from. Both
create paths preferred triple-c-snapshot-<id>:latest whenever it
existed, and container_needs_recreation compared the container's live
image against the triple-c.image label — which create_container wrote
from the same image it created from. A tautology that could never fire.
The only escape was Reset, which calls remove_project_volumes and
destroys the login, skills and transcripts.

Measured consequences on this host: real projects are missing socat (so
the auth bridge cannot tunnel) and bubblewrap (so sandbox mode does not
work), plus Mission Control and triple-c-sso-refresh, and sit 61
packages behind the base including ca-certificates, openssl and curl.

Detection. create_container now writes triple-c.base-image-id (the image
ID, not RepoDigests, which local-built and custom images do not have)
and triple-c.create-image. container_needs_recreation takes the expected
create-image and compares against the latter, so the check means
something. base-image-id is deliberately NOT compared: a base bump would
otherwise silently recreate from the snapshot, consuming the "you should
migrate" signal without migrating. Staleness is surfaced, never acted on
automatically.

Migration keeps the volumes. /home/claude and ~/.claude are volumes and
the image's copy is seed-only — permanently masked after first mount —
so the login, ~/.claude.json, skills, transcripts, scheduler tasks, SSH
keys, cargo, uv, ruff and Claude Code itself re-attach untouched. Only
root-level state is rebuilt: apt packages are replayed against the new
base rather than copied, so no stale libc is dragged forward, and
/usr/local, /opt and the non-bind-mounted parts of /workspace are copied
verbatim with tar --skip-old-files so they can never clobber a newer
base binary.

docker diff is not used: on a snapshot-derived container it reports only
changes since the last commit. Raw image-vs-image diffing is filtered
through dpkg ownership because it otherwise lies — 8,677 raw path
differences on a real project reduced to 2 genuinely user-authored
files, both loose /workspace-root files.

Crash safety. snapshot:latest keeps pointing at the old image until the
final commit, so any crash before it self-heals on next start. Later
crashes are caught by reconcile_project_statuses. The rollback pin is a
docker tag: 0.057s and 0 bytes. Rollback restores the system layer only
— volumes are never touched — and the UI says so rather than implying a
time machine.

Fixes an infinite recreation loop shipped with the MCP removal. docker
commit propagates labels to the image, so a container created from a
snapshot inherited its non-empty triple-c.mcp-fingerprint and the
one-shot shim recreated it again on every start, forever. Lineage labels
are now always written explicitly.

Documents the second, separate bug this uncovered: Dockerfile changes
under /home/claude never reach an existing project, migration or not,
because the volume masks them. Anything that must stay upgradable
belongs in /usr/local/bin or /opt, or must be seeded by entrypoint.sh.

145 Rust tests, 227 frontend tests, both builds clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-09 18:19:12 -07:00