fix(ios): derive openSecureWindow callbackURLScheme from redirectUri - #640
fix(ios): derive openSecureWindow callbackURLScheme from redirectUri#640kfarwell wants to merge 5 commits into
Conversation
openSecureWindow passed the authEndpoint's scheme (generally https) to ASWebAuthenticationSession, but should be the app's custom scheme per Apple's spec.
Beta npm buildThis PR comes from a fork, so beta publish is disabled for security. If you need a beta package, move the branch into this repository first. |
📝 WalkthroughWalkthrough
ChangesSecure window callback handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant openSecureWindow
participant ASWebAuthenticationSession
participant AuthProvider
participant SecureWindowRedirectSupport
openSecureWindow->>ASWebAuthenticationSession: start authentication with redirectUri scheme
ASWebAuthenticationSession->>AuthProvider: authenticate
AuthProvider-->>ASWebAuthenticationSession: redirect callback URL
ASWebAuthenticationSession->>SecureWindowRedirectSupport: matches callback URL against redirectUri
SecureWindowRedirectSupport-->>openSecureWindow: accept or reject callback
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
Hello thanks for this, can you check the ai issues ? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/Sources/InAppBrowserPlugin/InAppBrowserPlugin.swift`:
- Around line 127-130: Update the query-parameter validation around
receivedItems so duplicate occurrences of configured parameter names are
rejected when their values conflict, including callbacks containing both
attacker-controlled and expected values. Preserve acceptance of unrelated
provider parameters and require each configured parameter to match
unambiguously.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2184c6cb-c8a2-456b-8874-da58bab521d4
📒 Files selected for processing (1)
ios/Sources/InAppBrowserPlugin/InAppBrowserPlugin.swift
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual) → reviewed against branchmaininstead of the default branch
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Compare the user and password components, and require the callback's query items under each configured name to match exactly, so a conflicting duplicate is rejected and repeated items are all required. Add tests for the matching rules.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/Sources/InAppBrowserPlugin/InAppBrowserPlugin.swift`:
- Line 126: Update the redirect path comparisons in the matching logic around
normalizedPath to use percentEncodedPath for both received and expected URLs,
preserving encoded path segments while retaining existing normalization. Add a
regression test covering callbacks with an encoded slash versus a literal slash
and ensure they do not match.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 07c7a82a-b09a-4fa1-92d0-edf6abfbefb8
📒 Files selected for processing (2)
ios/Sources/InAppBrowserPlugin/InAppBrowserPlugin.swiftios/Tests/InAppBrowserPluginTests/SecureWindowRedirectSupportTests.swift
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual) → reviewed against branchmaininstead of the default branch
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Compare percent-encoded and require repeated query items in order.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| func testRejectsInjectedUserInfo() throws { | ||
| XCTAssertFalse(try matches("myapp://evil@callback?code=abc", "myapp://callback")) | ||
| XCTAssertFalse(try matches("https://evil:secret@example.com/callback", "https://example.com/callback")) |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/Tests/InAppBrowserPluginTests/SecureWindowRedirectSupportTests.swift (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the “any order” test cover two configured items.
The redirect URI configures only
type;codeis an extra provider parameter. This does not verify that multiple configured query items can be reordered. Add a second configured item and assert the callback succeeds with those configured items reversed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Tests/InAppBrowserPluginTests/SecureWindowRedirectSupportTests.swift` around lines 15 - 19, Update testAcceptsConfiguredQueryItemsInAnyOrder so the configured redirect URI includes two query items, then assert matches succeeds when the callback presents those configured items in reverse order while retaining any extra provider parameter coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ios/Tests/InAppBrowserPluginTests/SecureWindowRedirectSupportTests.swift`:
- Around line 15-19: Update testAcceptsConfiguredQueryItemsInAnyOrder so the
configured redirect URI includes two query items, then assert matches succeeds
when the callback presents those configured items in reverse order while
retaining any extra provider parameter coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63a962c2-8ac5-4dc0-a12a-150293ca4398
📒 Files selected for processing (2)
ios/Sources/InAppBrowserPlugin/InAppBrowserPlugin.swiftios/Tests/InAppBrowserPluginTests/SecureWindowRedirectSupportTests.swift
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual) → reviewed against branchmaininstead of the default branch
|
Now matching scheme/user/password/host/port/path and query items configured in the redirect URI in FYI looks like Android and web have the same startsWith issue. Cheers |
3 participants
Currently
openSecureWindowpasses the authEndpoint's scheme (https) ascallbackURLSchemetoASWebAuthenticationSession. Per Apple's documentation,callbackURLSchememust be the app's custom scheme for our session to intercept. This PR derives the callback scheme fromredirectUri, falling back to the oldurl.schemeif it can't parse one.http(s) works in practice only when the redirect scheme is registered in Info.plist, but this behaviour appears to be undocumented. With the fix, it is no longer necessary to add the scheme to Info.plist.
Issue reproduced and fix verified on iOS 26.5.
See: https://developer.apple.com/documentation/authenticationservices/aswebauthenticationsession/init(url:callbackurlscheme:completionhandler:)-7fpox
Summary by CodeRabbit