Skip to content

fix(messaging): stop merge tags inserting double quotes #91352

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.

Sign up for GitHub

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

Open
posthog wants to merge 2 commits into
base: master
Choose a base branch
from
+32 −1
Open

fix(messaging): stop merge tags inserting double quotes #91352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00b5773
fix(messaging): stop merge tags inserting double quotes
posthog[bot] Aug 30, 2026
d965136
Merge branch 'master' into posthog-self-driving/fixmessaging-stop-mer…
mayteio Sep 2, 2026
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
16 changes: 16 additions & 0 deletions frontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
EditorRef,
EmailTemplate,
EmailTemplaterLogicProps,
buildPersonPropertyMergeValue,
emailTemplaterLogic,
} from './emailTemplaterLogic'

Expand Down Expand Up @@ -622,3 +623,18 @@ describe('emailTemplaterLogic', () => {
})
})
})

describe('buildPersonPropertyMergeValue', () => {
// Never emit double quotes: they end an HTML attribute like a link href, and entity-encoded they
// render an empty value. A bare identifier uses dot access; everything else uses single-quoted
// brackets.
it.each([
['first_name', '{{person.properties.first_name}}'],
['$browser', "{{person.properties['$browser']}}"],
['renews on', "{{person.properties['renews on']}}"],
['a.b', "{{person.properties['a.b']}}"],
["it's", "{{person.properties['it\\'s']}}"],
])('builds %s as %s', (name, expected) => {
expect(buildPersonPropertyMergeValue(name)).toBe(expected)
})
})
17 changes: 16 additions & 1 deletion frontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,21 @@
const EMAIL_EDITOR_URL_PARAM = 'editor'
const EMAIL_EDITOR_URL_VALUE = 'email'

// A property name reads back as `person.properties.foo` only when it is a bare identifier. Anything
// else (spaces, a leading $, punctuation) needs bracket access. Use single quotes, never double: a
// double quote inside an HTML attribute like a link href ends the attribute and breaks the tag, and
// when it survives as an HTML entity the renderer resolves the tag to an empty string. This mirrors
// buildDelayExpression in products/workflows stepDelayLogic.
const BARE_IDENTIFIER_REGEX = /^[A-Za-z_][A-Za-z0-9_]*$/

export function buildPersonPropertyMergeValue(name: string): string {
if (BARE_IDENTIFIER_REGEX.test(name)) {
return `{{person.properties.${name}}}`
}
const escaped = name.replace(/\\/g, '\\\\').replace(/'/g, "\\'")

@parameterai parameterai Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Merge-tag helper still lets an attacker-controlled property name break out of the Liquid tag and inject literal HTML into saved email templates

buildPersonPropertyMergeValue() (frontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.tsx:161-167) only escapes backslash and single-quote characters before embedding the property name inside {{person.properties['...']}}. It does not escape { or }. property.name comes from PropertyDefinitions loaded from loadPersonPropertyDefinitions (line ~486), which reflects arbitrary person-property keys captured from real ingested events — i.e. any unauthenticated visitor hitting the project's public capture endpoint can set a person property whose name contains }} or {{. When an org member later opens the merge-tag menu and inserts that property, the generated string, e.g. for a name like foo}} <img src=x onerror=alert(1)> {{bar, becomes {{person.properties['foo}} <img src=x onerror=alert(1)> {{bar']}}, which is saved verbatim into the stored email template HTML.

At render time, LiquidRenderer.renderWithHogFunctionGlobals (nodejs/src/cdp/utils/liquid.ts) uses the non-greedy regex /\{\{(.*?)\}\}/ to find and entity-decode Liquid tags, then calls liquid.parseAndRenderSync. Because Liquid's tag scanner also looks for the first literal }}, the injected }} prematurely closes the tag; the attacker's HTML/script payload then sits as literal template markup outside any {{ }} expression, so LiquidJS's outputEscape: 'escape' (which only escapes resolved values, not literal template bytes) never touches it — it is emitted unescaped into every email sent from that template, reaching the org's real customers. Since this PR explicitly rewrites the merge-tag emission specifically to close an HTML/Liquid-breakout bug, the new escaping should also neutralize { and } (e.g. reject/strip them, or refuse bracket-notation for names containing them and fall back to a safe placeholder) rather than only guarding against straight double quotes.

Severity: medium | Confidence: 45% | React with 👍 if useful or 👎 if not

return `{{person.properties['${escaped}']}}`
}

export interface EmailTemplaterLogicProps {
value: EmailTemplate | null
onChange: (value: EmailTemplate) => void
Expand Down Expand Up @@ -499,7 +514,7 @@
personPropertyDefinitions.forEach((property: PropertyDefinition) => {
tags[property.name] = {
name: property.name,
value: `{{person.properties["${property.name}"]}}`,
value: buildPersonPropertyMergeValue(property.name),
sample: property.example || `Sample ${property.name}`,
}
})
Expand Down Expand Up @@ -851,7 +866,7 @@
},
})),

propsChanged(({ actions, props, values, cache }, oldProps) => {

Check warning on line 869 in frontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.tsx

View workflow job for this annotation

GitHub Actions / Frontend formatting

lint:complexity 

`<anonymous>` has cyclomatic complexity 12 (warn >10)
if (props.value && !objectsEqual(props.value, oldProps.value)) {
actions.resetEmailTemplate(props.value)
autoRevealAdvancedFields(actions, props)
Expand Down
Loading