Files
xarmian 8d3e389088 fix(server): a stream ends when the credential that opened it stops being valid (BUG-3007) (#1323)
* fix(server): a stream ends when the credential that opened it stops being valid (BUG-3007)

Three long-lived connections kept running after the credential that
opened them was destroyed. Measured on a live instance before any code
was written, not inferred from the handlers:

  - GET /api/v1/events        delivering 165s after a logout,  64s after a PAT revocation
  - GET /api/v1/events/stream open      180s after a logout, 120s after a PAT revocation
  - GET /api/v1/collab/{id}   open      150s after a logout, 100s after a PAT revocation

In every leg `GET /auth/me` on that same credential answered 401 while
the connection was still live, which is what rules out "the credential
was still valid" — the rival explanation a code reading cannot exclude.
The collab leg also carries a control: the same upgrade with no
Authorization header, and with a bogus bearer, both get 401, so the
socket that outlived its credential was genuinely authorized by it.

All three already revalidate SOMETHING on a jittered ~60s tick, and none
of them asked this question. `/events` revalidates the connecting user's
MEMBERSHIP, which a sign-out does not change. `/events/stream` is
stricter — `refreshUser` re-fetches by USER ID and fails closed on a
deleted or disabled user — and still misses it, because a logout
destroys a SESSION and leaves a live enabled user behind. Collab
re-fetches the ITEM and re-runs access for the principal captured at
UPGRADE time. Every one of those is a question about the PRINCIPAL; none
of them changes when the credential dies.

So there was no in-tree reference to copy. `streamCredentialStillValid`
is new, and the three ticks call it before their existing checks.

Scope is deliberately wider than "the session was destroyed": a revoked
PAT still streaming is the same defect for a CLI or MCP caller that a
destroyed session is for a browser, so the invariant is "the credential
that opened this connection is still valid". Stated here rather than
discovered later as a side effect. The PAT legs were REPRODUCED rather
than inferred from the session legs — a PAT authenticates through
`TokenAuth`, a different path with scopes and an allowed-workspace set,
so either endpoint could plausibly have failed closed on one and not the
other. Neither does.

No new plumbing: all three loops already retain the original
*http.Request for their existing checks, verified at each call site
(handlers_events.go:567, handlers_watch_events.go:420,
handlers_collab.go:344), so the credential is re-read from there rather
than threading a session id through a context that carries none today.

On the tick rather than per delivery: all three already have a tick and
a fail-closed branch to reuse, and a store lookup per delivered event
would sit on the hot path of a fan-out. One tick of bounded staleness is
what these loops already promise for membership changes.

A request carrying NO credential returns true. On the fresh-install
window and the legacy no-auth path there is nothing to invalidate, and
closing there would turn a security fix into an availability regression
on exactly the deployments least able to diagnose it. Pinned by its own
test.

Eleven tests. Six assert a stream STOPS (three connections × session
destroyed / PAT revoked); four are counterfactuals asserting it STAYS
OPEN, because all six would pass against a predicate that returned false
unconditionally — which would close every stream on its first tick and
be a worse defect than the one being fixed. The eleventh pins the
`padsess_` branch in both directions: a CLI session bearer is a SESSION,
not an API token, and validating it as a PAT would answer "no such
token" for a live session and close every CLI stream.

Three preconditions caught three false failures while these were being
written — a 503 with no watch bus, a 503 with no collab room manager,
and a 403 for a PAT-authenticated caller who was not a workspace member.
Each would have made a test pass for the wrong reason the moment the fix
landed. That is the fifth, sixth and seventh instance today of the same
shape: an absence asserted with no assertion that the thing producing it
happened.

Mutants, all confirmed to BUILD first:
  - predicate always permits -> 7 tests fail (all six STOPS + the bearer)
  - predicate always refuses -> 5 tests fail (all four STAYS OPEN + the bearer)
  - `padsess_` branch removed -> the bearer test fails, alone

Prose the change falsified, swept and corrected (CONVE-23):
`collabMembershipRevalInterval`'s doc comment and `handleCollab`'s header
both described the tick as covering revocation only; `refreshUser`'s
comment enumerated its fail-closed set without noting what it cannot
see. CLAUDE.md gains the invariant next to the two SSE endpoints.

Gates: make test exit 0, 0 FAIL; make lint 0 issues; make test-pg exit 0,
0 FAIL (run despite no SQL changing, because this is server-side and the
Postgres leg exercises a different store).

* fix(server): three-valued credential liveness, a closed default, and a read-only token door (BUG-3007, codex round 1)

Round 1 found two, and a lead ruling landed on a third — the gap I had
flagged myself.

1. HIGH — a transient store error was read as a revocation. The
   predicate was a bool, so `ValidateSession`/`ValidateToken` returning
   an error meant "invalid": one database blip would have closed every
   affected stream on the next tick, with SSE emitting `unauthorized`,
   watch SSE exiting and collab sending a policy-violation close. A
   fleet-wide reconnect storm for users whose credentials were fine.

   "The lookup says invalid" and "the lookup could not be performed" are
   opposite facts and a bool collapses them, so the predicate is
   three-valued now: valid / invalid / unknown. Unknown KEEPS the
   connection and logs — one tick of extra staleness, bounded by the
   store recovering, which is what these loops already accept for
   membership changes.

2. RULED — the default branch fails CLOSED. I had flagged that an
   unrecognised credential kind fell through to "still valid", and the
   ruling is that unknown-kind-keeps-streaming is the same defect in a
   different coat.

   Proving completeness needed a fact the request did not carry: HOW the
   principal was established. `ctxAuthKind` is set once, by the
   middleware branch that accepts the credential, and the predicate
   switches on it instead of re-sniffing the wire — so there is exactly
   one place on this server deciding whether a bearer is a session or an
   API token, rather than two that can drift. A future auth path that
   forgets to set it now ends long-lived connections loudly on the first
   tick and says why.

   The no-credential case is its own explicit branch rather than the
   default, so it cannot absorb a future kind by accident: the
   fresh-install window and the legacy no-auth path have nothing to
   revoke, and closing them would turn a security fix into an
   availability regression on the deployments least able to diagnose it.

3. MEDIUM — the tick was a database WRITE. `ValidateToken` bumps
   `last_used_at` unconditionally, so at the default 1000-connection
   admission limit this fix would have added ~16 sustained writes/second
   forever. `ValidateTokenForLiveness` is the read-only door, sharing one
   implementation with the ordinary one so the two cannot drift on what
   "valid" means.

   The cost is the smaller half of the argument. `last_used_at` is what
   an operator reads to decide whether a token is still in use before
   revoking it, and a background liveness probe is not use — letting it
   bump the field would make every idle-but-connected token look active
   and quietly defeat the audit the column exists for.

Four new tests, each pinning a branch the eleven connection tests cannot
reach on demand: unknown kind closes, no credential stays open, a store
error keeps the connection, and the liveness door leaves `last_used_at`
alone. That last one asserts the ORDINARY door moves it first — without
that precondition it would pass against a column nothing ever writes.

Mutants, all confirmed to BUILD first: default fails open again -> the
unknown-kind test fails; store error treated as revocation -> the
store-error test fails; liveness door touches `last_used_at` -> its test
fails. Each dies to exactly one test, so the three branches are pinned
independently.

Codex also confirmed clean this round: credential-kind coverage for the
three routes (OAuth and MCP tokens never reach them), the no-credential
path, session UA/IP binding, and the collab close/teardown race.

Gates: make test exit 0, 0 FAIL; make lint 0 issues; make test-pg exit 0,
0 FAIL — the Postgres leg matters more this round, since the store now
has a second validation door.

* fix(server): an accept point that resolves a principal without recording its credential kind now fails closed (BUG-3007, codex round 2)

Round 2 found the one hole the round-1 design left: the empty-kind branch
means "no credential on the wire" and KEEPS the connection, and both MCP
accept points landed there. They resolve a user through WithCurrentUser
without going through TokenAuth, so they recorded who but not how — the
exact silent exemption the fail-closed default branch exists to prevent,
reached by a different door.

Not reachable today: no MCP route is long-lived (dispatch_http_routes.go
maps nothing under /api/v1/events or /api/v1/collab), so this is a guard
rather than an incident.

Three changes:

- The empty-kind branch now distinguishes "nobody authenticated" from "an
  accept point authenticated somebody and did not say how". The first keeps
  the connection (fresh install, legacy no-auth); the second closes it. One
  guard covers every present and future accept point without naming any.
- The MCP PAT path records `api_token`. Its credential IS a PAT in the
  Authorization header, so the existing liveness door revalidates it
  verbatim — no new machinery.
- The MCP OAuth path deliberately records nothing. An opaque fosite token
  needs an introspection-backed door that distinguishes ErrInactiveToken
  from a storage error, and writing one for a route that does not exist is
  speculative; it fails closed and the comment says why.

WithCurrentUser now documents that it cannot record an auth kind and that a
long-lived route dispatched through it will be closed, not exempted.

Three tests. The principal-without-a-kind guard and the MCP PAT kind each
die to exactly one mutant (drop the guard; drop the WithValue). The third
answers round 2's other point: the existing store-error test closes the
whole store, which is permanent, so it proves the classification and not
the promise the classification is made for — that the tick tries again.
This one breaks the seam transiently (rename the table ValidateSession
reads, ask, put it back, ask again) and ends with a revocation, so a
predicate that returned valid unconditionally after an error would fail it.
It is a fence rather than a discriminating test: the mutant only it would
kill is a latching implementation, which this tree does not have.

Gates green including make test-pg; all three new tests run under Postgres.

* test(server): the transient-outage test restores its schema on failure, and gets a Postgres leg (BUG-3007, codex round 3)

Round 3 found no P0/P1 and confirmed the enumeration this fix rests on:
every production path that reaches the three long-lived routes with a
principal now records an auth kind, and nothing sets a user on top of a
different kind. Two P2s, both about the new test and the latent MCP half.

CORRECTION to the round-2 commit message and the PR body, which claimed the
three new tests were "confirmed to run under Postgres". They ran during
`make test-pg` and they ran on SQLite: `testServer` calls
storetest.NewSQLite unconditionally, so no test built on it is ever
Postgres-backed regardless of PAD_TEST_POSTGRES_URL. The claim was false as
written and I did not check the helper before making it.

So the outage test now has a real Postgres leg, via the existing
testServerPostgres helper, sharing one body with the SQLite one so they
cannot drift. The predicate is dialect-independent but the INSTRUMENT is
not — `ALTER TABLE ... RENAME TO` and the error a missing table produces
are both dialect-specific, and "a store error classifies as unknown" would
be worth nothing on the backend it was never asked on. Verified failing-
into-passing on a real container: `make test-pg TEST_PG_PKGS=./internal/server/`
runs the leg rather than skipping it, and it passes.

The restore is now registered with t.Cleanup the moment the table is
renamed, not left to a line after four assertions that can t.Fatalf past
it. Idempotent, so the in-test restore still happens where the test wants
it. Counterfactual run with an injected mid-outage failure: the only
failure reported is the injected one, so the cleanup restored the schema.

`buildHTTPRequest` gains the note round 3 asked for: it forwards the
principal and not the credential, so a synthesized MCP request carries
`api_token` with no bearer to re-check. Unreachable today (routeTable maps
no long-lived route) and closed either way, but the fix when that changes
is to forward the credential, not to widen the predicate.

Gates: make test exit 0, 0 FAIL; make lint 0 issues; make test-pg exit 0,
0 FAIL.

* docs: the BUG-3007 invariant said a no-credential request is unaffected; a resolved principal with no auth kind is closed

Rounds 2 and 3 changed what the empty-kind branch means, and the CLAUDE.md
paragraph written in round 0 still described the old reading. "A request
carrying NO credential is unaffected" is true only when there is also no
resolved principal; a request that names a user without recording how that
user was established is now CLOSED, because an accept point that records
who without recording what leaves a connection nobody can re-check.

Also says what an operator will otherwise learn from an incident: a store
error is not a revocation and keeps the connection to the next tick.
2026-09-10 16:47:31 -04:00

253 lines
11 KiB
Go

package server
import (
"context"
"github.com/PerpetualSoftware/pad/internal/models"
)
// Exported context-key helpers for callers that need to synthesize an
// authenticated request without going through the auth middleware
// chain (e.g. the in-process MCP HTTP-handler dispatcher in
// internal/mcp/dispatch_http.go, which resolves OAuth users via the
// MCP middleware and then calls the API handler tree directly).
//
// The corresponding context keys (ctxCurrentUser, ctxIsAPIToken) are
// kept package-private so the rest of the codebase can't bypass the
// middleware accidentally — these functions are the controlled surface.
// WithCurrentUser returns ctx decorated with the resolved user, the
// same way TokenAuth / SessionAuth do during a normal authenticated
// request. Subsequent handler-tree code can reach the user via the
// existing currentUser(r) helper without any change.
//
// It does NOT record HOW the credential was established (`ctxAuthKind`,
// BUG-3007), because it cannot know: the caller resolved the principal
// by some means this package did not witness. A request carrying a user
// with no auth kind is therefore CLOSED by the long-lived-connection
// liveness predicate (`streamCredentialStillValid`) rather than exempted
// from revalidation — see the empty-kind branch in
// stream_credential_liveness.go. Nothing today dispatches a long-lived
// route through this helper; a caller that wants to must teach the
// predicate its credential's door first.
//
// Pass nil to clear (rare; mostly useful in tests).
func WithCurrentUser(ctx context.Context, user *models.User) context.Context {
return context.WithValue(ctx, ctxCurrentUser, user)
}
// WithAPITokenAuth marks ctx as authenticated via an API token, the
// same way TokenAuth does on the Bearer-token path. Required when
// dispatching through the in-process handler from the OAuth-protected
// /mcp endpoint, because the auth middleware's downstream checks
// (e.g. CSRF exemption for token-auth callers) read ctxIsAPIToken to
// distinguish from session-cookie traffic.
func WithAPITokenAuth(ctx context.Context) context.Context {
return context.WithValue(ctx, ctxIsAPIToken, true)
}
// WithTokenWorkspaceID returns ctx decorated with a workspace-scope
// hint, mirroring TokenAuth's behaviour for workspace-scoped API
// tokens. Pass an empty string to clear (overrides any previous
// scope-binding to ""; downstream readers via tokenWorkspaceID(r)
// see the same "no scope" they would for a never-set context).
//
// The MCP dispatcher uses this to forward the OAuth-token's allowed
// workspace to the handler tree where existing access-control logic
// reads it via tokenWorkspaceID(r).
func WithTokenWorkspaceID(ctx context.Context, workspaceID string) context.Context {
// Always overwrite — passing "" must clear a stale scope set
// further up the context chain. Returning ctx unchanged on the
// empty path was a bug Codex caught in PR #343 review round 4.
return context.WithValue(ctx, ctxTokenWorkspaceID, workspaceID)
}
// CurrentUserFromContext returns the user attached by WithCurrentUser
// (or by the standard auth middleware), and a boolean signalling
// whether one was present. Read-only accessor — callers that need to
// set the user must use WithCurrentUser.
//
// Exported so out-of-package callers (notably internal/mcp's HTTP
// dispatcher tests) can verify the user round-trips correctly through
// the synthesized request without reaching into private helpers.
func CurrentUserFromContext(ctx context.Context) (*models.User, bool) {
v, ok := ctx.Value(ctxCurrentUser).(*models.User)
return v, ok && v != nil
}
// IsAPITokenFromContext reports whether the request was authenticated
// via an API token (vs. a session cookie or CLI session token).
// Mirrors the package-private isAPIToken(r) helper.
func IsAPITokenFromContext(ctx context.Context) bool {
v, _ := ctx.Value(ctxIsAPIToken).(bool)
return v
}
// WithTokenScopes returns ctx decorated with the API token's
// JSON-encoded scopes string (e.g. `["read"]`, `["*"]`). Stashed by
// the MCP Bearer middleware so the in-process dispatcher
// (internal/mcp/dispatch_http.go) can enforce per-tool scope checks
// — the dispatcher bypasses the standard TokenAuth chain by setting
// WithCurrentUser directly, so the chain-level scope check at
// middleware_auth.go:TokenAuth doesn't run for synthesized requests.
//
// Without this, a PAT with scope `["read"]` could drive write MCP
// tools because the in-process request looks pre-authenticated to
// the handler tree. Codex review #369 round 1 flagged the gap.
//
// Empty string clears any previously set scope.
func WithTokenScopes(ctx context.Context, scopes string) context.Context {
return context.WithValue(ctx, ctxTokenScopes, scopes)
}
// TokenScopesFromContext returns the JSON-encoded scopes attached by
// WithTokenScopes, or "" if none. Empty maps to "unrestricted" in
// TokenScopeAllows (the legacy behaviour) — callers wanting
// strict-deny on the unset path must check the empty-string branch
// before passing it on.
func TokenScopesFromContext(ctx context.Context) string {
v, _ := ctx.Value(ctxTokenScopes).(string)
return v
}
// TokenScopeAllows is the exported wrapper around the package-private
// tokenScopeAllows: returns true iff the scopes JSON permits the given
// HTTP method against path. Public so internal/mcp/dispatch_http.go
// can re-check scopes on each synthesized tool call without
// reimplementing the policy.
//
// Policy summary (see tokenScopeAllows for the full doc):
//
// - "" or `["*"]` → allow all methods (legacy / explicit wildcard).
// - `["read"]` → allow GET / HEAD / OPTIONS only.
// - `["write"]` → allow all methods.
// - explicit `[]` → allow all methods (legacy unrestricted form).
// - unparseable / null → deny.
// - unknown scope only → deny (policy-relevant unknowns are logged).
//
// Caller is expected to be the in-process MCP dispatcher; the
// chain-level enforcement on /api/v1/* still runs through tokenScopeAllows
// directly.
func TokenScopeAllows(scopesJSON, method, path string) bool {
return tokenScopeAllows(scopesJSON, method, path)
}
// WithTokenAllowedWorkspaces returns ctx decorated with the OAuth
// token's workspace allow-list set at consent time (TASK-952). The
// list is either a set of slugs (the user's specific selection) or
// `["*"]` (the wildcard checkbox). MCPBearerAuth's OAuth path stashes
// this on every request so RequireWorkspaceAccess can gate the
// resolved workspace against the allow-list (TASK-953) before
// running the standard membership check.
//
// nil clears any previously set allow-list — distinct from setting
// an empty slice, which would deny every workspace. PAT auth never
// calls this; the helper exists for the OAuth path only.
//
// Exported so the in-process MCP dispatcher can forward the same
// allow-list onto synthesized requests via Apply, matching the
// pattern WithTokenScopes uses (sub-PR E TASK-1027 round 1).
func WithTokenAllowedWorkspaces(ctx context.Context, slugs []string) context.Context {
if slugs == nil {
return context.WithValue(ctx, ctxTokenAllowedWorkspaces, []string(nil))
}
// Defensive copy — caller mutating after the call must not
// corrupt the per-request token state.
cp := make([]string, len(slugs))
copy(cp, slugs)
return context.WithValue(ctx, ctxTokenAllowedWorkspaces, cp)
}
// WithMCPTokenIdentity stashes the bearer-token identity so the MCP
// audit middleware can record which connection drove the call.
//
// kind is the discriminator — "oauth" for fosite-issued access tokens,
// "pat" for personal access tokens. ref is the connection identifier:
//
// - For "oauth", ref is the OAuth request_id chain identifier
// (preserved across refresh-token rotations — see
// internal/store/oauth.go). One value per "connection" the user
// authorized via consent.
// - For "pat", ref is api_tokens.id.
//
// Only called from MCPBearerAuth's two branches (PAT + OAuth).
// Other auth middleware doesn't touch /mcp, so the audit middleware
// reads "" and skips the row when no MCP identity was attached
// (defense in depth — the audit middleware only mounts behind
// MCPBearerAuth, so the empty case shouldn't fire in practice).
//
// Empty kind or ref clears any previously set value, which would only
// happen in a synthesized test request.
func WithMCPTokenIdentity(ctx context.Context, kind, ref string) context.Context {
ctx = context.WithValue(ctx, ctxMCPTokenKind, kind)
ctx = context.WithValue(ctx, ctxMCPTokenRef, ref)
return ctx
}
// MCPTokenIdentityFromContext returns (kind, ref) attached by
// WithMCPTokenIdentity, or ("", "") if none was set. The audit
// middleware uses this to populate token_kind + token_ref columns.
func MCPTokenIdentityFromContext(ctx context.Context) (kind, ref string) {
k, _ := ctx.Value(ctxMCPTokenKind).(string)
r, _ := ctx.Value(ctxMCPTokenRef).(string)
return k, r
}
// TokenAllowedWorkspacesFromContext returns the token's workspace
// allow-list, or nil if none was attached. Three return shapes
// matter to callers:
//
// - nil — no allow-list set (PAT auth, or pre-TASK-952 OAuth
// tokens). Caller should NOT apply any token-level gate; rely
// on standard membership checks.
// - []string{"*"} — wildcard. Caller should not apply per-slug
// gating; standard membership applies.
// - []string{"slug-a", ...} — explicit allow-list. Caller MUST
// deny any workspace not in this set, even if the user is a
// member of it.
//
// Returns a copy of the stored slice — callers can mutate without
// risk to the per-request context value.
func TokenAllowedWorkspacesFromContext(ctx context.Context) []string {
v, _ := ctx.Value(ctxTokenAllowedWorkspaces).([]string)
if v == nil {
return nil
}
out := make([]string, len(v))
copy(out, v)
return out
}
// TokenAllowedWorkspaceSet returns the token's allow-list as a slug-set for
// filtering a multi-workspace result, or nil when no per-slug gate applies.
// It is the multi-slug companion to tokenAllowedWorkspaceMatches (single
// slug, hot path): use the matcher to gate one resolved workspace, use this
// to filter a list.
//
// Return contract mirrors TokenAllowedWorkspacesFromContext:
//
// - nil allow-list (PAT / pre-consent / local stdio) → nil set → NO filter.
// - wildcard ["*"] → nil set → NO filter.
// - explicit ["a","b"] → set{a,b}; caller MUST drop any workspace whose
// slug is not a key, even one the user is a member of.
//
// An empty (non-nil) allow-list yields an empty (non-nil) set — fail closed,
// every workspace dropped — matching tokenAllowedWorkspaceMatches. The consent
// flow rejects empty lists, so this shouldn't occur in practice.
func TokenAllowedWorkspaceSet(ctx context.Context) map[string]struct{} {
allowed := TokenAllowedWorkspacesFromContext(ctx)
if allowed == nil {
return nil
}
for _, entry := range allowed {
if entry == "*" {
return nil
}
}
set := make(map[string]struct{}, len(allowed))
for _, slug := range allowed {
set[slug] = struct{}{}
}
return set
}