fix: reject empty owner in owner-scoped cascade revocation (closes #275)

[saucam]

Closes #275.

The bug

RevokeAllActiveForOwner accepted an empty owner_user_id and passed it straight into migration 041's seed predicate, which is a plain equality:

WHERE i.owner_user_id = p_owner_user_id
  AND i.account_id    = p_account_id

identities.owner_user_id is VARCHAR(255) NOT NULL (001_init_schema), so an ownerless identity stores '' — not NULL. And ownerless is a documented posture for discovered identities, not an anomaly: identity.go calls ownership "OPTIONAL — ownerless" for them, and postgres/identity.go has an explicit (owner_user_id IS NULL OR owner_user_id = '') facet built on exactly that value.

So a blank owner matched every ownerless identity in the account and cascade-revoked each one's entire delegation subtree.

The realistic trigger is the intended caller: an IdP offboarding webhook whose user-id field arrives blank turns "revoke one departing human's agents" into a tenant-wide outage of precisely the workloads nobody is watching. Revoked credentials cannot be un-revoked.

Reproduced against a real database

With migration 042 removed, seeding two ownerless identities plus a delegated child and calling with a blank owner:

WITHOUT MIGRATION 042 -> err=<nil>, revoked 3 credential(s):
  [ownerless-a-...-jti  ownerless-b-...-jti  ownerless-a-child-...]

No error, three credentials gone — including the delegated child, so the cascade reached past the seed rows. With 042 the function raises and nothing is touched.

The fix — guards at both layers

They protect different callers, and the failure mode is unrecoverable, so both earn their place:

• internal/service/credential.go rejects an empty owner_user_id or account_id before reaching the repo.
• Migration 042 mirrors it with RAISE EXCEPTION so the guarantee survives a caller that bypasses the service — a psql session, another service, or a repo method added later.

RAISE rather than returning zero rows on purpose: a blank owner is a caller bug, and silently reporting "revoked 0 credentials" would let a broken offboarding integration look healthy indefinitely.

041 is untouched — it's already merged, so 042 CREATE OR REPLACEs the function, mirroring how 031's down restores the 029 bodies. The down migration restores 041's body and carries an explicit warning that rolling back reintroduces the hole.

Everything else in the function — the recursive walk, CYCLE guard, depth cap 50, liveness filters on the final UPDATE only, and the RETURNING projection — is carried over verbatim. Signature unchanged, so no caller changes.

Tests — migration 041's first coverage

041 shipped with none, which is the second half of #275. It's the most intricate SQL in the repo (recursive CTE + CYCLE detection + depth cap + data-modifying CTE with RETURNING), and its first execution in anger would have been its first execution ever.

tests/integration/revoke_by_owner_test.go (real Postgres):

• empty owner rejected; both ownerless credentials and the delegated child survive — verified to fail without 042
• empty account rejected (the tenant-scope half)
• happy path: owner's credential + child + grandchild revoked exactly once each, while another owner's credential and an ownerless identity in the same account are untouched

internal/service/credential_revoke_owner_test.go: the service guard, built with a nil repo deliberately — a panic there proves the guard failed to short-circuit before the DB layer. Mutation-checked: removing the guard makes them fail.

go vet clean, gofmt clean, unit suite green, full integration suite green (68s).

Not in this PR

#275 also notes the primitive is unreachable — no caller, no Server method, no hooks.go wrapper. I've deliberately left that out: the identity-scoped sibling isn't exported either (it's reached from internal deactivation flows), so exposing this one means inventing a new public API shape, and choosing the offboarding entry point — Server method? admin endpoint? webhook handler? — is a design decision rather than a bug fix.

The issue's warning was that "wiring a caller without first adding the guard is the dangerous ordering". This PR establishes the guard, so whichever entry point is chosen can now be wired safely. Happy to follow up once we've picked the shape.

🤖 Generated with Claude Code

[saucam]

Self-audit: does this change introduce the same class of bug, and does the pattern recur?

This change

• 042's body is byte-identical to 041 apart from the two guard blocks — diffed with comments stripped. The recursive walk, CYCLE guard, depth cap 50, liveness filters and RETURNING projection are carried over verbatim, so CREATE OR REPLACE can't silently change traversal semantics.
• 042.down is a faithful revert — diffs identical to 041's body.
• The guard can't reject a legitimate call. There is no valid empty-owner or empty-account invocation: the function's entire purpose is owner-scoped, account-scoped revocation. This is the failure direction that bit fix(handler): enforce DPoP sender-constraint on forward-auth verify #272 (a guard so strict it rejected every legitimate request), so it was worth checking explicitly rather than assuming.
• No test pollution. The new integration tests seed identities and live credentials into the shared testAccountID. Every other account-scoped counter in the suite is keyed by a specific id / identity_id / credential_policy_id, none count account-wide, and the full suite passes green (68s).
• Both suites mutation-checked — removing the service guard panics on the nil repo; removing migration 042 makes the empty-owner test fail with 3 credentials revoked.

Does the pattern recur elsewhere?

Swept every SQL function and every bulk destructive repo operation. The shape that matters is "a legitimately-empty string is the primary breadth selector".

Site	Selector	Verdict
revoke_credentials_cascade	p_identity_id UUID	Safe — uuid cast rejects '' (22P02)
revoke_credential_cascade	p_id UUID + TEXT account/project	Safe — the UUID PK narrows to one row; empty TEXT can only narrow further, never widen
SigningCredential.RevokeWorkload	workload TEXT, no service-layer validation	Safe by upstream construction — see below
IdentityRepository.DeactivateStaleDiscovered	source_id, documented as legitimately empty	Same shape, materially lower severity — see below

RevokeWorkload has the identical shape to the pre-fix code here: WHERE workload = ? with a VARCHAR(255) NOT NULL column and a service method that is a pure passthrough with no validation. It's safe only because Attest — the sole path reaching SigningCredentialRepository.Create — rejects an empty/whitespace workload at issuance, so no row can hold workload = '' and an empty revoke matches zero rows.

That is fail-closed, but by accident of upstream validation rather than by a local guard — precisely the property #275 criticised about the identity-scoped sibling ("protected only by accident"). Not a bug today; worth a symmetric guard if anyone touches that path. I've deliberately not bundled it here, since it isn't currently exploitable and this PR is a targeted fix.

DeactivateStaleDiscovered does take a legitimately-empty source_id (the model documents it as empty for rows ingested without a source). But the blast radius is bounded by status = 'discovered' — rows the code notes hold no credentials, so nothing cascades — plus a staleness window, and it's a reversible status flip rather than irreversible revocation. An empty sourceID sweeping the "no source" bucket is also arguably the intended semantics. Flagging as an observation, not a defect.

Conclusion

revoke_credentials_by_owner was uniquely vulnerable because it is the only function whose breadth is set by a TEXT column that has a legitimately-populated empty value. That property doesn't hold anywhere else in the schema.