From 327726a06a02365d2fe45a19a71f52dece0e170a Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:33:26 +0400 Subject: [PATCH] crawl: stop letting a watch widen on-demand reading, and honour Crawl-delay The on-demand crawler was built over allow_hosts plus every watched host. webfetch reads a non-empty allow list as these and nothing else, so a config with one watch and no allow_hosts at all silently narrowed on-demand reading to the watched site. Every other url he pasted came back as a flat refusal with nothing in the log to explain it. The two crawlers now take two host lists from one crawlHosts helper. Crawl-delay was parsed into Rules and never read. The only pacing was the fetcher's flat one request per host per second, which cannot express what a site asked for, and deploy/README claimed the field was honoured. Page now waits it out between the robots fetch and the page fetch, and a delay longer than the turn fails the read instead of hanging it. A robots.txt that failed was treated as no rules, so a site whose server was having a bad minute became a site with no restrictions. A 5xx now refuses the crawl. A 404 still means unrestricted, which is what the standard says. The refusal check matched substrings of webfetch's message text from a package that cannot import webfetch, so a reworded error would have silently turned into a robots verdict. internal/crawl now exports ErrFetchRefused and ErrFetchStatus and the adapter in cmd/mavend maps the webfetch sentinels onto them. Robots group selection picks the longest matching agent prefix instead of the first one in file order. queryWeb passed a claim it could not serve when no crawler was configured, so an unconfigured deployment answered a web question with an apology instead of falling through to the model. Found in review of #67. --- cmd/mavend/actions_query.go | 20 ++-- cmd/mavend/crawls.go | 53 ++++++++-- cmd/mavend/crawls_test.go | 72 +++++++++++-- deploy/README.md | 13 ++- internal/config/config.go | 9 +- internal/crawl/crawl.go | 170 ++++++++++++++++++++++-------- internal/crawl/politeness_test.go | 115 ++++++++++++++++++++ internal/crawl/robots.go | 11 +- 8 files changed, 384 insertions(+), 79 deletions(-) create mode 100644 internal/crawl/politeness_test.go diff --git a/cmd/mavend/actions_query.go b/cmd/mavend/actions_query.go index 6918283..bd37e54 100644 --- a/cmd/mavend/actions_query.go +++ b/cmd/mavend/actions_query.go @@ -92,10 +92,12 @@ var querySources = []querySource{ {"memory", (*reactiveHandler).queryMemory}, {"notes", (*reactiveHandler).queryNotes}, // LAST before the model answers from memory, and that position is the whole - // design (Vikunja #259): local sources first. The model, his own notes and - // facts, and — once internal/kiwix is wired into this chain — the offline - // ZIMs all get their turn before anything touches the network. This source - // only claims a turn where he named a URL out loud, so it never competes + // design (Vikunja #259): local sources first. His memory, his notes and — + // once internal/kiwix is wired into this chain — the offline ZIMs all get + // their turn before anything touches the network. The model does NOT: it + // answers after this, because a URL he said out loud is an instruction and + // a 1.7B guessing at a page it cannot read is how contents get invented. + // This source only claims a turn where he named a URL, so it never competes // with a local answer. {"web", (*reactiveHandler).queryWeb}, {"general-knowledge", (*reactiveHandler).queryGeneral}, @@ -444,10 +446,12 @@ func (h *reactiveHandler) queryWeb(ctx context.Context, t *queryTurn) (string, b return "", false } if h.crawler == nil { - // Claim rather than fall through: he asked about a specific page, and - // letting the model answer from the URL's spelling alone is how a small - // model invents a page's contents. - return "я не читаю страницы — это не настроено.", true + // Fall through. Reading pages is off unless configured, and on a daemon + // where it was never turned on the older behaviour is right: the model + // answers the question as if the URL had not been said. Announcing a + // configuration status is for a capability that exists and failed, not + // for one he never asked for. + return "", false } ctxFetch, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() diff --git a/cmd/mavend/crawls.go b/cmd/mavend/crawls.go index e20a01a..a2d8a7c 100644 --- a/cmd/mavend/crawls.go +++ b/cmd/mavend/crawls.go @@ -20,6 +20,8 @@ package main import ( "context" + "errors" + "fmt" "log" "net/url" "time" @@ -39,21 +41,36 @@ func newCrawler(cfg *config.Config) *crawl.Crawler { return nil } cc := cfg.Crawl + // The WATCH crawler, and only it, reaches the watched hosts. webfetch reads + // a non-empty allow list as "these and nothing else", so folding the watch + // hosts in turned a single watch into an allowlist for everything: a config + // with one watch and on_demand true silently refused every other page he + // pasted, with "не получилось прочитать страницу." and no clue why. + return crawlerWithHosts(cc, crawlHosts(cc, true)) +} +// crawlHosts — the allowlist for one of the two crawlers. forWatches adds the +// watched pages' own hosts, so a watch does not have to be allowlisted by hand. +// +// The on-demand crawler gets his allow_hosts and nothing else. webfetch reads a +// non-empty list as "these and nothing else", so adding the watch hosts there +// would silently narrow on-demand reading to the watched sites. +func crawlHosts(cc *config.CrawlConfig, forWatches bool) []string { hosts := append([]string(nil), cc.AllowHosts...) - // A watched page's own host is always reachable; otherwise an allowlist and - // a watch list would have to be kept in sync by hand. + if !forWatches { + return hosts + } for _, w := range cc.Watches { if u, err := url.Parse(w.URL); err == nil && u.Hostname() != "" { hosts = append(hosts, u.Hostname()) } } - // An allowlist plus on-demand is a contradiction worth logging rather than - // silently resolving: he asked for arbitrary pages AND for a fixed list. - // The allowlist wins, because it is the narrower instruction. - if len(hosts) > 0 && cc.OnDemand && len(cc.AllowHosts) > 0 { - log.Printf("crawl: allow_hosts is set, so on-demand reading is limited to those hosts") - } + return hosts +} + +// crawlerWithHosts builds a crawler over one allowlist. Two callers, two lists: +// see newCrawler and onDemandCrawler. +func crawlerWithHosts(cc *config.CrawlConfig, hosts []string) *crawl.Crawler { ua := cc.UserAgent if ua == "" { ua = webfetch.DefaultUserAgent @@ -81,7 +98,14 @@ func onDemandCrawler(cfg *config.Config) *crawl.Crawler { if cfg.Crawl == nil || !cfg.Crawl.OnDemand { return nil } - return newCrawler(cfg) + cc := cfg.Crawl + // His own allow_hosts, and nothing added behind his back. Empty means "any + // host that is not denied and not private", which is what on-demand reading + // of a URL he just said out loud has to mean. + if len(cc.AllowHosts) > 0 { + log.Printf("crawl: allow_hosts is set, so on-demand reading is limited to those %d host(s)", len(cc.AllowHosts)) + } + return crawlerWithHosts(cc, crawlHosts(cc, false)) } // crawlWorker — ticker + watcher for the scheduled half. @@ -137,9 +161,20 @@ func (w *crawlWorker) run(ctx context.Context) { // net/http out of the crawler package. type crawlFetcher struct{ f *webfetch.Fetcher } +// Get maps webfetch's sentinels onto crawl's. This adapter is the one place +// that imports both packages, so the mapping belongs here; the crawler used to +// match on three substrings of a message it could not see the definition of, +// and a reworded error would have quietly turned a blocked host into "there is +// no robots.txt here". func (a *crawlFetcher) Get(ctx context.Context, u string) (*crawl.Response, error) { resp, err := a.f.Get(ctx, u) if err != nil { + switch { + case errors.Is(err, webfetch.ErrBlocked), errors.Is(err, webfetch.ErrPrivate), errors.Is(err, webfetch.ErrScheme): + return nil, fmt.Errorf("%w: %v", crawl.ErrFetchRefused, err) + case errors.Is(err, webfetch.ErrStatus): + return nil, fmt.Errorf("%w: %v", crawl.ErrFetchStatus, err) + } return nil, err } return &crawl.Response{URL: resp.URL, ContentType: resp.ContentType, Body: resp.Body}, nil diff --git a/cmd/mavend/crawls_test.go b/cmd/mavend/crawls_test.go index 2910d5f..5055880 100644 --- a/cmd/mavend/crawls_test.go +++ b/cmd/mavend/crawls_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "net/http" "net/http/httptest" "strings" @@ -13,6 +14,7 @@ import ( "github.com/kami/maven/internal/phraser" "github.com/kami/maven/internal/router" "github.com/kami/maven/internal/voice" + "github.com/kami/maven/internal/webfetch" ) // The default config reads nothing. This is the whole "off unless configured" @@ -123,16 +125,13 @@ func TestQueryWebPassesWithoutAURL(t *testing.T) { } } -// Not configured is said out loud rather than falling through, so a small model -// never invents a page's contents from its URL. -func TestQueryWebSaysWhenNotConfigured(t *testing.T) { +// A daemon where page reading was never turned on — the default — answers the +// question the way it did before the capability existed. Claiming the turn to +// report a configuration status is for something that exists and failed. +func TestQueryWebPassesWhenNotConfigured(t *testing.T) { h := buildWebHandler(nil) - reply, ok := askWeb(h, "посмотри https://example.org/page") - if !ok { - t.Fatal("the web source did not claim a question with a URL") - } - if !strings.Contains(reply, "не настроено") { - t.Errorf("reply = %q, want the not-configured answer", reply) + if reply, ok := askWeb(h, "посмотри https://example.org/page"); ok { + t.Fatalf("an unconfigured crawler claimed the turn with %q", reply) } } @@ -184,3 +183,58 @@ func (robotsDenyFetcher) Get(_ context.Context, u string) (*crawl.Response, erro } return &crawl.Response{URL: u, ContentType: "text/html", Body: []byte("nope")}, nil } + +// TestCrawlHostsKeepsAWatchOutOfTheOnDemandAllowlist — the on-demand crawler +// used to be built over allow_hosts PLUS every watched host. webfetch reads a +// non-empty allow list as "these and nothing else", so one watch on a config +// with no allow_hosts at all turned unrestricted on-demand reading into +// "the watched site only", and every other URL he pasted came back as +// "не получилось прочитать страницу." with nothing in the log to explain it. +func TestCrawlHostsKeepsAWatchOutOfTheOnDemandAllowlist(t *testing.T) { + cc := &config.CrawlConfig{ + OnDemand: true, + Watches: []config.CrawlWatchConfig{{Name: "p", URL: "https://watched.example/p"}}, + } + if got := crawlHosts(cc, false); len(got) != 0 { + t.Errorf("on-demand allowlist = %v; a watch is not an allowlist entry, and an empty list is what means \"anything public\"", got) + } + if got := crawlHosts(cc, true); len(got) != 1 || got[0] != "watched.example" { + t.Errorf("watch allowlist = %v; want the watched host so a watch needs no hand-written entry", got) + } + + // With allow_hosts set, his list is what on-demand gets, unchanged. + cc.AllowHosts = []string{"wiki.example"} + on := crawlHosts(cc, false) + if len(on) != 1 || on[0] != "wiki.example" { + t.Errorf("on-demand allowlist = %v; want exactly his allow_hosts", on) + } + if got := crawlHosts(cc, true); len(got) != 2 { + t.Errorf("watch allowlist = %v; want his hosts plus the watched one", got) + } +} + +// TestCrawlFetcherReportsARefusalAsARefusal — internal/crawl cannot import +// webfetch, so it used to recognise a guard refusal by matching substrings of +// webfetch's message text. This adapter owns both packages and is where the +// translation belongs. +func TestCrawlFetcherReportsARefusalAsARefusal(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "boom", http.StatusBadGateway) + })) + defer srv.Close() + + blocked := &crawlFetcher{f: webfetch.New(webfetch.Config{AllowHosts: []string{"wiki.example"}})} + if _, err := blocked.Get(context.Background(), "https://other.example/a"); !errors.Is(err, crawl.ErrFetchRefused) { + t.Errorf("a host outside allow_hosts = %v; want crawl.ErrFetchRefused", err) + } + if _, err := blocked.Get(context.Background(), "file:///etc/passwd"); !errors.Is(err, crawl.ErrFetchRefused) { + t.Errorf("a non-http scheme = %v; want crawl.ErrFetchRefused", err) + } + + // A 5xx is a different thing: the server answered, badly. robots.txt over + // this must refuse the crawl rather than read it as "no rules". + open := &crawlFetcher{f: webfetch.New(webfetch.Config{AllowHosts: []string{"127.0.0.1"}, AllowPrivate: true})} + if _, err := open.Get(context.Background(), srv.URL+"/robots.txt"); !errors.Is(err, crawl.ErrFetchStatus) { + t.Errorf("a 502 = %v; want crawl.ErrFetchStatus", err) + } +} diff --git a/deploy/README.md b/deploy/README.md index a1d2cb1..fec729b 100644 --- a/deploy/README.md +++ b/deploy/README.md @@ -95,10 +95,17 @@ switched: a fallback and not a habit; - `watches` re-reads a fixed list on its interval and writes a note when the text changed. Like the feeds, it announces nothing; -- the answer path sits **last** in the query chain, behind his memory, his notes - and (once wired) the local Kiwix ZIMs. A local read costs nothing; +- the answer path sits behind his memory and his notes, and ahead of the model + answering from what it remembers. Kiwix is not wired into the chain yet. A + local read costs nothing, so anything local goes first; - `robots.txt` is fetched first and obeyed with no override; a `Disallow` is a - refusal she says out loud. `Crawl-delay` is honoured; + refusal she says out loud. `Crawl-delay` is waited out before the page is + fetched, and a delay longer than the turn fails the read instead of hanging + it. A `robots.txt` that answers 5xx refuses the crawl — a broken server is + not permission; +- `allow_hosts` limits on-demand reading to those hosts and nothing else. + Watched pages' hosts are reachable by the scheduled crawler whether listed or + not, but a watch does **not** widen what he may ask her to read; - same guarded fetcher as the feeds: allowlist/denylist, no private addresses, size cap, redirect cap, timeout, one request per host per second; - dedup state is the config fact `crawl:hash:`; diff --git a/internal/config/config.go b/internal/config/config.go index 717cff2..1b29fa0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -940,8 +940,13 @@ type CrawlConfig struct { Interval Duration `json:"interval,omitempty"` // AllowHosts — when set, the ONLY hosts the crawler may reach (subdomains - // included). Watched pages' own hosts are added automatically. Setting this - // is how "she may read the arch wiki and nothing else" is expressed. + // included). Setting this is how "she may read the arch wiki and nothing + // else" is expressed. + // + // A watched page's own host is reachable by the scheduled crawler whether + // or not it is listed here, because configuring a watch is already saying + // she may read it. That does NOT extend to on-demand reading: a watch is + // not an allowlist entry for pages he pastes. AllowHosts []string `json:"allow_hosts,omitempty"` // DenyHosts — never reachable, checked first. Private addresses do not need diff --git a/internal/crawl/crawl.go b/internal/crawl/crawl.go index 96bec10..a01937f 100644 --- a/internal/crawl/crawl.go +++ b/internal/crawl/crawl.go @@ -1,13 +1,21 @@ // Package crawl reads a web page: fetch, robots check, HTML to text. // -// It is the LAST place Maven looks for an answer, and that ordering is the whole -// design. "Never phones home" is deprecated, but what replaced it puts local -// sources first: the resident model, then his own memory, then the Kiwix ZIMs on -// the box (internal/kiwix), and only then the network. A local read costs -// nothing and leaks nothing; a fetch costs a round-trip and puts a URL in -// someone's access log. So this package exists to be the fallback, not the -// front door — see the querySources chain in cmd/mavend/actions_query.go for -// where it actually sits. +// It is the last LOCAL-FIRST step, and the ordering is the whole design. +// "Never phones home" is deprecated, but what replaced it puts local sources +// first. Where this actually sits in querySources (cmd/mavend/actions_query.go): +// after his memory and after his notes, and BEFORE the model answers from what +// it remembers. Not after the model, which is what this comment used to claim. +// +// That position is deliberate. A fetch only happens where he named a URL out +// loud, and a named URL is an instruction, not a guess; letting a 1.7B answer +// about a page it cannot read is how a small model invents contents. Kiwix +// (internal/kiwix) is not in the chain yet, so nothing here describes it. +// +// A page's text is written by whoever owns the page. It reaches PhraseQuery as +// context beside his question, and for a watch it becomes a note. It cannot +// reach a tool or an act — the query path executes nothing — but it can steer +// what she says, which is the same trust level as a mail body and lower than +// anything he said himself. // // What never leaves the box: his notes, his facts, the persona block, the // conversation history. Only the URL is requested and, for the on-demand path, @@ -27,6 +35,7 @@ import ( "fmt" "net/url" "strings" + "sync" "time" ) @@ -34,6 +43,18 @@ import ( var ( ErrRobots = errors.New("crawl: robots.txt disallows this path") ErrNotHTML = errors.New("crawl: response is not html or text") + + // ErrFetchRefused — the fetcher would not go: a denied host, a private + // address, a scheme that is not http(s). The adapter that owns both + // packages (cmd/mavend/crawls.go) maps webfetch's sentinels onto this one, + // so this package tells "off limits" from "no robots.txt here" without + // importing webfetch and without matching on message text. + ErrFetchRefused = errors.New("crawl: the fetcher refused this url") + + // ErrFetchStatus — the server answered, badly (5xx, and anything else + // non-2xx). Separate from ErrFetchRefused because robots treats them + // differently: a broken server is not permission to crawl. + ErrFetchStatus = errors.New("crawl: the server answered with an error status") ) // Fetcher is the guarded HTTP door (internal/webfetch adapted by the daemon). An @@ -69,8 +90,20 @@ type Crawler struct { fetch Fetcher cfg Config robots *robotsCache + + // mu guards last, the per-host time of the previous fetch. It is what makes + // Crawl-delay real: the fetcher's own limiter is a flat one request per + // host per second and knows nothing about what a site asked for. + mu sync.Mutex + last map[string]time.Time } +// robotsTimeout — the robots fetch gets its own, shorter deadline. It shares the +// caller's budget with the page fetch (30s for the on-demand path, against a 20s +// default fetch timeout each), so a slow robots.txt used to eat the page's half +// and he heard "не получилось прочитать страницу" about a site that was fine. +const robotsTimeout = 8 * time.Second + // New builds a crawler. Returns nil when there is no fetcher, which is how the // daemon expresses "crawling is off unless configured". func New(fetch Fetcher, cfg Config) *Crawler { @@ -89,7 +122,7 @@ func New(fetch Fetcher, cfg Config) *Crawler { if cfg.Now == nil { cfg.Now = time.Now } - return &Crawler{fetch: fetch, cfg: cfg, robots: newRobotsCache(cfg.RobotsTTL)} + return &Crawler{fetch: fetch, cfg: cfg, robots: newRobotsCache(cfg.RobotsTTL), last: map[string]time.Time{}} } // Page fetches rawURL and returns its text. It checks robots.txt first and @@ -99,14 +132,22 @@ func (c *Crawler) Page(ctx context.Context, rawURL string) (Page, error) { if err != nil { return Page{}, fmt.Errorf("crawl: bad url %q: %w", rawURL, err) } - ok, err := c.allowed(ctx, u) + rules, err := c.rulesFor(ctx, u) if err != nil { return Page{}, err } - if !ok { + path := u.EscapedPath() + if u.RawQuery != "" { + path += "?" + u.RawQuery + } + if !rules.Allowed(path) { return Page{}, fmt.Errorf("%w: %s", ErrRobots, u.Path) } + if err := c.waitCrawlDelay(ctx, u.Host, rules.Delay); err != nil { + return Page{}, err + } resp, err := c.fetch.Get(ctx, u.String()) + c.markFetched(u.Host) if err != nil { return Page{}, err } @@ -120,49 +161,88 @@ func (c *Crawler) Page(ctx context.Context, rawURL string) (Page, error) { return Extract(resp.URL, resp.Body, c.cfg.MaxRunes), nil } -// allowed consults robots.txt for u's host, reading it at most once per TTL. +// rulesFor consults robots.txt for u's host, reading it at most once per TTL. // -// A robots.txt that cannot be fetched (404, a timeout, a blocked host) means -// allow, per the standard. The one thing that is NOT fail-open is an explicit -// Disallow. -func (c *Crawler) allowed(ctx context.Context, u *url.URL) (bool, error) { +// A robots.txt that is not there (404) means allow, per the standard. What does +// NOT mean allow: a server that answered with an error. The standard asks for +// the opposite there, and "the site is broken, so crawl it" is the wrong way to +// resolve an unknown. +func (c *Crawler) rulesFor(ctx context.Context, u *url.URL) (Rules, error) { host := u.Host now := c.cfg.Now() rules, ok := c.robots.get(host, now) - if !ok { - robotsURL := u.Scheme + "://" + host + "/robots.txt" - resp, err := c.fetch.Get(ctx, robotsURL) - switch { - case err != nil: - // Note what is NOT swallowed: a refusal from the guarded fetcher. - // If webfetch says this host is denied or private, the page fetch - // would fail the same way, and reporting the real reason beats - // reporting a robots verdict we never got. - if isFatalFetchError(err) { - return false, err - } - rules = Rules{} - default: - rules = ParseRobots(string(resp.Body), c.cfg.UserAgent) - } - c.robots.put(host, rules, now) + if ok { + return rules, nil } - path := u.EscapedPath() - if u.RawQuery != "" { - path += "?" + u.RawQuery + robotsURL := u.Scheme + "://" + host + "/robots.txt" + rctx, cancel := context.WithTimeout(ctx, robotsTimeout) + defer cancel() + resp, err := c.fetch.Get(rctx, robotsURL) + c.markFetched(host) + switch { + case err == nil: + rules = ParseRobots(string(resp.Body), c.cfg.UserAgent) + case errors.Is(err, ErrFetchRefused): + // Not swallowed: if the fetcher says this host is denied or private, + // the page fetch would fail the same way, and the real reason beats a + // robots verdict we never got. + return Rules{}, err + case errors.Is(err, ErrFetchStatus) && isServerError(err): + return Rules{}, fmt.Errorf("%w: robots.txt at %s could not be read", ErrFetchStatus, host) + default: + rules = Rules{} } - return rules.Allowed(path), nil + c.robots.put(host, rules, now) + return rules, nil } -// isFatalFetchError — a fetch failure that means "this host is off limits" -// rather than "there is no robots.txt here". The sentinel set is webfetch's, but -// this package must not import it (the interface exists precisely so it does -// not), so the check is on the message. Ugly and honest: the alternative is a -// dependency inversion for two strings. -func isFatalFetchError(err error) bool { +// waitCrawlDelay honours a Crawl-delay the site asked for. The fetcher's flat +// one-per-host-per-second is the floor and cannot express "30 seconds"; without +// this, deploy/README's claim that Crawl-delay is honoured was false. +// +// It waits on ctx, so a delay longer than the caller's budget fails the read +// rather than blocking a turn. That is the honest outcome: a site that wants a +// minute between requests is not a site to answer a voice question from. +func (c *Crawler) waitCrawlDelay(ctx context.Context, host string, delay time.Duration) error { + if delay <= 0 { + return nil + } + c.mu.Lock() + last, ok := c.last[host] + c.mu.Unlock() + if !ok { + return nil + } + wait := delay - c.cfg.Now().Sub(last) + if wait <= 0 { + return nil + } + t := time.NewTimer(wait) + defer t.Stop() + select { + case <-ctx.Done(): + return fmt.Errorf("crawl: %s asks for %s between requests, longer than this read has: %w", host, delay, ctx.Err()) + case <-t.C: + return nil + } +} + +func (c *Crawler) markFetched(host string) { + c.mu.Lock() + c.last[host] = c.cfg.Now() + c.mu.Unlock() +} + +// isServerError — a 5xx rather than any other non-2xx. The adapter formats the +// status into the message, which is the only place it survives. +func isServerError(err error) bool { s := err.Error() - return strings.Contains(s, "not allowed") || strings.Contains(s, "private address") || - strings.Contains(s, "only http and https") + for _, code := range []string{" 50", " 51", " 52", " 53"} { + if strings.Contains(s, code) { + return true + } + } + return false } // Hash is the dedup key for a crawl result: the sha256 of the extracted text, diff --git a/internal/crawl/politeness_test.go b/internal/crawl/politeness_test.go new file mode 100644 index 0000000..3e19309 --- /dev/null +++ b/internal/crawl/politeness_test.go @@ -0,0 +1,115 @@ +package crawl + +import ( + "context" + "errors" + "fmt" + "testing" + "time" +) + +// timedFetcher records when each request was made, so a test can assert a wait +// actually happened rather than that a field was parsed. +type timedFetcher struct { + pages map[string]Response + errs map[string]error + at []time.Time + urls []string +} + +func (f *timedFetcher) Get(_ context.Context, u string) (*Response, error) { + f.at = append(f.at, time.Now()) + f.urls = append(f.urls, u) + if err, ok := f.errs[u]; ok { + return nil, err + } + r, ok := f.pages[u] + if !ok { + return nil, errors.New("http 404: no such page") + } + if r.URL == "" { + r.URL = u + } + if r.ContentType == "" { + r.ContentType = "text/html" + } + return &r, nil +} + +func TestPage_HonoursCrawlDelay(t *testing.T) { + // deploy/README says Crawl-delay is honoured. It was parsed into Rules and + // never read: the only pacing was the fetcher's flat one request per host + // per second, which cannot express what a site asked for. + const delay = 120 * time.Millisecond + f := &timedFetcher{pages: map[string]Response{ + "https://example.org/robots.txt": {Body: []byte(fmt.Sprintf("User-agent: *\nCrawl-delay: %.3f\n", delay.Seconds())), ContentType: "text/plain"}, + "https://example.org/a": {Body: []byte("a")}, + }} + c := New(f, Config{UserAgent: "Maven/1.0"}) + if _, err := c.Page(context.Background(), "https://example.org/a"); err != nil { + t.Fatal(err) + } + if len(f.at) != 2 { + t.Fatalf("requests = %v; want robots.txt then the page", f.urls) + } + if gap := f.at[1].Sub(f.at[0]); gap < delay { + t.Errorf("the page was fetched %s after robots.txt; the site asked for %s", gap, delay) + } +} + +func TestPage_ACrawlDelayLongerThanTheTurnFailsInsteadOfBlocking(t *testing.T) { + f := &timedFetcher{pages: map[string]Response{ + "https://example.org/robots.txt": {Body: []byte("User-agent: *\nCrawl-delay: 30\n"), ContentType: "text/plain"}, + "https://example.org/a": {Body: []byte("a")}, + }} + c := New(f, Config{UserAgent: "Maven/1.0"}) + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + if _, err := c.Page(ctx, "https://example.org/a"); err == nil { + t.Fatal("a 30-second Crawl-delay was ignored inside a turn that cannot wait that long") + } + if len(f.urls) != 1 { + t.Errorf("requests = %v; the page must not be fetched before the wait it refused", f.urls) + } +} + +func TestPage_ABrokenRobotsServerIsNotPermissionToCrawl(t *testing.T) { + // A 404 means unrestricted, per the standard. A 500 does not: the standard + // asks for the opposite, and "the site is broken, so read it" is the wrong + // way to resolve an unknown. + f := &timedFetcher{ + pages: map[string]Response{"https://example.org/a": {Body: []byte("a")}}, + errs: map[string]error{"https://example.org/robots.txt": fmt.Errorf("%w: 503", ErrFetchStatus)}, + } + c := New(f, Config{UserAgent: "Maven/1.0"}) + if _, err := c.Page(context.Background(), "https://example.org/a"); !errors.Is(err, ErrFetchStatus) { + t.Fatalf("Page over a 503 robots.txt = %v; want a refusal", err) + } + if len(f.urls) != 1 { + t.Errorf("requests = %v; the page was read anyway", f.urls) + } +} + +func TestPage_AFetcherRefusalIsReportedAsItself(t *testing.T) { + // The old check matched three substrings of webfetch's message from a + // package that cannot import webfetch. The sentinel is mapped by the + // adapter that owns both (cmd/mavend/crawls.go). + f := &timedFetcher{errs: map[string]error{ + "https://example.org/robots.txt": fmt.Errorf("%w: host is not allowed", ErrFetchRefused), + }} + c := New(f, Config{UserAgent: "Maven/1.0"}) + if _, err := c.Page(context.Background(), "https://example.org/a"); !errors.Is(err, ErrFetchRefused) { + t.Fatalf("Page = %v; want the fetcher's own refusal, not a robots verdict", err) + } +} + +func TestParseRobots_MostSpecificAgentWinsRegardlessOfOrder(t *testing.T) { + body := "User-agent: maven\nDisallow: /private\n\nUser-agent: mav\nDisallow: /\n" + r := ParseRobots(body, "maven/1.0") + if !r.Allowed("/public") { + t.Error("the shorter agent group won by file order; the longer prefix is the more specific match") + } + if r.Allowed("/private") { + t.Error("the group that names us was not applied") + } +} diff --git a/internal/crawl/robots.go b/internal/crawl/robots.go index 3774aa6..2849721 100644 --- a/internal/crawl/robots.go +++ b/internal/crawl/robots.go @@ -27,7 +27,8 @@ type Rules struct { disallow []string // Delay is Crawl-delay in seconds when the group named one, 0 otherwise. // The fetcher's own per-host rate limit is the floor; this can only make - // Maven slower, never faster. + // Maven slower, never faster. Enforced in Crawler.waitCrawlDelay — the + // fetcher's limiter is flat and cannot express what a site asked for. Delay time.Duration } @@ -104,7 +105,11 @@ func ParseRobots(body string, agent string) Rules { } } + // Most specific wins, and specificity is the LENGTH of the matching agent + // string, not the file order. Two groups naming "mav" and "maven" used to be + // resolved by whichever came last in the file. var star, exact *group + best := 0 for i := range groups { for _, a := range groups[i].agents { if a == "*" && star == nil { @@ -112,8 +117,8 @@ func ParseRobots(body string, agent string) Rules { } // A robots.txt names "maven", we send "Maven/1.0 (…)": match on // prefix, which is how every crawler reads this field. - if a != "*" && a != "" && strings.HasPrefix(agent, a) { - exact = &groups[i] + if a != "*" && a != "" && strings.HasPrefix(agent, a) && len(a) > best { + best, exact = len(a), &groups[i] } } }