fix(coderd/notifications): skip markdown escaping for control-flow labels - #27978
Open
jdomeracki-coder wants to merge 2 commits into
Open
fix(coderd/notifications): skip markdown escaping for control-flow labels#27978jdomeracki-coder wants to merge 2 commits into
jdomeracki-coder wants to merge 2 commits into
Conversation
…bels Commit 07f79af rendered notification titles and bodies from a SanitizedPayload that Markdown-escapes every label value. That broke the AI budget admin templates, which gate a line on {{if eq .Labels.limit_source "user_override"}}: escaping turned the machine-set value into "user\\_override", so the comparison failed and the "This limit is a per-user override." line silently disappeared, causing the golden tests to fail. Add an explicit skip-list of control-flow label keys (currently just limit_source, set from the codersdk.AIBudgetLimitSource enum) that are left verbatim while all other user-controlled values remain escaped. Regenerate the AI budget user golden files for the legitimate title HTML escaping introduced by 07f79af. Generated by Coder Agents.
Apply gci/goimports ordering to smtp_internal_test.go so make fmt is a no-op and the format lint check passes. Generated by Coder Agents.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Commit
07f79af("fix: markdown rendering improvements") madenotifier.preparerender notification titles and bodies from aSanitizedPayloadthat Markdown-escapes every label value.The AI budget admin templates (added concurrently in #27415, migration
000553) gate a line on a control-flow comparison:{{- if eq .Labels.limit_source "user_override"}} This limit is a per-user override. {{- end}}limit_sourceis a machine-set enum (codersdk.AIBudgetLimitSource:user_override/group), not user input. Markdown escaping turnsuser_overrideintouser\_override, so theeqcomparison fails and the conditional line silently disappears. This brokeTestNotificationTemplates_Goldenonmainfor the AI budget templates.Fix
controlFlowLabels, currently justlimit_source) that are passed through verbatim. All other user-controlled values (display names, other labels) remain Markdown-escaped, preserving the injection hardening from07f79af.SanitizedPayloadnow accepts optionalskipLabels ...string.<title>/<h1>HTML escaping (You're->You're) introduced by07f79af. The admin HTML and webhook goldens are unchanged because the conditional line is restored.Why not just
make genthe goldens? Blindly accepting them would ship a real behavioral regression: admins would stop seeing the "per-user override" line. The skip-list restores correct behavior; the goldens only change where escaping is genuinely correct (HTML contexts).Tests
sanitize_test.go: skipped control-flow labels stay verbatim while user-controlled labels are still escaped, and an end-to-end render test asserting theeq .Labels.limit_source "user_override"conditional still renders. These lock the behavior so the goldens can't be silently regenerated away again.go test ./coderd/notifications/...passes, includingTestNotificationTemplates_Golden.Verification
limit_sourceis the onlyeq .Labels.*literal comparison across all notification template bodies.codersdk.AIBudgetLimitSourceenum incoderd/aibridgedserver/notifications.go, never from request input.<title>/<h1>; no'leaked into plaintext bodies or webhook JSON.Generated by Coder Agents on behalf of @jdomeracki-coder.