Skip to content

fix(caddy): render args.Headers as one directive per line, quote values#300

Merged
Cre-eD merged 2 commits into
mainfrom
fix/caddy-headers-render
May 29, 2026
Merged

fix(caddy): render args.Headers as one directive per line, quote values#300
Cre-eD merged 2 commits into
mainfrom
fix/caddy-headers-render

Conversation

@Cre-eD

@Cre-eD Cre-eD commented May 29, 2026

Copy link
Copy Markdown
Contributor

Summary

The args.Headers rendering path in the per-stack Caddyfile template has two coupled bugs that combine to make config.headers: unusable from stack yaml. Surfaced when a consumer tried to ship a multi-header response baseline (XFO + CSP + Permissions-Policy etc.).

Bugs

Bug 1 — same-line collapse

simple_container.go:685 (and :696 for the Prefix variant) contains:

    header_down Server nginx ${addHeaders}

The first entry of ${addHeaders} rendered as header_down X-... lands on the SAME LINE as header_down Server nginx, producing:

    header_down Server nginx header_down X-Frame-Options DENY
    header_down X-Content-Type-Options nosniff

Caddy rejects line 1 with too-many-args → Caddyfile fails to load → deploy outage on first apply of any stack that sets config.headers:.

Bug 2 — unquoted values

return fmt.Sprintf("header_down %s %s", h.Key, h.Value)

Caddyfile is whitespace-tokenized; multi-word values (CSP, Permissions-Policy, multi-directive HSTS, anything with spaces or semicolons) decompose into extra arguments and crash the parser.

Fix

  • Build addHeaders with a leading \n when non-empty so the first entry starts a new line.
  • Use %q on the value — multi-token strings survive Caddy's tokenizer. Single-token values ("DENY") get harmless surrounding quotes, still valid Caddyfile.
  • Sort entries by header name before emission so output is byte-stable across runs.

Tests

Four new regression tests; all four pass. Broader pkg/clouds/pulumi/kubernetes/... suite stays green.

Compatibility

A grep across the codebases I have access to found zero existing consumers of config.headers:. The empty-headers output is byte-identical to before this fix, so all existing stacks are unaffected.

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 <creeed22@gmail.com>
@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

Semgrep Scan Results

Repository: api | Commit: e3eb693

Check Status Details
✅ Semgrep Pass 0 total findings (no error/warning)

Scanned at 2026-05-29 09:17 UTC

@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

Security Scan Results

Repository: api | Commit: e3eb693

Check Status Details
✅ Secret Scan Pass No secrets detected
✅ Dependencies (Trivy) Pass 0 total (no critical/high)
✅ Dependencies (Grype) Pass 0 total (no critical/high)
📦 SBOM Generated 527 components (CycloneDX)

Scanned at 2026-05-29 09:17 UTC

…pty case

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 <creeed22@gmail.com>
@Cre-eD

Cre-eD commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

3-reviewer pass (claude-opus-4-8 + codex; gemini unavailable — API key expired)

Verdict from both reviewers: no BLOCKER, ship-able. Same IMPORTANT items raised in both reviews — addressed in commit 16ee262.

Addressed

  1. %q vs Caddy's quoted-string lexer: Both reviewers noted Go's %q and Caddy's lexer agree on \" and \\ (the only escape sequences both interpret) but diverge on \n/\t/control chars. Since HTTP header values must be single-line ASCII per RFC 9110, the divergence is outside the legal-input domain. Added an explicit comment block explaining the constraint.

  2. Missing test for the escape path: New TestCaddyfileEntry_HeaderValueWithEmbeddedDoubleQuote uses a realistic CSP fragment with a literal " to lock in the \" escape behaviour. A future regression to bare %s would be caught.

  3. Missing test for Prefix template variant: New TestCaddyfileEntry_HeadersOnPrefixTemplate exercises the handle_path branch (Domain="", Prefix="api"). Same addHeaders placeholder feeds both branches; without this test the second branch had zero CI signal.

  4. Empty-headers byte-identical compatibility: New TestCaddyfileEntry_EmptyHeadersByteIdenticalToPreFix asserts the exact header_down Server nginx \n shape, locking the parent-Caddy change-hash so existing header-less stacks don't drift after the upgrade.

  5. Determinism test comment: Reworded so the two-render-equal check is clearly supplementary; the alphabetical-index assertion is the load-bearing proof.

Skipped (NIT, agreed-with rationale)

  • Cosmetic 4-space vs 6-space ragged indent in the Prefix variant — Caddy is whitespace-insensitive inside blocks (claude opus + codex both said "harmless"). Not worth a code change.
  • Empty key + dup-case key concerns — vanishingly rare, no real-world consumer.
  • Defensive validation of newline/control in header values — Go's net/http rejects them at write-time anyway and the test+comment lock in the boundary.

Test results

All 7 caddyfile tests pass:

TestCaddyfileEntry_HeadersRenderOnTheirOwnLines           PASS
TestCaddyfileEntry_MultiTokenHeaderValuesAreQuoted        PASS
TestCaddyfileEntry_HeadersAreSortedDeterministically      PASS
TestCaddyfileEntry_EmptyHeadersStillRenders               PASS
TestCaddyfileEntry_EmptyHeadersByteIdenticalToPreFix      PASS  (new)
TestCaddyfileEntry_HeaderValueWithEmbeddedDoubleQuote     PASS  (new)
TestCaddyfileEntry_HeadersOnPrefixTemplate                PASS  (new)

Broader pkg/clouds/pulumi/kubernetes/... suite green; the pre-existing provisioner failure on main is unrelated.

@Cre-eD

Cre-eD commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

End-to-end verification — PASSED

Built a branch preview and pinned a downstream consumer's deploy workflow to it on a throwaway test branch. Added three config.headers: entries to a staging stack covering: single-token (enabled), multi-token with commas + spaces (alpha=1, beta=2, gamma=3), and a CSP-style value with embedded single quotes + semicolons.

Pulumi diff from the deploy log (proves SC renders correctly)

~ simple-container.com/caddyfile-entry:
      http://<host> {
        reverse_proxy http://<svc>.<ns>.svc.cluster.local:3000 {
          header_down Server nginx
    +     header_down Content-Security-Policy-Report-Only "default-src 'self'; img-src 'self' data:; report-to sc-test"
    +     header_down X-Test-Sc-Preview-Multi "alpha=1, beta=2, gamma=3"
    +     header_down X-Test-Sc-Preview-Single "enabled"
          import handle_server_error
        }

All four fix properties confirmed in the rendered annotation:

  1. Own-line emission — each header_down on its own indented line, NOT collapsed onto header_down Server nginx.
  2. Multi-token quoting"alpha=1, beta=2, gamma=3" and the embedded-single-quote CSP value properly wrapped in "…".
  3. Deterministic sort — alphabetical (Content-Security-… < X-Test-…-Multi < X-Test-…-Single).
  4. Single-token also quoted"enabled" (still valid Caddyfile, equivalent to the bareword form).

Live curl from the test host (proves Caddy actually emits them)

$ curl -sIL https://<test-host>/ | grep -iE 'x-test-sc-preview'
x-test-sc-preview-multi: alpha=1, beta=2, gamma=3
x-test-sc-preview-single: enabled

Both custom headers land in the HTTP response after passing through Caddy.

Ready to merge.

@Cre-eD
Cre-eD merged commit 55b840e into main May 29, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants