-
Notifications
You must be signed in to change notification settings - Fork 8
FRAME_ANCESTORS: embed the admin console, and nothing else - #40
FRAME_ANCESTORS: embed the admin console, and nothing else #40distronode-com wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
FRAME_ANCESTORS only unlocks framing. Cross-site iframe sessions still fail under the existing SameSite=Lax cookie — document the same-site constraint (or plan a cookie change) before operators treat this as a working embed.
Reviewed changes
Initial review of the full PR (1 commit): opt-in admin-only frame-ancestors via env, boot-time validation, and docs.
FRAME_ANCESTORSconfig + Validate — Space-separated sources; only'self'/https://host[:port]; any bad entry fails boot so a typo cannot silently widen embeddability.FrameAncestorsmiddleware — Sets CSP on the admin SPA only; empty list is a no-op (no default deny); deliberately omitsX-Frame-Options.- Wiring + tests — Applied solely around
frontend.Handler()under/admin/; config + response header tests pin unset vs configured behaviour. - Docs — CHANGELOG, DEPLOY, ARCHITECTURE describe scope and the three refusals.
⚠️ Authenticated embed needs same-site (or a cookie redesign)
CSP is necessary but not sufficient. Admin sessions are SameSite=Lax (createSession in internal/handler/session.go). Browsers withhold Lax cookies on cross-site nested navigations and subrequests, so a parent on another registrable domain can frame /admin/ after this PR and still get an unauthenticated shell (and no session on /v1 calls). Same-site parents (shared eTLD+1) and 'self' work; true third-party consoles do not without SameSite=None + Secure (and a CSRF story that already partially exists via SameOriginCheck) or a partitioned-cookie approach. The PR never states that limit, so operators can configure a green CSP and still hit a dead embed.
Technical details
# Document same-site embed constraint (or extend cookie model) ## Affected sites - `internal/handler/session.go` — `SameSite: http.SameSiteLaxMode` on `calnode_session` - `DEPLOY.md` / `CHANGELOG.md` / `docs/ARCHITECTURE.md` — feature described as embed-ready without cookie caveats - OAuth-in-iframe is a secondary footgun (IdP pages often deny framing); optional to mention ## Required outcome - Operators reading DEPLOY/ARCHITECTURE understand that working authenticated embeds require the parent to be **same-site** with `BASE_URL` (or `'self'`), unless/until cookies change - If third-party embed is an intentional goal, that is a separate design (SameSite=None + Secure, CSRF, third-party cookie / CHIPS deprecation) — not silent fallout of this CSP-only PR ## Suggested approach (optional) - Prefer documenting the same-site constraint in this PR; do not flip SameSite here without an explicit CSRF/third-party plan
Grok | X
Second of the features split out of Calnode#30. FRAME_ANCESTORS is a space-separated list of origins allowed to embed /admin/ in a frame; when set, the admin routes send `Content-Security-Policy: frame-ancestors <list>`. Unset, nothing is sent and /admin/ behaves exactly as it does today. Three deliberate refusals, each of which is the interesting half: ⛔ Scoped to the admin SPA, and it must stay that way. The public booking pages set `frame-ancestors 'none'` plus `X-Frame-Options: DENY` in their own handlers and this never reaches them. They are unauthenticated pages that collect names, emails and card details, and clickjacking one is worth more to an attacker than framing a console nobody can open without a session. ⛔ An entry that is not `https://host[:port]` or `'self'` stops the process booting rather than being dropped. A browser discards a source list it cannot parse, so a typo would leave the admin UI MORE embeddable than leaving the setting unset — the one failure mode a validation error must not have. ⛔ No X-Frame-Options is sent beside the CSP. That header has no allow-list form (its ALLOW-FROM was implemented by one browser and is dead), so the only value it could carry is SAMEORIGIN, which every browser that reads it applies INSTEAD of honouring frame-ancestors — breaking the embedding this exists to enable. With the list empty the wrapped handler sends no frame header at all rather than a default deny, because adding one would be a behaviour change smuggled in on an opt-in setting. TestAdminSPA_sendsNoFrameHeadersWhenUnset pins that. ⛔ One caveat the CSP cannot express, recorded in DEPLOY.md, ARCHITECTURE §786 and on the config field: calnode_session is SameSite=Lax, so a browser withholds it from a subresource request made by a CROSS-SITE parent. A console on an unrelated registrable domain gets exactly the frame it asked for and a login screen inside it, with nothing in the response saying why. Same-site parents ('self', or a host under BASE_URL's domain) are the working case. Relaxing that means SameSite=None on the session cookie, which withdraws the CSRF protection Lax gives every other route, so it is not offered as a setting. Mutation-checked rather than assumed: with the Set call replaced by a discard, TestAdminSPA_frameAncestorsWhenConfigured and ...OnSPAFallback both fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dc76391 to
6177596
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review: docs-only amendment on 6177596 documenting the same-site embed constraint raised before. No behavior or middleware changes.
- DEPLOY
FRAME_ANCESTORSrow — States same-site only in practice: Lax session cookie means a cross-site parent gets the frame and the login screen. - ARCHITECTURE §9 — Adds a ⛔ paragraph beside the CSP refusals: CSP is half the story;
SameSite=Noneis deliberately not offered because it drops Lax CSRF cover. FrameAncestorsfield comment — Same constraint on the config surface operators and future editors read first.
Prior same-site documentation threads resolved against this head.
Grok | X
Second of the features split out of #30.
FRAME_ANCESTORSis a space-separated list of origins allowed to embed/admin/in a frame. When set, the admin routes sendContent-Security-Policy: frame-ancestors <list>. Unset, nothing is sent and/admin/behaves exactly as it does today.Space-separated rather than comma-separated because the value goes into a CSP source list verbatim, so it reads the same in the environment as it does in the header.
The three refusals, which are the interesting half
⛔ Scoped to the admin SPA, and it has to stay that way. The public booking pages set
frame-ancestors 'none'plusX-Frame-Options: DENYin their own handlers (book.go,manage_handler.go,tracking_settings.go'spublicCSP) and this middleware never reaches them. They are unauthenticated pages that collect names, emails and card details; clickjacking one is worth more to an attacker than framing a console nobody can open without a session.⛔ A malformed entry stops the process booting rather than being dropped. A browser discards a source list it cannot parse, so a single typo would leave the admin UI more embeddable than leaving the setting unset. That is the one failure mode a validation error must not have, so
Validaterefuseshttp://on a public host, a wildcard, a path, a query, credentials,'none','unsafe-inline', and a bare scheme. One bad entry beside a good one still fails: a half-applied source list is a policy nobody wrote.⛔ No
X-Frame-Optionsis sent beside the CSP. That header has no allow-list form — itsALLOW-FROMwas implemented by one browser and is dead — so the only value it could carry here isSAMEORIGIN, which every browser that reads it applies instead of honouringframe-ancestors. Setting it would break the embedding this exists to enable. Every browser that can frame anything today supportsframe-ancestors.No default deny
With the list empty the wrapped handler sends no frame header at all, which is what
/admin/has always sent. It does not add a default deny, because that would be a behaviour change smuggled in on an opt-in setting, and someone framing the console today for their own reasons would find it broken by an upgrade they did not opt into.TestAdminSPA_sendsNoFrameHeadersWhenUnsetpins it.Verification
Mutation-checked rather than assumed: with
w.Header().Set("Content-Security-Policy", policy)replaced by a discard,TestAdminSPA_frameAncestorsWhenConfiguredandTestAdminSPA_frameAncestorsOnSPAFallbackboth fail. Restored, they pass.go vet ./...andgo test ./...are both clean on this branch — 26 packages, zero failures.Nine files, +283/−1. No schema, no new dependency, no change to any public surface.
🤖 Generated with Claude Code