feat: phone access + responsive bottom-sheet rail - #4
Conversation
petergyang
left a comment
There was a problem hiding this comment.
Thanks — reviewing from a phone over Tailscale is something we want, and the bottom-sheet UI looks thoughtful. A few things need to change before this can land, one of them security-critical:
1. The example config defeats the origin-isolation model. The README-less example sets HUMAN_REVIEW_ARTIFACT_HOST=100.x.y.z — the same host the shell is served from. That makes ARTIFACT_ORIGIN identical to the chrome origin, and since localhost-URL reviews keep allow-same-origin, the reviewed app's scripts get full same-origin access to the parent shell — including the API token in data-token. The entire two-loopback design exists to prevent exactly this. Remote use needs two distinct hostnames (e.g. the tailnet IP for the shell and the MagicDNS name for the artifact, or vice versa). Please add a hard guard: if the artifact host resolves to the same origin as the shell, refuse to serve URL-kind pages (or at minimum refuse to inject the token page) with a clear error explaining the two-hostname requirement.
2. Document the exposure model. With HUMAN_REVIEW_HOST=0.0.0.0, the token-free /artifact/<key>/… routes (keys are sha256(realpath)[:16], derivable from file paths) and sibling-asset serving become readable by anyone who can reach the port and send an allowed Host header, and /s/<id> embeds the token over plain HTTP. That's fine on a private tailnet and dangerous on shared Wi-Fi — the README needs a section that frames this feature as tailnet/VPN-only, shows the two-hostname config, and spells out what becomes network-readable.
3. Rebase onto current main. The branch predates several chrome changes on main (End review button, .ended overlay, comment editing styles, list support). GitHub reports it mergeable, but the css restructure moves the .handle/.rail rules those features sit next to — please rebase and re-verify the desktop layout with the newer UI elements present, plus the mobile sheet.
4. Smaller notes:
- The env vars are read by the detached server, so they only take effect when that server first starts. Either document "stop any running server first" or have the CLI warn when an env-configured value differs from the running server's.
HUMAN_REVIEW_PUBLIC_URLmakesopenBrowseropen the remote URL on the local machine — harmless, but consider skipping the auto-open when a public URL is set.
Happy to re-review quickly once the guard + docs are in.
Allow human-review to run over a real network host (e.g. Tailscale) so reviews work from a phone or another machine, not just localhost: - HUMAN_REVIEW_HOST: bind address (default 127.0.0.1, unchanged) - HUMAN_REVIEW_ALLOWED_HOSTS: comma-separated Host-header allowlist (default loopback-only, unchanged; DNS-rebinding protection intact) - HUMAN_REVIEW_ARTIFACT_HOST: host the artifact iframe loads from, injected into the shell as data-artifact-host (keeps the two-origin isolation when served over a real host) - HUMAN_REVIEW_CHROME_ORIGIN: explicit chrome origin injected into the artifact via data-chrome-origin, so the SDK's postMessage targets the shell even when it is not on the other loopback name - HUMAN_REVIEW_PUBLIC_URL: URL the CLI prints/opens for the user UI: the comments panel becomes a bottom sheet on narrow screens (mobile-first), floating over the artifact without disturbing the page, with a grabber tab toggle. Desktop (>=900px) keeps the classic right rail unchanged, via a media query. Tests: new test/remote-access.test.js covers host allowlisting and the two origin injections (71 total, all passing).
…en with PUBLIC_URL - server: refuse /s/<id> when HUMAN_REVIEW_ARTIFACT_HOST resolves to the same origin as the shell (a same-origin artifact would let a URL-kind review read data-token off the shell) — clear HTML error explains the two-distinct-hostnames requirement - cli: skip openBrowser when HUMAN_REVIEW_PUBLIC_URL is set (the phone opens the session, not the local machine) - README: 'Reviewing from another device' section — env table, two-host requirement, Tailscale example, exposure model (private networks only), env-vars-read-at-start note - tests: same-origin refused (500 + message), distinct hosts serve (200)
9c7e87e to
4164289
Compare
|
About this PR's tooling — for transparency on what you're merging: This PR (both the original phone-access work and today's review fixes) was authored and driven by Hermes Agent (Nous Research's agent framework) running on DeepSeek V4 Flash via the opencode-go provider. Tests were run locally (Node 24, 83 passing) and the layouts were verified via headless Chrome before pushing. Happy to re-run anything on request. |
What & why
human-review currently binds to
127.0.0.1only and hardcodes the two-loopback-origin trick (127.0.0.1shell ↔localhostartifact iframe). That means reviews can only happen in a browser on the same machine — no phone, no LAN, no Tailscale. This PR makes the tool work from a phone or any network-reachable device, and gives mobile users a proper bottom-sheet UI.Changes
Phone / remote access (all env-configurable, defaults unchanged)
HUMAN_REVIEW_HOST127.0.0.1)HUMAN_REVIEW_ALLOWED_HOSTSHUMAN_REVIEW_ARTIFACT_HOSTdata-artifact-host— preserves the two-origin isolation when served over a real hostHUMAN_REVIEW_CHROME_ORIGINdata-chrome-origin, so the SDK'spostMessagetargets the shell even when it's not the "other loopback"HUMAN_REVIEW_PUBLIC_URLSecurity guard: shell and artifact must be two distinct origins
The whole two-loopback design exists so the reviewed app's scripts can never read the session token off the shell. Remote use must keep that separation: the shell and the artifact need two distinct hostnames that both reach the machine (e.g. the tailnet IP for the shell and the MagicDNS name for the artifact, or vice versa). If
HUMAN_REVIEW_ARTIFACT_HOSTresolves to the same origin as the shell, the server now refuses to serve the session with a clear error page explaining the two-hostname requirement (a same-origin artifact would let a URL-kind review — which keepsallow-same-origin— readdata-tokenoff the shell).Example — run once, review from the phone (two hostnames,
100.x.y.z= tailnet IP,my-laptop.tailnet-name.ts.net= MagicDNS name):Then open
http://100.x.y.z:8124/s/<id>on the phone.Mobile bottom-sheet UI (responsive, desktop preserved)
@media (min-width: 900px)block. The toggle glyph adapts to layout viamatchMedia.Docs
README gains a "Reviewing from another device" section: the env-var table, the two-distinct-hostnames requirement, a Tailscale example, the exposure model (private-network-only: token-free
/artifact/<key>routes and/s/<id>over plain HTTP become reachable when bound to0.0.0.0), and a note that env vars are read when the server starts.Tests
New
test/remote-access.test.js:HUMAN_REVIEW_ALLOWED_HOSTSadmits an allowlisted Host header, still 403s strangersHUMAN_REVIEW_ARTIFACT_HOSTis injected into the shell pageHUMAN_REVIEW_CHROME_ORIGINis injected into the artifact83 tests pass (76 existing + 7 new), verified locally on Node 24; CI runs Node 20/22 on Linux/macOS/Windows.
Live-verified
Tested end-to-end with the review loop working from an Android phone over Tailscale: edits, element comments, deletions, and the write-back all round-tripped correctly.