feat(server): configurable CSP frame-src for cross-origin iframes

[adnaan]

Summary

• Adds SecurityConfig.FrameSrc []string to tinkerdown.yaml (security.frame_src).
• Threads it into both CSP write sites (API middleware + page-route inline header) via a new buildCSP() helper that also dedupes the previously-duplicated CSP literal.
• When set, emits frame-src 'self' <origin> ...; when empty, CSP is byte-identical to v0.1.15.

Why

The docs site needs to embed a separately-deployed LiveTemplate app (lt-landing-demo.fly.dev) on the landing page. Same-origin reverse-proxy embedding doesn't work — the LiveTemplate client auto-derives its WebSocket URL from window.location, which inside a same-origin iframe points at the docs host (no matching session). Cross-origin iframe lets the embedded app's client talk to its own deployment, but the current default CSP (default-src 'self') blocks the frame load until frame-src is configured.

The opt-in gate keeps the safe default (no cross-origin iframes) while letting site authors carefully widen it per-origin.

Test plan

• Unit: TestBuildCSP covers nil / empty / single / multi-origin
• Unit: TestSecurityHeadersMiddleware_FrameSrcEmittedFromConfig verifies the header path
• Unit: TestSecurityHeadersMiddleware_NoFrameSrcByDefault asserts the byte-identical default
• CI-scope full suite: go test -race -tags=ci -skip='E2E|e2e' ./... green locally
• After merge: cut v0.1.16, repin docs Dockerfile, set security.frame_src: ["https://lt-landing-demo.fly.dev"] in docs tinkerdown.yaml, swap landing iframe src to the cross-origin URL, drop the now-unused /demo/counter/ proxy route, deploy + verify

🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR adds a security.frame_src config knob that appends origins to a CSP frame-src directive, enabling cross-origin iframe embedding without touching the safe default. The core refactor (extracting buildCSP() to eliminate a duplicated CSP literal across two write sites) is the right approach and the implementation is clean.

---

Security — Input validation (actionable)

FrameSrc entries are emitted verbatim into the CSP header value with no sanitization:

csp += "; frame-src 'self' " + strings.Join(frameSrc, " ")

An entry containing ;, \n, or \r can inject additional CSP directives or—worse—additional HTTP response headers (HTTP header injection). For example:

security:
  frame_src:
    - "https://evil.example.com; script-src *"

…would produce frame-src 'self' https://evil.example.com; script-src *, silently widening script permissions.

Recommendation: validate each entry at config load time. A minimal guard that covers all realistic attack vectors:

func (c SecurityConfig) Validate() error {
    for _, origin := range c.FrameSrc {
        if strings.ContainsAny(origin, ";\r\n") {
            return fmt.Errorf("security.frame_src entry %q contains an invalid character", origin)
        }
        if _, err := url.ParseRequestURI(origin); err != nil {
            return fmt.Errorf("security.frame_src entry %q is not a valid URL: %w", origin, err)
        }
    }
    return nil
}

Call this alongside the existing ValidateOutputs / ValidateWebhooks calls at startup. This also gives operators a clear error instead of a silently broken CSP.

---

Performance — Minor

buildCSP() is called on every request in ServeHTTP (server.go:398). The middleware path pre-computes it once at construction (middleware.go:85), which is the better pattern. Since FrameSrc is immutable after config load, caching the result on the Server struct (or computing it once in NewWithConfig) would make the two code paths consistent and avoids repeated string allocation on the hot page-serving path.

// in Server struct
csp string

// in NewWithConfig
s.csp = buildCSP(cfg.Security.FrameSrc)

// in ServeHTTP
w.Header().Set("Content-Security-Policy", s.csp)

This is a minor concern given CSP construction is just string concatenation, but worth aligning with the middleware pattern.

---

Code quality

• buildCSP extraction: clean deduplication of the previously identical CSP literal. Good call.
• Struct as value type: Security SecurityConfig (not a pointer) is correct — zero value gives the safe default with no extra nil checks needed.
• omitempty on SecurityConfig: has no practical effect for yaml marshaling of a value-type struct, but it's harmless and signals intent.
• CSP semantics: X-Frame-Options: DENY (prevents this page from being framed) and frame-src (controls what this page can frame) are orthogonal — keeping DENY while widening frame-src is correct.

---

Test coverage

The test suite is solid:

• TestBuildCSP covers nil, empty slice, single, and multi-origin cases.
• TestSecurityHeadersMiddleware_FrameSrcEmittedFromConfig and _NoFrameSrcByDefault cover the middleware header path.

One gap: no test for an entry containing a ; or \n. This would naturally be added as part of a SecurityConfig.Validate() test.

---

Summary

Blocker	Input validation: verbatim emission of FrameSrc entries enables CSP directive injection
Minor	Pre-compute CSP in ServeHTTP path to match middleware pattern
Looks good	buildCSP deduplication, struct design, CSP/X-Frame-Options semantics, test coverage

The validation gap is the only thing I'd want addressed before merge — the rest of the implementation is well-reasoned and the PR description clearly explains the trade-offs.

[Copilot]

Pull request overview

Adds a configurable security.frame_src setting to allow embedding cross-origin iframes while keeping the existing default CSP unchanged when the setting is unset, and centralizes CSP construction in a shared helper.

Changes:

• Introduces SecurityConfig.FrameSrc in tinkerdown.yaml (security.frame_src) for opt-in CSP frame-src origins.
• Deduplicates CSP header construction into a new buildCSP() helper used by both the API security middleware and non-API page responses.
• Adds unit tests covering buildCSP() and verifying frame-src emission/omission behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
internal/server/server.go	Threads configured frame_src into the non-API response CSP via buildCSP().
internal/server/middleware.go	Updates security middleware to accept frameSrc and introduces buildCSP() helper.
internal/server/middleware_test.go	Adds unit tests for buildCSP() and middleware CSP behavior.
internal/config/config.go	Adds SecurityConfig and wires it into the top-level config schema.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Code Review

PR: feat(server): configurable CSP frame-src for cross-origin iframes
Overall: Well-scoped, clearly motivated change. The security validation, fail-fast startup check, and de-duplication of the CSP literal are all good instincts. A few things worth addressing before merge.

---

What this does

Adds security.frame_src to tinkerdown.yaml, threads it through both CSP-emission sites (API middleware + ServeHTTP direct path) via a new buildCSP() helper, and validates entries at config-load time to prevent header injection.

---

Issues

1. buildCSP() is called twice in NewWithConfig — one call is wasted

In server.go, NewWithConfig stores buildCSP(cfg.Security.FrameSrc) in s.csp and passes cfg.Security.FrameSrc to SecurityHeadersMiddleware(), which also calls buildCSP() internally. Both resulting strings are identical and immutable. The s.csp field could simply reuse the pre-built string from the middleware, or the middleware could accept a pre-built string instead of a slice. As written this is just two redundant string-builds at startup, but the field duplication (Server.csp vs the middleware closure's copy) could confuse someone maintaining this later.

2. Wildcard origins pass validation undocumented

url.Parse("https://*.example.com") succeeds in Go — Scheme is https, Host is *.example.com — so wildcard CSP origins would be accepted by Validate(). CSP itself supports wildcards like *.example.com, but the doc comment only shows exact scheme+host examples and doesn't mention wildcards. Either document that wildcards are accepted and valid, or explicitly reject them if they're outside the intended use case.

3. ServeHTTP (page-route path) isn't exercised by the new middleware tests

TestSecurityHeadersMiddleware_FrameSrcEmittedFromConfig and TestSecurityHeadersMiddleware_NoFrameSrcByDefault test the API-middleware path. The direct header-setting block in ServeHTTP (around line 399 in server.go) is a separate code path that now reads s.csp. There's no test that a Server constructed with a non-empty FrameSrc config actually emits frame-src on a page request. Worth adding, or at minimum noting in the test plan.

4. No integration test for Load() rejecting a bad config

TestSecurityConfigValidate tests Validate() directly, which is good. But nothing tests that calling Load() with a config file containing e.g. frame_src: ["https://evil.com; script-src *"] actually returns an error. Low-risk omission given the unit test coverage, but it would close the loop on the startup-failure guarantee mentioned in the code comment.

---

Nits

• SecurityHeadersMiddleware signature change is a breaking API change for any external callers. Since this appears to be an internal package (internal/server) that's fine — just worth noting if the package is ever re-exported.
• The csp struct field comment says "immutable for the server's lifetime" — accurate and useful; consider also noting it in the buildCSP godoc since that invariant depends on callers not mutating frameSrc after passing it in (Go doesn't enforce this, but strings.Join consumes the slice by value so it's safe here).
• Minor: test variable names mix tt and c across the two new test files (config_test.go uses tt, middleware_test.go uses c). Not wrong, but the project likely has a consistent convention worth following.

---

Security

The validation approach is sound — rejecting ;, \r, \n, space, and tab covers the realistic injection vectors for both CSP directive smuggling and HTTP header splitting. Requiring an absolute URL (scheme://host) is the right constraint for the intended use case. The fail-at-startup behavior is strictly better than a silently-broken CSP.

The existing 'unsafe-inline' / 'unsafe-eval' directives are inherited from before this PR and are outside its scope.

---

Summary

The core implementation is correct and safe. Issues 1 and 3 are the most worth addressing before merge — the redundant buildCSP() call is a latent confusion source, and the missing page-route test leaves a gap in the stated test plan. Issues 2 and 4 can reasonably be deferred if the team prefers to keep the PR focused.

[claude]

Code Review — PR #250: Configurable CSP frame-src

Overview

Clean, well-scoped change. buildCSP() eliminates a duplicated CSP literal, validation happens at startup so a misconfigured origin surfaces immediately, and the pre-computed csp / buildCSP(frameSrc) approach means zero per-request string allocations. Test coverage is solid across the security-relevant injection vectors.

---

Issues / Suggestions

1. http:// origins are accepted — consider enforcing HTTPS

Validate() requires a scheme, but doesn't restrict which scheme. An operator could set http://example.com and silently downgrade the security posture of the embedded content.

// After the url.Parse block:
if u.Scheme != "https" {
    return fmt.Errorf("security.frame_src entry %q must use the https scheme", origin)
}

If there's a legitimate development use-case for http://localhost, a specific carve-out for localhost/loopback is cleaner than leaving it fully open.

2. Comma is not rejected by Validate()

strings.ContainsAny blocks ;, \r, \n, space, and tab — but not ,. In HTTP, commas can serve as header-value separators in some parsing contexts. url.Parse("https://evil.com,foo") succeeds and returns Host: "evil.com,foo", which would slip through.

In practice the risk is low (Go's net/http writes CSP as a single header value), but it's cheap to add:

if strings.ContainsAny(origin, ";\r\n \t,") {

3. Redundant CSP computation for the API middleware path

NewWithConfig sets s.csp = buildCSP(cfg.Security.FrameSrc) for page routes, and separately passes cfg.Security.FrameSrc to SecurityHeadersMiddleware(...) which internally calls buildCSP again. Two identical strings are computed; neither is wrong, but one is unused for API routes.

Options (pick whichever is simpler):

• Pass s.csp into SecurityHeadersMiddleware instead of frameSrc (would need a signature change, or a pre-computed variant).
• Or accept the minor redundancy as-is — both are startup-time, not hot-path.

4. New() skips Validate() — safe today, fragile tomorrow

New() calls config.DefaultConfig() which always has an empty FrameSrc, so skipping validation is currently safe. But if DefaultConfig() ever grows a non-empty default, the startup check in Load() won't protect paths that go through New(). A defensive Validate() call in New() (ignoring or log.Fatal-ing the error) would close that gap.

---

Minor / Nits

• frame-ancestors 'none' + X-Frame-Options: DENY are both kept — intentional and correct (they're orthogonal to frame-src), but worth a one-line comment for the next reader.
• The defaultCSP constant in middleware_test.go duplicates the string a third time. It's used well (as a change-detector), but a // Do not modify without updating buildCSP note might save someone a grep.
• No operator-facing docs/changelog entry visible in the diff. If there's a CHANGELOG.md or a config-reference doc, the new security.frame_src key should appear there.

---

Summary

The core approach is sound and the injection-prevention logic is thoughtful. The HTTPS enforcement gap (item 1) is the most actionable security concern. Items 2–4 are minor. Happy to approve after item 1 is addressed (or explicitly documented as out of scope for now).

[claude]

Code Review — feat(server): configurable CSP frame-src for cross-origin iframes

Overview

This PR adds SecurityConfig.FrameSrc []string to tinkerdown.yaml, threads it into both CSP write sites via a new buildCSP() helper, and eliminates the previously-duplicated CSP literal. The change is backwards-compatible: an empty config produces a byte-identical CSP to v0.1.15.

---

What works well

• Input validation at startup is the right call. Failing loudly at Load() time rather than emitting a broken header silently is correct behavior.
• Validation is thorough. Rejecting ;, ,, CR, LF, space, and tab covers the realistic injection vectors (directive injection, header splitting, CSP token splitting). Good to see the full set.
• buildCSP() extraction is a clean refactor. The previous copy-paste of the CSP literal across two write sites was a maintenance hazard; this eliminates it correctly.
• Pre-computing the CSP at server construction (s.csp) rather than rebuilding it per-request is the right performance choice.
• Test coverage is excellent — 10+ targeted cases covering both write sites, all injection vectors, the byte-identical default, and the end-to-end Load() rejection path.

---

Issues and suggestions

Notable gap: the embeddee also needs configurable frame-ancestors

The PR solves the embedder side (frame-src on the docs site). But if lt-landing-demo.fly.dev is itself a tinkerdown deployment, it has frame-ancestors 'none' hardcoded in its CSP (and X-Frame-Options: DENY), which will block browsers from loading it as an iframe regardless of the docs site's frame-src. The browser enforces both sides.

The post-merge checklist in the PR description skips past this: it only mentions setting frame_src on the docs side. If the demo is not tinkerdown, this is fine. If it is, either:

• A SecurityConfig.FrameAncestors []string field is also needed (mirroring this PR), or
• The demo deployment needs a non-tinkerdown reverse proxy that strips/replaces X-Frame-Options and frame-ancestors.

Worth clarifying before merge so the post-merge deploy step doesn't hit a surprise.

buildCSP trusts its callers to have pre-validated input

SecurityHeadersMiddleware(frameSrc []string) accepts raw []string and passes it directly to buildCSP without re-validating. The design relies on every caller having already run SecurityConfig.Validate(). That invariant holds for the current callers (both pass cfg.Security.FrameSrc, which was validated at Load() time), but the relationship is implicit. A lightweight option is a package-level comment on buildCSP noting it expects pre-validated input — or alternatively, accepting a SecurityConfig value so the type system makes the coupling explicit.

Minor: buildCSP is invoked twice for the same config

NewWithConfig sets csp: buildCSP(cfg.Security.FrameSrc) on the server struct, and separately passes cfg.Security.FrameSrc into SecurityHeadersMiddleware() which also calls buildCSP internally. Both produce the same string from the same source, but the redundancy means there are two independent call sites to keep in sync. The API middleware path could use srv.csp directly instead:

// Instead of:
handler = SecurityHeadersMiddleware(cfg.Security.FrameSrc)(handler)

// Could be:
handler = SecurityHeadersMiddleware(srv.csp)(handler)
// or accept a pre-built string to avoid the second buildCSP call

This is cosmetic — no correctness risk today — but worth a note.

No scheme allowlist in validation

url.Parse accepts any scheme. An operator who mistypes ftp://example.com or blob: would pass validation and get a syntactically valid but probably unintended frame-src entry. Since iframes in practice only use http:// or https://, a non-blocking warning log (or a stricter reject) on non-http(s) schemes would surface typos earlier. Low priority, but a real UX improvement.

---

Summary

The implementation is solid and security-conscious. The main concern is the frame-ancestors gap — if the demo is a tinkerdown deployment, the post-merge step will require either a follow-up PR or out-of-band header manipulation on the demo side. Everything else is minor polish. Consider clarifying the embeddee situation in the PR description or test plan before merging.

🤖 Generated with Claude Code