-
Notifications
You must be signed in to change notification settings - Fork 788
fix(httpx): honor session cookies across HTTP client request paths #2104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -284,6 +284,7 @@ def _build_request( | |
| method=method, | ||
| headers=dict(headers) if headers else None, | ||
| content=payload, | ||
| cookies=session.cookies.jar if session else None, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: with |
||
| extensions={'crawlee_session': session if self._persist_cookies_per_session else None}, | ||
| timeout=timeout or httpx.USE_CLIENT_DEFAULT, | ||
| ) | ||
|
|
@@ -333,15 +334,19 @@ def _get_client(self, proxy_url: str | None) -> httpx.AsyncClient: | |
| def _combine_headers(self, explicit_headers: HttpHeaders | None) -> HttpHeaders | None: | ||
| """Merge default headers with explicit headers for an HTTP request. | ||
|
|
||
| Generate a final set of request headers by combining default headers, a random User-Agent header, | ||
| and any explicitly provided headers. | ||
| Generate a final set of request headers by combining default headers from a single fingerprint | ||
| (Accept, Accept-Language, User-Agent) and any explicitly provided headers. Using one fingerprint | ||
| avoids mixing Accept headers from one browser profile with a User-Agent from another. | ||
| """ | ||
| common_headers = self._header_generator.get_common_headers() if self._header_generator else HttpHeaders() | ||
| user_agent_header = ( | ||
| self._header_generator.get_random_user_agent_header() if self._header_generator else HttpHeaders() | ||
| ) | ||
| if self._header_generator: | ||
| generated_headers = self._header_generator.get_specific_headers( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: this was the last production caller of |
||
| header_names={'Accept', 'Accept-Language', 'User-Agent'}, | ||
| ) | ||
| else: | ||
| generated_headers = HttpHeaders() | ||
|
|
||
| explicit_headers = explicit_headers or HttpHeaders() | ||
| headers = common_headers | user_agent_header | explicit_headers | ||
| headers = generated_headers | explicit_headers | ||
| return headers or None | ||
|
|
||
| @staticmethod | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |||||||||||||||||||||
| from contextlib import asynccontextmanager | ||||||||||||||||||||||
| from logging import getLogger | ||||||||||||||||||||||
| from typing import TYPE_CHECKING, Any, TypedDict | ||||||||||||||||||||||
| from urllib.request import Request as UrllibRequest | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from cachetools import LRUCache | ||||||||||||||||||||||
| from impit import AsyncClient, Browser, HTTPError, Response, TimeoutException, TransportError | ||||||||||||||||||||||
|
|
@@ -30,6 +31,9 @@ | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| logger = getLogger(__name__) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Cache key: (proxy_url, id(cookie_jar) or None) | ||||||||||||||||||||||
| _ClientCacheKey = tuple[str | None, int | None] | ||||||||||||||||||||||
|
Comment on lines
+34
to
+35
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class _ClientCacheEntry(TypedDict): | ||||||||||||||||||||||
| """Type definition for client cache entries.""" | ||||||||||||||||||||||
|
|
@@ -116,7 +120,42 @@ def __init__( | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| self._async_client_kwargs = async_client_kwargs | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| self._client_by_proxy_url = LRUCache[str | None, _ClientCacheEntry](maxsize=10) | ||||||||||||||||||||||
| self._client_cache = LRUCache[_ClientCacheKey, _ClientCacheEntry](maxsize=10) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _prepare_cookies_and_headers( | ||||||||||||||||||||||
| self, | ||||||||||||||||||||||
| *, | ||||||||||||||||||||||
| session: Session | None, | ||||||||||||||||||||||
| url: str, | ||||||||||||||||||||||
| headers: HttpHeaders | dict[str, str] | None, | ||||||||||||||||||||||
| ) -> tuple[CookieJar | None, HttpHeaders]: | ||||||||||||||||||||||
| """Resolve cookie jar / Cookie header based on `persist_cookies_per_session`. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| When persistence is enabled, attach the session jar to Impit so response cookies update it. | ||||||||||||||||||||||
| When persistence is disabled, send existing cookies via the `Cookie` header and keep the | ||||||||||||||||||||||
| shared client (no jar) so clients stay cached and reusable. | ||||||||||||||||||||||
|
Comment on lines
+132
to
+136
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: these lines stop at ~96-99 chars against the 120-col limit the rest of the file fills, and the codebase writes
Suggested change
|
||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| if isinstance(headers, dict) or headers is None: | ||||||||||||||||||||||
| headers = HttpHeaders(headers or {}) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if session is None: | ||||||||||||||||||||||
| return None, headers | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if self._persist_cookies_per_session: | ||||||||||||||||||||||
| return session.cookies.jar, headers | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if cookie_header := self._get_cookie_header(session.cookies.jar, url, headers): | ||||||||||||||||||||||
| headers = headers | HttpHeaders({'Cookie': cookie_header}) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return None, headers | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @staticmethod | ||||||||||||||||||||||
| def _get_cookie_header(jar: CookieJar, url: str, headers: HttpHeaders | None = None) -> str: | ||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the only caller always passes headers, so the default is dead.
Suggested change
|
||||||||||||||||||||||
| """Build a Cookie request header from a jar without attaching the jar to the client.""" | ||||||||||||||||||||||
| # UrllibRequest is only used to format Cookie headers via CookieJar; it never opens a connection. | ||||||||||||||||||||||
| request = UrllibRequest(url, headers=dict(headers) if headers else {}) # noqa: S310 | ||||||||||||||||||||||
| jar.add_cookie_header(request) | ||||||||||||||||||||||
| return request.get_header('Cookie', '') | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @override | ||||||||||||||||||||||
| async def crawl( | ||||||||||||||||||||||
|
|
@@ -128,14 +167,19 @@ async def crawl( | |||||||||||||||||||||
| statistics: Statistics | None = None, | ||||||||||||||||||||||
| timeout: timedelta | None = None, | ||||||||||||||||||||||
| ) -> HttpCrawlingResult: | ||||||||||||||||||||||
| client = self._get_client(proxy_info.url if proxy_info else None, session.cookies.jar if session else None) | ||||||||||||||||||||||
| cookie_jar, headers = self._prepare_cookies_and_headers( | ||||||||||||||||||||||
| session=session, | ||||||||||||||||||||||
| url=request.url, | ||||||||||||||||||||||
| headers=request.headers, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| client = self._get_client(proxy_info.url if proxy_info else None, cookie_jar) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| try: | ||||||||||||||||||||||
| response = await client.request( | ||||||||||||||||||||||
| url=request.url, | ||||||||||||||||||||||
| method=request.method, | ||||||||||||||||||||||
| content=request.payload, | ||||||||||||||||||||||
| headers=dict(request.headers) if request.headers else None, | ||||||||||||||||||||||
| headers=dict(headers) if headers else None, | ||||||||||||||||||||||
| timeout=timeout.total_seconds() if timeout else None, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| except TimeoutException as exc: | ||||||||||||||||||||||
|
|
@@ -166,10 +210,8 @@ async def send_request( | |||||||||||||||||||||
| ) -> HttpResponse: | ||||||||||||||||||||||
| validate_http_url(url) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if isinstance(headers, dict) or headers is None: | ||||||||||||||||||||||
| headers = HttpHeaders(headers or {}) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| client = self._get_client(proxy_info.url if proxy_info else None, session.cookies.jar if session else None) | ||||||||||||||||||||||
| cookie_jar, headers = self._prepare_cookies_and_headers(session=session, url=url, headers=headers) | ||||||||||||||||||||||
| client = self._get_client(proxy_info.url if proxy_info else None, cookie_jar) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| try: | ||||||||||||||||||||||
| response = await client.request( | ||||||||||||||||||||||
|
|
@@ -203,7 +245,8 @@ async def stream( | |||||||||||||||||||||
| ) -> AsyncGenerator[HttpResponse]: | ||||||||||||||||||||||
| validate_http_url(url) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| client = self._get_client(proxy_info.url if proxy_info else None, session.cookies.jar if session else None) | ||||||||||||||||||||||
| cookie_jar, headers = self._prepare_cookies_and_headers(session=session, url=url, headers=headers) | ||||||||||||||||||||||
| client = self._get_client(proxy_info.url if proxy_info else None, cookie_jar) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| try: | ||||||||||||||||||||||
| response = await client.request( | ||||||||||||||||||||||
|
|
@@ -222,18 +265,22 @@ async def stream( | |||||||||||||||||||||
| finally: | ||||||||||||||||||||||
| response.close() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @staticmethod | ||||||||||||||||||||||
| def _make_cache_key(proxy_url: str | None, cookie_jar: CookieJar | None) -> _ClientCacheKey: | ||||||||||||||||||||||
| return (proxy_url, id(cookie_jar) if cookie_jar is not None else None) | ||||||||||||||||||||||
|
Comment on lines
+269
to
+270
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: a static helper with a single call site that builds a two-element tuple -- inline it, or drop it entirely (see the comment on L34). |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _get_client(self, proxy_url: str | None, cookie_jar: CookieJar | None) -> AsyncClient: | ||||||||||||||||||||||
| """Retrieve or create an HTTP client for the given proxy URL. | ||||||||||||||||||||||
| """Retrieve or create an HTTP client for the given proxy URL and cookie jar. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| If a client for the specified proxy URL does not exist, create and store a new one. | ||||||||||||||||||||||
| Clients are cached by `(proxy_url, cookie_jar identity)` so sessions with different jars do not share | ||||||||||||||||||||||
| a client. When cookie persistence is disabled, cookies are sent via headers and `cookie_jar` is `None`, | ||||||||||||||||||||||
| so a shared client can be reused for the proxy. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| cached_data = self._client_by_proxy_url.get(proxy_url) | ||||||||||||||||||||||
| if cached_data: | ||||||||||||||||||||||
| client = cached_data['client'] | ||||||||||||||||||||||
| client_cookie_jar = cached_data['cookie_jar'] | ||||||||||||||||||||||
| if client_cookie_jar is cookie_jar: | ||||||||||||||||||||||
| # If the cookie jar matches, return the existing client. | ||||||||||||||||||||||
| return client | ||||||||||||||||||||||
| cache_key = self._make_cache_key(proxy_url, cookie_jar) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| cached_data = self._client_cache.get(cache_key) | ||||||||||||||||||||||
| if cached_data and cached_data['cookie_jar'] is cookie_jar: | ||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||||||||||||||||||||||
| return cached_data['client'] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Prepare a default kwargs for the new client. | ||||||||||||||||||||||
| kwargs: dict[str, Any] = { | ||||||||||||||||||||||
|
|
@@ -249,7 +296,7 @@ def _get_client(self, proxy_url: str | None, cookie_jar: CookieJar | None) -> As | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| client = AsyncClient(**kwargs, cookie_jar=cookie_jar) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| self._client_by_proxy_url[proxy_url] = _ClientCacheEntry(client=client, cookie_jar=cookie_jar) | ||||||||||||||||||||||
| self._client_cache[cache_key] = _ClientCacheEntry(client=client, cookie_jar=cookie_jar) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return client | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -270,4 +317,4 @@ def _is_proxy_error(error: HTTPError) -> bool: | |||||||||||||||||||||
| @override | ||||||||||||||||||||||
| async def cleanup(self) -> None: | ||||||||||||||||||||||
| """Clean up resources used by the HTTP client.""" | ||||||||||||||||||||||
| self._client_by_proxy_url.clear() | ||||||||||||||||||||||
| self._client_cache.clear() | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import asyncio | ||
| import importlib | ||
| import json | ||
| import os | ||
| import sys | ||
| from typing import TYPE_CHECKING | ||
|
|
@@ -14,6 +15,7 @@ | |
| from crawlee import Request | ||
| from crawlee.errors import ProxyError | ||
| from crawlee.http_clients import CurlImpersonateHttpClient, HttpClient, HttpxHttpClient, ImpitHttpClient | ||
| from crawlee.sessions import Session | ||
| from crawlee.statistics import Statistics | ||
| from tests.unit.server import generate_file_content | ||
| from tests.unit.server_endpoints import HELLO_WORLD | ||
|
|
@@ -323,3 +325,65 @@ def test_import_error_handled(optional_module_name: str, import_path: str) -> No | |
| sys.modules.pop(mod_name, None) | ||
| with pytest.raises(ImportError): | ||
| importlib.import_module(import_path) | ||
|
|
||
|
|
||
| async def test_send_request_sends_session_cookies(http_client: HttpClient, server_url: URL) -> None: | ||
| """`send_request` must attach existing session cookies (same as `crawl`).""" | ||
| session = Session() | ||
| session.cookies.set('auth', 'token-1', domain=server_url.host or '127.0.0.1', path='/') | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
|
|
||
| response = await http_client.send_request(str(server_url / 'cookies'), session=session) | ||
| body = json.loads(await response.read()) | ||
|
|
||
| assert body['cookies'] == {'auth': 'token-1'} | ||
|
|
||
|
|
||
| async def test_stream_sends_session_cookies(http_client: HttpClient, server_url: URL) -> None: | ||
| """`stream` must attach existing session cookies (same as `crawl`).""" | ||
| session = Session() | ||
| session.cookies.set('auth', 'token-2', domain=server_url.host or '127.0.0.1', path='/') | ||
|
|
||
| content = b'' | ||
| async with http_client.stream(str(server_url / 'cookies'), session=session) as response: | ||
| async for chunk in response.read_stream(): | ||
| content += chunk | ||
|
|
||
| assert json.loads(content)['cookies'] == {'auth': 'token-2'} | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| 'custom_http_client', | ||
| [ | ||
| pytest.param(CurlImpersonateHttpClient(persist_cookies_per_session=False), id='curl'), | ||
| pytest.param(HttpxHttpClient(persist_cookies_per_session=False), id='httpx'), | ||
| pytest.param(ImpitHttpClient(persist_cookies_per_session=False), id='impit'), | ||
| ], | ||
| indirect=['custom_http_client'], | ||
| ) | ||
| async def test_persist_cookies_per_session_false(custom_http_client: HttpClient, server_url: URL) -> None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: no test enters the new |
||
| """When persistence is disabled, response Set-Cookie must not update the session jar.""" | ||
| session = Session() | ||
| request = Request.from_url(str(server_url.with_path('set_cookies').extend_query(a=1))) | ||
|
|
||
| await custom_http_client.crawl(request, session=session) | ||
|
|
||
| assert {cookie['name']: cookie['value'] for cookie in session.cookies.get_cookies_as_dicts()} == {} | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| 'custom_http_client', | ||
| [ | ||
| pytest.param(CurlImpersonateHttpClient(persist_cookies_per_session=True), id='curl'), | ||
| pytest.param(HttpxHttpClient(persist_cookies_per_session=True), id='httpx'), | ||
| pytest.param(ImpitHttpClient(persist_cookies_per_session=True), id='impit'), | ||
| ], | ||
| indirect=['custom_http_client'], | ||
| ) | ||
| async def test_persist_cookies_per_session_true(custom_http_client: HttpClient, server_url: URL) -> None: | ||
| """When persistence is enabled, response Set-Cookie must update the session jar.""" | ||
| session = Session() | ||
| request = Request.from_url(str(server_url.with_path('set_cookies').extend_query(a=1))) | ||
|
|
||
| await custom_http_client.crawl(request, session=session) | ||
|
|
||
| assert {cookie['name']: cookie['value'] for cookie in session.cookies.get_cookies_as_dicts()} == {'a': '1'} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,12 @@ | |
| import json | ||
| import logging | ||
| from typing import TYPE_CHECKING | ||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
|
|
||
| from crawlee._types import HttpHeaders | ||
| from crawlee.fingerprint_suite import HeaderGenerator | ||
| from crawlee.fingerprint_suite._browserforge_adapter import get_available_header_values | ||
| from crawlee.fingerprint_suite._consts import COMMON_ACCEPT_LANGUAGE | ||
| from crawlee.http_clients import HttpxHttpClient | ||
|
|
@@ -54,3 +57,19 @@ async def test_common_headers_and_user_agent(server_url: URL, header_network: di | |
| assert 'user-agent' in response_headers | ||
| assert 'python-httpx' not in response_headers['user-agent'] | ||
| assert response_headers['user-agent'] in get_available_header_values(header_network, {'User-Agent', 'user-agent'}) | ||
|
|
||
|
|
||
| async def test_headers_come_from_single_fingerprint() -> None: | ||
| """Accept and User-Agent must come from the same generated fingerprint profile.""" | ||
| header_generator = HeaderGenerator() | ||
| fingerprint = {'Accept': 'text/html', 'Accept-Language': 'en-US', 'User-Agent': 'TestAgent/1.0'} | ||
|
|
||
| with patch.object(header_generator, 'get_specific_headers', return_value=HttpHeaders(fingerprint)) as mocked: | ||
| client = HttpxHttpClient(header_generator=header_generator) | ||
| combined = client._combine_headers(None) | ||
|
|
||
| mocked.assert_called_once_with(header_names={'Accept', 'Accept-Language', 'User-Agent'}) | ||
|
Comment on lines
+64
to
+71
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: this test cannot fail for the reason it exists. It patches |
||
| assert combined is not None | ||
| assert combined['accept'] == 'text/html' | ||
| assert combined['accept-language'] == 'en-US' | ||
| assert combined['user-agent'] == 'TestAgent/1.0' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| from crawlee.http_clients import ImpitHttpClient | ||
|
|
||
| if TYPE_CHECKING: | ||
| from yarl import URL | ||
|
|
||
|
|
||
| async def test_cleanup_clears_client_cache(server_url: URL) -> None: | ||
| """`ImpitHttpClient.cleanup` must drop cached clients so the next request creates a fresh one.""" | ||
| client = ImpitHttpClient() | ||
| async with client: | ||
| await client.send_request(str(server_url)) | ||
| assert len(client._client_cache) == 1 | ||
| first_client = next(iter(client._client_cache.values()))['client'] | ||
|
|
||
| await client.cleanup() | ||
| assert len(client._client_cache) == 0 | ||
|
|
||
| await client.send_request(str(server_url)) | ||
| assert len(client._client_cache) == 1 | ||
| second_client = next(iter(client._client_cache.values()))['client'] | ||
| assert second_client is not first_client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this fixes the direct-request case, but httpx still drops session cookies on redirect.
Client._redirect_headersends with an unconditionalheaders.pop("Cookie", None)and rebuilds cookies from the client jar, which crawlee deliberately keeps empty (_HttpxTransportdeletesSet-Cookie), so per-requestcookies=never survives a hop.Measured against a local
/start-> 302 ->/echowith this branch: impit (both persistence modes) and curl forwardauth=secret, httpx sends noCookieat all. Not a regression, but it is the same defect this PR targets and auth flows redirect constantly. Either set theCookieheader explicitly (reusing_get_cookie_header, as the impit path now does) or seed the client jar per request. At minimum, add a redirect case to the shared cookie tests -- the new tests hide this, sincetest_send_request_sends_session_cookieshits/cookiesdirectly andtest_persist_cookies_per_session_*only asserts jar contents.