Require claims on publish and fix nil-gap in consistency check#727
Merged
Require claims on publish and fix nil-gap in consistency check#727
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #727 +/- ##
==========================================
+ Coverage 59.80% 60.34% +0.54%
==========================================
Files 106 106
Lines 10306 10325 +19
==========================================
+ Hits 6163 6231 +68
+ Misses 3600 3550 -50
- Partials 543 544 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
blkt
reviewed
Apr 20, 2026
The publish endpoint accepted entries without claims when auth was enabled, creating entries invisible to all non-super-admin users. The per-user filter returns false for NULL-claims entries, so these became permanently undiscoverable. Additionally, the claim consistency check on subsequent publishes only compared claims when both sides were non-nil. Publishing with claims against a claimless entry (or vice versa) silently succeeded with no mismatch error. The API handler now rejects publish requests that omit claims when a JWT is present (400 Bad Request). A new `checkClaimConsistency` helper replaces the inline checks in both the MCP and skills publish paths, treating asymmetric nil/non-nil as a mismatch (409 Conflict). Fixes #726 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The `TestAuthzIntegration_EmptyClaimsBehavior` test was written to document the previous behavior where publishing with empty claims created invisible entries. Now that the API rejects empty/missing claims with 400 when auth is enabled, the test verifies the rejection instead and removes the dead-entry visibility checks. Fixes #726 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The claims-required check in `publishEntry` previously fired based on whether the request carried JWT claims in its context, conflating a per-request signal with a server-wide policy. If this endpoint were ever added to `PublicPaths` or the auth middleware chain changed, the guard would silently disarm and the NULL-claims bug would reappear. Thread `*config.AuthConfig` into the v1 router and derive an `authEnabled` flag from `Mode != anonymous`. The publish handler now gates on that flag, so the requirement tracks configuration rather than per-request state. Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Three test fixtures still declared their inline file data in the old
ToolHive format (`{"servers":{}}` at root), which has been unsupported
since #724. They now emit the upstream format
(`{"meta":{...},"data":{"servers":[...]}}`) and include a placeholder
server entry because the validator rejects empty server lists.
Fixes the `TestAuthzIntegration_SourceCRUD` integration failure and the
`admin_claims_test.go` suite — both were failing with "meta is required"
/ "data is required" before, and then "upstream registry must contain
at least one server" once the envelope was added.
d4a5527 to
8e5c713
Compare
blkt
approved these changes
Apr 20, 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
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.
Summary
POST /v1/entries) that omitclaimswhen authentication is enabled (JWT present) — returns 400 Bad RequestcheckClaimConsistencyhelper used by both MCP and skills publish pathsProblem
Two related bugs in the publish endpoint:
Claims not required when auth is on. Publishing without claims creates an entry with NULL claims in the database. The per-user filter (
checkClaims) returnsfalsefor emptyrecordJSON, making these entries invisible to all non-super-admin users — permanently dead entries with no error signal.Nil-gap in claim consistency check. The guard
if claimsJSON != nil && existing.Claims != nilonly fires when both sides are non-nil. Publishing without claims against an entry that has claims (or vice versa) silently succeeds with 201, giving the publisher no indication of the inconsistency.Changes
internal/api/v1/entries.gointernal/service/db/claims_filter.gocheckClaimConsistencywith symmetric nil handlinginternal/service/db/impl_mcp.gocheckClaimConsistencycallinternal/service/db/impl_skills.gocheckClaimConsistencycallinternal/api/v1/entries_test.gointernal/service/db/claims_filter_validate_test.gocheckClaimConsistencyTest plan
TestPublishEntryClaimsRequired— authenticated without claims → 400, with claims → 201, unauthenticated without claims → 201TestCheckClaimConsistency— both nil, both empty, asymmetric nil (both directions), identical claims, different values, extra keys (both directions)TestPublishServerVersion_ClaimsSubsetandTestPublishSkill_ClaimsSubsetstill passTestDeleteServerVersion_ClaimsAuthorizationandTestDeleteSkillVersion_ClaimsAuthorizationstill passtask lint-fix)Fixes #726
🤖 Generated with Claude Code