-
Notifications
You must be signed in to change notification settings - Fork 790
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
91e9be4
dccf1e7
086b115
d4f5fae
6aaadc4
6b2c8c8
bf9a8d0
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |||||||||||||
| from contextlib import asynccontextmanager | ||||||||||||||
| from logging import DEBUG, WARNING, getLogger | ||||||||||||||
| from typing import TYPE_CHECKING, Any, cast | ||||||||||||||
| from urllib.request import Request as UrllibRequest | ||||||||||||||
|
|
||||||||||||||
| import httpx | ||||||||||||||
| from typing_extensions import override | ||||||||||||||
|
|
@@ -20,6 +21,7 @@ | |||||||||||||
| if TYPE_CHECKING: | ||||||||||||||
| from collections.abc import AsyncGenerator, AsyncIterator | ||||||||||||||
| from datetime import timedelta | ||||||||||||||
| from http.cookiejar import CookieJar | ||||||||||||||
| from ssl import SSLContext | ||||||||||||||
|
|
||||||||||||||
| from crawlee import Request | ||||||||||||||
|
|
@@ -63,26 +65,41 @@ async def read_stream(self) -> AsyncIterator[bytes]: | |||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class _HttpxTransport(httpx.AsyncHTTPTransport): | ||||||||||||||
| """HTTP transport adapter that stores response cookies in a `Session`. | ||||||||||||||
| """HTTP transport adapter that keeps session cookies off the shared `httpx` client. | ||||||||||||||
|
|
||||||||||||||
| This transport adapter modifies the handling of HTTP requests to update the session cookies | ||||||||||||||
| based on the response cookies, ensuring that the cookies are stored in the session object | ||||||||||||||
| rather than the `HTTPX` client itself. | ||||||||||||||
| Outbound cookies are applied per hop (including redirects) from the jar in request extensions, | ||||||||||||||
| because httpx strips the `Cookie` header on redirect and rebuilds it from the client jar. Response | ||||||||||||||
| `Set-Cookie` values are stored on the session and removed from the response so the shared client | ||||||||||||||
| jar stays empty and reusable across sessions. | ||||||||||||||
| """ | ||||||||||||||
|
|
||||||||||||||
| @override | ||||||||||||||
| async def handle_async_request(self, request: httpx.Request) -> httpx.Response: | ||||||||||||||
| if cookie_jar := cast('CookieJar | None', request.extensions.get('crawlee_cookie_jar')): | ||||||||||||||
| self._apply_cookie_header(request, cookie_jar) | ||||||||||||||
|
|
||||||||||||||
| response = await super().handle_async_request(request) | ||||||||||||||
| response.request = request | ||||||||||||||
|
|
||||||||||||||
| if session := cast('Session', request.extensions.get('crawlee_session')): | ||||||||||||||
| if session := cast('Session | None', request.extensions.get('crawlee_session')): | ||||||||||||||
| session.cookies.store_cookies(list(response.cookies.jar)) | ||||||||||||||
|
|
||||||||||||||
| if 'Set-Cookie' in response.headers: | ||||||||||||||
| del response.headers['Set-Cookie'] | ||||||||||||||
|
|
||||||||||||||
| return response | ||||||||||||||
|
|
||||||||||||||
| @staticmethod | ||||||||||||||
| def _apply_cookie_header(request: httpx.Request, jar: CookieJar) -> None: | ||||||||||||||
| """Set the Cookie header from a jar for the current request URL.""" | ||||||||||||||
| urllib_request = UrllibRequest(str(request.url), headers=dict(request.headers)) # noqa: S310 | ||||||||||||||
| jar.add_cookie_header(urllib_request) | ||||||||||||||
| cookie_header = urllib_request.get_header('Cookie') | ||||||||||||||
| if cookie_header: | ||||||||||||||
| request.headers['cookie'] = cookie_header | ||||||||||||||
| else: | ||||||||||||||
| request.headers.pop('cookie', None) | ||||||||||||||
|
Comment on lines
+98
to
+101
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
Suggested change
Comment on lines
+92
to
+101
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: this
|
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| @docs_group('HTTP clients') | ||||||||||||||
| class HttpxHttpClient(HttpClient): | ||||||||||||||
|
|
@@ -158,16 +175,15 @@ async def crawl( | |||||||||||||
| timeout: timedelta | None = None, | ||||||||||||||
| ) -> HttpCrawlingResult: | ||||||||||||||
| client = self._get_client(proxy_info.url if proxy_info else None) | ||||||||||||||
| headers = self._combine_headers(request.headers) | ||||||||||||||
|
|
||||||||||||||
| http_request = client.build_request( | ||||||||||||||
| http_request = self._build_request( | ||||||||||||||
| client=client, | ||||||||||||||
| url=request.url, | ||||||||||||||
| method=request.method, | ||||||||||||||
| headers=headers, | ||||||||||||||
| content=request.payload, | ||||||||||||||
| cookies=session.cookies.jar if session else None, | ||||||||||||||
| extensions={'crawlee_session': session if self._persist_cookies_per_session else None}, | ||||||||||||||
| timeout=timeout.total_seconds() if timeout is not None else httpx.USE_CLIENT_DEFAULT, | ||||||||||||||
| headers=request.headers, | ||||||||||||||
| payload=request.payload, | ||||||||||||||
| session=session, | ||||||||||||||
| timeout=httpx.Timeout(timeout.total_seconds()) if timeout is not None else None, | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| try: | ||||||||||||||
|
|
@@ -284,17 +300,22 @@ def _build_request( | |||||||||||||
| method=method, | ||||||||||||||
| headers=dict(headers) if headers else None, | ||||||||||||||
| content=payload, | ||||||||||||||
| extensions={'crawlee_session': session if self._persist_cookies_per_session else None}, | ||||||||||||||
| cookies=session.cookies.jar if session else None, | ||||||||||||||
|
vdusek marked this conversation as resolved.
vdusek marked this conversation as resolved.
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. With this new approach that uses transport, Also, the session is already being passed to the transport. Use the session's |
||||||||||||||
| extensions={ | ||||||||||||||
| # Used by the transport to re-apply cookies on every hop (httpx strips Cookie on redirect). | ||||||||||||||
| 'crawlee_cookie_jar': session.cookies.jar if session else None, | ||||||||||||||
| 'crawlee_session': session if self._persist_cookies_per_session else None, | ||||||||||||||
| }, | ||||||||||||||
|
Comment on lines
+304
to
+308
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: building on Mantisus' open thread here -- the transport can't just read the jar off Pass |
||||||||||||||
| timeout=timeout or httpx.USE_CLIENT_DEFAULT, | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| def _get_client(self, proxy_url: str | None) -> httpx.AsyncClient: | ||||||||||||||
| """Retrieve or create an HTTP client for the given proxy URL. | ||||||||||||||
|
|
||||||||||||||
| If a client for the specified proxy URL does not exist, create and store a new one. | ||||||||||||||
| Clients are shared per proxy (not per session). Session cookies stay on the request / | ||||||||||||||
| transport path so concurrent sessions can reuse one client and its connection pool. | ||||||||||||||
| """ | ||||||||||||||
| if not self._transport: | ||||||||||||||
| # Configure connection pool limits and keep-alive connections for transport | ||||||||||||||
| limits = self._async_client_kwargs.get( | ||||||||||||||
| 'limits', httpx.Limits(max_connections=1000, max_keepalive_connections=200) | ||||||||||||||
| ) | ||||||||||||||
|
|
@@ -307,15 +328,13 @@ def _get_client(self, proxy_url: str | None) -> httpx.AsyncClient: | |||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| if proxy_url not in self._client_by_proxy_url: | ||||||||||||||
| # Prepare a default kwargs for the new client. | ||||||||||||||
| kwargs: dict[str, Any] = { | ||||||||||||||
| 'proxy': proxy_url, | ||||||||||||||
| 'http1': self._http1, | ||||||||||||||
| 'http2': self._http2, | ||||||||||||||
| 'follow_redirects': True, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| # Update the default kwargs with any additional user-provided kwargs. | ||||||||||||||
| kwargs.update(self._async_client_kwargs) | ||||||||||||||
|
|
||||||||||||||
| kwargs.update( | ||||||||||||||
|
|
@@ -333,15 +352,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( | ||||||||||||||
|
vdusek marked this conversation as resolved.
|
||||||||||||||
| 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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,8 +2,10 @@ | |||||
|
|
||||||
| import asyncio | ||||||
| from contextlib import asynccontextmanager | ||||||
| from http.cookiejar import CookieJar | ||||||
| from logging import getLogger | ||||||
| from typing import TYPE_CHECKING, Any, TypedDict | ||||||
| from typing import TYPE_CHECKING, Any | ||||||
| from urllib.request import Request as UrllibRequest | ||||||
|
|
||||||
| from cachetools import LRUCache | ||||||
| from impit import AsyncClient, Browser, HTTPError, Response, TimeoutException, TransportError | ||||||
|
|
@@ -20,7 +22,6 @@ | |||||
| if TYPE_CHECKING: | ||||||
| from collections.abc import AsyncGenerator, AsyncIterator | ||||||
| from datetime import timedelta | ||||||
| from http.cookiejar import CookieJar | ||||||
|
|
||||||
| from crawlee import Request | ||||||
| from crawlee._types import HttpMethod, HttpPayload | ||||||
|
|
@@ -31,13 +32,6 @@ | |||||
| logger = getLogger(__name__) | ||||||
|
|
||||||
|
|
||||||
| class _ClientCacheEntry(TypedDict): | ||||||
| """Type definition for client cache entries.""" | ||||||
|
|
||||||
| client: AsyncClient | ||||||
| cookie_jar: CookieJar | None | ||||||
|
|
||||||
|
|
||||||
| class _ImpitResponse: | ||||||
| """Adapter class for `impit.Response` to conform to the `HttpResponse` protocol.""" | ||||||
|
|
||||||
|
|
@@ -116,7 +110,41 @@ def __init__( | |||||
|
|
||||||
| self._async_client_kwargs = async_client_kwargs | ||||||
|
|
||||||
| self._client_by_proxy_url = LRUCache[str | None, _ClientCacheEntry](maxsize=10) | ||||||
| self._client_cache = LRUCache[tuple[str | None, CookieJar | None], AsyncClient](maxsize=10) | ||||||
|
Comment on lines
-119
to
+113
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. Why rename it? All HTTP clients name this attribute |
||||||
|
|
||||||
| 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. | ||||||
| """ | ||||||
| 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) -> str: | ||||||
| """Build a Cookie request header from a jar without attaching the jar to the client.""" | ||||||
| request = UrllibRequest(url, headers=dict(headers)) # noqa: S310 | ||||||
| jar.add_cookie_header(request) | ||||||
| return request.get_header('Cookie', '') | ||||||
|
|
||||||
| @override | ||||||
| async def crawl( | ||||||
|
|
@@ -128,14 +156,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 +199,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 +234,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( | ||||||
|
|
@@ -223,19 +255,17 @@ async def stream( | |||||
| response.close() | ||||||
|
|
||||||
| 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)` — CookieJar hashes by identity so sessions with different | ||||||
|
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. Not em dashes please -
Suggested change
|
||||||
| jars get separate clients. When cookie persistence is disabled, `cookie_jar` is `None` and a shared client | ||||||
| is 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 | ||||||
|
|
||||||
| # Prepare a default kwargs for the new client. | ||||||
| cache_key = (proxy_url, cookie_jar) | ||||||
|
|
||||||
| if cache_key in self._client_cache: | ||||||
| return self._client_cache[cache_key] | ||||||
|
|
||||||
| kwargs: dict[str, Any] = { | ||||||
| 'proxy': proxy_url, | ||||||
| 'http3': self._http3, | ||||||
|
|
@@ -244,12 +274,10 @@ def _get_client(self, proxy_url: str | None, cookie_jar: CookieJar | None) -> As | |||||
| 'browser': self._browser, | ||||||
| } | ||||||
|
|
||||||
| # Update the default kwargs with any additional user-provided kwargs. | ||||||
| kwargs.update(self._async_client_kwargs) | ||||||
|
|
||||||
| 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] = client | ||||||
|
|
||||||
| return client | ||||||
|
|
||||||
|
|
@@ -270,4 +298,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() | ||||||
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: httpx is the only client that strips this -- impit (
_impit.py:50-51) and curl (_curl_impersonate.py:101-102) pass response headers through untouched. So the publicHttpResponse.headerscarriesset-cookieunder two clients and never under the third.Pre-existing, but this PR promotes the deletion from implementation detail to stated mechanism in the new docstring, and it interacts with the semantics just given to
persist_cookies_per_session=False: under httpx with persistence off, aSet-Cookieis now observable nowhere -- not in the session, not in the response -- while impit and curl users can still read it. The new cross-client cookie tests are the natural place to pin that divergence, or one line onHttpResponse.headersto document it.