diff --git a/cmd/mavend/crawls.go b/cmd/mavend/crawls.go index a2d8a7c..afc314f 100644 --- a/cmd/mavend/crawls.go +++ b/cmd/mavend/crawls.go @@ -173,6 +173,13 @@ func (a *crawlFetcher) Get(ctx context.Context, u string) (*crawl.Response, erro 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): + // Carry the code across the seam. The crawler needs to tell a 5xx + // from a 404 to decide what a failed robots.txt means, and it must + // not learn that by reading this sentence. + var se *webfetch.StatusError + if errors.As(err, &se) { + return nil, &crawl.StatusError{Code: se.Code} + } return nil, fmt.Errorf("%w: %v", crawl.ErrFetchStatus, err) } return nil, err diff --git a/internal/crawl/crawl.go b/internal/crawl/crawl.go index a01937f..b30e4c9 100644 --- a/internal/crawl/crawl.go +++ b/internal/crawl/crawl.go @@ -57,6 +57,16 @@ var ( ErrFetchStatus = errors.New("crawl: the server answered with an error status") ) +// StatusError is ErrFetchStatus with the code the server actually sent. The +// adapter builds it; isServerError reads Code rather than the message, so a +// reworded error can no longer turn a 503 robots.txt into permission to crawl. +type StatusError struct{ Code int } + +func (e *StatusError) Error() string { + return fmt.Sprintf("crawl: the server answered with status %d", e.Code) +} +func (e *StatusError) Unwrap() error { return ErrFetchStatus } + // Fetcher is the guarded HTTP door (internal/webfetch adapted by the daemon). An // interface so this package constructs no http.Client of its own and can be // tested without a network. @@ -233,16 +243,15 @@ func (c *Crawler) markFetched(host string) { 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. +// isServerError — a 5xx rather than any other non-2xx. A status the adapter +// could not recover reads as 0 and is not a server error, which keeps the +// standard's "404 means allow" as the default for an unknown. func isServerError(err error) bool { - s := err.Error() - for _, code := range []string{" 50", " 51", " 52", " 53"} { - if strings.Contains(s, code) { - return true - } + var se *StatusError + if !errors.As(err, &se) { + return false } - return false + return se.Code >= 500 && se.Code <= 599 } // 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 index 3e19309..f2e0971 100644 --- a/internal/crawl/politeness_test.go +++ b/internal/crawl/politeness_test.go @@ -79,7 +79,7 @@ func TestPage_ABrokenRobotsServerIsNotPermissionToCrawl(t *testing.T) { // 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)}, + errs: map[string]error{"https://example.org/robots.txt": &StatusError{Code: 503}}, } c := New(f, Config{UserAgent: "Maven/1.0"}) if _, err := c.Page(context.Background(), "https://example.org/a"); !errors.Is(err, ErrFetchStatus) { diff --git a/internal/rss/feed.go b/internal/rss/feed.go index 06edfa3..d3b68e7 100644 --- a/internal/rss/feed.go +++ b/internal/rss/feed.go @@ -173,8 +173,9 @@ var ( ) // PlainText strips markup and decodes entities — feed summaries are HTML, and -// what reaches a note (and possibly the TTS) must be text. Exported because the -// crawler's extractor needs exactly this on a bigger input. +// what reaches a note (and possibly the TTS) must be text. Exported so a caller +// holding raw feed markup can reduce it the same way; crawl/extract.go does the +// bigger job on a whole document and does not go through here. func PlainText(s string) string { s = scriptRE.ReplaceAllString(s, " ") s = tagRE.ReplaceAllString(s, " ") diff --git a/internal/rss/poller.go b/internal/rss/poller.go index 24e3049..353619c 100644 --- a/internal/rss/poller.go +++ b/internal/rss/poller.go @@ -1,6 +1,7 @@ package rss import ( + "bytes" "context" "fmt" "log" @@ -164,7 +165,9 @@ func (p *Poller) PollFeed(ctx context.Context, f FeedConfig, now time.Time) (int if err != nil { return 0, err } - feed, err := Parse(strings.NewReader(string(body.Bytes))) + // bytes.NewReader and not strings.NewReader(string(…)): the latter copied a + // feed document that can run to a megabyte, for nothing. + feed, err := Parse(bytes.NewReader(body.Bytes)) if err != nil { return 0, err } diff --git a/internal/webfetch/webfetch.go b/internal/webfetch/webfetch.go index 5f58050..aea5284 100644 --- a/internal/webfetch/webfetch.go +++ b/internal/webfetch/webfetch.go @@ -61,6 +61,14 @@ var ( ErrStatus = errors.New("webfetch: non-2xx status") ) +// StatusError is a non-2xx reply, carrying the code. It unwraps to ErrStatus, +// so errors.Is keeps working, and it exists so a caller can tell a 404 from a +// 503 with errors.As instead of grepping the message for digits. +type StatusError struct{ Code int } + +func (e *StatusError) Error() string { return fmt.Sprintf("webfetch: non-2xx status: %d", e.Code) } +func (e *StatusError) Unwrap() error { return ErrStatus } + // Config are the limits. Every zero value means "the default above", so // Config{} is safe; the only field that changes behaviour by being empty is // AllowHosts (empty ⇒ any public host that is not denied). @@ -232,6 +240,13 @@ func (f *Fetcher) do(ctx context.Context, method, rawURL string, body []byte, hd } defer resp.Body.Close() + // Status first, body second. A server that answered 500 has no body worth + // reading, and reading it anyway cost up to MaxBytes off the wire and + // reported an oversized error page as ErrTooLarge, which names the wrong + // cause. The body is closed either way by the defer above. + if resp.StatusCode < 200 || resp.StatusCode > 299 { + return nil, &StatusError{Code: resp.StatusCode} + } respBody, err := io.ReadAll(io.LimitReader(resp.Body, f.cfg.MaxBytes+1)) if err != nil { return nil, err @@ -239,9 +254,6 @@ func (f *Fetcher) do(ctx context.Context, method, rawURL string, body []byte, hd if int64(len(respBody)) > f.cfg.MaxBytes { return nil, fmt.Errorf("%w (%d bytes)", ErrTooLarge, f.cfg.MaxBytes) } - if resp.StatusCode < 200 || resp.StatusCode > 299 { - return nil, fmt.Errorf("%w: %d", ErrStatus, resp.StatusCode) - } out := &Response{ URL: resp.Request.URL.String(), Status: resp.StatusCode, @@ -307,8 +319,9 @@ func (f *Fetcher) waitTurn(ctx context.Context, host string) error { } // HostMatches reports whether host equals one of pats or is a subdomain of one. -// Exported because the crawler applies the same rule to links it decides not to -// follow, before it ever builds a request. +// Exported so anything that wants to apply the same allow/deny rule to a link +// before building a request reads it from here rather than reimplementing it. +// Nothing outside this package calls it today. func HostMatches(host string, pats []string) bool { host = strings.ToLower(strings.TrimSuffix(host, ".")) for _, p := range pats { diff --git a/internal/webfetch/webfetch_test.go b/internal/webfetch/webfetch_test.go index fafdabb..3eb8d3a 100644 --- a/internal/webfetch/webfetch_test.go +++ b/internal/webfetch/webfetch_test.go @@ -1,6 +1,7 @@ package webfetch import ( + "bytes" "context" "errors" "io" @@ -212,6 +213,26 @@ func TestNon2xxIsAnError(t *testing.T) { } } +func TestNon2xxCarriesTheCodeAndBeatsTheSizeCap(t *testing.T) { + // A big error page used to be read in full and reported as ErrTooLarge, + // which names the size and hides the 503. The status is checked first now, + // and the code survives for a caller that has to tell 5xx from 404. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + w.Write(bytes.Repeat([]byte("x"), 5000)) + })) + defer srv.Close() + f := testFetcher(t, Config{MaxBytes: 100}) + _, err := f.Get(context.Background(), srv.URL) + if !errors.Is(err, ErrStatus) || errors.Is(err, ErrTooLarge) { + t.Fatalf("error = %v, want ErrStatus and not ErrTooLarge", err) + } + var se *StatusError + if !errors.As(err, &se) || se.Code != http.StatusServiceUnavailable { + t.Fatalf("error = %v, want a StatusError carrying 503", err) + } +} + func TestUserAgentIsSent(t *testing.T) { got := make(chan string, 1) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {