From 1a77dbbd2bf12117fbba1bb0bd4f8f936ef9fb83 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 23 Oct 2026 16:07:43 +0200 Subject: [PATCH] feat: bound template execution in tmplfuncs by refusing call, capping printf/print/println/html/js/urlquery/join output, metering range steps, limiting variables to 64 and function arguments to 32, adding a context-escaping CompileHTML, and make the integration fallback a single non-rescanning pass under the same output cap --- docs/content/docs/guides/expressions.mdx | 2 +- internal/app/integration/template.go | 26 ++- internal/app/integration/template_test.go | 7 + internal/pkg/tmplfuncs/bounded.go | 133 ++++++++++++++++ internal/pkg/tmplfuncs/bounded_test.go | 105 ++++++++++++ internal/pkg/tmplfuncs/limits.go | 186 ++++++++++++++++++---- internal/pkg/tmplfuncs/tmplfuncs.go | 13 +- 7 files changed, 429 insertions(+), 43 deletions(-) create mode 100644 internal/pkg/tmplfuncs/bounded.go create mode 100644 internal/pkg/tmplfuncs/bounded_test.go diff --git a/docs/content/docs/guides/expressions.mdx b/docs/content/docs/guides/expressions.mdx index a98eb22a5..f72dc73fd 100644 --- a/docs/content/docs/guides/expressions.mdx +++ b/docs/content/docs/guides/expressions.mdx @@ -140,7 +140,7 @@ New reply from {{.contact_email}} on campaign {{.campaign_id}} ({{.intent}}) | Warmup health changed | `email`, `new_state`, `previous_state`, `reason` | | Campaign action | `contact_email`, `contact_id`, `campaign_id`, `campaign_name`, `first_name`, `last_name`, `company`, `phone` | -Action text is a **full template, not token substitution**: conditionals, pipelines, helpers, `{{range}}`, and nested fields (`{{.lead.company}}`) all work, and values keep their type. Because numbers stay numbers, `{{if gt .confidence 0.8}}` works directly in a message body. A malformed template never blocks the action; it falls back to plain `{{.key}}` substitution. Unknown keys render empty. `{{range}}` loops over a field of the data (`{{range .lead.tags}}`), never a number or a function's result, and nests at most two deep; `{{template}}` and `{{block}}` are not supported, and output is capped at 1 MB. A template past any of these limits falls back the same way. +Action text is a **full template, not token substitution**: conditionals, pipelines, helpers, `{{range}}`, and nested fields (`{{.lead.company}}`) all work, and values keep their type. Because numbers stay numbers, `{{if gt .confidence 0.8}}` works directly in a message body. A malformed template never blocks the action; it falls back to plain `{{.key}}` substitution. Unknown keys render empty. `{{range}}` loops over a field of the data (`{{range .lead.tags}}`), never a number or a function's result, and nests at most two deep; `{{template}}`, `{{block}}` and `call` are not supported, a template declares at most 64 variables and a function takes at most 32 arguments, and output (fallback included) is capped at 1 MB. A template past any of these limits falls back the same way. diff --git a/internal/app/integration/template.go b/internal/app/integration/template.go index 3230160fe..b4a5e70c4 100644 --- a/internal/app/integration/template.go +++ b/internal/app/integration/template.go @@ -144,22 +144,32 @@ func compileTemplate(tmpl string) *template.Template { // naiveRenderTemplate is a literal {{.key}} substitution (a leading dot is // optional here), used as a safe fallback when a template can't compile/execute -// so a single malformed block never blanks the whole value. +// so a single malformed block never blanks the whole value. One left-to-right +// pass: a substituted value is never rescanned, and values share MaxOutput. func naiveRenderTemplate(tmpl string, data map[string]any) string { - out := tmpl + var b strings.Builder + budget := tmplfuncs.MaxOutput + rest := tmpl for { - start := strings.Index(out, "{{") + start := strings.Index(rest, "{{") if start < 0 { break } - end := strings.Index(out[start:], "}}") + end := strings.Index(rest[start:], "}}") if end < 0 { break } end += start - key := strings.TrimSpace(out[start+2 : end]) - key = strings.TrimPrefix(key, ".") - out = out[:start] + stringFromMap(data, key) + out[end+2:] + key := strings.TrimPrefix(strings.TrimSpace(rest[start+2:end]), ".") + value := stringFromMap(data, key) + if len(value) > budget { + value = "" + } + budget -= len(value) + b.WriteString(rest[:start]) + b.WriteString(value) + rest = rest[end+2:] } - return strings.TrimSpace(out) + b.WriteString(rest) + return strings.TrimSpace(b.String()) } diff --git a/internal/app/integration/template_test.go b/internal/app/integration/template_test.go index 74a4c7c79..24077e357 100644 --- a/internal/app/integration/template_test.go +++ b/internal/app/integration/template_test.go @@ -21,3 +21,10 @@ func TestRenderTemplateRefusesRangeOverNumber(t *testing.T) { t.Fatal("ValidExpression accepted a range over a number") } } + +func TestNaiveRenderNeverExpandsSubstitutedValues(t *testing.T) { + data := map[string]any{"a": "{{.a}}{{.a}}", "b": "B"} + if got := naiveRenderTemplate(`{{if}} {{.a}}|{{b}}`, data); got != "{{.a}}{{.a}}|B" { + t.Fatalf("got %q", got) + } +} diff --git a/internal/pkg/tmplfuncs/bounded.go b/internal/pkg/tmplfuncs/bounded.go new file mode 100644 index 000000000..e09686785 --- /dev/null +++ b/internal/pkg/tmplfuncs/bounded.go @@ -0,0 +1,133 @@ +package tmplfuncs + +import ( + "errors" + "fmt" + "reflect" + "strings" + "text/template" +) + +var ( + errCallRefused = errors.New("call is not supported") + errPrintfForm = errors.New("printf widths and argument indexes must be literal") +) + +// argSize is the printed length of a value, read without printing a string. +func argSize(v any) int { + if v == nil { + return 0 + } + if s, ok := v.([]string); ok { + n := 2 + for _, x := range s { + n += len(x) + 1 + } + return n + } + if rv := reflect.ValueOf(v); rv.Kind() == reflect.String { + return rv.Len() + } + return len(fmt.Sprint(v)) +} + +func argsSize(args []any) int { + n := 0 + for _, a := range args { + n += argSize(a) + 1 + } + return n +} + +// bounded refuses a result that would pass MaxOutput, before building it. +func bounded(size int, build func() string) (string, error) { + if size > MaxOutput { + return "", ErrOutputLimit + } + out := build() + if len(out) > MaxOutput { + return "", ErrOutputLimit + } + return out, nil +} + +// literalNumber reads the digits at format[i:], capped past MaxOutput. +func literalNumber(format string, i int) (int, int) { + n := 0 + for i < len(format) && format[i] >= '0' && format[i] <= '9' { + n = min(n*10+int(format[i]-'0'), MaxOutput+1) + i++ + } + return n, i +} + +// printfSize is an upper bound on what fmt.Sprintf(format, args...) prints. It +// refuses * widths and explicit argument indexes, whose cost it cannot see. +func printfSize(format string, args []any) (int, error) { + widest := 0 + for _, a := range args { + widest = max(widest, argSize(a)) + } + total := len(format) + argsSize(args) + for i := 0; i < len(format); i++ { + if format[i] != '%' { + continue + } + i++ + for i < len(format) && strings.IndexByte("+-# 0", format[i]) >= 0 { + i++ + } + width, precision := 0, 0 + width, i = literalNumber(format, i) + if i < len(format) && format[i] == '.' { + precision, i = literalNumber(format, i+1) + } + if i < len(format) && (format[i] == '*' || format[i] == '[') { + return 0, errPrintfForm + } + total += width + precision + widest + 32 + if total > MaxOutput { + return 0, ErrOutputLimit + } + } + return total, nil +} + +// boundedBuiltins replace the text/template builtins that can call into the +// data or build a string of any size; each refuses a result past MaxOutput. +var boundedBuiltins = template.FuncMap{ + "call": func(...any) (string, error) { return "", errCallRefused }, + "printf": func(format string, args ...any) (string, error) { + size, err := printfSize(format, args) + if err != nil { + return "", err + } + return bounded(size, func() string { return fmt.Sprintf(format, args...) }) + }, + "print": func(args ...any) (string, error) { + return bounded(argsSize(args), func() string { return fmt.Sprint(args...) }) + }, + "println": func(args ...any) (string, error) { + return bounded(argsSize(args)+1, func() string { return fmt.Sprintln(args...) }) + }, + "html": func(args ...any) (string, error) { + return bounded(argsSize(args), func() string { return template.HTMLEscaper(args...) }) + }, + "js": func(args ...any) (string, error) { + return bounded(argsSize(args), func() string { return template.JSEscaper(args...) }) + }, + "urlquery": func(args ...any) (string, error) { + return bounded(argsSize(args), func() string { return template.URLQueryEscaper(args...) }) + }, +} + +// boundedJoin is strings.Join over a list, refused past MaxOutput. +func boundedJoin(list, sep any) (string, error) { + items, _ := ListStrings(list) + s := valueString(sep) + size := len(s) * len(items) + for _, x := range items { + size += len(x) + } + return bounded(size, func() string { return strings.Join(items, s) }) +} diff --git a/internal/pkg/tmplfuncs/bounded_test.go b/internal/pkg/tmplfuncs/bounded_test.go new file mode 100644 index 000000000..c0bef91e0 --- /dev/null +++ b/internal/pkg/tmplfuncs/bounded_test.go @@ -0,0 +1,105 @@ +package tmplfuncs + +import ( + "errors" + "strings" + "testing" +) + +func TestBuiltinsAreBounded(t *testing.T) { + big := strings.Repeat("x", MaxOutput/2+1) + data := map[string]any{ + "name": "Ann", "big": big, "list": []string{big, big, big}, + "fn": func() string { return "called" }, + } + for _, src := range []string{ + `{{call .fn}}`, + `{{printf "%2000000s" .name}}`, + `{{printf "%.2000000f" 1.5}}`, + `{{printf "%*s" 2000000 .name}}`, + `{{printf "%[1]s%[1]s%[1]s" .big}}`, + `{{printf "%s%s" .big .big}}`, + `{{print .big .big}}`, + `{{println .big .big}}`, + `{{html .big .big}}`, + `{{js .big .big}}`, + `{{urlquery .big .big}}`, + `{{join .list ","}}`, + } { + for kind, compile := range map[string]func(string, string) (Executor, error){ + "text": func(n, s string) (Executor, error) { return Compile(n, s) }, + "html": func(n, s string) (Executor, error) { return CompileHTML(n, s) }, + } { + tmpl, err := compile("t", src) + if err != nil { + continue + } + if out, err := Execute(tmpl, data); err == nil { + t.Errorf("%s %s rendered %d bytes, want a refusal", kind, src, len(out)) + } + } + } +} + +func TestBuiltinsStillRender(t *testing.T) { + data := map[string]any{"name": "Ann & Co", "n": 3, "list": []string{"a", "b"}} + for src, want := range map[string]string{ + `{{printf "%s-%03d" .name .n}}`: "Ann & Co-003", + `{{print .name .n}}`: "Ann & Co3", + `{{html .name}}`: "Ann & Co", + `{{urlquery .name}}`: "Ann+%26+Co", + `{{join .list ", "}}`: "a, b", + `{{.name | printf "%q"}}`: `"Ann & Co"`, + `{{range .list}}{{.}}{{end}}`: "ab", + `{{range $i, $v := .list}}{{$i}}{{end}}`: "01", + } { + tmpl, err := Compile("t", src) + if err != nil { + t.Fatalf("Compile(%q): %v", src, err) + } + if out, err := Execute(tmpl, data); err != nil || out != want { + t.Errorf("%s = %q, %v; want %q", src, out, err, want) + } + } +} + +func TestExecuteBoundsRangeSteps(t *testing.T) { + const src = `{{range .n}}{{range $.n}}{{end}}{{end}}` + text, err := Compile("t", src) + if err != nil { + t.Fatal(err) + } + html, err := CompileHTML("t", src) + if err != nil { + t.Fatal(err) + } + for _, tmpl := range []Executor{text, html} { + if _, err := Execute(tmpl, map[string]any{"n": 100000}); !errors.Is(err, ErrStepLimit) { + t.Fatalf("err = %v, want ErrStepLimit", err) + } + } +} + +func TestCompileRefusesTooManyVariablesAndArgs(t *testing.T) { + vars := strings.Repeat(`{{$a := 1}}`, MaxVariables+1) + args := `{{print` + strings.Repeat(" 1", MaxArgs+1) + `}}` + for _, src := range []string{vars, args} { + if _, err := Compile("t", src); err == nil { + t.Errorf("Compile accepted %.40s...", src) + } + if _, err := CompileHTML("t", src); err == nil { + t.Errorf("CompileHTML accepted %.40s...", src) + } + } +} + +func TestCompileHTMLEscapesInContext(t *testing.T) { + tmpl, err := CompileHTML("t", `{{.n}}`) + if err != nil { + t.Fatal(err) + } + out, err := Execute(tmpl, map[string]any{"u": "javascript:alert(1)", "n": `"x"`}) + if err != nil || out != `<b>"x"</b>` { + t.Fatalf("got %q, %v", out, err) + } +} diff --git a/internal/pkg/tmplfuncs/limits.go b/internal/pkg/tmplfuncs/limits.go index cf3247cff..089e41389 100644 --- a/internal/pkg/tmplfuncs/limits.go +++ b/internal/pkg/tmplfuncs/limits.go @@ -3,6 +3,8 @@ package tmplfuncs import ( "errors" "fmt" + htmltemplate "html/template" + "io" "strings" "sync" "sync/atomic" @@ -13,17 +15,31 @@ import ( // MaxOutput bounds what one render of a user-authored template may write. const MaxOutput = 1 << 20 +// MaxSteps bounds how many writes one render may make, which bounds range iterations. +const MaxSteps = 1 << 18 + // MaxRangeDepth bounds how deeply {{range}} blocks may nest. const MaxRangeDepth = 2 +// MaxVariables bounds how many variables one template may declare. +const MaxVariables = 64 + +// MaxArgs bounds how many arguments one command may pass. +const MaxArgs = 32 + // ErrOutputLimit is returned when a render passes MaxOutput. var ErrOutputLimit = errors.New("template output exceeds the size limit") +// ErrStepLimit is returned when a render passes MaxSteps. +var ErrStepLimit = errors.New("template render exceeds the step limit") + var ( errRangeSource = errors.New("range only iterates over a field, like {{range .items}}") errRangeDepth = fmt.Errorf("range blocks may nest at most %d deep", MaxRangeDepth) errTemplateUse = errors.New("{{template}} and {{block}} are not supported") errRootAssign = errors.New("$ cannot be reassigned") + errVariables = fmt.Errorf("a template may declare at most %d variables", MaxVariables) + errArgs = fmt.Errorf("a function may take at most %d arguments", MaxArgs) ) // Compile parses a user-authored template with the shared funcs and absent keys @@ -36,18 +52,51 @@ func Compile(name, src string) (*template.Template, error) { if err := CheckLimits(t); err != nil { return nil, err } + for _, tt := range t.Templates() { + meterRanges(tt.Tree) + } + return t, nil +} + +// CompileHTML is Compile for an HTML document: html/template escapes every +// value for the context it lands in, under the same funcs and limits. +func CompileHTML(name, src string) (*htmltemplate.Template, error) { + t, err := htmltemplate.New(name).Funcs(funcs).Option("missingkey=zero").Parse(src) + if err != nil { + return nil, err + } + trees := make([]*parse.Tree, 0, 1) + for _, tt := range t.Templates() { + trees = append(trees, tt.Tree) + } + if err := checkTrees(trees); err != nil { + return nil, err + } + for _, tree := range trees { + meterRanges(tree) + } return t, nil } // CheckLimits refuses constructs whose cost is not bounded by the data: a // {{range}} over anything but a field of the data, {{range}} nested past -// MaxRangeDepth, {{template}} calls, and reassigning $. +// MaxRangeDepth, {{template}} calls, reassigning $, more than MaxVariables +// declarations and commands with more than MaxArgs arguments. func CheckLimits(t *template.Template) error { + trees := make([]*parse.Tree, 0, 1) for _, tt := range t.Templates() { - if tt.Tree == nil || tt.Root == nil { + trees = append(trees, tt.Tree) + } + return checkTrees(trees) +} + +func checkTrees(trees []*parse.Tree) error { + vars := 0 + for _, tree := range trees { + if tree == nil || tree.Root == nil { continue } - if err := checkNode(tt.Root, 0, true); err != nil { + if err := checkNode(tree.Root, 0, true, &vars); err != nil { return err } } @@ -55,21 +104,21 @@ func CheckLimits(t *template.Template) error { } // checkNode walks n; dotIsData is whether dot can only hold a value from the data. -func checkNode(n parse.Node, depth int, dotIsData bool) error { +func checkNode(n parse.Node, depth int, dotIsData bool, vars *int) error { switch x := n.(type) { case *parse.ListNode: if x == nil { return nil } for _, c := range x.Nodes { - if err := checkNode(c, depth, dotIsData); err != nil { + if err := checkNode(c, depth, dotIsData, vars); err != nil { return err } } case *parse.ActionNode: - return checkPipe(x.Pipe) + return checkPipe(x.Pipe, vars) case *parse.RangeNode: - if err := checkPipe(x.Pipe); err != nil { + if err := checkPipe(x.Pipe, vars); err != nil { return err } if depth+1 > MaxRangeDepth { @@ -78,34 +127,35 @@ func checkNode(n parse.Node, depth int, dotIsData bool) error { if !isDataPath(x.Pipe, dotIsData) { return errRangeSource } - if err := checkNode(x.List, depth+1, true); err != nil { + if err := checkNode(x.List, depth+1, true, vars); err != nil { return err } - return checkNode(x.ElseList, depth, dotIsData) + return checkNode(x.ElseList, depth, dotIsData, vars) case *parse.IfNode: - if err := checkPipe(x.Pipe); err != nil { + if err := checkPipe(x.Pipe, vars); err != nil { return err } - if err := checkNode(x.List, depth, dotIsData); err != nil { + if err := checkNode(x.List, depth, dotIsData, vars); err != nil { return err } - return checkNode(x.ElseList, depth, dotIsData) + return checkNode(x.ElseList, depth, dotIsData, vars) case *parse.WithNode: - if err := checkPipe(x.Pipe); err != nil { + if err := checkPipe(x.Pipe, vars); err != nil { return err } - if err := checkNode(x.List, depth, isDataPath(x.Pipe, dotIsData)); err != nil { + if err := checkNode(x.List, depth, isDataPath(x.Pipe, dotIsData), vars); err != nil { return err } - return checkNode(x.ElseList, depth, dotIsData) + return checkNode(x.ElseList, depth, dotIsData, vars) case *parse.TemplateNode: return errTemplateUse } return nil } -// checkPipe refuses a pipeline that declares or assigns $, the data root. -func checkPipe(p *parse.PipeNode) error { +// checkPipe refuses a pipeline that declares or assigns $, declares past +// MaxVariables, or passes a command more than MaxArgs arguments. +func checkPipe(p *parse.PipeNode, vars *int) error { if p == nil { return nil } @@ -113,11 +163,20 @@ func checkPipe(p *parse.PipeNode) error { if len(v.Ident) > 0 && v.Ident[0] == "$" { return errRootAssign } + if !p.IsAssign { + *vars++ + } + } + if *vars > MaxVariables { + return errVariables } for _, c := range p.Cmds { + if len(c.Args) > MaxArgs+1 { + return errArgs + } for _, a := range c.Args { if sub, ok := a.(*parse.PipeNode); ok { - if err := checkPipe(sub); err != nil { + if err := checkPipe(sub, vars); err != nil { return err } } @@ -148,9 +207,64 @@ func isPathNode(n parse.Node, dotIsData bool) bool { return false } -// Execute renders t into a string, failing once the output passes MaxOutput. -func Execute(t *template.Template, data any) (string, error) { - w := &cappedWriter{max: MaxOutput} +// meterRanges opens every range body with an empty write, so each iteration +// counts against MaxSteps even when the body writes nothing. +func meterRanges(tree *parse.Tree) { + if tree == nil { + return + } + var walk func(n parse.Node) + walk = func(n parse.Node) { + switch x := n.(type) { + case *parse.ListNode: + if x == nil { + return + } + for _, c := range x.Nodes { + walk(c) + } + case *parse.RangeNode: + walk(x.List) + walk(x.ElseList) + if x.List != nil { + x.List.Nodes = append([]parse.Node{emptyText()}, x.List.Nodes...) + } + case *parse.IfNode: + walk(x.List) + walk(x.ElseList) + case *parse.WithNode: + walk(x.List) + walk(x.ElseList) + } + } + walk(tree.Root) +} + +// emptyText returns a text node owned by a parsed tree, so copying it is safe. +func emptyText() parse.Node { + trees, err := parse.Parse("step", " ", "", "") + if err != nil { + panic(err) + } + n := trees["step"].Root.Nodes[0].(*parse.TextNode) + n.Text = n.Text[:0] + return n +} + +// Executor is a compiled text or HTML template. +type Executor interface { + Execute(w io.Writer, data any) error +} + +// Execute renders t into a string, failing once the output passes MaxOutput +// or the render passes MaxSteps. +func Execute(t Executor, data any) (string, error) { + return ExecuteLimit(t, data, MaxOutput) +} + +// ExecuteLimit is Execute with a smaller output budget, for callers sharing one. +func ExecuteLimit(t Executor, data any, limit int) (string, error) { + w := &cappedWriter{max: min(limit, MaxOutput)} if err := t.Execute(w, data); err != nil { return "", err } @@ -158,39 +272,53 @@ func Execute(t *template.Template, data any) (string, error) { } type cappedWriter struct { - b strings.Builder - max int + b strings.Builder + max int + steps int } func (w *cappedWriter) Write(p []byte) (int, error) { + w.steps++ + if w.steps > MaxSteps { + return 0, ErrStepLimit + } if w.b.Len()+len(p) > w.max { return 0, ErrOutputLimit } return w.b.Write(p) } +// IsLimit reports whether err is a render passing one of the limits. +func IsLimit(err error) bool { + return errors.Is(err, ErrOutputLimit) || errors.Is(err, ErrStepLimit) +} + // CacheCap bounds how many compiled templates a Cache keeps. const CacheCap = 4096 -// Cache maps a template source to its compiled form, nil marking a source +// Cache maps a template source to its compiled text template. +type Cache = TypedCache[*template.Template] + +// TypedCache maps a template source to its compiled form, nil marking a source // that failed to compile. Past CacheCap a miss is recompiled, not stored. -type Cache struct { +type TypedCache[T any] struct { m sync.Map n atomic.Int64 } // Load returns the cached template for src and whether src was cached. -func (c *Cache) Load(src string) (*template.Template, bool) { +func (c *TypedCache[T]) Load(src string) (T, bool) { v, ok := c.m.Load(src) if !ok { - return nil, false + var zero T + return zero, false } - t, _ := v.(*template.Template) + t, _ := v.(T) return t, true } // Store caches t (nil for a known-bad source) unless the cache is full. -func (c *Cache) Store(src string, t *template.Template) { +func (c *TypedCache[T]) Store(src string, t T) { if c.n.Load() >= CacheCap { return } diff --git a/internal/pkg/tmplfuncs/tmplfuncs.go b/internal/pkg/tmplfuncs/tmplfuncs.go index 0e84c60c3..a5642a9d6 100644 --- a/internal/pkg/tmplfuncs/tmplfuncs.go +++ b/internal/pkg/tmplfuncs/tmplfuncs.go @@ -110,11 +110,8 @@ var funcs = template.FuncMap{ }, // List helpers: {{has .ai_labels "Meeting"}} is whole-item membership, so // "Interested" does not match "Not interested"; {{join .ai_labels ", "}}. - "has": ListHas, - "join": func(list, sep any) string { - items, _ := ListStrings(list) - return strings.Join(items, valueString(sep)) - }, + "has": ListHas, + "join": boundedJoin, "hasPrefix": func(s, p any) bool { return strings.HasPrefix(valueString(s), valueString(p)) }, "hasSuffix": func(s, p any) bool { return strings.HasSuffix(valueString(s), valueString(p)) }, // Fallback: {{.FirstName | default "there"}} -> "there" when the field is blank. @@ -126,6 +123,12 @@ var funcs = template.FuncMap{ }, } +func init() { + for name, fn := range boundedBuiltins { + funcs[name] = fn + } +} + // FuncMap returns the shared template function set. The returned map is shared // (the funcs are stateless), so callers must not mutate it. func FuncMap() template.FuncMap {