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 76f80c48..1963709f 100644 --- a/internal/extensionserver/mutate/localreply.go +++ b/internal/extensionserver/mutate/localreply.go @@ -2,6 +2,7 @@ package mutate import ( "fmt" + "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 +147,114 @@ 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)}, }, }, }, }}, } } + +// 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 +// 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 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)) + 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 cmd := matchAllowedCommand(s[i:]); cmd != "" { + b.WriteString(cmd) + i += len(cmd) + continue + } + b.WriteString("%%") + i++ + } + 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 eeb87b2f..a7bd2d5e 100644 --- a/internal/extensionserver/mutate/localreply_test.go +++ b/internal/extensionserver/mutate/localreply_test.go @@ -1,6 +1,7 @@ package mutate import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -11,6 +12,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 +221,84 @@ 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 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%%"}, + } + 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") + + // 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")) + }) +}