Skip to content

fix: improve 2FA handling for China mainland accounts #222

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.

Sign up for GitHub

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

Open
Twsa wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: improve 2FA handling for China mainland accounts #222

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 82 additions & 8 deletions pyicloud/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,16 @@ def _authenticate_with_token(self) -> None:
if not self.session.data.get("session_token"):
raise PyiCloudFailedLoginException("No session token available")

# Auto-detect China mainland based on account country in session data
account_country = self.session.data.get("account_country", "")
if account_country == "CHN" and not self._is_china_mainland:
LOGGER.debug("Auto-switching to China mainland endpoints based on account_country")
self._is_china_mainland = True
self._setup_endpoints()
# Update session headers with new endpoint
self.session.headers["Origin"] = self._home_endpoint
self.session.headers["Referer"] = f"{self._home_endpoint}/"
Comment on lines +639 to +647
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all callers of _authenticate_with_token to confirm where it can be re-entered.
rg -nP -C2 '\b_authenticate_with_token\s*\('

Repository: timlaing/pyicloud

Length of output: 1215

🏁 Script executed:

#!/bin/bash
# Extract trust_session method to understand the flow and when line 1261 is reached
rg -nA 80 'def trust_session' pyicloud/base.py | head -150

Repository: timlaing/pyicloud

Length of output: 4464

🏁 Script executed:

#!/bin/bash
# Check PyiCloudSession for any cookie or header management
rg -n 'class PyiCloudSession' pyicloud/

Repository: timlaing/pyicloud

Length of output: 125

🏁 Script executed:

#!/bin/bash
# Get the full _authenticate_with_token method
rg -nA 50 'def _authenticate_with_token\(self\)' pyicloud/base.py | head -100

Repository: timlaing/pyicloud

Length of output: 2684

🏁 Script executed:

#!/bin/bash
# Get _get_auth_headers method
rg -nA 20 'def _get_auth_headers' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 985

🏁 Script executed:

#!/bin/bash
# Check if setup_endpoints modifies the session or cookies
rg -nA 15 'def _setup_endpoints' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 1084

🏁 Script executed:

#!/bin/bash
# Look for tests related to trust_session or multi-auth flows
fd -e py test | xargs grep -l 'trust_session\|_authenticate_with_token' 2>/dev/null | head -5

Repository: timlaing/pyicloud

Length of output: 164

🏁 Script executed:

#!/bin/bash
# Check if there are any cookie-related dependencies in the code
rg 'cookies|cookie_jar' pyicloud/ -i

Repository: timlaing/pyicloud

Length of output: 2308

🏁 Script executed:

#!/bin/bash
# Verify what the /accountLogin endpoint actually requires
rg -B5 -A10 'accountLogin' pyicloud/base.py | head -40

Repository: timlaing/pyicloud

Length of output: 1719

🏁 Script executed:

#!/bin/bash
# Check if there's any reference to what /accountLogin expects or returns
rg -B3 -A3 'X-APPLE-WEBAUTH-TOKEN' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 903

🏁 Script executed:

#!/bin/bash
# Look at the test files to see if they cover the 302 redirect or trust_session flow
rg -A20 'def test.*trust' tests/test_base.py | head -50

Repository: timlaing/pyicloud

Length of output: 1940

🏁 Script executed:

#!/bin/bash
# Check how requests.Session handles cookies across domain changes
rg -n 'allow_redirects' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 227

🏁 Script executed:

#!/bin/bash
# Look at the full requires_2sa and requires_2fa properties to verify redundancy claim
rg -B2 -A8 'def requires_2sa|def requires_2fa' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 705

🏁 Script executed:

#!/bin/bash
# Check the _requires_mfa usage to understand the redundancy claim
rg -n '_requires_mfa' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 353

🏁 Script executed:

#!/bin/bash
# Check the _is_mfa_required method implementation
rg -B2 -A10 'def _is_mfa_required' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 493

🏁 Script executed:

#!/bin/bash
# Check where _requires_mfa is set to True to understand its semantics
rg -B3 -A3 '_requires_mfa = True' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 337

🏁 Script executed:

#!/bin/bash
# Verify the order of operations in trust_session - when is _requires_mfa cleared
rg -B10 'self._requires_mfa = False' pyicloud/base.py | grep -A10 'trust_session'

Repository: timlaing/pyicloud

Length of output: 205

🏁 Script executed:

#!/bin/bash
# Check if PyiCloudSession has any automatic header extraction logic
rg -n 'X-Apple-Session-Token\|X-Apple-TwoSV-Trust-Token\|X-Apple-ID-Session-Id' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Look at the full flow around line 1226 to see if _requires_mfa should be cleared before or after trust attempt
rg -B5 -A50 'def trust_session' pyicloud/base.py | head -70

Repository: timlaing/pyicloud

Length of output: 2867

🏁 Script executed:

#!/bin/bash
# Check the response handling to see if 204 is truly guaranteed before line 1261 is reached
rg -n 'status_code.*204\|204.*status' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Check if there are tests for trust_session failure cases
rg -A30 'test_trust_session' tests/test_base.py

Repository: timlaing/pyicloud

Length of output: 2043

🏁 Script executed:

#!/bin/bash
# Verify if line 1226 _requires_mfa clear happens before the trust attempt
sed -n '1224,1280p' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 3088

China auto-switch mid-flow is safe for initial login, but verify second-entry cookie state.

The auto-detect logic at line 641 uses a guard (and not self._is_china_mainland) so it triggers only once per session. However, the 302 redirect path (lines 662–676) provides an alternative switch route, and neither path clears cookies that were already set against the international domain.

requests.Session scopes cookies by domain: any cookies sent to setup.icloud.com or www.icloud.com before the switch will not be sent to the .cn endpoint in subsequent requests. For a fresh login before any service instantiation, this is benign. However, when _authenticate_with_token is re-entered at line 1261 (within trust_session), the session may already carry cookies from the initial international auth attempt. Posting to .cn thereafter without those cookies could fail if Apple's endpoint expects them for session continuity, even though the initial POST to /accountLogin uses token-based auth.

Additionally, the 302 redirect path (line 672) retries the POST without refreshing the Origin/Referer headers, whereas the auto-detect branch does (lines 646–647). This inconsistency could trigger Apple's validation checks.

Recommend adding a test for the "second auth on the same session after China switch via 302 redirect" path to confirm the session state remains valid.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pyicloud/base.py` around lines 639 - 647, The China auto-switch path sets
_is_china_mainland and updates endpoints via _setup_endpoints and
session.headers["Origin"/"Referer"] but the alternate 302-redirect retry path
does not refresh headers or ensure cookies match the new .cn domain, which can
break re-entry into _authenticate_with_token (called from trust_session); fix
by, when switching to China (either via account_country auto-detect or the 302
redirect retry flow), explicitly clear or migrate cookies tied to the previous
international domain on self.session.cookies (or recreate the requests.Session)
and ensure session.headers["Origin"] and session.headers["Referer"] are updated
before retrying the POST; add a unit/integration test that simulates a first
auth attempt against the international domain then triggers the 302-driven China
switch and re-enters _authenticate_with_token to verify the session sends
correct cookies/headers to the .cn endpoints.


try:
login_data: dict[str, Any] = {
"accountCountryCode": self.session.data.get("account_country"),
Expand All @@ -647,6 +657,24 @@ def _authenticate_with_token(self) -> None:
resp: Response = self.session.post(
f"{self._setup_endpoint}/accountLogin", json=login_data
)

# Check if response indicates wrong domain (China account using international endpoint)
if resp.status_code == 302:
try:
body = resp.json()
if body.get("domainToUse"):
LOGGER.debug("Detected domain redirect to %s, switching endpoints", body["domainToUse"])
# Switch to China endpoint if not already
if not self._is_china_mainland and ".cn" in body["domainToUse"]:
self._is_china_mainland = True
self._setup_endpoints()
# Retry with correct endpoint
resp = self.session.post(
f"{self._setup_endpoint}/accountLogin", json=login_data
)
except (ValueError, KeyError):
pass # Not JSON or unexpected format

Comment on lines +660 to +677
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm whether PyiCloudSession.post disables redirects so a 302 is observable.
rg -nP -C5 '\b(def\s+(post|request)|allow_redirects)\b' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 1626

🏁 Script executed:

#!/bin/bash
# Get full post method definition to check redirect settings
rg -nA30 'def post\(' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Get the full context around lines 660-677 in base.py
sed -n '650,680p' pyicloud/base.py | cat -n

Repository: timlaing/pyicloud

Length of output: 1773

🏁 Script executed:

#!/bin/bash
# Check if session is configured to disable redirects globally
rg -n 'allow_redirects.*=.*False' pyicloud/

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Search for PyiCloudSession class to understand how it handles requests
rg -n 'class PyiCloudSession' pyicloud/

Repository: timlaing/pyicloud

Length of output: 125

🏁 Script executed:

#!/bin/bash
# Look at the actual post call in context - line 658
sed -n '655,665p' pyicloud/base.py | cat -n

Repository: timlaing/pyicloud

Length of output: 536

🏁 Script executed:

#!/bin/bash
# Check if allow_redirects is set in session.post call itself anywhere in base.py
rg -n 'allow_redirects' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Look at PyiCloudSession class initialization and configuration
sed -n '60,150p' pyicloud/session.py | cat -n

Repository: timlaing/pyicloud

Length of output: 3923

🏁 Script executed:

#!/bin/bash
# Search for hooks, redirects, or other session-level configuration in session.py
rg -n 'hooks|redirect' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 333

🏁 Script executed:

#!/bin/bash
# Check if there's a mocked response or test configuration affecting this
rg -n 'Response|status_code.*302' pyicloud/base.py | head -20

Repository: timlaing/pyicloud

Length of output: 1140

🏁 Script executed:

#!/bin/bash
# Check what hooks are configured in session.py
rg -n -B2 -A2 'hooks' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 684

🏁 Script executed:

#!/bin/bash
# Look at the request method implementation more carefully - around line 186-230
sed -n '186,230p' pyicloud/session.py | cat -n

Repository: timlaing/pyicloud

Length of output: 1400

🏁 Script executed:

#!/bin/bash
# Check if there's a response hook that might prevent redirect following
rg -n 'response.*hook\|def.*hook\|BeforeRedirect' pyicloud/

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Look for the _request method implementation that's called by request()
rg -n 'def _request' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 109

🏁 Script executed:

#!/bin/bash
# Check the entire session.py to see if allow_redirects is modified anywhere
wc -l pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 85

🏁 Script executed:

#!/bin/bash
# View the full session.py to understand the complete flow
cat pyicloud/session.py | head -300

Repository: timlaing/pyicloud

Length of output: 8592

🏁 Script executed:

#!/bin/bash
# Continue reading session.py from line 300 onwards
sed -n '300,453p' pyicloud/session.py | cat -n

Repository: timlaing/pyicloud

Length of output: 6714

🏁 Script executed:

#!/bin/bash
# Check the actual call to post() in base.py to see if allow_redirects is passed
sed -n '655,665p' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 459

🏁 Script executed:

#!/bin/bash
# Search for where post() might be called with allow_redirects=False
rg -n 'post\(' pyicloud/base.py | head -10

Repository: timlaing/pyicloud

Length of output: 582

🏁 Script executed:

#!/bin/bash
# Search for tests that might exercise this 302 handling code
find . -name '*test*.py' -type f | head -5

Repository: timlaing/pyicloud

Length of output: 213

🏁 Script executed:

#!/bin/bash
# Look for tests related to China endpoint or 302
rg -l '302|domainToUse|china' --type py | head -10

Repository: timlaing/pyicloud

Length of output: 298

🏁 Script executed:

#!/bin/bash
# Check if there are any mocks or fixtures that disable redirects
rg -n 'allow_redirects.*False|redirect' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 258

🏁 Script executed:

#!/bin/bash
# Look at conftest.py to understand test setup
head -100 tests/conftest.py

Repository: timlaing/pyicloud

Length of output: 3185

🏁 Script executed:

#!/bin/bash
# Search for any requests mocking or allow_redirects in tests
rg -n 'allow_redirects|302|domainToUse' tests/

Repository: timlaing/pyicloud

Length of output: 820

🏁 Script executed:

#!/bin/bash
# Check the test_hsa2_bridge.py for clues about how 302 responses are handled
rg -B5 -A10 '302|domainToUse' tests/test_hsa2_bridge.py

Repository: timlaing/pyicloud

Length of output: 3486

🏁 Script executed:

#!/bin/bash
# Look at test_base.py around line 983 to see what those allow_redirects=True tests are
sed -n '975,1000p' tests/test_base.py

Repository: timlaing/pyicloud

Length of output: 839

🏁 Script executed:

#!/bin/bash
# Search for tests related to accountLogin or domain switching
rg -B10 -A10 'accountLogin|domainToUse' tests/

Repository: timlaing/pyicloud

Length of output: 7228

🏁 Script executed:

#!/bin/bash
# Check if there are any mocks for requests.Session.post that might disable redirects
rg -n 'post.*allow_redirects|MagicMock.*post' tests/test_base.py | head -20

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Look for PyiCloudSessionMock to understand how tests handle this
rg -n 'class PyiCloudSessionMock' tests/

Repository: timlaing/pyicloud

Length of output: 131

🏁 Script executed:

#!/bin/bash
# Check tests/__init__.py to see how mock session handles POST
sed -n '1,100p' tests/__init__.py | cat -n

Repository: timlaing/pyicloud

Length of output: 3628

🏁 Script executed:

#!/bin/bash
# Look at the actual _handle_account_login implementation
rg -B5 -A20 '_handle_account_login' tests/__init__.py

Repository: timlaing/pyicloud

Length of output: 2160

🏁 Script executed:

#!/bin/bash
# Search for explicit post method override in PyiCloudSession
sed -n '60,453p' pyicloud/session.py | grep -n 'def post'

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Check if there's any code that explicitly calls allow_redirects=False
rg -n 'post.*allow_redirects.*False' pyicloud/

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Double-check: look at where accountLogin is called and see the exact arguments
sed -n '655,680p' pyicloud/base.py | cat -n

Repository: timlaing/pyicloud

Length of output: 1429

🏁 Script executed:

#!/bin/bash
# Look at the actual requests behavior - check if there's any hook or other mechanism
# Search more broadly for any session configuration
rg -n 'def __init__' pyicloud/session.py -A30

Repository: timlaing/pyicloud

Length of output: 1102

🏁 Script executed:

#!/bin/bash
# Check if requests.Session has any redirect-disabling hooks or if there's patching in tests
rg -n 'allow_redirects\|PreparedRequest\|request_hook' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Critical: Check git history or comments that might explain the 302 handling
rg -B10 'status_code == 302' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 537

🏁 Script executed:

#!/bin/bash
# Search for tests that specifically test the 302 handling
rg -B5 -A15 'status_code.*302\|domainToUse' tests/test_base.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Look for any test mocks that return 302
rg -n '302' tests/

Repository: timlaing/pyicloud

Length of output: 647

🏁 Script executed:

#!/bin/bash
# Check if requests.Session is patched in conftest to disable redirects
rg -n 'allow_redirects|redirects\|PreparedRequest' tests/conftest.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Check git log to see when this 302 handling was added
git log -p --all -S 'status_code == 302' -- pyicloud/base.py | head -100

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Search for any documentation or comments about why 302 is handled
rg -B20 'status_code == 302' pyicloud/base.py | head -50

Repository: timlaing/pyicloud

Length of output: 1041

🏁 Script executed:

#!/bin/bash
# Check if maybe this is testing code or if there's any reason this would work
# Look at what requests actually does - search for any custom request hooks
rg -n 'self.hooks\|hooks.*=' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Final check: look at the exact context around the auto-detect branch (lines 646-647)
# to confirm the header update pattern mentioned in the review
sed -n '640,680p' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 2077

Fix silent fall-through on 302 redirect and missing header update on retry.

Three substantive issues in this code block:

  1. Silent fall-through to garbage data: If the 302 response has no domainToUse field, _is_china_mainland is already True, body is not JSON-parseable, or the retry itself returns another 302, control falls through to line 679 where resp.raise_for_status() does not raise for 3xx status codes. The code then stores the redirect-hint payload (e.g. {"domainToUse": "..."}) as account login data, corrupting subsequent calls to _handle_accept_terms and _update_state.

  2. Retry omits header updates: Unlike the auto-detect branch above (lines 646–647), the retry POST does not refresh session.headers["Origin"] or session.headers["Referer"] after switching to the new endpoint. Apple's IDMS/setup endpoints typically validate these headers; the retry can be rejected even though the URL is now correct.

  3. Imprecise exception handling: except (ValueError, KeyError): pass silently discards errors. KeyError is unreachable (only .get() and guarded indexing used), and AttributeError (if response body is not a dict) is not caught. At minimum, log at debug level.

Suggested fix 
            resp: Response = self.session.post(
                f"{self._setup_endpoint}/accountLogin", json=login_data
            )

            # Check if response indicates wrong domain (China account using international endpoint)
            if resp.status_code == 302:
+                domain_to_use: Optional[str] = None
                 try:
                     body = resp.json()
-                    if body.get("domainToUse"):
-                        LOGGER.debug("Detected domain redirect to %s", body["domainToUse"])
-                        # Switch to China endpoint if not already
-                        if not self._is_china_mainland and ".cn" in body["domainToUse"]:
-                            self._is_china_mainland = True
-                            self._setup_endpoints()
-                            # Retry with correct endpoint
-                            resp = self.session.post(
-                                f"{self._setup_endpoint}/accountLogin", json=login_data
-                            )
-                except (ValueError, KeyError):
-                    pass  # Not JSON or unexpected format
+                    if isinstance(body, dict):
+                        domain_to_use = body.get("domainToUse")
+                except ValueError:
+                    LOGGER.debug("accountLogin 302 body was not JSON", exc_info=True)
+
+                if domain_to_use and ".cn" in domain_to_use and not self._is_china_mainland:
+                    LOGGER.debug("Switching to China endpoints based on domainToUse=%s", domain_to_use)
+                    self._is_china_mainland = True
+                    self._setup_endpoints()
+                    self.session.headers["Origin"] = self._home_endpoint
+                    self.session.headers["Referer"] = f"{self._home_endpoint}/"
+                    resp = self.session.post(
+                        f"{self._setup_endpoint}/accountLogin", json=login_data
+                    )
+
+                if resp.status_code == 302:
+                    raise PyiCloudFailedLoginException(
+                        f"accountLogin returned an unhandled 302 redirect (domainToUse={domain_to_use!r})"
+                    )

             resp.raise_for_status()
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pyicloud/base.py` around lines 660 - 677, The 302-handling block can silently
fall through and corrupt login state and also retries without updating headers;
update the logic in the block handling resp.status_code == 302 so that if
resp.json() does not contain a usable domainToUse or parsing fails you
explicitly raise or return instead of continuing, and when you successfully
switch endpoints (set self._is_china_mainland and call self._setup_endpoints())
refresh session.headers["Origin"] and session.headers["Referer"] before
reissuing the POST (the same headers update done in the auto-detect branch),
then reassign resp from the retry and ensure you handle another non-2xx/3xx by
calling resp.raise_for_status() only after confirming it's not a redirect;
replace the broad silent except (ValueError, KeyError): pass with logging at
debug (catch ValueError and AttributeError) so malformed or non-dict bodies are
recorded rather than ignored.

resp.raise_for_status()

self.data = resp.json()
Expand Down Expand Up @@ -743,15 +771,21 @@ def requires_2sa(self) -> bool:
"""Returns True if two-step authentication is required."""
return (
self._is_mfa_required()
and self.data.get("dsInfo", {}).get("hsaVersion", 0) >= 1
and (
self.data.get("dsInfo", {}).get("hsaVersion", 0) >= 1
or self._requires_mfa
Copy link
Copy Markdown
Owner

@timlaing timlaing May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is performed in the _is_mfa_required method and as such not needed here

)
)

@property
def requires_2fa(self) -> bool:
"""Returns True if two-factor authentication is required."""
return (
self._is_mfa_required()
and self.data.get("dsInfo", {}).get("hsaVersion", 0) == 2
and (
self.data.get("dsInfo", {}).get("hsaVersion", 0) == 2
or self._requires_mfa
Copy link
Copy Markdown
Owner

@timlaing timlaing May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is performed in the _is_mfa_required method and as such not needed here

)
)

@property
Expand Down Expand Up @@ -1194,15 +1228,55 @@ def trust_session(self) -> bool:
headers: dict[str, Any] = self._get_auth_headers()

try:
self.session.get(
response = self.session.get(
f"{self._auth_endpoint}/2sv/trust",
headers=headers,
)
self._authenticate_with_token()
LOGGER.debug("Session trust successful.")
return True
except (PyiCloudAPIResponseException, PyiCloud2FARequiredException):
LOGGER.error("Session trust failed.")
LOGGER.debug("trust_session response status: %s", response.status_code)
LOGGER.debug("trust_session response headers: %s", dict(response.headers))
Comment on lines +1235 to +1236
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't log full response headers — they contain session/trust tokens.

response.headers for /2sv/trust includes X-Apple-Session-Token, X-Apple-TwoSV-Trust-Token, X-Apple-ID-Session-Id, and scnt. These are bearer credentials for the account; dumping them at DEBUG means anyone with access to the log file (or a user pasting their debug log into a bug report) is effectively handing over the account. The status code is fine; the headers are not.

🛡️ Suggested fix 
-            LOGGER.debug("trust_session response status: %s", response.status_code)
-            LOGGER.debug("trust_session response headers: %s", dict(response.headers))
+            LOGGER.debug("trust_session response status: %s", response.status_code)
+            # Avoid logging response.headers verbatim: they carry session/trust tokens.
+            LOGGER.debug(
+                "trust_session response header keys: %s",
+                sorted(response.headers.keys()),
+            )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
LOGGER.debug("trust_session response status: %s", response.status_code)
LOGGER.debug("trust_session response headers: %s", dict(response.headers))
LOGGER.debug("trust_session response status: %s", response.status_code)
# Avoid logging response.headers verbatim: they carry session/trust tokens.
LOGGER.debug(
"trust_session response header keys: %s",
sorted(response.headers.keys()),
)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pyicloud/base.py` around lines 1235 - 1236, The debug log currently prints
full response headers in pyicloud/base.py (the LOGGER.debug call that logs
dict(response.headers)), which exposes sensitive session/trust tokens; change
this so only non-sensitive information is logged — e.g., keep the existing
LOGGER.debug("trust_session response status: %s", response.status_code) and
remove or replace the LOGGER.debug that prints dict(response.headers) with
either no header logging or a filtered/redacted version that strips keys like
"X-Apple-Session-Token", "X-Apple-TwoSV-Trust-Token", "X-Apple-ID-Session-Id",
and "scnt" before logging; locate the logging lines around the trust_session
handling and update the LOGGER.debug that references response.headers
accordingly.


# GET /2sv/trust returning 204 means trust was successfully established
if response.status_code == 204:
LOGGER.debug("Session trust established (server returned 204)")
# Extract and update session tokens from response headers
new_session_token = response.headers.get('X-Apple-Session-Token')
new_scnt = response.headers.get('scnt')
new_session_id = response.headers.get('X-Apple-ID-Session-Id')
trust_token = response.headers.get('X-Apple-TwoSV-Trust-Token')

if new_session_token:
self.session.data['session_token'] = new_session_token
if new_scnt:
self.session.data['scnt'] = new_scnt
if new_session_id:
self.session.data['session_id'] = new_session_id
if trust_token:
self.session.data['trust_token'] = trust_token
Comment on lines +1242 to +1254
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look at how PyiCloudSession captures auth headers from responses.
rg -nP -C3 'X-Apple-Session-Token|X-Apple-TwoSV-Trust-Token|X-Apple-ID-Session-Id|\bscnt\b' pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 43

🏁 Script executed:

# First, find the PyiCloudSession class definition
rg -n 'class PyiCloudSession' --type py

Repository: timlaing/pyicloud

Length of output: 195

🏁 Script executed:

# Look for response hooks or session data updates in session-related files
fd -e py 'session' --type f

Repository: timlaing/pyicloud

Length of output: 81

🏁 Script executed:

# Check the structure of pyicloud module
ls -la pyicloud/

Repository: timlaing/pyicloud

Length of output: 1178

🏁 Script executed:

# Read the PyiCloudSession class to understand response hooks
cat -n pyicloud/session.py

Repository: timlaing/pyicloud

Length of output: 17321

🏁 Script executed:

# Read the relevant section of base.py around lines 1242-1254
sed -n '1230,1270p' pyicloud/base.py

Repository: timlaing/pyicloud

Length of output: 2346

🏁 Script executed:

# Check what HEADER_DATA contains in const.py
rg -n 'HEADER_DATA' pyicloud/const.py -A 10

Repository: timlaing/pyicloud

Length of output: 558

Remove the redundant manual header extraction; PyiCloudSession already handles these headers.

The PyiCloudSession._update_session_data() method (lines 170–175 in pyicloud/session.py) automatically extracts all four headers (X-Apple-Session-Token, scnt, X-Apple-ID-Session-Id, X-Apple-TwoSV-Trust-Token) from response headers via the HEADER_DATA mapping in const.py and updates session.data immediately after every request. This code in base.py runs after that automatic update, overwriting freshly captured values with conditional logic that skips missing headers, creating a window where stale or incomplete session data persists. Remove these lines entirely.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pyicloud/base.py` around lines 1242 - 1254, Remove the manual header
extraction block in pyicloud/base.py that reads
response.headers.get('X-Apple-Session-Token'), 'scnt', 'X-Apple-ID-Session-Id',
and 'X-Apple-TwoSV-Trust-Token' and conditionally writes them into
self.session.data; instead rely on PyiCloudSession._update_session_data() (which
uses the HEADER_DATA mapping) to perform this update automatically to avoid
overwriting fresh values with stale/missing ones—delete the entire conditional
assignment block handling new_session_token, new_scnt, new_session_id, and
trust_token.

Comment on lines +1241 to +1254
Copy link
Copy Markdown
Owner

@timlaing timlaing May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already handled by the session class


# Now try to get full account data.
# This may raise PyiCloud2FARequiredException if the session
# still appears to need 2FA (Apple's server behavior), but
# trust is already established, so we catch and return True.
try:
self._authenticate_with_token()
# Successfully authenticated with token - trust is complete
self.data['hsaTrustedBrowser'] = True
LOGGER.debug("Session trust successful.")
return True
except PyiCloud2FARequiredException:
# Even though _authenticate_with_token raised 2FA exception,
# the trust request itself succeeded (204). So we mark as trusted.
LOGGER.debug("trust_session: _authenticate_with_token raised 2FA but trust is established")
self.data['hsaTrustedBrowser'] = True
self.data['hsaChallengeRequired'] = False
LOGGER.debug("Session trust marked as successful despite 2FA exception.")
Comment on lines +1266 to +1272
Copy link
Copy Markdown
Owner

@timlaing timlaing May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to hide valid errors, such as authentication failure on subsequent logins where the browser is trusted.

return True
Comment on lines +1266 to +1273
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Forcing hsaChallengeRequired = False can mask a genuine 2FA-required state.

Catching PyiCloud2FARequiredException and unconditionally setting both hsaTrustedBrowser = True and hsaChallengeRequired = False makes requires_2fa/requires_2sa return False even in cases where the server really is asking for another factor (e.g. trust succeeded but a different policy now demands re-verification, or the inner _authenticate_with_token raised 2FA for a reason unrelated to trust). The CLI in pyicloud/cli/context.py will then skip _handle_2fa/_handle_2sa and proceed as if logged in, producing a confusing failure later.

Consider gating this on something stronger than "we got a 204" — e.g. only force-trust when the inner exception's response indicates trust was the missing piece, or at minimum log at WARNING (not DEBUG) so users can see the override happened.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pyicloud/base.py` around lines 1266 - 1273, The current trust_session handler
catches PyiCloud2FARequiredException and unconditionally sets
self.data['hsaTrustedBrowser']=True and self.data['hsaChallengeRequired']=False
after _authenticate_with_token, which can mask real 2FA requirements; modify
trust_session so it only forces hsaTrustedBrowser when the caught
PyiCloud2FARequiredException (or its underlying response) clearly indicates the
trust request succeeded for that purpose (inspect the exception/response
body/status to confirm trust was the missing piece) and otherwise preserve
hsaChallengeRequired state; if you must override, change LOGGER.debug to
LOGGER.warning and include the exception/response details in the log so callers
of requires_2fa/requires_2sa (and CLI _handle_2fa/_handle_2sa) can behave
correctly.


# If not 204, something unexpected happened
LOGGER.warning("Unexpected trust response status: %s", response.status_code)
return False
except (PyiCloudAPIResponseException, PyiCloud2FARequiredException) as exc:
LOGGER.error("Session trust failed. Exception: %s", exc)
return False

def get_webservice_url(self, ws_key: str) -> str:
Expand Down
Loading