fix(browser): make the automation CSP apply, and test the Chrome paths

docs/code-patterns.md

@@ -15,6 +15,8 @@
 
 Worth knowing before touching related code:
 
+- The automation session's document CSP (`internal/browser/automation.go`, `restrictDocument`) only works because of two Chrome details. A document is paused again at the response stage only if its `ContinueRequest` sets `InterceptResponse`. And Chrome applies just the `sandbox` part of a CSP header edited with `Fetch.continueResponse`: `worker-src` and `connect-src` are ignored, so the response is fulfilled with its own body and the new header instead. Before this was found the policy never ran at all. `TestInjectedCSPApplies` runs a real page against it.
+- Screenshots are named by their actual format: Chrome returns JPEG below quality 100 and PNG at 100, and they used to be JPEG data in `.png` files.
 - `history.Store.UpdatePipelineStatus` only moves a request forward (ranks in `internal/history/pipeline_rank.go`): an earlier or unknown stage is silently ignored, so a late "awaiting response" can't undo a confirmation. A person's explicit decision in the UI goes through `OverridePipelineStatus`, which always applies. Both record who made the change in `pipeline_actor`.
 - `batch_approvals` timestamps are stored and compared in UTC. SQLite compares them as text, so a local UTC offset (DST) would break the ordering. `ConsumeBatchApproval` is the only way to spend an approval and it is atomic: one approval, one batch.
 - `no_data_held` is a value in the export JSON (`response_type`, `pipeline_status`), so it can only be added to, never renamed. `MigrateNoData` only relabels saved replies; resolving a request is `ResolveNoData`'s job (and the reclassify action's).

internal/browser/automation.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/rand"
 	"crypto/sha256"
+	"encoding/base64"
 	"encoding/json"
 	"fmt"
 	"net"
@@ -216,12 +217,7 @@ func (s *AutomationSession) Open(caller context.Context, raw string) (*Snapshot,
 		case *fetch.EventRequestPaused:
 			go s.checkRequest(e)
 		case *network.EventResponseReceived:
-			// Our DNS lookup and Chrome's can differ, so check the address Chrome used.
-			if e.Response != nil && s.policy.LocalOrigin == "" && !publicAddress(e.Response.RemoteIPAddress) {
-				s.mu.Lock()
-				s.networkBlocked = "The page connected to a private network address, so it was blocked. Review the broker's URLs."
-				s.mu.Unlock()
-			}
+			s.noteResponse(e)
 		}
 	})
 	ctx, cancel := s.operation(caller)
@@ -233,18 +229,63 @@ func (s *AutomationSession) Open(caller context.Context, raw string) (*Snapshot,
 	return s.Observe(caller)
 }
 
+// csp is added to every document the session loads. It keeps popups, workers
+// and out-of-scope connections from escaping the tab's request interceptor;
+// policies stack, so the site's own stays in force.
+func (p URLPolicy) csp() string {
+	return "sandbox allow-scripts allow-same-origin allow-forms allow-modals; worker-src 'none'; connect-src " + strings.Join(p.connectSources(), " ")
+}
+
+// restrictDocument adds the CSP to a document response. Chrome only enforces the
+// sandbox part of a header changed through ContinueResponse and ignores
+// worker-src and connect-src from it, so the response is fulfilled with its own
+// body and the new header instead, which makes the whole policy count.
+func (s *AutomationSession) restrictDocument(ctx context.Context, e *fetch.EventRequestPaused) {
+	headers := append(e.ResponseHeaders, &fetch.HeaderEntry{Name: "Content-Security-Policy", Value: s.policy.csp()})
+	var err error
+	if e.ResponseStatusCode >= 300 && e.ResponseStatusCode < 400 {
+		// A redirect has no page; the next hop comes back through the interceptor.
+		if err := fetch.ContinueResponse(e.RequestID).Do(ctx); err != nil {
+			_ = fetch.FailRequest(e.RequestID, network.ErrorReasonBlockedByClient).Do(ctx)
+		}
+		return
+	}
+	body, bodyErr := fetch.GetResponseBody(e.RequestID).Do(ctx)
+	switch {
+	case bodyErr != nil:
+		// Nothing to read (an empty response): the sandbox still applies.
+		err = fetch.ContinueResponse(e.RequestID).WithResponseCode(e.ResponseStatusCode).WithResponseHeaders(headers).Do(ctx)
+	default:
+		// GetResponseBody returns the decoded body, so drop the encoding headers.
+		kept := make([]*fetch.HeaderEntry, 0, len(headers))
+		for _, h := range headers {
+			if n := strings.ToLower(h.Name); n != "content-encoding" && n != "content-length" {
+				kept = append(kept, h)
+			}
+		}
+		err = fetch.FulfillRequest(e.RequestID, e.ResponseStatusCode).WithResponseHeaders(kept).WithBody(base64.StdEncoding.EncodeToString(body)).Do(ctx)
+	}
+	if err != nil {
+		_ = fetch.FailRequest(e.RequestID, network.ErrorReasonBlockedByClient).Do(ctx)
+	}
+}
+
+// noteResponse stops the session when Chrome connected to a non-public address.
+// Our DNS lookup and Chrome's can differ, so check the address Chrome used.
+func (s *AutomationSession) noteResponse(e *network.EventResponseReceived) {
+	if e.Response != nil && s.policy.LocalOrigin == "" && !publicAddress(e.Response.RemoteIPAddress) {
+		s.mu.Lock()
+		s.networkBlocked = "The page connected to a private network address, so it was blocked. Review the broker's URLs."
+		s.mu.Unlock()
+	}
+}
+
 func (s *AutomationSession) checkRequest(e *fetch.EventRequestPaused) {
 	ctx, cancel := context.WithTimeout(s.b.ctx, 10*time.Second)
 	defer cancel()
 	execCtx := cdp.WithExecutor(ctx, chromedp.FromContext(ctx).Target)
 	if e.ResponseStatusCode != 0 {
-		// popups, workers and stray WebSockets could escape the request interceptor, so add
-		// a CSP; policies stack, so the site's own stays in force
-		csp := "sandbox allow-scripts allow-same-origin allow-forms allow-modals; worker-src 'none'; connect-src " + strings.Join(s.policy.connectSources(), " ")
-		headers := append(e.ResponseHeaders, &fetch.HeaderEntry{Name: "Content-Security-Policy", Value: csp})
-		if err := fetch.ContinueResponse(e.RequestID).WithResponseCode(e.ResponseStatusCode).WithResponseHeaders(headers).Do(execCtx); err != nil {
-			_ = fetch.FailRequest(e.RequestID, network.ErrorReasonBlockedByClient).Do(execCtx)
-		}
+		s.restrictDocument(execCtx, e)
 		return
 	}
 	// every request: redirects, frames, XHR, form actions. third-party portals must be in the catalog
@@ -259,7 +300,13 @@ func (s *AutomationSession) checkRequest(e *fetch.EventRequestPaused) {
 		_ = fetch.FailRequest(e.RequestID, network.ErrorReasonBlockedByClient).Do(execCtx)
 		return
 	}
-	_ = fetch.ContinueRequest(e.RequestID).Do(execCtx)
+	// Chrome only pauses a document again at the response stage, where the CSP
+	// is added, when the request asks for it.
+	cont := fetch.ContinueRequest(e.RequestID)
+	if e.ResourceType == network.ResourceTypeDocument {
+		cont = cont.WithInterceptResponse(true)
+	}
+	_ = cont.Do(execCtx)
 }
 
 func (s *AutomationSession) evaluate(ctx context.Context, expression string, out any) error {

internal/browser/automation_extra_test.go

@@ -0,0 +1,249 @@
+package browser
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/chromedp/cdproto/network"
+)
+
+// --- no Chrome needed ---------------------------------------------------
+
+func TestNewAutomationSessionNeedsApprovedDomains(t *testing.T) {
+	if _, err := NewAutomationSession(DefaultConfig(), testProfile(), URLPolicy{}); err == nil || !strings.Contains(err.Error(), "no approved domains") {
+		t.Fatalf("a broker with no domains: %v", err)
+	}
+	// Chrome starts lazily, so building and closing a session needs no browser.
+	s, err := NewAutomationSession(DefaultConfig(), testProfile(), NewURLPolicy("https://broker.example", ""))
+	if err != nil {
+		t.Fatal(err)
+	}
+	s.Close()
+}
+
+func TestSnapshotFingerprintIgnoresEphemeralFields(t *testing.T) {
+	a := Snapshot{ID: "one", Screenshot: "a.png", URL: "https://x.example/f", Text: "Remove me"}
+	b := a
+	b.ID, b.Screenshot = "two", "b.png"
+	if a.Fingerprint() != b.Fingerprint() {
+		t.Error("re-observing the same page changed its fingerprint")
+	}
+	b.Text = "Thank you"
+	if a.Fingerprint() == b.Fingerprint() {
+		t.Error("a changed page kept its fingerprint")
+	}
+}
+
+func TestPolicyEdgeCases(t *testing.T) {
+	if got := (URLPolicy{LocalOrigin: "http://127.0.0.1:9"}).connectSources(); got[len(got)-1] != "http://127.0.0.1:9" {
+		t.Errorf("connect sources = %v, want the local origin included", got)
+	}
+	err := NewURLPolicy("https://does-not-resolve.invalid", "").Validate(context.Background(), "https://does-not-resolve.invalid/form")
+	if err == nil || !strings.Contains(err.Error(), "cannot verify destination address") {
+		t.Errorf("unresolvable host: %v", err)
+	}
+}
+
+func TestOpenReportsChromeThatCannotStart(t *testing.T) {
+	cfg := DefaultConfig()
+	cfg.ExecutablePath = "/nonexistent/chrome"
+	s, err := NewAutomationSession(cfg, testProfile(), URLPolicy{LocalOrigin: "http://127.0.0.1:1"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+	if _, err := s.Open(context.Background(), "http://127.0.0.1:1/form"); err == nil || !strings.Contains(err.Error(), "start Chrome") {
+		t.Fatalf("Open with no browser: %v", err)
+	}
+}
+
+func TestNoteResponseStopsTheSessionOnPrivateAddresses(t *testing.T) {
+	response := func(ip string) *network.EventResponseReceived {
+		return &network.EventResponseReceived{Response: &network.Response{RemoteIPAddress: ip}}
+	}
+	for ip, blocked := range map[string]bool{"93.184.216.34": false, "": false, "10.0.0.5": true, "127.0.0.1": true, "169.254.169.254": true} {
+		s := &AutomationSession{policy: NewURLPolicy("https://broker.example", "")}
+		s.noteResponse(response(ip))
+		if got := s.blockedReason() != ""; got != blocked {
+			t.Errorf("response from %q: blocked = %v, want %v", ip, got, blocked)
+		}
+	}
+	s := &AutomationSession{policy: NewURLPolicy("https://broker.example", "")}
+	s.noteResponse(&network.EventResponseReceived{}) // no response attached
+	if s.blockedReason() != "" {
+		t.Error("an event with no response blocked the session")
+	}
+	// The demo's own loopback server is allowed.
+	s = &AutomationSession{policy: URLPolicy{LocalOrigin: "http://127.0.0.1:9"}}
+	s.noteResponse(response("127.0.0.1"))
+	if s.blockedReason() != "" {
+		t.Error("the demo's local server was blocked")
+	}
+}
+
+// --- real Chrome --------------------------------------------------------
+
+const controlsPage = `<!doctype html><title>Controls</title>
+<form onsubmit="return false">
+  <input name="nickname" aria-label="Nickname">
+  <input type="checkbox" name="agree" aria-label="Agree">
+  <select name="plan" aria-label="Plan"><option>Basic</option></select>
+  <button type="button" id="go">Go</button>
+  <button type="button" id="gone">Vanishing</button>
+</form>`
+
+// Each message comes from the page script or the Go wrapper in Act, and each is
+// what an agent sees when its plan no longer matches the page.
+func TestActReportsWhyAControlCannotBeUsed(t *testing.T) {
+	s, base := automationFixture(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "text/html")
+		_, _ = fmt.Fprint(w, controlsPage)
+	}))
+	ctx := context.Background()
+	if _, err := s.Open(ctx, base); err != nil {
+		t.Fatal(err)
+	}
+	pageCtx, cancel := s.operation(ctx) // evaluate needs the browser's own context
+	defer cancel()
+	observe := func() *Snapshot {
+		t.Helper()
+		snap, err := s.Observe(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return snap
+	}
+	mustFail := func(snap *Snapshot, label, action, value, want string) {
+		t.Helper()
+		_, err := s.Act(ctx, snap.ID, controlRef(t, snap, label), action, value)
+		if err == nil || !strings.Contains(err.Error(), want) {
+			t.Errorf("%s %s %q: error = %v, want it to mention %q", action, label, value, err, want)
+		}
+	}
+
+	snap := observe()
+	mustFail(snap, "Plan", "fill", "Enterprise", "Option not found")
+	snap = observe() // a failed action leaves the snapshot usable, but be explicit
+	mustFail(snap, "Agree", "fill", "maybe", "true or false")
+	snap = observe()
+	mustFail(snap, "Go", "fill", "text", "not a form field")
+	snap = observe()
+	mustFail(snap, "Go", "tap", "", "Unsupported action")
+
+	// A control removed after the snapshot, and one disabled after it.
+	snap = observe()
+	gone, goRef := controlRef(t, snap, "Vanishing"), controlRef(t, snap, "Go")
+	if err := s.evaluate(pageCtx, `document.getElementById('gone').remove(); document.getElementById('go').disabled = true; true`, nil); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := s.Act(ctx, snap.ID, gone, "click", ""); err == nil || !strings.Contains(err.Error(), "Control changed") {
+		t.Errorf("removed control: %v", err)
+	}
+	if _, err := s.Act(ctx, snap.ID, goRef, "click", ""); err == nil || !strings.Contains(err.Error(), "Control unavailable") {
+		t.Errorf("disabled control: %v", err)
+	}
+}
+
+// The document CSP is what keeps a page from opening popups or starting
+// workers that the request interceptor cannot see. Chrome ignores most of it
+// when the header is only edited in place, so this checks it really applies.
+func TestInjectedCSPApplies(t *testing.T) {
+	mux := http.NewServeMux()
+	mux.HandleFunc("/worker.js", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "text/javascript")
+		_, _ = fmt.Fprint(w, `postMessage('ran')`)
+	})
+	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "text/html")
+		_, _ = fmt.Fprint(w, `<!doctype html><title>starting</title><script>
+const log = [window.open('about:blank') ? 'popup-opened' : 'popup-blocked'];
+function done() { document.title = log.join(' '); }
+try {
+  const w = new Worker('/worker.js');
+  w.onmessage = () => { log.push('worker-ran'); done(); };
+  w.onerror = () => { log.push('worker-blocked'); done(); };
+} catch (e) { log.push('worker-blocked'); done(); }
+</script><button>Continue</button>`)
+	})
+	s, base := automationFixture(t, mux)
+	ctx := context.Background()
+	if _, err := s.Open(ctx, base); err != nil {
+		t.Fatal(err)
+	}
+	// The page finishes asynchronously, whichever way the worker goes.
+	var title string
+	for range 40 {
+		snap, err := s.Observe(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if title = snap.Title; title != "starting" {
+			break
+		}
+	}
+	if title != "popup-blocked worker-blocked" {
+		t.Fatalf("title = %q, want popup-blocked worker-blocked: the injected CSP did not apply", title)
+	}
+}
+
+func TestOpenReportsAPageThatCannotBeLoaded(t *testing.T) {
+	b := requireChrome(t)
+	b.config.ScreenshotDir = t.TempDir()
+	web := httptest.NewServer(http.NotFoundHandler())
+	url := web.URL
+	web.Close() // nothing is listening any more
+	s := &AutomationSession{b: b, policy: URLPolicy{LocalOrigin: url}}
+	t.Cleanup(s.Close)
+	if _, err := s.Open(context.Background(), url); err == nil || !strings.Contains(err.Error(), "open task page") {
+		t.Fatalf("Open of an unreachable page: %v", err)
+	}
+}
+
+func TestInjectedPolicyStringKeepsPopupsAndWorkersOut(t *testing.T) {
+	csp := NewURLPolicy("https://broker.example", "").csp()
+	for _, want := range []string{"sandbox ", "allow-forms", "worker-src 'none'", "connect-src 'self' https://broker.example https://www.broker.example"} {
+		if !strings.Contains(csp, want) {
+			t.Errorf("policy %q is missing %q", csp, want)
+		}
+	}
+	if strings.Contains(csp, "allow-popups") || strings.Contains(csp, "allow-top-navigation") {
+		t.Errorf("policy %q allows what the sandbox is there to stop", csp)
+	}
+}
+
+// A redirect has no page of its own: it must go through untouched, and the page
+// it lands on must still get the policy.
+func TestRedirectedDocumentStillGetsThePolicy(t *testing.T) {
+	mux := http.NewServeMux()
+	mux.HandleFunc("/start", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/landing", http.StatusFound)
+	})
+	mux.HandleFunc("/landing", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "text/html")
+		_, _ = fmt.Fprint(w, `<!doctype html><title>landed</title><script>
+document.title = window.open('about:blank') ? 'landed popup-opened' : 'landed popup-blocked';
+</script><button>Continue</button>`)
+	})
+	s, base := automationFixture(t, mux)
+	snap, err := s.Open(context.Background(), base+"/start")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !strings.HasSuffix(snap.URL, "/landing") || snap.Title != "landed popup-blocked" {
+		t.Fatalf("after the redirect: url=%q title=%q", snap.URL, snap.Title)
+	}
+}
+
+// A response with nothing to read still loads, with the sandbox applied.
+func TestEmptyDocumentStillLoads(t *testing.T) {
+	s, base := automationFixture(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "text/html")
+	}))
+	if _, err := s.Open(context.Background(), base); err != nil {
+		t.Fatalf("an empty page should still open: %v", err)
+	}
+}

internal/browser/browser.go

@@ -379,7 +379,12 @@ func (b *Browser) takeScreenshotQuality(ctx context.Context, brokerID, suffix st
 		return "", err
 	}
 
-	filename := fmt.Sprintf("%s_%s_%d.png", brokerID, suffix, time.Now().Unix())
+	// Chrome returns JPEG below quality 100, and these used to be named .png.
+	ext := "jpg"
+	if quality >= 100 {
+		ext = "png"
+	}
+	filename := fmt.Sprintf("%s_%s_%d.%s", brokerID, suffix, time.Now().Unix(), ext)
 	filepath := filepath.Join(b.config.ScreenshotDir, filename)
 
 	if err := os.WriteFile(filepath, buf, 0600); err != nil {