From 86f5d3ffdc564b09b83d039dda2c54a9845692f7 Mon Sep 17 00:00:00 2001 From: Felix Geelhaar Date: Sun, 5 Jul 2026 11:05:18 +0200 Subject: [PATCH] feat(security): re-validate redirects and scope auth headers via a shared Fetch interceptor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The URL policy was only checked once, before navigating, so a public URL that 302'd to an internal host (127.0.0.1, cloud metadata) was followed unchecked — redirect-SSRF (finding #4, HIGH). And auth middleware set Network.setExtraHTTPHeaders session-wide, leaking the Authorization header to every host including redirect targets (#9). Adds a single Page-level request interceptor (page_fetch.go) that owns CDP Fetch and multiplexes rules — first Block wins, header additions merge onto the continued request — so URL policy, resource blocking, and header injection share one Fetch session instead of fighting over Fetch.enable. Built on the off-read- loop dispatch from #53, so the continue/fail calls a handler makes don't deadlock. - Redirect guard: every Document request (initial nav + each redirect) is re-validated against the URL policy; installed by default when the validator blocks private IPs, skipped for trusted (AllowPrivateIPs) sessions. - Auth scoping: HeaderAuth now injects only for same-origin-as-navigation requests (tracked via Navigate), so the token isn't sent cross-origin or to a redirect destination. - BlockResources refactored onto the interceptor so it coexists with the guard. Tests: unit (origin/pattern/same-origin logic) + Chrome integration under -race proving a redirect is blocked at the Fetch layer, a no-op rule doesn't break navigation, and same-origin header injection works. Full integration suite green. Claude-Session: https://claude.ai/code/session_01QKTcmXFTKoTQr7mB3HCuHZ --- middleware/auth.go | 18 ++- middleware/network.go | 38 ++--- page.go | 20 +++ page_fetch.go | 270 +++++++++++++++++++++++++++++++++ page_fetch_integration_test.go | 153 +++++++++++++++++++ page_fetch_test.go | 77 ++++++++++ 6 files changed, 549 insertions(+), 27 deletions(-) create mode 100644 page_fetch.go create mode 100644 page_fetch_integration_test.go create mode 100644 page_fetch_test.go diff --git a/middleware/auth.go b/middleware/auth.go index 29c4da2..474b269 100644 --- a/middleware/auth.go +++ b/middleware/auth.go @@ -36,7 +36,11 @@ func BearerAuth(token string) browse.HandlerFunc { return HeaderAuth("Authorization", "Bearer "+token) } -// HeaderAuth returns middleware that sets a custom header on all requests. +// HeaderAuth returns middleware that injects a custom header, scoped to the +// navigation's own origin. Unlike a session-wide Network.setExtraHTTPHeaders +// (which sends the header to every host the page contacts), this adds the header +// only to same-origin requests — so an auth token isn't leaked to cross-origin +// subresources or a redirect destination. func HeaderAuth(name, value string) browse.HandlerFunc { return func(c *browse.Context) { page := c.Page() @@ -44,16 +48,20 @@ func HeaderAuth(name, value string) browse.HandlerFunc { c.Next() return } - _, _ = page.Call("Network.enable", nil) - _, err := page.Call("Network.setExtraHTTPHeaders", map[string]any{ - "headers": map[string]string{ - name: value, + remove, err := page.InterceptRequests(browse.RequestRule{ + Name: "auth-header:" + name, + Decide: func(r browse.InterceptedRequest) browse.RequestVerdict { + if r.SameOriginAsTop() { + return browse.RequestVerdict{AddHeaders: map[string]string{name: value}} + } + return browse.RequestVerdict{} }, }) if err != nil { c.AbortWithError(fmt.Errorf("browse: failed to set auth header: %w", err)) return } + defer remove() c.Next() } } diff --git a/middleware/network.go b/middleware/network.go index f4b3597..78f30be 100644 --- a/middleware/network.go +++ b/middleware/network.go @@ -18,38 +18,32 @@ func BlockResources(resourceTypes ...string) browse.HandlerFunc { return } - // Enable Fetch domain to intercept requests - patterns := make([]map[string]string, 0, len(resourceTypes)) + blocked := make(map[string]struct{}, len(resourceTypes)) for _, rt := range resourceTypes { - patterns = append(patterns, map[string]string{ - "resourceType": rt, - }) + blocked[rt] = struct{}{} } - _, err := page.Call("Fetch.enable", map[string]any{ - "patterns": patterns, + // Register a rule on the page's shared request interceptor so resource + // blocking coexists with the URL-policy guard and header injection + // instead of fighting over Fetch.enable. + remove, err := page.InterceptRequests(browse.RequestRule{ + Name: "block-resources", + ResourceTypes: resourceTypes, + Decide: func(r browse.InterceptedRequest) browse.RequestVerdict { + if _, ok := blocked[r.ResourceType]; ok { + return browse.RequestVerdict{Block: true, BlockReason: "BlockedByClient"} + } + return browse.RequestVerdict{} + }, }) if err != nil { - // Fetch domain might not be available; proceed without blocking + // Fetch domain might not be available; proceed without blocking. c.Next() return } - - // Listen for intercepted requests and fail them - page.OnSession("Fetch.requestPaused", func(params map[string]any) { - requestID, _ := params["requestId"].(string) - if requestID != "" { - _, _ = page.Call("Fetch.failRequest", map[string]any{ - "requestId": requestID, - "errorReason": "BlockedByClient", - }) - } - }) + defer remove() c.Next() - - // Disable Fetch after task completes - _, _ = page.Call("Fetch.disable", nil) } } diff --git a/page.go b/page.go index 084bb03..3818176 100644 --- a/page.go +++ b/page.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "strings" + "sync" "time" "go.klarlabs.de/scout/internal/cdp" @@ -30,6 +31,10 @@ type Page struct { rootNodeID int64 // cached DOM root node ID urlValidator URLValidator flattenedNodes []flatNode // cached flattened DOM (pierce: true) + + fetch *fetchInterceptor // lazy CDP Fetch interception multiplexer + navOrigin string // origin of the last caller-initiated Navigate + navOriginMu sync.Mutex } // call sends a CDP command scoped to this page's session, with a per-call @@ -77,6 +82,17 @@ func newPage(conn *cdp.Conn, targetID string, timeout time.Duration, validator U } } + // Re-validate every navigation/redirect against the URL policy at request + // time — this blocks a public URL that redirects to an internal host, which + // the one-shot pre-navigation check can't catch. Trusted (AllowPrivateIPs) + // sessions skip it. + if !p.urlValidator.AllowPrivateIPs { + if err := p.installURLPolicy(); err != nil { + cancel() + return nil, fmt.Errorf("browse: failed to install URL policy: %w", err) + } + } + return p, nil } @@ -87,6 +103,10 @@ func (p *Page) Navigate(rawURL string) error { return &NavigationError{URL: rawURL, Err: err} } + // Record the intended target origin so header-injection rules can scope to it + // (and not leak to cross-origin subresources or redirect destinations). + p.setTopLevelOrigin(originOf(rawURL)) + // Invalidate cached root node ID and flattened DOM on navigation p.rootNodeID = 0 p.flattenedNodes = nil diff --git a/page_fetch.go b/page_fetch.go new file mode 100644 index 0000000..c011cb2 --- /dev/null +++ b/page_fetch.go @@ -0,0 +1,270 @@ +package browse + +import ( + "net/url" + "sync" +) + +// RequestRule inspects intercepted requests and decides what happens to each. +// Rules are consulted in registration order: the first to Block fails the +// request; otherwise every rule's AddHeaders are merged onto a continued +// request. This lets independent concerns — URL policy, resource blocking, +// header injection — share a single CDP Fetch session instead of fighting over +// Fetch.enable (only one owner can, and each paused request must be resolved +// exactly once). +type RequestRule struct { + Name string + // ResourceTypes limits which CDP resource types are intercepted for this + // rule (e.g. "Document", "Image"). Empty means all types. This only shapes + // the Fetch pattern set; a rule must still self-filter in Decide because + // another rule may have widened interception. + ResourceTypes []string + Decide func(InterceptedRequest) RequestVerdict +} + +// InterceptedRequest is the subset of a paused CDP request a rule sees. +type InterceptedRequest struct { + URL string + Method string + ResourceType string + Headers map[string]string + // TopLevelOrigin is the origin of the last caller-initiated navigation, used + // to scope header injection to the intended target (not cross-origin + // subresources or redirect destinations). + TopLevelOrigin string +} + +// RequestVerdict is a rule's decision for one intercepted request. +type RequestVerdict struct { + Block bool + BlockReason string // CDP errorReason; defaults to BlockedByClient + AddHeaders map[string]string // merged onto the continued request's headers +} + +// fetchInterceptor is the single owner of CDP Fetch interception for a page. +type fetchInterceptor struct { + page *Page + mu sync.Mutex + rules []RequestRule + unsub func() + on bool +} + +// InterceptRequests registers a request-interception rule and returns a func to +// remove it. Registering the first rule enables Fetch; removing the last +// disables it. +func (p *Page) InterceptRequests(rule RequestRule) (func(), error) { + fi := p.fetchInterceptor() + fi.mu.Lock() + fi.rules = append(fi.rules, rule) + err := fi.syncLocked() + fi.mu.Unlock() + if err != nil { + p.removeRule(rule.Name) + return func() {}, err + } + var once sync.Once + return func() { once.Do(func() { p.removeRule(rule.Name) }) }, nil +} + +func (p *Page) fetchInterceptor() *fetchInterceptor { + if p.fetch == nil { + p.fetch = &fetchInterceptor{page: p} + } + return p.fetch +} + +func (p *Page) removeRule(name string) { + fi := p.fetch + if fi == nil { + return + } + fi.mu.Lock() + defer fi.mu.Unlock() + for i, r := range fi.rules { + if r.Name == name { + fi.rules = append(fi.rules[:i], fi.rules[i+1:]...) + break + } + } + _ = fi.syncLocked() +} + +// syncLocked reconciles the CDP Fetch state with the current rule set. Caller +// holds fi.mu. +func (fi *fetchInterceptor) syncLocked() error { + if len(fi.rules) == 0 { + if fi.on { + _, _ = fi.page.call("Fetch.disable", nil) + if fi.unsub != nil { + fi.unsub() + fi.unsub = nil + } + fi.on = false + } + return nil + } + if _, err := fi.page.call("Fetch.enable", map[string]any{"patterns": fi.patternsLocked()}); err != nil { + return err + } + if fi.unsub == nil { + fi.unsub = fi.page.OnSession("Fetch.requestPaused", fi.handle) + } + fi.on = true + return nil +} + +// patternsLocked builds the Fetch pattern set as the union of the rules' types; +// any rule wanting all types collapses it to a single catch-all pattern. +func (fi *fetchInterceptor) patternsLocked() []map[string]any { + seen := map[string]struct{}{} + for _, r := range fi.rules { + if len(r.ResourceTypes) == 0 { + return []map[string]any{{}} // all requests + } + for _, t := range r.ResourceTypes { + seen[t] = struct{}{} + } + } + out := make([]map[string]any, 0, len(seen)) + for t := range seen { + out = append(out, map[string]any{"resourceType": t}) + } + return out +} + +// handle resolves one paused request. It runs on the CDP dispatch goroutine +// (off the read loop), so the continue/fail CDP calls it makes here don't +// deadlock. +func (fi *fetchInterceptor) handle(params map[string]any) { + reqID, _ := params["requestId"].(string) + if reqID == "" { + return + } + reqObj, _ := params["request"].(map[string]any) + req := InterceptedRequest{ + ResourceType: asString(params["resourceType"]), + TopLevelOrigin: fi.page.topLevelOrigin(), + } + if reqObj != nil { + req.URL = asString(reqObj["url"]) + req.Method = asString(reqObj["method"]) + req.Headers = asStringMap(reqObj["headers"]) + } + + fi.mu.Lock() + rules := append([]RequestRule(nil), fi.rules...) + fi.mu.Unlock() + + merged := map[string]string{} + for _, r := range rules { + v := r.Decide(req) + if v.Block { + reason := v.BlockReason + if reason == "" { + reason = "BlockedByClient" + } + _, _ = fi.page.call("Fetch.failRequest", map[string]any{"requestId": reqID, "errorReason": reason}) + return + } + for k, val := range v.AddHeaders { + merged[k] = val + } + } + + cont := map[string]any{"requestId": reqID} + if len(merged) > 0 { + all := map[string]string{} + for k, v := range req.Headers { + all[k] = v + } + for k, v := range merged { + all[k] = v + } + entries := make([]map[string]string, 0, len(all)) + for k, v := range all { + entries = append(entries, map[string]string{"name": k, "value": v}) + } + cont["headers"] = entries + } + _, _ = fi.page.call("Fetch.continueRequest", cont) +} + +// installURLPolicy adds the redirect/navigation guard: every Document request +// (initial navigation and each redirect) is re-validated against the page's URL +// policy, so a public URL that redirects to an internal host is blocked before +// Chrome ever fetches it. Only meaningful when the validator blocks private IPs. +func (p *Page) installURLPolicy() error { + _, err := p.InterceptRequests(RequestRule{ + Name: "url-policy", + ResourceTypes: []string{"Document"}, + Decide: func(r InterceptedRequest) RequestVerdict { + if r.ResourceType != "Document" { + return RequestVerdict{} + } + if err := p.urlValidator.Validate(r.URL); err != nil { + return RequestVerdict{Block: true, BlockReason: "AccessDenied"} + } + return RequestVerdict{} + }, + }) + return err +} + +func (p *Page) topLevelOrigin() string { + p.navOriginMu.Lock() + defer p.navOriginMu.Unlock() + return p.navOrigin +} + +func (p *Page) setTopLevelOrigin(origin string) { + p.navOriginMu.Lock() + p.navOrigin = origin + p.navOriginMu.Unlock() +} + +// SameOriginAsTop reports whether the request targets the same origin as the +// page's last caller-initiated navigation — used by header-injection rules to +// avoid leaking credentials to cross-origin subresources or redirect targets. +func (r InterceptedRequest) SameOriginAsTop() bool { + return sameOrigin(r.URL, r.TopLevelOrigin) +} + +func sameOrigin(rawURL, origin string) bool { + if origin == "" { + return false + } + u, err := url.Parse(rawURL) + if err != nil { + return false + } + return u.Scheme+"://"+u.Host == origin +} + +// originOf returns scheme://host for a URL, or "" if it can't be parsed. +func originOf(rawURL string) string { + u, err := url.Parse(rawURL) + if err != nil || u.Host == "" { + return "" + } + return u.Scheme + "://" + u.Host +} + +func asString(v any) string { + s, _ := v.(string) + return s +} + +func asStringMap(v any) map[string]string { + m, ok := v.(map[string]any) + if !ok { + return nil + } + out := make(map[string]string, len(m)) + for k, val := range m { + if s, ok := val.(string); ok { + out[k] = s + } + } + return out +} diff --git a/page_fetch_integration_test.go b/page_fetch_integration_test.go new file mode 100644 index 0000000..a5356bc --- /dev/null +++ b/page_fetch_integration_test.go @@ -0,0 +1,153 @@ +//go:build integration + +package browse + +import ( + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" +) + +// redirectServer serves /start (302 → /blocked) and records whether /blocked was +// ever actually fetched, plus any X-Test header seen on /start. +func redirectServer(blockedHit *atomic.Bool, sawHeader *atomic.Bool) *httptest.Server { + mux := http.NewServeMux() + mux.HandleFunc("/start", func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("X-Test") == "yes" { + sawHeader.Store(true) + } + http.Redirect(w, r, "/blocked", http.StatusFound) + }) + mux.HandleFunc("/blocked", func(w http.ResponseWriter, _ *http.Request) { + blockedHit.Store(true) + _, _ = w.Write([]byte("SECRET-INTERNAL-DATA")) + }) + return httptest.NewServer(mux) +} + +// TestIntegrationInterceptor_BlocksRedirect proves the shared interceptor fails a +// redirect at the CDP Fetch layer: with a rule blocking /blocked, Chrome must +// never fetch it even though /start redirects there. This is the mechanism the +// URL-policy (redirect-SSRF) guard relies on. +func TestIntegrationInterceptor_BlocksRedirect(t *testing.T) { + if testing.Short() { + t.Skip("requires Chrome") + } + var blockedHit, sawHeader atomic.Bool + ts := redirectServer(&blockedHit, &sawHeader) + defer ts.Close() + + engine := New(WithHeadless(true), WithAllowPrivateIPs(true)) + if err := engine.Launch(); err != nil { + t.Fatalf("Launch: %v", err) + } + defer engine.Close() + + engine.Task("block", func(c *Context) { + page := c.Page() + remove, err := page.InterceptRequests(RequestRule{ + Name: "block-blocked", + ResourceTypes: []string{"Document"}, + Decide: func(r InterceptedRequest) RequestVerdict { + if strings.Contains(r.URL, "/blocked") { + return RequestVerdict{Block: true, BlockReason: "AccessDenied"} + } + return RequestVerdict{} + }, + }) + if err != nil { + c.AbortWithError(err) + return + } + defer remove() + _ = page.Navigate(ts.URL + "/start") // the redirect must be blocked + }) + _ = engine.Run("block") // navigation may error on the blocked redirect — fine + + if blockedHit.Load() { + t.Error("interceptor did not block the redirect: /blocked was fetched") + } +} + +// TestIntegrationInterceptor_AllowsWhenNoMatch proves the continue path doesn't +// break navigation: a rule that never blocks must let /start → /blocked proceed. +func TestIntegrationInterceptor_AllowsWhenNoMatch(t *testing.T) { + if testing.Short() { + t.Skip("requires Chrome") + } + var blockedHit, sawHeader atomic.Bool + ts := redirectServer(&blockedHit, &sawHeader) + defer ts.Close() + + engine := New(WithHeadless(true), WithAllowPrivateIPs(true)) + if err := engine.Launch(); err != nil { + t.Fatalf("Launch: %v", err) + } + defer engine.Close() + + engine.Task("allow", func(c *Context) { + page := c.Page() + remove, err := page.InterceptRequests(RequestRule{ + Name: "noop", + ResourceTypes: []string{"Document"}, + Decide: func(InterceptedRequest) RequestVerdict { return RequestVerdict{} }, + }) + if err != nil { + c.AbortWithError(err) + return + } + defer remove() + c.MustNavigate(ts.URL + "/start") + }) + if err := engine.Run("allow"); err != nil { + t.Fatalf("Run: %v", err) + } + + if !blockedHit.Load() { + t.Error("interceptor broke navigation: /blocked never reached with a no-op rule") + } +} + +// TestIntegrationInterceptor_InjectsSameOriginHeader proves header injection is +// applied to same-origin requests via the continue-with-headers path. +func TestIntegrationInterceptor_InjectsSameOriginHeader(t *testing.T) { + if testing.Short() { + t.Skip("requires Chrome") + } + var blockedHit, sawHeader atomic.Bool + ts := redirectServer(&blockedHit, &sawHeader) + defer ts.Close() + + engine := New(WithHeadless(true), WithAllowPrivateIPs(true)) + if err := engine.Launch(); err != nil { + t.Fatalf("Launch: %v", err) + } + defer engine.Close() + + engine.Task("hdr", func(c *Context) { + page := c.Page() + remove, err := page.InterceptRequests(RequestRule{ + Name: "inject", + ResourceTypes: []string{"Document"}, + Decide: func(r InterceptedRequest) RequestVerdict { + if r.SameOriginAsTop() { + return RequestVerdict{AddHeaders: map[string]string{"X-Test": "yes"}} + } + return RequestVerdict{} + }, + }) + if err != nil { + c.AbortWithError(err) + return + } + defer remove() + _ = page.Navigate(ts.URL + "/start") + }) + _ = engine.Run("hdr") + + if !sawHeader.Load() { + t.Error("same-origin header injection failed: /start did not receive X-Test") + } +} diff --git a/page_fetch_test.go b/page_fetch_test.go new file mode 100644 index 0000000..6ec5f38 --- /dev/null +++ b/page_fetch_test.go @@ -0,0 +1,77 @@ +package browse + +import "testing" + +func TestSameOrigin(t *testing.T) { + cases := []struct { + url, origin string + want bool + }{ + {"https://a.com/x", "https://a.com", true}, + {"https://a.com/x?q=1#f", "https://a.com", true}, + {"https://a.com:8080/x", "https://a.com", false}, // port differs + {"http://a.com", "https://a.com", false}, // scheme differs + {"https://b.com", "https://a.com", false}, // host differs + {"https://a.com", "", false}, // no top-level origin + } + for _, c := range cases { + if got := sameOrigin(c.url, c.origin); got != c.want { + t.Errorf("sameOrigin(%q, %q) = %v, want %v", c.url, c.origin, got, c.want) + } + } +} + +func TestOriginOf(t *testing.T) { + cases := []struct{ url, want string }{ + {"https://a.com/x?y=1", "https://a.com"}, + {"https://a.com:8080/x", "https://a.com:8080"}, + {"http://a.com", "http://a.com"}, + {"not-a-url", ""}, + {"", ""}, + } + for _, c := range cases { + if got := originOf(c.url); got != c.want { + t.Errorf("originOf(%q) = %q, want %q", c.url, got, c.want) + } + } +} + +func TestFetchInterceptorPatterns(t *testing.T) { + // A union of typed rules yields one pattern per distinct resource type. + fi := &fetchInterceptor{rules: []RequestRule{ + {Name: "a", ResourceTypes: []string{"Document"}}, + {Name: "b", ResourceTypes: []string{"Image", "Document"}}, + }} + pats := fi.patternsLocked() + if len(pats) != 2 { + t.Fatalf("expected 2 patterns (Document, Image), got %d: %+v", len(pats), pats) + } + got := map[string]bool{} + for _, p := range pats { + got[p["resourceType"].(string)] = true + } + if !got["Document"] || !got["Image"] { + t.Errorf("patterns missing a type: %+v", pats) + } + + // Any rule wanting all types collapses to a single catch-all pattern. + fiAll := &fetchInterceptor{rules: []RequestRule{ + {Name: "typed", ResourceTypes: []string{"Document"}}, + {Name: "all"}, // no ResourceTypes = all + }} + all := fiAll.patternsLocked() + if len(all) != 1 || len(all[0]) != 0 { + t.Errorf("a catch-all rule should produce one empty pattern, got %+v", all) + } +} + +func TestRequestVerdictSameOriginAsTop(t *testing.T) { + r := InterceptedRequest{URL: "https://api.example.com/v1", TopLevelOrigin: "https://api.example.com"} + if !r.SameOriginAsTop() { + t.Error("same-origin request should match the top-level origin") + } + cross := InterceptedRequest{URL: "https://evil.example.com/steal", TopLevelOrigin: "https://api.example.com"} + if cross.SameOriginAsTop() { + t.Error("cross-origin request must not match the top-level origin") + } +}