From 3fdb44bce23d4908e84e891f206b2e2e306dc5ca Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Wed, 4 Nov 2026 22:56:02 +0100 Subject: [PATCH] fix: the crawler and the Chrome guard keep to the site's port Fetcher.Allowed and Key, the rendered comparison and the Chrome request guard accept an address only on the site's effective port (explicit, else 443 for https and 80 for http), so page code cannot make headless Chrome reach other services on the site host's address and a link to another port is not reported as a page of the site. --- README.md | 2 +- classes/crawl/checks.go | 2 +- classes/crawl/compare.go | 27 ++++++------------ classes/crawl/compare_test.go | 20 +++++++++---- classes/crawl/fetch.go | 35 ++++++++++++++++++++--- classes/crawl/fetch_test.go | 51 ++++++++++++++++++++++++++++++++++ classes/crawl/render.go | 12 ++++---- classes/crawl/render_chrome.go | 13 +++++---- classes/crawl/render_test.go | 24 ++++++++++++---- 9 files changed, 138 insertions(+), 48 deletions(-) create mode 100644 classes/crawl/fetch_test.go diff --git a/README.md b/README.md index f25ad7e..5a152f1 100644 --- a/README.md +++ b/README.md @@ -253,7 +253,7 @@ A crawl is a conga job (`golem15.seo.crawl`, one attempt) started from **Start c **Raw HTML against the CMS.** The server-rendered HTML is checked against the record behind the page (`content_js` with `details.check: cms`): the record's title must appear in an H1, the text must reach half of the record's word count, and the record's primary parent (the breadcrumb link) must be linked. The comparison with the JavaScript-rendered page is `details.check: rendered` (next section); without a Chrome path every crawl records it as skipped (P6 and `seo_get_crawl` say so). -**Rendered comparison.** When `golem15.seo.crawl_chrome_path` names a Chrome or Chromium binary the operator installed on the server (none is installed by default), the crawler renders the first `golem15.seo.crawl_render_max` pages (default 200, in crawl order, so the home page and the top-level pages come first) in headless Chrome through `github.com/chromedp/chromedp`, right after each page's raw fetch, and compares the DOM after JavaScript with the raw HTML: same-host links, an H1, or at least 50 words and 20 % more text that appear only after JavaScript are one `content_js` finding with `details.check: rendered` and `details.what` naming them (up to five links). The browser is held to the crawler's limits: before Chrome starts, the site host and its `www` twin are resolved and every address checked like the crawler's dial guard (`fetchguard.IsPrivateAddr`; a private address refuses unless `golem15.seo.crawl_trusted` is on), and Chrome is pinned to those addresses through `--host-resolver-rules` while every other host resolves to nothing; every request is intercepted and continued only when it is https (http only in trusted mode) to the site host or its twin, or a `data:`/`blob:` URL, so third-party scripts, fonts, analytics and metadata addresses are never loaded. Each crawl gets a fresh temporary profile, removed when the crawl ends, with no cookies, credentials or extensions and downloads denied; one tab renders at a time, at most 5 s to open the tab (`crawl.TabOpenTimeout`) and 20 s for the page under one deadline, so a hung Chrome cannot stall the crawl, the DOM is capped at 5 MiB, Chrome's own sandbox stays on, and Chrome is closed when the crawl finishes, fails or is cancelled. The path is config only (or `SUMMER_GOLEM15__SEO__CRAWL_CHROME_PATH`), never a setting or MCP value. The crawl records `render_status` `done` with the pages compared, `skipped` without a path, or `failed` with the reason (a relative path or one that is not an executable file, a private address, Chrome not starting, a page that cannot be rendered); a renderer failure never fails the crawl, and P6, `GET /_seo/api/v1/crawls/{id}` and `seo_get_crawl` (`render {status, pages, note, message}`) say which. +**Rendered comparison.** When `golem15.seo.crawl_chrome_path` names a Chrome or Chromium binary the operator installed on the server (none is installed by default), the crawler renders the first `golem15.seo.crawl_render_max` pages (default 200, in crawl order, so the home page and the top-level pages come first) in headless Chrome through `github.com/chromedp/chromedp`, right after each page's raw fetch, and compares the DOM after JavaScript with the raw HTML: same-host links, an H1, or at least 50 words and 20 % more text that appear only after JavaScript are one `content_js` finding with `details.check: rendered` and `details.what` naming them (up to five links). The browser is held to the crawler's limits: before Chrome starts, the site host and its `www` twin are resolved and every address checked like the crawler's dial guard (`fetchguard.IsPrivateAddr`; a private address refuses unless `golem15.seo.crawl_trusted` is on), and Chrome is pinned to those addresses through `--host-resolver-rules` while every other host resolves to nothing; every request is intercepted and continued only when it is https (http only in trusted mode) to the site host or its twin on the site's own port (the port of `golem15.seo.site_url`, else 443 for https and 80 for http), or a `data:`/`blob:` URL, so page code cannot reach other services on the site host's address, so third-party scripts, fonts, analytics and metadata addresses are never loaded. Each crawl gets a fresh temporary profile, removed when the crawl ends, with no cookies, credentials or extensions and downloads denied; one tab renders at a time, at most 5 s to open the tab (`crawl.TabOpenTimeout`) and 20 s for the page under one deadline, so a hung Chrome cannot stall the crawl, the DOM is capped at 5 MiB, Chrome's own sandbox stays on, and Chrome is closed when the crawl finishes, fails or is cancelled. The path is config only (or `SUMMER_GOLEM15__SEO__CRAWL_CHROME_PATH`), never a setting or MCP value. The crawl records `render_status` `done` with the pages compared, `skipped` without a path, or `failed` with the reason (a relative path or one that is not an executable file, a private address, Chrome not starting, a page that cannot be rendered); a renderer failure never fails the crawl, and P6, `GET /_seo/api/v1/crawls/{id}` and `seo_get_crawl` (`render {status, pages, note, message}`) say which. **Fix routes.** An issue a CMS value fixes names its tool: `seo_set_url_seo` (titles, descriptions, canonicals, sitemap membership of a record, structured data), `seo_update_robots`, `seo_create_redirect` (a missing page, a page that answers an error), `seo_create_page` (a page missing from the registry), `seo_update_settings` (publisher and site nodes), `sitemap_update_settings` (a listed noindex address without a CMS record, an Additional address of the sitemap: remove it there; `details.extra` is true, and without the sitemap plugin the issue is `fix: code` naming the sitemap's address list), or the content type's own tool from `pact.ContentDescriptor.Tools` when the problem sits in the record's content: H1 findings prefer `Tools[headings]`, image alt text `Tools[image_alt]`, other content findings `Tools[edit]` (the registry pages name `seo_update_page`). A finding of the site's template code, or of content whose type names no fitting tool, is `fix: code` with `details.what` naming the element to change. No issue has neither. diff --git a/classes/crawl/checks.go b/classes/crawl/checks.go index 490c6be..717c4c8 100644 --- a/classes/crawl/checks.go +++ b/classes/crawl/checks.go @@ -731,7 +731,7 @@ func checkRenderedPage(res *Result, p *Page, rec *Record) []Finding { if p.Rendered == nil || res.fetcher == nil { return nil } - out := compareRendered(p.Facts, p.Rendered, res.fetcher.Hosts()) + out := compareRendered(p.Facts, p.Rendered, res.fetcher.Key) for i := range out { out[i].URL, out[i].Locale, out[i].Record = p.Key, locale(rec), rec } diff --git a/classes/crawl/compare.go b/classes/crawl/compare.go index 1888f9a..dc5b106 100644 --- a/classes/crawl/compare.go +++ b/classes/crawl/compare.go @@ -16,32 +16,21 @@ const ( ) // compareRendered compares a page's raw HTML with its JavaScript-rendered -// DOM (D-70, P33-55; pure): same-host links, an H1 or a substantial amount +// DOM (D-70, P33-55; pure): same-site links, an H1 or a substantial amount // of text that appear only after JavaScript are one content_js finding -// with details.check rendered and details.what naming them. hosts are the -// site host and its www twin. An identical rendering finds nothing. -func compareRendered(raw, rendered *PageFacts, hosts []string) []Finding { +// with details.check rendered and details.what naming them. key is the +// crawl's address of a link (Fetcher.Key: "" off the site, another port +// included). An identical rendering finds nothing. +func compareRendered(raw, rendered *PageFacts, key func(*url.URL) string) []Finding { if raw == nil || rendered == nil { return nil } same := func(u *url.URL) (string, bool) { - if u == nil { + if u == nil || key == nil { return "", false } - host := strings.ToLower(u.Hostname()) - for _, h := range hosts { - if host == h { - p := u.EscapedPath() - if p == "" { - p = "/" - } - if u.RawQuery != "" { - p += "?" + u.RawQuery - } - return p, true - } - } - return "", false + k := key(u) + return k, k != "" } before := map[string]bool{} for _, l := range raw.Links { diff --git a/classes/crawl/compare_test.go b/classes/crawl/compare_test.go index 7fa0799..566b94d 100644 --- a/classes/crawl/compare_test.go +++ b/classes/crawl/compare_test.go @@ -18,7 +18,15 @@ func linkFacts(words int, h1 string, hrefs ...string) *PageFacts { return f } -var siteHosts = []string{"example.test", "www.example.test"} +// siteKey is the crawl key of a link on https://example.test (its www +// twin too, port 443 only). +func siteKey(u *url.URL) string { + f, err := NewFetcher("https://example.test", nil) + if err != nil { + panic(err) + } + return f.Key(u) +} // TestCompareRendered pins the rendered comparison (P33-55): a same-host // link (the www twin too), an H1 and growth of at least 50 words and 20 % @@ -28,7 +36,7 @@ var siteHosts = []string{"example.test", "www.example.test"} func TestCompareRendered(t *testing.T) { raw := linkFacts(100, "", "https://example.test/a") rendered := linkFacts(160, "Grzyby", "https://example.test/a", "https://www.example.test/b?x=1", "https://other.test/c", "https://example.test/b?x=1") - got := compareRendered(raw, rendered, siteHosts) + got := compareRendered(raw, rendered, siteKey) if len(got) != 1 || got[0].Kind != KindContentJS || got[0].Check != checkRendered { t.Fatalf("findings = %+v", got) } @@ -47,14 +55,14 @@ func TestCompareRendered(t *testing.T) { "50 words under 20 %": {linkFacts(300, "T"), linkFacts(350, "T")}, "H1 already there": {linkFacts(10, "Raw"), linkFacts(10, "Rendered")}, } { - if got := compareRendered(pair[0], pair[1], siteHosts); got != nil { + if got := compareRendered(pair[0], pair[1], siteKey); got != nil { t.Errorf("%s: %+v", name, got) } } - if got := compareRendered(linkFacts(0, "T"), linkFacts(50, "T"), siteHosts); len(got) != 1 { + if got := compareRendered(linkFacts(0, "T"), linkFacts(50, "T"), siteKey); len(got) != 1 { t.Fatalf("50 words from an empty page: %+v", got) } - if compareRendered(nil, rendered, siteHosts) != nil || compareRendered(raw, nil, siteHosts) != nil { + if compareRendered(nil, rendered, siteKey) != nil || compareRendered(raw, nil, siteKey) != nil { t.Fatal("a missing side compares to nothing") } } @@ -66,7 +74,7 @@ func TestCompareRenderedCap(t *testing.T) { for _, p := range []string{"", "/1", "/2", "/3", "/4", "/5", "/6"} { hrefs = append(hrefs, "https://example.test"+p) } - got := compareRendered(linkFacts(10, "T"), linkFacts(10, "T", hrefs...), siteHosts) + got := compareRendered(linkFacts(10, "T"), linkFacts(10, "T", hrefs...), siteKey) links := got[0].Details["links"].([]string) if len(links) != maxJSOnlyURLs || links[0] != "/" { t.Fatalf("links = %v", links) diff --git a/classes/crawl/fetch.go b/classes/crawl/fetch.go index 3d7b238..fec3f70 100644 --- a/classes/crawl/fetch.go +++ b/classes/crawl/fetch.go @@ -65,10 +65,29 @@ type Fetcher struct { client *fetchguard.Client site *url.URL hosts []string + port string trusted bool ua string } +// effectivePort is u's port: the explicit one, else 443 for https and 80 +// for http ("" for another scheme without a port). +func effectivePort(u *url.URL) string { + if u == nil { + return "" + } + if p := u.Port(); p != "" { + return p + } + switch strings.ToLower(u.Scheme) { + case "https": + return "443" + case "http": + return "80" + } + return "" +} + // SiteHosts returns the host of site and its www twin. func SiteHosts(host string) []string { host = strings.ToLower(strings.TrimSpace(host)) @@ -92,7 +111,7 @@ func NewFetcher(site string, cfg *compass.Config) (*Fetcher, error) { if err != nil || u.Host == "" || (u.Scheme != "https" && u.Scheme != "http") { return nil, ErrNoSiteURL } - f := &Fetcher{site: u, hosts: SiteHosts(u.Hostname()), trusted: Trusted(cfg), ua: UserAgent(u.Scheme + "://" + u.Host)} + f := &Fetcher{site: u, hosts: SiteHosts(u.Hostname()), port: effectivePort(u), trusted: Trusted(cfg), ua: UserAgent(u.Scheme + "://" + u.Host)} policy := fetchguard.Policy{Mode: fetchguard.AllowHostsMode, AllowHosts: f.hosts, MaxBytes: MaxBody, Timeout: FetchTimeout} if f.trusted { policy.Mode = fetchguard.TrustedMode @@ -116,13 +135,21 @@ func (f *Fetcher) Site() *url.URL { return f.site } // UA returns the crawler's User-Agent. func (f *Fetcher) UA() string { return f.ua } +// Port returns the site's effective port (explicit, else 443 for https +// and 80 for http). +func (f *Fetcher) Port() string { return f.port } + // Allowed reports whether u may be fetched: https (http only in trusted -// mode) on the site host or its www twin, without credentials. In trusted -// mode the host list is checked here, since fetchguard checks none then. +// mode) on the site host or its www twin and on the site's effective port, +// without credentials. In trusted mode the host list is checked here, +// since fetchguard checks none then. func (f *Fetcher) Allowed(u *url.URL) bool { if u == nil || u.User != nil { return false } + if effectivePort(u) != f.port { + return false + } switch strings.ToLower(u.Scheme) { case "https": case "http": @@ -142,7 +169,7 @@ func (f *Fetcher) Allowed(u *url.URL) bool { } // Key is the address of u on the site: its path with the query, or "" -// when u is not on the site. +// when u is not on the site (another host, scheme or port). func (f *Fetcher) Key(u *url.URL) string { if !f.Allowed(u) { return "" diff --git a/classes/crawl/fetch_test.go b/classes/crawl/fetch_test.go new file mode 100644 index 0000000..247e949 --- /dev/null +++ b/classes/crawl/fetch_test.go @@ -0,0 +1,51 @@ +package crawl + +import ( + "net/url" + "testing" +) + +// TestFetcherPortAndKey pins L3 WR-03 (and WR-04, P33-61): the fetcher +// accepts an address only on the site's effective port, so a link to +// another port is neither fetched nor reported as a page of the site. +func TestFetcherPortAndKey(t *testing.T) { + f, err := NewFetcher("https://site.test", nil) + if err != nil { + t.Fatal(err) + } + if f.Port() != "443" { + t.Fatalf("Port = %q", f.Port()) + } + parse := func(raw string) *url.URL { + u, err := url.Parse(raw) + if err != nil { + t.Fatal(err) + } + return u + } + for raw, want := range map[string]string{ + "https://site.test:8443/x": "", + "https://site.test:443/x": "/x", + "https://www.site.test/x": "/x", + "https://site.test:80/x": "", + "http://site.test/x": "", + "https://other.test/x": "", + "https://site.test": "/", + "https://site.test/a?b=1&c=": "/a?b=1&c=", + } { + u := parse(raw) + if got := f.Key(u); got != want { + t.Errorf("Key(%s) = %q, want %q", raw, got, want) + } + if f.Allowed(u) != (want != "") { + t.Errorf("Allowed(%s) = %v", raw, f.Allowed(u)) + } + } + local, err := NewFetcher("http://localhost:3000", nil) + if err != nil { + t.Fatal(err) + } + if local.Port() != "3000" || effectivePort(parse("http://x")) != "80" || effectivePort(parse("ftp://x")) != "" || effectivePort(nil) != "" { + t.Fatal("effectivePort is wrong") + } +} diff --git a/classes/crawl/render.go b/classes/crawl/render.go index 605e006..9de36aa 100644 --- a/classes/crawl/render.go +++ b/classes/crawl/render.go @@ -103,7 +103,7 @@ func NewRenderer(ctx context.Context, cfg *compass.Config, site *url.URL, truste if err != nil { return nil, err } - r, err := newChromeRenderer(path, hosts, trusted, ua) + r, err := newChromeRenderer(path, hosts, effectivePort(site), trusted, ua) if err != nil { return nil, err } @@ -161,9 +161,11 @@ func resolverRules(hosts map[string][]netip.Addr) string { } // allowRequest reports whether the headless browser may load rawURL: https -// (http only in trusted mode) to one of hosts, or a data: or blob: URL. -// Every other request, third-party scripts included, is failed. -func allowRequest(rawURL string, hosts []string, trusted bool) bool { +// (http only in trusted mode) to one of hosts on the site's effective port +// (port; the request's explicit port, else 443 for https and 80 for http), +// or a data: or blob: URL. Every other request, third-party scripts and +// other services on the site host's address included, is failed. +func allowRequest(rawURL string, hosts []string, port string, trusted bool) bool { u, err := url.Parse(rawURL) if err != nil { return false @@ -179,7 +181,7 @@ func allowRequest(rawURL string, hosts []string, trusted bool) bool { default: return false } - if u.User != nil { + if u.User != nil || effectivePort(u) != port { return false } host := strings.ToLower(u.Hostname()) diff --git a/classes/crawl/render_chrome.go b/classes/crawl/render_chrome.go index cfd7f79..bee8f6f 100644 --- a/classes/crawl/render_chrome.go +++ b/classes/crawl/render_chrome.go @@ -29,7 +29,7 @@ import ( // fetchguard.IsPrivateAddr; every other host resolves to NOTFOUND; // - every request is paused through the Fetch domain and continued only // when allowRequest passes (https, http only in trusted mode, to an -// allowed host, or data: and blob:); the others fail with +// allowed host on the site's port, or data: and blob:); the others fail with // BlockedByClient, so third-party scripts are never loaded; // - a fresh temporary profile per crawl, removed by Close; no cookies, // credentials or extensions; downloads denied; @@ -43,6 +43,7 @@ import ( // added that weakens Chrome's sandbox. type chromeRenderer struct { allowed []string + port string trusted bool profile string @@ -75,13 +76,13 @@ func seconds(d time.Duration) string { } // newChromeRenderer starts Chrome at path for the hosts and their checked -// addresses. -func newChromeRenderer(path string, hosts map[string][]netip.Addr, trusted bool, ua string) (*chromeRenderer, error) { +// addresses, on the site's effective port. +func newChromeRenderer(path string, hosts map[string][]netip.Addr, port string, trusted bool, ua string) (*chromeRenderer, error) { profile, err := os.MkdirTemp("", "summercms-seo-chrome-") if err != nil { return nil, fmt.Errorf("the Chrome profile cannot be created: %w", err) } - r := &chromeRenderer{trusted: trusted, profile: profile} + r := &chromeRenderer{port: port, trusted: trusted, profile: profile} for h := range hosts { r.allowed = append(r.allowed, h) } @@ -117,7 +118,7 @@ func (r *chromeRenderer) Render(ctx context.Context, pageURL string) (string, er if closed { return "", fmt.Errorf("Chrome is closed") } - if !allowRequest(pageURL, r.allowed, r.trusted) { + if !allowRequest(pageURL, r.allowed, r.port, r.trusted) { return "", fmt.Errorf("the address %s is outside the site", pageURL) } // One deadline bounds opening the tab and rendering the page; the tab @@ -206,7 +207,7 @@ func (r *chromeRenderer) decide(ctx context.Context, target cdp.Session, ev fetc if ev.Request != nil { u = ev.Request.URL } - if allowRequest(u, r.allowed, r.trusted) { + if allowRequest(u, r.allowed, r.port, r.trusted) { _, _ = cdp.Call(ctx, target, fetch.ContinueRequest, fetch.ContinueRequestParams{RequestID: ev.RequestID}) return } diff --git a/classes/crawl/render_test.go b/classes/crawl/render_test.go index cb8c146..874d097 100644 --- a/classes/crawl/render_test.go +++ b/classes/crawl/render_test.go @@ -14,8 +14,9 @@ import ( ) // TestAllowRequest pins what the headless browser may load: https on the -// site host and its www twin, data: and blob:; http only in trusted mode; -// other hosts, credentials in the URL and every other scheme refused. +// site host and its www twin on the site's port, data: and blob:; http +// only in trusted mode, also on the site's port; other hosts and ports, +// credentials in the URL and every other scheme refused (L3 WR-03). func TestAllowRequest(t *testing.T) { hosts := []string{"example.test", "www.example.test"} for _, tc := range []struct { @@ -25,11 +26,13 @@ func TestAllowRequest(t *testing.T) { }{ {"https://example.test/a.js", false, true}, {"https://WWW.example.test/b.css", false, true}, - {"https://example.test:8443/x", false, true}, + {"https://example.test:8443/x", false, false}, + {"https://example.test:443/x", false, true}, + {"https://example.test:80/x", false, false}, {"data:image/png;base64,AAAA", false, true}, {"blob:https://example.test/uuid", false, true}, {"http://example.test/a.js", false, false}, - {"http://example.test/a.js", true, true}, + {"http://example.test/a.js", true, false}, // trusted, but port 80 is not the site's 443 {"https://cdn.other.test/lib.js", false, false}, {"https://example.test.evil.test/x", false, false}, {"https://user:pw@example.test/x", false, false}, @@ -38,11 +41,19 @@ func TestAllowRequest(t *testing.T) { {"javascript:alert(1)", true, false}, {"://bad", false, false}, } { - if got := allowRequest(tc.url, hosts, tc.trusted); got != tc.want { + if got := allowRequest(tc.url, hosts, "443", tc.trusted); got != tc.want { t.Errorf("allowRequest(%q, trusted=%v) = %v, want %v", tc.url, tc.trusted, got, tc.want) } } - if !allowRequest("https://[::1]/x", []string{"[::1]"}, false) { + // A trusted http site on its own port: that port only. + if !allowRequest("http://example.test:3000/x", hosts, "3000", true) || allowRequest("http://example.test:3001/x", hosts, "3000", true) || + allowRequest("http://example.test/x", hosts, "3000", true) { + t.Error("trusted http keeps to the site's port") + } + if !allowRequest("data:text/plain,x", hosts, "3000", false) || !allowRequest("blob:https://example.test/uuid", hosts, "3000", false) { + t.Error("data: and blob: are allowed on any port") + } + if !allowRequest("https://[::1]/x", []string{"[::1]"}, "443", false) { t.Fatal("a bracketed IPv6 host must match") } } @@ -138,6 +149,7 @@ func TestRenderTabTimeout(t *testing.T) { t.Cleanup(func() { close(hung) }) r := &chromeRenderer{ allowed: []string{"example.test"}, + port: "443", browserCtx: context.Background(), newTab: func(parent context.Context) (context.Context, context.CancelFunc) { return context.WithCancel(parent)