feat: incoming message origin validation for android#475
Conversation
Assisted-By: devx/6d172f11-c70b-447c-9803-84d58a5e6c3a
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| * Native checkout is open by default: leaving this empty trusts every origin. Once populated, the | ||
| * effective allowlist is these origins plus the cart URL origin and `shop.app` (including its | ||
| * subdomains). Entries may be exact origins (`https://example.com`), wildcard subdomains | ||
| * (`*.example.com`), or `"*"` to explicitly trust every origin. |
There was a problem hiding this comment.
It looks like this example is missing the scheme, so the parser ignores it. Could we use https://*.example.com here and in the matching OriginAllowlist.kt example?
| var platform: Platform? = null, | ||
| var logLevel: LogLevel = LogLevel.WARN, | ||
| var preloading: Preloading = Preloading(), | ||
| var allowedMessageOrigins: Set<String> = emptySet(), |
There was a problem hiding this comment.
These are public Kotlin API additions, but I could not find the matching API baseline update. Could we refresh platforms/android/lib/api/lib.api so the API check stays green?
| val reason = "origin \"$sourceOrigin\" is not in the allowlist" | ||
| val callback = ShopifyCheckoutKit.configuration.onMessageRejected | ||
| if (callback != null) { | ||
| callback(RejectedMessage(origin = sourceOrigin, message = message, reason = reason)) |
There was a problem hiding this comment.
Could we protect this callback with try/catch? A merchant callback can throw here and escape the WebView listener. Logging the callback error and adding a small regression test would keep the rejection path best-effort.
| val (scheme, suffix, port) = match.destructured | ||
| val suffixHost = suffix.lowercase() | ||
| return target.scheme.equals(scheme, ignoreCase = true) && | ||
| port.ifEmpty { null } == target.port && |
There was a problem hiding this comment.
I think default ports need normalization here. https://host:443 currently differs from the same browser origin serialized as https://host. Could we compare effective ports for cart, exact, and wildcard origins and cover :443 and :80 in tests?
| private const val WILDCARD_ALL = "*" | ||
| private val SHOP_APP_PATTERNS = listOf(SHOP_APP_ORIGIN, "https://*.shop.app") | ||
|
|
||
| private val ORIGIN_PATTERN = Regex("""^([a-zA-Z][\w+.\-]*)://([^/:]+)(?::(\d+))?$""") |
There was a problem hiding this comment.
It looks like this regex cannot parse bracketed IPv6 hosts, which means a valid IPv6 cart or configured origin can be rejected. Would it be worth using a bracket-aware parser and adding default- and explicit-port IPv6 tests?
2 participants
What changes are you making?
How to test
Before you merge
Important
platforms/swift/README.mdand/orplatforms/android/README.md)Releasing a new Swift version?
ShopifyCheckoutKit.podspecplatforms/swift/Sources/ShopifyCheckoutKit/ShopifyCheckoutKit.swiftplatforms/swift/README.md(major version only)Releasing a new Embedded Checkout Protocol version?
embeddedCheckoutProtocolAndroidinplatforms/android/gradle/libs.versions.tomlprotocol/languages/kotlin/embedded-checkout-protocol/api/embedded-checkout-protocol.apiif the public API changedReleasing a new Android version?
checkoutKitAndroidinplatforms/android/gradle/libs.versions.tomlplatforms/android/README.mdTip
See the Contributing documentation for the full release process per platform.