-
Notifications
You must be signed in to change notification settings - Fork 61
Open external links from inside canvases in the browser #3662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { canvasNavIntentSchema } from "./freeformSchemas"; | ||
|
|
||
| describe("canvasNavIntentSchema", () => { | ||
| it.each([ | ||
| { target: "task", taskId: "t1" }, | ||
| { target: "new-task" }, | ||
| { target: "canvas", dashboardId: "d1" }, | ||
| { target: "new-canvas" }, | ||
| { target: "external", url: "https://posthog.com/insight/abc" }, | ||
| { target: "external", url: "http://example.com" }, | ||
| { target: "external", url: "mailto:support@posthog.com" }, | ||
| ])("accepts a valid intent: %o", (intent) => { | ||
| expect(canvasNavIntentSchema.safeParse(intent).success).toBe(true); | ||
| }); | ||
|
|
||
| // The url refine is the first scheme-validation layer: a disallowed scheme (or | ||
| // a missing url) must be dropped before it can reach the host's browser open. | ||
| it.each([ | ||
| { | ||
| name: "javascript: scheme", | ||
| intent: { target: "external", url: "javascript:alert(1)" }, | ||
| }, | ||
| { | ||
| name: "file: scheme", | ||
| intent: { target: "external", url: "file:///etc/passwd" }, | ||
| }, | ||
| { | ||
| name: "custom deep-link scheme", | ||
| intent: { target: "external", url: "ms-msdt://x" }, | ||
| }, | ||
| { | ||
| name: "non-URL string", | ||
| intent: { target: "external", url: "not a url" }, | ||
| }, | ||
| { name: "missing url", intent: { target: "external" } }, | ||
| ])("rejects $name", ({ intent }) => { | ||
| expect(canvasNavIntentSchema.safeParse(intent).success).toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,16 @@ import { | |
| navigateToChannelNewTask, | ||
| navigateToChannelTask, | ||
| } from "@posthog/ui/router/navigationBridge"; | ||
| import { openUrlInBrowser } from "@posthog/ui/utils/browser"; | ||
| import { useMutation } from "@tanstack/react-query"; | ||
| import { useCallback, useState } from "react"; | ||
| import { useCallback, useRef, useState } from "react"; | ||
|
|
||
| // Minimum gap between two accepted `toExternal` opens. A `navigate` frame is | ||
| // fire-and-forget over postMessage and can't carry a trusted user-gesture bit, | ||
| // so a buggy/malicious canvas could call it in a loop and spam the OS browser | ||
| // with tabs. A human can't click faster than this, so real clicks always pass; | ||
| // only a programmatic burst is throttled. | ||
| const EXTERNAL_OPEN_MIN_INTERVAL_MS = 750; | ||
|
|
||
| /** | ||
| * Routes a canvas's allowlisted nav intent to real host navigation. channelId is | ||
|
|
@@ -21,6 +29,7 @@ export function useCanvasNavigation( | |
| channelId: string, | ||
| ): (intent: CanvasNavIntent) => void { | ||
| const createAndOpen = useCreateAndOpenDashboard(channelId); | ||
| const lastExternalOpenRef = useRef(0); | ||
| return useCallback( | ||
| (intent: CanvasNavIntent) => { | ||
| switch (intent.target) { | ||
|
|
@@ -36,6 +45,14 @@ export function useCanvasNavigation( | |
| case "new-canvas": | ||
| void createAndOpen(); | ||
| break; | ||
| case "external": { | ||
| const now = Date.now(); | ||
| if (now - lastExternalOpenRef.current < EXTERNAL_OPEN_MIN_INTERVAL_MS) | ||
| break; | ||
| lastExternalOpenRef.current = now; | ||
| void openUrlInBrowser(intent.url); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Rule Used: Always wrap asynchronous calls that may fail, such... (source) Learned From Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/ui/src/features/canvas/freeform/useHomeCanvasView.ts
Line: 53
Comment:
**Async Open Failure Escapes Fallback**
When `os.openExternal.mutate()` rejects asynchronously, `openUrlInBrowser` does not catch that rejection because its internal `openExternalUrl(url)` call is not awaited. This fire-and-forget call then leaves an unhandled rejection and never reaches the `window.open` fallback, so external canvas links silently fail when the host procedure is unavailable or errors.
**Rule Used:** Always wrap asynchronous calls that may fail, such... ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=df81f021-48c3-45e4-981f-7348133f5f7a))
**Learned From**
[PostHog/posthog#32098](https://github.com/PostHog/posthog/pull/32098)
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On a web host without the external-open host procedure, this call reaches Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/ui/src/features/canvas/freeform/useHomeCanvasView.ts
Line: 53
Comment:
**Web Fallback Loses User Gesture**
On a web host without the external-open host procedure, this call reaches `window.open` only after a `postMessage` event rather than during the original click. Browser popup blocking can therefore reject every documented `toExternal` request, leaving canvas links unresponsive for web users.
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!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a warm iframe slot is reassigned, its Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/ui/src/features/canvas/freeform/useHomeCanvasView.ts
Line: 53
Comment:
**Stale Frame Opens Old Link**
When a warm iframe slot is reassigned, its `contentWindow` and message listener remain active while the navigation handler is replaced. A delayed `toExternal` message from the previous canvas still passes the source check and reaches this new side-effecting branch, opening a URL after the user has moved to another canvas.
How can I resolve this? If you propose a fix, please make it concise. |
||
| break; | ||
| } | ||
| } | ||
| }, | ||
| [channelId, createAndOpen], | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium: External navigation does not require user interaction
A malicious canvas can call
ph.navigate.toExternal()during rendering and open an arbitrary web or mail link without a click, repeating the action every 750 ms. The scheme allowlist prevents custom-protocol execution, but the timer only slows unsolicited navigation; require a host-controlled confirmation or another trusted user action before invokingopenUrlInBrowser.