[TON-1601] Improve iframe security#117
Open
ProudOfZiggy wants to merge 7 commits into
Open
Conversation
📝 WalkthroughWalkthrough
ChangesIframe Origin Enforcement and Demo Security Matrix
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over WalletKitRealBridgeIframeScreen,TonConnectInjector: Demo screen injects iframe test case
end
participant Screen as WalletKitRealBridgeIframeScreen
participant WebView
participant TonConnectInjector
participant WebMessageListener as WebMessageListener (iframeSecLog)
participant IframeSecLog
Screen->>WebView: evaluateJavascript(selected.spawnJs())
WebView->>TonConnectInjector: postMessage(bridgeMsg, sourceOrigin, isMainFrame)
TonConnectInjector->>TonConnectInjector: handleBridgeRequest(resolvedOrigin)
alt opaque-origin subframe
TonConnectInjector-->>WebView: errorResponse(403)
WebView->>WebMessageListener: iframeSecLog(frameLabel, action, claimedOrigin, sourceOrigin)
WebMessageListener->>IframeSecLog: add(entry, isNative=false)
else authorized frame
TonConnectInjector-->>Screen: TONWalletKitEvent emitted
Screen->>IframeSecLog: addNative(action, domain)
WebView->>WebMessageListener: iframeSecLog(frameLabel, action, claimedOrigin, sourceOrigin)
WebMessageListener->>IframeSecLog: add(entry, isNative=false)
end
IframeSecLog-->>Screen: entries updated → LogRow rendered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TONWalletKit-Android/impl/src/main/java/io/ton/walletkit/browser/TonConnectInjector.kt (1)
470-484:⚠️ Potential issue | 🟡 MinorConsider explicitly removing the WebMessageListener in cleanup().
The
addWebMessageListenerregistered insetup()(line 221) should be paired with an explicit removal incleanup(). While the code currently relies on the listener being implicitly replaced if a newTonConnectInjectoris set up on the same WebView, callingWebViewCompat.removeWebMessageListener(webView, BrowserConstants.JS_INTERFACE_NAME)incleanup()is more explicit and prevents potential issues with WebView reuse:override fun cleanup() { if (isCleanedUp) { return } isCleanedUp = true Logger.d(TAG, "Cleaning up TonConnect injector (browser closed, but session remains active)") if (WebViewFeature.isFeatureSupported(WebViewFeature.WEB_MESSAGE_LISTENER)) { WebViewCompat.removeWebMessageListener(webView, BrowserConstants.JS_INTERFACE_NAME) } scope.cancel() pendingRequests.clear() }🤖 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 `@TONWalletKit-Android/impl/src/main/java/io/ton/walletkit/browser/TonConnectInjector.kt` around lines 470 - 484, The cleanup() method does not explicitly remove the WebMessageListener that was registered in the setup() method. Add an explicit removal of the WebMessageListener in the cleanup() method by calling WebViewCompat.removeWebMessageListener() with the webView and BrowserConstants.JS_INTERFACE_NAME parameters, wrapped in a WebViewFeature.isFeatureSupported() check to ensure compatibility. Place this removal before the scope.cancel() call to ensure proper cleanup order.
🧹 Nitpick comments (4)
AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/WalletKitRealBridgeIframeScreen.kt (2)
287-287: 💤 Low value
SimpleDateFormatis not thread-safe.
SimpleDateFormatinstances are not thread-safe and shouldn't be shared. While Compose UI runs on the main thread, consider using a local instance in the composable or usingjava.time.format.DateTimeFormatterwhich is thread-safe.For demo code this is low risk, but a quick fix would be:
private fun formatTime(timestamp: Long): String = SimpleDateFormat("HH:mm:ss.SSS", Locale.US).format(Date(timestamp))🤖 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 `@AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/WalletKitRealBridgeIframeScreen.kt` at line 287, The `timeFormat` property is declared as a shared class-level instance of `SimpleDateFormat`, which is not thread-safe. Remove this property and create a private helper function that instantiates a new `SimpleDateFormat` locally each time formatting is needed, or use `java.time.format.DateTimeFormatter` as a thread-safe alternative. Update all usages of `timeFormat` to call this new function instead of accessing the shared instance directly.
178-222: ⚡ Quick winMissing
cleanupTonConnect()on WebView disposal in both diagnostic screens. BothAndroidViewusages callinjectTonConnect(walletKit)but never callcleanupTonConnect()when the composable leaves composition, which leaks the injector's coroutine scope and pending requests map.
AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/WalletKitRealBridgeIframeScreen.kt#L178-L222: AddonRelease = { it.cleanupTonConnect() }to theAndroidView.AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/WalletKitInvestigationScreen.kt#L172-L201: AddonRelease = { it.cleanupTonConnect() }to theAndroidView.🤖 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 `@AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/WalletKitRealBridgeIframeScreen.kt` around lines 178 - 222, Both AndroidView composables call injectTonConnect(walletKit) but are missing cleanup when the composable leaves composition, causing a resource leak of the injector's coroutine scope and pending requests map. Add the onRelease parameter to each AndroidView factory lambda to call cleanupTonConnect() on disposal. At AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/WalletKitRealBridgeIframeScreen.kt lines 178-222, add onRelease = { it.cleanupTonConnect() } to the AndroidView composable that contains the injectTonConnect(walletKit) call. At AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/WalletKitInvestigationScreen.kt lines 172-201, similarly add onRelease = { it.cleanupTonConnect() } to the AndroidView composable that contains its injectTonConnect(walletKit) call.AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/RealBridgeIframeCase.kt (1)
349-363: 💤 Low valueConsider escaping
</script>sequences in jsString().If the input string contains
</script>, the generated JS embedded in HTML could prematurely close the script tag. For demo diagnostic code this is low risk, but addings.replace("</", "<\\/")would make it more robust.🤖 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 `@AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/RealBridgeIframeCase.kt` around lines 349 - 363, The jsString function does not escape forward slash sequences that could prematurely close script tags when the generated JavaScript is embedded in HTML. Add a string replacement in the jsString function to escape `</` sequences by converting them to `<\/` before or after the existing character-by-character escaping logic. This will prevent any input containing `</script>` or similar sequences from breaking out of the HTML script tag prematurely.TONWalletKit-Android/impl/src/main/java/io/ton/walletkit/browser/TonConnectInjector.kt (1)
274-286: 💤 Low valueRetry loop executes 51 iterations, not 50.
The condition
tries++ > 50uses post-increment, so it evaluates astries > 50after incrementing. This meanstriesreaches 51 before the condition is true (0→1→...→51). Minor off-by-one if the intent was exactly 50 retries.If intentional (101ms max wait), consider clarifying the comment or using
tries++ >= 50.🤖 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 `@TONWalletKit-Android/impl/src/main/java/io/ton/walletkit/browser/TonConnectInjector.kt` around lines 274 - 286, The retry loop condition in the setInterval callback uses post-increment with `tries++ > 50`, which allows 51 iterations instead of 50 because the condition is evaluated after incrementing. To fix this, either change the condition to `tries++ >= 50` if exactly 50 retries are intended, or add a clarifying comment explaining that 51 iterations (approximately 101ms maximum wait time) is intentional. The loop polls for the JS_INTERFACE_NAME to be defined, so ensure whichever approach you choose aligns with the intended polling behavior.
🤖 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
`@TONWalletKit-Android/impl/src/main/java/io/ton/walletkit/browser/TonConnectInjector.kt`:
- Around line 470-484: The cleanup() method does not explicitly remove the
WebMessageListener that was registered in the setup() method. Add an explicit
removal of the WebMessageListener in the cleanup() method by calling
WebViewCompat.removeWebMessageListener() with the webView and
BrowserConstants.JS_INTERFACE_NAME parameters, wrapped in a
WebViewFeature.isFeatureSupported() check to ensure compatibility. Place this
removal before the scope.cancel() call to ensure proper cleanup order.
---
Nitpick comments:
In
`@AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/RealBridgeIframeCase.kt`:
- Around line 349-363: The jsString function does not escape forward slash
sequences that could prematurely close script tags when the generated JavaScript
is embedded in HTML. Add a string replacement in the jsString function to escape
`</` sequences by converting them to `<\/` before or after the existing
character-by-character escaping logic. This will prevent any input containing
`</script>` or similar sequences from breaking out of the HTML script tag
prematurely.
In
`@AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/WalletKitRealBridgeIframeScreen.kt`:
- Line 287: The `timeFormat` property is declared as a shared class-level
instance of `SimpleDateFormat`, which is not thread-safe. Remove this property
and create a private helper function that instantiates a new `SimpleDateFormat`
locally each time formatting is needed, or use
`java.time.format.DateTimeFormatter` as a thread-safe alternative. Update all
usages of `timeFormat` to call this new function instead of accessing the shared
instance directly.
- Around line 178-222: Both AndroidView composables call
injectTonConnect(walletKit) but are missing cleanup when the composable leaves
composition, causing a resource leak of the injector's coroutine scope and
pending requests map. Add the onRelease parameter to each AndroidView factory
lambda to call cleanupTonConnect() on disposal. At
AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/WalletKitRealBridgeIframeScreen.kt
lines 178-222, add onRelease = { it.cleanupTonConnect() } to the AndroidView
composable that contains the injectTonConnect(walletKit) call. At
AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/WalletKitInvestigationScreen.kt
lines 172-201, similarly add onRelease = { it.cleanupTonConnect() } to the
AndroidView composable that contains its injectTonConnect(walletKit) call.
In
`@TONWalletKit-Android/impl/src/main/java/io/ton/walletkit/browser/TonConnectInjector.kt`:
- Around line 274-286: The retry loop condition in the setInterval callback uses
post-increment with `tries++ > 50`, which allows 51 iterations instead of 50
because the condition is evaluated after incrementing. To fix this, either
change the condition to `tries++ >= 50` if exactly 50 retries are intended, or
add a clarifying comment explaining that 51 iterations (approximately 101ms
maximum wait time) is intentional. The loop polls for the JS_INTERFACE_NAME to
be defined, so ensure whichever approach you choose aligns with the intended
polling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b403d16-855b-42c8-a1dd-ae34f21026cf
📒 Files selected for processing (6)
AndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/WalletKitInvestigationScreen.ktAndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/WalletScreen.ktAndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/IframeSecurityLog.ktAndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/RealBridgeIframeCase.ktAndroidDemo/app/src/main/java/io/ton/walletkit/demo/presentation/ui/screen/iframesec/WalletKitRealBridgeIframeScreen.ktTONWalletKit-Android/impl/src/main/java/io/ton/walletkit/browser/TonConnectInjector.kt
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
2 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.
Summary by CodeRabbit
New Features
Security Improvements