Default-deny on empty resource claims when authz is active#762
Merged
Default-deny on empty resource claims when authz is active#762
Conversation
Tighten `validateClaimsSubset` and `validateClaimsSubsetBytes` so a
resource with empty/nil claims is invisible to claim-bearing callers
when authz is configured, instead of being treated as "open." This
aligns the single-resource gate with the list filter and with the
fail-safe-defaults principle (Saltzer & Schroeder; NIST SP 800-162;
OWASP Access Control). Unlabeled is not the same as public — "public"
must be expressed with an explicit positive claim that the role config
maps to all authenticated users.
Both gates become methods on `*dbService` so they can read `s.skipAuthz`.
When authz is not configured (auth-only / anonymous), `skipAuthz=true`
and the gates short-circuit to allow — claim-based authorization stays
opt-in. Super-admin still bypasses uniformly, so operators can always
reach unlabeled resources to fix or tag them. Helpers
`lookupRegistryIDWithGate`, `checkRegistryExistsWithGate`,
`resolveSourceIDsWithGate`, `replaceRegistrySourcesWithGate`,
`streamSourceRows`, and `streamRegistryRows` are likewise methods now
because they all delegate to the gates.
Existing tests that asserted the old "empty = open" behavior are
flipped to expect `ErrClaimsInsufficient`. Shadowing fixtures gain
`{sub: "claims-test-user"}` claims on the registry and sources so the
access gate passes and the test can still exercise entry-level dedup.
The `internal` managed source in the authz integration fixtures gets
a tenant-wide `{org: "acme"}` claim so writers can reference it; the
"outsider sees only internal source" sub-test is reframed as "outsider
sees no sources," which is what default-deny actually implies.
Updates `auth.md` §4 to document the new posture and the
`skipAuthz`-driven opt-in behavior.
Improves on #761
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #762 +/- ##
==========================================
- Coverage 60.57% 60.50% -0.07%
==========================================
Files 108 108
Lines 10495 10576 +81
==========================================
+ Hits 6357 6399 +42
- Misses 3585 3608 +23
- Partials 553 569 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 tasks
Add a paragraph to `auth.md` §4 spelling out what happens to rows with `claims IS NULL` when a deployment moves from "empty = open" to default-deny — they become invisible to every claim-bearing caller. Two recovery paths are called out: per-entry retagging by super-admin, and tenant-wide claims on the managed source so writers can reference it. Also notes that no automatic backfill exists by design, since that would re-introduce the "empty = caller identity" behavior the rule is built to reject. Improves on previous commit (Default-deny on empty resource claims).
Three follow-up fixes the reviewer flagged on the default-deny commit:
`claimsContain` now fails closed when a record value has an unsupported
type (nil, number, nested object, etc.) via a new `isValidClaimValue`
helper. `ValidateClaimValues` blocks such values at the API edge, but
rows persisted via direct DB writes or future sync paths could carry
them — without this guard, `toStringSet` returned an empty required
set and the matcher silently treated the requirement as
vacuously-satisfied. With the change, an entry with `{"team": null}`
is denied even if the caller has the `team` key, because the gate
refuses to match against a value type that should never have been
persisted in the first place.
`newClaimsFilterWith` is now a method on `*dbService` and short-circuits
on `s.skipAuthz`. Previously each list callsite did
`if s.skipAuthz { claimsFilter = nil }` after building the filter; that
guard could be forgotten by a future list path. Centralising the check
in the helper mirrors how the gate methods already handle skipAuthz.
`ErrNoManagedSource` now maps to 503 in `publishEntry` and
`deletePublishedEntry`, matching `updateEntryClaims` and
`getEntryClaims`. The error means "the managed source isn't currently
available for write," which is a temporary-unavailability condition,
not an internal server error. Swagger annotations and tests updated.
Two follow-ups the reviewer surfaced:
The four list paths in `impl_mcp.go` and `impl_skills.go` had a
`if s.skipAuthz { gateClaims = nil }` block above their
`s.lookupRegistryIDWithGate(...)` call. That guard is dead weight
since `lookupRegistryIDWithGate` delegates to
`s.validateClaimsSubsetBytes`, which already short-circuits to allow
when `skipAuthz` is true. Removing the blocks means the "is authz on?"
decision lives in exactly one place — the gate method itself —
mirroring the same pattern `s.newClaimsFilterWith` already follows.
`isValidClaimValue` and the corresponding `claimsContain` denial path
were untested. Add `TestIsValidClaimValue` covering the full type
matrix (string / []string / []any of strings allowed; nil, numbers,
booleans, nested maps, mixed-type arrays denied), plus six new
`TestCheckClaims` cases that drive a record through the gate with
each unsupported type. The intentional carve-out for empty arrays
(`{"team": []}` is vacuously satisfied) gets its own case so future
refactors can't drop it silently.
The version-promotion loop in `GetSkillVersion` had its own
`if s.skipAuthz || callerJSON == nil || checkClaims(...)` chain instead
of going through `s.newClaimsFilterWith` like every other list and
get path. Two problems with that:
The inline check duplicated logic the helper already centralises.
`newClaimsFilterWith` returns nil for skipAuthz, anonymous, *and*
super-admin callers — three uniform bypasses. The inline version only
covered the first two, so a super-admin with a JWT that didn't directly
cover any source's claims (e.g. `{role: "super-admin"}` against an entry
tagged `{org: "acme", team: "data"}`) failed promotion and got 404 —
contradicting the uniform super-admin bypass rule (auth.md §3).
The inline check meant the "is authz on?" decision lived in two places
for skill versions while `ListSkills`, `ListServers`, `GetServerVersion`
all delegate to the helper. Routing through `newClaimsFilterWith`
collapses that asymmetry: every read-path filter callsite is now
identical, and the bypass cases are owned by exactly one method.
`TestGetSkillVersion_SuperAdminBypassesPromotion` exercises the fix —
a super-admin caller with non-covering JWT claims sees the
highest-priority source's row, while the same caller without the role
gets `ErrNotFound`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
blkt
reviewed
Apr 29, 2026
Revert the read- and write-path claim helpers in `internal/service/db/`
back to package functions instead of `*dbService` methods, restoring the
original posture that keeps validation logic out of internal-state
inspection. `validateClaimsSubset`, `validateClaimsSubsetBytes`,
`newClaimsFilterWith`, `lookupRegistryIDWithGate`,
`checkRegistryExistsWithGate`, `streamRegistryRows`, `streamSourceRows`,
`resolveSourceIDsWithGate`, and `replaceRegistrySourcesWithGate` are now
pure functions again. Each callsite reapplies the
`gateClaims := …; if s.skipAuthz { gateClaims = nil }` pattern so the
short-circuit lives where the state is read, not inside the matching
algorithm. The `GetSkillVersion` super-admin-bypass fix is preserved by
funnelling promotion through the same `newClaimsFilterWith` filter
combined with the callsite skipAuthz reset. `auth.md` §4 implementation
note is updated to describe the package-function design.
Drop the `503` status code change for `ErrNoManagedSource` on
`publishEntry` and `deletePublishedEntry`. They return `500` again, in
line with `data-model.md` §3 which mandates that response. Swagger
annotations and tests are reverted accordingly; `updateEntryClaims` and
`getEntryClaims` keep their pre-existing `503` since they were already
that way on `main` and are out of scope here.
Addresses review comments on #762.
blkt
approved these changes
Apr 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
When
auth.authzis configured, an unlabeled resource (noclaims) used to be treated as public — every authenticated user could see it. This PR flips that to default-deny: no claims = invisible to anyone who isn't super-admin.This is how the list endpoints already worked. The single-resource gate was the odd one out, so the same resource could be invisible in
GET /v1/sourcesbut readable viaGET /v1/sources/{name}. Now both paths agree.The bypass cases stay the same:
auth.authzconfigured (skipAuthz=true) → no gate at all.What an operator needs to know
If you have
auth.authzon and resources without claims (entries from a synced Git source where the source row is untagged, an untagged managed source, etc.), those become invisible to claim-bearing callers after this lands. Two recovery paths:PUT /v1/entries/{type}/{name}/claimsper entry, orPUT /v1/sources/{name}for sources.claims: {org: "acme"}); synced entries inherit it.auth.md§4 has the full upgrade notes.Bonus cleanups rolled in
claimsContainnow fails closed on unsupported value types (nil/number/nested map/…) — defense in depth in case anything bypassesValidateClaimValues.ErrNoManagedSource→ 503 (was 500) onpublishEntryanddeletePublishedEntry. MatchesupdateEntryClaimsand pairs with the newGETin AddGetEntryClaimsadmin endpoint #761.s.skipAuthzshort-circuit lives in the gate helpers now, not duplicated at every callsite. Includes a fix to the skill version promotion loop, which previously bypassed for skipAuthz/anonymous but not super-admin (latent bug onmain) —TestGetSkillVersion_SuperAdminBypassesPromotioncovers it.Test plan
task lint-fix— cleantask gen+task docs— no diffs from generated codego test ./...— greengo test -tags=integration ./internal/authz/...— greeninternalmanaged-source claim choice ({org: acme}) matches the deployment story.🤖 Generated with Claude Code