From 44bd01d51cc0d2008a73e67845922a23d7aee344 Mon Sep 17 00:00:00 2001 From: Elias Dorneles Date: Thu, 10 Sep 2015 16:19:56 -0300 Subject: [PATCH 1/6] preparatory refactor in redirect mware --- scrapy/downloadermiddlewares/redirect.py | 31 +++++++++++------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index 3cf8d2bee..c14340b27 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -55,28 +55,25 @@ class RedirectMiddleware(BaseRedirectMiddleware): def process_response(self, request, response, spider): if (request.meta.get('dont_redirect', False) or - response.status in getattr(spider, 'handle_httpstatus_list', []) or - response.status in request.meta.get('handle_httpstatus_list', []) or - request.meta.get('handle_httpstatus_all', False)): + response.status in getattr(spider, 'handle_httpstatus_list', []) or + response.status in request.meta.get('handle_httpstatus_list', []) or + request.meta.get('handle_httpstatus_all', False)): return response - if request.method == 'HEAD': - if response.status in [301, 302, 303, 307] and 'Location' in response.headers: - redirected_url = urljoin(request.url, response.headers['location']) + location = None + if 'Location' in response.headers: + location = response.headers['location'] + + if location is not None and response.status in [301, 302, 303, 307]: + redirected_url = urljoin(request.url, location) + + if response.status in [301, 307] or request.method == 'HEAD': redirected = request.replace(url=redirected_url) return self._redirect(redirected, request, spider, response.status) - else: - return response - if response.status in [302, 303] and 'Location' in response.headers: - redirected_url = urljoin(request.url, response.headers['location']) - redirected = self._redirect_request_using_get(request, redirected_url) - return self._redirect(redirected, request, spider, response.status) - - if response.status in [301, 307] and 'Location' in response.headers: - redirected_url = urljoin(request.url, response.headers['location']) - redirected = request.replace(url=redirected_url) - return self._redirect(redirected, request, spider, response.status) + if response.status in [302, 303]: + redirected = self._redirect_request_using_get(request, redirected_url) + return self._redirect(redirected, request, spider, response.status) return response From defa89913559b543cf7336653e44350481c1c38a Mon Sep 17 00:00:00 2001 From: Elias Dorneles Date: Thu, 10 Sep 2015 16:31:59 -0300 Subject: [PATCH 2/6] PY3 port redirect middleware --- scrapy/downloadermiddlewares/redirect.py | 2 +- tests/py3-ignores.txt | 1 - tests/test_downloadermiddleware_redirect.py | 19 +++++++++++-------- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index c14340b27..ceb0a55a3 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -62,7 +62,7 @@ class RedirectMiddleware(BaseRedirectMiddleware): location = None if 'Location' in response.headers: - location = response.headers['location'] + location = response.headers['location'].decode('latin1') if location is not None and response.status in [301, 302, 303, 307]: redirected_url = urljoin(request.url, location) diff --git a/tests/py3-ignores.txt b/tests/py3-ignores.txt index 95e2181e3..e40b4c73c 100644 --- a/tests/py3-ignores.txt +++ b/tests/py3-ignores.txt @@ -10,7 +10,6 @@ tests/test_downloadermiddleware_httpcache.py tests/test_downloadermiddleware_httpcompression.py tests/test_downloadermiddleware_httpproxy.py tests/test_downloadermiddleware.py -tests/test_downloadermiddleware_redirect.py tests/test_downloadermiddleware_retry.py tests/test_engine.py tests/test_mail.py diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index b3db7c42b..15e1dff37 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -164,13 +164,13 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_priority_adjust(self): req = Request('http://a.com') - rsp = HtmlResponse(req.url, body=self._body()) + rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') req2 = self.mw.process_response(req, rsp, self.spider) assert req2.priority > req.priority def test_meta_refresh(self): req = Request(url='http://example.org') - rsp = HtmlResponse(req.url, body=self._body()) + rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') req2 = self.mw.process_response(req, rsp, self.spider) assert isinstance(req2, Request) self.assertEqual(req2.url, 'http://example.org/newpage') @@ -178,14 +178,16 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_meta_refresh_with_high_interval(self): # meta-refresh with high intervals don't trigger redirects req = Request(url='http://example.org') - rsp = HtmlResponse(url='http://example.org', body=self._body(interval=1000)) + rsp = HtmlResponse(url='http://example.org', + body=self._body(interval=1000), + encoding='utf-8') rsp2 = self.mw.process_response(req, rsp, self.spider) assert rsp is rsp2 def test_meta_refresh_trough_posted_request(self): req = Request(url='http://example.org', method='POST', body='test', headers={'Content-Type': 'text/plain', 'Content-length': '4'}) - rsp = HtmlResponse(req.url, body=self._body()) + rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') req2 = self.mw.process_response(req, rsp, self.spider) assert isinstance(req2, Request) @@ -201,7 +203,7 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_max_redirect_times(self): self.mw.max_redirect_times = 1 req = Request('http://scrapytest.org/max') - rsp = HtmlResponse(req.url, body=self._body()) + rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') req = self.mw.process_response(req, rsp, self.spider) assert isinstance(req, Request) @@ -212,7 +214,7 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_ttl(self): self.mw.max_redirect_times = 100 req = Request('http://scrapytest.org/302', meta={'redirect_ttl': 1}) - rsp = HtmlResponse(req.url, body=self._body()) + rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') req = self.mw.process_response(req, rsp, self.spider) assert isinstance(req, Request) @@ -220,10 +222,10 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_redirect_urls(self): req1 = Request('http://scrapytest.org/first') - rsp1 = HtmlResponse(req1.url, body=self._body(url='/redirected')) + rsp1 = HtmlResponse(req1.url, body=self._body(url='/redirected'), encoding='utf-8') req2 = self.mw.process_response(req1, rsp1, self.spider) assert isinstance(req2, Request), req2 - rsp2 = HtmlResponse(req2.url, body=self._body(url='/redirected2')) + rsp2 = HtmlResponse(req2.url, body=self._body(url='/redirected2'), encoding='utf-8') req3 = self.mw.process_response(req2, rsp2, self.spider) assert isinstance(req3, Request), req3 self.assertEqual(req2.url, 'http://scrapytest.org/redirected') @@ -231,5 +233,6 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): self.assertEqual(req3.url, 'http://scrapytest.org/redirected2') self.assertEqual(req3.meta['redirect_urls'], ['http://scrapytest.org/first', 'http://scrapytest.org/redirected']) + if __name__ == "__main__": unittest.main() From 179c4588ca12d363ab8ce9d2badad6c456d353a9 Mon Sep 17 00:00:00 2001 From: Elias Dorneles Date: Thu, 10 Sep 2015 19:36:47 -0300 Subject: [PATCH 3/6] adding test for latin1 location --- tests/test_downloadermiddleware_redirect.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index 15e1dff37..11d9fd152 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -1,3 +1,5 @@ +# -*- coding: utf-8 -*- + import unittest from scrapy.downloadermiddlewares.redirect import RedirectMiddleware, MetaRefreshMiddleware @@ -150,6 +152,14 @@ class RedirectMiddlewareTest(unittest.TestCase): [404, 301, 302]})) _test_passthrough(Request(url, meta={'handle_httpstatus_all': True})) + def test_latin1_location(self): + req = Request('http://scrapytest.org/first') + latin1_path = u'/ação'.encode('latin1') + resp = Response('http://scrapytest.org/first', headers={'Location': latin1_path}, status=302) + req_result = self.mw.process_response(req, resp, self.spider) + perc_encoded_utf8_url = 'http://scrapytest.org/a%C3%A7%C3%A3o' + self.assertEquals(perc_encoded_utf8_url, req_result.url) + class MetaRefreshMiddlewareTest(unittest.TestCase): From 81950f773d588000ef2bbe720caa0c2fc4d3f0df Mon Sep 17 00:00:00 2001 From: Elias Dorneles Date: Fri, 11 Sep 2015 18:51:48 -0300 Subject: [PATCH 4/6] made encoding conversion more explicit, added test for header with utf-8 encoding replicating what browsers do --- scrapy/downloadermiddlewares/redirect.py | 4 +++- tests/test_downloadermiddleware_redirect.py | 12 ++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index ceb0a55a3..3a6da85e8 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -3,6 +3,7 @@ from six.moves.urllib.parse import urljoin from scrapy.http import HtmlResponse from scrapy.utils.response import get_meta_refresh +from scrapy.utils.python import to_native_str from scrapy.exceptions import IgnoreRequest, NotConfigured logger = logging.getLogger(__name__) @@ -62,7 +63,8 @@ class RedirectMiddleware(BaseRedirectMiddleware): location = None if 'Location' in response.headers: - location = response.headers['location'].decode('latin1') + # HTTP header is ascii or latin1, redirected url will be percent-encoded utf-8 + location = to_native_str(response.headers['location'].decode('latin1')) if location is not None and response.status in [301, 302, 303, 307]: redirected_url = urljoin(request.url, location) diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index 11d9fd152..1f4c2d67a 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -154,12 +154,20 @@ class RedirectMiddlewareTest(unittest.TestCase): def test_latin1_location(self): req = Request('http://scrapytest.org/first') - latin1_path = u'/ação'.encode('latin1') - resp = Response('http://scrapytest.org/first', headers={'Location': latin1_path}, status=302) + latin1_location = u'/ação'.encode('latin1') # HTTP historically supports latin1 + resp = Response('http://scrapytest.org/first', headers={'Location': latin1_location}, status=302) req_result = self.mw.process_response(req, resp, self.spider) perc_encoded_utf8_url = 'http://scrapytest.org/a%C3%A7%C3%A3o' self.assertEquals(perc_encoded_utf8_url, req_result.url) + def test_location_with_wrong_encoding(self): + req = Request('http://scrapytest.org/first') + utf8_location = u'/ação' # header with wrong encoding (utf-8) + resp = Response('http://scrapytest.org/first', headers={'Location': utf8_location}, status=302) + req_result = self.mw.process_response(req, resp, self.spider) + perc_encoded_utf8_url = 'http://scrapytest.org/a%C3%83%C2%A7%C3%83%C2%A3o' + self.assertEquals(perc_encoded_utf8_url, req_result.url) + class MetaRefreshMiddlewareTest(unittest.TestCase): From b06d0706e6644187b4c4a6f7ac80e8adbf376907 Mon Sep 17 00:00:00 2001 From: Elias Dorneles Date: Sun, 13 Sep 2015 11:49:59 -0300 Subject: [PATCH 5/6] refactoring redirect logic --- scrapy/downloadermiddlewares/redirect.py | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index 3a6da85e8..4ed7e4c24 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -61,23 +61,21 @@ class RedirectMiddleware(BaseRedirectMiddleware): request.meta.get('handle_httpstatus_all', False)): return response - location = None - if 'Location' in response.headers: - # HTTP header is ascii or latin1, redirected url will be percent-encoded utf-8 - location = to_native_str(response.headers['location'].decode('latin1')) + allowed_status = (301, 302, 303, 307) + if 'Location' not in response.headers or response.status not in allowed_status: + return response - if location is not None and response.status in [301, 302, 303, 307]: - redirected_url = urljoin(request.url, location) + # HTTP header is ascii or latin1, redirected url will be percent-encoded utf-8 + location = to_native_str(response.headers['location'].decode('latin1')) - if response.status in [301, 307] or request.method == 'HEAD': - redirected = request.replace(url=redirected_url) - return self._redirect(redirected, request, spider, response.status) + redirected_url = urljoin(request.url, location) - if response.status in [302, 303]: - redirected = self._redirect_request_using_get(request, redirected_url) - return self._redirect(redirected, request, spider, response.status) + if response.status in (301, 307) or request.method == 'HEAD': + redirected = request.replace(url=redirected_url) + return self._redirect(redirected, request, spider, response.status) - return response + redirected = self._redirect_request_using_get(request, redirected_url) + return self._redirect(redirected, request, spider, response.status) class MetaRefreshMiddleware(BaseRedirectMiddleware): From 430e90f4b209af51deec0a85d214dfa3171eaf0e Mon Sep 17 00:00:00 2001 From: Elias Dorneles Date: Sun, 13 Sep 2015 12:29:41 -0300 Subject: [PATCH 6/6] minor refactor on metarefresh redirect mware test --- tests/test_downloadermiddleware_redirect.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index 1f4c2d67a..9db073cc5 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -177,18 +177,18 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): self.mw = MetaRefreshMiddleware.from_crawler(crawler) def _body(self, interval=5, url='http://example.org/newpage'): - return """"""\ - .format(interval, url) + html = u"""""" + return html.format(interval, url).encode('utf-8') def test_priority_adjust(self): req = Request('http://a.com') - rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') + rsp = HtmlResponse(req.url, body=self._body()) req2 = self.mw.process_response(req, rsp, self.spider) assert req2.priority > req.priority def test_meta_refresh(self): req = Request(url='http://example.org') - rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') + rsp = HtmlResponse(req.url, body=self._body()) req2 = self.mw.process_response(req, rsp, self.spider) assert isinstance(req2, Request) self.assertEqual(req2.url, 'http://example.org/newpage') @@ -205,7 +205,7 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_meta_refresh_trough_posted_request(self): req = Request(url='http://example.org', method='POST', body='test', headers={'Content-Type': 'text/plain', 'Content-length': '4'}) - rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') + rsp = HtmlResponse(req.url, body=self._body()) req2 = self.mw.process_response(req, rsp, self.spider) assert isinstance(req2, Request) @@ -221,7 +221,7 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_max_redirect_times(self): self.mw.max_redirect_times = 1 req = Request('http://scrapytest.org/max') - rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') + rsp = HtmlResponse(req.url, body=self._body()) req = self.mw.process_response(req, rsp, self.spider) assert isinstance(req, Request) @@ -232,7 +232,7 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_ttl(self): self.mw.max_redirect_times = 100 req = Request('http://scrapytest.org/302', meta={'redirect_ttl': 1}) - rsp = HtmlResponse(req.url, body=self._body(), encoding='utf-8') + rsp = HtmlResponse(req.url, body=self._body()) req = self.mw.process_response(req, rsp, self.spider) assert isinstance(req, Request) @@ -240,10 +240,10 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): def test_redirect_urls(self): req1 = Request('http://scrapytest.org/first') - rsp1 = HtmlResponse(req1.url, body=self._body(url='/redirected'), encoding='utf-8') + rsp1 = HtmlResponse(req1.url, body=self._body(url='/redirected')) req2 = self.mw.process_response(req1, rsp1, self.spider) assert isinstance(req2, Request), req2 - rsp2 = HtmlResponse(req2.url, body=self._body(url='/redirected2'), encoding='utf-8') + rsp2 = HtmlResponse(req2.url, body=self._body(url='/redirected2')) req3 = self.mw.process_response(req2, rsp2, self.spider) assert isinstance(req3, Request), req3 self.assertEqual(req2.url, 'http://scrapytest.org/redirected')