Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/core/src/canvas/canvasTemplates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ const FREEFORM_BASE = [
'- `await ph.query(arg)` is the SECONDARY/escape path (ad-hoc, NOT saved) — reach for it only when you genuinely cannot save an insight. `arg` is a typed query node `ph.query({ kind: "TrendsQuery", series: [...], dateRange: {...} })` (series-object result, as above) or an inline HogQL string `ph.query("SELECT …")` (rows result, as above). Same result shapes as ph.loadInsight; prefer ph.loadInsight.',
'- `ph.capture(event, properties?, distinctId?)` sends an analytics event to the project (fire-and-forget; returns a promise). Use this for click/interaction tracking — e.g. `ph.capture("button_clicked", { label })`. NEVER roll your own posthog client or fetch the capture endpoint yourself.',
"- Session replay, $session_id, and person attribution are handled automatically by the host's posthog-js running in the sandbox — you do NOT set session ids or initialise recording; just call ph.capture for custom events.",
'NAVIGATION — `ph.navigate` is the ONLY way to move the user. A raw `<a href>` / `target="_blank"` is silently swallowed by the sandbox and does NOTHING, so NEVER use one:',
'- OUTBOUND/external links (e.g. an insight on PostHog.com, docs, any http(s) URL) MUST call `ph.navigate.toExternal(url)` — wire it to an `onClick`, e.g. a Quill `Button variant="link"`. Only http(s)/mailto URLs open; anything else is dropped by the host.',
"- IN-APP moves: `ph.navigate.toTask(taskId)`, `ph.navigate.toNewTask()`, `ph.navigate.toCanvas(dashboardId)`, `ph.navigate.toNewCanvas()`. All fire-and-forget and stay within the current channel.",
"- Load data inside `useEffect` with `useState`; show a loading state first, then render. Handle the empty/error case. Keep result sets small — aggregate in the query, don't fetch raw event dumps.",
];

Expand Down
40 changes: 40 additions & 0 deletions packages/core/src/canvas/freeformSchemas.test.ts
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);
});
});
20 changes: 17 additions & 3 deletions packages/core/src/canvas/freeformSchemas.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { isSafeExternalUrl } from "@posthog/shared";
import { z } from "zod";

// The template id for freeform-React canvases. Stored on a canvas's meta so the
Expand Down Expand Up @@ -203,14 +204,27 @@ export type HostToCanvasMessage = z.infer<typeof hostToCanvasMessageSchema>;

// The ONLY navigations a canvas may request of the host. The canvas runs
// untrusted code in a null-origin iframe, so this nested union IS the security
// allowlist: there is no free-form path/route field, only these four targets.
// `channelId` is intentionally absent — the host supplies it from the loaded
// record so the iframe can never pick the channel, only which task/dashboard.
// allowlist: there is no free-form path/route field, only these targets.
// `channelId` is intentionally absent from the in-app targets — the host
// supplies it from the loaded record so the iframe can never pick the channel,
// only which task/dashboard.
//
// `external` is the one target that carries a URL, for opening an outbound link
// in the user's browser. The `isSafeExternalUrl` refine is the FIRST of several
// scheme-validation layers (repeated in openUrlInBrowser, the os.openExternal
// tRPC input, and the Electron main handler): a navigate frame whose url fails
// it is dropped at safeParse and never reaches the host.
export const canvasNavIntentSchema = z.discriminatedUnion("target", [
z.object({ target: z.literal("task"), taskId: z.string().min(1) }),
z.object({ target: z.literal("new-task") }),
z.object({ target: z.literal("canvas"), dashboardId: z.string().min(1) }),
z.object({ target: z.literal("new-canvas") }),
z.object({
target: z.literal("external"),
url: z
.string()
.refine(isSafeExternalUrl, "Only http(s)/mailto URLs may be opened"),
}),
]);
export type CanvasNavIntent = z.infer<typeof canvasNavIntentSchema>;

Expand Down
14 changes: 14 additions & 0 deletions packages/ui/src/features/canvas/freeform/sandboxRuntime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,18 @@ describe("buildSandboxDocument", () => {
);
expect(html).toContain("jsxUnicodeEscapesPlugin");
});

it("exposes the ph.navigate shim including toExternal", () => {
const html = buildSandboxDocument("edit");
for (const method of [
"toTask",
"toNewTask",
"toCanvas",
"toNewCanvas",
"toExternal",
]) {
expect(html).toContain(`${method}:`);
}
expect(html).toContain('nav: { target: "external", url }');
});
});
6 changes: 5 additions & 1 deletion packages/ui/src/features/canvas/freeform/sandboxRuntime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,16 @@ export function buildSandboxDocument(
},
// Navigate the host app. Fire-and-forget: the host validates the intent
// against its allowlist and routes within the current channel. The canvas
// cannot pick the channel or an arbitrary path — only these four targets.
// cannot pick the channel or an arbitrary path — only these targets.
// toExternal opens an outbound link in the user's browser (a raw
// <a target="_blank"> is swallowed by the sandbox); the host only opens
// http(s)/mailto URLs and drops anything else.
navigate: {
toTask: (taskId) => post({ type: "navigate", nav: { target: "task", taskId } }),
toNewTask: () => post({ type: "navigate", nav: { target: "new-task" } }),
toCanvas: (dashboardId) => post({ type: "navigate", nav: { target: "canvas", dashboardId } }),
toNewCanvas: () => post({ type: "navigate", nav: { target: "new-canvas" } }),
toExternal: (url) => post({ type: "navigate", nav: { target: "external", url } }),
},
};

Expand Down
19 changes: 18 additions & 1 deletion packages/ui/src/features/canvas/freeform/useHomeCanvasView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand All @@ -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);

Copy link
Copy Markdown

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 invoking openUrlInBrowser.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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)

Learned From
PostHog/posthog#32098

Prompt To Fix With AI
This 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Prompt To Fix With AI
This 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!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Prompt To Fix With AI
This 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],
Expand Down
Loading