[MOB-8537] updates action runner logic to check allowed protocols #769
Conversation
jena-chakour
requested review from
joaodordio and
davidtruong
and removed request for
bradumbaugh
There was a problem hiding this comment.
Pull Request Overview
This PR updates the ActionRunner to enforce an allowed-protocols check before attempting to open URLs via handlers or the system opener.
- Introduces a
guardto early-exit if the action isn’t anopenUrlor the URL’s protocol isn’t allowed - Refactors external handler invocation and URL opening into separate conditional blocks
- Ensures a definitive
falsereturn when no handling occurs
Comments suppressed due to low confidence (2)
swift-sdk/Internal/ActionRunner.swift:45
- [nitpick] You're calling
detectActionType(fromAction:)again. Consider capturing the URL once (e.g., via the earlier guard) and reusing it to avoid duplicate calls and improve readability.
if case let .openUrl(url) = detectActionType(fromAction: action),
swift-sdk/Internal/ActionRunner.swift:33
- New protocol-filtering behavior should be covered by unit tests. Please add tests for allowed vs. disallowed protocols to prevent regressions.
allowedProtocols: [String] = []) -> Bool {
| guard case let .openUrl(url) = detectActionType(fromAction: action), | ||
| shouldOpenUrl(url: url, from: context.source, withAllowedProtocols: allowedProtocols) else { | ||
| return false | ||
| } | ||
|
|
||
| if case let handled = callExternalHandlers(action: action, | ||
| from: context.source, | ||
| urlHandler: urlHandler, | ||
| customActionHandler: customActionHandler), handled { | ||
| return true | ||
| } | ||
|
|
||
| if case let .openUrl(url) = detectActionType(fromAction: action), | ||
| let urlOpener = urlOpener { | ||
| urlOpener.open(url: url) | ||
| return true | ||
| } else { | ||
| if case let .openUrl(url) = detectActionType(fromAction: action), | ||
| shouldOpenUrl(url: url, from: context.source, withAllowedProtocols: allowedProtocols), | ||
| let urlOpener = urlOpener { | ||
| urlOpener.open(url: url) | ||
| return true | ||
| } else { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return false | ||
|
|
There was a problem hiding this comment.
This guard only allows openUrl actions through and returns false for all other action types, preventing external/custom handlers from running on non-URL actions. Consider separating the protocol check into the URL branch so other action types still invoke callExternalHandlers.
Copilot uses AI. Check for mistakes.
| if case let handled = callExternalHandlers(action: action, | ||
| from: context.source, | ||
| urlHandler: urlHandler, | ||
| customActionHandler: customActionHandler), handled { |
There was a problem hiding this comment.
[nitpick] Using case let for a Boolean result is unconventional. Replace if case let handled = ... with let handled = ... outside the if, then if handled { ... } for clarity.
Copilot uses AI. Check for mistakes.
🔹 Jira Ticket(s)
✏️ Description