fix: restore SMS and trusted-device 2FA auth flows#210
Merged
timlaing merged 5 commits intotimlaing:mainfrom Apr 3, 2026
Merged
fix: restore SMS and trusted-device 2FA auth flows#210timlaing merged 5 commits intotimlaing:mainfrom
timlaing merged 5 commits intotimlaing:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an HSA2 trusted-device bridge (WebSocket + prover) and SMS fallback to the 2FA flow, updates MFA discovery and CLI interaction to request delivery before prompting, introduces bridge/session helpers and exceptions, adds raw-request entrypoint, tests, docs, and a cryptography dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PyiCloud as PyiCloud Auth
participant Apple as Apple Auth Endpoint
participant Bridge as HSA2 Bridge
participant SMS as SMS Service
Client->>PyiCloud: request_2fa_code()
PyiCloud->>Apple: GET /appleauth/auth (Accept: text/html)
Apple-->>PyiCloud: Auth response + HTML boot context
PyiCloud->>PyiCloud: Parse boot context, derive delivery method
alt Bridge available & supported
PyiCloud->>Bridge: Open WebSocket, generate ECDSA keypair
Bridge-->>PyiCloud: Push token / challenge
PyiCloud->>Apple: POST bridge step-0
Bridge-->>PyiCloud: Bridge push payload (step 2 salt)
PyiCloud-->>Client: "Trusted-device prompt received"
else Bridge unavailable or timeout
PyiCloud->>SMS: PUT /verify/phone (request SMS)
SMS-->>Client: SMS code delivered
PyiCloud-->>Client: "Code sent via SMS"
end
Client->>PyiCloud: validate_2fa_code(code)
alt Verification via Bridge
PyiCloud->>PyiCloud: SPake2 handshake (salt + code)
PyiCloud->>Bridge: POST bridge step-2 confirmation
Bridge-->>PyiCloud: Bridge push (step-4 encrypted code)
PyiCloud->>PyiCloud: Decrypt code, verify
PyiCloud->>Apple: POST bridge step-4 completion
Apple-->>PyiCloud: Success
else Verification via SMS
PyiCloud->>Apple: POST /verify/phone/securitycode
Apple-->>PyiCloud: Success
end
PyiCloud->>PyiCloud: Clear bridge state, reset delivery tracking
PyiCloud-->>Client: Authentication complete
sequenceDiagram
participant Prover as TrustedDeviceProver
participant Verifier as TrustedDeviceVerifier
Prover->>Prover: init_with_salt(salt_b64, code)
Prover->>Prover: Generate P-256 keypair
Prover->>Verifier: get_message1() → point hex
Verifier->>Verifier: Generate P-256 keypair
Verifier-->>Prover: message1 → point hex
Prover->>Prover: process_message1(peer_point) → shared secret, transcript
Prover->>Verifier: get_message2() → HMAC hex
Verifier->>Verifier: Verify message2, derive key
Verifier-->>Prover: Encrypted code (AES-GCM)
Prover->>Prover: process_message2(verifier_msg) → verify + derive bridge key
Prover->>Prover: decrypt_message(ciphertext) → plaintext code
Prover-->>Verifier: Code decrypted and verified
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Thanks 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)
1162-1181:⚠️ Potential issue | 🟡 MinorPotential
modemismatch between SMS trigger and validation.The
modefield at line 1171 usestrusted_phone_number.push_mode, which may beNoneif Apple's payload didn't include it. However,_request_sms_2fa_code(line 866) always sends"mode": "sms". This inconsistency could cause validation failures if the mode doesn't match what was used to trigger delivery.Proposed fix to default to "sms" when push_mode is None
data: dict[str, Any] = { "phoneNumber": trusted_phone_number.as_phone_number_payload(), "securityCode": {"code": code}, - "mode": trusted_phone_number.push_mode, + "mode": trusted_phone_number.push_mode or "sms", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/base.py` around lines 1162 - 1181, In _validate_sms_code, the "mode" value is set from trusted_phone_number.push_mode which can be None and mismatch _request_sms_2fa_code which sends "mode":"sms"; update _validate_sms_code to use trusted_phone_number.push_mode or default to "sms" when push_mode is falsy (e.g., mode = trusted_phone_number.push_mode or "sms") so the verification payload mode matches the SMS trigger; locate the method _validate_sms_code, the call to self._trusted_phone_number(), and the "mode" field in the data dict to apply this change.
🧹 Nitpick comments (1)
tests/test_base.py (1)
963-974: Prefertmp_pathhere over a shared temp directory.Reusing
<tmp>/python-test-results/bridge-authmakes the session/cookie files process-global, so repeated or parallel test runs can collide. A per-test temp path keeps this persistence assertion isolated.Proposed change
def test_session_persistence_excludes_trusted_device_bridge_state( pyicloud_service_working: PyiCloudService, + tmp_path: Path, ) -> None: """Bridge-only state should remain in memory and never be written to persisted session files.""" - temp_root = Path(tempfile.gettempdir()) / "python-test-results" / "bridge-auth" + temp_root = tmp_path / "bridge-auth" temp_root.mkdir(parents=True, exist_ok=True) session = PyiCloudSession( service=pyicloud_service_working, client_id="", cookie_directory=str(temp_root),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_base.py` around lines 963 - 974, Replace the shared process-global temp directory with the pytest tmp_path fixture in test_session_persistence_excludes_trusted_device_bridge_state: instead of constructing temp_root from tempfile.gettempdir(), accept tmp_path as a test parameter and use tmp_path (or tmp_path / "bridge-auth") for the cookie_directory when creating the PyiCloudSession instance so the session/cookie files are isolated per test run and avoid collisions.
🤖 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/cli/context.py`:
- Around line 339-381: The flow currently always enters the numeric 2FA prompt
loop even when api.request_2fa_code() returns False; change the control so that
after calling api.request_2fa_code() you only proceed into the typer.prompt /
api.validate_2fa_code() loop when request_2fa_code() returned True, and when it
returned False provide an immediate actionable response (e.g., print guidance or
raise CLIAbort indicating this is a non-code/security-key challenge and explain
how to proceed) instead of prompting for a six-digit code; update the logic
around api.request_2fa_code(), the subsequent max_attempts/typer.prompt loop,
and any error handling for PyiCloudTrustedDeviceVerificationException to ensure
the security-key path is handled separately from the numeric-code path.
In `@pyicloud/exceptions.py`:
- Around line 102-107: The two new exceptions
PyiCloudTrustedDevicePromptException and
PyiCloudTrustedDeviceVerificationException currently inherit from
PyiCloudException which prevents them from being caught by broad API-response
handlers; change their base class to PyiCloudAPIResponseException so they remain
distinct types but are still part of the API-response hierarchy (update the
class definitions for PyiCloudTrustedDevicePromptException and
PyiCloudTrustedDeviceVerificationException to inherit from
PyiCloudAPIResponseException instead of PyiCloudException).
In `@pyicloud/session.py`:
- Around line 231-251: _request_raw currently lets requests transport exceptions
bubble up, creating an inconsistent failure contract vs the normalized path used
by _request; wrap the call to super().request in a try/except that catches
requests.exceptions.RequestException and normalize the failure the same way the
regular request path does (reuse the same helper or logic used by _request to
convert transport/timeout/redirect errors into the unified failure
representation), then ensure _update_session_data/_save_session_data still run
if appropriate and return the normalized Response-like result or raise the same
normalized exception type as _request.
In `@README.md`:
- Around line 1262-1265: Remove the duplicate section headings by keeping only
one "Notes CLI" and one "Notes CLI Example" heading; search for the repeated
heading strings ("Notes CLI" and "Notes CLI Example") in README.md (occurrences
around the reported areas) and delete the redundant duplicates so the document
outline no longer contains repeated headings and MD024 is resolved.
- Around line 185-187: Update the README sentence to avoid implying
request_2fa_code() initiates security-key flows: state that for HSA2 accounts
request_2fa_code() starts Apple's active delivery route for the current
challenge (which may present a trusted-device prompt or send an SMS code), and
explicitly note that security-key flows are handled separately (the example
checks security_key_names and initiates security-key handling outside
request_2fa_code()). Mention the check of security_key_names and that
security-key handling occurs before/independently of calling request_2fa_code().
---
Outside diff comments:
In `@pyicloud/base.py`:
- Around line 1162-1181: In _validate_sms_code, the "mode" value is set from
trusted_phone_number.push_mode which can be None and mismatch
_request_sms_2fa_code which sends "mode":"sms"; update _validate_sms_code to use
trusted_phone_number.push_mode or default to "sms" when push_mode is falsy
(e.g., mode = trusted_phone_number.push_mode or "sms") so the verification
payload mode matches the SMS trigger; locate the method _validate_sms_code, the
call to self._trusted_phone_number(), and the "mode" field in the data dict to
apply this change.
---
Nitpick comments:
In `@tests/test_base.py`:
- Around line 963-974: Replace the shared process-global temp directory with the
pytest tmp_path fixture in
test_session_persistence_excludes_trusted_device_bridge_state: instead of
constructing temp_root from tempfile.gettempdir(), accept tmp_path as a test
parameter and use tmp_path (or tmp_path / "bridge-auth") for the
cookie_directory when creating the PyiCloudSession instance so the
session/cookie files are isolated per test run and avoid collisions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07a6b59c-234a-4361-b714-12b9df4ced40
📒 Files selected for processing (11)
README.mdpyicloud/base.pypyicloud/cli/context.pypyicloud/exceptions.pypyicloud/hsa2_bridge.pypyicloud/hsa2_bridge_prover.pypyicloud/session.pyrequirements.txttests/test_base.pytests/test_cmdline.pytests/test_hsa2_bridge.py
MrJarnould
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/hsa2_bridge_prover.py`:
- Around line 443-450: The ephemeral scalar generation uses
secrets.randbelow(_P256_ORDER) which can yield 0; update init_with_salt (and the
similar site around 517-525) to draw a non-zero scalar for x_scalar (and any
other ephemeral scalars) by looping until a value in 1..(_P256_ORDER-1) is
produced before constructing _ClientHandshake; reference _compute_w0_w1,
_ClientHandshake, secrets.randbelow and _P256_ORDER when making the change so
the scalar domain matches the non-zero group elements used elsewhere.
- Around line 496-511: The decrypt_message method can raise IndexError/KeyError
or cryptography InvalidTag for malformed payloads, bypassing the caller's
ValueError handling; import InvalidTag from cryptography.exceptions at module
scope and wrap the payload parsing and AESGCM.decrypt call in a try/except that
catches (IndexError, KeyError, InvalidTag) and re-raises a ValueError("Malformed
bridge payload") (preserving the original message if desired), ensuring
references to decrypt_message, _AES_GCM_LAYOUTS and self._verifier_key are
handled inside the try block.
In `@tests/test_base.py`:
- Around line 1004-1049: The test never exercises the persistence boundary
because it leaves bridge secrets on
pyicloud_service_working._trusted_device_bridge_state while _save_session_data()
only serialises session._data; update the
test_session_persistence_excludes_trusted_device_bridge_state test to inject the
bridge-only fields into session._data (or otherwise ensure they appear in the
in-memory dict that PyiCloudSession._save_session_data serialises) before
calling session._save_session_data(), then assert those bridge values are not
present in the persisted session file or cookiejar; reference
PyiCloudSession._save_session_data, session._data, and
pyicloud_service_working._trusted_device_bridge_state to locate the relevant
code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60823ba4-c185-4a6e-8e82-242f64d5fe33
📒 Files selected for processing (9)
README.mdpyicloud/base.pypyicloud/cli/context.pypyicloud/exceptions.pypyicloud/hsa2_bridge.pypyicloud/hsa2_bridge_prover.pypyicloud/session.pytests/test_base.pytests/test_cmdline.py
✅ Files skipped from review due to trivial changes (3)
- README.md
- tests/test_cmdline.py
- pyicloud/hsa2_bridge.py
🚧 Files skipped from review as they are similar to previous changes (3)
- pyicloud/exceptions.py
- pyicloud/cli/context.py
- pyicloud/base.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Owner
|
Thanks for the fix. |
timlaing
pushed a commit
to MrJarnould/pyicloud
that referenced
this pull request
Apr 3, 2026
* feat: handle Apple's HSA2 trusted-device prompts * Trim unrelated Notes PR scope * Address CodeRabbit review comments * Add docstrings for auth bridge PR scope * Harden bridge prover and persistence tests
12 tasks
timlaing
added a commit
that referenced
this pull request
Apr 3, 2026
* feat: add CLI support for Notes and Reminders * fix: repair Notes folder desired keys * fix: derive reminder list counts from membership * fix: speed up Notes sync and fix Reminders hashtag round-trips * docs: use installed icloud CLI in README examples * fix: address Notes and Reminders CLI review nits * Update permissions for autolabeler workflow Signed-off-by: Tim Laing <11019084+timlaing@users.noreply.github.com> * Fix formatting in autolabeler workflow by removing unnecessary blank line * feat: add a root --version CLI flag (#214) Co-authored-by: Tim Laing <11019084+timlaing@users.noreply.github.com> * fix: restore SMS and trusted-device 2FA auth flows (#210) * feat: handle Apple's HSA2 trusted-device prompts * Trim unrelated Notes PR scope * Address CodeRabbit review comments * Add docstrings for auth bridge PR scope * Harden bridge prover and persistence tests * Update protobuf requirement from <7,>=6.31.1 to >=6.31.1,<8 (#208) Updates the requirements on [protobuf](https://github.com/protocolbuffers/protobuf) to permit the latest version. - [Release notes](https://github.com/protocolbuffers/protobuf/releases) - [Commits](https://github.com/protocolbuffers/protobuf/commits) --- updated-dependencies: - dependency-name: protobuf dependency-version: 7.34.1 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github/codeql-action from 4.34.1 to 4.35.1 (#209) Bumps [github/codeql-action](https://github.com/github/codeql-action) from 4.34.1 to 4.35.1. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@3869755...c10b806) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: 4.35.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor: remove obsolete 2FA tests for trusted device and SMS flows --------- Signed-off-by: Tim Laing <11019084+timlaing@users.noreply.github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Tim Laing <11019084+timlaing@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
This PR fixes two broken HSA2 authentication flows in
pyicloud:fixes #204)Before this change, SMS delivery could fail because
pyiclouddid not reliably parse Apple's SMS delivery mode and did not make the request needed to trigger SMS delivery before prompting for a code. The trusted-device flow also failed becausepyiclouddid not implement the Apple bridge flow required to request and validate trusted-device prompts.This PR fixes those issues by:
request_2fa_code()to start active HSA2 code delivery before validationType of change
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: