Allow Go templates in campaign From header - #3056
Conversation
Mirror the Subject templating path so the campaign From header can be
personalized per subscriber (e.g. `{{ .Subscriber.FirstName }}
<news@example.com>`). Compile FromEmail when it contains template
syntax, execute it per message before send, and relax the From input
validation to allow template syntax (compile errors still surface).
1fe0545 to
5c33a0d
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issues Found
No issues found.
Summary
Total issues: 0 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch is correct
Explanation: The diff cleanly adds compilation and per-subscriber rendering for templated campaign From headers, and updates validation to render against a dummy subscriber before address checks. No discrete production-breaking issues were identified in the introduced changes.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 26 turns, 27 tool calls, 4m 13s
- Tokens: in
18.0K| cached306.2K| out13.8K(total337.9K) - Cost:
$0.2775
2aa246d to
e1dfc2a
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Validate/sanitize rendered templated From header before sending (
internal/manager/message.go:45-52)FromEmailtemplates bypass the existing static email validation and the rendered value is assigned directly tomsg.from; if the template expands to an invalid address or contains CR/LF (eg. subscriber-provided name like"Eve\r\nBcc: victim@x.com"), downstream SMTP/message building may fail or allow header injection because the onlymail.ParseAddressusage is for Message-Id generation and its error is ignored. Consider rejecting renderedFromvalues that contain\r/\nand/or requiremail.ParseAddressto succeed (per-message at render time) to prevent malformed/injectable headers from being emitted.
Summary
Total issues: 1 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The feature works functionally, but it introduces a new path where unvalidated, subscriber-influenced template output becomes the SMTP From header. Without per-message validation/sanitization, malformed output can break deliveries and can enable CRLF header injection in some SMTP backends.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 18 turns, 20 tool calls, 2m 22s
- Tokens: in
17.6K| cached206.3K| out7.8K(total231.8K) - Cost:
$0.1764
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Validate rendered templated From header before sending (
internal/manager/message.go:45-52)CampaignMessage.render()copies the rendered template output directly intom.fromand it later becomesmodels.Message.Fromfor SMTP sending; if the template (or subscriber data used inside it) produces an invalid RFC5322 address or contains\r/\n, some SMTP backends will hard-fail the send and others may treat it as header-injection input. Add a post-render validation step (egmail.ParseAddress+ explicit CR/LF/control-char rejection) and return a clear error before queueing the message.
- [P1] Don't skip all FromEmail validation when template syntax is present (
cmd/campaigns.go:663-670)validateCampaignFields()now bypasses the From address regex/sanitization as soon as{{...}}is detected, which permits storing obviously unsafe values (notably ones containing CR/LF) that can later become email headers. Even if full address validation can't be done pre-render, it should still reject newline/control characters (and ideally enforce a reasonable max length) for templated From strings to avoid persistent bad campaign configs.
Summary
Total issues: 2 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The feature works functionally, but it introduces a new path where unvalidated, potentially invalid/unsafe data can become the outgoing email From header. This can cause production send failures for certain subscriber data and may enable header-injection style inputs unless the rendered output is validated/sanitized.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 35 turns, 36 tool calls, 3m 31s
- Tokens: in
22.5K| cached433.2K| out10.6K(total466.3K) - Cost:
$0.2633
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Reject CR/LF in templated campaign FromEmail during validation (
cmd/campaigns.go:664-669)validateCampaignFields()skips all address sanitization whenFromEmailcontains{{...}}, so a campaign can be saved with a multi-lineFromEmail(eg"foo\r\nBcc:..."wrapped in a template) which then becomes a raw SMTP header value at send time; this is both a send-breaker and a potential header-injection vector, so templated values should still be rejected if they contain CR/LF and ideally be validated after render.
- [P1] Validate rendered From header before putting it on outbound messages (
internal/manager/message.go:42-54)- When
FromEmailTplis executed, the rendered string is assigned tom.fromand later copied intomodels.Message.Fromwithout any normalization or validation; if the template expands to an invalid address or includes CR/LF via subscriber/campaign fields, SMTP delivery may fail or extra headers may be injected. Consider trimming and rejecting any output containing\r/\n, and/or requiringmail.ParseAddressto succeed before enqueueing the message.
- When
Summary
Total issues: 2 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The feature works functionally, but templated From values bypass existing validation and the rendered output is used as a header verbatim, which can lead to invalid outbound messages and enable header injection when templates expand to multi-line values.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 21 turns, 24 tool calls, 2m 48s
- Tokens: in
15.6K| cached237.3K| out8.5K(total261.3K) - Cost:
$0.1872
e1dfc2a to
4d8c246
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Validate/sanitize rendered templated From header to prevent header injection (
internal/manager/message.go:42-52)FromEmailcan now be templated and is rendered directly intomsg.fromwithout validating the final value or stripping CR/LF. If the template output contains newlines (eg.{{.Subscriber.Name}}where a subscriber name includes\r\n...), the resulting rawFromstring may break MIME formatting or enable header injection depending on the SMTP backend; even without newlines, invalid rendered addresses can cause send failures/bounces. Consider validating the rendered string withmail.ParseAddress(or existing From regex) and explicitly rejecting/stripping\r/\nbefore constructing the outbound message.
Important (P2)
- [P2] Avoid skipping FromEmail validation solely due to presence of template braces (
cmd/campaigns.go:663-668)validateCampaignFields()now bypasses From address validation whenever the string contains{{...}}, which makes it easy to persist obviously-invalid From values (or values containing CR/LF) and only discover problems at send time. If templating must be allowed, it is still useful to do basic structural validation (eg. reject\r/\n, and/or validate the rendered preview in UI/API) rather than treating any{{...}}as valid.
Summary
Total issues: 1 critical, 1 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The feature works conceptually, but templated From values are accepted and emitted without validating or sanitizing the rendered header, which can lead to malformed emails and potential header-injection scenarios. Tightening validation on the rendered From string (and/or applying basic CR/LF checks at save time) is needed before this is safe for production.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 31 turns, 34 tool calls, 3m 20s
- Tokens: in
22.7K| cached436.1K| out10.7K(total469.5K) - Cost:
$0.2656
|
Hi @nemunaire. I refactored and simplified the logic and added it to the central template compilation, rather than it being a separate |
|
Hi @knadh. Tested on top of v0.6.2. It also works for me as expected. 🚀 |
2 participants
Some concurrent softwares (like ActiveCampaign) allows template in the From header.
This pull request adds support for this!