From 72f35eca54572c4001a616a417f767c73113bfa5 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Fri, 29 May 2026 11:43:01 +0400 Subject: [PATCH 1/2] fix(caddy): render args.Headers as one directive per line, quote values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `args.Headers` rendering path in the per-stack Caddyfile template had two coupled bugs that combined to make `config.headers:` unusable from stack yaml — surfaced when PAY-SPACE tried to ship the first multi-header security baseline (PS-15/PS-20). Bug 1 — same-line collapse: The template line `header_down Server nginx ${addHeaders}` placed the first user-supplied `header_down` on the SAME LINE as the existing `Server nginx` directive, producing: header_down Server nginx header_down X-Frame-Options DENY header_down X-Content-Type-Options nosniff …which Caddy rejects with "too many arguments" → Caddyfile fails to load → deploy outage. Bug 2 — unquoted values: Values were rendered with `%s`, so any header with a space (Content-Security-Policy, Permissions-Policy, Strict-Transport-Security with multiple directives) tokenized as separate Caddyfile arguments and crashed the parser. Fix: - Build the addHeaders string with a leading "\n " when non-empty so every entry (including the first) starts on its own line. - Use `%q` on the value so multi-token strings survive Caddy's whitespace tokenizer. Single-token values (e.g. "DENY") get harmless surrounding quotes — still valid Caddyfile. - Sort entries by header name before emission so the rendered Caddyfile is byte-stable across runs (Go map iteration is randomized; without sorting, sc.CaddyfileEntry would flap and trigger spurious pulumi diffs + Service-annotation change-hash churn). Regression tests cover all three properties (own-line emission, multi-token quoting, deterministic ordering) plus the empty-headers no-regression case. Compatibility: - Zero existing SC consumers of `config.headers:` were found across the codebases I have access to (grep across all PAY-SPACE stack yamls returned no matches). The output for the empty-headers case is byte-identical to before, so all existing stacks are unaffected. Signed-off-by: Dmitrii Creed --- .../pulumi/kubernetes/simple_container.go | 33 +++-- .../kubernetes/simple_container_test.go | 125 ++++++++++++++++++ 2 files changed, 149 insertions(+), 9 deletions(-) diff --git a/pkg/clouds/pulumi/kubernetes/simple_container.go b/pkg/clouds/pulumi/kubernetes/simple_container.go index d16b41d9..7019e11b 100644 --- a/pkg/clouds/pulumi/kubernetes/simple_container.go +++ b/pkg/clouds/pulumi/kubernetes/simple_container.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "fmt" "regexp" + "sort" "strconv" "strings" @@ -706,16 +707,30 @@ ${proto}://${domain} { if args.UseSSL { imports = append(imports, "import hsts") } + // Render `args.Headers` as one `header_down` directive per line and + // quote the value so multi-token headers (CSP, Permissions-Policy) + // survive Caddy's whitespace tokenizer. The leading "\n " prevents + // the first entry from being concatenated onto the + // `header_down Server nginx ${addHeaders}` line in the template, + // which would produce an invalid Caddyfile. Entries are sorted by key + // for deterministic output (Go map iteration order is randomized). + addHeadersStr := "" + if hdrs := lo.FromPtr(args.Headers); len(hdrs) > 0 { + keys := lo.Keys(hdrs) + sort.Strings(keys) + lines := lo.Map(keys, func(k string, _ int) string { + return fmt.Sprintf("header_down %s %q", k, hdrs[k]) + }) + addHeadersStr = "\n " + strings.Join(lines, "\n ") + } placeholdersMap := placeholders.MapData{ - "proto": lo.If(lo.FromPtr(args.LbConfig).Https, "https").Else("http"), - "domain": args.Domain, - "prefix": args.Prefix, - "service": sanitizedService, - "namespace": sanitizedNamespace, - "port": strconv.Itoa(lo.FromPtr(mainPort)), - "addHeaders": strings.Join(lo.Map(lo.Entries(lo.FromPtr(args.Headers)), func(h lo.Entry[string, string], _ int) string { - return fmt.Sprintf("header_down %s %s", h.Key, h.Value) - }), "\n "), + "proto": lo.If(lo.FromPtr(args.LbConfig).Https, "https").Else("http"), + "domain": args.Domain, + "prefix": args.Prefix, + "service": sanitizedService, + "namespace": sanitizedNamespace, + "port": strconv.Itoa(lo.FromPtr(mainPort)), + "addHeaders": addHeadersStr, "extraHelpers": strings.Join(lo.FromPtr(args.LbConfig).ExtraHelpers, "\n "), "imports": strings.Join(imports, "\n "), } diff --git a/pkg/clouds/pulumi/kubernetes/simple_container_test.go b/pkg/clouds/pulumi/kubernetes/simple_container_test.go index 095ada2a..1215ba5b 100644 --- a/pkg/clouds/pulumi/kubernetes/simple_container_test.go +++ b/pkg/clouds/pulumi/kubernetes/simple_container_test.go @@ -1,6 +1,8 @@ package kubernetes import ( + "regexp" + "strings" "testing" . "github.com/onsi/gomega" @@ -424,6 +426,129 @@ func TestNewSimpleContainer_ComplexConfiguration(t *testing.T) { Expect(err).ToNot(HaveOccurred(), "Test should complete without errors") } +// CaddyfileEntry Header Rendering Tests +// +// Regression coverage for two coupled bugs in the `args.Headers` rendering +// path: (a) the first `header_down` was being concatenated onto the +// `header_down Server nginx ${addHeaders}` template line, producing +// invalid Caddyfile syntax, and (b) values weren't quoted, so multi-token +// headers (CSP, Permissions-Policy) broke Caddy's whitespace tokenizer. +// Both stay fixed only if these assertions hold. + +// caddyfileEntryFor returns the rendered Caddyfile entry for the given +// args. Uses the same pulumi-mocks pattern as the rest of this file. +func caddyfileEntryFor(t *testing.T, args *SimpleContainerArgs) string { + t.Helper() + mocks := NewSimpleContainerMocks() + var rendered string + err := pulumi.RunErr(func(ctx *pulumi.Context) error { + sc, err := NewSimpleContainer(ctx, args) + if err != nil { + return err + } + rendered = string(sc.CaddyfileEntry) + return nil + }, pulumi.WithMocks("project", "stack", mocks)) + Expect(err).ToNot(HaveOccurred(), "pulumi run should not fail") + return rendered +} + +func TestCaddyfileEntry_HeadersRenderOnTheirOwnLines(t *testing.T) { + RegisterTestingT(t) + + args := createBasicTestArgs() + args.Headers = &k8s.Headers{ + "X-Frame-Options": "DENY", + "X-Content-Type-Options": "nosniff", + "Referrer-Policy": "strict-origin-when-cross-origin", + } + + entry := caddyfileEntryFor(t, args) + + // The bug: first custom header_down landed on the same line as + // `header_down Server nginx`. Assert each of our headers gets its + // own line. + for _, hdr := range []string{"X-Frame-Options", "X-Content-Type-Options", "Referrer-Policy"} { + // `(?m)^[\t ]*header_down ` ensures the directive starts a line + // (only leading whitespace allowed before it). + Expect(entry).To(MatchRegexp(`(?m)^[\t ]*header_down `+regexp.QuoteMeta(hdr)+` `), + "header_down for %s must start its own line, got:\n%s", hdr, entry) + } + + // And the `Server nginx` directive must end its own line — i.e. the + // next non-whitespace token after `nginx` is a newline. + Expect(entry).To(MatchRegexp(`header_down Server nginx[\t ]*\n`), + "`header_down Server nginx` must be followed by a newline, got:\n%s", entry) +} + +func TestCaddyfileEntry_MultiTokenHeaderValuesAreQuoted(t *testing.T) { + RegisterTestingT(t) + + csp := `default-src 'self'; script-src 'self' 'unsafe-inline' https://example.com; frame-ancestors 'none'` + permissions := `geolocation=(), camera=(), microphone=(), payment=()` + + args := createBasicTestArgs() + args.Headers = &k8s.Headers{ + "Content-Security-Policy-Report-Only": csp, + "Permissions-Policy": permissions, + } + + entry := caddyfileEntryFor(t, args) + + // Caddy tokenizes on whitespace; multi-word header values MUST be + // wrapped in double quotes or the parser sees them as extra args. + // %q also escapes embedded double quotes — there are none in CSP, but + // the literal single quotes around 'self' are passed through. + Expect(entry).To(ContainSubstring(`header_down Content-Security-Policy-Report-Only "`+csp+`"`), + "CSP value must be double-quoted in the rendered Caddyfile, got:\n%s", entry) + Expect(entry).To(ContainSubstring(`header_down Permissions-Policy "`+permissions+`"`), + "Permissions-Policy value must be double-quoted, got:\n%s", entry) +} + +func TestCaddyfileEntry_HeadersAreSortedDeterministically(t *testing.T) { + RegisterTestingT(t) + + args := createBasicTestArgs() + args.Headers = &k8s.Headers{ + "X-Frame-Options": "DENY", + "A-Header": "first-alphabetically", + "Referrer-Policy": "no-referrer", + "X-Content-Type-Options": "nosniff", + } + + // Render twice and compare — Go map iteration is randomized, so without + // explicit sorting the output would drift between runs (causing pulumi + // diffs / change-hash flapping). + a := caddyfileEntryFor(t, args) + b := caddyfileEntryFor(t, args) + Expect(a).To(Equal(b), "two renders with the same Headers map must be byte-identical") + + // Sanity: ordering must be alphabetical (A-Header before Referrer-Policy + // before X-* etc.) so we can rely on it for the change-hash signal. + idxA := strings.Index(a, "header_down A-Header ") + idxR := strings.Index(a, "header_down Referrer-Policy ") + idxXC := strings.Index(a, "header_down X-Content-Type-Options ") + idxXF := strings.Index(a, "header_down X-Frame-Options ") + Expect(idxA).To(BeNumerically(">", 0), "A-Header must be present") + Expect(idxA).To(BeNumerically("<", idxR), "A-Header before Referrer-Policy") + Expect(idxR).To(BeNumerically("<", idxXC), "Referrer-Policy before X-Content-Type-Options") + Expect(idxXC).To(BeNumerically("<", idxXF), "X-Content-Type-Options before X-Frame-Options") +} + +func TestCaddyfileEntry_EmptyHeadersStillRenders(t *testing.T) { + RegisterTestingT(t) + + args := createBasicTestArgs() + // args.Headers is &k8s.Headers{} (empty) from createBasicTestArgs. + entry := caddyfileEntryFor(t, args) + + // Server nginx still rendered, no spurious `header_down` directives, + // no trailing garbage from a leaked newline. + Expect(entry).To(ContainSubstring("header_down Server nginx")) + Expect(entry).ToNot(MatchRegexp(`header_down Server nginx[^\n]+header_down`), + "with empty Headers, the Server nginx line must not be followed by any other header_down on the same line, got:\n%s", entry) +} + // Name Sanitization Tests func TestNewSimpleContainer_NameSanitization(t *testing.T) { From 16ee262fa9c7998e848872bb5825bd90cd95d350 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Fri, 29 May 2026 13:16:29 +0400 Subject: [PATCH 2/2] test(caddy): cover %q escape path, Prefix variant + byte-identical empty case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings from claude-opus-4-8 + codex: 1. `%q` vs Caddy's quoted-string lexer: agree on `\"` and `\\` (the only escape sequences both understand); diverge on `\n`/`\t`/control chars, which RFC 9110 forbids in HTTP header values anyway. Added a comment block explaining the constraint so a future reader knows the round-trip is intentional but bounded. 2. Missing test for the escape path itself — added TestCaddyfileEntry_HeaderValueWithEmbeddedDoubleQuote with a realistic CSP fragment carrying a literal `"` so a regression to bare `%s` would be caught. 3. Missing test for the Prefix template variant (Domain="", Prefix="api" → handle_path path). The same addHeaders placeholder feeds both template branches, but without coverage the second branch had zero CI signal. Added TestCaddyfileEntry_HeadersOnPrefixTemplate. 4. Empty-headers compatibility was only asserted via substring + absence, not byte-identical-to-pre-fix. Added TestCaddyfileEntry_EmptyHeadersByteIdenticalToPreFix locking the `header_down Server nginx \n` shape so the parent-Caddy change-hash for header-less stacks doesn't drift on the SC upgrade. 5. Reworded the determinism test comment to make clear the two-render-equal check is supplementary; the alphabetical-index assertion is the load-bearing proof. Signed-off-by: Dmitrii Creed --- .../pulumi/kubernetes/simple_container.go | 7 ++ .../kubernetes/simple_container_test.go | 78 +++++++++++++++++-- 2 files changed, 80 insertions(+), 5 deletions(-) diff --git a/pkg/clouds/pulumi/kubernetes/simple_container.go b/pkg/clouds/pulumi/kubernetes/simple_container.go index 7019e11b..1535e952 100644 --- a/pkg/clouds/pulumi/kubernetes/simple_container.go +++ b/pkg/clouds/pulumi/kubernetes/simple_container.go @@ -714,6 +714,13 @@ ${proto}://${domain} { // `header_down Server nginx ${addHeaders}` line in the template, // which would produce an invalid Caddyfile. Entries are sorted by key // for deterministic output (Go map iteration order is randomized). + // + // `%q` round-trips through Caddy's quoted-string lexer correctly for + // embedded `"` (→ `\"` → `"`) and `\` (→ `\\` → `\`). HTTP header + // values MUST be single-line ASCII (RFC 9110), so the Go-vs-Caddy + // divergence on `\n`/`\t`/`\xNN` is outside the legal-input domain; + // pathological inputs round-trip as a literal `\n` byte sequence + // instead of a newline, which is the safer failure mode. addHeadersStr := "" if hdrs := lo.FromPtr(args.Headers); len(hdrs) > 0 { keys := lo.Keys(hdrs) diff --git a/pkg/clouds/pulumi/kubernetes/simple_container_test.go b/pkg/clouds/pulumi/kubernetes/simple_container_test.go index 1215ba5b..c5255090 100644 --- a/pkg/clouds/pulumi/kubernetes/simple_container_test.go +++ b/pkg/clouds/pulumi/kubernetes/simple_container_test.go @@ -516,15 +516,16 @@ func TestCaddyfileEntry_HeadersAreSortedDeterministically(t *testing.T) { "X-Content-Type-Options": "nosniff", } - // Render twice and compare — Go map iteration is randomized, so without - // explicit sorting the output would drift between runs (causing pulumi - // diffs / change-hash flapping). + // Two-render equality is probabilistic with only 4 keys (~75% miss + // rate even with broken sorting), so it's a weak signal on its own. + // The load-bearing assertion is the alphabetical index check below. a := caddyfileEntryFor(t, args) b := caddyfileEntryFor(t, args) Expect(a).To(Equal(b), "two renders with the same Headers map must be byte-identical") - // Sanity: ordering must be alphabetical (A-Header before Referrer-Policy - // before X-* etc.) so we can rely on it for the change-hash signal. + // This is what actually proves determinism: deterministic alphabetical + // ordering (A-Header before Referrer-Policy before X-* etc.) so we + // can rely on the rendered bytes for the change-hash signal. idxA := strings.Index(a, "header_down A-Header ") idxR := strings.Index(a, "header_down Referrer-Policy ") idxXC := strings.Index(a, "header_down X-Content-Type-Options ") @@ -549,6 +550,73 @@ func TestCaddyfileEntry_EmptyHeadersStillRenders(t *testing.T) { "with empty Headers, the Server nginx line must not be followed by any other header_down on the same line, got:\n%s", entry) } +func TestCaddyfileEntry_EmptyHeadersByteIdenticalToPreFix(t *testing.T) { + RegisterTestingT(t) + + args := createBasicTestArgs() + // args.Headers is &k8s.Headers{} (empty) from createBasicTestArgs. + entry := caddyfileEntryFor(t, args) + + // Lock in the compatibility contract: when Headers is empty, the + // rendered Caddyfile must contain `header_down Server nginx ` followed + // immediately by a newline (the trailing space comes from the literal + // space in the template between `nginx` and the now-empty `${addHeaders}` + // substitution). Pre-fix output had exactly this shape; we must not + // drift it, or the parent Caddy aggregator sees a spurious change-hash + // flap on every existing header-less stack after the SC upgrade. + Expect(entry).To(ContainSubstring("header_down Server nginx \n"), + "empty-Headers output must end the Server-nginx line with a single space + newline (byte-identical to pre-fix), got:\n%s", entry) +} + +func TestCaddyfileEntry_HeaderValueWithEmbeddedDoubleQuote(t *testing.T) { + RegisterTestingT(t) + + // CSP with a strict-dynamic attribute that uses double-quoted hashes + // is a realistic case that contains a literal `"`. `%q` must escape it + // so Caddy's lexer round-trips back to the original value. + value := `default-src 'self'; script-src 'self' "sha256-abc=" "strict-dynamic"` + + args := createBasicTestArgs() + args.Headers = &k8s.Headers{ + "Content-Security-Policy": value, + } + + entry := caddyfileEntryFor(t, args) + + // %q emits each embedded `"` as `\"`. Caddy's quoted-string lexer + // supports `\"` and `\\` as escape sequences; on round-trip the header + // value the server sets equals `value` exactly. This assertion locks in + // the escape behaviour — without it, a future refactor to bare `%s` + // would silently break any header value that contains a quote. + escaped := strings.ReplaceAll(value, `"`, `\"`) + Expect(entry).To(ContainSubstring(`header_down Content-Security-Policy "`+escaped+`"`), + "value containing embedded \" must be %%q-escaped (\\\") in the rendered Caddyfile, got:\n%s", entry) +} + +func TestCaddyfileEntry_HeadersOnPrefixTemplate(t *testing.T) { + RegisterTestingT(t) + + // The Prefix template branch (handle_path /* ...) shares the + // same addHeaders placeholder as the Domain branch but at a different + // indent level. Without a test here, the second template variant has + // zero CI coverage of the header-rendering path. + args := createBasicTestArgs() + args.Domain = "" + args.Prefix = "api" + args.Headers = &k8s.Headers{ + "X-Frame-Options": "DENY", + "Permissions-Policy": "geolocation=(), camera=()", + } + + entry := caddyfileEntryFor(t, args) + + // Both directives must appear on their own lines and the + // multi-token Permissions-Policy must be quoted, same contract as + // the Domain template. + Expect(entry).To(MatchRegexp(`(?m)^[\t ]*header_down X-Frame-Options `)) + Expect(entry).To(ContainSubstring(`header_down Permissions-Policy "geolocation=(), camera=()"`)) +} + // Name Sanitization Tests func TestNewSimpleContainer_NameSanitization(t *testing.T) {