Skip to content

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Oct 16, 2025

Problem

Users would like to securely embed PostHog insights onto their websites, providing both the query and the response.

Changes

Adds "share as template with cached results" and "fetch latest results for shared template" to the sharing modal:

2025-10-16 21 51 14

When used, the code just opens the insights:

image

How did you test this code?

Opened the shared insight in a modal. Tried with different insight types.

Copy link
Contributor

github-actions bot commented Oct 16, 2025

Size Change: +132 B (0%)

Total Size: 3.06 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 3.06 MB +132 B (0%)

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 16)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 16)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 16)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 16)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@mariusandra mariusandra marked this pull request as ready for review October 16, 2025 19:57
Comment on lines +46 to +51
const handleMessage = (event: MessageEvent): void => {
const parsed = parsePayload(event.data, 'postMessage')
if (!hasParsedPayloadData(parsed)) {
return
}
setState((previous) => applyParsedPayload(previous, parsed))
Copy link
Contributor

Choose a reason for hiding this comment

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

The message event handler should validate the origin of incoming messages to prevent cross-site scripting attacks. Currently, any website could send messages to this iframe. Consider adding an origin check:

const handleMessage = (event: MessageEvent): void => {
    // Verify the message comes from an allowed origin
    if (event.origin !== targetOrigin) {
        return;
    }
    
    const parsed = parsePayload(event.data, 'postMessage')
    if (!hasParsedPayloadData(parsed)) {
        return
    }
    setState((previous) => applyParsedPayload(previous, parsed))
}

This is particularly important for iframes that will be embedded on third-party sites.

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

20 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

const escapedResults = escapeScriptJson(serializedResults)
const escapedQuery = escapeScriptJson(serializedQuery)

return `<iframe id="${iframeId}" src="${renderQueryUrl}" style="width: 100%; height: 600px; border: 0;" loading="lazy"></iframe>
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Potential XSS via unescaped iframeId attribute. The iframeId comes from insightShortId via getRenderQueryIframeId() which sanitizes with regex, but it's inserted directly into HTML. If the sanitization fails or is bypassed, this could allow attribute injection.

Suggested change
return `<iframe id="${iframeId}" src="${renderQueryUrl}" style="width: 100%; height: 600px; border: 0;" loading="lazy"></iframe>
return `<iframe id="${escapeHtmlAttribute(iframeId)}" src="${escapeHtmlAttribute(renderQueryUrl)}" style="width: 100%; height: 600px; border: 0;" loading="lazy"></iframe>
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/Sharing/SharingModal.tsx
Line: 471:471

Comment:
**logic:** Potential XSS via unescaped `iframeId` attribute. The `iframeId` comes from `insightShortId` via `getRenderQueryIframeId()` which sanitizes with regex, but it's inserted directly into HTML. If the sanitization fails or is bypassed, this could allow attribute injection.

```suggestion
    return `<iframe id="${escapeHtmlAttribute(iframeId)}" src="${escapeHtmlAttribute(renderQueryUrl)}" style="width: 100%; height: 600px; border: 0;" loading="lazy"></iframe>
```

How can I resolve this? If you propose a fix, please make it concise.

return `<iframe id="${iframeId}" src="${renderQueryUrl}" style="width: 100%; height: 600px; border: 0;" loading="lazy"></iframe>
<script>
(function () {
const iframe = document.getElementById('${iframeId}')
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Potential DOM-based XSS via unescaped iframeId in getElementById. While less critical since it's in a user-controlled snippet, the iframe ID should still be properly escaped to prevent breaking out of the string context.

Suggested change
const iframe = document.getElementById('${iframeId}')
const iframe = document.getElementById('${iframeId.replace(/'/g, "\\'")}')
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/Sharing/SharingModal.tsx
Line: 474:474

Comment:
**logic:** Potential DOM-based XSS via unescaped `iframeId` in `getElementById`. While less critical since it's in a user-controlled snippet, the iframe ID should still be properly escaped to prevent breaking out of the string context.

```suggestion
    const iframe = document.getElementById('${iframeId.replace(/'/g, "\\'")}')
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +143 to +149
@xframe_options_exempt
def render_query(request: HttpRequest) -> HttpResponse:
"""Render a lightweight container for third parties to display PostHog visualizations."""
from posthog.api.sharing import get_global_themes

payload = {"query": None, "cachedResults": None, "context": None, "insight": None, "themes": get_global_themes()}
return render_template("render_query.html", request, context={"render_query_payload": payload})
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Missing authentication check for render_query endpoint. This public endpoint returns global themes without any authentication. While it's intentionally public (due to @xframe_options_exempt), the lack of rate limiting or any access control could be abused.

Consider adding rate limiting or at minimum, documenting why this endpoint is intentionally public and unauthenticated.

Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/views.py
Line: 143:149

Comment:
**logic:** Missing authentication check for `render_query` endpoint. This public endpoint returns global themes without any authentication. While it's intentionally public (due to `@xframe_options_exempt`), the lack of rate limiting or any access control could be abused.

Consider adding rate limiting or at minimum, documenting why this endpoint is intentionally public and unauthenticated.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 435 to 437
templateLink={apiQuerySnippet}
heading="Fetch latest results for shared template"
piiWarning="Use this snippet to call the Query API directly and retrieve the freshest results for this insight."
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Misleading heading and tooltip. The heading says "Fetch latest results" but the tooltip says "shares the full insight query" - this section doesn't actually fetch results, it only provides code to call the API. The piiWarning is also labeled as describing what the snippet does, not what to be aware of.

Suggested change
templateLink={apiQuerySnippet}
heading="Fetch latest results for shared template"
piiWarning="Use this snippet to call the Query API directly and retrieve the freshest results for this insight."
heading="Call Query API to fetch latest results"
tooltip="Use this snippet to call the Query API directly and retrieve the freshest results for this insight."
piiWarning="This request shares the full insight query, including filters and properties. Review it before sharing."
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/Sharing/SharingModal.tsx
Line: 435:437

Comment:
**style:** Misleading heading and tooltip. The heading says "Fetch latest results" but the tooltip says "shares the full insight query" - this section doesn't actually fetch results, it only provides code to call the API. The `piiWarning` is also labeled as describing what the snippet does, not what to be aware of.

```suggestion
                            heading="Call Query API to fetch latest results"
                            tooltip="Use this snippet to call the Query API directly and retrieve the freshest results for this insight."
                            piiWarning="This request shares the full insight query, including filters and properties. Review it before sharing."
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +68 to +71
// use the posthog theme unless someone set a specific theme for the team
const environmentTheme = themes.find(
(theme) => !currentTeam || theme.id === currentTeam.default_data_theme
)
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Logic error in theme selection. The condition !currentTeam || theme.id === currentTeam.default_data_theme will match the first theme when currentTeam is null/undefined, regardless of whether it's the correct theme. This doesn't match the original logic which specifically looked for a team's default theme.

Suggested change
// use the posthog theme unless someone set a specific theme for the team
const environmentTheme = themes.find(
(theme) => !currentTeam || theme.id === currentTeam.default_data_theme
)
const environmentTheme = currentTeam
? themes.find((theme) => theme.id === currentTeam.default_data_theme)
: null
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/dataThemeLogic.tsx
Line: 68:71

Comment:
**logic:** Logic error in theme selection. The condition `!currentTeam || theme.id === currentTeam.default_data_theme` will match the first theme when `currentTeam` is null/undefined, regardless of whether it's the correct theme. This doesn't match the original logic which specifically looked for a team's default theme.

```suggestion
                const environmentTheme = currentTeam
                    ? themes.find((theme) => theme.id === currentTeam.default_data_theme)
                    : null
```

How can I resolve this? If you propose a fix, please make it concise.

@aspicer
Copy link
Contributor

aspicer commented Oct 16, 2025

This code looks good and tested working. Actually think you fixed an issue with exporter.html so that's nice.

My only issue is with the wording of the stuff in the share modal.

The two sections aren't really connected in concept.

The first one I might call
"Share as unsaved insight"
Share a link to an unsaved copy of this insight.

As for "Share as template with cached results" I might want to call it "Static embed with pre-computed data" and have the description say something like "IFrame that embeds this insight with the current data."

Let me know if you have other thoughts!

@aspicer aspicer enabled auto-merge (squash) October 16, 2025 23:41
Comment on lines +69 to +71
const environmentTheme = themes.find(
(theme) => !currentTeam || theme.id === currentTeam.default_data_theme
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic Bug: Incorrect theme selection when currentTeam is null

The find predicate (theme) => !currentTeam || theme.id === currentTeam.default_data_theme will return the first theme in the array when currentTeam is null, instead of falling back to posthogTheme. Since !currentTeam evaluates to true, the predicate returns true for every theme, causing find() to return the first one.

const environmentTheme = currentTeam 
    ? themes.find((theme) => theme.id === currentTeam.default_data_theme)
    : null

This ensures that when there's no team (render_query case), it falls through to use posthogTheme as intended.

Suggested change
const environmentTheme = themes.find(
(theme) => !currentTeam || theme.id === currentTeam.default_data_theme
)
const environmentTheme = currentTeam
? themes.find((theme) => theme.id === currentTeam.default_data_theme)
: null

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 16)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants