From b4648236a3ffe5f71e9171df2d1d55a9061e5607 Mon Sep 17 00:00:00 2001 From: Evan Vetere Date: Thu, 9 Jul 2026 21:39:56 -0400 Subject: [PATCH 1/2] fix: escape literal percent in error-page local reply body The extension server injects the branded error page into Envoy's local_reply_config as a SubstitutionFormatString (TextFormatSource), so Envoy parses the body as a format template. Any bare '%' in the HTML is read as the start of a command operator; the embedded default page's CSS (e.g. "height: 100%") is not escaped, so Envoy rejects the whole listener ("Couldn't find valid command at position 330"). On the failOpen:false downstream hook that NACKs the xDS update fleet-wide, taking every route with a TrafficProtectionPolicy in Enforce mode offline. Escape bare '%' to '%%' at inject time while preserving already-escaped '%%' and valid '%COMMAND%' operators (such as the intended %RESPONSE_CODE%). The transform is idempotent and also protects any mounted override page. Fixes #243 Key changes: - add escapeEnvoyFormatLiterals and apply it to the local reply body - unit tests for the escaper and a regression guard asserting the embedded default page is Envoy-safe after escaping --- internal/extensionserver/mutate/localreply.go | 45 ++++++++++++++- .../extensionserver/mutate/localreply_test.go | 55 +++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/internal/extensionserver/mutate/localreply.go b/internal/extensionserver/mutate/localreply.go index 76f80c48..af4457dc 100644 --- a/internal/extensionserver/mutate/localreply.go +++ b/internal/extensionserver/mutate/localreply.go @@ -2,6 +2,8 @@ package mutate import ( "fmt" + "regexp" + "strings" accesslogv3 "github.com/envoyproxy/go-control-plane/envoy/config/accesslog/v3" corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -146,10 +148,51 @@ func buildLocalReplyConfig(cfg *LocalReplyConfig) *hcmv3.LocalReplyConfig { ContentType: cfg.ContentType, Format: &corev3.SubstitutionFormatString_TextFormatSource{ TextFormatSource: &corev3.DataSource{ - Specifier: &corev3.DataSource_InlineString{InlineString: cfg.BodyHTML}, + Specifier: &corev3.DataSource_InlineString{InlineString: escapeEnvoyFormatLiterals(cfg.BodyHTML)}, }, }, }, }}, } } + +// envoyCommandToken matches a single well-formed Envoy substitution command +// operator (e.g. %RESPONSE_CODE%, %REQ(x-header)%, %REQ(x-header):10%) anchored +// at the start of the input. It is used to distinguish intended command +// operators from literal percent signs. +var envoyCommandToken = regexp.MustCompile(`^%[A-Z0-9_]+(\([^)]*\))?(:[0-9]+)?%`) + +// escapeEnvoyFormatLiterals makes an arbitrary string safe to embed as the +// text_format of an Envoy SubstitutionFormatString. Envoy parses that string as +// a format template, so any bare '%' is read as the start of a command operator +// and an unrecognized one is rejected — which, on the failOpen:false downstream +// hook, NACKs the whole listener xDS update fleet-wide (see issue #243: the +// branded error page's CSS "height: 100%" took the listener down). +// +// Bare '%' are escaped to '%%' (Envoy's literal-percent escape) while +// already-escaped '%%' and valid '%COMMAND%' operators (such as the intended +// %RESPONSE_CODE%) are preserved unchanged. The transform is idempotent. +func escapeEnvoyFormatLiterals(s string) string { + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); { + if s[i] != '%' { + b.WriteByte(s[i]) + i++ + continue + } + if i+1 < len(s) && s[i+1] == '%' { + b.WriteString("%%") + i += 2 + continue + } + if m := envoyCommandToken.FindString(s[i:]); m != "" { + b.WriteString(m) + i += len(m) + continue + } + b.WriteString("%%") + i++ + } + return b.String() +} diff --git a/internal/extensionserver/mutate/localreply_test.go b/internal/extensionserver/mutate/localreply_test.go index eeb87b2f..6f3c2bdf 100644 --- a/internal/extensionserver/mutate/localreply_test.go +++ b/internal/extensionserver/mutate/localreply_test.go @@ -1,6 +1,8 @@ package mutate import ( + "regexp" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -11,6 +13,8 @@ import ( routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" hcmv3 "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/network/http_connection_manager/v3" "google.golang.org/protobuf/types/known/anypb" + + "go.datum.net/network-services-operator/internal/extensionserver/assets" ) const ( @@ -218,3 +222,54 @@ func TestInjectLocalReplyConfig_NoOp(t *testing.T) { }) } } + +func TestEscapeEnvoyFormatLiterals(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"empty", "", ""}, + {"no percent", "plain", "plain"}, + {"bare percent in css", "height: 100%;", "height: 100%%;"}, + {"multiple bare percents", "120% 50% 0%", "120%% 50%% 0%%"}, + {"preserve command", "code %RESPONSE_CODE% end", "code %RESPONSE_CODE% end"}, + {"preserve command with args", "%REQ(x-header)%", "%REQ(x-header)%"}, + {"preserve command with length", "%REQ(x-header):10%", "%REQ(x-header):10%"}, + {"already escaped stays escaped", "width: 100%%;", "width: 100%%;"}, + {"mixed command and literal", "%RESPONSE_CODE% at 100%", "%RESPONSE_CODE% at 100%%"}, + {"trailing bare percent", "50%", "50%%"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := escapeEnvoyFormatLiterals(tc.in) + assert.Equal(t, tc.want, got) + assert.Equal(t, got, escapeEnvoyFormatLiterals(got), "must be idempotent") + }) + } +} + +// TestEmbeddedDefaultPageIsEnvoySafe is the regression guard for issue #243: +// the embedded default error page contains literal CSS percent signs +// (e.g. "height: 100%") that Envoy would misparse as command operators and +// reject the listener. After escaping, no bare '%' may remain and the intended +// %RESPONSE_CODE% operator must survive. +func TestEmbeddedDefaultPageIsEnvoySafe(t *testing.T) { + raw := assets.DefaultError5xxHTML + require.Contains(t, raw, "height: 100%;", "test premise: raw page has an unescaped percent") + + escaped := escapeEnvoyFormatLiterals(raw) + + assert.Contains(t, escaped, "height: 100%%;", "literal percent must be escaped") + assert.Equal(t, 1, strings.Count(escaped, "%RESPONSE_CODE%"), "command operator must be preserved exactly once") + assert.Equal(t, escaped, escapeEnvoyFormatLiterals(escaped), "escaping must be idempotent") + + // No bare '%' may remain: every '%' is either part of a '%%' pair or a + // valid command token. Strip the escaped pairs first, then the command + // tokens (with a non-anchored copy of the operator pattern), and assert + // nothing is left. + commandToken := regexp.MustCompile(`%[A-Z0-9_]+(\([^)]*\))?(:[0-9]+)?%`) + residual := strings.ReplaceAll(escaped, "%%", "") + residual = commandToken.ReplaceAllString(residual, "") + assert.NotContains(t, residual, "%", "no bare percent may remain after escaping") +} From 1c34afa21e8bc67f980da88134f2b7b812c7a023 Mon Sep 17 00:00:00 2001 From: Evan Vetere Date: Thu, 9 Jul 2026 21:59:29 -0400 Subject: [PATCH 2/2] fix: tighten error-page escaper to an allowlist The percent escaper preserved any command-shaped token (%NAME%, %REQ(...)%), but only %RESPONSE_CODE% is ever templated into the body. A command-shaped literal in an override page (e.g. "%COMPLETE%") is not a valid Envoy operator and, passed through unescaped, re-triggers the exact listener rejection this escaper exists to prevent (issue #243). Replace the grammar match with an allowlist of operators we actually emit, so every other %...% token is treated as a literal and escaped. Add a startup validation that round-trips the assembled local_reply_config and asserts the injected body carries no bare '%', catching a malformed body in the operator process instead of NACKing the xDS update fleet-wide on the failOpen:false downstream hook. A failed override falls back to the embedded default; a failed default disables branding rather than stalling the data plane. Key changes: - replace envoyCommandToken regex with envoyBodyAllowedCommands allowlist - add assertEnvoyFormatSafe and ValidateLocalReplyConfig - validate at extension-server startup with fail-safe fallback - update escaper tests for allowlist behavior; add validation tests --- internal/extensionserver/cmd/run.go | 22 ++++- internal/extensionserver/mutate/localreply.go | 84 ++++++++++++++++--- .../extensionserver/mutate/localreply_test.go | 53 +++++++++--- 3 files changed, 135 insertions(+), 24 deletions(-) diff --git a/internal/extensionserver/cmd/run.go b/internal/extensionserver/cmd/run.go index 2ddae519..cac6e9f9 100644 --- a/internal/extensionserver/cmd/run.go +++ b/internal/extensionserver/cmd/run.go @@ -142,13 +142,33 @@ func buildLocalReplyConfig(cfg config.ErrorPageConfig, log *slog.Logger) mutate. } } - return mutate.LocalReplyConfig{ + lrConfig := mutate.LocalReplyConfig{ Disabled: !cfg.Enabled, MinStatusCode: cfg.MinStatusCode, RuntimeKey: cfg.RuntimeKey, BodyHTML: body, ContentType: cfg.ContentType, } + + // Validate before the config can reach the data plane. Escaping already makes + // the body Envoy-safe, so this is defense-in-depth: if the escaper or its + // allowlist ever regresses, fail loud here and fall back to a known-safe body + // instead of NACKing the xDS update fleet-wide on the failOpen:false hook. + if err := mutate.ValidateLocalReplyConfig(&lrConfig); err != nil { + if body != extassets.DefaultError5xxHTML { + log.Error("error-page body override failed validation; using embedded default", + "path", cfg.BodyPath, "err", err) + lrConfig.BodyHTML = extassets.DefaultError5xxHTML + err = mutate.ValidateLocalReplyConfig(&lrConfig) + } + if err != nil { + log.Error("embedded default error page failed validation; disabling branded error page", + "err", err) + lrConfig.Disabled = true + } + } + + return lrConfig } // run starts the extension server with the given options. It owns cache startup, diff --git a/internal/extensionserver/mutate/localreply.go b/internal/extensionserver/mutate/localreply.go index af4457dc..1963709f 100644 --- a/internal/extensionserver/mutate/localreply.go +++ b/internal/extensionserver/mutate/localreply.go @@ -2,7 +2,6 @@ package mutate import ( "fmt" - "regexp" "strings" accesslogv3 "github.com/envoyproxy/go-control-plane/envoy/config/accesslog/v3" @@ -156,11 +155,27 @@ func buildLocalReplyConfig(cfg *LocalReplyConfig) *hcmv3.LocalReplyConfig { } } -// envoyCommandToken matches a single well-formed Envoy substitution command -// operator (e.g. %RESPONSE_CODE%, %REQ(x-header)%, %REQ(x-header):10%) anchored -// at the start of the input. It is used to distinguish intended command -// operators from literal percent signs. -var envoyCommandToken = regexp.MustCompile(`^%[A-Z0-9_]+(\([^)]*\))?(:[0-9]+)?%`) +// envoyBodyAllowedCommands is the allowlist of Envoy substitution command +// operators permitted to pass through unescaped in an error-page body. It is an +// allowlist, not a grammar match: only operators we actually template are +// preserved, because any *other* command-shaped token (e.g. a literal +// "%COMPLETE%" in an override page) is not a valid Envoy operator and would +// re-trigger the exact listener rejection this escaper exists to prevent +// (issue #243). Keep entries longest-first so matching is unambiguous. +var envoyBodyAllowedCommands = []string{ + "%RESPONSE_CODE%", +} + +// matchAllowedCommand returns the allowlisted command operator that prefixes s, +// or "" if none does. +func matchAllowedCommand(s string) string { + for _, cmd := range envoyBodyAllowedCommands { + if strings.HasPrefix(s, cmd) { + return cmd + } + } + return "" +} // escapeEnvoyFormatLiterals makes an arbitrary string safe to embed as the // text_format of an Envoy SubstitutionFormatString. Envoy parses that string as @@ -170,8 +185,11 @@ var envoyCommandToken = regexp.MustCompile(`^%[A-Z0-9_]+(\([^)]*\))?(:[0-9]+)?%` // branded error page's CSS "height: 100%" took the listener down). // // Bare '%' are escaped to '%%' (Envoy's literal-percent escape) while -// already-escaped '%%' and valid '%COMMAND%' operators (such as the intended -// %RESPONSE_CODE%) are preserved unchanged. The transform is idempotent. +// already-escaped '%%' and allowlisted operators (envoyBodyAllowedCommands, +// e.g. the intended %RESPONSE_CODE%) are preserved unchanged. Every other +// command-shaped token is treated as a literal and escaped, so an unrecognized +// operator can never survive into the pushed config. The transform is +// idempotent. func escapeEnvoyFormatLiterals(s string) string { var b strings.Builder b.Grow(len(s)) @@ -186,9 +204,9 @@ func escapeEnvoyFormatLiterals(s string) string { i += 2 continue } - if m := envoyCommandToken.FindString(s[i:]); m != "" { - b.WriteString(m) - i += len(m) + if cmd := matchAllowedCommand(s[i:]); cmd != "" { + b.WriteString(cmd) + i += len(cmd) continue } b.WriteString("%%") @@ -196,3 +214,47 @@ func escapeEnvoyFormatLiterals(s string) string { } return b.String() } + +// assertEnvoyFormatSafe returns an error if s still contains a bare '%' that +// Envoy would misparse as an incomplete or unrecognized command operator. It is +// the inverse invariant of escapeEnvoyFormatLiterals and backs the startup +// validation in ValidateLocalReplyConfig. +func assertEnvoyFormatSafe(s string) error { + for i := 0; i < len(s); { + if s[i] != '%' { + i++ + continue + } + if i+1 < len(s) && s[i+1] == '%' { + i += 2 + continue + } + if cmd := matchAllowedCommand(s[i:]); cmd != "" { + i += len(cmd) + continue + } + return fmt.Errorf("bare %% at byte offset %d", i) + } + return nil +} + +// ValidateLocalReplyConfig assembles the local_reply_config and asserts it is +// safe to push, so a malformed body is caught in the operator process at startup +// instead of NACKing the xDS update fleet-wide on the failOpen:false downstream +// hook (issue #243). It round-trips the config through proto marshalling and +// verifies the injected body carries no bare '%'. A disabled or empty config is +// vacuously valid (injection is a no-op). +func ValidateLocalReplyConfig(cfg *LocalReplyConfig) error { + if cfg.Disabled || cfg.BodyHTML == "" { + return nil + } + lrc := buildLocalReplyConfig(cfg) + if _, err := anypb.New(lrc); err != nil { + return fmt.Errorf("marshal local_reply_config: %w", err) + } + body := lrc.GetMappers()[0].GetBodyFormatOverride().GetTextFormatSource().GetInlineString() + if err := assertEnvoyFormatSafe(body); err != nil { + return fmt.Errorf("assembled error-page body is not Envoy-safe: %w", err) + } + return nil +} diff --git a/internal/extensionserver/mutate/localreply_test.go b/internal/extensionserver/mutate/localreply_test.go index 6f3c2bdf..a7bd2d5e 100644 --- a/internal/extensionserver/mutate/localreply_test.go +++ b/internal/extensionserver/mutate/localreply_test.go @@ -1,7 +1,6 @@ package mutate import ( - "regexp" "strings" "testing" @@ -233,9 +232,10 @@ func TestEscapeEnvoyFormatLiterals(t *testing.T) { {"no percent", "plain", "plain"}, {"bare percent in css", "height: 100%;", "height: 100%%;"}, {"multiple bare percents", "120% 50% 0%", "120%% 50%% 0%%"}, - {"preserve command", "code %RESPONSE_CODE% end", "code %RESPONSE_CODE% end"}, - {"preserve command with args", "%REQ(x-header)%", "%REQ(x-header)%"}, - {"preserve command with length", "%REQ(x-header):10%", "%REQ(x-header):10%"}, + {"preserve allowlisted command", "code %RESPONSE_CODE% end", "code %RESPONSE_CODE% end"}, + {"escape non-allowlisted command", "%REQ(x-header)%", "%%REQ(x-header)%%"}, + {"escape non-allowlisted command with length", "%REQ(x-header):10%", "%%REQ(x-header):10%%"}, + {"escape command-shaped literal", "progress %COMPLETE% now", "progress %%COMPLETE%% now"}, {"already escaped stays escaped", "width: 100%%;", "width: 100%%;"}, {"mixed command and literal", "%RESPONSE_CODE% at 100%", "%RESPONSE_CODE% at 100%%"}, {"trailing bare percent", "50%", "50%%"}, @@ -264,12 +264,41 @@ func TestEmbeddedDefaultPageIsEnvoySafe(t *testing.T) { assert.Equal(t, 1, strings.Count(escaped, "%RESPONSE_CODE%"), "command operator must be preserved exactly once") assert.Equal(t, escaped, escapeEnvoyFormatLiterals(escaped), "escaping must be idempotent") - // No bare '%' may remain: every '%' is either part of a '%%' pair or a - // valid command token. Strip the escaped pairs first, then the command - // tokens (with a non-anchored copy of the operator pattern), and assert - // nothing is left. - commandToken := regexp.MustCompile(`%[A-Z0-9_]+(\([^)]*\))?(:[0-9]+)?%`) - residual := strings.ReplaceAll(escaped, "%%", "") - residual = commandToken.ReplaceAllString(residual, "") - assert.NotContains(t, residual, "%", "no bare percent may remain after escaping") + // The escaped body must satisfy the same invariant the startup validator + // asserts: no bare '%' Envoy could misparse as a command operator. + assert.NoError(t, assertEnvoyFormatSafe(escaped), "no bare percent may remain after escaping") +} + +// TestValidateLocalReplyConfig covers the startup guard (issue #243): the +// assembled config must validate for the embedded default, reject a body whose +// bare '%' would reach Envoy unescaped, and no-op when injection is disabled. +func TestValidateLocalReplyConfig(t *testing.T) { + base := testLocalReplyConfig() + + t.Run("embedded default is valid", func(t *testing.T) { + cfg := *base + cfg.BodyHTML = assets.DefaultError5xxHTML + assert.NoError(t, ValidateLocalReplyConfig(&cfg)) + }) + + t.Run("escaped body is valid", func(t *testing.T) { + cfg := *base + cfg.BodyHTML = "height: 100%;" + assert.NoError(t, ValidateLocalReplyConfig(&cfg)) + }) + + t.Run("disabled is a no-op", func(t *testing.T) { + cfg := *base + cfg.Disabled = true + cfg.BodyHTML = "height: 100%;" + assert.NoError(t, ValidateLocalReplyConfig(&cfg)) + }) + + t.Run("bare percent surviving into the body is rejected", func(t *testing.T) { + // buildLocalReplyConfig escapes, so this asserts the raw invariant the + // validator enforces on the injected body. + assert.Error(t, assertEnvoyFormatSafe("height: 100%;")) + assert.Error(t, assertEnvoyFormatSafe("%COMPLETE%")) + assert.NoError(t, assertEnvoyFormatSafe("code %RESPONSE_CODE% ok")) + }) }