Skip to content

refactor(api/alerts): route runtime values through the Handlebars view#2278

Merged
kodiakhq[bot] merged 3 commits into
mainfrom
port-ee-pr-2077
May 15, 2026
Merged

refactor(api/alerts): route runtime values through the Handlebars view#2278
kodiakhq[bot] merged 3 commits into
mainfrom
port-ee-pr-2077

Conversation

@teeohhem
Copy link
Copy Markdown
Contributor

@teeohhem teeohhem commented May 14, 2026

Summary

Alert bodies for saved-search and resolved alerts previously interpolated group and the truncated query results into the Handlebars template source via JS template literals, then compiled the result. Route both through the view instead and reference them via template syntax ({{#if group}}{{{group}}}{{/if}}, {{{__hdx_query_results__}}}), matching how the rest of the template already handles dynamic values.

Adds regression tests covering literal rendering of Handlebars-like substrings in those inputs.

Test plan

  • npx jest src/tasks/checkAlerts/__tests__/renderAlertTemplate.test.ts — 74 passed, 72 snapshots unchanged
  • yarn lint + tsc --noEmit on packages/api — clean
  • Two new tests cover Handlebars-like syntax in (a) query result lines and (b) group; both assert the substring renders literally and is not re-evaluated as template source

Additional notes

  • Backported from EE

Alert bodies for saved-search and resolved alerts previously interpolated
`group` and the truncated query results into the Handlebars template source
via JS template literals, then compiled the result. Route both through the
view instead and reference them via template syntax
(`{{#if group}}{{{group}}}{{/if}}`, `{{{__hdx_query_results__}}}`), matching
how the rest of the template already handles dynamic values.

Adds regression tests covering literal rendering of Handlebars-like
substrings in those inputs. Snapshots unchanged.
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 14, 2026

🦋 Changeset detected

Latest commit: cb4b9b6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented May 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 15, 2026 7:36pm

Request Review

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label May 14, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Critical-path files (1):
    • packages/api/src/tasks/checkAlerts/template.ts

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 1
  • Production lines changed: 24 (+ 44 in test files, excluded from tier calculation)
  • Branch: port-ee-pr-2077
  • Author: teeohhem

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

PR Review

✅ No critical issues found.

Clean, well-scoped fix. Routing group and query results through the Handlebars view (instead of JS template-literal interpolation into template source) is the correct mitigation against template-injection from untrusted log content / group keys. A few minor observations:

  • {{{group}}} triple-stash is correct here — alert bodies are markdown/text destined for Slack/webhooks, not HTML, so escaping would corrupt the output. The value is rendered as data, not parsed as template, so {{...}} syntax inside it cannot trigger helpers like __hdx_notify_channel__.
  • ✅ Tests cover both injection vectors (CSV log line and group) and explicitly assert the {{value}} payload is not interpolated against the view — good regression coverage.
  • ℹ️ Minor: __hdx_query_results__ is added as an intersection field on view for one branch only. If another branch were added later that forgets to set it but uses the same template skeleton, you'd silently get an empty code block. Not a bug today, just worth a unit test if more branches grow this pattern.
  • ℹ️ Pre-existing (not introduced here): truncatedResults containing a literal `` ``` sequence would still break the fenced code block in the rendered message. Out of scope for this PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

E2E Test Results

All tests passed • 178 passed • 3 skipped • 1180s

Status Count
✅ Passed 178
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

Deep Review

✅ No critical issues found.

This PR removes a real Handlebars template-injection surface by routing untrusted group and query-result strings through the view (with triple-stash placeholders) instead of concatenating them into template source via JS template literals. The mutation of a freshly spread view copy keeps the input parameter immutable, and the reserved __hdx_query_results__ key is initialized before any caller-provided field could overwrite it. The new ALERT/saved-search test is well-targeted: it both asserts the literal payload survives and asserts {{value}} is not substituted, which directly proves the source string is no longer being recompiled as a template.

🔵 P3 nitpicks (2)
  • packages/api/src/tasks/checkAlerts/__tests__/renderAlertTemplate.test.ts:250 — Malicious-payload coverage exercises only the ALERT/saved-search branch; the same {{{group}}} swap was applied to the resolved (isAlertResolved) and AlertSource.TILE branches in template.ts:637 and template.ts:731 but neither path has an injection regression test.
    • Fix: Add analogous toContain(\Group: "${maliciousPayload}"`)assertions forAlertState.OKsaved-search and formakeTileView({ group: maliciousPayload })`.
  • packages/api/src/tasks/checkAlerts/template.ts:530 — The __hdx_query_results__ field is declared via an inline intersection inside renderAlertTemplate, so the magic key is invisible from AlertMessageTemplateDefaultView alone and easy to miss when adding new render paths.
    • Fix: Hoist a local type InternalRenderView = AlertMessageTemplateDefaultView & { __hdx_query_results__: string } (or a small RenderContext wrapper) so the contract is named.

Reviewers (5): correctness, security, testing, maintainability, project-standards

@teeohhem teeohhem requested a review from wrn14897 May 14, 2026 16:17
@kodiakhq kodiakhq Bot merged commit 46c1459 into main May 15, 2026
19 checks passed
@kodiakhq kodiakhq Bot deleted the port-ee-pr-2077 branch May 15, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants