-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(auth): replace raw Cookie header with httpx cookie jar to fix #273 #276
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: main
Are you sure you want to change the base?
Changes from all commits
2990ba2
3d90567
31b8011
8ecb9c9
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 | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -51,6 +51,7 @@ | |||||||||||||||||||||||||||||||
| ".google.com", | ||||||||||||||||||||||||||||||||
| "notebooklm.google.com", | ||||||||||||||||||||||||||||||||
| ".googleusercontent.com", | ||||||||||||||||||||||||||||||||
| "accounts.google.com", # Required for token refresh redirects | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Regional Google ccTLDs where Google may set auth cookies | ||||||||||||||||||||||||||||||||
|
|
@@ -642,11 +643,88 @@ def load_httpx_cookies(path: Path | None = None) -> "httpx.Cookies": | |||||||||||||||||||||||||||||||
| return cookies | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def extract_cookies_with_domains( | ||||||||||||||||||||||||||||||||
| storage_state: dict[str, Any], | ||||||||||||||||||||||||||||||||
| ) -> dict[tuple[str, str], str]: | ||||||||||||||||||||||||||||||||
| """Extract Google cookies from storage state preserving original domains. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Unlike extract_cookies_from_storage() which returns a simple dict of | ||||||||||||||||||||||||||||||||
| name->value, this function returns a dict of (name, domain)->value tuples | ||||||||||||||||||||||||||||||||
| to preserve the original cookie domains. This is required for building | ||||||||||||||||||||||||||||||||
| proper httpx.Cookies jars that handle cross-domain redirects correctly. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||
| storage_state: Parsed JSON from Playwright's storage state file. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||
| Dict mapping (cookie_name, domain) tuples to values. | ||||||||||||||||||||||||||||||||
| Example: {("SID", ".google.com"): "abc123", ("HSID", ".google.com"): "def456"} | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Raises: | ||||||||||||||||||||||||||||||||
| ValueError: If required cookies (SID) are missing from storage state. | ||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||
| cookie_map: dict[tuple[str, str], str] = {} | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| for cookie in storage_state.get("cookies", []): | ||||||||||||||||||||||||||||||||
| domain = cookie.get("domain", "") | ||||||||||||||||||||||||||||||||
| name = cookie.get("name") | ||||||||||||||||||||||||||||||||
| value = cookie.get("value", "") | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if not _is_allowed_auth_domain(domain) or not name or not value: | ||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| key = (name, domain) | ||||||||||||||||||||||||||||||||
| # Prefer .google.com over regional domains | ||||||||||||||||||||||||||||||||
| if key not in cookie_map: | ||||||||||||||||||||||||||||||||
| cookie_map[key] = value | ||||||||||||||||||||||||||||||||
| elif domain == ".google.com": | ||||||||||||||||||||||||||||||||
| # .google.com takes precedence | ||||||||||||||||||||||||||||||||
| cookie_map[key] = value | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Validate required cookies exist (any domain) | ||||||||||||||||||||||||||||||||
| cookie_names = {name for name, _ in cookie_map} | ||||||||||||||||||||||||||||||||
| missing = MINIMUM_REQUIRED_COOKIES - cookie_names | ||||||||||||||||||||||||||||||||
| if missing: | ||||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||||
| f"Missing required cookies: {missing}\nRun 'notebooklm login' to authenticate." | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return cookie_map | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def build_httpx_cookies_from_storage(path: Path | None = None) -> "httpx.Cookies": | ||||||||||||||||||||||||||||||||
| """Build an httpx.Cookies jar with original domains preserved. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| This function loads cookies from storage and creates a proper httpx.Cookies | ||||||||||||||||||||||||||||||||
| jar with the original domains intact. This is critical for cross-domain | ||||||||||||||||||||||||||||||||
| redirects (e.g., to accounts.google.com for token refresh) to work correctly. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||
| path: Path to storage_state.json. If provided, takes precedence over env vars. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||
| httpx.Cookies jar with all cookies set to their original domains. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Raises: | ||||||||||||||||||||||||||||||||
| FileNotFoundError: If storage file doesn't exist. | ||||||||||||||||||||||||||||||||
| ValueError: If required cookies are missing or JSON is malformed. | ||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||
| storage_state = _load_storage_state(path) | ||||||||||||||||||||||||||||||||
| cookie_map = extract_cookies_with_domains(storage_state) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| cookies = httpx.Cookies() | ||||||||||||||||||||||||||||||||
| for (name, domain), value in cookie_map.items(): | ||||||||||||||||||||||||||||||||
| cookies.set(name, value, domain=domain) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return cookies | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| async def fetch_tokens(cookies: dict[str, str]) -> tuple[str, str]: | ||||||||||||||||||||||||||||||||
| """Fetch CSRF token and session ID from NotebookLM homepage. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Makes an authenticated request to NotebookLM and extracts the required | ||||||||||||||||||||||||||||||||
| tokens from the page HTML. | ||||||||||||||||||||||||||||||||
| tokens from the page HTML. Uses httpx.Cookies() jar to properly handle | ||||||||||||||||||||||||||||||||
| cross-domain redirects (e.g., to accounts.google.com for token refresh). | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||
| cookies: Dict of Google auth cookies | ||||||||||||||||||||||||||||||||
|
|
@@ -659,12 +737,16 @@ async def fetch_tokens(cookies: dict[str, str]) -> tuple[str, str]: | |||||||||||||||||||||||||||||||
| ValueError: If tokens cannot be extracted from response | ||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||
| logger.debug("Fetching CSRF and session tokens from NotebookLM") | ||||||||||||||||||||||||||||||||
| cookie_header = "; ".join(f"{k}={v}" for k, v in cookies.items()) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| async with httpx.AsyncClient() as client: | ||||||||||||||||||||||||||||||||
| # Build httpx.Cookies jar instead of raw header to properly handle | ||||||||||||||||||||||||||||||||
| # cross-domain redirects (e.g., to accounts.google.com for refresh) | ||||||||||||||||||||||||||||||||
| cookie_jar = httpx.Cookies() | ||||||||||||||||||||||||||||||||
| for name, value in cookies.items(): | ||||||||||||||||||||||||||||||||
| cookie_jar.set(name, value, domain=".google.com") | ||||||||||||||||||||||||||||||||
|
Comment on lines
+743
to
+745
Contributor
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. This logic for building a cookie jar is duplicated in ClientCore._build_cookies_jar. It should be centralized (e.g., as a method on AuthTokens) to ensure consistent behavior. Additionally, as noted in the core client review, forcing the .google.com domain for all cookies may break functionality for other domains like googleusercontent.com which are used for media downloads.
Comment on lines
+741
to
+745
|
||||||||||||||||||||||||||||||||
| # Build httpx.Cookies jar instead of raw header to properly handle | |
| # cross-domain redirects (e.g., to accounts.google.com for refresh) | |
| cookie_jar = httpx.Cookies() | |
| for name, value in cookies.items(): | |
| cookie_jar.set(name, value, domain=".google.com") | |
| # Build an httpx.Cookies jar instead of a raw Cookie header so cookies | |
| # participate in redirect handling. The input is a flattened name/value | |
| # mapping, so domain specificity has already been lost by this point. | |
| # To preserve current behavior while allowing redirects through the Google | |
| # Accounts host, register each cookie for both the general Google domain | |
| # and the concrete accounts.google.com host. | |
| cookie_jar = httpx.Cookies() | |
| for name, value in cookies.items(): | |
| cookie_jar.set(name, value, domain=".google.com") | |
| cookie_jar.set(name, value, domain="accounts.google.com") |
Copilot
AI
Apr 13, 2026
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.
The new redirect-handling behavior in fetch_tokens() is not directly asserted in tests. There are existing pytest_httpx patterns in this repo to inspect outgoing requests; adding an assertion that the redirected request to accounts.google.com still includes a Cookie header (and/or that the request chain completes when cookies are valid) would prevent regressions of #273.
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.
Forcing the ".google.com" domain on all cookies is a problematic side effect of using a flat dictionary (self.auth.cookies) to populate the cookie jar. While this ensures cookies are sent to accounts.google.com during redirects, it also means that cookies originally intended for other domains—such as .googleusercontent.com (which is used for media downloads and is explicitly allowed in auth.py)—will now be sent to .google.com and not to their intended destination. This will likely break authenticated downloads from Google's content servers. Additionally, ensure that when making requests to external URLs using these cookies, the domain is validated against an allowlist of trusted domains to prevent credential leakage.
References