From 1df318ba52a4aa499888d64264c5189f6ba0b095 Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Wed, 21 Oct 2026 10:37:41 +0530 Subject: [PATCH] fix: JSON-escape values templated into a component's vega_spec ResolveComponent resolved a custom chart's vega_spec with the plain template data, so a field display name or string param containing a quote or backslash was spliced into the JSON unescaped. An ejected chart bound to a measure with display_name 'Revenue "net"' then failed in InjectVegaParams (or, without scalar params, in the renderer as "Invalid Vega-Lite spec"). Resolve the vega_spec with a copy of the template data in which every string (.params, .fields, .env, .user) is JSON-escaped, so substituted values are valid inside the JSON strings they land in. The spec is still resolved as a single template, so template scope across JSON strings is unchanged, and the scalar params are injected from the unescaped args as before. --- runtime/canvas/params.go | 43 ++++++++ runtime/canvas/params_test.go | 41 ++++++++ runtime/server/canvases.go | 34 +++++-- runtime/server/canvases_test.go | 169 ++++++++++++++++++++++++++++++++ 4 files changed, 279 insertions(+), 8 deletions(-) diff --git a/runtime/canvas/params.go b/runtime/canvas/params.go index 05a7c790caf..894dbedf706 100644 --- a/runtime/canvas/params.go +++ b/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 diff --git a/runtime/canvas/params_test.go b/runtime/canvas/params_test.go index 13778d4c1bd..f379db3193f 100644 --- a/runtime/canvas/params_test.go +++ b/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, & 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, & 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{ diff --git a/runtime/server/canvases.go b/runtime/server/canvases.go index c164e844e29..af33b2d958a 100644 --- a/runtime/server/canvases.go +++ b/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) diff --git a/runtime/server/canvases_test.go b/runtime/server/canvases_test.go index 5f2455c03c0..8cec0a1d77e 100644 --- a/runtime/server/canvases_test.go +++ b/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.