Skip to content

Bind the gate to the canonical site key, not the Host header - #1

Merged
luthermonson merged 3 commits into
mainfrom
abi-minor-3-canonical-site-key
Sep 3, 2026
Merged

Bind the gate to the canonical site key, not the Host header#1
luthermonson merged 3 commits into
mainfrom
abi-minor-3-canonical-site-key

Conversation

@luthermonson

@luthermonson luthermonson commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Downstream half of ephpm/ephpm#449. Of the three repos that issue names, this is
the one where the change is semantic, not just a signature bump: this module
reads vhost_id() as a tenant identity inside an authorization decision.

Unblocked and re-pinned. ephpm/ephpm#448 merged to main as 691e6fef and
shipped in v0.8.9. The rev now points at c2774ab6 — the commit tagged
v0.8.9 — not at #448's PR-branch head 21a7c8a7, which never landed on main
(the PR was squash-merged) and would have become unfetchable once the branch was
deleted. No API drift between the two revs: the only changes to the consumed
crates are docs plus a !Send marker on the host-side SiteKvScope, which
neither module uses.

The change in one line

Two things that used to be one string are now two, and each does exactly one job.

value used for
site keyreq.vhost_id() pr-1 the sites access check, the OAuth state binding, the session's site claim
request hostreq.http_host() pr-1.preview.example.com the derived redirect_uri, and nothing else

Under a sites_domain_suffix those differ, and both directions matter. The site
key is the tenant boundary — the same identity that picks the per-site database
and KV keyspace — and it is not a routable authority: a redirect_uri of
https://pr-1/… goes nowhere. Getting this backwards would have been the easy
mistake, so it has its own test.

What each piece buys

A new SiteIdentity (Tenant(key) / Untenanted). The two uses of the
tenant identity want different things from an absent tenant, and collapsing them
is how you end up guessing. key() returns Option<&str> for the sites
lookup; claim() returns a string for the token, and for an untenanted request
that string is the constant ephpm_middleware::UNMATCHED_VHOST — uppercase,
therefore unspellable as a site key, therefore incapable of colliding with a real
tenant. Two different unrecognised hostnames also cannot mint two identities for
what is one and the same default document root.

Config::check_for takes Option<&str> and fails closed. With a sites
table configured, a request that matched none of the mapped vhosts is denied —
it must not inherit the top-level repo. Test: a request whose Host still
reads like a mapped site gets 403 when no vhost matched.

…but not on a single-site node. A node with no sites_dir has no virtual
hosts, so vhost_id() is None on every request. Treating that as "no
tenant, deny" would black-hole the whole site. With no sites table the
top-level target applies as before; a_single_site_node_still_logs_in_with_no_sites_table
pins it.

Deleted the local normalize_vhost on the request path. Re-normalising a
client string is a guess about what the router did; the router now just says.
What remains is normalize_site_key, which only tidies what an operator typed,
plus a new validate_site_key mirroring ePHPm's is_valid_site_key — so a
sites table still written in hostname terms (a port, an IPv6 literal) now
fails the mount at startup instead of silently never matching.
validate_vhost becomes validate_redirect_host, guarding the one place a host
still reaches an outbound URL.

session-cookie: site_param carries the canonical key, and is omitted
entirely when there is no tenant rather than filled in from the header. Its own
copy of normalize_vhost is gone too.

Breaking changes — and why they cost nothing today

  1. sites is keyed by the site key, not the request hostname. With
    sites_domain_suffix = ".preview.example.com", the key for
    pr-1.preview.example.com is pr-1. Keys that cannot be site keys now fail
    startup; dotted keys are still accepted (a deployment without a suffix really
    does key on the full name).
  2. Sessions issued before the upgrade carry the old host-shaped site claim.
    They stay signature-valid but name a tenant that no longer exists under that
    spelling. Any user with a live session is logged out once and logs in
    again; there is no migration path and none is warranted, since the sessions
    default to an eight-hour expiry.

Checked the live consumers before merging, and there are none. Swept
ephpm/switchboard, ephpm/wordpress-sample, ephpm/switchboard-infra and
ephpm/switchboard-api for [[middleware]] mounts, the module names, .so
references, sites tables and sites_domain_suffix — including
git log --all -S history sweeps for a mount that existed and was removed.
Nothing mounts this module, nothing ever has, and no sites table exists
outside this repo's own tests. switchboard-infra's StackScript writes an
ephpm.toml containing only [server] and [db.sqlite], and switchboard's
preview-app guide still tells users to "assume your preview URL is public".

So breaking change 1 has no deployed config to migrate, and breaking change
2 has no live sessions to invalidate. Both are recorded here and in the README
so the history carries them for whoever mounts this first.

That sweep also falsified a claim in this repo's own README — that switchboard
and wordpress-sample compile this in. Corrected in de94ef3 rather than left
to mislead the next reader into thinking a migration was needed.

Deliberately not done here

ephpm#396 — the verifier ignores the site claim. The issuer binds every
session to one tenant; session-cookie checks signature and expiry and never
reads the binding, so a session issued for one preview verifies on every preview
served by the same mount. That is open, it is rated CRITICAL, and this PR does
not fix it.

What this PR does is make the fix possible: before minor 3 the only identity
available to the verifier was client-controlled, so a claims["site"] == req.vhost_id() comparison was not meaningful. Now it is. I left it out because
#396 has its own design decision to make — whether a site-less token (the
"share link" story) is rejected — and folding a CRITICAL security fix into an
ABI bump makes both harder to review. It is now stated plainly in the module
docs and the README instead of being inferable from the absence of a check.

Also corrected two doc claims the new pin falsifies: the middleware KV surface
is no longer process-global (ephpm#376), and the ABI does now expose a request
scheme (minor 2) — so require_https is implementable, and is now
documented as not-implemented rather than as impossible.

Verification

cargo +nightly fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace (71 + 9 + 31 tests), cargo build --workspace --release. CI green on the re-pinned commit 3740190 and again on
de94ef3. New coverage: the sites table keyed on the router's key rather than
the header, the untenanted deny, the single-site fall-through, the
redirect_uri using the host, the state binding on the site key, and the
untenanted bucket being one identity rather than one per host.

ePHPm #448 takes the middleware ABI to minor 3, where
`Request::vhost_id()` returns `Option<&str>` carrying the router's
CANONICAL SITE KEY and `None` for a host that matched no virtual host.
Before that it was the raw `Host` header: client-controlled,
un-normalised, never absent. This module reads that value as a tenant
identity in an authorization decision, so the change is semantic, not
just a signature bump (ephpm#390, ephpm#449).

The central change is that two things which used to be one string are
now two, and each is used for exactly one job:

  * the SITE KEY selects the `sites` access check, binds the OAuth
    `state`, and becomes the session token's `site` claim. New
    `SiteIdentity` carries `Tenant(key)` vs `Untenanted` so those two
    uses can want different things from an absent tenant.
  * the REQUEST HOST (`req.http_host()`, ABI minor 2) builds the derived
    `redirect_uri` and nothing else. It has to: under a
    `sites_domain_suffix` the site key is the suffix-stripped directory
    name (`pr-1`), which is not an authority a browser can be
    redirected back to.

`Config::check_for` now takes `Option<&str>` and fails closed on `None`
when a `sites` table is configured — a request that matched none of the
mapped vhosts is not one of them, and must not inherit the top-level
target. With no `sites` table the default check still applies, which is
the single-site deployment: a node with no virtual hosts has
`vhost_id() == None` on every request, and treating that as "deny"
would black-hole the whole site.

An untenanted request's `site` claim and `state` binding are the
constant `ephpm_middleware::UNMATCHED_VHOST`. It is uppercase and so
unspellable as a site key, so it can never collide with a real tenant,
and two different unrecognised hostnames cannot mint two identities for
what is one and the same default document root.

Deleted the local `normalize_vhost` re-normalisation on the request
path: re-normalising a client string is a guess about what the router
did, and the router now just says. What is left is
`normalize_site_key`, which only tidies what an operator typed in
`sites`, plus a new `validate_site_key` mirroring ePHPm's
`is_valid_site_key` — so a `sites` table still written in request-
hostname terms (a port, an IPv6 literal) FAILS THE MOUNT instead of
silently never matching. `validate_vhost` becomes
`validate_redirect_host`, guarding the one place a host still reaches
an outbound URL.

session-cookie: `site_param` now carries the canonical site key, and is
omitted entirely when there is no tenant rather than filled in from the
header. Its local `normalize_vhost` is deleted for the same reason.

BREAKING (config): `sites` is keyed by the site key, not the request
hostname — with `sites_domain_suffix = ".preview.example.com"` the key
for `pr-1.preview.example.com` is `pr-1`. Sessions issued before the
upgrade carry the old host-shaped `site` claim and name a tenant that
no longer exists under that spelling; users log in again once. Both are
documented in the README and the module docs.

Also corrected two doc claims that the new pin falsifies: the KV
surface is no longer process-global (ephpm#376), and the ABI does now
expose a request scheme (minor 2) — so `require_https` is
*implementable*, and is documented as not implemented rather than as
impossible. And stated plainly, in both module docs and the README,
that the verifier does not check the `site` claim this issuer writes:
that is ephpm#396, still open, and minor 3 is what makes fixing it
possible.

Tests: the site key and the request host are separate fixtures
throughout (`SITE`/`HOST`). New coverage for the sites table being
keyed on the router's key and not the header, the untenanted deny, the
single-site fall-through, the redirect_uri using the host, the state
binding on the site key, and the untenanted bucket being one identity
rather than one per host.

Refs ephpm/ephpm#449
The previous rev `21a7c8a7` was ephpm#448's PR-branch head, which never
landed on main: #448 was squash-merged as `691e6fef`, so the old pin is
unreachable once the branch is deleted.

Re-pinned to `c2774ab6` — the commit tagged v0.8.9, the first published
ePHPm release carrying minor 3. Pinning the tag's commit rather than the
tag name keeps the pin immutable; pinning the release rather than the raw
merge commit means these modules build against an ABI that shipped in a
host binary operators can actually run.

No API drift between the two revs: the only changes to the consumed crates
are documentation plus a `!Send` marker on the host-side `SiteKvScope`,
which neither module uses. Manifests are byte-identical, so the lockfile
needed only the rev rewrite.
The section claimed the switchboard preview control plane and the
wordpress-sample PR-preview app compile this in. Checked before merging
the sites key-form change, because that claim is what decides whether the
change is breaking in practice: neither repo mounts it, neither ever has
in its git history, and switchboard-infra's StackScript writes an
ephpm.toml with only [server] and [db.sqlite].

switchboard's own preview-app guide says the opposite of the claim -
"assume your preview URL is public" - so the README was the outlier.

Restated as intent rather than fact, which is also the honest framing for
the sites breaking change: there is no deployed config in hostname form
to migrate.
@luthermonson
luthermonson merged commit 919449a into main Sep 3, 2026
4 checks passed
@luthermonson
luthermonson deleted the abi-minor-3-canonical-site-key branch September 3, 2026 00:33
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