fix(mobile): validate external URL schemes before opening (port #2494)#2498
Open
Gilbert09 wants to merge 2 commits into
Open
fix(mobile): validate external URL schemes before opening (port #2494)#2498Gilbert09 wants to merge 2 commits into
Gilbert09 wants to merge 2 commits into
Conversation
Port the desktop hardening from #2494 to the mobile app: before handing a URL to `Linking.openURL` / `WebBrowser.openBrowserAsync`, validate its scheme against the shared allowlist (`http:`/`https:`/`mailto:`) so tampered or attacker-supplied URLs from markdown, chat, signal reports, or MCP app content can't trigger unsafe schemes (`file:`, `data:`, `javascript:`, custom deep-links, etc.). - Add `openExternalUrl(url)` helper in `apps/mobile/src/lib` that gates `Linking.openURL` behind `@posthog/shared`'s `isSafeExternalUrl` and keeps the existing silent no-op on failure. - Route the untrusted Linking call sites through it: MarkdownText, MarkdownImage, GithubRefChip, PostHogRefChip, SignalCard, SuggestedReviewers, PrStatusBadge, and the MCP template docs link. - Gate the MCP app bridge `WebBrowser.openBrowserAsync` behind `isSafeExternalUrl`. - Add unit tests covering the allow/deny matrix and that a rejected URL never reaches `Linking.openURL`. The test also exercises `isSafeExternalUrl` inside the mobile package to confirm it resolves and runs there. RN 0.81's `URL` extracts schemes via regex in its `protocol` getter, so no polyfill fallback is needed. Generated-By: PostHog Code Task-Id: 4fe18724-3034-4b84-8924-5b52a4b933fe
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/mobile/src/features/mcp/components/McpAppHost.tsx:111-114
**Silent drop on blocked WebView URL**
`handleOpenLink` returns early without logging when the URL fails `isSafeExternalUrl`, whereas `openExternalUrl` always emits a `log.warn`. The MCP WebView bridge is the highest-risk source of attacker-controlled URLs, so a blocked attempt here would go completely undetected in logs. Consider adding a `log.warn` before the early return to match the helper's behaviour.
### Issue 2 of 2
apps/mobile/src/lib/openExternalUrl.test.ts:43-54
**Non-parameterised tests in `openExternalUrl` block**
The `isSafeExternalUrl` tests above correctly use `it.each`, but the two `openExternalUrl` tests are written as standalone cases. Given the team's preference for parameterised tests, both cases could be expressed as a single `it.each` table with columns `[url, expectedCall]` — checking `toHaveBeenCalledWith(url)` for safe URLs and `not.toHaveBeenCalled()` for unsafe ones (using a sentinel like `null` to distinguish). With only two cases the gain is small, but it would also make it easier to add future URL examples.
Reviews (1): Last reviewed commit: "fix(mobile): validate external URL schem..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
111
to
114
| const handleOpenLink = useCallback(async (args: { url: string }) => { | ||
| if (!isSafeExternalUrl(args.url)) return; | ||
| await WebBrowser.openBrowserAsync(args.url); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
handleOpenLink returns early without logging when the URL fails isSafeExternalUrl, whereas openExternalUrl always emits a log.warn. The MCP WebView bridge is the highest-risk source of attacker-controlled URLs, so a blocked attempt here would go completely undetected in logs. Consider adding a log.warn before the early return to match the helper's behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/mobile/src/features/mcp/components/McpAppHost.tsx
Line: 111-114
Comment:
**Silent drop on blocked WebView URL**
`handleOpenLink` returns early without logging when the URL fails `isSafeExternalUrl`, whereas `openExternalUrl` always emits a `log.warn`. The MCP WebView bridge is the highest-risk source of attacker-controlled URLs, so a blocked attempt here would go completely undetected in logs. Consider adding a `log.warn` before the early return to match the helper's behaviour.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+43
to
+54
| }); | ||
|
|
||
| it("opens a safe URL", () => { | ||
| openExternalUrl("https://example.com"); | ||
| expect(openURL).toHaveBeenCalledWith("https://example.com"); | ||
| }); | ||
|
|
||
| it("does not open an unsafe URL", () => { | ||
| openExternalUrl("javascript:alert(1)"); | ||
| expect(openURL).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
openExternalUrl block
The isSafeExternalUrl tests above correctly use it.each, but the two openExternalUrl tests are written as standalone cases. Given the team's preference for parameterised tests, both cases could be expressed as a single it.each table with columns [url, expectedCall] — checking toHaveBeenCalledWith(url) for safe URLs and not.toHaveBeenCalled() for unsafe ones (using a sentinel like null to distinguish). With only two cases the gain is small, but it would also make it easier to add future URL examples.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/mobile/src/lib/openExternalUrl.test.ts
Line: 43-54
Comment:
**Non-parameterised tests in `openExternalUrl` block**
The `isSafeExternalUrl` tests above correctly use `it.each`, but the two `openExternalUrl` tests are written as standalone cases. Given the team's preference for parameterised tests, both cases could be expressed as a single `it.each` table with columns `[url, expectedCall]` — checking `toHaveBeenCalledWith(url)` for safe URLs and `not.toHaveBeenCalled()` for unsafe ones (using a sentinel like `null` to distinguish). With only two cases the gain is small, but it would also make it easier to add future URL examples.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Address Greptile review feedback: - McpAppHost.handleOpenLink now logs a warning when it drops an unsafe URL, matching openExternalUrl's behaviour. The MCP WebView bridge is the highest-risk URL source, so blocked attempts shouldn't be silent. - Collapse the two openExternalUrl test cases into a single it.each table. Generated-By: PostHog Code Task-Id: 4fe18724-3034-4b84-8924-5b52a4b933fe
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
1 participant
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
Ports the desktop hardening from #2494 to the mobile app. Before handing a URL to
Linking.openURL/WebBrowser.openBrowserAsync, the scheme is validated against the shared allowlist (http:/https:/mailto:via@posthog/shared'sisSafeExternalUrl), so a tampered or attacker-supplied URL — from a markdown link, chat content, signal report, or MCP app — can't trigger an unsafe scheme (file:,smb:,data:,javascript:,ms-msdt:, custom app deep-links, etc.). Unsafe URLs are silently dropped, preserving the existing no-op UX.Changes
apps/mobile/src/lib/openExternalUrl.ts— gatesLinking.openURLbehindisSafeExternalUrland keeps the.catch(() => {})no-op on failure.chat/MarkdownText.tsx(arbitrary markdown links)chat/MarkdownImage.tsx,chat/GithubRefChip.tsx,chat/PostHogRefChip.tsxinbox/SignalCard.tsx(externalUrl),inbox/SuggestedReviewers.tsxtasks/PrStatusBadge.tsxmcp-servers/template/[id].tsx(docs_url)mcp/McpAppHost.tsx— gatesWebBrowser.openBrowserAsyncbehindisSafeExternalUrl(the URL comes from the sandboxed WebView).OAuth flows (
openAuthSessionAsync) and the fixed-domain settings link open trusted PostHog/GitHub URLs and are intentionally left unchanged.RN
URLnoteThe task flagged that React Native's
URLpolyfill can parse differently. Confirmed against RN 0.81'sLibraries/Blob/URL.js: itsprotocolgetter extracts the scheme via/^([a-zA-Z][a-zA-Z\d+\-.]*):/, soisSafeExternalUrlclassifies schemes correctly on Hermes — no fallback scheme check is needed.Tests
apps/mobile/src/lib/openExternalUrl.test.ts:javascript:/file:/data:/smb:/custom-scheme and unparseable input rejected.Linking.openURL; a safe URL does.isSafeExternalUrlinside the mobile package to confirm it resolves and runs in this bundle.Full mobile suite: 215 passing. Lint clean; typecheck adds no new errors. No desktop or
packages/sharedcode was touched.