Skip to content

FRAME_ANCESTORS: embed the admin console, and nothing else - #40

Open
distronode-com wants to merge 1 commit into
Calnode:mainfrom
distronode-com:feat/frame-ancestors
Open

FRAME_ANCESTORS: embed the admin console, and nothing else#40
distronode-com wants to merge 1 commit into
Calnode:mainfrom
distronode-com:feat/frame-ancestors

Conversation

@distronode-com

Copy link
Copy Markdown
Contributor

Second of the features split out of #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.

FRAME_ANCESTORS="https://console.example.com 'self'"

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' plus X-Frame-Options: DENY in their own handlers (book.go, manage_handler.go, tracking_settings.go's publicCSP) 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 Validate refuses http:// 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-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 here is SAMEORIGIN, which every browser that reads it applies instead of honouring frame-ancestors. Setting it would break the embedding this exists to enable. Every browser that can frame anything today supports frame-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_sendsNoFrameHeadersWhenUnset pins it.

Verification

Mutation-checked rather than assumed: with w.Header().Set("Content-Security-Policy", policy) replaced by a discard, TestAdminSPA_frameAncestorsWhenConfigured and TestAdminSPA_frameAncestorsOnSPAFallback both fail. Restored, they pass.

go vet ./... and go 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

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

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_ANCESTORS config + Validate — Space-separated sources; only 'self' / https://host[:port]; any bad entry fails boot so a typo cannot silently widen embeddability.
  • FrameAncestors middleware — Sets CSP on the admin SPA only; empty list is a no-op (no default deny); deliberately omits X-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

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Grok𝕏

Comment thread DEPLOY.md Outdated
Comment thread docs/ARCHITECTURE.md
Comment thread internal/config/config.go
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>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

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_ANCESTORS row — 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=None is deliberately not offered because it drops Lax CSRF cover.
  • FrameAncestors field comment — Same constraint on the config surface operators and future editors read first.

Prior same-site documentation threads resolved against this head.

Pullfrog  | View workflow run | Using Grok𝕏

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.

1 participant