From e38c9a3a7ff458f0bf583b174ff00942cfa75656 Mon Sep 17 00:00:00 2001 From: Maris Popens Date: Sat, 17 Oct 2026 16:59:41 +0300 Subject: [PATCH 1/2] fix(browser): make the automation CSP apply, and test the Chrome paths The document CSP that keeps popups, workers and stray connections out of an agent-driven page never ran: Chrome paused documents only at the request stage, and only applies the sandbox part of a CSP header edited with continueResponse. Documents are now paused at the response stage and fulfilled with the policy. Screenshots are named by their real format (JPEG was written to .png files). Adds Chrome fixtures for control errors, the CSP and unloadable pages, and tests for the IMAP expunge warnings. --- docs/code-patterns.md | 2 + internal/browser/automation.go | 75 +++++-- internal/browser/automation_extra_test.go | 216 +++++++++++++++++++++ internal/browser/browser.go | 7 +- internal/browser/screenshot_format_test.go | 37 ++++ internal/inbox/imap_server_test.go | 62 ++++++ 6 files changed, 384 insertions(+), 15 deletions(-) create mode 100644 internal/browser/automation_extra_test.go create mode 100644 internal/browser/screenshot_format_test.go diff --git a/docs/code-patterns.md b/docs/code-patterns.md index 8a0f9de..1c21ad6 100644 --- a/docs/code-patterns.md +++ b/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). diff --git a/internal/browser/automation.go b/internal/browser/automation.go index b2f2adb..bcd86aa 100644 --- a/internal/browser/automation.go +++ b/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 { diff --git a/internal/browser/automation_extra_test.go b/internal/browser/automation_extra_test.go new file mode 100644 index 0000000..c0c9ad7 --- /dev/null +++ b/internal/browser/automation_extra_test.go @@ -0,0 +1,216 @@ +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 = `Controls +
+ + + + + +
` + +// 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, `starting`) + }) + 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) + } +} diff --git a/internal/browser/browser.go b/internal/browser/browser.go index 8dc4dbf..efc7746 100644 --- a/internal/browser/browser.go +++ b/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 { diff --git a/internal/browser/screenshot_format_test.go b/internal/browser/screenshot_format_test.go new file mode 100644 index 0000000..d1ccd6e --- /dev/null +++ b/internal/browser/screenshot_format_test.go @@ -0,0 +1,37 @@ +package browser + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +// The file name must say what is inside: Chrome returns JPEG below quality 100 +// and PNG at 100, and screenshots used to be JPEG data named .png. +func TestScreenshotFilesAreNamedByTheirFormat(t *testing.T) { + b := requireChrome(t) + b.config.ScreenshotDir = t.TempDir() + + for _, tc := range []struct { + quality int + ext string + magic []byte + }{ + {90, ".jpg", []byte{0xFF, 0xD8, 0xFF}}, + {100, ".png", []byte("\x89PNG")}, + } { + name, err := b.takeScreenshotQuality(b.ctx, "acme", "shot", tc.quality) + if err != nil { + t.Fatal(err) + } + if !strings.HasSuffix(name, tc.ext) { + t.Errorf("quality %d: file %q, want a %s extension", tc.quality, name, tc.ext) + } + data, err := os.ReadFile(filepath.Join(b.config.ScreenshotDir, name)) + if err != nil || !bytes.HasPrefix(data, tc.magic) { + t.Errorf("quality %d: %q does not start with the %s signature (%v)", tc.quality, name, tc.ext, err) + } + } +} diff --git a/internal/inbox/imap_server_test.go b/internal/inbox/imap_server_test.go index 016866c..53803e4 100644 --- a/internal/inbox/imap_server_test.go +++ b/internal/inbox/imap_server_test.go @@ -1,17 +1,20 @@ package inbox import ( + "bytes" "context" "fmt" "io" "log" "net" + "os" "path/filepath" "strconv" "strings" "testing" "time" + "github.com/emersion/go-imap" "github.com/emersion/go-imap/backend/memory" "github.com/emersion/go-imap/server" @@ -221,3 +224,62 @@ func TestArchiveEmailsNothingToDo(t *testing.T) { t.Error("archiving into a missing folder should fail") } } + +// Without UID EXPUNGE (go-imap v1.2.1), archiving by COPY then EXPUNGE removes +// every \Deleted message in the mailbox, not just the ones being archived. +// Mail another client already flagged would go too, so the code warns before +// and after instead of doing it silently. +func TestArchiveEmailsWarnsWhenOtherMailIsAlreadyFlaggedDeleted(t *testing.T) { + f := newIMAPFixture(t, false) // no MOVE: COPY + \Deleted + EXPUNGE + f.Deliver(t, "INBOX", "privacy@acme.example", "Re: first", "first") + f.Deliver(t, "INBOX", "privacy@acme.example", "Re: second", "second") + f.Mailbox(t, "Eraser") // the archive folder must exist for COPY + + inbox := f.Mailbox(t, "INBOX") + n := len(inbox.Messages) + stray, ours := inbox.Messages[n-2], inbox.Messages[n-1] + stray.Flags = append(stray.Flags, imap.DeletedFlag) + before := f.Count(t, "INBOX") + + var logs bytes.Buffer + log.SetOutput(&logs) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + + m := f.connect(t) + if err := m.ArchiveEmails([]uint32{ours.Uid}, "Eraser"); err != nil { + t.Fatal(err) + } + out := logs.String() + for _, want := range []string{ + "1 message(s) besides the 1 just archived are already flagged \\Deleted", + "expunge removed 2 message(s) but this operation only intended to remove 1", + } { + if !strings.Contains(out, want) { + t.Errorf("log is missing %q:\n%s", want, out) + } + } + // This is the behaviour the warnings exist for: the stray message is gone too. + if got := f.Count(t, "INBOX"); got != before-2 { + t.Errorf("INBOX has %d messages, want %d", got, before-2) + } +} + +// With nothing else flagged, archiving by COPY and EXPUNGE stays quiet. +func TestArchiveEmailsWithoutStrayFlagsDoesNotWarn(t *testing.T) { + f := newIMAPFixture(t, false) + f.Deliver(t, "INBOX", "privacy@acme.example", "Re: only", "only") + f.Mailbox(t, "Eraser") + inbox := f.Mailbox(t, "INBOX") + ours := inbox.Messages[len(inbox.Messages)-1] + + var logs bytes.Buffer + log.SetOutput(&logs) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + + if err := f.connect(t).ArchiveEmails([]uint32{ours.Uid}, "Eraser"); err != nil { + t.Fatal(err) + } + if strings.Contains(logs.String(), "WARNING") { + t.Errorf("unexpected warning:\n%s", logs.String()) + } +} From 06cc780480cf4777a989343af7035906a9956f27 Mon Sep 17 00:00:00 2001 From: Maris Popens Date: Sat, 17 Oct 2026 17:05:56 +0300 Subject: [PATCH 2/2] test(browser): cover redirects and empty pages under the injected policy --- internal/browser/automation_extra_test.go | 33 +++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/browser/automation_extra_test.go b/internal/browser/automation_extra_test.go index c0c9ad7..187384c 100644 --- a/internal/browser/automation_extra_test.go +++ b/internal/browser/automation_extra_test.go @@ -214,3 +214,36 @@ func TestInjectedPolicyStringKeepsPopupsAndWorkersOut(t *testing.T) { 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, `landed`) + }) + 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) + } +}