diff --git a/.claude/resources/architecture.md b/.claude/resources/architecture.md index 6776280..3ef4221 100644 --- a/.claude/resources/architecture.md +++ b/.claude/resources/architecture.md @@ -20,7 +20,7 @@ Data flows outermost → innermost: `cli` parses → resolves a `target` → get - `result` — the envelope, `Err`, and the exit-code table. No dependencies; the root of the contract. - `target` — the target grammar (`idprefix | url: | title: | @N`) and `Resolve` against a tab list. - `config` — layered defaults: built-in < config file (`~/.config/chrome-cdp/config.toml`) < `CHROME_CDP_*` env < flag. `Builtin()`, `Resolve()`, `FromEnv()`. -- `browser` — endpoint discovery: finds Chrome's `DevToolsActivePort` file and computes the per-endpoint key (see connection model below). +- `browser` — endpoint discovery and classification: finds Chrome's `DevToolsActivePort` file, computes the per-endpoint key, and probes the debug endpoint's WebSocket upgrade (`WSState`, `AwaitUpgrade`) — see connection model below. - `chrome` — the `Browser` interface and its chromedp-backed implementation: snapshot, click/type/fill/select, grid, wait, raw CDP. The real driver logic lives here. - `chrometest` — `StubBrowser`, a permissive `chrome.Browser` double embedded by the `cli` and `daemon` tests. - `state` — the sticky current-target store, keyed per endpoint so distinct `--port`s don't share a "current tab". @@ -46,6 +46,16 @@ When adding a command that needs a new capability, add the method to the `chrome - The **daemon** (`chrome-cdp __daemon `, a hidden mode) holds one CDP connection for ~30 min and serves commands over a Unix socket, so the "Allow debugging?" consent fires once. `--no-daemon` bypasses it and connects directly (used by tests and one-shot scripts). - Chrome M136+ dropped the classic `--remote-debugging-port` for the default profile; `browser` reads `DevToolsActivePort` and connects directly, which is why it keeps working where older tools broke. +- The **consent prompt** is a third connection state, not a failure (RFC-0013). + While Chrome holds "Allow remote debugging?" it accepts the TCP connect and then stalls the WebSocket upgrade forever — no error, only silence — so `browser.WSState` is three-way (`WSRefused` / `WSPending` / `WSReady`) and `DecideConnection` maps an open-but-hanging endpoint to its own `ConsentPending` action. + The upgrade itself lives in `chrome/probe.go` (`AwaitUpgrade`, `ProbeWS`, `ResolveWSURL`), next to the connection it feeds; `browser` keeps the vocabulary and the ladder and stays free of I/O against Chrome. + The daemon holds that upgrade open for `consent_timeout` (default 120s, clamped to `[1s, 10m]` by `chrome.ClampConsentTimeout` where flag/env/config resolve) and publishes a `.pending` marker so `Ensure` extends its own deadline instead of declaring a live daemon dead; a refused endpoint still fails in milliseconds, which is what makes the long wait safe. + `WSState.String()` is the wire value `doctor` reports as `state`, so there is one vocabulary rather than a hand-maintained second list. + Never lead a failure message with the `chrome://inspect` toggle: `browser.EnableAdvice` is the one authored answer, and it recommends `--remote-debugging-port` first because that path never prompts. + `browser.ConsentPromptAdvice` is the matching one for describing the dialog itself — modal to the browser, behind the window, no other input accepted — and every message that mentions the prompt composes it rather than rewriting it. +- **Anything `doctor` reports is a claim it has to have verified**, and anything it echoes ends up in an agent transcript (the Skill runs `doctor --json` first). + A daemon's `running: true` is not evidence — the socket outlives the connection — so `__status` reports `connected` from the `List` round trip it makes, and `doctor` requires that before saying `ready`. + The status payload carries a `target_count`, never the tab list: titles and URLs are not an answer to "can I connect?". ## Human vs. JSON rendering diff --git a/README.md b/README.md index 6cddce2..0354b5b 100644 --- a/README.md +++ b/README.md @@ -30,9 +30,19 @@ Because it attaches to the browser you're already using, an app you're signed in Recent Homebrew may print a tap-trust notice for third-party taps on first install. The install still proceeds; to acknowledge it explicitly, run `brew trust --cask sanketsudake/tap/chrome-cdp` first. -2. **Let Chrome accept a debugger** — open `chrome://inspect/#remote-debugging` and toggle it on (a one-time consent; `chrome-cdp` never suppresses it). +2. **Let Chrome accept a debugger.** + Launch it with the flag — this never prompts: -3. **Check the connection** — `chrome-cdp doctor` confirms it's ready, or prints the exact fix. + ```sh + open -a "Google Chrome" --args --remote-debugging-port=9222 # macOS + google-chrome --remote-debugging-port=9222 # Linux + ``` + + Or, to attach to a Chrome that is already running on the default profile, toggle `chrome://inspect/#remote-debugging` on. + That path raises a consent prompt on **every fresh attach**, and the prompt is modal to the whole browser — until it is answered Chrome accepts no other input, and it can sit behind the window, so an unanswered one looks like a crash. + `chrome-cdp` waits for it (see `--consent-timeout`) and never suppresses it. + +3. **Check the connection** — `chrome-cdp doctor` actually connects and reports `ready`, `consent_pending`, or `no_endpoint`, with the exact fix. 4. **Drive it:** @@ -113,7 +123,7 @@ Shell completion is built in: `chrome-cdp completion bash|zsh|fish|powershell`. ## Security -A live debug endpoint is **full control** of whatever your Chrome is signed into — treat enabling `chrome://inspect` like opening a local root shell into your browser's sessions, and only do it when you intend to automate. +A live debug endpoint is **full control** of whatever your Chrome is signed into — treat enabling remote debugging (by flag or by toggle) like opening a local root shell into your browser's sessions, and only do it when you intend to automate. - **Loopback only.** It connects to `127.0.0.1` and never binds the debug port to a non-loopback interface. diff --git a/cmd/chrome-cdp/connect.go b/cmd/chrome-cdp/connect.go new file mode 100644 index 0000000..e753b2f --- /dev/null +++ b/cmd/chrome-cdp/connect.go @@ -0,0 +1,35 @@ +package main + +import ( + "fmt" + "io" + + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" + "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" + "github.com/sanketsudake/chrome-cdp-cli/internal/cli" + "github.com/sanketsudake/chrome-cdp-cli/internal/config" +) + +// directConnectOptions builds the chrome.Options for a --no-daemon connect. +// +// It exists as its own function for the hook at the bottom. Options. +// OnConsentPending was only ever assigned by the daemon, so the --no-daemon +// path sat in complete silence for the whole consent budget — up to two +// minutes during which the user's browser is frozen, the tool has printed +// nothing, and RFC-0013's US-2 ("tell me what is happening") is unmet on the +// one path where nothing else can tell them: there is no daemon to publish a +// .pending marker and no Ensure to read it. +func directConnectOptions(portFile string, o cli.ConnOpts, defs config.Defaults, w io.Writer) chrome.Options { + return chrome.Options{ + PortFile: portFile, NoLaunch: o.NoLaunch, ProfileDir: o.ProfileDir, Port: o.Port, + ConsentTimeout: o.ConsentTimeout, + // Fires once, the moment the upgrade is classified as pending — while + // the dialog is still on screen, which is the only time saying so helps. + OnConsentPending: func() { + fmt.Fprintln(w, "chrome-cdp:", browser.ConsentPromptAdvice) + }, + ConsoleBuffer: defs.ConsoleBuffer, ConsoleMaxEntry: defs.ConsoleMaxEntry, + NetBuffer: defs.NetBuffer, NetMaxBody: defs.NetMaxBody, + RecordBuffer: defs.RecordBuffer, RecordMaxBytes: defs.RecordMaxBytes, + } +} diff --git a/cmd/chrome-cdp/connect_test.go b/cmd/chrome-cdp/connect_test.go new file mode 100644 index 0000000..e50d84b --- /dev/null +++ b/cmd/chrome-cdp/connect_test.go @@ -0,0 +1,30 @@ +package main + +import ( + "bytes" + "strings" + "testing" + + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" + "github.com/sanketsudake/chrome-cdp-cli/internal/cli" + "github.com/sanketsudake/chrome-cdp-cli/internal/config" +) + +// TestDirectConnectAnnouncesThePendingPrompt is US-2 on the --no-daemon path. +// +// chrome.Options.OnConsentPending was assigned in exactly one place, inside the +// daemon, and the --no-daemon connect passed a ConsentTimeout with no hook. So +// that path waited out the prompt in complete silence — up to two minutes with +// a frozen browser and nothing on stderr — and no test caught it, because the +// tests that cover the hook supply their own. +func TestDirectConnectAnnouncesThePendingPrompt(t *testing.T) { + var buf bytes.Buffer + opts := directConnectOptions("", cli.ConnOpts{}, config.Builtin(), &buf) + if opts.OnConsentPending == nil { + t.Fatal("--no-daemon connects with no pending hook: the user is told nothing for the whole consent budget") + } + opts.OnConsentPending() + if !strings.Contains(buf.String(), browser.ConsentPromptAdvice) { + t.Errorf("the --no-daemon notice does not carry browser.ConsentPromptAdvice:\n%s", buf.String()) + } +} diff --git a/cmd/chrome-cdp/main.go b/cmd/chrome-cdp/main.go index 0a214d5..bd433fa 100644 --- a/cmd/chrome-cdp/main.go +++ b/cmd/chrome-cdp/main.go @@ -4,7 +4,6 @@ package main import ( "context" "fmt" - "maps" "os" "strconv" "time" @@ -31,6 +30,7 @@ func main() { ProfileDir: env.ProfileDir, Port: env.Port, NoLaunch: env.NoLaunch, + ConsentTimeout: env.ConsentTimeout, ConsoleBuffer: env.ConsoleBuffer, ConsoleMaxEntry: env.ConsoleMaxEntry, NetBuffer: env.NetBuffer, @@ -103,6 +103,12 @@ func main() { if o.NoLaunch { env = append(env, "CHROME_CDP_NO_LAUNCH=1") } + // The daemon is the process that actually waits out the consent prompt, + // so --consent-timeout has to reach it; it only ever parses the + // environment. Forwarded unconditionally: o.ConsentTimeout is already + // normalised, and a "> 0" guard here is how the client and the daemon + // it spawned ended up waiting for different lengths of time. + env = append(env, "CHROME_CDP_CONSENT_TIMEOUT="+o.ConsentTimeout.String()) // The daemon parses only the environment, so config-file values for the // event-capture bounds have to be forwarded explicitly or the buffers it // holds would silently fall back to the built-in sizes. @@ -119,14 +125,9 @@ func main() { app.WithConnector(func(ctx context.Context, o cli.ConnOpts) (chrome.Browser, error) { if o.NoDaemon { - return chrome.Connect(ctx, chrome.Options{ - PortFile: portFile, NoLaunch: o.NoLaunch, ProfileDir: o.ProfileDir, Port: o.Port, - ConsoleBuffer: defs.ConsoleBuffer, ConsoleMaxEntry: defs.ConsoleMaxEntry, - NetBuffer: defs.NetBuffer, NetMaxBody: defs.NetMaxBody, - RecordBuffer: defs.RecordBuffer, RecordMaxBytes: defs.RecordMaxBytes, - }) + return chrome.Connect(ctx, directConnectOptions(portFile, o, defs, os.Stderr)) } - client, err := daemon.Ensure(socketFor(o), exe, daemonEnv(o)) + client, err := daemon.Ensure(ctx, socketFor(o), exe, daemonEnv(o), o.ConsentTimeout) if err != nil { return nil, err } @@ -136,7 +137,7 @@ func main() { app.WithDaemonCtl( func(o cli.ConnOpts) (map[string]any, error) { sock := socketFor(o) - if _, err := daemon.Ensure(sock, exe, daemonEnv(o)); err != nil { + if _, err := daemon.Ensure(context.Background(), sock, exe, daemonEnv(o), o.ConsentTimeout); err != nil { return nil, err } return map[string]any{"started": true, "socket": sock, "endpoint": browser.EndpointKey(portFile, o.Port)}, nil @@ -152,7 +153,7 @@ func main() { return map[string]any{"stopped": true}, nil }, func(o cli.ConnOpts) (map[string]any, error) { - return daemonStatus(socketFor(o), browser.EndpointKey(portFile, o.Port)) + return daemon.Status(socketFor(o), browser.EndpointKey(portFile, o.Port)) }, ) @@ -160,19 +161,3 @@ func main() { app.Close() os.Exit(code) } - -// daemonStatus reports whether the daemon for this endpoint is running and, when -// it is, what it's attached to (the live tab list, best-effort). -func daemonStatus(sock, endpoint string) (map[string]any, error) { - res := map[string]any{"socket": sock, "endpoint": endpoint} - c := daemon.TryConnect(sock) - if c == nil { - res["running"] = false - return res, nil - } - res["running"] = true - if info, err := c.StatusInfo(); err == nil { - maps.Copy(res, info) - } - return res, nil -} diff --git a/config.example.toml b/config.example.toml index 1866cd0..5dea026 100644 --- a/config.example.toml +++ b/config.example.toml @@ -12,6 +12,18 @@ # target = "url:github" # default tab when neither --target nor `use` is set # json = false # emit the machine-readable JSON envelope by default # timeout = "30s" # max time to wait per command (Go duration string) +# consent_timeout = "120s" # how long to hold the connection open waiting for Chrome's +# # browser-modal "Allow remote debugging?" prompt to be answered. +# # Only an OPEN port whose upgrade is hanging waits this long; a +# # refused endpoint still fails in milliseconds. Shorten it if you +# # only ever launch Chrome with --remote-debugging-port (which +# # never prompts); lengthen it if the dialog tends to hide behind +# # the window and you want more time to find it. +# # Clamped to 1s-10m. "0s" (or a negative value) means the 120s +# # default, not "do not wait" — a zero wait abandons the prompt +# # the instant it is raised, which is the failure this setting +# # exists to prevent. The ceiling is there because this is also +# # how long a second command can be held up behind the first. # no_launch = false # never auto-launch a managed fallback Chrome # no_daemon = false # connect directly instead of via the shared daemon # no_color = false # plain, symbol-free human output diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 433a575..fb654d2 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -20,12 +20,16 @@ Branch on the exit code, not on message text. | 0 | — | success | | 1 | generic | unclassified failure | | 2 | usage | bad flags or arguments | -| 3 | connection | attach / launch failed | +| 3 | connection | attach / launch failed, or Chrome's consent prompt is unanswered | | 4 | target/timeout | selector not found, timed out, or ambiguous/unknown target | | 5 | cdp | CDP protocol error | | 6 | daemon | daemon error | | 7 | permission_denied | refused by [policy](#policy) — the origin, the verb, or the upload path is out of bounds | +Exit 3 covers three `error.code` values: `connection_failed`, `not_debug_enabled`, and `consent_pending`. +The last one means Chrome accepted the connection and then went silent because it is holding its browser-modal "Allow remote debugging?" dialog — nothing is broken, a human has not answered yet. +It is a distinct code on the same number because the remedy is "click the dialog", not "check your setup"; the numbers are contract and do not grow for a new failure mode. + Exit 7 is deliberately distinct from exit 4: an agent has to be able to tell "policy forbids this, stop and tell the user" from "element not found, retry differently". Without `--json` the same information renders as a short human line (result to stdout, errors to stderr). @@ -39,6 +43,7 @@ These apply to every command. | `--json` | off | one JSON value to stdout | | `--target ` | sticky tab | tab to act on (see [Targeting](#targeting-a-tab)) | | `--timeout ` | `30s` | max time to wait for the command | +| `--consent-timeout ` | `120s` | how long to wait for Chrome's "Allow remote debugging?" prompt (a refused endpoint still fails fast) | | `--by ` | `css` | selector syntax (see [Addressing](#addressing-elements)) | | `--wait ` | `visible` | element wait: `visible` \| `ready` \| `enabled` | | `--no-wait` | off | act immediately; fail fast instead of waiting | @@ -873,7 +878,7 @@ chrome-cdp raw Browser.getVersion --browser # browser-level met | Command | Does | |---------|------| -| `doctor` | check the connection and print the exact fix if it's not ready | +| `doctor` | probe the connection, report `ready` \| `consent_pending` \| `no_endpoint` \| `unverified`, and print the exact fix (`--no-probe` to connect to nothing) | | `daemon start\|stop\|status` | manage the background connection | | `policy init` | write a starter [`[policy]`](#policy) table allow-listing the current tab's origin (`--wildcard`, `--print`, `-o`) | | `exit-codes` | print the exit-code table | @@ -882,13 +887,71 @@ chrome-cdp raw Browser.getVersion --browser # browser-level met ## Connection model -`chrome-cdp` attaches to your **real** Chrome via the one-time `chrome://inspect/#remote-debugging` toggle; it reads Chrome's `DevToolsActivePort` file and connects the WebSocket directly (the classic `--remote-debugging-port` flag no longer works on the default profile since Chrome M136). +`chrome-cdp` attaches to your **real** Chrome. +There are two ways to let it, and they are not equivalent. + +**Recommended — launch Chrome with the flag.** +It never prompts: + +```sh +open -a "Google Chrome" --args --remote-debugging-port=9222 # macOS +google-chrome --remote-debugging-port=9222 # Linux +``` + +**Alternative — the `chrome://inspect/#remote-debugging` toggle.** +It works on the default profile where the classic flag does not (Chrome M136+ dropped `--remote-debugging-port` for the *default* profile, which is why the toggle exists), but it raises a consent prompt on **every fresh attach**. +That prompt is browser-modal: until it is answered, Chrome accepts no other input, so an unanswered one looks exactly like a crashed browser. +Read [The consent prompt](#the-consent-prompt) before choosing it. + +Either way, `chrome-cdp` reads Chrome's `DevToolsActivePort` file and connects the WebSocket directly. If no debug-enabled Chrome is found, it launches a managed Chrome on a dedicated profile alongside your real one. -A background **daemon** holds the connection, so Chrome's "Allow debugging?" prompt appears once per session rather than once per command. +A background **daemon** holds the connection, so the consent prompt appears once per session rather than once per command. It starts lazily on first use and idles out after 30 minutes; manage it with `daemon start|stop|status`, or bypass it with `--no-daemon`. -Run `chrome-cdp doctor` to check the connection and get the exact fix when it isn't ready. +### The consent prompt + +On the `chrome://inspect` path, a fresh attach makes Chrome ask "Allow remote debugging?". +Three things about it are worth knowing before it happens: + +- It is **modal to the whole browser**, not to a tab. + Nothing else in Chrome responds until it is answered. +- It can sit **behind** the Chrome window, so the usual experience is a browser that appears frozen with no visible dialog. +- Answering it late is fine. + `chrome-cdp` holds the connection open for `--consent-timeout` (default 120s) and connects the moment you click Allow. + +If the wait runs out you get exit 3 with `error.code: consent_pending` and a message naming the dialog. +A **refused** endpoint is unaffected by any of this and still fails in milliseconds — only an open port whose upgrade is hanging earns the long wait. + +While the prompt is up, `chrome-cdp` says so on stderr rather than waiting silently, on the daemon path and with `--no-daemon` alike. +A second command started during the wait says that it is queueing behind the first rather than opening a second connection, and if the first gives up with `consent_pending` the ones behind it inherit that answer instead of each raising a fresh prompt. + +`--consent-timeout` (config key `consent_timeout`) is clamped to between `1s` and `10m`. +`0s` or a negative value means the 120s default rather than "do not wait", which would abandon the prompt the moment it was raised; the ceiling exists because the value is also how long a queued command can be held up. + +### `doctor` + +`chrome-cdp doctor` answers "can I connect?" by connecting, and reports one of four states: + +| `state` | Means | Envelope | +|---------|-------|----------| +| `ready` | the WebSocket upgrade completed, or a running daemon answered a live CDP round trip | `ok: true` | +| `consent_pending` | the port accepted and went silent — Chrome is holding the prompt | exit 3, `consent_pending` | +| `no_endpoint` | nothing usable answered (no port file, a stale one, or another process on the port) | exit 3, `connection_failed` | +| `unverified` | `--no-probe`: an endpoint exists and nothing was checked | `ok: true` | + +When the daemon is running AND it has just proved its connection to Chrome, `doctor` answers **through it** (`via: "daemon"`) and opens no new connection — probing is itself a connection request, and on the toggle path that is what raises the prompt. +A daemon that is merely *running* is not an answer: it holds its socket for its whole idle window, so quitting Chrome leaves a reachable daemon with a dead connection behind it, and `doctor` falls through to the probe rather than reporting ready. +Otherwise it says on stderr that it is about to connect, then probes (`via: "probe"`). +`--no-probe` reports only what the port file says, clearly marked `state: "unverified"`. + +`doctor` honours `--port` like every other verb: `doctor --port 9333` diagnoses the Chrome on that port, not whichever one the `DevToolsActivePort` file names. + +A `ready` verdict reached by probing says so: the probe's own connection is closed once it has its answer, so on the toggle path the next command is a fresh attach and can prompt again. +Run `chrome-cdp daemon start` to be asked once per session instead. + +The result carries `state`, `via`, `probed`, and the endpoint it looked at (`endpoint`, plus `port_file` and `ws` where they apply). +The daemon-backed answer adds `running`, `connected`, `socket`, and `target_count` — a **count**, not the tab list: `doctor --json` is the first thing many callers run, and open tab titles and URLs are not an answer to "can I connect?". ## Policy @@ -1051,6 +1114,7 @@ Persist flags you'd otherwise retype in `$XDG_CONFIG_HOME/chrome-cdp/config.toml ```toml json = true # default to machine-readable output timeout = "10s" +consent_timeout = "2m" # how long to wait for Chrome's consent prompt (1s-10m; 0 means the 120s default) by = "search" # default selector syntax target = "url:github" # default tab when neither --target nor `use` is set ``` diff --git a/docs/rfc/0013-consent-prompt-lifecycle.md b/docs/rfc/0013-consent-prompt-lifecycle.md new file mode 100644 index 0000000..adb2326 --- /dev/null +++ b/docs/rfc/0013-consent-prompt-lifecycle.md @@ -0,0 +1,182 @@ +# RFC-0013: Surviving Chrome's consent prompt + +- **Status:** Draft +- **Priority:** P0 +- **Area:** connection +- **Depends on:** the spawn serialisation in #17 (necessary, not sufficient) + +## Summary + +Stop `chrome-cdp` from wedging the user's browser on the one-time "Allow remote debugging?" prompt. +Three changes: wait for the consent it asks for instead of abandoning it, detect the pending state instead of reporting a generic failure, and prefer the connection path that never prompts. + +## What happened + +A user's Chrome froze with the consent dialog on screen and no button responding. +It was reproduced deliberately afterwards, and the reproduction contradicted the first diagnosis. + +The initial theory was **stacked prompts**: several `chrome-cdp` processes had started at once, each found no daemon, each spawned one, and each spawned daemon attached to Chrome and raised its own prompt. +That much is real, and #17 fixes it — eight concurrent callers produced eight daemons before the fix and one after. + +But the controlled reproduction showed a **single** prompt wedges Chrome just as thoroughly. +Serialising the spawn is necessary and not sufficient. + +### What the reproduction established + +Measured against a real Chrome made debug-enabled through the `chrome://inspect/#remote-debugging` toggle: + +| Probe | While consent is pending | +|-------|--------------------------| +| TCP connect to `127.0.0.1:9222` | succeeds immediately | +| `GET /json/version` | 404 in under a millisecond | +| WebSocket upgrade to the browser endpoint | **hangs — never completes, never refuses** | + +The hang is the mechanism. +Chrome does not reject the connection while it waits for the user; it holds the upgrade open and says nothing. +So the client has no error to classify — only silence — which is why the failure surfaces as an undifferentiated timeout. + +Three consequences followed, each independently a defect: + +1. **The daemon abandons the prompt it raised.** + `chrome.Connect` dials, the upgrade hangs, the dial times out in about ten seconds, and the daemon writes its error and exits. + The modal is left on screen with nothing behind it. + Clicking Allow then grants consent to a connection that no longer exists. +2. **Ten seconds is not a human timescale for a dialog that may be invisible.** + The prompt is browser-modal and can sit behind the window. + A user who has not clicked it has usually not *seen* it, and by the time they do, the process that asked is gone. +3. **`doctor` reports readiness it never verified.** + It reads the `DevToolsActivePort` file and reports "debug endpoint reachable — Path B attach ready", handing back a `ws://` URL. + It does not probe. + The one command whose job is to answer "can I connect?" answered yes while every connection was hanging. + +A fourth observation is recorded because it cost time: **`/json/version` returning 404 is not a consent signal.** +It returns 404 in this connection mode whether or not consent has been granted — the toggle path exposes the WebSocket without the HTTP JSON API. +An early reading of that 404 as "consent pending" was wrong, and any detection built on it would be too. + +## User stories + +**US-1 — Do not freeze my browser.** +As a user, I want a tool that asks for consent to still be there when I answer, so that a prompt I did not see immediately does not leave my browser unusable. +*Acceptance:* a prompt answered minutes after it appears results in a working connection, not an orphaned dialog. + +**US-2 — Tell me what is happening.** +As a user staring at an unresponsive browser, I want to be told a consent prompt is pending and where to find it, so that I know this is a dialog and not a crash. +*Acceptance:* while the upgrade is hanging, the CLI reports a distinct pending state naming the prompt, not a generic connection failure. +This holds on every path that can wait: the daemon, `--no-daemon`, and a second command queued behind the first — which says it is queueing rather than blocking in silence. + +**US-3 — Do not ask at all when you do not have to.** +As a user, I want to be steered to the launch flag that skips consent entirely, so that routine use never involves a modal. +*Acceptance:* when no usable endpoint exists, the CLI recommends `--remote-debugging-port` before it recommends the toggle that prompts. + +**US-4 — Recover without losing my tabs.** +As a user whose browser is already wedged, I want to be told the actual remedy, so that I am not left force-quitting on a guess. +*Acceptance:* the failure message names the recovery and does not imply the browser has crashed. + +**US-5 — One prompt, not many.** +As a user running several commands at once, I want at most one consent request. +*Acceptance:* covered by #17 for concurrent spawns; this RFC keeps it true across the wait as well. +Serialising the spawn only guarantees one prompt *at a time*: without more, each queued caller in turn cleared the previous verdict and raised its own, so eight commands became eight sequential prompts. +A `consent_pending` verdict is inherited by callers released within a few seconds of it, so a queue drains on one answer. + +## Proposed changes + +### 1. Wait for the consent + +The daemon's initial connect must outlive the dialog. +Replace the single short dial with a bounded wait — proposal: `consent_timeout`, default **120s** — during which the daemon stays alive and keeps the pending upgrade open. + +Distinguishing the pending state from a dead endpoint is what makes a long wait safe. +A refused TCP connection is a real failure and must stay fast; a hanging *upgrade* against an *open* port is the consent signature and is the only case that earns the long wait. + +### 2. Report the pending state + +Add `CodeConsentPending = "consent_pending"` mapped to the existing connection exit code, so a caller can branch on it without a new number. + +While waiting, the CLI says a consent prompt is pending, that it is browser-modal and may be behind the window, and that Chrome will accept no other input until it is answered. +That last clause is the part a user cannot deduce, and is why a frozen browser reads as a crash. + +### 3. Make `doctor` probe + +`doctor` must attempt the upgrade rather than trusting the port file, and report one of: no endpoint, consent pending, or ready (plus `unverified`, which is what `--no-probe` reports and is explicitly not an answer). +A diagnostic that reports readiness without testing it is worse than no diagnostic, because it sends the user looking somewhere else. + +Probing is itself a connection request, so `doctor` must reuse a live daemon when one exists rather than raising a prompt of its own. +"Live" means the daemon has just completed a round trip to Chrome, not that its socket answered: the socket outlives the connection by up to the daemon's whole idle window, so `running: true` is the same unverified claim as the port file, one level up. + +Two consequences of `doctor` being the first thing many callers run: + +- It reports a **count** of open tabs, never their titles or URLs. + The Agent Skill makes `doctor --json` step 1 of every session, and a diagnostic that answers "can I connect?" with a list of the user's OAuth callbacks and reset tokens is answering a question nobody asked. +- A `ready` reached by probing says what it cost. + The probe closes its own connection, so on the toggle path the next command is a fresh attach and prompts again — a verdict falsified by the act of producing it, unless it is disclosed. + +`doctor` resolves `--port` like every other verb. +Diagnosing a different browser than the one the flag names is the same class of error as diagnosing one nobody connected to. + +### 4. Prefer the path that never prompts + +When no usable endpoint exists, recommend relaunching Chrome with `--remote-debugging-port=9222` **first**, and the `chrome://inspect` toggle second with a note that it prompts on every fresh attach. +The toggle is currently presented as the primary route, which routes every new user through the failure this RFC exists to remove. + +## Verification scenarios + +**VS-1 — A hanging upgrade is classified as consent pending, not as a timeout.** +Given a listener that accepts TCP and never completes the WebSocket upgrade, when the daemon connects, then it reports `consent_pending` and stays alive rather than exiting. + +**VS-2 — A refused endpoint still fails fast.** +Given a closed port, when the daemon connects, then it fails within a second or two with `connection_failed`, not after the consent timeout. + +**VS-3 — Consent answered late still works.** +Given a listener that completes the upgrade after 30 seconds, when the daemon connects, then the connection succeeds and no prompt is orphaned. + +**VS-4 — The consent wait is bounded.** +Given a listener that never completes the upgrade, when `consent_timeout` elapses, then the daemon exits with a message naming the prompt and the recovery. + +**VS-5 — `doctor` distinguishes all three states.** +Table over: no endpoint, open-but-hanging, and ready — each reported distinctly, and the ready case verified by a completed upgrade rather than by the port file alone. +"Verified" excludes every proxy for a completed round trip, including a daemon that is merely running. + +**VS-6 — `doctor` does not raise its own prompt.** +Given a running daemon, when `doctor` runs, then it answers through the daemon and initiates no new connection. + +**VS-8 — the probe reads what it is owed and no more.** +Given a listener that accepts and then streams bytes with no newline, the probe classifies it as refused at its read limit rather than accumulating for the whole consent budget. +Chrome's debug port is a loopback port any local process can bind, and the budget this RFC introduces is what turns an unbounded read into gigabytes. + +**VS-9 — a 101 that is not our handshake is not ready.** +Given a listener answering `101` without a valid `Sec-WebSocket-Accept` for the key we sent, the endpoint is classified refused. + +**VS-7 — Concurrency stays at one prompt.** +The guard from #17, restated here so this RFC's changes cannot regress it. + +## Test plan + +The pending state is a **local listener that accepts and stalls**, so almost all of this is testable with `net.Listen` and no browser at all — which matters, because the manual reproduction wedged a real browser twice and must not be the regression test. + +- **Pure/stub (`-short`):** VS-1 through VS-5 against hand-built listeners — refusing, stalling, and completing-after-a-delay. + This is where the classification logic belongs. +- **Daemon:** VS-6 and VS-7 against the existing socket harness. +- **Live Chrome:** none. + Consent cannot be granted programmatically, and a test that needs a human click is not a test. + +Note for anyone extending this: a long `t.TempDir()` path breaks a Unix socket bind on darwin — `sun_path` caps near 104 bytes and the directory embeds the test name, which fails with a bare `bind: invalid argument`. + +## Out of scope + +- Granting or suppressing consent programmatically. + It is a deliberate user decision and the tool should not try to route around it. +- Anything about Chrome's own modal behaviour, which is not ours to change. +- Windows: the project ships linux and darwin only. + +## Open questions + +1. Should the CLI **refuse to raise a prompt at all** unless the user opts in — erroring with "relaunch with `--remote-debugging-port`" instead? + Safest, but it removes the zero-config path that makes the tool pleasant on first use. + **Recommendation:** keep the prompt, fix the waiting, and lead with the flag in the docs. +2. Is 120s the right consent timeout? + Long enough for a hidden dialog, short enough that a genuinely dead endpoint is not mistaken for a slow human. + **Recommendation:** 120s, as a config key so it can be argued with. + **Resolved:** 120s, clamped to `[1s, 10m]` and normalised once where flag, environment and config file resolve. + The clamp is not tidiness: `0s` meant "the default" to one layer and "do not wait" to another, which restored the orphaned-prompt failure through the parameter's own zero value, and an inherited `CHROME_CDP_CONSENT_TIMEOUT=8760h` would hold the spawn lock — and therefore every other command — for a year. +3. Should `doctor` be able to probe *without* a daemon, accepting that it may raise a prompt? + **Recommendation:** yes, but say so before doing it, since the user ran a diagnostic and did not ask to connect. diff --git a/docs/rfc/README.md b/docs/rfc/README.md index f61b5d7..31e227b 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -40,6 +40,7 @@ The ordering below follows that: make the interaction surface complete, then mak | [0010](0010-page-reading-ergonomics.md) | Page-reading ergonomics: `text --article`, `eval --await` | P2 | reading | Draft | | [0011](0011-session-recording.md) | Session recording: `record` and GIF export | P2 | capture | Draft | | [0012](0012-domain-allowlist.md) | Domain allow-list: bounding what the CLI may drive | P2 | safety | Draft | +| [0013](0013-consent-prompt-lifecycle.md) | Surviving Chrome's consent prompt | P0 | connection | Draft | ## Dependency graph diff --git a/docs/using-with-ai-agents.md b/docs/using-with-ai-agents.md index 6380ce8..e96d91a 100644 --- a/docs/using-with-ai-agents.md +++ b/docs/using-with-ai-agents.md @@ -27,7 +27,7 @@ On failure, `ok` is `false`, `error{code,message,details}` explains it, and the |-----:|---------|------------------------| | 0 | success | continue | | 2 | bad flags/args | fix the call | -| 3 | connection | ask the user to enable `chrome://inspect` | +| 3 | connection | check `error.code`: `consent_pending` means a modal "Allow remote debugging?" dialog is unanswered (tell the user it is behind the window and freezes all of Chrome); otherwise ask them to relaunch Chrome with `--remote-debugging-port=9222` | | 4 | not found / timeout / ambiguous | re-`snap` and retry, or foreground the tab | | 5 | CDP error | surface it | diff --git a/internal/browser/browser.go b/internal/browser/browser.go index 6d3f6fd..73ae49f 100644 --- a/internal/browser/browser.go +++ b/internal/browser/browser.go @@ -1,6 +1,10 @@ // Package browser holds the connection-layer logic for reaching Chrome over CDP: -// the DevToolsActivePort reader (Path B) and the connection-ladder decision. -// It is deliberately free of chromedp so it unit-tests without a live browser. +// the DevToolsActivePort reader (Path B), the endpoint resolution, the +// vocabulary a probe answers in (WSState), and the connection-ladder decision. +// +// It is deliberately free of chromedp AND of any I/O against Chrome itself, so +// it unit-tests without a live browser. The socket work that classifies an +// endpoint used to live here and no longer does: see chrome.AwaitUpgrade. package browser import ( @@ -77,6 +81,41 @@ func FindPortFile(override string) string { return "" } +// Endpoint is where a command should try to reach Chrome, and how that was +// decided. It exists because two callers have to agree on it: chrome.Connect, +// which attaches, and `doctor`, which diagnoses. doctor used to read the port +// file directly and ignore --port entirely, so `doctor --port 9333` probed a +// different browser than the one the flag named and reported IT healthy. +type Endpoint struct { + // URL is the ws:// (port-file path) or http:// (explicit --port) endpoint, + // or "" when none was found. + URL string + // PortFile is the DevToolsActivePort file URL came from, "" when an + // explicit --port was used (there is no file in that case). + PortFile string + // Err is set when a port file was found and could not be read or parsed — + // distinct from "no endpoint", because the remedy is different. + Err error +} + +// FindEndpoint resolves the debug endpoint from an explicit port, else the +// DevToolsActivePort file. An explicit --port wins: it names a specific Chrome, +// and a port file naming a different one is not a fallback for it. +func FindEndpoint(portFileOverride string, port int) Endpoint { + if port != 0 { + return Endpoint{URL: fmt.Sprintf("http://127.0.0.1:%d", port)} + } + pf := FindPortFile(portFileOverride) + if pf == "" { + return Endpoint{} + } + ws, err := WSURLFromPortFile(pf) + if err != nil { + return Endpoint{PortFile: pf, Err: err} + } + return Endpoint{URL: ws, PortFile: pf} +} + // EndpointKey identifies the debug endpoint a command targets, so the daemon // socket and sticky state are keyed to the actual Chrome instance rather than a // fixed port. An explicit --port wins (distinct ports get distinct keys); @@ -106,16 +145,97 @@ func HostPort(wsURL string) (string, bool) { return hp, hp != "" } +// EnableAdvice is the single authored answer to "how do I make Chrome +// debuggable?", and it leads with the launch flag ON PURPOSE. +// +// --remote-debugging-port skips the consent dialog entirely. The +// chrome://inspect toggle raises a browser-modal prompt on every fresh attach, +// and every message in this tool used to recommend it first — which routed each +// new user straight through the failure RFC-0013 exists to remove. The order of +// these two clauses is the fix. +const EnableAdvice = "relaunch Chrome with --remote-debugging-port=9222 " + + "(on macOS: open -a \"Google Chrome\" --args --remote-debugging-port=9222), which never prompts; " + + "or enable chrome://inspect/#remote-debugging, which raises a consent prompt on every fresh attach" + +// ConsentPromptAdvice is the single authored explanation of Chrome's consent +// dialog, for every place that has to describe it: the connect timeout, the +// generic dial failure, the daemon's wait notice, the client's give-up message, +// and doctor's consent_pending state. +// +// Every clause is here because a user cannot deduce it. That the dialog is +// modal to the BROWSER is why the frozen window is a symptom and not a crash; +// that it can sit behind the window is why they have not seen it; that nothing +// else in Chrome responds until it is answered is why the tool looks like the +// thing that broke. Five hand-written copies had already drifted — one said +// "behind" where the others shouted it, one said "blocks all other input" — +// and two test files asserted on a substring list that one of those copies +// would have failed. +const ConsentPromptAdvice = "Chrome is holding its \"Allow remote debugging?\" consent prompt. " + + "The prompt is browser-modal and can sit BEHIND the Chrome window, and Chrome accepts no other input until it is answered, " + + "so a browser that looks frozen or crashed is usually this dialog. Find it and click Allow." + +// WSState is what one WebSocket upgrade against Chrome's browser-level debug +// endpoint actually did. It is three-way, and that is the whole point. +// +// While consent for a fresh attach is pending, Chrome does not refuse the +// connection: it accepts the TCP connect, then holds the upgrade open and says +// nothing until the user answers a browser-modal dialog. There is no error to +// classify — only silence. A boolean "reachable" collapses that silence into the +// same value as a refused port, so the tool cannot tell "nothing is listening" +// (a real failure, and fast) from "Chrome is waiting for a human" (not a failure +// at all, and slow by nature). Splitting them is what lets a refused endpoint +// keep failing in milliseconds while a pending one is waited out for minutes. +// +// Note that Chrome's HTTP JSON API is NOT a substitute signal: on the +// chrome://inspect toggle path GET /json/version answers 404 whether or not +// consent has been granted. Only the upgrade distinguishes the states. +type WSState int + +const ( + // WSRefused: nothing accepted the connection, or something answered the + // upgrade with anything other than 101 (a stale port file, a different + // server on the port). A real failure. + WSRefused WSState = iota + // WSPending: the port accepted and the upgrade never completed. This is the + // consent signature. + WSPending + // WSReady: the upgrade completed — the endpoint is live and consented. + WSReady +) + +// String is the WIRE value of this state: it is what `doctor` reports as the +// envelope's `state` field, and callers branch on it. There is exactly one +// vocabulary for these three answers on purpose. There used to be two — this +// method said "refused" where doctor said "no_endpoint" — kept apart only by +// this method having no callers at all, which is not a design, it is a +// coincidence that was one use away from shipping two names for one state. +func (s WSState) String() string { + switch s { + case WSPending: + return "consent_pending" + case WSReady: + return "ready" + default: + // Refused is reported as no_endpoint because that is what it means to + // the user: nothing usable is there. The distinction the probe cares + // about — refused versus silent — is already carried by WSPending. + return "no_endpoint" + } +} + // Action is the connection-ladder outcome for a given Probe. type Action int const ( - Attach Action = iota // attach to Probe.PortFileWS (Path B) + Attach Action = iota // attach to Probe.Endpoint (Path B) Launch // launch a managed Chrome (Path A fallback) - InstructToggle // Chrome is running but not debug-enabled — guide chrome://inspect + InstructToggle // Chrome is running but not debug-enabled — guide the launch flag / chrome://inspect InstructNoLaunch // nothing debug-enabled and --no-launch — print the launch command + ConsentPending // open port, hanging upgrade — Chrome is holding its consent prompt ) +// String names an Action. Its only callers are %v in test failure messages, +// which is reason enough: the alternative is a diff that says "= 4, want 2". func (a Action) String() string { switch a { case Attach: @@ -126,6 +246,8 @@ func (a Action) String() string { return "instruct-toggle" case InstructNoLaunch: return "instruct-no-launch" + case ConsentPending: + return "consent-pending" default: return "unknown" } @@ -133,20 +255,37 @@ func (a Action) String() string { // Probe captures the observable connection state the ladder decides on. type Probe struct { - PortFileWS string // ws:// from DevToolsActivePort, or "" if unavailable - WSReachable bool // did a WS connect to PortFileWS succeed? - ChromeRunning bool // is a Chrome process running (possibly without debug)? - NoLaunch bool // the --no-launch flag + // Endpoint is where Chrome was looked for: a ws:// URL from + // DevToolsActivePort, an http:// one from --port, or "" if neither + // resolved. It was called PortFileWS, which was untrue of half the values + // it holds. + // + // The ladder does not read it: WS already implies it, since an upgrade can + // only be attempted against an endpoint. It is here because a Probe is a + // record of what was observed, and "where" is part of that. + Endpoint string + WS WSState // what one WebSocket upgrade against Endpoint did + ChromeRunning bool // is a Chrome process running (possibly without debug)? + NoLaunch bool // the --no-launch flag } // DecideConnection walks the connection ladder: -// 1. reachable debug endpoint -> Attach (Path B) -// 2. no reachable endpoint, Chrome up -> InstructToggle (use the running session; never shadow it) -// 3. no reachable endpoint, no Chrome -> Launch (Path A) unless --no-launch -// 4. ...with --no-launch -> InstructNoLaunch +// 1. upgrade completed -> Attach (Path B) +// 2. open port, hanging upgrade -> ConsentPending (Chrome is asking the user, not failing) +// 3. no reachable endpoint, Chrome up -> InstructToggle (use the running session; never shadow it) +// 4. no reachable endpoint, no Chrome -> Launch (Path A) unless --no-launch +// 5. ...with --no-launch -> InstructNoLaunch +// +// Rungs 1 and 2 are separate only because WS is three-way: see WSState. func DecideConnection(p Probe) Action { - if p.PortFileWS != "" && p.WSReachable { + // No guard on Endpoint being set: WS is WSRefused unless an upgrade was + // actually attempted, and an upgrade is only attempted against an endpoint, + // so the check was restating its own precondition. + switch p.WS { + case WSReady: return Attach + case WSPending: + return ConsentPending } if p.ChromeRunning { return InstructToggle diff --git a/internal/browser/browser_test.go b/internal/browser/browser_test.go index 488ce0d..02e306b 100644 --- a/internal/browser/browser_test.go +++ b/internal/browser/browser_test.go @@ -59,10 +59,19 @@ func TestDecideConnection(t *testing.T) { p Probe want Action }{ - {"reachable debug endpoint -> attach (Path B)", - Probe{PortFileWS: "ws://127.0.0.1:9222/x", WSReachable: true}, Attach}, - {"stale port file + chrome running -> instruct toggle", - Probe{PortFileWS: "ws://127.0.0.1:9222/x", WSReachable: false, ChromeRunning: true}, InstructToggle}, + {"completed upgrade -> attach (Path B)", + Probe{Endpoint: "ws://127.0.0.1:9222/x", WS: WSReady}, Attach}, + {"open port, hanging upgrade -> consent pending (NOT a timeout, NOT the toggle)", + Probe{Endpoint: "ws://127.0.0.1:9222/x", WS: WSPending}, ConsentPending}, + {"open port, hanging upgrade, chrome running -> still consent pending", + Probe{Endpoint: "ws://127.0.0.1:9222/x", WS: WSPending, ChromeRunning: true}, ConsentPending}, + {"open port, hanging upgrade, --no-launch -> still consent pending (nothing to launch, it is asking)", + Probe{Endpoint: "ws://127.0.0.1:9222/x", WS: WSPending, NoLaunch: true}, ConsentPending}, + // There is no "pending with no endpoint" case: an upgrade is only ever + // attempted against an endpoint, so WS being anything but WSRefused + // already says one was found. + {"stale port file (refused) + chrome running -> instruct toggle", + Probe{Endpoint: "ws://127.0.0.1:9222/x", WS: WSRefused, ChromeRunning: true}, InstructToggle}, {"no debug + chrome running -> instruct toggle (don't shadow)", Probe{ChromeRunning: true}, InstructToggle}, {"no debug + no chrome -> launch managed (Path A)", diff --git a/internal/chrome/cdp.go b/internal/chrome/cdp.go index 07ef27c..ff8cc1f 100644 --- a/internal/chrome/cdp.go +++ b/internal/chrome/cdp.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "hash/fnv" - "net" "os" "os/exec" "path/filepath" @@ -29,6 +28,7 @@ import ( "github.com/sanketsudake/chrome-cdp-cli/internal/browser" "github.com/sanketsudake/chrome-cdp-cli/internal/eventbuf" + "github.com/sanketsudake/chrome-cdp-cli/internal/result" "github.com/sanketsudake/chrome-cdp-cli/internal/target" ) @@ -40,6 +40,31 @@ type Options struct { NoLaunch bool // don't fall back to launching a managed Chrome Headless bool // headless for the managed-launch fallback (tests use this) + // ConsentTimeout bounds the wait for Chrome's "Allow remote debugging?" + // dialog (config key consent_timeout). It applies ONLY to an open port + // whose upgrade is hanging; a refused endpoint still fails in + // milliseconds. See browser.AwaitUpgrade. + // + // It arrives already normalised: config resolution is the one boundary + // where flag, env and file meet, and ClampConsentTimeout runs there. + ConsentTimeout time.Duration + // ConsentPendingAfter is how much silence from an OPEN port counts as + // "Chrome is asking the user", i.e. when OnConsentPending fires. Zero means + // DefaultConsentPendingAfter, which is what production always uses. + // + // It is a field rather than a package var because a test needs to shrink + // it, and the tests that need it most are in another package: the daemon's + // had to sit a 3s consent budget and a 3s poll around a 2s threshold it + // could not reach, measuring 3.05s against a marker at ~2.0s. A 1.5x margin + // on a timing this repo has already been bitten by four times on macOS is + // not a margin. Options already carries ConsentTimeout and + // OnConsentPending; this belongs with them. + ConsentPendingAfter time.Duration + // OnConsentPending fires once, as soon as the upgrade is classified as + // pending — i.e. while the dialog is still on screen, not after the wait. + // The daemon uses it to tell the CLI what it is waiting for. + OnConsentPending func() + // Event-capture bounds (config keys console_buffer / console_max_entry). // Zero means the built-in default; see configureCapture. ConsoleBuffer int // retained console messages per target @@ -132,8 +157,57 @@ func newCDP(managed bool, alloc context.Context, allocCancel context.CancelFunc, return c } +// Connection-probe timings. The dial and the pending threshold are short because +// this is loopback: a debug endpoint that has not accepted in two seconds is not +// slow, it is absent, and one that has accepted but said nothing for two seconds +// is not busy, it is waiting for a human. +const ( + // DefaultConsentTimeout is how long an open-but-silent endpoint is waited + // out (config key consent_timeout). Two minutes is a human timescale for a + // browser-modal dialog that can sit behind the window; ten seconds is not, + // and ten seconds is what used to abandon the prompt it had just raised. + DefaultConsentTimeout = 120 * time.Second + // MinConsentTimeout and MaxConsentTimeout bound what a configured value is + // allowed to be. Below the floor the "wait" is not a wait at all and the + // prompt is abandoned as soon as it is raised — the original defect. Above + // the ceiling it stops being a timeout: the daemon spawn lock is held for + // as long as this value, so an inherited CHROME_CDP_CONSENT_TIMEOUT=8760h + // would block every other invocation for a year. + MinConsentTimeout = 1 * time.Second + MaxConsentTimeout = 10 * time.Minute + // DefaultConsentPendingAfter is how much silence from an open port counts + // as "Chrome is asking the user". Two seconds, because this is loopback: a + // debug endpoint that has accepted and then said nothing for two seconds is + // not busy, it is waiting for a human. + DefaultConsentPendingAfter = 2 * time.Second +) + +// ClampConsentTimeout normalises a configured consent budget: zero or negative +// (unset, or a "0s" that meant nothing in particular) becomes the default, and +// anything outside [MinConsentTimeout, MaxConsentTimeout] is pulled to the +// nearer bound. +// +// It exists so that every layer that reads this number reads it the SAME way. +// Before, chrome.Connect mapped <= 0 to the default, daemon.Ensure took the +// zero literally, and main forwarded the env var only when > 0 — so +// consent_timeout = "0s" produced a daemon waiting 120s, a client giving up at +// 10s, and the message "still waiting ... after 0s". +func ClampConsentTimeout(d time.Duration) time.Duration { + switch { + case d <= 0: + return DefaultConsentTimeout + case d < MinConsentTimeout: + return MinConsentTimeout + case d > MaxConsentTimeout: + return MaxConsentTimeout + } + return d +} + // Connect walks the connection ladder (mirroring browser.DecideConnection): -// - a reachable DevToolsActivePort endpoint -> attach (Path B) +// - a completed WebSocket upgrade -> attach (Path B) +// - an open port with a hanging upgrade -> wait out Chrome's consent +// prompt, then ConnectError{consent_pending} if it is never answered // - a running but non-debug Chrome -> ConnectError{not_debug_enabled} // (never shadow the user's session with a second browser) // - nothing running, --no-launch -> ConnectError{connection_failed} @@ -146,17 +220,35 @@ func newCDP(managed bool, alloc context.Context, allocCancel context.CancelFunc, // there is exactly one authored copy of the ladder. func Connect(_ context.Context, opts Options) (*CDP, error) { // An explicit --port takes precedence over the DevToolsActivePort file. - var endpoint string - if opts.Port != 0 { - endpoint = fmt.Sprintf("http://127.0.0.1:%d", opts.Port) - } else if pf := browser.FindPortFile(opts.PortFile); pf != "" { - if ws, err := browser.WSURLFromPortFile(pf); err == nil { - endpoint = ws - } + // Shared with `doctor` so the command that diagnoses the connection and the + // command that makes it are talking about the same Chrome. + endpoint := browser.FindEndpoint(opts.PortFile, opts.Port).URL + // Already clamped by whoever resolved the flag/env/config; run it again + // rather than trust that. It is the same function, so this cannot become a + // second, disagreeing policy — which is the only thing that went wrong here + // before. + consent := ClampConsentTimeout(opts.ConsentTimeout) + pendingAfter := opts.ConsentPendingAfter + if pendingAfter <= 0 { + pendingAfter = DefaultConsentPendingAfter + } + // One upgrade decides the ladder's first two rungs, and it is the ONLY thing + // here that can raise a consent prompt. chromedp cannot do this itself: + // bounding its first Run with a context deadline would tear down the browser + // it just allocated, so the classification has to happen on a socket we own. + // The socket is then held (up.Close is deferred past the attach) so the + // consent the user just granted is still live when chromedp arrives. + ws := browser.WSRefused + attachTo := endpoint + if wsURL, ok := ResolveWSURL(endpoint); ok { + attachTo = wsURL + up := AwaitUpgrade(wsURL, UpgradeTimings{PendingAfter: pendingAfter, Total: consent}, opts.OnConsentPending) + defer up.Close() + ws = up.State } probe := browser.Probe{ - PortFileWS: endpoint, - WSReachable: endpoint != "" && Reachable(endpoint), + Endpoint: endpoint, + WS: ws, ChromeRunning: chromeRunning(), NoLaunch: opts.NoLaunch, } @@ -164,16 +256,22 @@ func Connect(_ context.Context, opts Options) (*CDP, error) { var err error switch browser.DecideConnection(probe) { case browser.Attach: - c, err = attach(endpoint) + // The RESOLVED ws:// URL, not the endpoint: ResolveWSURL already did + // this lookup, and handing chromedp the http:// form made it repeat the + // request — against the claim, three lines up, that the probe and the + // attach agree on what they are talking to. + c, err = attach(attachTo) + case browser.ConsentPending: + return nil, &ConnectError{Code: result.CodeConsentPending, Message: consentPendingMsg(consent)} case browser.InstructToggle: return nil, &ConnectError{ - Code: "not_debug_enabled", - Message: "Chrome is running but not debug-enabled — open chrome://inspect/#remote-debugging and toggle it on (chrome-cdp will not open a second browser over your session)", + Code: result.CodeNotDebug, + Message: "Chrome is running but not debug-enabled — " + browser.EnableAdvice + " (chrome-cdp will not open a second browser over your session)", } case browser.InstructNoLaunch: return nil, &ConnectError{ - Code: "connection_failed", - Message: "no debug-enabled Chrome found and --no-launch is set — enable chrome://inspect/#remote-debugging or drop --no-launch", + Code: result.CodeConnection, + Message: "no debug-enabled Chrome found and --no-launch is set — " + browser.EnableAdvice + ", or drop --no-launch", } default: // Launch c, err = launch(opts.Headless, opts.ProfileDir, opts.Port) @@ -247,6 +345,13 @@ func startBase(managed bool, alloc context.Context, allocCancel context.CancelFu return c, nil } +// consentPendingMsg explains a wait that ran out with the dialog still +// unanswered, composed from the one authored description of the prompt. +func consentPendingMsg(waited time.Duration) string { + return fmt.Sprintf("%s It has not been answered in %s — retry once you have, and raise --consent-timeout if you need longer. "+ + "To avoid the prompt entirely, %s.", browser.ConsentPromptAdvice, waited, browser.EnableAdvice) +} + // connectFailMsg turns a raw allocator/dial failure into an actionable message. // The common attach case — "could not dial … deadline exceeded" — is almost // always Chrome holding a pending "Allow remote debugging?" consent prompt (or a @@ -254,14 +359,18 @@ func startBase(managed bool, alloc context.Context, allocCancel context.CancelFu func connectFailMsg(managed bool, what string, err error) string { s := err.Error() if !managed && (strings.Contains(s, "could not dial") || strings.Contains(s, "deadline exceeded")) { - return "cannot reach Chrome's debug endpoint — if Chrome is showing an \"Allow remote debugging?\" prompt, click Allow (it can be behind the window), then retry; if it stays unresponsive the endpoint is wedged: quit and reopen Chrome, re-enable chrome://inspect/#remote-debugging, and keep the daemon running so the consent is asked once, not per command" + return "cannot reach Chrome's debug endpoint. " + browser.ConsentPromptAdvice + + " If it stays unresponsive the endpoint is wedged instead: quit and reopen Chrome, then " + browser.EnableAdvice + + ", and keep the daemon running so the consent is asked once, not per command" } return fmt.Sprintf("%s: %v", what, err) } // chromeRunning best-effort detects an already-running Chrome (so we instruct // the toggle instead of shadowing the user's session with a managed browser). -func chromeRunning() bool { +// It is a var so a connection test can pin the answer: whether the machine +// running the test happens to have Chrome open must not change the ladder. +var chromeRunning = func() bool { var name string switch runtime.GOOS { case "darwin": @@ -1609,18 +1718,3 @@ func (c *CDP) Raw(ctx context.Context, id, method string, params json.RawMessage _ = json.Unmarshal(res, &v) return v, nil } - -// Reachable reports whether the loopback debug port is actually listening -// (used by `doctor` so a stale port file isn't reported as ready). -func Reachable(wsURL string) bool { - hostport, ok := browser.HostPort(wsURL) - if !ok { - return false - } - conn, err := net.DialTimeout("tcp", hostport, 500*time.Millisecond) - if err != nil { - return false - } - _ = conn.Close() - return true -} diff --git a/internal/chrome/consent_test.go b/internal/chrome/consent_test.go new file mode 100644 index 0000000..0850c78 --- /dev/null +++ b/internal/chrome/consent_test.go @@ -0,0 +1,213 @@ +package chrome + +import ( + "context" + "errors" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" + "github.com/sanketsudake/chrome-cdp-cli/internal/probetest" + "github.com/sanketsudake/chrome-cdp-cli/internal/result" +) + +// RFC-0013. The consent-pending state is a TCP listener that accepts and then +// stalls, so every scenario here runs against net.Listen and no browser at all. +// That is not a convenience: reproducing this by hand wedged a real user's +// Chrome twice, and a regression test that needs a human to click a modal is not +// a test. + +// pinChromeRunning fixes the pgrep answer: whether the machine running the test +// happens to have Chrome open must not decide which rung of the ladder we land on. +func pinChromeRunning(t *testing.T, running bool) { + t.Helper() + prev := chromeRunning + chromeRunning = func() bool { return running } + t.Cleanup(func() { chromeRunning = prev }) +} + +func connectErrCode(t *testing.T, err error) string { + t.Helper() + if err == nil { + t.Fatal("Connect succeeded against a stub listener, want an error") + } + var ce *ConnectError + if !errors.As(err, &ce) { + t.Fatalf("error %v is not a *ConnectError, so its code never reaches the envelope", err) + } + return ce.Code +} + +// TestConnectConsentPendingWaitsAndReports is VS-1 and VS-4. +// +// The old behaviour: the dial timed out in ~10s, the daemon wrote its error and +// exited, and the modal it had raised was left on screen with nothing behind it. +// Clicking Allow then granted consent to a connection that no longer existed. +func TestConnectConsentPendingWaitsAndReports(t *testing.T) { + ep := probetest.Stall(t) + pinChromeRunning(t, true) // even so: a hanging upgrade is not "enable the toggle" + + var pendingAt time.Duration + start := time.Now() + _, err := Connect(context.Background(), Options{ + PortFile: ep.PortFile(t), + NoLaunch: true, + ConsentTimeout: 2 * time.Second, + ConsentPendingAfter: 200 * time.Millisecond, + OnConsentPending: func() { pendingAt = time.Since(start) }, + }) + elapsed := time.Since(start) + + if got := connectErrCode(t, err); got != result.CodeConsentPending { + t.Errorf("error.code = %q, want %q — a hanging upgrade must not surface as a generic failure", got, result.CodeConsentPending) + } + // VS-1: it stayed alive well past the dial timeout that used to abandon the + // prompt, rather than giving up at ~10s. + if elapsed < 1800*time.Millisecond { + t.Errorf("gave up after %v, want the full ~2s consent budget", elapsed) + } + // VS-4: and the wait is bounded — a long wait is not an unbounded one. + if elapsed > 20*time.Second { + t.Errorf("waited %v; the consent wait must be bounded by consent_timeout", elapsed) + } + if pendingAt == 0 { + t.Error("OnConsentPending never fired — the daemon can only tell the user while the dialog is up if it knows during the wait") + } else if pendingAt > elapsed/2 { + t.Errorf("OnConsentPending fired after %v of a %v wait — it must announce while the dialog is up, not on the way out", pendingAt, elapsed) + } + + // VS-4: the message has to carry the one authored explanation of the prompt + // AND the recovery, because the symptom the user is looking at is a browser + // that appears to have crashed. Asserting on the const rather than on a list + // of substrings is the point: five hand-written copies of this paragraph had + // already drifted, and a substring list cannot tell the difference. + msg := err.Error() + if !strings.Contains(msg, browser.ConsentPromptAdvice) { + t.Errorf("the consent-timeout message does not carry browser.ConsentPromptAdvice:\n%s", msg) + } + if !strings.Contains(msg, "--remote-debugging-port=9222") { + t.Errorf("the consent-timeout message does not name the recovery:\n%s", msg) + } +} + +// TestConnectRefusedEndpointFailsFast is VS-2, the safety property that makes a +// two-minute wait acceptable at all: only an OPEN port earns it. +func TestConnectRefusedEndpointFailsFast(t *testing.T) { + pf := probetest.Closed(t).PortFile(t) // nothing is listening there + pinChromeRunning(t, false) + + start := time.Now() + _, cerr := Connect(context.Background(), Options{ + PortFile: pf, + NoLaunch: true, // never launch a real browser from a test + ConsentTimeout: 60 * time.Second, + }) + elapsed := time.Since(start) + + if got := connectErrCode(t, cerr); got != result.CodeConnection { + t.Errorf("error.code = %q, want %q", got, result.CodeConnection) + } + if elapsed > 3*time.Second { + t.Fatalf("a closed port took %v to fail — it must fail fast, not wait out consent_timeout", elapsed) + } +} + +// TestConnectLateConsentIsNotAbandoned is VS-3: consent answered long after the +// old ~10s dial timeout still finds the connection there. +// +// It stops at the classification rather than asserting a working CDP session, +// because the stub is a socket and not a browser — a full success would need a +// fake Chrome speaking the protocol, and the defect this pins is entirely about +// whether we were still connected when the answer arrived. +func TestConnectLateConsentIsNotAbandoned(t *testing.T) { + ep := probetest.Answer(t, 700*time.Millisecond, "HTTP/1.1 101 Switching Protocols") + pinChromeRunning(t, true) + + start := time.Now() + _, err := Connect(context.Background(), Options{ + PortFile: ep.PortFile(t), + NoLaunch: true, + ConsentTimeout: 10 * time.Second, + }) + elapsed := time.Since(start) + + if code := connectErrCode(t, err); code == result.CodeConsentPending { + t.Errorf("a completed upgrade was still reported as %q — a late Allow must be accepted, not timed out", code) + } + if !ep.AnsweredLive() { + t.Error("the endpoint answered into a closed socket: the consent prompt was orphaned") + } + if elapsed < 600*time.Millisecond { + t.Errorf("returned after %v, before the endpoint answered at 700ms — it gave up on the prompt", elapsed) + } +} + +// TestConnectNoEndpointLeadsWithTheLaunchFlag pins US-3: the route that never +// prompts is recommended before the toggle that prompts every time. +func TestConnectNoEndpointLeadsWithTheLaunchFlag(t *testing.T) { + pinChromeRunning(t, true) + _, err := Connect(context.Background(), Options{ + PortFile: filepath.Join(t.TempDir(), "no-such-port-file"), + NoLaunch: true, + }) + if got := connectErrCode(t, err); got != result.CodeNotDebug { + t.Fatalf("error.code = %q, want %q", got, result.CodeNotDebug) + } + msg := err.Error() + flagAt, toggleAt := strings.Index(msg, "--remote-debugging-port"), strings.Index(msg, "chrome://inspect") + if flagAt < 0 || toggleAt < 0 { + t.Fatalf("both routes should be offered:\n%s", msg) + } + if flagAt > toggleAt { + t.Errorf("the message recommends the chrome://inspect toggle before the launch flag, which routes every new user through the consent prompt:\n%s", msg) + } +} + +// TestConnectExplicitPortStillProbes guards the path RFC-0013 now tells everyone +// to use. An explicit --port names an HTTP endpoint, not a WebSocket one; if the +// probe handshakes against that URL directly it never sees a 101 and reports a +// healthy Chrome as unreachable. Resolving through /json/version first is what +// keeps the recommended route working. +func TestConnectExplicitPortStillProbes(t *testing.T) { + ep := probetest.Chrome(t, 0, "") // JSON API present; the upgrade stalls + p, _ := strconv.Atoi(ep.Port()) + pinChromeRunning(t, false) + + _, err := Connect(context.Background(), Options{ + Port: p, NoLaunch: true, ConsentTimeout: 700 * time.Millisecond, + ConsentPendingAfter: 100 * time.Millisecond, + }) + if got := connectErrCode(t, err); got != result.CodeConsentPending { + t.Errorf("error.code = %q, want %q — the --port endpoint was not probed as a WebSocket", got, result.CodeConsentPending) + } +} + +// TestConnectExplicitPortDetectsConsentWithoutJSONVersion is the same path with +// the JSON API withheld, which is what the toggle actually does. +// +// RFC-0013's fourth observation: on the chrome://inspect path /json/version +// 404s regardless of consent state. So `--port 9222` against a toggle-path +// Chrome holding an unanswered prompt used to resolve to nothing, classify as +// refused, and fall to InstructToggle — telling the user "Chrome is running but +// not debug-enabled" about a Chrome that IS debug-enabled and is at that moment +// showing them the dialog. It sent them to re-enable a setting already on. +func TestConnectExplicitPortDetectsConsentWithoutJSONVersion(t *testing.T) { + // Stall(): it 404s nothing and answers nothing — every request, including + // /json/version, is accepted and then met with silence, which is what the + // toggle path looks like with a prompt on screen. + ep := probetest.Stall(t) + p, _ := strconv.Atoi(ep.Port()) + pinChromeRunning(t, true) // and yet: a hanging upgrade is not "enable the toggle" + + _, cerr := Connect(context.Background(), Options{ + Port: p, NoLaunch: true, ConsentTimeout: 700 * time.Millisecond, + ConsentPendingAfter: 100 * time.Millisecond, + }) + if got := connectErrCode(t, cerr); got != result.CodeConsentPending { + t.Errorf("error.code = %q, want %q — with /json/version not answering, the pending prompt is invisible and the user is told to re-enable a setting that is already on:\n%v", + got, result.CodeConsentPending, cerr) + } +} diff --git a/internal/chrome/consent_timeout_test.go b/internal/chrome/consent_timeout_test.go new file mode 100644 index 0000000..15863e6 --- /dev/null +++ b/internal/chrome/consent_timeout_test.go @@ -0,0 +1,27 @@ +package chrome + +import ( + "testing" + "time" +) + +func TestClampConsentTimeout(t *testing.T) { + t.Parallel() + for _, c := range []struct { + in time.Duration + want time.Duration + }{ + {0, DefaultConsentTimeout}, + {-1, DefaultConsentTimeout}, + {-time.Hour, DefaultConsentTimeout}, + {time.Millisecond, MinConsentTimeout}, + {MinConsentTimeout, MinConsentTimeout}, + {45 * time.Second, 45 * time.Second}, + {MaxConsentTimeout, MaxConsentTimeout}, + {8760 * time.Hour, MaxConsentTimeout}, + } { + if got := ClampConsentTimeout(c.in); got != c.want { + t.Errorf("ClampConsentTimeout(%v) = %v, want %v", c.in, got, c.want) + } + } +} diff --git a/internal/chrome/probe.go b/internal/chrome/probe.go new file mode 100644 index 0000000..5007c8d --- /dev/null +++ b/internal/chrome/probe.go @@ -0,0 +1,399 @@ +// Probe classification of Chrome's debug endpoint: the one WebSocket handshake +// that tells "nothing is listening" apart from "Chrome is asking the user". +// +// It lives here, next to the chromedp connection it feeds, rather than in +// internal/browser — whose own doc says it is deliberately free of chromedp so +// it unit-tests without a live browser. That claim held for a port-file parser +// and a decision table; it did not survive a TCP dialer, an HTTP client, a +// hand-rolled RFC 6455 handshake, a reader goroutine, and a live net.Conn +// handed across the package boundary for this package to close. The tests are +// net.Listen-based and need no browser either way. + +package chrome + +import ( + "bufio" + "crypto/rand" + "crypto/sha1" + "encoding/base64" + "encoding/json" + "errors" + "io" + "net" + "net/http" + "net/url" + "os" + "strings" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" +) + +// Upgrade is one probe's outcome plus, when the endpoint accepted, the socket it +// used. +// +// The socket is kept rather than closed on purpose. Chrome asks for consent per +// fresh attach, so a probe that connects, learns the answer and hangs up has +// spent the user's click on a connection nobody kept; the follow-on attach would +// be a fresh one again. Holding it open until the real attach has been +// established means the click the user just made is still doing work when the +// attach lands. Close it as soon as the attach returns — see chrome.Connect. +type Upgrade struct { + State browser.WSState + conn net.Conn +} + +// Close releases the probe socket. Safe on a refused or pending Upgrade, which +// have no socket to release — AwaitUpgrade never returns nil, so that is the +// only case there is. +func (u *Upgrade) Close() { + if u.conn == nil { + return + } + _ = u.conn.Close() + u.conn = nil +} + +// ResolveWSURL returns the browser-level ws:// URL to probe and attach to. +// +// A ws:// endpoint (the DevToolsActivePort path) is already one. An explicit +// --port names an http:// endpoint instead, and the browser-level WebSocket path +// is normally discoverable through Chrome's HTTP JSON API — the same resolution +// chromedp's remote allocator performs, done here first so the probe and the +// attach agree on what they are talking to. +// +// This is NOT the consent check, and the difference matters: /json/version +// answers the same whether or not consent is pending (on the chrome://inspect +// path it 404s either way), so it can locate an endpoint and can never classify +// one. Only the upgrade does that. +// +// Which is exactly why a failed lookup falls back to the ws:// ROOT of the same +// host:port rather than giving up. On the toggle path the JSON API is simply +// absent, so "no answer from /json/version" carried no information at all — and +// treating it as "no endpoint" made the pending state undetectable on the one +// path that actually prompts: `--port 9222` against a toggle-path Chrome +// holding an unanswered dialog reported not_debug_enabled, sending the user to +// re-enable a setting that was already on. Probing the root instead costs +// nothing in the granted case (an endpoint that will not upgrade there is +// classified refused, which is where it already was) and makes the hang — the +// one unambiguous consent signature — visible. +func ResolveWSURL(endpoint string) (string, bool) { + switch { + case strings.HasPrefix(endpoint, "ws://"), strings.HasPrefix(endpoint, "wss://"): + return endpoint, true + case strings.HasPrefix(endpoint, "http://"), strings.HasPrefix(endpoint, "https://"): + default: + return "", false + } + hostport, ok := browser.HostPort(endpoint) + if !ok { + return "", false + } + if ws, ok := wsFromJSONVersion(endpoint, hostport); ok { + return ws, true + } + return "ws://" + hostport + "/", true +} + +// wsFromJSONVersion asks Chrome's HTTP JSON API where the browser-level +// WebSocket is. It reports false for every way that can fail to answer, all of +// which mean the same thing here: ask the socket instead. +// +// The answer is only accepted if it points back at the endpoint we asked. This +// is a question about ONE loopback port, and both the redirect chain and the +// returned URL used to be taken on trust: http.Client follows up to ten +// redirects, and the webSocketDebuggerUrl was returned verbatim, so a request +// about 127.0.0.1 was verified to come back with ws://10.1.2.3:4444/pwned — +// and that URL is what the probe dials and what chromedp attaches to. +func wsFromJSONVersion(endpoint, hostport string) (string, bool) { + client := &http.Client{ + Timeout: dialTimeout, + // A redirect is not an answer to "where is YOUR WebSocket". + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + resp, err := client.Get(strings.TrimSuffix(endpoint, "/") + "/json/version") + if err != nil { + return "", false + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return "", false + } + var v struct { + WS string `json:"webSocketDebuggerUrl"` + } + if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<20)).Decode(&v); err != nil || v.WS == "" { + return "", false + } + u, err := url.Parse(v.WS) + if err != nil || (u.Scheme != "ws" && u.Scheme != "wss") || u.User != nil || !sameEndpoint(u.Host, hostport) { + return "", false + } + return v.WS, true +} + +// sameEndpoint reports whether a returned authority names the endpoint we +// asked. The port must match exactly; the host must match too, except that two +// spellings of loopback are accepted for each other because Chrome answers with +// whichever one it was asked on and the caller may have used the other. +func sameEndpoint(got, want string) bool { + if got == want { + return true + } + gh, gp, err := net.SplitHostPort(got) + if err != nil { + return false + } + wh, wp, err := net.SplitHostPort(want) + if err != nil || gp != wp { + return false + } + return isLoopback(gh) && isLoopback(wh) +} + +func isLoopback(host string) bool { + if host == "localhost" { + return true + } + ip := net.ParseIP(strings.Trim(host, "[]")) + return ip != nil && ip.IsLoopback() +} + +// maxHandshakeResponse caps what the probe will read looking for the +// handshake's status line and headers. Those are a few hundred bytes; 8 KiB is +// generous and still nothing at all to hold. +const maxHandshakeResponse = 8 << 10 + +// UpgradeTimings bounds one probe. It is a struct rather than two positional +// durations because the two are easy to swap and the consequences are not +// symmetric: a pending threshold longer than the budget announces nothing, and +// a budget shorter than the threshold abandons the prompt it just raised. +type UpgradeTimings struct { + // PendingAfter is how much silence counts as "Chrome is asking the user". + // Reaching it calls onPending (once) so the caller can say so WHILE it + // waits, rather than afterwards, which would be a post-mortem. + PendingAfter time.Duration + // Total is the whole budget. The same upgrade stays open across it, so an + // answer that arrives late still lands on a live connection instead of an + // orphaned one. + Total time.Duration +} + +// dialTimeout bounds the TCP connect. Nothing listening is a fast, ordinary +// failure and must stay one — that is the safety property that makes the long +// consent wait acceptable at all. It is a constant, not a parameter: this is +// loopback, both call sites had independently declared the same two seconds, +// and a caller has no information that would make a different value right. +const dialTimeout = 2 * time.Second + +// AwaitUpgrade dials wsURL and performs exactly ONE WebSocket handshake against +// it, classifying the result. +// +// It is deliberately a single connection: every connection to the debug endpoint +// is a consent request, and stacking those is what wedges a browser. +func AwaitUpgrade(wsURL string, t UpgradeTimings, onPending func()) *Upgrade { + hostport, ok := browser.HostPort(wsURL) + if !ok { + return &Upgrade{State: browser.WSRefused} + } + conn, dialErr := net.DialTimeout("tcp", hostport, dialTimeout) + if dialErr != nil { + return &Upgrade{State: browser.WSRefused} + } + key, err := writeUpgradeRequest(conn, wsURL, hostport) + if err != nil { + _ = conn.Close() + return &Upgrade{State: browser.WSRefused} + } + wait := t.Total + + // The read runs in a goroutine because there is nothing else to bound its + // COMPLETION: a pending endpoint never writes and never closes. Closing + // conn is what unblocks it, which the caller (or the timeout path below) + // always does. + // + // Its SIZE and its lifetime are bounded here, and both bounds are load- + // bearing. Anything that can bind the loopback debug port can answer, and + // what is expected back is one line of HTTP: without the LimitReader, + // ReadString('\n') on a newline-free stream accumulated at gigabytes per + // second for the caller's whole budget — two minutes in the daemon. The + // read deadline is the matching bound in time, so the goroutine cannot + // outlive the wait even if nobody closes the socket. + // + // The deadline sits a little PAST the budget rather than on it. It is a + // backstop against a goroutine that outlives everything, not a second copy + // of the budget — the timers below own that — and putting it exactly on the + // budget makes an answer that lands as the wait ends unreadable, so a + // completed handshake still in flight would be reported as a pending + // prompt. + _ = conn.SetReadDeadline(time.Now().Add(wait + dialTimeout)) + answered := make(chan bool, 1) + go func() { + ok, err := readHandshakeResponse(conn, key) + if errors.Is(err, os.ErrDeadlineExceeded) { + // Hitting the deadline is the endpoint's SILENCE, not its answer: + // reporting it as a failed handshake would classify a Chrome that + // is still holding the consent prompt as refused. The caller's own + // timers below say what silence means. + return + } + answered <- ok + }() + + // Two independent timers on one loop. They were once two sequential selects + // with a "rest" computation between them, which had a case the loop simply + // does not have: when PendingAfter was >= Total the remainder came out <= 0 + // and the function returned browser.WSPending having never looked at the answer + // channel again, so a completed handshake sitting in the buffer was thrown + // away and its socket closed — doctor reporting consent_pending for a ready + // endpoint. + pending := time.NewTimer(min(t.PendingAfter, wait)) + defer pending.Stop() + total := time.NewTimer(wait) + defer total.Stop() + for { + select { + case ok := <-answered: + return upgraded(conn, ok) + case <-pending.C: + // Silence past PendingAfter on an OPEN port: the consent + // signature. Say so now — a user who has not seen the dialog needs + // telling while it is still on screen — and keep this same upgrade + // open for the rest of the budget. + if onPending != nil { + onPending() + } + case <-total.C: + // The budget and the answer can come ready in the same instant, + // and select picks between ready cases at random. Ask once more + // before discarding a handshake that did complete. + select { + case ok := <-answered: + return upgraded(conn, ok) + default: + } + _ = conn.Close() + return &Upgrade{State: browser.WSPending} + } + } +} + +// upgraded turns a completed handshake into an Upgrade, keeping the socket only +// when it is worth keeping. NOT called settle: everything else in this package +// that says "settle" means "wait until it stops moving" (settle, settledPageRect, +// settledNodePoint), and this one means "close it or keep it". +func upgraded(conn net.Conn, ok bool) *Upgrade { + if !ok { + _ = conn.Close() + return &Upgrade{State: browser.WSRefused} + } + // The probe's read deadline bounded the handshake; a socket that is being + // KEPT must not carry it into the attach that follows. + _ = conn.SetReadDeadline(time.Time{}) + return &Upgrade{State: browser.WSReady, conn: conn} +} + +// ProbeWS classifies an endpoint for a caller that wants the answer and not the +// socket — `doctor`, which must report what it verified and hold nothing. +// +// It therefore does the thing Upgrade's doc comment warns about: it connects, +// learns the answer, and hangs up, spending the user's click on a connection +// nobody kept. That is the right trade HERE and only here. doctor is a +// diagnostic with nothing to hand a live socket to, and holding one open past +// the command that made it would be worse. What it must not do is pretend +// otherwise, so doctor's ready verdict says the connection was closed and the +// next command may prompt again — see runDoctor. +func ProbeWS(wsURL string, wait time.Duration) browser.WSState { + u := AwaitUpgrade(wsURL, UpgradeTimings{PendingAfter: wait, Total: wait}, nil) + defer u.Close() + return u.State +} + +// The handshake is written and verified by hand rather than with a WebSocket +// library, and that IS the right call here even though it looks like the wrong +// one. What this code has to observe is "accepted, then silent for two +// minutes", and a dialer's API cannot express that: it returns a connection or +// an error, and the state we care about is neither. Owning the socket is the +// only way to hold a pending upgrade open across the consent wait, which is the +// whole point. (It also keeps a WebSocket library out of the direct +// dependencies for one handshake.) + +// writeUpgradeRequest sends a minimal RFC 6455 handshake and returns the +// Sec-WebSocket-Key it used, which the response has to be checked against. +// Nothing is ever sent over the resulting connection, so no CDP session is +// started and no target is created. +func writeUpgradeRequest(conn net.Conn, wsURL, hostport string) (key string, err error) { + path := "/" + if u, perr := url.Parse(wsURL); perr == nil && u.Path != "" { + path = u.RequestURI() + } + var nonce [16]byte + if _, err := rand.Read(nonce[:]); err != nil { + return "", err + } + key = base64.StdEncoding.EncodeToString(nonce[:]) + req := "GET " + path + " HTTP/1.1\r\n" + + "Host: " + hostport + "\r\n" + + "Upgrade: websocket\r\n" + + "Connection: Upgrade\r\n" + + "Sec-WebSocket-Key: " + key + "\r\n" + + "Sec-WebSocket-Version: 13\r\n\r\n" + _ = conn.SetWriteDeadline(time.Now().Add(dialTimeout)) + _, err = conn.Write([]byte(req)) + _ = conn.SetWriteDeadline(time.Time{}) + return key, err +} + +// wsGUID is RFC 6455's fixed accept-key salt. +const wsGUID = "258EAFA5-E914-47DA-95CA-5AB0DC85B11A" + +// acceptFor is the Sec-WebSocket-Accept a correct server must return for key. +func acceptFor(key string) string { + sum := sha1.Sum([]byte(key + wsGUID)) + return base64.StdEncoding.EncodeToString(sum[:]) +} + +// readHandshakeResponse reads the status line and headers and reports whether +// this is really a WebSocket server completing OUR handshake. +// +// The key was generated with crypto/rand and then never checked, so the whole +// test was "did the status line contain 101" — which anything replying +// "HTTP/9.9 101 whatever" passes. Chrome's debug port is a loopback port any +// local process can bind, and being told "ready" by something that is not +// Chrome is how a probe ends up handing chromedp a socket that will never speak +// CDP. Verifying the accept key is what makes the 101 mean this server saw this +// request. +func readHandshakeResponse(conn net.Conn, key string) (bool, error) { + r := bufio.NewReader(io.LimitReader(conn, maxHandshakeResponse)) + line, err := r.ReadString('\n') + if err != nil || !isSwitchingProtocols(line) { + return false, err + } + var upgraded, accepted bool + for { + h, err := r.ReadString('\n') + if err != nil { + return false, err + } + if strings.TrimSpace(h) == "" { // end of headers + break + } + name, value, ok := strings.Cut(h, ":") + if !ok { + continue + } + switch strings.ToLower(strings.TrimSpace(name)) { + case "upgrade": + upgraded = strings.EqualFold(strings.TrimSpace(value), "websocket") + case "sec-websocket-accept": + accepted = strings.TrimSpace(value) == acceptFor(key) + } + } + return upgraded && accepted, nil +} + +// isSwitchingProtocols reports whether an HTTP status line accepted the upgrade. +func isSwitchingProtocols(line string) bool { + f := strings.Fields(strings.TrimSpace(line)) + return len(f) >= 2 && strings.HasPrefix(f[0], "HTTP/") && f[1] == "101" +} diff --git a/internal/chrome/probe_test.go b/internal/chrome/probe_test.go new file mode 100644 index 0000000..e137460 --- /dev/null +++ b/internal/chrome/probe_test.go @@ -0,0 +1,287 @@ +package chrome + +import ( + "bytes" + "fmt" + "net" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" + "github.com/sanketsudake/chrome-cdp-cli/internal/probetest" +) + +// wsRoot is the ws:// root of an http:// endpoint — where the browser-level +// endpoint lives when /json/version cannot say. +func wsRoot(httpURL string) string { + return "ws://" + authority(httpURL) + "/" +} + +// TestAwaitUpgradeRefusedIsFast is the safety property behind the long consent +// wait: only an OPEN port earns it. A dead endpoint must fail in milliseconds, +// never after the consent timeout. +func TestAwaitUpgradeRefusedIsFast(t *testing.T) { + t.Parallel() + start := time.Now() + u := AwaitUpgrade(probetest.Closed(t).WS(), UpgradeTimings{PendingAfter: time.Second, Total: 30 * time.Second}, nil) + defer u.Close() + if u.State != browser.WSRefused { + t.Errorf("closed port classified %v, want refused", u.State) + } + if el := time.Since(start); el > 2*time.Second { + t.Errorf("a refused endpoint took %v — it must fail fast, not wait out the consent budget", el) + } +} + +// TestAwaitUpgradePendingIsBoundedAndAnnounced covers the consent signature: +// silence on an open port is reported while it is happening, and the wait ends. +func TestAwaitUpgradePendingIsBoundedAndAnnounced(t *testing.T) { + t.Parallel() + ep := probetest.Stall(t) + var pendingAt time.Duration + start := time.Now() + u := AwaitUpgrade(ep.WS(), UpgradeTimings{PendingAfter: 100 * time.Millisecond, Total: 600 * time.Millisecond}, func() { + pendingAt = time.Since(start) + }) + defer u.Close() + elapsed := time.Since(start) + + if u.State != browser.WSPending { + t.Fatalf("a stalling endpoint classified %v, want pending", u.State) + } + if pendingAt == 0 { + t.Error("onPending never fired — the user is told only after the wait, which is the bug") + } + // An ORDERING relation, not a wall-clock ceiling: the property is that the + // announcement lands during the wait rather than on the way out of it, and + // a scheduler hiccup on a loaded CI box is not a regression. + if pendingAt > elapsed/2 { + t.Errorf("onPending fired after %v of a %v wait — it must announce while the dialog is up, not on the way out", pendingAt, elapsed) + } + if elapsed < 500*time.Millisecond { + t.Errorf("gave up after %v, want the full ~600ms budget", elapsed) + } + if elapsed > 3*time.Second { + t.Errorf("the wait is unbounded (%v)", elapsed) + } + if got := ep.Conns(); got != 1 { + t.Errorf("probe opened %d connections, want exactly 1 — each one is a consent request", got) + } +} + +// TestAwaitUpgradeLateAnswerStillSucceeds is the orphaned-prompt regression: an +// answer that arrives long after the old ~10s dial timeout must still land on a +// live connection. +func TestAwaitUpgradeLateAnswerStillSucceeds(t *testing.T) { + t.Parallel() + ep := probetest.Answer(t, 300*time.Millisecond, "HTTP/1.1 101 Switching Protocols") + var announced bool + u := AwaitUpgrade(ep.WS(), UpgradeTimings{PendingAfter: 50 * time.Millisecond, Total: 5 * time.Second}, func() { announced = true }) + defer u.Close() + + if u.State != browser.WSReady { + t.Fatalf("a late-but-completed upgrade classified %v, want ready", u.State) + } + if !announced { + t.Error("the pending state was never announced even though the answer took 6x the threshold") + } + if !ep.AnsweredLive() { + t.Error("the endpoint answered into a closed socket — the prompt was orphaned") + } + if u.conn == nil { + t.Error("a ready upgrade must keep its socket, so the granted consent is still held when the attach lands") + } +} + +// floodListener accepts and then streams bytes that never contain a newline — +// a hostile or broken local process on the debug port. Anything that can bind +// 127.0.0.1:9222 can be this. +func floodListener(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { _ = ln.Close() }) + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + go func(c net.Conn) { + defer c.Close() + chunk := bytes.Repeat([]byte("A"), 32<<10) + for { + if _, err := c.Write(chunk); err != nil { + return + } + } + }(c) + } + }() + return fmt.Sprintf("ws://%s/devtools/browser/stub", ln.Addr().String()) +} + +// TestAwaitUpgradeBoundsTheResponse: the status line is one line of HTTP, and +// the reader must be bounded like one. +// +// bufio.Reader.ReadString('\n') accumulates without limit and had no read +// deadline, so the only ceiling was the caller's total wait — 120s in the +// daemon. A listener streaming newline-free bytes drove 18 GB of heap in six +// seconds; the daemon's full budget reaches hundreds of gigabytes. Nothing here +// needs more than a status line, so nothing here should read more than one. +func TestAwaitUpgradeBoundsTheResponse(t *testing.T) { + t.Parallel() + start := time.Now() + u := AwaitUpgrade(floodListener(t), UpgradeTimings{PendingAfter: 30 * time.Second, Total: 30 * time.Second}, nil) + defer u.Close() + if u.State != browser.WSRefused { + t.Errorf("an endpoint that answers with garbage classified %v, want refused", u.State) + } + if el := time.Since(start); el > 5*time.Second { + t.Errorf("the read ran for %v — a bounded read ends at the limit, not at the consent budget", el) + } +} + +// TestProbeWSClassifiesAllThree is doctor's view: three endpoints, three answers, +// and the ready one established by a completed upgrade rather than a port file. +func TestProbeWSClassifiesAllThree(t *testing.T) { + t.Parallel() + stalling := probetest.Stall(t) + ready := probetest.Answer(t, 0, "HTTP/1.1 101 Switching Protocols") + // An endpoint that ANSWERS with something other than 101 is a live server + // that is not a CDP browser (a stale port file reused by another process). + wrong := probetest.Answer(t, 0, "HTTP/1.1 404 Not Found") + + for _, c := range []struct { + name string + ws string + want browser.WSState + }{ + {"nothing listening", probetest.Closed(t).WS(), browser.WSRefused}, + {"accepts and stalls", stalling.WS(), browser.WSPending}, + {"completes the upgrade", ready.WS(), browser.WSReady}, + {"answers 404", wrong.WS(), browser.WSRefused}, + {"not a ws url", "::::", browser.WSRefused}, + } { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + if got := ProbeWS(c.ws, 400*time.Millisecond); got != c.want { + t.Errorf("ProbeWS = %v, want %v", got, c.want) + } + }) + } +} + +// TestResolveWSURL covers the endpoint shapes the two connection paths produce. +// An explicit --port names an http:// endpoint, and without this resolution the +// upgrade probe would handshake against "/" and classify a perfectly healthy +// Chrome as refused. +func TestResolveWSURL(t *testing.T) { + t.Parallel() + + // A healthy Chrome: /json/version answers with its OWN host:port. + var ok *httptest.Server + ok = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/json/version" { + http.NotFound(w, r) + return + } + fmt.Fprintf(w, `{"Browser":"Chrome/1","webSocketDebuggerUrl":"ws://%s/devtools/browser/abc"}`, authority(ok.URL)) + })) + // t.Cleanup, not defer: the parallel subtests below run after this function + // returns, so a deferred Close would shut the server before they use it. + t.Cleanup(ok.Close) + + // A 404 on /json/version is exactly what the chrome://inspect path returns, + // consent or no consent. + notFound := httptest.NewServer(http.HandlerFunc(http.NotFound)) + t.Cleanup(notFound.Close) + + // Not Chrome: it answers the question with somewhere else entirely. That + // URL used to be returned verbatim, and it is what the probe dials and what + // chromedp attaches to. + elsewhere := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"webSocketDebuggerUrl":"ws://10.1.2.3:4444/pwned"}`) + })) + t.Cleanup(elsewhere.Close) + + // Also not Chrome: it redirects, and http.Client follows up to ten of those + // by default — off this machine, from a question about 127.0.0.1. + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, elsewhere.URL+"/json/version", http.StatusFound) + })) + t.Cleanup(redirector.Close) + + for _, c := range []struct { + name string + endpoint string + want string + wantOK bool + }{ + {"a ws url passes through", "ws://127.0.0.1:9222/devtools/browser/x", "ws://127.0.0.1:9222/devtools/browser/x", true}, + {"an http endpoint resolves via /json/version", ok.URL, "ws://" + authority(ok.URL) + "/devtools/browser/abc", true}, + // The chrome://inspect toggle path 404s /json/version whether or not + // consent has been granted, so a 404 must NOT end the resolution: it + // leaves the browser endpoint at the root of the same host:port, and + // probing that is the only way the pending state is visible on the one + // path that actually prompts. + {"a 404 falls back to the ws root", notFound.URL, wsRoot(notFound.URL), true}, + {"nothing listening still falls back", "http://127.0.0.1:1", "ws://127.0.0.1:1/", true}, + // Both of these fall back to the local root rather than being believed: + // the only endpoint this resolution is allowed to name is the one it + // asked. + {"a foreign ws url is not believed", elsewhere.URL, wsRoot(elsewhere.URL), true}, + {"a redirect is not followed", redirector.URL, wsRoot(redirector.URL), true}, + {"empty", "", "", false}, + } { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + got, gotOK := ResolveWSURL(c.endpoint) + if gotOK != c.wantOK || got != c.want { + t.Errorf("ResolveWSURL(%q) = %q,%v; want %q,%v", c.endpoint, got, gotOK, c.want, c.wantOK) + } + }) + } +} + +// authority is the host:port of an http:// test-server URL. +func authority(httpURL string) string { return strings.TrimPrefix(httpURL, "http://") } + +// TestAwaitUpgradeVerifiesTheHandshake. The Sec-WebSocket-Key was generated +// with crypto/rand and then never looked at again, so "ready" meant nothing +// more than "the status line said 101". Chrome's debug port is a loopback port +// any local process can bind, and being told ready by something that is not a +// WebSocket server is how the probe hands chromedp a socket that will never +// speak CDP. +func TestAwaitUpgradeVerifiesTheHandshake(t *testing.T) { + t.Parallel() + for _, c := range []struct { + name string + reply func(req string) string + }{ + {"a bare 101 with no headers at all", func(string) string { + return "HTTP/9.9 101 whatever\r\n\r\n" + }}, + {"101 with the wrong accept key", func(string) string { + return "HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nSec-WebSocket-Accept: AAAAAAAAAAAAAAAAAAAAAAAAAAA=\r\n\r\n" + }}, + {"101 that never says it upgraded", func(req string) string { + return "HTTP/1.1 101 Switching Protocols\r\nSec-WebSocket-Accept: " + probetest.AcceptFor(probetest.RequestKey(req)) + "\r\n\r\n" + }}, + } { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + ep := probetest.AnswerRaw(t, 0, c.reply) + u := AwaitUpgrade(ep.WS(), UpgradeTimings{PendingAfter: time.Second, Total: time.Second}, nil) + defer u.Close() + if u.State != browser.WSRefused { + t.Errorf("classified %v: a 101 that does not complete OUR handshake is not a Chrome we can attach to", u.State) + } + }) + } +} diff --git a/internal/chrome/record.go b/internal/chrome/record.go index a417859..7cb2249 100644 --- a/internal/chrome/record.go +++ b/internal/chrome/record.go @@ -215,6 +215,33 @@ func (o RecordOpts) withDefaults(maxFrames int) RecordOpts { return o } +// scaleBox turns a viewport and a scale factor into the pixel cap +// startScreencast takes — it wants a maximum SIZE, not a factor. +// +// Split out so the arithmetic half of "--scale halves the capture" is testable +// without a browser: the live half (Chrome honours the cap) needs a renderer, +// this half does not, and keeping them together made the whole claim hostage to +// a viewport that was observed moving mid-test. +func scaleBox(vw, vh, scale float64) (int64, int64) { + return int64(math.Round(vw * scale)), int64(math.Round(vh * scale)) +} + +// pageAgreesViewport reports whether the page's own innerWidth/innerHeight +// matches the box CDP just reported, to within a pixel of rounding. +// +// The two are independent measures that settle independently, so a disagreement +// means the viewport is mid-resize and NEITHER is trustworthy yet. A read error +// counts as agreement: this is a cross-check, not a gate, and a tab that cannot +// answer JS is a reason to fall back to the CDP number rather than to refuse to +// record. +func pageAgreesViewport(ctx context.Context, v Rect) bool { + var wh []float64 + if err := chromedp.Evaluate(`[innerWidth, innerHeight]`, &wh).Do(ctx); err != nil || len(wh) != 2 { + return true + } + return math.Abs(wh[0]-v.Width) <= 1 && math.Abs(wh[1]-v.Height) <= 1 +} + // startRecordCapture registers the screencast listener for a freshly attached // tab. It is called from listenCapture, under c.mu, exactly once per tab. // @@ -288,6 +315,21 @@ func (c *CDP) RecordStart(ctx context.Context, id string, opts RecordOpts) (map[ return err } v := m.viewport + // Cross-check against the page's OWN view of the box. CDP's visual + // viewport and innerWidth/innerHeight settle independently after a + // resize, and CI caught them disagreeing — the page reported the new + // 800x600 while getLayoutMetrics still said 600x600, so the whole + // recording was sized from a square that never existed. Stability in + // one measure is not evidence; agreement between two is. + if v.Width > 0 && v.Height > 0 && !pageAgreesViewport(actx, v) { + agree, prev = 0, Rect{} + select { + case <-actx.Done(): + return actx.Err() + case <-t.C: + } + continue + } if v.Width > 0 && v.Height > 0 && sameRect(prev, v) { if agree++; agree >= wantAgree-1 { vw, vh = v.Width, v.Height @@ -312,8 +354,7 @@ func (c *CDP) RecordStart(ctx context.Context, id string, opts RecordOpts) (map[ })); err != nil { return nil, err } - maxW := int64(math.Round(vw * opts.Scale)) - maxH := int64(math.Round(vh * opts.Scale)) + maxW, maxH := scaleBox(vw, vh, opts.Scale) r := newRecorder(opts, tctx, c.recMaxBytes) // Set before the recorder is published, so nothing can read it concurrently. diff --git a/internal/chrome/record_test.go b/internal/chrome/record_test.go index de6699a..e4440d4 100644 --- a/internal/chrome/record_test.go +++ b/internal/chrome/record_test.go @@ -493,40 +493,38 @@ func TestRecordLive(t *testing.T) { // VS-8: --scale halves the captured frame. // - // Both references are taken here, back to back, rather than comparing the - // long-running recording above against a late capture. A headless window's - // viewport changes as it settles — observed flipping between 756x413 and - // 413x413 within one test — so two captures taken seconds apart can be - // sized from different windows and the comparison says nothing about scale. - // Adjacent captures share whatever the window happens to be, which is what - // makes this an assertion about the scale factor and not about window - // settling. + // Asserted against the box the recorder ASKED Chrome for, not against a + // second capture. The emulated viewport was observed reverting between two + // adjacent recordings (800x600 then 600x600), which makes a frame-to-frame + // comparison a measurement of window stability rather than of --scale. // - // The viewport is pinned first because adjacency alone was not enough: the - // window was still observed changing between two back-to-back captures - // (413x413 then 756x413), which is a property of the headless window and - // not of --scale. Pinning removes that variable; the assertion is a RATIO - // between two captures, so it stays true whatever the box is. - if _, err := b.EmulateViewport(ctx, id, 800, 600); err != nil { - t.Fatalf("EmulateViewport: %v", err) - } - // And WAIT for the override to take effect. setDeviceMetricsOverride - // returns before the visual viewport reports the new size, so capturing - // immediately can size the screencast from a transient reading — observed - // producing a square 300x300 from a 600x600 mid-resize viewport. Polling - // the page's own view of the box is the deterministic precondition. - waitViewport(ctx, t, b, id, 800, 600) - full := recordOneFrame(ctx, t, b, id, RecordOpts{FPS: 8, Scale: 1}) - half := recordOneFrame(ctx, t, b, id, RecordOpts{FPS: 8, Scale: 0.5}) - // Check the precondition before the claim, so a window that moved under the - // test says so instead of being reported as a --scale defect. - if !within(full.Width, 800, 0.15) || !within(full.Height, 600, 0.15) { - t.Fatalf("the scale-1 reference is %dx%d, not the pinned 800x600 — the viewport moved under the test, so this run says nothing about --scale", - full.Width, full.Height) - } - if !within(half.Width, float64(full.Width)*0.5, 0.15) || !within(half.Height, float64(full.Height)*0.5, 0.15) { - t.Errorf("--scale 0.5 produced %dx%d against an unscaled %dx%d, want about half", - half.Width, half.Height, full.Width, full.Height) + // So the claim is decomposed. Here, live: Chrome honours the cap it was + // given, so the frame matches the reported box. In TestScaleSizesTheBox, + // pure: the box is round(viewport x scale). Together those are VS-8, and + // neither half depends on the viewport holding still. + half, meta := recordOneFrame(ctx, t, b, id, RecordOpts{FPS: 8, Scale: 0.5}) + maxW, _ := meta["max_width"].(int64) + maxH, _ := meta["max_height"].(int64) + if maxW <= 0 || maxH <= 0 { + t.Fatalf("RecordStart reported no max box (%v x %v) — nothing to check --scale against", meta["max_width"], meta["max_height"]) + } + // An UPPER BOUND, because that is what the cap actually promises. The + // comment on max_width in record.go says it outright: a screencast frame is + // never upscaled to fill the box, so the real dimensions are whatever the + // compositor produces WITHIN it. Asserting equality contradicted that and + // failed on CI with a legitimate 207x207 frame inside a 378x207 box — the + // window had resized between the box being computed and the frame being + // produced, which is Chrome's business, not this feature's. + // + // What is ours to guarantee is that the cap is honoured as a cap and that a + // scaled recording still produces frames. The arithmetic that makes the cap + // half the viewport is TestScaleSizesTheBox, with no browser involved. + if half.Width <= 0 || half.Height <= 0 { + t.Errorf("--scale 0.5 produced a %dx%d frame — no pixels at all", half.Width, half.Height) + } + if half.Width > int(maxW)+1 || half.Height > int(maxH)+1 { + t.Errorf("--scale 0.5 produced a %dx%d frame, larger than the %dx%d box it asked Chrome for", + half.Width, half.Height, maxW, maxH) } // VS-1: what came back really is an animation. @@ -589,11 +587,13 @@ func TestRecordLive(t *testing.T) { } // recordOneFrame records just long enough to capture a frame, and returns it. -func recordOneFrame(ctx context.Context, t *testing.T, b *CDP, id string, opts RecordOpts) Frame { +func recordOneFrame(ctx context.Context, t *testing.T, b *CDP, id string, opts RecordOpts) (Frame, map[string]any) { t.Helper() - if _, err := b.RecordStart(ctx, id, opts); err != nil { + start, err := b.RecordStart(ctx, id, opts) + if err != nil { t.Fatalf("RecordStart(%+v): %v", opts, err) } + deadline := time.Now().Add(30 * time.Second) for i := 0; time.Now().Before(deadline); i++ { if _, err := b.Eval(ctx, id, fmt.Sprintf("document.body.style.background='rgb(10,%d,90)'", i%200), EvalOpts{}); err != nil { @@ -615,7 +615,7 @@ func recordOneFrame(ctx context.Context, t *testing.T, b *CDP, id string, opts R if len(frames) == 0 { t.Fatal("a reference recording captured no frames") } - return frames[0] + return frames[0], start } // within reports whether got is within tol (a fraction) of want. @@ -663,3 +663,32 @@ func waitViewport(ctx context.Context, t *testing.T, b *CDP, id string, w, h int } t.Fatalf("viewport never reached %dx%d on both the layout and visual measures (last %v)", w, h, got) } + +// The arithmetic half of VS-8: the cap handed to startScreencast is the +// viewport scaled, so --scale 0.5 asks for half. The live half (Chrome honours +// that cap) is in TestRecordLive; neither depends on the viewport holding still, +// which it was observed not doing. +func TestScaleSizesTheBox(t *testing.T) { + t.Parallel() + cases := map[string]struct { + vw, vh, scale float64 + wantW, wantH int64 + }{ + "half": {800, 600, 0.5, 400, 300}, + "full": {800, 600, 1, 800, 600}, + "quarter": {800, 600, 0.25, 200, 150}, + "rounds to even": {801, 601, 0.5, 401, 301}, + "non-square": {1440, 900, 0.5, 720, 450}, + "rounds up at .5": {3, 3, 0.5, 2, 2}, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + gotW, gotH := scaleBox(c.vw, c.vh, c.scale) + if gotW != c.wantW || gotH != c.wantH { + t.Errorf("scaleBox(%v, %v, %v) = %dx%d, want %dx%d", + c.vw, c.vh, c.scale, gotW, gotH, c.wantW, c.wantH) + } + }) + } +} diff --git a/internal/cli/app.go b/internal/cli/app.go index c6c44c2..d9befc1 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -25,29 +25,33 @@ type App struct { in io.Reader // stdin for `session` NDJSON commands (defaults to os.Stdin) // global flags - jsonOut bool - targetFlag string - timeout time.Duration - noLaunch bool - profileDir string - port int - byFlag string - waitFlag string - roleFlag string - nthFlag int - matchFlag string - inRowFlag string // --in-row: scope a --by name match to the row whose text contains this - onDialog string // --on-dialog: auto-handle a native dialog opened during an action (accept|dismiss) - noWait bool - actWaitText string // --wait-text: after an action verb succeeds, wait until this text appears - pierce bool - noDaemon bool - quiet bool - verbose bool - noColor bool - noInput bool - allowFlag []string // --allow: one-off origin allow-list, replacing the configured one - policyOff bool // --policy-off: explicit, logged, never implicit + jsonOut bool + targetFlag string + timeout time.Duration + // consentTimeout is how long the connection holder waits out Chrome's + // browser-modal consent prompt. It is NOT --timeout: a command deadline + // bounds work, this one bounds a human finding a dialog. + consentTimeout time.Duration + noLaunch bool + profileDir string + port int + byFlag string + waitFlag string + roleFlag string + nthFlag int + matchFlag string + inRowFlag string // --in-row: scope a --by name match to the row whose text contains this + onDialog string // --on-dialog: auto-handle a native dialog opened during an action (accept|dismiss) + noWait bool + actWaitText string // --wait-text: after an action verb succeeds, wait until this text appears + pierce bool + noDaemon bool + quiet bool + verbose bool + noColor bool + noInput bool + allowFlag []string // --allow: one-off origin allow-list, replacing the configured one + policyOff bool // --policy-off: explicit, logged, never implicit // verbPath is the running command's full cobra path minus the root // ("click", "cookie set"), captured per Execute in PersistentPreRun. It is @@ -138,10 +142,25 @@ type ConnOpts struct { ProfileDir string Port int NoDaemon bool + // ConsentTimeout travels with the connection options because it is the + // daemon that does the waiting, and the daemon is spawned from these. It is + // always normalised — see connOpts. + ConsentTimeout time.Duration } func (a *App) connOpts() ConnOpts { - return ConnOpts{NoLaunch: a.noLaunch, ProfileDir: a.profileDir, Port: a.port, NoDaemon: a.noDaemon} + return ConnOpts{ + NoLaunch: a.noLaunch, ProfileDir: a.profileDir, Port: a.port, + NoDaemon: a.noDaemon, + // The flag is the last of the three ways this value gets set (config + // resolution clamps the file and the environment), so it is clamped + // here — with the same function, so no layer can read the number + // differently. An explicit `--consent-timeout 0s` otherwise reached + // daemon.Ensure as a literal zero while the daemon it spawned resolved + // the same key to 120s, and the client reported a failure that had not + // happened. + ConsentTimeout: chrome.ClampConsentTimeout(a.consentTimeout), + } } // WithConnector wires a lazy Browser connector (used by main()); it is invoked diff --git a/internal/cli/commands.go b/internal/cli/commands.go index 135d4df..585bb75 100644 --- a/internal/cli/commands.go +++ b/internal/cli/commands.go @@ -12,7 +12,6 @@ import ( "github.com/spf13/cobra" - "github.com/sanketsudake/chrome-cdp-cli/internal/browser" "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" "github.com/sanketsudake/chrome-cdp-cli/internal/result" ) @@ -57,6 +56,7 @@ func (a *App) newRoot() *cobra.Command { pf.BoolVar(&a.jsonOut, "json", d.JSON, "machine-readable output (one JSON value to stdout)") pf.StringVar(&a.targetFlag, "target", "", "tab to act on (idprefix | url: | title: | @N)") pf.DurationVar(&a.timeout, "timeout", d.Timeout, "max time to wait for the command") + pf.DurationVar(&a.consentTimeout, "consent-timeout", d.ConsentTimeout, "how long to wait for Chrome's \"Allow remote debugging?\" prompt to be answered (a refused endpoint still fails fast)") pf.BoolVar(&a.noLaunch, "no-launch", d.NoLaunch, "don't auto-launch a fallback Chrome") pf.BoolVar(&a.noDaemon, "no-daemon", d.NoDaemon, "connect directly instead of via the shared daemon") pf.StringVar(&a.profileDir, "profile-dir", d.ProfileDir, "managed-launch Chrome profile dir (else $CHROME_CDP_PROFILE or ~/.cache/chrome-cdp/profile)") @@ -768,31 +768,6 @@ func (a *App) withWaitText(c *cobra.Command) *cobra.Command { return c } -func (a *App) cmdDoctor() *cobra.Command { - return &cobra.Command{ - Use: "doctor", Short: "Check the Chrome connection and explain how to fix it", - RunE: func(*cobra.Command, []string) error { - pf := browser.FindPortFile("") - if pf == "" { - a.emitErr("doctor", "connection_failed", "no DevToolsActivePort found — enable chrome://inspect/#remote-debugging on your Chrome, or run a command without --no-launch to auto-launch a managed Chrome", nil) - return nil - } - ws, err := browser.WSURLFromPortFile(pf) - if err != nil { - a.emitErr("doctor", "connection_failed", "port file unreadable: "+err.Error(), map[string]any{"port_file": pf}) - return nil - } - // Probe reachability — a stale port file must not report "ready". - if !chrome.Reachable(ws) { - a.emitErr("doctor", "connection_failed", "port file present but the debug endpoint is not reachable (stale) — re-enable chrome://inspect/#remote-debugging", map[string]any{"port_file": pf, "ws": ws}) - return nil - } - a.emitOK("doctor", nil, map[string]any{"port_file": pf, "ws": ws, "status": "debug endpoint reachable — Path B attach ready"}) - return nil - }, - } -} - func (a *App) cmdDaemon() *cobra.Command { daemon := &cobra.Command{Use: "daemon", Short: "Manage the background CDP connection"} emit := func(res map[string]any, err error) { diff --git a/internal/cli/config_wire_test.go b/internal/cli/config_wire_test.go index 727c57a..de494c2 100644 --- a/internal/cli/config_wire_test.go +++ b/internal/cli/config_wire_test.go @@ -5,9 +5,11 @@ package cli import ( "bytes" + "context" "testing" "time" + "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" "github.com/sanketsudake/chrome-cdp-cli/internal/config" "github.com/sanketsudake/chrome-cdp-cli/internal/target" ) @@ -66,3 +68,33 @@ func TestConfigTargetIsFallback(t *testing.T) { t.Fatalf("explicit --target should override config target, exit=%d stderr=%s", code, e2.String()) } } + +// TestConsentTimeoutReachesTheConnector pins the last link of RFC-0013's +// precedence chain: the resolved value has to arrive at the connector, because +// the connector is what hands it to the daemon that does the waiting. A key that +// resolves correctly and is then dropped on the floor is the same as no key. +func TestConsentTimeoutReachesTheConnector(t *testing.T) { + t.Parallel() + capture := func(args ...string) time.Duration { + t.Helper() + var got ConnOpts + var out, errb bytes.Buffer + app := New(nil, &out, &errb). + WithDefaults(config.Defaults{By: "css", Wait: "visible", Timeout: 5 * time.Second, ConsentTimeout: 90 * time.Second}). + WithConnector(func(_ context.Context, o ConnOpts) (chrome.Browser, error) { + got = o + return &fakeBrowser{tabs: []target.Info{{ID: "aa11", Title: "A", URL: "u"}}}, nil + }) + if code := app.Execute(args...); code != 0 { + t.Fatalf("exit = %d, stderr=%s", code, errb.String()) + } + return got.ConsentTimeout + } + + if got := capture("list", "--json"); got != 90*time.Second { + t.Errorf("resolved consent_timeout = %v, want the config value 90s", got) + } + if got := capture("list", "--json", "--consent-timeout", "10s"); got != 10*time.Second { + t.Errorf("explicit --consent-timeout = %v, want 10s (a flag must beat the file)", got) + } +} diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go new file mode 100644 index 0000000..2048a3d --- /dev/null +++ b/internal/cli/doctor.go @@ -0,0 +1,181 @@ +package cli + +import ( + "fmt" + "time" + + "github.com/spf13/cobra" + + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" + "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" + "github.com/sanketsudake/chrome-cdp-cli/internal/result" +) + +// doctorProbeWait is doctor's own probe budget. It is much shorter than the +// connect path's consent budget on purpose: doctor answers a question, it does +// not wait out a dialog, and five seconds of silence from a loopback endpoint +// is already conclusive. A var only so a test can shrink the clock. +var doctorProbeWait = 5 * time.Second + +// doctor reports `state` in the envelope so a caller branches on a value rather +// than on prose. Three of the four values ARE browser.WSState's — the probe's +// answer is the state, and deriving it is what keeps one vocabulary instead of +// two lists that agree by hand until they do not. +// +// stateUnverified is the fourth and has no WSState, because it is the absence +// of a probe rather than the result of one. +const stateUnverified = "unverified" + +// cmdDoctor answers "can I connect?" by actually connecting. A diagnostic that +// reports readiness it did not verify is worse than no diagnostic, because it +// sends the user looking somewhere else. +// +// The awkwardness is that verifying costs a connection, and a connection is a +// consent request — so doctor prefers evidence that costs nothing: a live daemon +// that has just proved its CDP connection is a stronger answer than any probe, +// and asking it touches Chrome not at all. +func (a *App) cmdDoctor() *cobra.Command { + var noProbe bool + c := &cobra.Command{ + Use: "doctor", + Short: "Check the Chrome connection by probing it, and explain how to fix it", + Long: "Report one of three states: no_endpoint, consent_pending, or ready.\n\n" + + "When the background daemon is running, doctor answers through it and opens no\n" + + "new connection. Otherwise it attempts a WebSocket upgrade against the debug\n" + + "endpoint — which is itself a connection request, so Chrome may raise its\n" + + "consent prompt. Pass --no-probe to report only what the port file says.", + RunE: func(*cobra.Command, []string) error { + a.runDoctor(noProbe) + return nil + }, + } + c.Flags().BoolVar(&noProbe, "no-probe", false, "don't open a connection; report only what the DevToolsActivePort file says (unverified)") + return c +} + +func (a *App) runDoctor(noProbe bool) { + // VS-6. A running daemon has already been through the whole ladder and holds + // the connection; re-probing here would raise a second consent request for an + // answer we can get for free. + if via, ok := a.doctorViaDaemon(); ok { + a.emitOK("doctor", nil, via) + return + } + + // --port names a SPECIFIC Chrome, and every other verb resolves it before + // the port file. doctor read the port file directly and never looked at the + // flag, so `doctor --port 9333` diagnosed whichever browser the file + // happened to name and reported that one healthy. + ep := browser.FindEndpoint("", a.port) + if ep.Err != nil { + a.emitErr("doctor", result.CodeConnection, + "the DevToolsActivePort file is unreadable ("+ep.Err.Error()+") — "+browser.EnableAdvice, + map[string]any{"state": browser.WSRefused.String(), "port_file": ep.PortFile}) + return + } + if ep.URL == "" { + a.emitErr("doctor", result.CodeConnection, + "no debug endpoint found (no DevToolsActivePort file) — "+browser.EnableAdvice, + map[string]any{"state": browser.WSRefused.String()}) + return + } + base := map[string]any{"endpoint": ep.URL, "via": "probe", "probed": true} + if ep.PortFile != "" { + base["port_file"] = ep.PortFile + } + if noProbe { + a.emitOK("doctor", nil, map[string]any{ + "endpoint": ep.URL, "port_file": ep.PortFile, "via": "port-file", "probed": false, "state": stateUnverified, + "status": "an endpoint was found, but --no-probe means nothing was verified — a stale port file looks exactly like this", + }) + return + } + + // Open question 3: doctor may probe without a daemon, but it says so first. + // The user asked for a diagnosis, not for a connection, and on the + // chrome://inspect path a connection is what raises the modal prompt. + if !a.quiet { + fmt.Fprintln(a.err, "chrome-cdp doctor: no daemon is running, so this opens one connection to Chrome to verify the endpoint; on the chrome://inspect path that can raise Chrome's consent prompt (use --no-probe to skip)") + } + // An explicit --port names an HTTP endpoint; the browser-level WebSocket + // path has to be resolved before anything can be upgraded against it. + ws, ok := chrome.ResolveWSURL(ep.URL) + if !ok { + a.emitErr("doctor", result.CodeConnection, + "nothing usable answered at "+ep.URL+" (stale port file, or another process on that port) — "+browser.EnableAdvice, + base) + return + } + base["ws"] = ws + + // The probe's answer IS the state; only the prose and the exit code differ + // per outcome. + state := chrome.ProbeWS(ws, doctorProbeWait) + base["state"] = state.String() + switch state { + case browser.WSReady: + // Say what the verdict cost. ProbeWS hangs up on every outcome, + // including this one, so on the chrome://inspect path the consent this + // probe just used is gone and the next command is a fresh attach that + // will prompt again. Handing the live socket on to a connection instead + // is the alternative, and doctor is the wrong place for it: it is a + // diagnostic, it was not asked to connect, and it has nothing to hand + // the socket to. So it is disclosed rather than hidden. + base["status"] = "debug endpoint ready — the WebSocket upgrade completed, so an attach will connect. " + + "This probe's own connection was then closed, so on the chrome://inspect path the next command is a fresh attach and Chrome may prompt again; " + + "start the daemon (chrome-cdp daemon start) to be asked once per session." + a.emitOK("doctor", nil, base) + case browser.WSPending: + a.emitErr("doctor", result.CodeConsentPending, + "the debug endpoint accepted the connection and then went silent. "+browser.ConsentPromptAdvice+ + " To stop being asked at all, "+browser.EnableAdvice+".", + base) + default: + a.emitErr("doctor", result.CodeConnection, + "an endpoint was found but nothing usable answered at "+ws+" (stale port file, or another process on that port) — "+browser.EnableAdvice, + base) + } +} + +// doctorViaDaemon returns the daemon-backed answer when a daemon for this +// endpoint holds a connection it has just PROVED. +// +// The daemon binds its socket only after chrome.Connect succeeded, so liveness +// once looked like evidence — but the socket outlives the connection. Quit +// Chrome and the daemon keeps its listener for the rest of its idle window with +// a dead chromedp connection behind it, so `running: true` is exactly the same +// unverified claim as the DevToolsActivePort file this RFC removed one level +// down. `connected` is the daemon's answer to a round trip it just made to +// Chrome (see the __status dispatch), and nothing short of that earns `ready`: +// anything else falls through to the probe, which asks Chrome itself. +// +// What crosses into the envelope is an ALLOWLIST, not the daemon's map. That +// map used to be copied wholesale, and it carried every open tab's title and +// URL — into `doctor --json`, which the Agent Skill runs as step 1 of every +// session, before any tab has been chosen. The count answers the question a +// diagnostic is asking; the URLs only answer a question nobody asked. +func (a *App) doctorViaDaemon() (map[string]any, bool) { + if a.noDaemon || a.daemonStatus == nil { + return nil, false + } + st, err := a.daemonStatus(a.connOpts()) + if err != nil { + return nil, false + } + if running, _ := st["running"].(bool); !running { + return nil, false + } + if connected, _ := st["connected"].(bool); !connected { + return nil, false + } + res := map[string]any{ + "state": browser.WSReady.String(), "via": "daemon", "probed": false, "running": true, "connected": true, + "status": "debug endpoint ready — the running daemon answered a live CDP round trip (no new connection was opened, so no consent prompt was raised)", + } + for _, k := range []string{"endpoint", "socket", "target_count"} { + if v, ok := st[k]; ok { + res[k] = v + } + } + return res, true +} diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go new file mode 100644 index 0000000..c675034 --- /dev/null +++ b/internal/cli/doctor_test.go @@ -0,0 +1,353 @@ +package cli + +import ( + "bytes" + "encoding/json" + "errors" + "strings" + "testing" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" + "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" + "github.com/sanketsudake/chrome-cdp-cli/internal/probetest" + "github.com/sanketsudake/chrome-cdp-cli/internal/result" +) + +// RFC-0013 VS-5 / VS-6. `doctor` used to read the DevToolsActivePort file and +// report "Path B attach ready" with a ws:// URL, having never connected. During +// the reproduction it said ready while every connection was hanging on an +// unanswered consent prompt. These tests pin the three states it must now +// distinguish, and the one case where it must NOT connect at all. + +// stubEndpoint points CHROME_CDP_PORT_FILE at a stub endpoint in one of the +// shapes doctor has to tell apart, and returns it so a test can assert how many +// connections doctor opened — each one being a consent request. +func stubEndpoint(t *testing.T, answer string) *probetest.Endpoint { + t.Helper() + var ep *probetest.Endpoint + switch answer { + case "": + ep = probetest.Stall(t) // accepts and stalls: consent pending + case "closed": + ep = probetest.Closed(t) // nothing listening: no endpoint + default: + ep = probetest.Answer(t, 0, answer) + } + ep.UsePortFile(t) + return ep +} + +// runDoctorApp runs `doctor --json`, optionally with a daemon-status seam wired. +func runDoctorApp(t *testing.T, status func(ConnOpts) (map[string]any, error), args ...string) (env map[string]any, stderr string, code int) { + t.Helper() + var out, errb bytes.Buffer + app := New(nil, &out, &errb) + if status != nil { + app.WithDaemonCtl(nil, nil, status) + } + code = app.Execute(append([]string{"doctor", "--json"}, args...)...) + if s := strings.TrimSpace(out.String()); strings.HasPrefix(s, "{") { + if err := json.Unmarshal([]byte(s), &env); err != nil { + t.Fatalf("stdout is not one JSON value: %v\n%s", err, s) + } + } + return env, errb.String(), code +} + +func doctorState(t *testing.T, env map[string]any) string { + t.Helper() + if res, ok := env["result"].(map[string]any); ok { + s, _ := res["state"].(string) + return s + } + if e, ok := env["error"].(map[string]any); ok { + s, _ := e["state"].(string) + return s + } + t.Fatalf("envelope has neither result nor error: %v", env) + return "" +} + +func doctorErrCode(env map[string]any) string { + e, _ := env["error"].(map[string]any) + c, _ := e["code"].(string) + return c +} + +// TestDoctorDistinguishesAllThreeStates is VS-5. The ready case is established +// by a COMPLETED upgrade, not by the presence of a port file — that distinction +// is the whole defect. +func TestDoctorDistinguishesAllThreeStates(t *testing.T) { + prev := doctorProbeWait + doctorProbeWait = 400 * time.Millisecond + t.Cleanup(func() { doctorProbeWait = prev }) + + for _, c := range []struct { + name string + answer string + wantState string + wantCode string + wantOK bool + }{ + {"nothing listening", "closed", browser.WSRefused.String(), result.CodeConnection, false}, + {"accepts and stalls", "", browser.WSPending.String(), result.CodeConsentPending, false}, + {"completes the upgrade", "HTTP/1.1 101 Switching Protocols", browser.WSReady.String(), "", true}, + } { + t.Run(c.name, func(t *testing.T) { + stubEndpoint(t, c.answer) + env, stderr, code := runDoctorApp(t, nil) + if env["ok"] != c.wantOK { + t.Fatalf("ok = %v, want %v (envelope %v)", env["ok"], c.wantOK, env) + } + if got := doctorState(t, env); got != c.wantState { + t.Errorf("state = %q, want %q", got, c.wantState) + } + if !c.wantOK { + if got := doctorErrCode(env); got != c.wantCode { + t.Errorf("error.code = %q, want %q", got, c.wantCode) + } + if code != result.ExitConnection { + t.Errorf("exit = %d, want %d", code, result.ExitConnection) + } + } + // Open question 3: probing is itself a connection request, so a + // diagnostic that was not asked to connect says so before it does. + if !strings.Contains(stderr, "opens one connection") { + t.Errorf("doctor probed without warning that it would:\n%s", stderr) + } + }) + } +} + +// TestDoctorConsentPendingNamesTheDialog: the state is only useful if the +// message tells a user staring at a frozen browser what they are looking at. +func TestDoctorConsentPendingNamesTheDialog(t *testing.T) { + prev := doctorProbeWait + doctorProbeWait = 400 * time.Millisecond + t.Cleanup(func() { doctorProbeWait = prev }) + + stubEndpoint(t, "") + env, _, _ := runDoctorApp(t, nil) + e, _ := env["error"].(map[string]any) + msg, _ := e["message"].(string) + if !strings.Contains(msg, browser.ConsentPromptAdvice) { + t.Errorf("the consent_pending message does not carry browser.ConsentPromptAdvice:\n%s", msg) + } + if !strings.Contains(msg, "--remote-debugging-port=9222") { + t.Errorf("the consent_pending message does not name the recovery:\n%s", msg) + } +} + +// TestDoctorAnswersThroughARunningDaemon is VS-6. +// +// Probing is a connection request, and on the chrome://inspect path a connection +// request is what raises the modal prompt. A running daemon is already holding a +// verified connection, so doctor must answer through it and open nothing — +// proved here by counting connections to the endpoint. +func TestDoctorAnswersThroughARunningDaemon(t *testing.T) { + conns := stubEndpoint(t, "") // would classify as consent_pending IF probed + + env, stderr, code := runDoctorApp(t, func(ConnOpts) (map[string]any, error) { + return map[string]any{"running": true, "connected": true, "socket": "/tmp/x.sock", "target_count": 3}, nil + }) + + if env["ok"] != true || code != result.ExitOK { + t.Fatalf("doctor with a live daemon: ok=%v exit=%d (%v)", env["ok"], code, env) + } + if got := doctorState(t, env); got != browser.WSReady.String() { + t.Errorf("state = %q, want %q", got, browser.WSReady.String()) + } + res := env["result"].(map[string]any) + if res["via"] != "daemon" { + t.Errorf("via = %v, want daemon", res["via"]) + } + if res["probed"] != false { + t.Errorf("probed = %v, want false", res["probed"]) + } + if res["target_count"] != float64(3) { + t.Errorf("the daemon's own status fields should survive into the envelope: %v", res) + } + if n := conns.Conns(); n != 0 { + t.Errorf("doctor opened %d connection(s) to Chrome while a daemon was running — each one is a fresh consent request", n) + } + if strings.Contains(stderr, "opens one connection") { + t.Errorf("doctor announced a probe it did not make:\n%s", stderr) + } +} + +// TestDoctorRequiresEvidenceFromTheDaemon is the second-order version of the +// defect this RFC's item 3 names: doctor stopped trusting the port file and +// started trusting `running: true` instead, which is just as unverified. +// +// The trigger is ordinary: start a daemon, quit Chrome. The chromedp connection +// is dead, but the daemon holds its listener for the whole idle window, so +// TryConnect still succeeds. Every state short of a daemon that has proved its +// connection must fall through to the probe rather than report ready. +func TestDoctorRequiresEvidenceFromTheDaemon(t *testing.T) { + prev := doctorProbeWait + doctorProbeWait = 400 * time.Millisecond + t.Cleanup(func() { doctorProbeWait = prev }) + + for _, c := range []struct { + name string + status map[string]any + err error + }{ + {"running but the CDP connection is dead", map[string]any{"running": true, "connected": false}, nil}, + {"running with no connection evidence at all", map[string]any{"running": true}, nil}, + {"the status call itself failed", nil, errors.New("dial unix: connection refused")}, + } { + t.Run(c.name, func(t *testing.T) { + // A stalling endpoint: if doctor falls through and probes, it says + // consent_pending. Anything claiming `ready` came from the daemon. + stubEndpoint(t, "") + env, _, _ := runDoctorApp(t, func(ConnOpts) (map[string]any, error) { return c.status, c.err }) + if got := doctorState(t, env); got == browser.WSReady.String() { + t.Errorf("doctor reported %q from a daemon that never proved a live CDP connection: %v", got, env) + } + }) + } +} + +// TestDoctorDoesNotLeakOpenTabURLs. SKILL.md makes `doctor --json` step 1 of +// every agent session, so anything doctor echoes is pulled into the transcript +// before a tab has even been selected. Blanket-copying the daemon's status map +// put every open tab's title and full URL there — OAuth callbacks, reset +// tokens, internal hostnames. +func TestDoctorDoesNotLeakOpenTabURLs(t *testing.T) { + stubEndpoint(t, "") + env, _, _ := runDoctorApp(t, func(ConnOpts) (map[string]any, error) { + return map[string]any{ + "running": true, "connected": true, "socket": "/tmp/x.sock", + "targets": []map[string]any{ + {"id": "1", "title": "Reset your password", "url": "https://intranet.example/reset?token=s3cret"}, + }, + "target_count": 1, + }, nil + }) + blob, err := json.Marshal(env) + if err != nil { + t.Fatalf("marshal: %v", err) + } + for _, leak := range []string{"s3cret", "Reset your password", "intranet.example"} { + if strings.Contains(string(blob), leak) { + t.Errorf("doctor echoed %q into the envelope:\n%s", leak, blob) + } + } + res, _ := env["result"].(map[string]any) + if res["target_count"] != float64(1) { + t.Errorf("target_count = %v, want 1 — the count is the useful part, the URLs are not", res["target_count"]) + } +} + +// TestDoctorNoDaemonStatusStillProbes: a daemon that is not running is not an +// answer, so doctor falls through to the probe rather than reporting ready. +func TestDoctorNoDaemonStatusStillProbes(t *testing.T) { + conns := stubEndpoint(t, "HTTP/1.1 101 Switching Protocols") + env, _, _ := runDoctorApp(t, func(ConnOpts) (map[string]any, error) { + return map[string]any{"running": false}, nil + }) + if got := doctorState(t, env); got != browser.WSReady.String() { + t.Errorf("state = %q, want %q", got, browser.WSReady.String()) + } + if env["result"].(map[string]any)["via"] != "probe" { + t.Errorf("with no daemon the answer must come from a probe: %v", env["result"]) + } + if n := conns.Conns(); n != 1 { + t.Errorf("probed with %d connections, want exactly 1", n) + } +} + +// TestDoctorNoProbeRefusesToClaimReadiness: the escape hatch for a user who does +// not want a diagnostic to open a connection must not resurrect the old lie. +func TestDoctorNoProbeRefusesToClaimReadiness(t *testing.T) { + conns := stubEndpoint(t, "") + env, _, _ := runDoctorApp(t, nil, "--no-probe") + if got := doctorState(t, env); got == browser.WSReady.String() { + t.Error("--no-probe reported ready without verifying anything, which is the bug this RFC exists to fix") + } + if n := conns.Conns(); n != 0 { + t.Errorf("--no-probe opened %d connection(s), want 0", n) + } +} + +// TestConsentTimeoutFlagIsNormalised: the flag is the third way this value can +// be set, and the only one config resolution does not see. An explicit +// `--consent-timeout 0s` used to reach daemon.Ensure as a literal zero while +// the daemon it spawned resolved the same key to 120s from its environment — +// so the client reported "still waiting ... after 0s" for a daemon that was +// still holding the prompt open. +func TestConsentTimeoutFlagIsNormalised(t *testing.T) { + prev := doctorProbeWait + doctorProbeWait = 100 * time.Millisecond + t.Cleanup(func() { doctorProbeWait = prev }) + stubEndpoint(t, "") + for _, c := range []struct { + flag string + want time.Duration + }{ + {"0s", chrome.DefaultConsentTimeout}, + {"-3s", chrome.DefaultConsentTimeout}, + {"8760h", chrome.MaxConsentTimeout}, + {"45s", 45 * time.Second}, + } { + t.Run(c.flag, func(t *testing.T) { + var got time.Duration + runDoctorApp(t, func(o ConnOpts) (map[string]any, error) { + got = o.ConsentTimeout + return map[string]any{"running": false}, nil + }, "--consent-timeout", c.flag) + if got != c.want { + t.Errorf("--consent-timeout %s reached the connector as %v, want %v", c.flag, got, c.want) + } + }) + } +} + +// TestDoctorHonoursExplicitPort. Every other verb resolves its endpoint from +// --port before the DevToolsActivePort file; doctor called FindPortFile("") and +// never looked at the flag. So `doctor --port 9333` probed whatever Chrome the +// port file happened to name and pronounced THAT one healthy — a diagnostic +// answering a question about a different browser than the one asked about. +func TestDoctorHonoursExplicitPort(t *testing.T) { + prev := doctorProbeWait + doctorProbeWait = 300 * time.Millisecond + t.Cleanup(func() { doctorProbeWait = prev }) + + // The port file names a perfectly healthy endpoint... + stubEndpoint(t, "HTTP/1.1 101 Switching Protocols") + + // ...and --port names one that is holding a consent prompt. + stalled := probetest.Stall(t) + + env, _, _ := runDoctorApp(t, nil, "--port", stalled.Port()) + if got := doctorState(t, env); got != browser.WSPending.String() { + t.Errorf("state = %q, want %q — doctor diagnosed a different Chrome than --port named: %v", got, browser.WSPending.String(), env) + } + if stalled.Conns() == 0 { + t.Error("doctor never contacted the --port endpoint at all") + } +} + +// TestDoctorReadyViaProbeSaysTheClickWillBeSpent. +// +// browser.Upgrade's doc comment states the governing model: "a probe that +// connects, learns the answer and hangs up has spent the user's click on a +// connection nobody kept". ProbeWS closes the socket on every outcome including +// WSReady, so doctor's `ready` verdict is falsified by the act of producing it +// — on the chrome://inspect path the next command raises a second prompt. The +// verdict is still worth having; it just has to say what it cost. +func TestDoctorReadyViaProbeSaysTheClickWillBeSpent(t *testing.T) { + prev := doctorProbeWait + doctorProbeWait = 400 * time.Millisecond + t.Cleanup(func() { doctorProbeWait = prev }) + + stubEndpoint(t, "HTTP/1.1 101 Switching Protocols") + env, _, _ := runDoctorApp(t, nil) + res, _ := env["result"].(map[string]any) + status, _ := res["status"].(string) + if !strings.Contains(status, "prompt again") { + t.Errorf("doctor reported ready without saying the probe's connection was closed and the next command may prompt again:\n%s", status) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index baeb4a8..d154dfc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -21,16 +21,22 @@ import ( // Defaults are the effective global-flag defaults after the config file and // CHROME_CDP_* env are merged over the built-in values. type Defaults struct { - Timeout time.Duration - By string - Wait string - Target string - Port int - ProfileDir string - NoLaunch bool - NoDaemon bool - JSON bool - NoColor bool + Timeout time.Duration + // ConsentTimeout bounds the wait for Chrome's browser-modal "Allow remote + // debugging?" prompt (config key consent_timeout). It is separate from + // Timeout because it is not a command deadline at all: it is how long a + // human is given to find and click a dialog that may be behind the window, + // so it is measured in minutes where Timeout is measured in seconds. + ConsentTimeout time.Duration + By string + Wait string + Target string + Port int + ProfileDir string + NoLaunch bool + NoDaemon bool + JSON bool + NoColor bool // Policy is the optional [policy] table (RFC-0012). No CHROME_CDP_* variable // sets any of its keys: a safety boundary whose CONTENTS an inherited @@ -103,7 +109,8 @@ type Policy struct { // the environment sets a value. func Builtin() Defaults { return Defaults{ - Timeout: 30 * time.Second, By: "css", Wait: "visible", + Timeout: 30 * time.Second, ConsentTimeout: chrome.DefaultConsentTimeout, + By: "css", Wait: "visible", ConsoleBuffer: chrome.DefaultConsoleBuffer, ConsoleMaxEntry: chrome.DefaultConsoleMaxEntry, NetBuffer: chrome.DefaultNetBuffer, NetMaxBody: chrome.DefaultNetMaxBody, RecordBuffer: chrome.DefaultRecordFrames, RecordMaxBytes: chrome.DefaultRecordMaxBytes, @@ -113,16 +120,17 @@ func Builtin() Defaults { // file mirrors the TOML schema; pointer fields distinguish "set in file" from // "absent", so an omitted key leaves the built-in (or env) value intact. type file struct { - Timeout *string `toml:"timeout"` - By *string `toml:"by"` - Wait *string `toml:"wait"` - Target *string `toml:"target"` - Port *int `toml:"port"` - ProfileDir *string `toml:"profile_dir"` - NoLaunch *bool `toml:"no_launch"` - NoDaemon *bool `toml:"no_daemon"` - JSON *bool `toml:"json"` - NoColor *bool `toml:"no_color"` + Timeout *string `toml:"timeout"` + ConsentTimeout *string `toml:"consent_timeout"` + By *string `toml:"by"` + Wait *string `toml:"wait"` + Target *string `toml:"target"` + Port *int `toml:"port"` + ProfileDir *string `toml:"profile_dir"` + NoLaunch *bool `toml:"no_launch"` + NoDaemon *bool `toml:"no_daemon"` + JSON *bool `toml:"json"` + NoColor *bool `toml:"no_color"` ConsoleBuffer *int `toml:"console_buffer"` ConsoleMaxEntry *int `toml:"console_max_entry"` @@ -175,6 +183,7 @@ func ResolveFrom(path string, getenv func(string) string) (Defaults, error) { d := Builtin() err := applyFile(&d, path) applyEnv(&d, getenv) + normalise(&d) return d, err } @@ -184,9 +193,25 @@ func ResolveFrom(path string, getenv func(string) string) (Defaults, error) { func FromEnv() Defaults { d := Builtin() applyEnv(&d, os.Getenv) + normalise(&d) return d } +// normalise pulls resolved values into the range the rest of the program is +// entitled to assume. This is the single place it happens: resolution is where +// flag defaults, environment and config file meet, so a value that is sane here +// is sane in every layer downstream. +// +// The consent budget is the one that needed it. A zero (from consent_timeout = +// "0s", or an env var someone cleared) meant "the default" to chrome.Connect +// and "no wait at all" to daemon.Ensure, which put back the orphaned-prompt +// failure the setting exists to prevent — and an inherited +// CHROME_CDP_CONSENT_TIMEOUT=8760h would have held the daemon spawn lock for a +// year. +func normalise(d *Defaults) { + d.ConsentTimeout = chrome.ClampConsentTimeout(d.ConsentTimeout) +} + // applyFile overlays a config file onto d. A missing file is not an error; a // present-but-malformed file is (returned so the caller can warn), and d is // left at its built-in values in that case. @@ -238,6 +263,11 @@ func applyFile(d *Defaults, path string) error { d.Timeout = t } } + if f.ConsentTimeout != nil { + if t, err := time.ParseDuration(*f.ConsentTimeout); err == nil { + d.ConsentTimeout = t + } + } if f.By != nil { d.By = *f.By } @@ -375,6 +405,11 @@ func applyEnv(d *Defaults, getenv func(string) string) { d.Timeout = t } } + if v := getenv("CHROME_CDP_CONSENT_TIMEOUT"); v != "" { + if t, err := time.ParseDuration(v); err == nil { + d.ConsentTimeout = t + } + } if v := getenv("CHROME_CDP_BY"); v != "" { d.By = v } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e23d0a7..27e99c1 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -560,3 +560,44 @@ func TestConsoleBufferPrecedence(t *testing.T) { } }) } + +// TestConsentTimeoutPrecedence pins RFC-0013's new key through the whole +// precedence chain. It matters more than most: the value decides how long a +// daemon holds a connection open waiting for a human, so a config file that is +// silently ignored means the wedge this key exists to prevent. +func TestConsentTimeoutPrecedence(t *testing.T) { + t.Parallel() + + // Built-in: two minutes, a human timescale for a dialog that can hide behind + // the window. + d, _ := ResolveFrom(filepath.Join(t.TempDir(), "absent.toml"), noEnv) + if d.ConsentTimeout != chrome.DefaultConsentTimeout { + t.Errorf("built-in consent_timeout = %v, want %v", d.ConsentTimeout, chrome.DefaultConsentTimeout) + } + + p := writeConfig(t, "consent_timeout = \"45s\"\n") + d, err := ResolveFrom(p, noEnv) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if d.ConsentTimeout != 45*time.Second { + t.Errorf("config consent_timeout = %v, want 45s", d.ConsentTimeout) + } + + // Env beats the file. + d, _ = ResolveFrom(p, envFrom(map[string]string{"CHROME_CDP_CONSENT_TIMEOUT": "3m"})) + if d.ConsentTimeout != 3*time.Minute { + t.Errorf("env consent_timeout should win, got %v", d.ConsentTimeout) + } + + // A malformed value leaves the lower-precedence value in place rather than + // bricking the connection, matching every other duration key here. + d, _ = ResolveFrom(p, envFrom(map[string]string{"CHROME_CDP_CONSENT_TIMEOUT": "banana"})) + if d.ConsentTimeout != 45*time.Second { + t.Errorf("a malformed env value should leave the file value alone, got %v", d.ConsentTimeout) + } + d, _ = ResolveFrom(writeConfig(t, "consent_timeout = \"nope\"\n"), noEnv) + if d.ConsentTimeout != chrome.DefaultConsentTimeout { + t.Errorf("a malformed file value should leave the built-in alone, got %v", d.ConsentTimeout) + } +} diff --git a/internal/config/consent_test.go b/internal/config/consent_test.go new file mode 100644 index 0000000..3bb7834 --- /dev/null +++ b/internal/config/consent_test.go @@ -0,0 +1,71 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" +) + +// The consent budget crosses four layers — flag, config file, env, and the +// daemon's own environment — and every one of them used to be free to read a +// zero or an absurd value differently. chrome.Connect mapped <= 0 to the +// default, daemon.Ensure did not, and main forwarded the env var only when +// > 0: with consent_timeout = "0s" the daemon waited 120s while its client gave +// up at 10s and reported "still waiting ... after 0s", which is the +// orphaned-prompt failure the parameter exists to prevent, restored through the +// zero value. +// +// Resolution is where flag/env/config meet, so it is where the value is made +// sane, once. + +func TestResolveClampsTheConsentTimeout(t *testing.T) { + for _, c := range []struct { + name string + file string + env string + want time.Duration + }{ + {"unset", "", "", chrome.DefaultConsentTimeout}, + {"zero means the default, not no wait", `consent_timeout = "0s"`, "", chrome.DefaultConsentTimeout}, + {"negative means the default", `consent_timeout = "-5s"`, "", chrome.DefaultConsentTimeout}, + {"a sane value survives", `consent_timeout = "45s"`, "", 45 * time.Second}, + {"an inherited year does not hold the spawn lock for a year", "", "8760h", chrome.MaxConsentTimeout}, + {"a sub-second value is raised to the floor", "", "10ms", chrome.MinConsentTimeout}, + {"env zero means the default too", "", "0s", chrome.DefaultConsentTimeout}, + } { + t.Run(c.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.toml") + if c.file != "" { + if err := os.WriteFile(path, []byte(c.file+"\n"), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + } + getenv := func(k string) string { + if k == "CHROME_CDP_CONSENT_TIMEOUT" { + return c.env + } + return "" + } + d, err := ResolveFrom(path, getenv) + if err != nil { + t.Fatalf("ResolveFrom: %v", err) + } + if d.ConsentTimeout != c.want { + t.Errorf("ConsentTimeout = %v, want %v", d.ConsentTimeout, c.want) + } + }) + } +} + +// TestFromEnvClampsTheConsentTimeout: the daemon subprocess resolves its +// options from the environment alone, so it needs the same clamp — otherwise +// the two processes that must agree on this number are the two that disagree. +func TestFromEnvClampsTheConsentTimeout(t *testing.T) { + t.Setenv("CHROME_CDP_CONSENT_TIMEOUT", "0s") + if got := FromEnv().ConsentTimeout; got != chrome.DefaultConsentTimeout { + t.Errorf("ConsentTimeout = %v, want %v", got, chrome.DefaultConsentTimeout) + } +} diff --git a/internal/daemon/consent_test.go b/internal/daemon/consent_test.go new file mode 100644 index 0000000..10e9500 --- /dev/null +++ b/internal/daemon/consent_test.go @@ -0,0 +1,244 @@ +package daemon + +import ( + "context" + "errors" + "net" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" + "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" + "github.com/sanketsudake/chrome-cdp-cli/internal/result" +) + +// RFC-0013. Ensure has to know about the consent prompt even though the daemon +// is the process doing the waiting: the daemon is detached, so a client that +// gave up on its own ten-second clock would report a failure that had not +// happened and leave a live daemon behind holding a connection nobody uses. + +// shrinkStartupWait shortens both the plain startup budget and the grace on top +// of the consent budget, so these run in milliseconds. +func shrinkStartupWait(t *testing.T, d time.Duration) { + t.Helper() + prev := startupWait + startupWait = d + t.Cleanup(func() { startupWait = prev }) +} + +// captureNotices redirects the advisories Ensure prints while it waits. The +// mutex is not decoration: lockSpawn's contention notice comes from whichever +// goroutine is blocked on the lock, not from the caller's. +func captureNotices(t *testing.T) func() []string { + t.Helper() + var mu sync.Mutex + var got []string + prev := notice + notice = func(msg string) { + mu.Lock() + defer mu.Unlock() + got = append(got, msg) + } + t.Cleanup(func() { notice = prev }) + return func() []string { + mu.Lock() + defer mu.Unlock() + return append([]string(nil), got...) + } +} + +// bindAfter makes a fake daemon that binds sockPath after delay, so the socket +// becomes connectable at a time the test controls. +func bindAfter(t *testing.T, delay time.Duration) func(sockPath string) { + t.Helper() + return func(sockPath string) { + go func() { + time.Sleep(delay) + ln, err := net.Listen("unix", sockPath) + if err != nil { + return + } + t.Cleanup(func() { _ = ln.Close() }) + for { + c, err := ln.Accept() + if err != nil { + return + } + _ = c.Close() + } + }() + } +} + +// TestEnsureWaitsOutTheConsentPrompt is VS-1 from the client side: a daemon that +// says it is holding a consent prompt is waited for, not declared dead. +func TestEnsureWaitsOutTheConsentPrompt(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + shrinkStartupWait(t, 300*time.Millisecond) + notices := captureNotices(t) + + bind := bindAfter(t, time.Second) // ~3x the plain startup budget + restore := swapSpawn(func(_, sockPath string, _ []string) (*daemonProc, error) { + // The real daemon publishes this the moment chrome.Connect classifies the + // upgrade as pending — while the dialog is still on screen. + if err := os.WriteFile(sockPath+pendingSuffix, []byte("waiting\n"), 0o600); err != nil { + t.Errorf("write pending sidecar: %v", err) + } + bind(sockPath) + return liveProc(t), nil + }) + defer restore() + + start := time.Now() + c, err := Ensure(context.Background(), sock, "unused", nil, 3*time.Second) + if err != nil { + t.Fatalf("Ensure gave up on a daemon that was waiting for consent: %v", err) + } + if c == nil { + t.Fatal("nil client and no error") + } + if el := time.Since(start); el < 900*time.Millisecond { + t.Errorf("connected after %v, before the daemon was up — the test is not exercising the wait", el) + } + said := strings.Join(notices(), "\n") + if said == "" { + t.Error("nothing was said while waiting; a user staring at a frozen browser has to be told it is a dialog") + } else if !strings.Contains(said, browser.ConsentPromptAdvice) { + t.Errorf("the wait notice does not carry browser.ConsentPromptAdvice:\n%s", said) + } +} + +// TestEnsureBoundsTheConsentWait is VS-4 from the client side: patient, not +// infinite. +func TestEnsureBoundsTheConsentWait(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + shrinkStartupWait(t, 200*time.Millisecond) + captureNotices(t) + + restore := swapSpawn(func(_, sockPath string, _ []string) (*daemonProc, error) { + return liveProc(t), os.WriteFile(sockPath+pendingSuffix, []byte("waiting\n"), 0o600) + }) + defer restore() + + start := time.Now() + _, err := Ensure(context.Background(), sock, "unused", nil, 500*time.Millisecond) + elapsed := time.Since(start) + + var ce *chrome.ConnectError + if !errors.As(err, &ce) { + t.Fatalf("error %v is not a *ConnectError, so no stable code reaches the envelope", err) + } + if ce.Code != result.CodeConsentPending { + t.Errorf("error.code = %q, want %q", ce.Code, result.CodeConsentPending) + } + if elapsed < 500*time.Millisecond { + t.Errorf("gave up after %v, inside the consent budget", elapsed) + } + if elapsed > 5*time.Second { + t.Errorf("waited %v — the consent wait must be bounded", elapsed) + } +} + +// TestEnsureFailsFastWithoutAPendingPrompt keeps the long wait from leaking into +// the ordinary broken-daemon case: no pending marker, no extension. +func TestEnsureFailsFastWithoutAPendingPrompt(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + shrinkStartupWait(t, 300*time.Millisecond) + + restore := swapSpawn(func(string, string, []string) (*daemonProc, error) { return liveProc(t), nil }) // never binds + defer restore() + + start := time.Now() + _, err := Ensure(context.Background(), sock, "unused", nil, 60*time.Second) + elapsed := time.Since(start) + + var ce *chrome.ConnectError + if !errors.As(err, &ce) || ce.Code != result.CodeDaemon { + t.Errorf("error = %v, want a daemon_error", err) + } + if elapsed > 3*time.Second { + t.Errorf("a daemon that never came up took %v — only a PENDING one earns the consent budget", elapsed) + } +} + +// TestRunDaemonPublishesPendingWhileWaiting is VS-1 end to end within the daemon +// process: the pending state is published DURING the wait (so the CLI can say so +// while the dialog is up), and the connect failure that eventually crosses the +// process boundary keeps its consent_pending code. +func TestRunDaemonPublishesPendingWhileWaiting(t *testing.T) { + // A listener that accepts and stalls is exactly the consent-pending state. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer ln.Close() + go func() { + var held []net.Conn + defer func() { + for _, c := range held { + _ = c.Close() + } + }() + for { + c, err := ln.Accept() + if err != nil { + return + } + held = append(held, c) + } + }() + + dir := shortTempDir(t) + _, port, _ := net.SplitHostPort(ln.Addr().String()) + pf := filepath.Join(dir, "DevToolsActivePort") + if err := os.WriteFile(pf, []byte(port+"\n/devtools/browser/stub\n"), 0o600); err != nil { + t.Fatalf("write port file: %v", err) + } + sock := filepath.Join(dir, "d.sock") + + done := make(chan error, 1) + go func() { + done <- RunDaemon(sock, chrome.Options{ + PortFile: pf, NoLaunch: true, ConsentTimeout: 3 * time.Second, + // The marker is published at this threshold, and the poll below + // has three seconds to see it. With the production 2s threshold + // the two were 3.05s and ~2.0s apart — a 1.5x margin on exactly + // the kind of timing that has flaked in CI here four times. + ConsentPendingAfter: 100 * time.Millisecond, + }, time.Minute) + }() + + sawPending := false + for deadline := time.Now().Add(3 * time.Second); time.Now().Before(deadline); { + if _, err := os.Stat(sock + pendingSuffix); err == nil { + sawPending = true + break + } + time.Sleep(50 * time.Millisecond) + } + if !sawPending { + t.Error("the daemon never published the pending marker while it waited — the CLI has no way to know it is a dialog and not a hang") + } + + select { + case <-done: + case <-time.After(15 * time.Second): + t.Fatal("RunDaemon never returned; the consent wait is unbounded") + } + + data, err := os.ReadFile(sock + errSuffix) + if err != nil { + t.Fatalf("the daemon left no error sidecar: %v", err) + } + var ce *chrome.ConnectError + if !errors.As(decodeConnectErr(data), &ce) || ce.Code != result.CodeConsentPending { + t.Errorf("the sidecar decodes to %v, want a consent_pending ConnectError", decodeConnectErr(data)) + } + if _, err := os.Stat(sock + pendingSuffix); err == nil { + t.Error("the pending marker outlived the wait; a later run would read a stale prompt") + } +} diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index a7aef29..e6e5303 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -12,6 +12,7 @@ import ( "encoding/json" "errors" "io" + "maps" "net" "sync" "time" @@ -273,13 +274,20 @@ func (s *server) dispatch(ctx context.Context, method string, args []json.RawMes b := s.b switch method { case "__status": - // Best-effort: report the tabs the daemon can currently see. A List - // failure just omits them rather than failing the status call. - info := map[string]any{"connected": true} - if tabs, err := b.List(ctx); err == nil { - info["targets"] = tabs - } - return info, nil + // `connected` is the ANSWER TO the List below, never an assumption. A + // daemon whose Chrome has quit still owns its listener for the rest of + // the idle window, so being reachable proves only that the process is + // alive; the round trip to Chrome is the only thing that proves the CDP + // connection is. Hardcoding true here is what let `doctor` report + // "ready" for a browser that had been closed. + // + // Only the COUNT of tabs crosses the socket. The full target list + // carries every open tab's title and URL, and this payload is echoed by + // `doctor --json`, which the Agent Skill runs as step 1 of every + // session — that is a transcript full of OAuth callbacks and reset + // tokens for a question that was only ever "can I connect?". + tabs, err := b.List(ctx) + return map[string]any{"connected": err == nil, "target_count": len(tabs)}, nil case "List": return b.List(ctx) case "Open": @@ -565,7 +573,7 @@ func (c *Client) Status() error { return c.call(ctx, "__status", nil) } -// StatusInfo returns the daemon's status payload: {connected, targets}. +// StatusInfo returns the daemon's status payload: {connected, target_count}. func (c *Client) StatusInfo() (map[string]any, error) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -573,6 +581,30 @@ func (c *Client) StatusInfo() (map[string]any, error) { return out, c.call(ctx, "__status", &out) } +// Status reports whether the daemon for this endpoint is running and, when it +// is, what its connection to Chrome actually did. +// +// The StatusInfo error is PROPAGATED, not swallowed. A daemon that answers its +// socket and then fails the status call is precisely the interesting case — it +// is alive and its CDP connection is not — and discarding the error left +// `running: true` standing as if nothing had gone wrong, which is one of the +// three unverified claims that let `doctor` say ready. +func Status(sock, endpoint string) (map[string]any, error) { + res := map[string]any{"socket": sock, "endpoint": endpoint} + c := TryConnect(sock) + if c == nil { + res["running"] = false + return res, nil + } + info, err := c.StatusInfo() + if err != nil { + return nil, err + } + res["running"] = true + maps.Copy(res, info) + return res, nil +} + // Stop asks the daemon to shut down. func (c *Client) Stop() error { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) diff --git a/internal/daemon/daemon_test.go b/internal/daemon/daemon_test.go index 0a98276..1cec3e5 100644 --- a/internal/daemon/daemon_test.go +++ b/internal/daemon/daemon_test.go @@ -227,7 +227,7 @@ func TestEnsureConnectsToExisting(t *testing.T) { // A daemon is already listening, so Ensure connects without spawning (the // exe path is never used). - c, err := Ensure(sock, "/nonexistent-exe", nil) + c, err := Ensure(t.Context(), sock, "/nonexistent-exe", nil, time.Minute) if err != nil { t.Fatalf("Ensure should connect to the running daemon: %v", err) } diff --git a/internal/daemon/lifecycle.go b/internal/daemon/lifecycle.go index 0118289..f7ea1cf 100644 --- a/internal/daemon/lifecycle.go +++ b/internal/daemon/lifecycle.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "net" "os" "os/exec" @@ -12,6 +13,7 @@ import ( "syscall" "time" + "github.com/sanketsudake/chrome-cdp-cli/internal/browser" "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" "github.com/sanketsudake/chrome-cdp-cli/internal/result" ) @@ -80,6 +82,36 @@ func decodeConnectErr(data []byte) error { return errors.New(e.Message) } +// Sidecar files the daemon leaves next to its socket so its state crosses the +// process boundary: the spawned daemon is detached and has no stderr the user +// will ever read. +const ( + errSuffix = ".err" // a connect failure, with its stable code + pendingSuffix = ".pending" // "I am waiting on Chrome's consent prompt" + lockSuffix = ".lock" // the spawn-and-wait exclusion (see lockSpawn) +) + +// startupWait is how long a daemon gets to come up before Ensure gives up, and +// (after a pending prompt is seen) the grace on top of the consent budget. It is +// short because a daemon that is going to work works immediately — EXCEPT when +// Chrome is asking the user for consent, which is why the pending sidecar +// extends the deadline rather than this being large. A var only so tests can +// shrink the clock. +var startupWait = 10 * time.Second + +// notice prints a one-line advisory to the user while Ensure waits. It is a var +// so a test can capture it without a terminal. +var notice = func(msg string) { fmt.Fprintln(os.Stderr, "chrome-cdp:", msg) } + +// lockWaitNotice is said when another chrome-cdp already holds the spawn lock — +// which, when a prompt is pending, means this command is about to wait minutes. +const lockWaitNotice = "another chrome-cdp is already starting the connection; waiting for it rather than opening a second one " + + "(a second connection would raise a second consent prompt)." + +// consentWaitNotice is said WHILE the dialog is on screen, which is the only +// time it can help. Told afterwards it is a post-mortem. +const consentWaitNotice = browser.ConsentPromptAdvice + " Nothing else is needed; this command is waiting for you." + // TryConnect returns a Client if a daemon is already listening on sockPath. func TryConnect(sockPath string) *Client { conn, err := net.DialTimeout("unix", sockPath, 500*time.Millisecond) @@ -92,51 +124,301 @@ func TryConnect(sockPath string) *Client { // Ensure connects to a running daemon, or spawns one (detached) and waits for it // to come up. env carries the connection options (CHROME_CDP_*) for the daemon. -func Ensure(sockPath, exePath string, env []string) (*Client, error) { +// +// consentTimeout is how long the spawned daemon is allowed to spend waiting out +// Chrome's consent prompt. Ensure has to know it too: the daemon's wait is +// invisible from here, and a client that gave up at ten seconds while its daemon +// was still holding the connection would report a failure that had not happened. +// It arrives normalised (see chrome.ClampConsentTimeout). +// +// ctx bounds the whole thing, including the wait for the spawn lock. Without +// it, --timeout stopped applying the moment a command needed a connection: the +// lock's holder may be sitting on an unanswered prompt for two minutes, and +// every caller behind it inherited that wait with no way to say otherwise. +func Ensure(ctx context.Context, sockPath, exePath string, env []string, consentTimeout time.Duration) (*Client, error) { + if c := TryConnect(sockPath); c != nil { + return c, nil + } + + // From here on, exactly one process at a time. Concurrent invocations that + // all find no daemon would otherwise each spawn one, and each spawned daemon + // attaches to Chrome — which raises a SEPARATE browser-modal "Allow remote + // debugging?" prompt. Several stacked prompts is not a slower version of + // one: the visible dialog need not be the one holding input, so the whole + // browser looks frozen with no button that responds. The daemon exists so + // that prompt happens once per session; nothing was making the FIRST attach + // single-file. + // + // The unlinks below are the other half. Outside the lock they can delete a + // socket a sibling daemon has just bound, orphaning a live daemon that no + // client can ever reach. + unlock, err := lockSpawn(ctx, sockPath) + if err != nil { + return nil, err + } + defer unlock() + + // Re-check under the lock: while we waited, the holder may have started the + // daemon we were about to duplicate. This is what makes N callers converge + // on one daemon and one prompt. if c := TryConnect(sockPath); c != nil { return c, nil } - _ = os.Remove(sockPath) // clear a stale socket file - _ = os.Remove(sockPath + ".err") // and a stale error, so we only read THIS spawn's + // The holder may instead have come back with a verdict, and a FRESH + // consent_pending is one to inherit rather than re-derive. Deriving it + // again means spawning a daemon, attaching, and raising a second prompt at + // a browser that is already holding one — so eight queued callers came to + // eight sequential prompts and about seventeen minutes, which is US-5 + // ("at most one consent request") failing while VS-7 ("never two at once") + // passed. Only consent_pending is inherited, and only briefly: every other + // failure is per-attempt, and a verdict older than the TTL has probably + // been overtaken by the user finding the dialog and clicking Allow. + if err := recentConsentVerdict(sockPath); err != nil { + return nil, err + } + + _ = os.Remove(sockPath) // clear a stale socket file + _ = os.Remove(sockPath + errSuffix) // and a stale error, so we only read THIS spawn's + _ = os.Remove(sockPath + pendingSuffix) // ditto a stale consent marker + + proc, err := spawnDaemon(exePath, sockPath, env) + if err != nil { + return nil, err + } + + // The deadline MOVES. A daemon that is merely slow gets ten seconds; one that + // says it is holding a consent prompt gets the whole consent budget, because + // the thing it is waiting for is a human. The grace lets the daemon hit its + // own timeout first, so the error the user sees is the specific one it wrote + // rather than a generic "daemon did not start". + deadline := time.Now().Add(startupWait) + waiting := false + for { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(100 * time.Millisecond): + } + if c := TryConnect(sockPath); c != nil { + return c, nil + } + // The daemon writes its connect error here before exiting; decode it so + // the specific code (e.g. consent_pending) survives the process boundary. + if data, e := os.ReadFile(sockPath + errSuffix); e == nil && len(data) > 0 { + return nil, decodeConnectErr(data) + } + // A child that is gone with nothing written is over, whatever the + // deadline says. Nothing else can answer this: the sidecars are the + // daemon's own reports, and a daemon killed outright (or panicking + // inside chrome.Connect, which RunDaemon does not recover) files none. + // Before this, such a daemon left the pending marker standing and the + // caller waited the whole ~130s for a process that no longer existed. + if proc.gone() { + return nil, &chrome.ConnectError{Code: result.CodeDaemon, + Message: "the daemon exited without reporting why (it was killed, or it crashed while connecting) — retry, and if it repeats run with --no-daemon to see the connect error directly"} + } + if !waiting { + if _, e := os.Stat(sockPath + pendingSuffix); e == nil { + waiting = true + deadline = time.Now().Add(consentTimeout + startupWait) + notice(consentWaitNotice) + } + } + if time.Now().After(deadline) { + break + } + } + if waiting { + return nil, &chrome.ConnectError{Code: result.CodeConsentPending, + Message: "the daemon is still waiting for consent after " + consentTimeout.String() + ". " + browser.ConsentPromptAdvice} + } + return nil, &chrome.ConnectError{Code: result.CodeDaemon, + Message: "daemon did not start within " + startupWait.String() + ". " + browser.ConsentPromptAdvice} +} + +// daemonProc is Ensure's handle on the daemon it spawned. All it carries is +// "has it exited", which is the one question the sidecar files cannot answer: a +// daemon SIGKILLed, or panicking inside chrome.Connect (RunDaemon has no +// recover), leaves a .pending marker and no .err, and without this the wait had +// no reason to stop before the whole consent budget had elapsed. +// +// The signal comes from Wait rather than from kill(pid, 0), because the daemon +// is our child until it is reparented: a dead one is a zombie, and a zombie +// answers a liveness signal perfectly well. Waiting reaps it AND tells us. +type daemonProc struct{ exited chan struct{} } + +// gone reports whether the daemon process has already exited. +func (p *daemonProc) gone() bool { + if p == nil { + return false + } + select { + case <-p.exited: + return true + default: + return false + } +} + +// spawnDaemon starts the detached daemon process. It is a variable so a test can +// substitute a spawn it can count, without a real Chrome or a real binary. +var spawnDaemon = func(exePath, sockPath string, env []string) (*daemonProc, error) { cmd := exec.Command(exePath, "__daemon", sockPath) cmd.Env = env cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} // detach into its own session if err := cmd.Start(); err != nil { return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot start daemon: " + err.Error()} } - _ = cmd.Process.Release() + p := &daemonProc{exited: make(chan struct{})} + // Reaps the child if it dies while we are still here, and leaves the + // daemon entirely alone if it does not: this process exits within seconds + // either way, and the daemon (setsid) is reparented and carries on. + go func() { + _ = cmd.Wait() + close(p.exited) + }() + return p, nil +} - for range 100 { // up to ~10s for the first Allow-dialog click - time.Sleep(100 * time.Millisecond) - if c := TryConnect(sockPath); c != nil { - return c, nil +// lockSpawn takes an exclusive advisory lock covering the spawn-and-wait for one +// socket path, and returns the release. The lock file is never removed: unlinking +// it would let a later caller lock a different inode and defeat the exclusion. +// +// The wait is deliberately unbounded. The holder may be waiting out a consent +// prompt the user has not clicked yet, and blocking behind it is the correct +// outcome — spawning our own would add another prompt to the pile, which is the +// failure this exists to prevent. +// +// It is not, however, silent. The non-blocking attempt comes first purely so +// that contention can be NAMED: a second command run during a pending prompt +// used to hang for over two minutes with no output at all, which is the same +// "my tool has frozen and I do not know why" that US-2 exists to end — arrived +// at by the fix for US-2. +func lockSpawn(ctx context.Context, sockPath string) (func(), error) { + f, err := os.OpenFile(sockPath+lockSuffix, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot open the daemon spawn lock: " + err.Error()} + } + unlock := func() { + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + _ = f.Close() + } + fail := func(err error) (func(), error) { + _ = f.Close() + return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot take the daemon spawn lock: " + err.Error()} + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err == nil { + return unlock, nil + } else if !errors.Is(err, syscall.EWOULDBLOCK) { + return fail(err) + } + notice(lockWaitNotice) + + // flock has no deadline, so the blocking wait runs on its own goroutine and + // the context is honoured here. If the context wins, the goroutine may + // still acquire the lock afterwards — so it is handed the release to run + // itself, rather than being abandoned holding it. + // done is UNBUFFERED on purpose: the send succeeds only while this function + // is still selecting on it, so "the caller has gone" and "the caller took + // the lock" cannot both happen. + done := make(chan error) + abandoned := make(chan struct{}) + go func() { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX) + select { + case done <- err: + case <-abandoned: // nobody is waiting any more: give the lock straight back + if err == nil { + unlock() + } else { + _ = f.Close() + } } - // The daemon writes its connect error here before exiting; decode it so - // the specific code (e.g. not_debug_enabled) survives the process boundary. - if data, e := os.ReadFile(sockPath + ".err"); e == nil && len(data) > 0 { - return nil, decodeConnectErr(data) + }() + select { + case err := <-done: + if err != nil { + return fail(err) } + return unlock, nil + case <-ctx.Done(): + close(abandoned) + return nil, ctx.Err() } - return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "daemon did not start within 10s — did you click Allow in Chrome?"} +} + +// consentVerdictTTL is how long a consent_pending verdict left by the previous +// holder is inherited instead of re-derived. Queued callers are released within +// milliseconds of the verdict being written, so this only has to be long enough +// to drain a queue — and short, because the moment the user finds the dialog +// and clicks Allow the verdict is wrong, and a caller told to go looking for a +// prompt they have already answered is worse off than one that simply retried. +const consentVerdictTTL = 5 * time.Second + +// recentConsentVerdict returns the previous holder's consent_pending failure +// when it is recent enough to still be true, and nil otherwise. +func recentConsentVerdict(sockPath string) error { + path := sockPath + errSuffix + st, err := os.Stat(path) + if err != nil || time.Since(st.ModTime()) > consentVerdictTTL { + return nil + } + data, err := os.ReadFile(path) + if err != nil || len(data) == 0 { + return nil + } + var ce *chrome.ConnectError + if decoded := decodeConnectErr(data); errors.As(decoded, &ce) && ce.Code == result.CodeConsentPending { + return decoded + } + return nil +} + +// connectBrowser is chrome.Connect behind a seam, so RunDaemon's behaviour +// AFTER a successful connect is testable without a browser. +var connectBrowser = func(ctx context.Context, opts chrome.Options) (chrome.Browser, error) { + return chrome.Connect(ctx, opts) } // RunDaemon connects Chrome and serves sockPath until idle or stopped. Used by // the hidden `__daemon` invocation. func RunDaemon(sockPath string, opts chrome.Options, idle time.Duration) error { - b, err := chrome.Connect(context.Background(), opts) + // The daemon is detached: nothing it writes to stderr will ever be read. So + // "I am waiting on the consent prompt" is published as a file next to the + // socket, which is the only channel Ensure has into a connect that has not + // finished. Written BEFORE the wait, not after — the point is to tell the + // user while the dialog is still on screen. + pending := sockPath + pendingSuffix + _ = os.Remove(pending) + // Only the file's EXISTENCE is a signal; Ensure never reads it. The text is + // for whoever ends up cat-ing a stray sidecar out of the runtime dir and + // wondering what left it there. + opts.OnConsentPending = func() { + _ = os.WriteFile(pending, []byte("waiting for Chrome's remote-debugging consent prompt\n"), 0o600) + } + b, err := connectBrowser(context.Background(), opts) + _ = os.Remove(pending) if err != nil { // Leave the reason (with its code) for Ensure to surface, then exit. - _ = os.WriteFile(sockPath+".err", encodeConnectErr(err), 0o600) + _ = os.WriteFile(sockPath+errSuffix, encodeConnectErr(err), 0o600) return err } - _ = os.Remove(sockPath + ".err") + _ = os.Remove(sockPath + errSuffix) defer b.Close() _ = os.Remove(sockPath) ln, err := net.Listen("unix", sockPath) if err != nil { - return err + // Everything after the connect used to report only to a stderr nobody + // reads: the daemon is detached. So a bind failure — which the darwin + // sun_path limit makes entirely reachable — left the pending marker as + // the last thing Ensure had seen, and the user was told about a consent + // prompt for a failure that had nothing to do with one. + berr := &chrome.ConnectError{Code: result.CodeDaemon, + Message: "the daemon connected to Chrome but could not bind its socket at " + sockPath + " (" + err.Error() + ")"} + _ = os.WriteFile(sockPath+errSuffix, encodeConnectErr(berr), 0o600) + return berr } defer os.Remove(sockPath) diff --git a/internal/daemon/lifecycle_failure_test.go b/internal/daemon/lifecycle_failure_test.go new file mode 100644 index 0000000..960540c --- /dev/null +++ b/internal/daemon/lifecycle_failure_test.go @@ -0,0 +1,155 @@ +package daemon + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" + "github.com/sanketsudake/chrome-cdp-cli/internal/chrometest" + "github.com/sanketsudake/chrome-cdp-cli/internal/result" +) + +// deadProc is a daemon handle whose process has already gone. +func deadProc() *daemonProc { + p := &daemonProc{exited: make(chan struct{})} + close(p.exited) + return p +} + +// TestEnsureNoticesADeadDaemon. Once the pending marker appears, the only exits +// from the wait were a bindable socket, an .err sidecar, or the deadline — so a +// daemon that published "I am waiting on the prompt" and was then SIGKILLed (or +// panicked inside chrome.Connect, which RunDaemon does not recover) left the +// caller sitting for the whole consent budget plus the startup grace, ~130s, +// waiting for a process that no longer existed. +// +// A child that is gone with no .err behind it is an immediate failure, and it +// is not a consent failure: saying "still waiting on the prompt" about a dead +// daemon sends the user hunting for a dialog that nothing is holding. +func TestEnsureNoticesADeadDaemon(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + shrinkStartupWait(t, 200*time.Millisecond) + captureNotices(t) + + restore := swapSpawn(func(_, sockPath string, _ []string) (*daemonProc, error) { + // Publish the marker, then die without writing an error — a SIGKILL, or + // a panic in the connect. + if err := os.WriteFile(sockPath+pendingSuffix, []byte("waiting\n"), 0o600); err != nil { + t.Errorf("write pending sidecar: %v", err) + } + return deadProc(), nil + }) + defer restore() + + start := time.Now() + _, err := Ensure(context.Background(), sock, "unused", nil, 60*time.Second) + elapsed := time.Since(start) + + if elapsed > 3*time.Second { + t.Errorf("waited %v for a daemon that had already exited — the consent budget is for a live daemon holding a prompt", elapsed) + } + var ce *chrome.ConnectError + if !errors.As(err, &ce) { + t.Fatalf("error %v is not a *ConnectError", err) + } + if ce.Code != result.CodeDaemon { + t.Errorf("error.code = %q, want %q — a dead daemon is a daemon failure, not a pending prompt", ce.Code, result.CodeDaemon) + } +} + +// TestLockSpawnSaysItIsWaiting. The lock wait is unbounded on purpose (the +// holder may be waiting out a prompt nobody has clicked, and spawning our own +// would add to the pile), but it was also silent: a second invocation during a +// pending prompt printed nothing for 130 seconds. US-2 says tell the user what +// is happening, and this is the path that most needs it. +func TestLockSpawnSaysItIsWaiting(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + notices := captureNotices(t) + + // Hold the lock the way another chrome-cdp process would. flock is per + // open-file-description, so a second open in this process still blocks. + held, err := os.OpenFile(sock+lockSuffix, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatalf("open lock: %v", err) + } + defer held.Close() + if err := syscall.Flock(int(held.Fd()), syscall.LOCK_EX); err != nil { + t.Fatalf("flock: %v", err) + } + + got := make(chan func(), 1) + go func() { + unlock, err := lockSpawn(context.Background(), sock) + if err != nil { + t.Errorf("lockSpawn: %v", err) + return + } + got <- unlock + }() + + var said string + for deadline := time.Now().Add(2 * time.Second); time.Now().Before(deadline); { + if got := notices(); len(got) > 0 { + said = got[0] + break + } + time.Sleep(20 * time.Millisecond) + } + if said == "" { + t.Fatal("lockSpawn blocked in silence; a user running a second command during a pending prompt sees nothing at all") + } + if !strings.Contains(said, "waiting") { + t.Errorf("the contention notice does not say what it is waiting for:\n%s", said) + } + + _ = syscall.Flock(int(held.Fd()), syscall.LOCK_UN) + select { + case unlock := <-got: + unlock() + case <-time.After(3 * time.Second): + t.Fatal("lockSpawn never acquired the lock after it was released") + } +} + +// TestRunDaemonReportsAPostConnectFailure. Everything after chrome.Connect +// wrote no .err sidecar, so a daemon that connected and then failed to bind its +// socket exited with nothing but a stderr nobody reads — and Ensure, seeing the +// pending marker and no error, reported the CONSENT PROMPT for what was a bind +// failure. The RFC's own darwin sun_path note makes that reachable. +func TestRunDaemonReportsAPostConnectFailure(t *testing.T) { + prev := connectBrowser + connectBrowser = func(context.Context, chrome.Options) (chrome.Browser, error) { + return chrometest.StubBrowser{}, nil + } + t.Cleanup(func() { connectBrowser = prev }) + + // A socket path already occupied by a non-empty directory: the connect + // succeeds, the unlink and then the bind cannot. + sock := filepath.Join(shortTempDir(t), "d.sock") + if err := os.MkdirAll(filepath.Join(sock, "occupied"), 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + err := RunDaemon(sock, chrome.Options{}, time.Minute) + if err == nil { + t.Fatal("RunDaemon returned nil for a socket it could not bind") + } + + data, rerr := os.ReadFile(sock + errSuffix) + if rerr != nil { + t.Fatalf("the daemon left no error sidecar for a bind failure, so Ensure can only report the consent prompt: %v", rerr) + } + var ce *chrome.ConnectError + decoded := decodeConnectErr(data) + if !errors.As(decoded, &ce) || ce.Code != result.CodeDaemon { + t.Errorf("the sidecar decodes to %v, want a daemon_error ConnectError", decoded) + } + if strings.Contains(decoded.Error(), "Allow remote debugging") { + t.Errorf("a bind failure is reported as a consent prompt:\n%s", decoded) + } +} diff --git a/internal/daemon/queue_test.go b/internal/daemon/queue_test.go new file mode 100644 index 0000000..b1db932 --- /dev/null +++ b/internal/daemon/queue_test.go @@ -0,0 +1,124 @@ +package daemon + +import ( + "context" + "errors" + "os" + "path/filepath" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/chrome" + "github.com/sanketsudake/chrome-cdp-cli/internal/result" +) + +// RFC-0013 US-5 is "at most one consent request", and #17 only made that true +// for the FIRST attach. Behind the spawn lock, every queued caller in turn +// cleared the previous verdict, spawned its own daemon and raised its own +// prompt: eight concurrent commands against an unanswered dialog came to about +// seventeen minutes and eight sequential prompts. VS-7 held (never two at once) +// and US-5 did not. + +// writeConsentVerdict leaves the sidecar a timed-out daemon leaves behind. +func writeConsentVerdict(t *testing.T, sock string, age time.Duration) { + t.Helper() + path := sock + errSuffix + payload := encodeConnectErr(&chrome.ConnectError{ + Code: result.CodeConsentPending, Message: "the daemon is still waiting on Chrome's \"Allow remote debugging?\" prompt", + }) + if err := os.WriteFile(path, payload, 0o600); err != nil { + t.Fatalf("write verdict: %v", err) + } + when := time.Now().Add(-age) + if err := os.Chtimes(path, when, when); err != nil { + t.Fatalf("chtimes: %v", err) + } +} + +func TestEnsureInheritsARecentConsentVerdict(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + shrinkStartupWait(t, 200*time.Millisecond) + captureNotices(t) + writeConsentVerdict(t, sock, 0) + + var spawns atomic.Int32 + restore := swapSpawn(func(string, string, []string) (*daemonProc, error) { + spawns.Add(1) + return liveProc(t), nil + }) + defer restore() + + start := time.Now() + _, err := Ensure(context.Background(), sock, "unused", nil, 60*time.Second) + + var ce *chrome.ConnectError + if !errors.As(err, &ce) || ce.Code != result.CodeConsentPending { + t.Fatalf("error = %v, want a consent_pending ConnectError inherited from the holder", err) + } + if got := spawns.Load(); got != 0 { + t.Errorf("spawned %d daemons while a fresh consent verdict was on disk — each spawn attaches and raises its own prompt", got) + } + if el := time.Since(start); el > 2*time.Second { + t.Errorf("took %v to inherit a verdict already written down", el) + } +} + +func TestEnsureIgnoresAStaleConsentVerdict(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + shrinkStartupWait(t, 200*time.Millisecond) + captureNotices(t) + // Old enough that the user has plausibly found the dialog and clicked + // Allow: a verdict outlives its usefulness quickly, and refusing to retry + // would strand them behind an answer they have already given. + writeConsentVerdict(t, sock, time.Hour) + + var spawns atomic.Int32 + restore := swapSpawn(func(string, string, []string) (*daemonProc, error) { + spawns.Add(1) + return liveProc(t), nil + }) + defer restore() + + if _, err := Ensure(context.Background(), sock, "unused", nil, time.Second); err == nil { + t.Fatal("Ensure succeeded against a daemon that never bound") + } + if got := spawns.Load(); got != 1 { + t.Errorf("spawned %d daemons, want 1 — a stale verdict must not become permanent", got) + } +} + +// TestEnsureLockWaitHonoursTheContext. Ensure took no ctx at all, so a caller +// queued behind a holder that was waiting out a prompt blocked for the holder's +// whole budget no matter what --timeout said. +func TestEnsureLockWaitHonoursTheContext(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + captureNotices(t) + + held, err := os.OpenFile(sock+lockSuffix, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatalf("open lock: %v", err) + } + defer held.Close() + if err := syscall.Flock(int(held.Fd()), syscall.LOCK_EX); err != nil { + t.Fatalf("flock: %v", err) + } + defer syscall.Flock(int(held.Fd()), syscall.LOCK_UN) + + restore := swapSpawn(func(string, string, []string) (*daemonProc, error) { + t.Error("spawned a daemon while another process held the lock") + return liveProc(t), nil + }) + defer restore() + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + start := time.Now() + if _, err := Ensure(ctx, sock, "unused", nil, 60*time.Second); err == nil { + t.Fatal("Ensure returned no error after its context expired") + } + if el := time.Since(start); el > 5*time.Second { + t.Errorf("Ensure blocked on the spawn lock for %v after a 200ms deadline", el) + } +} diff --git a/internal/daemon/spawn_test.go b/internal/daemon/spawn_test.go new file mode 100644 index 0000000..e02f5a1 --- /dev/null +++ b/internal/daemon/spawn_test.go @@ -0,0 +1,133 @@ +package daemon + +import ( + "context" + "net" + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" + "time" +) + +// TestEnsureSpawnsOneDaemonUnderConcurrency: N callers, one daemon, one consent +// prompt. Why that matters, and why the unlinks have to be inside the lock too, +// is documented on Ensure. +func TestEnsureSpawnsOneDaemonUnderConcurrency(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + // Seven of the eight callers lose the lock race and say so; capture that + // rather than printing it eight times. + captureNotices(t) + + var spawns atomic.Int32 + restore := swapSpawn(func(_, sockPath string, _ []string) (*daemonProc, error) { + spawns.Add(1) + // Behave like the real daemon: bind the socket, a moment later, so the + // window between spawning and being connectable is real rather than + // instantaneous. Every caller must still converge on this one listener. + go func() { + time.Sleep(150 * time.Millisecond) + ln, err := net.Listen("unix", sockPath) + if err != nil { + t.Errorf("fake daemon could not bind %s: %v", sockPath, err) + return + } + t.Cleanup(func() { _ = ln.Close() }) + for { + c, err := ln.Accept() + if err != nil { + return + } + _ = c.Close() + } + }() + return liveProc(t), nil + }) + defer restore() + + const callers = 8 + var wg sync.WaitGroup + errs := make([]error, callers) + clients := make([]*Client, callers) + for i := range callers { + wg.Go(func() { + clients[i], errs[i] = Ensure(context.Background(), sock, "unused", nil, 2*time.Second) + }) + } + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Errorf("caller %d: Ensure failed: %v", i, err) + } else if clients[i] == nil { + t.Errorf("caller %d: got a nil client and no error", i) + } + } + if got := spawns.Load(); got != 1 { + t.Fatalf("%d daemons were spawned for %d concurrent callers, want exactly 1 — "+ + "each spawn attaches to Chrome and raises its own consent prompt", got, callers) + } +} + +// TestEnsureReusesARunningDaemon pins the fast path: an already-listening socket +// is connected to without taking the lock or spawning anything. +func TestEnsureReusesARunningDaemon(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + ln, err := net.Listen("unix", sock) + if err != nil { + t.Fatalf("listen: %v", err) + } + defer ln.Close() + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + _ = c.Close() + } + }() + + var spawns atomic.Int32 + restore := swapSpawn(func(string, string, []string) (*daemonProc, error) { + spawns.Add(1) + return liveProc(t), nil + }) + defer restore() + + if _, err := Ensure(context.Background(), sock, "unused", nil, 2*time.Second); err != nil { + t.Fatalf("Ensure against a live daemon: %v", err) + } + if got := spawns.Load(); got != 0 { + t.Errorf("spawned %d daemons while one was already running, want 0", got) + } +} + +// shortTempDir returns a temp dir with a SHORT path. A Unix socket address is +// capped near 104 bytes on darwin, and t.TempDir() embeds the test's name — long +// enough here to fail the bind with a bare "invalid argument". +func shortTempDir(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("", "cdpd") + if err != nil { + t.Fatalf("MkdirTemp: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + return dir +} + +func swapSpawn(fn func(exePath, sockPath string, env []string) (*daemonProc, error)) func() { + prev := spawnDaemon + spawnDaemon = fn + return func() { spawnDaemon = prev } +} + +// liveProc is a daemon handle whose process is still running: Ensure's liveness +// check must find nothing wrong, so the sidecars and the deadline decide. +func liveProc(t *testing.T) *daemonProc { + t.Helper() + p := &daemonProc{exited: make(chan struct{})} + t.Cleanup(func() { close(p.exited) }) + return p +} diff --git a/internal/daemon/status_test.go b/internal/daemon/status_test.go new file mode 100644 index 0000000..c35d889 --- /dev/null +++ b/internal/daemon/status_test.go @@ -0,0 +1,118 @@ +package daemon + +import ( + "context" + "encoding/json" + "errors" + "net" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/sanketsudake/chrome-cdp-cli/internal/chrometest" + "github.com/sanketsudake/chrome-cdp-cli/internal/target" +) + +// listBrowser is a Browser whose List answers however the test needs — the one +// call __status makes, and therefore the only evidence it has that the CDP +// connection is still alive. +type listBrowser struct { + chrometest.StubBrowser + tabs []target.Info + err error +} + +func (b listBrowser) List(context.Context) ([]target.Info, error) { return b.tabs, b.err } + +// TestStatusReportsAFailedListAsNotConnected is the daemon half of the doctor +// defect: __status used to hardcode "connected": true and treat a List failure +// as "omit targets". A daemon whose Chrome has quit holds a dead chromedp +// connection but keeps its listener for the whole idle window, so `running` is +// no evidence at all — the List it already performs is. +func TestStatusReportsAFailedListAsNotConnected(t *testing.T) { + t.Parallel() + + for _, c := range []struct { + name string + b listBrowser + wantConnected bool + wantCount int + }{ + {"list fails", listBrowser{err: errors.New("websocket: close 1006")}, false, 0}, + {"list works", listBrowser{tabs: []target.Info{{ID: "a"}, {ID: "b"}}}, true, 2}, + } { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + s := &server{b: c.b} + res, err := s.dispatch(t.Context(), "__status", nil) + if err != nil { + t.Fatalf("__status: %v", err) + } + info, ok := res.(map[string]any) + if !ok { + t.Fatalf("__status returned %T, want a map", res) + } + if info["connected"] != c.wantConnected { + t.Errorf("connected = %v, want %v — a status that always says true verifies nothing", info["connected"], c.wantConnected) + } + if got, _ := info["target_count"].(int); got != c.wantCount { + t.Errorf("target_count = %v, want %d", info["target_count"], c.wantCount) + } + }) + } +} + +// TestStatusPublishesNoTabTitlesOrURLs: the daemon's status payload is what +// `doctor --json` echoes, and SKILL.md makes doctor step 1 of every agent +// session. Open-tab URLs (OAuth callbacks, reset tokens, internal hostnames) +// must not ride along. +func TestStatusPublishesNoTabTitlesOrURLs(t *testing.T) { + t.Parallel() + s := &server{b: listBrowser{tabs: []target.Info{ + {ID: "1", Title: "Reset your password", URL: "https://intranet.example/reset?token=s3cret"}, + }}} + res, err := s.dispatch(t.Context(), "__status", nil) + if err != nil { + t.Fatalf("__status: %v", err) + } + payload, err := json.Marshal(res) + if err != nil { + t.Fatalf("marshal: %v", err) + } + for _, leak := range []string{"s3cret", "Reset your password", "intranet.example"} { + if strings.Contains(string(payload), leak) { + t.Errorf("__status leaked %q into its payload:\n%s", leak, payload) + } + } +} + +// TestStatusPropagatesADeadDaemonsError: Status must not report `running: true` +// with the status call's error discarded. A daemon holding a dead CDP +// connection answers the socket and fails the call, and swallowing that is one +// of the three unverified claims that let doctor say "ready". +func TestStatusPropagatesADeadDaemonsError(t *testing.T) { + t.Parallel() + sock := filepath.Join(shortTempDir(t), "d.sock") + ln, err := net.Listen("unix", sock) + if err != nil { + t.Fatalf("listen: %v", err) + } + defer ln.Close() + // Accept, then hang up without answering: exactly what a daemon whose + // dispatch cannot complete looks like from here. + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + time.Sleep(10 * time.Millisecond) + _ = c.Close() + } + }() + + if _, err := Status(sock, "127.0.0.1:9222"); err == nil { + t.Error("Status swallowed the failure and reported the daemon as usable") + } +} diff --git a/internal/probetest/probetest.go b/internal/probetest/probetest.go new file mode 100644 index 0000000..2855631 --- /dev/null +++ b/internal/probetest/probetest.go @@ -0,0 +1,244 @@ +// Package probetest builds the stub debug endpoints that RFC-0013's tests run +// against: a port that accepts and stalls, one that completes the WebSocket +// upgrade (now or later), and one with nothing listening at all. +// +// It exists because the consent-pending state is exactly a TCP listener that +// accepts and says nothing, so every scenario in that RFC is reproducible with +// net.Listen and no browser — which matters more here than usual: reproducing +// the bug by hand wedged a real user's Chrome twice, and a regression test that +// needs a human to click a browser-modal dialog is not a test. +// +// Three packages needed the same three listeners and each grew its own, with +// different signatures and separately maintained comments. Only two of them +// counted accepted connections, which is the assertion that proves the property +// the whole RFC turns on: each connection to the debug endpoint is a consent +// request, so "how many did we open" is the question. +// +// It cannot live in chrometest: that package imports internal/chrome, and +// internal/chrome is one of the packages that needs this. +package probetest + +import ( + "crypto/sha1" + "encoding/base64" + "fmt" + "net" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + "time" +) + +// Endpoint is a stub Chrome debug endpoint in one of the shapes the probe has +// to tell apart. +type Endpoint struct { + ln net.Listener + conns atomic.Int32 + answered atomic.Bool +} + +// Stall accepts connections and never answers — Chrome holding an unanswered +// consent prompt, which is the only state that hangs rather than failing. +func Stall(t *testing.T) *Endpoint { + t.Helper() + e := listen(t) + go e.accept(func(c net.Conn) { + <-t.Context().Done() // hold it open, saying nothing + _ = c.Close() + }) + return e +} + +// Answer accepts and, after delay, completes the WebSocket upgrade with status +// — the user finding the dialog behind the window and clicking Allow. A 101 +// status gets a full, VALID RFC 6455 response, with Sec-WebSocket-Accept +// computed from the key the client actually sent; the probe verifies it, so a +// stub that skipped it would be testing nothing. +// +// Pass a non-101 status for a live server that is not a CDP browser (a stale +// port file whose port another process has taken). +func Answer(t *testing.T, delay time.Duration, status string) *Endpoint { + t.Helper() + return answering(t, delay, func(req string) string { return handshakeReply(status, req) }) +} + +// handshakeReply is a server's response to an upgrade request: a full, valid +// RFC 6455 completion for a 101, or the bare status line for anything else. +func handshakeReply(status, req string) string { + if !strings.Contains(status, " 101 ") { + return status + "\r\n\r\n" + } + return status + "\r\n" + + "Upgrade: websocket\r\n" + + "Connection: Upgrade\r\n" + + "Sec-WebSocket-Accept: " + AcceptFor(RequestKey(req)) + "\r\n\r\n" +} + +// Chrome is an endpoint with the HTTP JSON API present, i.e. a Chrome launched +// with --remote-debugging-port rather than toggled on in chrome://inspect. It +// answers /json/version with a WebSocket URL on its OWN host:port — which is +// the only authority the resolver will believe — and treats every other +// request as the upgrade: stalling forever when status is empty, replying with +// status after delay otherwise. +// +// One listener, because a real Chrome is one listener. Splitting the JSON API +// and the WebSocket across two ports would be testing a shape that cannot +// occur, and the resolver rejects it on purpose. +func Chrome(t *testing.T, delay time.Duration, status string) *Endpoint { + t.Helper() + e := listen(t) + go e.accept(func(c net.Conn) { + buf := make([]byte, 2048) + n, _ := c.Read(buf) + req := string(buf[:n]) + if strings.Contains(req, "/json/version") { + body := fmt.Sprintf(`{"Browser":"Chrome/1","webSocketDebuggerUrl":%q}`, e.WS()) + fmt.Fprintf(c, "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: %d\r\nConnection: close\r\n\r\n%s", len(body), body) + _ = c.Close() + return + } + if status == "" { + <-t.Context().Done() // the upgrade: accepted, and then silence + _ = c.Close() + return + } + defer c.Close() + select { + case <-time.After(delay): + case <-t.Context().Done(): + return + } + if _, err := c.Write([]byte(handshakeReply(status, req))); err == nil { + e.answered.Store(true) + } + time.Sleep(50 * time.Millisecond) + }) + return e +} + +// AnswerRaw accepts and, after delay, writes exactly what reply returns for the +// request it received — for the handshakes that are meant to be wrong. +func AnswerRaw(t *testing.T, delay time.Duration, reply func(req string) string) *Endpoint { + t.Helper() + return answering(t, delay, reply) +} + +func answering(t *testing.T, delay time.Duration, reply func(req string) string) *Endpoint { + t.Helper() + e := listen(t) + go e.accept(func(c net.Conn) { + defer c.Close() + buf := make([]byte, 2048) + n, _ := c.Read(buf) + req := string(buf[:n]) + select { + case <-time.After(delay): + case <-t.Context().Done(): + return + } + if _, err := c.Write([]byte(reply(req))); err == nil { + // The write LANDED, so the socket was still open when the answer + // arrived. That is precisely what "the prompt was not orphaned" + // means in the failure this all exists to prevent. + e.answered.Store(true) + } + time.Sleep(50 * time.Millisecond) + }) + return e +} + +// RequestKey pulls the Sec-WebSocket-Key out of a handshake request. +func RequestKey(req string) string { + for line := range strings.SplitSeq(req, "\r\n") { + if name, value, ok := strings.Cut(line, ":"); ok && strings.EqualFold(strings.TrimSpace(name), "Sec-WebSocket-Key") { + return strings.TrimSpace(value) + } + } + return "" +} + +// AcceptFor is RFC 6455's Sec-WebSocket-Accept for a given key. +func AcceptFor(key string) string { + sum := sha1.Sum([]byte(key + "258EAFA5-E914-47DA-95CA-5AB0DC85B11A")) + return base64.StdEncoding.EncodeToString(sum[:]) +} + +// Closed returns an endpoint with nothing listening on it: a stale port file, +// or a Chrome that has quit. It must fail in milliseconds, which is the safety +// property that makes waiting minutes on a stalling one acceptable. +func Closed(t *testing.T) *Endpoint { + t.Helper() + e := listen(t) + _ = e.ln.Close() + return e +} + +func listen(t *testing.T) *Endpoint { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { _ = ln.Close() }) + return &Endpoint{ln: ln} +} + +func (e *Endpoint) accept(handle func(net.Conn)) { + for { + c, err := e.ln.Accept() + if err != nil { + return + } + e.conns.Add(1) + go handle(c) + } +} + +// Addr is the endpoint's address, listening or not. +func (e *Endpoint) Addr() net.Addr { return e.ln.Addr() } + +// HostPort is the endpoint's "127.0.0.1:port". +func (e *Endpoint) HostPort() string { return e.ln.Addr().String() } + +// Port is the endpoint's port number as a string. +func (e *Endpoint) Port() string { + _, port, _ := net.SplitHostPort(e.ln.Addr().String()) + return port +} + +// WS is the browser-level ws:// URL for this endpoint. +func (e *Endpoint) WS() string { + return fmt.Sprintf("ws://%s/devtools/browser/stub", e.HostPort()) +} + +// Conns is how many connections have been accepted. Every one of them is a +// consent request, so a test that asserts zero (or exactly one) is asserting +// the property the RFC exists to protect. +func (e *Endpoint) Conns() int32 { return e.conns.Load() } + +// AnsweredLive reports whether an Answer endpoint's reply landed on a socket +// that was still open — i.e. whether the consent the user granted went to a +// connection somebody had kept. +func (e *Endpoint) AnsweredLive() bool { return e.answered.Load() } + +// PortFile writes a DevToolsActivePort file pointing at this endpoint and +// returns its path. +func (e *Endpoint) PortFile(t *testing.T) string { + t.Helper() + p := filepath.Join(t.TempDir(), "DevToolsActivePort") + if err := os.WriteFile(p, []byte(e.Port()+"\n/devtools/browser/stub\n"), 0o600); err != nil { + t.Fatalf("write port file: %v", err) + } + return p +} + +// UsePortFile writes the port file and points CHROME_CDP_PORT_FILE at it, so a +// command discovers this endpoint the way it discovers a real Chrome. +func (e *Endpoint) UsePortFile(t *testing.T) string { + t.Helper() + p := e.PortFile(t) + t.Setenv("CHROME_CDP_PORT_FILE", p) + return p +} diff --git a/internal/result/result.go b/internal/result/result.go index 29e2d67..9ea24b4 100644 --- a/internal/result/result.go +++ b/internal/result/result.go @@ -20,10 +20,17 @@ const ( // Stable error.code strings emitted in the envelope's error object. Named so // call sites don't scatter bare string literals. const ( - CodeGeneric = "generic" - CodeUsage = "usage" - CodeConnection = "connection_failed" - CodeNotDebug = "not_debug_enabled" + CodeGeneric = "generic" + CodeUsage = "usage" + CodeConnection = "connection_failed" + CodeNotDebug = "not_debug_enabled" + // CodeConsentPending is Chrome holding its browser-modal "Allow remote + // debugging?" dialog: the port is open, the WebSocket upgrade is hanging, + // and nothing is wrong except that a human has not answered yet. It shares + // exit 3 with the other connection failures deliberately — a new number + // would break the documented contract — but it is a distinct code because + // the remedy is "click the dialog", not "check your setup". + CodeConsentPending = "consent_pending" CodeTargetTimeout = "target_timeout" CodeTargetNotFound = "target_not_found" CodeAmbiguous = "ambiguous_target" @@ -51,6 +58,7 @@ var codeToExit = map[string]int{ CodeUsage: ExitUsage, CodeConnection: ExitConnection, CodeNotDebug: ExitConnection, + CodeConsentPending: ExitConnection, CodeTargetTimeout: ExitTarget, CodeTargetNotFound: ExitTarget, CodeAmbiguous: ExitTarget, @@ -74,7 +82,7 @@ func ExitCodes() []ExitCodeDoc { {ExitOK, "success"}, {ExitGeneric, "generic / unclassified (also: an assertion tripped, e.g. console --fail-on-match)"}, {ExitUsage, "usage (bad flags/args)"}, - {ExitConnection, "connection (attach/launch failed)"}, + {ExitConnection, "connection (attach/launch failed, or Chrome's consent prompt is unanswered)"}, {ExitTarget, "target/timeout (selector not found, timeout, ambiguous/unknown target)"}, {ExitCDP, "cdp protocol error"}, {ExitDaemon, "daemon error"}, diff --git a/internal/result/result_test.go b/internal/result/result_test.go index a823093..017d77a 100644 --- a/internal/result/result_test.go +++ b/internal/result/result_test.go @@ -14,6 +14,9 @@ func TestExitCodeFor(t *testing.T) { {"usage", ExitUsage}, {"connection_failed", ExitConnection}, {"not_debug_enabled", ExitConnection}, + // A pending consent prompt is a distinct code on the EXISTING connection + // exit code: a caller branches on error.code, and the number is contract. + {"consent_pending", ExitConnection}, {"target_timeout", ExitTarget}, {"target_not_found", ExitTarget}, {"ambiguous_target", ExitTarget}, diff --git a/skills/drive-chrome-cdp/SKILL.md b/skills/drive-chrome-cdp/SKILL.md index 56c45ca..645157c 100644 --- a/skills/drive-chrome-cdp/SKILL.md +++ b/skills/drive-chrome-cdp/SKILL.md @@ -15,17 +15,29 @@ Because it drives the real profile, live logins are reused: **type no credential ## Setup (once) 1. Confirm the binary and connection: `chrome-cdp doctor --json`. - - `ok:true` → Path B attach ready; proceed. - - `ok:false` with `connection_failed` → tell the user to enable **`chrome://inspect/#remote-debugging`** (the one-time toggle), then re-run `doctor`. + It probes for real (or answers through a daemon that has just proved its connection) and reports `result.state` / `error.state`: + - `ready` → proceed. + - `consent_pending` → Chrome is holding an **"Allow remote debugging?"** dialog. + Tell the user it is **modal to the whole browser**, that it can sit **behind** the Chrome window, and that Chrome will accept no other input until they click Allow — a browser that looks frozen is this dialog, not a crash. + Then re-run `doctor`. + - `no_endpoint` → ask the user to relaunch Chrome with **`--remote-debugging-port=9222`** (`open -a "Google Chrome" --args --remote-debugging-port=9222` on macOS), which never prompts. + Only if they must attach to an already-running default-profile Chrome, have them enable **`chrome://inspect/#remote-debugging`** — that path prompts on every fresh attach. Do **not** work around consent. -2. A background daemon holds the connection, so Chrome's "Allow debugging?" prompt appears once per session, not per command. + - `unverified` → only from `--no-probe`; nothing was checked, so it is not an answer. + + `doctor` reports the endpoint it looked at and, on the daemon path, a `target_count`. + It does not report open tab titles or URLs, so this step tells you whether you can connect and nothing about what the user has open. + Use `tabs --json` when you actually need the tab list. +2. A background daemon holds the connection, so the consent prompt appears once per session, not per command. It starts on first use. `chrome-cdp daemon status --json` shows it; `--no-daemon` bypasses it. 3. **Avoid re-triggering the consent prompt.** - A fresh attach (the first command after `daemon stop`, or after a Chrome restart) re-shows Chrome's "Allow remote debugging?" prompt; if it isn't clicked it can wedge Chrome. + On the `chrome://inspect` path a fresh attach (the first command after `daemon stop`, or after a Chrome restart) re-shows the prompt. Keep the daemon alive — don't `daemon stop` mid-session. - If a command returns `connection_failed`, its message now says whether to click the Allow prompt (it can hide behind the window) or restart Chrome. - To skip the prompt entirely, have the user launch Chrome with `--remote-debugging-port=9222` (e.g. `open -a "Google Chrome" --args --remote-debugging-port=9222`). + The CLI now **waits** for the prompt rather than abandoning it: it holds the connection open for `--consent-timeout` (default 120s, clamped to 1s-10m) and connects the moment Allow is clicked, so a late answer still works. + If it runs out you get exit 3 with `error.code: consent_pending`; the recovery is to click Allow and retry, not to restart Chrome. + Do not retry in a loop while a prompt is pending: a retry started within a few seconds of a `consent_pending` inherits that answer rather than asking again, and one started later raises a second dialog at a browser that is already holding one. + Launching Chrome with `--remote-debugging-port=9222` skips the prompt entirely — prefer recommending that. ## The loop @@ -213,6 +225,7 @@ Every `--json` command emits one envelope: ``` Failures: same shape with `"ok": false` and `error{code,message,…}`, plus a nonzero exit code — `0` ok · `1` generic · `2` usage · `3` connection · `4` target/timeout · `5` cdp · `6` daemon · `7` permission_denied. +Exit `3` carries three codes; `consent_pending` is the one that needs a human, not a fix (see [Setup](#setup-once) step 1). Branch on these, not on message text (`chrome-cdp exit-codes` prints the table). Exit `7` means a policy forbids this — do not retry, tell the user (see the Policy section of the CLI reference).