diff --git a/CHANGELOG.md b/CHANGELOG.md index b496eae..a631b1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,19 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad pool could satisfy are all excluded, so the explanation never appears attached to the wrong cause. Three new/changed keys in all eight locales. +- **`FRAME_ANCESTORS`: embed the admin UI in your own console.** Space-separated origins + (`https://console.example.com 'self'`); when set, `/admin/` sends + `Content-Security-Policy: frame-ancestors `. The public booking pages are + untouched and still deny framing outright — this is about the console, not the pages + that take card details. + + Two deliberate refusals. An entry that is not `https://host[:port]` or `'self'` stops + the app booting rather than being ignored, because a browser drops a source list it + cannot parse, which would leave the admin UI *more* embeddable than the setting being + unset. And no `X-Frame-Options` is sent beside it: that header has no allow-list form, + so the only value it could carry is `SAMEORIGIN`, which browsers honour instead of the + CSP and would break the embedding this exists for. + ### Fixed - **Constraint violations are recognised by SQLite's error code rather than by its English message.** Thirteen call sites asked `strings.Contains(err.Error(), "UNIQUE diff --git a/DEPLOY.md b/DEPLOY.md index 3ad7cbb..1236239 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -36,6 +36,7 @@ This guide covers a generic Docker deploy and a step-by-step **Railway** deploy | `LITESTREAM_REPLICA_URL` | recommended | — | Enables continuous SQLite backup (see §6). | | `COOKIE_SECURE` | no | https→true | Override cookie Secure flag; defaults from `BASE_URL` scheme. | | `TRUSTED_PROXY_CIDRS` | no | — | Comma-separated CIDRs (a bare address = one host) whose `X-Forwarded-For` is believed when keying per-IP rate limits, e.g. `10.0.0.0/8`. Include a fronting CDN's own ranges so the walk steps over its edge and lands on the visitor. Unset ⇒ the header is ignored and the limit keys on the TCP peer, so behind a CDN every visitor shares one bucket. **Only list networks you control**: anything in the list can name any client IP it likes. Single-value vendor headers (`CF-Connecting-IP`, `X-Real-IP`) are never read, from any peer. | +| `FRAME_ANCESTORS` | no | — | **Space**-separated origins allowed to embed the **admin UI** in a frame, e.g. `https://console.example.com 'self'`. Each entry must be `https://host[:port]` or `'self'` — anything else and **the app refuses to start**, because browsers drop a policy they cannot parse. Does not affect the public booking pages, which always deny framing. ⛔ **Same-site only in practice**: `calnode_session` is `SameSite=Lax`, so a cross-site parent can frame `/admin/` and still never be sent the cookie — it gets the login screen inside the frame. Use `'self'` or a host sharing `BASE_URL`'s registrable domain. | | `LOG_LEVEL` | no | `info` | `debug`/`info`/`warn`/`error`. | ¹ Email is optional to boot, but bookings won't send confirmations until SMTP is configured (env **or** the admin UI). Precedence is **env var > DB setting > default**. diff --git a/cmd/calnode/main.go b/cmd/calnode/main.go index 29a38aa..115a695 100644 --- a/cmd/calnode/main.go +++ b/cmd/calnode/main.go @@ -53,6 +53,13 @@ func main() { bi := buildinfo.Get() logger.Info("starting calnode", "version", bi.Version, "commit", bi.Commit, "build_time", bi.BuildTime, "dirty", bi.Dirty) + // Config whose wrong value is worse than its absence is checked here rather than + // tolerated at request time — see (*config.Config).Validate. + if err := cfg.Validate(); err != nil { + logger.Error("invalid configuration", "error", err) + os.Exit(1) + } + if cfg.GoogleClientID != "" { slog.Info("Google OAuth configured", "client_id_prefix", cfg.GoogleClientID[:20]) } else { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e91d5db..f45889d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -783,6 +783,33 @@ as the desired state: strict default and relaxes only when head code-injection is configured (broad `https:` or the operator's `tracking_csp_allow`). Don't re-hardcode the CSP on the `book`/`manage` handlers — route it through `publicCSP`. +9. **`FRAME_ANCESTORS` is the admin SPA's only, and it must stay that way.** Set it + (space-separated `https://host[:port]` / `'self'`) and the handler under `/admin/` + sends `Content-Security-Policy: frame-ancestors ` so an operator can embed the + console in their own tooling. `internal/server`'s `FrameAncestors` middleware wraps + `frontend.Handler()` and nothing else: the public booking pages keep + `frame-ancestors 'none'` + `X-Frame-Options: DENY` unconditionally, because they are + unauthenticated pages collecting names, emails and card details and clickjacking one + is worth more than framing a console nobody reaches without a session. + ⚠️ **Unset sends no frame header at all, which is what `/admin/` has always sent** — + the SPA is framable by default. This setting deliberately does *not* add a default + deny, since an opt-in flag must not smuggle in a behaviour change; + `TestAdminSPA_sendsNoFrameHeadersWhenUnset` pins the current answer so changing it is + a decision. No `X-Frame-Options` is sent beside the CSP either: that header has no + allow-list form (`ALLOW-FROM` is dead), so the only value it could carry is + `SAMEORIGIN`, which browsers apply *instead of* the CSP and would break the embedding. + An entry that isn't `https://host[:port]` or `'self'` fails `config.Validate()` and the + process **refuses to start** — a browser drops a source list it cannot parse, so a + typo would otherwise leave `/admin/` more embeddable than with the setting unset. + ⛔ **The CSP is only half of what an embed needs, and the other half is not + configurable.** `calnode_session` is `SameSite=Lax` (`session.go`), so a browser does + not send it on a subresource request from a **cross-site** parent. A console on an + unrelated eTLD+1 may therefore frame `/admin/` after this setting and still get the + login screen inside the frame, forever — the header permits the embed and the cookie + declines to authenticate it. Working embeds are same-site: `'self'`, or a parent + sharing `BASE_URL`'s registrable domain. Relaxing that means `SameSite=None; Secure` + on the session cookie, which removes the CSRF protection the `Lax` default provides + for every other route, so it is deliberately not offered as a setting. --- diff --git a/internal/config/config.go b/internal/config/config.go index 8e19f05..bf63157 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,7 +1,9 @@ package config import ( + "fmt" "log/slog" + "net/url" "os" "strconv" "strings" @@ -67,6 +69,28 @@ type Config struct { // walk steps over its edge address and lands on the visitor. TrustedProxyCIDRs []string + // FrameAncestors lists the origins allowed to embed the admin SPA in a frame, as a + // Content-Security-Policy frame-ancestors source list. Space-separated, matching the + // CSP syntax it becomes. Empty (the default) ⇒ nothing is sent and /admin/ behaves + // exactly as it did. Each entry must be `https://host[:port]` or `'self'`; anything + // else fails Validate and the process refuses to start, because a directive the + // browser cannot parse is a directive that silently allows everything. + // + // Only the admin SPA is affected. The public booking pages keep their + // `frame-ancestors 'none'` + `X-Frame-Options: DENY` unconditionally — those are + // unauthenticated pages that take payment details, and no operator convenience is + // worth making them embeddable. + // + // ⛔ Permitting the frame is not the same as making it work, and this setting is only + // the first half. calnode_session is SameSite=Lax, so a browser withholds it from a + // subresource request made by a CROSS-SITE parent: an operator who lists a console on + // an unrelated registrable domain gets the frame they asked for and a login screen + // inside it, with nothing in the response explaining why. Same-site parents ('self', + // or a host under BASE_URL's registrable domain) are the working case. The fix would + // be SameSite=None on the session cookie, which withdraws the CSRF protection Lax + // gives every other route, so it is not on offer here. + FrameAncestors []string + // DemoMode turns this instance into a public, self-resetting demo: seeds sample // data on every boot (there's no persistent volume, so every boot is a fresh DB), // disables calendar/Zoom connect, serves a disallow-all robots.txt, and exposes @@ -106,6 +130,9 @@ func Load() *Config { EmbedAllowedOrigins: splitCSV(getEnv("EMBED_ALLOWED_ORIGINS", "")), DataDir: getEnv("DATA_DIR", "data"), TrustedProxyCIDRs: splitCSV(getEnv("TRUSTED_PROXY_CIDRS", "")), + // Space-separated, not comma: the value goes into a CSP source list verbatim, so + // it reads the same in the env var as it does in the header. + FrameAncestors: strings.Fields(getEnv("FRAME_ANCESTORS", "")), } cfg.EncryptionKey = os.Getenv("CALNODE_ENCRYPTION_KEY") @@ -121,6 +148,60 @@ func Load() *Config { return cfg } +// Validate reports the configuration errors an operator has to fix before the process +// can safely serve traffic. Called from main after Load; a non-nil error is fatal. +// +// It holds the settings whose wrong value is worse than their absence. A malformed CSP +// directive is the example: browsers drop a source list they cannot parse, so the admin +// UI would end up MORE embeddable than with the setting unset, and nothing in the +// response would say so. +func (c *Config) Validate() error { + for _, origin := range c.FrameAncestors { + if err := validFrameAncestor(origin); err != nil { + return fmt.Errorf("FRAME_ANCESTORS: %w", err) + } + } + return nil +} + +// validFrameAncestor accepts 'self' or an https origin with no path, credentials, query +// or fragment. +// +// Wildcards are deliberately refused even though CSP allows them. `https://*.example.com` +// trusts every host any subdomain of that name ever points at, including one taken over +// later; an operator who needs two hosts can name two hosts. Plain http is refused for +// the same reason the admin session cookie is Secure — the framing page would be able to +// read nothing, but its own compromise becomes a foothold. +func validFrameAncestor(origin string) error { + if origin == "'self'" { + return nil + } + if strings.HasPrefix(origin, "'") { + // 'none', 'unsafe-inline' and friends are keywords this setting has no use for: + // 'none' is not "unset" (see FrameAncestors) and the rest are not source + // expressions at all. Refusing them keeps the accepted grammar one line long. + return fmt.Errorf("%q is not a supported keyword; use 'self' or an https:// origin", origin) + } + u, err := url.Parse(origin) + switch { + case err != nil: + return fmt.Errorf("%q is not a URL: %w", origin, err) + case u.Scheme != "https": + return fmt.Errorf("%q must use https://", origin) + case u.Host == "": + return fmt.Errorf("%q has no host", origin) + case strings.Contains(u.Host, "*"): + return fmt.Errorf("%q must name one host, not a wildcard", origin) + case u.User != nil: + return fmt.Errorf("%q must not carry credentials", origin) + case u.Path != "" && u.Path != "/": + return fmt.Errorf("%q must be an origin, with no path", origin) + case u.RawQuery != "" || u.Fragment != "": + return fmt.Errorf("%q must be an origin, with no query or fragment", origin) + } + return nil +} + func parseLogLevel(s string) slog.Level { switch s { case "debug": diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 990f1e4..4b727c4 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -134,3 +134,77 @@ func TestLoad_dataDir(t *testing.T) { t.Errorf("DataDir = %q; want /var/lib/calnode", cfg.DataDir) } } + +// --------------------------------------------------------------------------- +// FRAME_ANCESTORS +// --------------------------------------------------------------------------- + +func TestLoad_frameAncestorsIsSpaceSeparated(t *testing.T) { + t.Setenv("FRAME_ANCESTORS", " https://console.example.test 'self' ") + + cfg := config.Load() + + if len(cfg.FrameAncestors) != 2 { + t.Fatalf("FrameAncestors = %#v; want 2 entries", cfg.FrameAncestors) + } + if cfg.FrameAncestors[0] != "https://console.example.test" || cfg.FrameAncestors[1] != "'self'" { + t.Errorf("FrameAncestors = %#v; want the two sources unchanged", cfg.FrameAncestors) + } + if err := cfg.Validate(); err != nil { + t.Errorf("Validate() = %v; want nil", err) + } +} + +func TestLoad_frameAncestorsDefaultsToEmpty(t *testing.T) { + os.Unsetenv("FRAME_ANCESTORS") + + cfg := config.Load() + + if len(cfg.FrameAncestors) != 0 { + t.Errorf("FrameAncestors = %#v; want empty", cfg.FrameAncestors) + } + if err := cfg.Validate(); err != nil { + t.Errorf("Validate() = %v; want nil", err) + } +} + +// A directive the browser cannot parse is dropped whole, which would leave the admin SPA +// more embeddable than with the setting unset. Refusing to start is the only outcome that +// cannot be missed. +func TestValidate_rejectsBadFrameAncestors(t *testing.T) { + cases := map[string]string{ + "plain http": "http://console.example.test", + "no scheme": "console.example.test", + "wildcard host": "https://*.example.test", + "with a path": "https://console.example.test/admin", + "with a query": "https://console.example.test?x=1", + "credentials": "https://user:pw@console.example.test", + "none keyword": "'none'", + "unsafe keyword": "'unsafe-inline'", + "scheme only": "https://", + } + for name, value := range cases { + t.Run(name, func(t *testing.T) { + t.Setenv("FRAME_ANCESTORS", value) + if err := config.Load().Validate(); err == nil { + t.Errorf("Validate() = nil for %q; want an error", value) + } + }) + } +} + +// One bad entry beside a good one still fails: a half-applied source list is a policy +// nobody wrote. +func TestValidate_rejectsAListWithOneBadEntry(t *testing.T) { + t.Setenv("FRAME_ANCESTORS", "https://good.example.test http://bad.example.test") + if err := config.Load().Validate(); err == nil { + t.Error("Validate() = nil; want an error naming the http entry") + } +} + +func TestValidate_acceptsAPortAndATrailingSlash(t *testing.T) { + t.Setenv("FRAME_ANCESTORS", "https://console.example.test:8443 https://other.example.test/") + if err := config.Load().Validate(); err != nil { + t.Errorf("Validate() = %v; want nil", err) + } +} diff --git a/internal/server/frameancestors_test.go b/internal/server/frameancestors_test.go new file mode 100644 index 0000000..f586794 --- /dev/null +++ b/internal/server/frameancestors_test.go @@ -0,0 +1,59 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/calnode/calnode/frontend" +) + +// serveAdminSPA runs a request through the admin SPA handler wrapped exactly as New +// wraps it, and returns the response. +func serveAdminSPA(origins []string, path string) *httptest.ResponseRecorder { + h := FrameAncestors(origins)(frontend.Handler()) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) + return rec +} + +// ⚠️ This pins what /admin/ sends TODAY, which is no frame header at all: the SPA is +// framable by any site unless an operator says otherwise. FRAME_ANCESTORS deliberately +// does not change that when unset — an opt-in setting must not smuggle in a default deny +// — so this test exists to make the next person's change to it deliberate rather than +// incidental. The public booking pages are the ones that carry an unconditional DENY. +func TestAdminSPA_sendsNoFrameHeadersWhenUnset(t *testing.T) { + for _, path := range []string{"/", "/bookings", "/favicon.svg"} { + t.Run(path, func(t *testing.T) { + rec := serveAdminSPA(nil, path) + if got := rec.Header().Get("Content-Security-Policy"); got != "" { + t.Errorf("Content-Security-Policy = %q; want it absent", got) + } + if got := rec.Header().Get("X-Frame-Options"); got != "" { + t.Errorf("X-Frame-Options = %q; want it absent", got) + } + }) + } +} + +func TestAdminSPA_frameAncestorsWhenConfigured(t *testing.T) { + rec := serveAdminSPA([]string{"https://console.example.test", "'self'"}, "/") + + want := "frame-ancestors https://console.example.test 'self'" + if got := rec.Header().Get("Content-Security-Policy"); got != want { + t.Errorf("Content-Security-Policy = %q; want %q", got, want) + } + // X-Frame-Options has no allow-list form, and SAMEORIGIN would be honoured instead of + // the CSP by the browsers that read it, breaking the embedding this enables. + if got := rec.Header().Get("X-Frame-Options"); got != "" { + t.Errorf("X-Frame-Options = %q; want it absent alongside frame-ancestors", got) + } +} + +// The SPA fallback route (any client-side path) carries it too, not just the shell. +func TestAdminSPA_frameAncestorsOnSPAFallback(t *testing.T) { + rec := serveAdminSPA([]string{"'self'"}, "/settings/video") + if got := rec.Header().Get("Content-Security-Policy"); got != "frame-ancestors 'self'" { + t.Errorf("Content-Security-Policy = %q; want frame-ancestors 'self'", got) + } +} diff --git a/internal/server/middleware.go b/internal/server/middleware.go index 0f5c412..a1f970f 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -240,6 +240,41 @@ func (rl *rateLimiter) cleanup() { } } +// FrameAncestors returns middleware that sets `Content-Security-Policy: +// frame-ancestors ` on the responses it wraps, so an operator can embed the +// admin SPA in their own console. An empty list is a pass-through. +// +// ⛔ Scoped to the admin SPA on purpose, and it must stay that way. The public booking +// pages set `frame-ancestors 'none'` plus `X-Frame-Options: DENY` in their own handlers +// (book.go, manage_handler.go, tracking_settings.go's publicCSP) and this must never +// reach them: they are unauthenticated pages that collect names, emails and card +// details, and clickjacking one is worth more to an attacker than framing an admin UI +// nobody can reach without a session. +// +// No `X-Frame-Options` is set beside the CSP. That header has no allow-list form — its +// `ALLOW-FROM` was only ever implemented by one browser and is dead — so the only value +// it could carry here is `SAMEORIGIN`, which every browser that reads it would apply +// INSTEAD of honouring the CSP, breaking the embedding this exists to enable. Every +// browser that can frame anything today supports frame-ancestors. +// +// ⚠️ With the list empty the wrapped handler sends no frame header at all, which is what +// /admin/ has always sent: this middleware does not add a default deny, because that +// would be a behaviour change smuggled in on an opt-in setting. See +// TestAdminSPA_sendsNoFrameHeadersWhenUnset, which pins it. +func FrameAncestors(origins []string) func(http.Handler) http.Handler { + if len(origins) == 0 { + return func(next http.Handler) http.Handler { return next } + } + policy := "frame-ancestors " + strings.Join(origins, " ") + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Set before next writes: a header added after the first Write is dropped. + w.Header().Set("Content-Security-Policy", policy) + next.ServeHTTP(w, r) + }) + } +} + // remoteIP returns the IP a per-IP limit keys on. // // By default that is the TCP-level remote address, stripped of its port, and the diff --git a/internal/server/server.go b/internal/server/server.go index 460b46f..a0825e4 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -533,7 +533,10 @@ func New(ctx context.Context, cfg *config.Config, db *sql.DB, logger *slog.Logge mux.Handle("GET /favicon.ico", favicon) // Admin SPA — served at /admin/* with SPA fallback for client-side routing. - adminSPA := frontend.Handler() + // FrameAncestors is applied here and nowhere else: FRAME_ANCESTORS is about embedding + // the admin console, and the public pages' own DENY must not be reachable from a + // config flag. + adminSPA := FrameAncestors(cfg.FrameAncestors)(frontend.Handler()) mux.Handle("GET /admin", http.RedirectHandler("/admin/", http.StatusMovedPermanently)) mux.Handle("/admin/", http.StripPrefix("/admin", adminSPA))