Merge 6122180869 into ad43bf0c56

scrapy/downloadermiddlewares/redirect.py

@@ -106,6 +106,23 @@ class BaseRedirectMiddleware:
                 reason,
             ]
             redirected.dont_filter = request.dont_filter
+            if not redirected.dont_filter and self._is_redirect_chain_duplicate(
+                redirected, request
+            ):
+                # The dupe filter marks a request's fingerprint as seen as
+                # soon as it is scheduled, before its response (and any
+                # resulting redirect) is known. If this redirect chain loops
+                # back to a URL that fingerprints the same as an earlier
+                # request in this *same* chain -- a literal self-redirect, a
+                # redirect whose query string is just reordered (which the
+                # fingerprinter canonicalizes away), or a longer
+                # A -> B -> C -> A style loop -- the redirected request would
+                # otherwise be treated as an already-seen duplicate and
+                # silently dropped instead of followed. Bypass the filter for
+                # that specific case only, so normal cross-request dedup
+                # (unrelated pages redirecting to the same canonical URL)
+                # still works as before.
+                redirected.dont_filter = True
             redirected.priority = request.priority + self.priority_adjust
             logger.debug(
                 "Redirecting (%(reason)s) to %(redirected)s from %(request)s",
@@ -120,6 +137,24 @@ class BaseRedirectMiddleware:
         )
         raise IgnoreRequest("max redirections reached")
 
+    def _is_redirect_chain_duplicate(
+        self, redirected: Request, request: Request
+    ) -> bool:
+        """Whether *redirected* fingerprints the same as *request* or as any
+        earlier request in the same redirect chain as *request*.
+
+        The set of fingerprints accumulated so far in the chain is carried
+        forward in ``request.meta["redirect_fingerprints"]``, bounded by
+        :setting:`REDIRECT_MAX_TIMES` since it grows by at most one entry per
+        hop.
+        """
+        assert self.crawler.request_fingerprinter
+        fingerprint = self.crawler.request_fingerprinter.fingerprint
+        seen_fingerprints = set(request.meta.get("redirect_fingerprints", ()))
+        seen_fingerprints.add(fingerprint(request))
+        redirected.meta["redirect_fingerprints"] = seen_fingerprints
+        return fingerprint(redirected) in seen_fingerprints
+
     def _build_redirect_request(
         self, source_request: Request, response: Response, *, url: str, **kwargs: Any
     ) -> Request:

tests/test_downloadermiddleware_redirect.py

@@ -4,6 +4,7 @@ from unittest.mock import MagicMock
 import pytest
 
 from scrapy.downloadermiddlewares.redirect import RedirectMiddleware
+from scrapy.dupefilters import RFPDupeFilter
 from scrapy.exceptions import NotConfigured
 from scrapy.http import Request, Response
 from scrapy.spidermiddlewares.referer import (
@@ -296,6 +297,84 @@ class TestRedirectMiddleware(TestRedirectBase):
         assert req2.url == url3
         assert req2.method == "HEAD"
 
+    def test_self_redirect_bypasses_dupefilter(self):
+        # A redirect target that fingerprints identically to the request
+        # that produced it (e.g. a literal self-redirect, or a redirect
+        # whose query-string parameters are just reordered, which the
+        # fingerprinter canonicalizes away) must not be dropped by the
+        # dupe filter, since the filter already marked that fingerprint
+        # as seen when the source request was scheduled (see #1225).
+        url = "http://www.example.com/302?a=1&b=2"
+        redirected_url = "http://www.example.com/302?b=2&a=1"
+        req = Request(url)
+        rsp = Response(url, headers={"Location": redirected_url}, status=302)
+
+        req2 = self.mw.process_response(req, rsp)
+        assert isinstance(req2, Request)
+        assert req2.url == redirected_url
+        assert req2.dont_filter is True
+
+        fingerprint = self.mw.crawler.request_fingerprinter.fingerprint
+        assert fingerprint(req2) == fingerprint(req)
+
+        dupefilter = RFPDupeFilter.from_crawler(self.mw.crawler)
+        assert dupefilter.request_seen(req) is False
+        # Without the dont_filter bypass, the scheduler would treat req2 as
+        # an already-seen duplicate of req and silently drop it.
+        assert dupefilter.request_seen(req2) is True
+
+    def test_redirect_chain_loop_bypasses_dupefilter(self):
+        # A redirect chain that loops back to an *earlier* request in the
+        # same chain (A -> B -> C -> A) must also bypass the dupe filter for
+        # the final hop, even though that hop's immediate predecessor (C)
+        # fingerprints differently from A. A fix that only compares a
+        # redirect against its direct predecessor would miss this case.
+        dupefilter = RFPDupeFilter.from_crawler(self.mw.crawler)
+        fingerprint = self.mw.crawler.request_fingerprinter.fingerprint
+
+        req_a = Request("http://www.example.com/a")
+        assert dupefilter.request_seen(req_a) is False
+
+        rsp_a = Response(
+            req_a.url, headers={"Location": "http://www.example.com/b"}, status=302
+        )
+        req_b = self.mw.process_response(req_a, rsp_a)
+        assert isinstance(req_b, Request)
+        assert req_b.dont_filter is False
+        assert dupefilter.request_seen(req_b) is False
+
+        rsp_b = Response(
+            req_b.url, headers={"Location": "http://www.example.com/c"}, status=302
+        )
+        req_c = self.mw.process_response(req_b, rsp_b)
+        assert isinstance(req_c, Request)
+        assert req_c.dont_filter is False
+        assert dupefilter.request_seen(req_c) is False
+
+        rsp_c = Response(
+            req_c.url, headers={"Location": "http://www.example.com/a"}, status=302
+        )
+        req_a_again = self.mw.process_response(req_c, rsp_c)
+        assert isinstance(req_a_again, Request)
+        assert fingerprint(req_a_again) == fingerprint(req_a)
+        assert req_a_again.dont_filter is True
+        assert dupefilter.request_seen(req_a_again) is True
+
+    def test_cross_request_duplicate_redirect_still_filtered(self):
+        # The dupefilter bypass must stay narrowly scoped: a redirect to a
+        # URL that genuinely differs from every request in its own chain
+        # should not be exempted, so normal cross-request dedup (e.g. two
+        # different pages redirecting to the same canonical URL) keeps
+        # working as before.
+        url = "http://www.example.com/302"
+        redirected_url = "http://www.example.com/redirected"
+        req = Request(url)
+        rsp = Response(url, headers={"Location": redirected_url}, status=302)
+
+        req2 = self.mw.process_response(req, rsp)
+        assert isinstance(req2, Request)
+        assert req2.dont_filter is False
+
     def test_spider_handling(self):
         self.mw.crawler.spider.handle_httpstatus_list = [404, 301, 302]
         url = "http://www.example.com/301"