fix: improve 2FA handling for China mainland accounts#222
fix: improve 2FA handling for China mainland accounts#222Twsa wants to merge 1 commit intotimlaing:mainfrom
Conversation
- Fix requires_2fa/requires_2sa to check _requires_mfa flag when hsaVersion is not returned by Apple's server - Auto-detect China mainland endpoints when account_country is CHN - Handle 302 redirect with domainToUse hint to switch to correct regional endpoint - Improve trust_session to properly handle 204 response from /2sv/trust and mark session as trusted when trust is established
📝 WalkthroughSummary by CodeRabbit
WalkthroughModifications to authentication logic in Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant pyicloud as pyicloud.base
participant Auth as Authentication Service
participant Redirect as Redirect Handler
Browser->>pyicloud: authenticate_with_token()
pyicloud->>pyicloud: Check account_country in session.data
alt China Mainland Detected
pyicloud->>pyicloud: Rebuild endpoints with .cn domains
pyicloud->>pyicloud: Update request headers
end
pyicloud->>Auth: accountLogin request
alt HTTP 302 Redirect
Auth-->>pyicloud: 302 response with domainToUse
pyicloud->>Redirect: Parse JSON for domainToUse
alt .cn domain suggested
Redirect->>pyicloud: Switch to China endpoints
pyicloud->>Auth: Retry accountLogin
end
else HTTP 200 Success
Auth-->>pyicloud: Authentication successful
end
pyicloud->>pyicloud: Check requires_2fa (_requires_mfa flag)
alt 2FA Required
pyicloud-->>Browser: Raise 2FA Exception
else Proceed
pyicloud-->>Browser: Authentication complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.5)pyicloud/base.pyThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyicloud/base.py (1)
1224-1230:⚠️ Potential issue | 🟠 MajorDon't clear
_requires_mfabefore trust is actually established.
self._requires_mfa = Falseis set unconditionally on entry. If the GET on line 1231 raises, returns a non-204 status, or the inner_authenticate_with_tokenultimately fails, the method returnsFalsebut_requires_mfahas already been zeroed — meaning subsequent calls torequires_2fa/requires_2sawill report no MFA needed even though MFA clearly was needed. Move this assignment to the success path (right next to wherehsaTrustedBrowseris set) so the flag is only cleared when trust really succeeds.🛠️ Suggested fix
def trust_session(self) -> bool: """Request session trust to avoid user log in going forward.""" - self._requires_mfa = False - headers: dict[str, Any] = self._get_auth_headers()…and in the 204 success branch (lines 1263-1273), add
self._requires_mfa = Falsealongsideself.data['hsaTrustedBrowser'] = True.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/base.py` around lines 1224 - 1230, The trust_session method currently clears self._requires_mfa immediately on entry which can incorrectly suppress MFA flags if the trust request fails; move the assignment so that self._requires_mfa = False is only set when trust is actually established (i.e., in the success/204 branch where you already set self.data['hsaTrustedBrowser'] = True), and remove the unconditional clear at the top of trust_session so that failures in the GET or in _authenticate_with_token do not incorrectly clear the MFA requirement.
🧹 Nitpick comments (1)
pyicloud/base.py (1)
769-789: Redundant inner check —_is_mfa_required()already covers_requires_mfa.
_is_mfa_required()(line 763-767) already returnsTruewheneverself._requires_mfaisTrue. Once the outer call isTruebecause of_requires_mfa, the new inneror self._requires_mfais alsoTrue, so the disjunction collapses to: "outer is true AND (hsaVersionmatches OR_requires_mfa)". Functionally correct, but the logic can be expressed more simply and is easier to reason about for the next reader.♻️ Optional simplification
`@property` 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 - or self._requires_mfa - ) + self._requires_mfa + or ( + self._is_mfa_required() + and self.data.get("dsInfo", {}).get("hsaVersion", 0) >= 1 + ) ) `@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 - or self._requires_mfa - ) + self._requires_mfa + or ( + self._is_mfa_required() + and self.data.get("dsInfo", {}).get("hsaVersion", 0) == 2 + ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/base.py` around lines 769 - 789, The requires_2sa and requires_2fa properties contain a redundant inner "or self._requires_mfa" because _is_mfa_required() already returns True when self._requires_mfa is True; update both properties (requires_2sa, requires_2fa) to remove the inner disjunction and simply combine _is_mfa_required() with the dsInfo hsaVersion checks (self.data.get("dsInfo", {}).get("hsaVersion", 0) >= 1 for requires_2sa and == 2 for requires_2fa) so the logic is clearer while preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyicloud/base.py`:
- Around line 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.
- Around line 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.
- Around line 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.
- Around line 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.
- Around line 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.
---
Outside diff comments:
In `@pyicloud/base.py`:
- Around line 1224-1230: The trust_session method currently clears
self._requires_mfa immediately on entry which can incorrectly suppress MFA flags
if the trust request fails; move the assignment so that self._requires_mfa =
False is only set when trust is actually established (i.e., in the success/204
branch where you already set self.data['hsaTrustedBrowser'] = True), and remove
the unconditional clear at the top of trust_session so that failures in the GET
or in _authenticate_with_token do not incorrectly clear the MFA requirement.
---
Nitpick comments:
In `@pyicloud/base.py`:
- Around line 769-789: The requires_2sa and requires_2fa properties contain a
redundant inner "or self._requires_mfa" because _is_mfa_required() already
returns True when self._requires_mfa is True; update both properties
(requires_2sa, requires_2fa) to remove the inner disjunction and simply combine
_is_mfa_required() with the dsInfo hsaVersion checks (self.data.get("dsInfo",
{}).get("hsaVersion", 0) >= 1 for requires_2sa and == 2 for requires_2fa) so the
logic is clearer while preserving behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # 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}/" |
There was a problem hiding this comment.
🧩 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 -150Repository: 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 -100Repository: timlaing/pyicloud
Length of output: 2684
🏁 Script executed:
#!/bin/bash
# Get _get_auth_headers method
rg -nA 20 'def _get_auth_headers' pyicloud/base.pyRepository: 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.pyRepository: 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 -5Repository: 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/ -iRepository: 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 -40Repository: 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.pyRepository: 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 -50Repository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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 -70Repository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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.
|
|
||
| # 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 | ||
|
|
There was a problem hiding this comment.
🧩 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.pyRepository: 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.pyRepository: 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 -nRepository: 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 -nRepository: 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.pyRepository: 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 -nRepository: 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.pyRepository: 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 -20Repository: 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.pyRepository: 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 -nRepository: 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.pyRepository: 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.pyRepository: 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 -300Repository: 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 -nRepository: 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.pyRepository: 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 -10Repository: 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 -5Repository: 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 -10Repository: 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.pyRepository: timlaing/pyicloud
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Look at conftest.py to understand test setup
head -100 tests/conftest.pyRepository: 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.pyRepository: 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.pyRepository: 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 -20Repository: 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 -nRepository: 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__.pyRepository: 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 -nRepository: 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 -A30Repository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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 -100Repository: 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 -50Repository: 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.pyRepository: 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.pyRepository: 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:
-
Silent fall-through to garbage data: If the 302 response has no
domainToUsefield,_is_china_mainlandis already True,bodyis not JSON-parseable, or the retry itself returns another 302, control falls through to line 679 whereresp.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_termsand_update_state. -
Retry omits header updates: Unlike the auto-detect branch above (lines 646–647), the retry POST does not refresh
session.headers["Origin"]orsession.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. -
Imprecise exception handling:
except (ValueError, KeyError): passsilently discards errors.KeyErroris unreachable (only.get()and guarded indexing used), andAttributeError(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.
| LOGGER.debug("trust_session response status: %s", response.status_code) | ||
| LOGGER.debug("trust_session response headers: %s", dict(response.headers)) |
There was a problem hiding this comment.
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.
| 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.
| 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 |
There was a problem hiding this comment.
🧩 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.pyRepository: timlaing/pyicloud
Length of output: 43
🏁 Script executed:
# First, find the PyiCloudSession class definition
rg -n 'class PyiCloudSession' --type pyRepository: 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 fRepository: 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.pyRepository: 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.pyRepository: timlaing/pyicloud
Length of output: 2346
🏁 Script executed:
# Check what HEADER_DATA contains in const.py
rg -n 'HEADER_DATA' pyicloud/const.py -A 10Repository: 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.
| 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.") | ||
| return True |
There was a problem hiding this comment.
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.
timlaing
left a comment
timlaing
left a comment
There was a problem hiding this comment.
@Twsa - Thank you for your contribution
Please break this pull request down in to two fixes:
- For automatic domain resolution
- 2FA handling.
The automatic domain handling looks good, but work in going to be required for the 2FA code.
| and self.data.get("dsInfo", {}).get("hsaVersion", 0) >= 1 | ||
| and ( | ||
| self.data.get("dsInfo", {}).get("hsaVersion", 0) >= 1 | ||
| or self._requires_mfa |
There was a problem hiding this comment.
This check is performed in the _is_mfa_required method and as such not needed here
| and self.data.get("dsInfo", {}).get("hsaVersion", 0) == 2 | ||
| and ( | ||
| self.data.get("dsInfo", {}).get("hsaVersion", 0) == 2 | ||
| or self._requires_mfa |
There was a problem hiding this comment.
This check is performed in the _is_mfa_required method and as such not needed here
| # 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 |
There was a problem hiding this comment.
This is already handled by the session class
| 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.") |
There was a problem hiding this comment.
This is going to hide valid errors, such as authentication failure on subsequent logins where the browser is trusted.
2 participants
Summary
Fixes 2FA authentication for China mainland Apple ID accounts by addressing three issues:
hsaVersion, these properties incorrectly returnFalseeven when_requires_mfaisTrue.cnendpoints but the library defaults to international endpointsGET /2sv/trustreturns 204 (success) but subsequentaccountLoginstill requires 2FAChanges
base.py
requires_2saandrequires_2fa: Added check for_requires_mfaflag alongsidehsaVersion_authenticate_with_token: Auto-detect China mainland based onaccount_country: CHNand switch endpoints; handle 302 redirect withdomainToUsehinttrust_session: Properly handle 204 response from/2sv/trustand mark session as trusted when trust is established; extract session tokens from response headersTest
Successfully tested with China mainland Apple ID:
Related Issues