fix: JSON-escape values templated into a component's vega_spec

runtime/canvas/params.go

@@ -168,6 +168,49 @@ func ValidateParamBindings(params []*runtimev1.ComponentParam, bound map[string]
 	return nil
 }
 
+// EscapeJSONStrings returns a copy of the value with every string in it JSON-escaped (without the enclosing quotes),
+// recursing into maps and slices. It prepares template data for resolving a Vega-Lite spec,
+// which is a JSON document held in a string: a display name or string param containing a quote or backslash
+// would otherwise be spliced into the JSON unescaped and make it unparseable.
+// Numbers and booleans are returned as they are.
+func EscapeJSONStrings[T any](val T) T {
+	return escapeJSONStrings(val).(T)
+}
+
+func escapeJSONStrings(val any) any {
+	switch val := val.(type) {
+	case string:
+		var b strings.Builder
+		enc := json.NewEncoder(&b)
+		enc.SetEscapeHTML(false)
+		if err := enc.Encode(val); err != nil {
+			return val
+		}
+		// Strip the enclosing quotes and the trailing newline the encoder adds.
+		return strings.TrimSuffix(strings.TrimPrefix(strings.TrimSuffix(b.String(), "\n"), `"`), `"`)
+	case map[string]any:
+		res := make(map[string]any, len(val))
+		for k, v := range val {
+			res[k] = escapeJSONStrings(v)
+		}
+		return res
+	case map[string]string:
+		res := make(map[string]string, len(val))
+		for k, v := range val {
+			res[k] = escapeJSONStrings(v).(string)
+		}
+		return res
+	case []any:
+		res := make([]any, len(val))
+		for i, v := range val {
+			res[i] = escapeJSONStrings(v)
+		}
+		return res
+	default:
+		return val
+	}
+}
+
 // InjectVegaParams injects the values of a component's scalar params as native Vega-Lite params
 // into a Vega-Lite spec (a JSON object), making them usable in Vega expressions, predicates and extents.
 // If the spec already declares a top-level param with the same name, only its "value" is overridden

runtime/canvas/params_test.go

@@ -1,6 +1,7 @@
 package canvas_test
 
 import (
+	"encoding/json"
 	"testing"
 
 	runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1"
@@ -138,6 +139,46 @@ func TestValidateParamBindings(t *testing.T) {
 	require.ErrorContains(t, err, `not a measure in metrics view "mv1"`)
 }
 
+func TestEscapeJSONStrings(t *testing.T) {
+	in := map[string]any{
+		"params": map[string]any{
+			"measure":  "revenue",
+			"subtitle": `back\slash, "quote", tab\t, <html> & newline\n`,
+			"limit":    10.0,
+			"smooth":   true,
+			"groups":   []any{`a "b"`, 1.0, nil},
+		},
+		"fields": map[string]any{
+			"measure": map[string]any{"display_name": `Revenue "net"`},
+		},
+	}
+	out := canvas.EscapeJSONStrings(in)
+	require.Equal(t, map[string]any{
+		"params": map[string]any{
+			"measure":  "revenue",
+			"subtitle": `back\\slash, \"quote\", tab\\t, <html> & newline\\n`,
+			"limit":    10.0,
+			"smooth":   true,
+			"groups":   []any{`a \"b\"`, 1.0, nil},
+		},
+		"fields": map[string]any{
+			"measure": map[string]any{"display_name": `Revenue \"net\"`},
+		},
+	}, out)
+	// The input is not modified.
+	require.Equal(t, `Revenue "net"`, in["fields"].(map[string]any)["measure"].(map[string]any)["display_name"])
+
+	// Control characters are escaped too, and every escaped string decodes back to the original.
+	for _, s := range []string{"tab\there", "new\nline", "nul\x00", `C:\path\"dir"`, "plain"} {
+		var back string
+		require.NoError(t, json.Unmarshal([]byte(`"`+canvas.EscapeJSONStrings(s)+`"`), &back))
+		require.Equal(t, s, back)
+	}
+
+	require.Equal(t, map[string]string{"k": `v \"q\"`}, canvas.EscapeJSONStrings(map[string]string{"k": `v "q"`}))
+	require.Nil(t, canvas.EscapeJSONStrings[map[string]any](nil)["x"])
+}
+
 func TestCoerceScalarParams(t *testing.T) {
 	params := testParams(t)
 	args := map[string]any{

runtime/server/canvases.go

@@ -128,6 +128,17 @@ func (s *Server) ResolveComponent(ctx context.Context, req *runtimev1.ResolveCom
 	if spec.RendererProperties != nil {
 		props := canvas.CoerceScalarParams(spec.RendererProperties.AsMap(), spec.Params, effectiveArgs)
 
+		// A custom chart's vega_spec is a JSON document held in a string, so it is set aside and
+		// resolved below with JSON-escaped template data; resolving it with the plain data would
+		// splice a display name or string param containing a quote into the JSON unescaped.
+		var vegaSpec string
+		var hasVegaSpec bool
+		if spec.Renderer == "custom_chart" {
+			if vegaSpec, hasVegaSpec = props["vega_spec"].(string); hasVegaSpec {
+				delete(props, "vega_spec")
+			}
+		}
+
 		v, err := parser.ResolveTemplateRecursively(props, td, false)
 		if err != nil {
 			return nil, status.Errorf(codes.InvalidArgument, "failed to resolve renderer properties: %s", err.Error())
@@ -138,15 +149,22 @@ func (s *Server) ResolveComponent(ctx context.Context, req *runtimev1.ResolveCom
 			return nil, status.Errorf(codes.Internal, "failed to convert resolved renderer properties to map: %v", v)
 		}
 
-		// For custom charts, inject scalar param values as native Vega-Lite params into the vega_spec.
-		if spec.Renderer == "custom_chart" {
-			if vegaSpec, ok := props["vega_spec"].(string); ok {
-				injected, err := canvas.InjectVegaParams(vegaSpec, spec.Params, effectiveArgs)
-				if err != nil {
-					return nil, status.Errorf(codes.InvalidArgument, "failed to inject params into vega_spec: %s", err.Error())
-				}
-				props["vega_spec"] = injected
+		// For custom charts, resolve the vega_spec and inject scalar param values into it as native Vega-Lite params.
+		// The params are injected from the unescaped args; InjectVegaParams encodes them itself.
+		if hasVegaSpec {
+			vegaTD := td
+			vegaTD.User = canvas.EscapeJSONStrings(td.User)
+			vegaTD.Variables = canvas.EscapeJSONStrings(td.Variables)
+			vegaTD.ExtraProps = canvas.EscapeJSONStrings(td.ExtraProps)
+			resolved, err := parser.ResolveTemplate(vegaSpec, vegaTD, false)
+			if err != nil {
+				return nil, status.Errorf(codes.InvalidArgument, "failed to resolve vega_spec: %s", err.Error())
+			}
+			injected, err := canvas.InjectVegaParams(resolved, spec.Params, effectiveArgs)
+			if err != nil {
+				return nil, status.Errorf(codes.InvalidArgument, "failed to inject params into vega_spec: %s", err.Error())
 			}
+			props["vega_spec"] = injected
 		}
 
 		rendererProps, err = structpb.NewStruct(props)

runtime/server/canvases_test.go

@@ -1439,6 +1439,175 @@ custom_chart:
 	require.Empty(t, vegaSpec.Encoding.Y.Format)
 }
 
+// TestResolveEjectedComponentEscapesTemplateValues checks that values substituted into a vega_spec
+// are JSON-escaped: a display name or string param containing a quote or backslash must not break
+// the spec, either at the renderer (which parses it as JSON) or in InjectVegaParams.
+func TestResolveEjectedComponentEscapesTemplateValues(t *testing.T) {
+	rt, instanceID := testruntime.NewInstanceWithOptions(t, testruntime.InstanceOptions{
+		Files: map[string]string{
+			"rill.yaml": "",
+			"m1.sql":    `SELECT 'US' AS country, 1 AS amount`,
+			"mv1.yaml": `
+type: metrics_view
+version: 1
+model: m1
+dimensions:
+- column: country
+  display_name: 'Country "ISO"'
+measures:
+- name: revenue
+  display_name: 'Revenue "net"'
+  expression: SUM(amount)
+`,
+			"trend.yaml": `
+type: component
+params:
+  - name: metrics_view
+    type: metrics_view
+    required: true
+  - name: dim
+    type: dimension
+    required: true
+  - name: measure
+    type: measure
+    required: true
+  - name: subtitle
+    type: string
+    default: 'Quoted "subtitle" with a back\slash'
+  - name: limit
+    type: number
+    default: 10
+custom_chart:
+  metrics_sql: "SELECT {{ .params.dim }}, {{ .params.measure }} FROM {{ .params.metrics_view }}"
+  vega_spec: |
+    {
+      "title": {"text": "{{ .fields.measure.display_name }}", "subtitle": "{{ .params.subtitle }}"},
+      "mark": "bar",
+      "encoding": {
+        "x": {"field": "{{ .params.dim }}", "title": "{{ .fields.dim.display_name }}"},
+        "y": {"field": "{{ .params.measure }}", "title": "{{ .fields.measure.display_name }}"}
+      },
+      "data": {"name": "query1"}
+    }
+`,
+		},
+	})
+	testruntime.RequireReconcileState(t, rt, instanceID, 4, 0, 0)
+
+	server, err := server.NewServer(context.Background(), &server.Options{}, rt, zap.NewNop(), ratelimit.NewNoop(), activity.NewNoopClient())
+	require.NoError(t, err)
+
+	args, err := structpb.NewStruct(map[string]any{
+		"metrics_view": "mv1",
+		"dim":          "country",
+		"measure":      "revenue",
+	})
+	require.NoError(t, err)
+
+	res, err := server.ResolveComponent(testCtx(), &runtimev1.ResolveComponentRequest{
+		InstanceId: instanceID,
+		Component:  "trend",
+		Args:       args,
+	})
+	require.NoError(t, err)
+
+	props := res.RendererProperties.AsMap()
+	var vegaSpec struct {
+		Title struct {
+			Text     string `json:"text"`
+			Subtitle string `json:"subtitle"`
+		} `json:"title"`
+		Encoding struct {
+			X struct {
+				Field string `json:"field"`
+				Title string `json:"title"`
+			} `json:"x"`
+			Y struct {
+				Field string `json:"field"`
+				Title string `json:"title"`
+			} `json:"y"`
+		} `json:"encoding"`
+		Params []struct {
+			Name  string `json:"name"`
+			Value any    `json:"value"`
+		} `json:"params"`
+	}
+	require.NoError(t, json.Unmarshal([]byte(props["vega_spec"].(string)), &vegaSpec))
+
+	require.Equal(t, `Revenue "net"`, vegaSpec.Title.Text)
+	require.Equal(t, `Quoted "subtitle" with a back\slash`, vegaSpec.Title.Subtitle)
+	require.Equal(t, "country", vegaSpec.Encoding.X.Field)
+	require.Equal(t, `Country "ISO"`, vegaSpec.Encoding.X.Title)
+	require.Equal(t, "revenue", vegaSpec.Encoding.Y.Field)
+	require.Equal(t, `Revenue "net"`, vegaSpec.Encoding.Y.Title)
+
+	// The scalar params are still injected as native Vega-Lite params.
+	require.Len(t, vegaSpec.Params, 2)
+	require.Equal(t, "subtitle", vegaSpec.Params[0].Name)
+	require.Equal(t, `Quoted "subtitle" with a back\slash`, vegaSpec.Params[0].Value)
+	require.Equal(t, "limit", vegaSpec.Params[1].Name)
+	require.Equal(t, float64(10), vegaSpec.Params[1].Value)
+}
+
+// TestResolveComponentVegaSpecTemplateScope checks that a vega_spec is still resolved as a single
+// template, so a variable declared in one JSON string can be read in another and a control block can
+// span strings.
+func TestResolveComponentVegaSpecTemplateScope(t *testing.T) {
+	rt, instanceID := testruntime.NewInstanceWithOptions(t, testruntime.InstanceOptions{
+		Files: map[string]string{
+			"rill.yaml": "",
+			"m1.sql":    `SELECT 'US' AS country, 1 AS amount`,
+			"scoped.yaml": `
+type: component
+params:
+  - name: label
+    type: string
+    default: Sales
+  - name: detailed
+    type: boolean
+    default: true
+custom_chart:
+  metrics_sql: "SELECT country, amount FROM m1"
+  vega_spec: |
+    {
+      "title": "{{ $label := .params.label }}{{ $label }}",
+      "description": "{{ $label }}",
+      "mark": "{{ if .params.detailed }}point{{ else }}bar{{ end }}",
+      "params": [{"name": "detailed"}, {"name": "label"}]
+    }
+`,
+		},
+	})
+	testruntime.RequireReconcileState(t, rt, instanceID, 3, 0, 0)
+
+	server, err := server.NewServer(context.Background(), &server.Options{}, rt, zap.NewNop(), ratelimit.NewNoop(), activity.NewNoopClient())
+	require.NoError(t, err)
+
+	res, err := server.ResolveComponent(testCtx(), &runtimev1.ResolveComponentRequest{
+		InstanceId: instanceID,
+		Component:  "scoped",
+	})
+	require.NoError(t, err)
+
+	var vegaSpec struct {
+		Title       string `json:"title"`
+		Description string `json:"description"`
+		Mark        string `json:"mark"`
+		Params      []struct {
+			Name  string `json:"name"`
+			Value any    `json:"value"`
+		} `json:"params"`
+	}
+	props := res.RendererProperties.AsMap()
+	require.NoError(t, json.Unmarshal([]byte(props["vega_spec"].(string)), &vegaSpec))
+	require.Equal(t, "Sales", vegaSpec.Title)
+	require.Equal(t, "Sales", vegaSpec.Description)
+	require.Equal(t, "point", vegaSpec.Mark)
+	require.Len(t, vegaSpec.Params, 2)
+	require.Equal(t, true, vegaSpec.Params[0].Value)
+	require.Equal(t, "Sales", vegaSpec.Params[1].Value)
+}
+
 // TestResolveEjectedComponentFromAgentInstructions runs the exact spec shape that the component
 // authoring instructions tell the developer agent to write when it ejects a chart spec
 // (runtime/ai/instructions/data/resources/component.md), so the two cannot drift apart.