Conversation
…edicate guard (Spec 107 PR-C)
…ex search and minting doors (Spec 107 PR-C)
…esolution on every authentication, scoped index search (Spec 107 PR-C) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…oute allowlist and Web UI gating (Spec 107 PR-C)
…E/activity projections, Web UI on the SSO session alone (Spec 107 PR-C) T083: SessionPrincipalResolver type in internal/httpapi/session_principal.go; rewrote apiKeyAuthMiddleware with FR-001 precedence (X-API-Key > Authorization > ?apikey > session cookie). Auth context now excludes session principals from CanRevealSecrets. T084: Session principal resolver built in setup.go via userHandlers.ResolveAgentTokenOwner, installed via Dependencies and wired in serveredition_wire.go. T085: GET /profiles omits profiles whose entitlements don't intersect session scope; GET /profiles/active returns empty when active profile is hidden. SSE and activity paths re-resolve session principals before every frame for live-narrowing. T086: Activity filtering and masking for tenant sessions; GET /user/activity added; admin_user principals answer unconditionally with empty list (SC-006). T088–T089: Frontend principal-kind gating for tenant sessions (onboarded to /login, hidden config/routing/servers/stats/security/activity surfaces). AdminUsers adds Groups column; AdminServers shows server-scoped group/grant chips. Playwright e2e test added (quickstart.md spec). Both editions build clean; all unit-tests.yml -tags server race suites pass. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…load and OAS wiring for PR-C
Full plan.md gate set run against PR-C HEAD (1e379ff): all gates pass, no PR-C code required fixing. golangci-lint's pinned v2 binary refuses the go1.26 module (pre-existing tooling gap); the @latest fallback found 16 issues, all pre-existing and outside the PR-C diff. The isolated test-api-e2e.sh run's two failures (launcher-lifecycle reconnect/log capture) are spec-046 fixture flakes unrelated to any PR-C file, reproduced identically on two runs. Records the full pass/fail table under '## PR-C / ### Automated checks' in verification.md.
Four genuine defects found and fixed across three opencode
gpt-5.6-sol/terra review chunks (a fourth chunk was clean plus two
low-value findings rejected as pinned-by-design or non-security):
- tenantSessionAllowlist matched on the DECODED r.URL.Path, but chi
routes on RawPath — a server name containing a literal "/" (nothing
forbids one; only ":" is refused) reaches the real handler via its
percent-encoded id while the allowlist Cuts the decoded path on the
wrong boundary, bypassing the named must-refuse for
/servers/{id}/tool-calls. Fixed by matching on RawPath when set.
- visibleSharedServers/visibleSharedServer independently re-read the
live admin-config snapshot after the entitlement predicate had
already read its own, opening a hot-reload race where a server
could be entitled against one snapshot and disclosed from another.
Fixed with tenantEntitledSnapshot, which fetches the snapshot once
and threads it through both the predicate and the disclosure lookup.
- GET /user/activity read AllowedServers off the AuthContext, but the
ServerEditionAuthMiddleware this door is actually mounted behind
never populates that field (only the separate SessionPrincipalResolver
path does) — so every tenant on this door saw every user's activity
for every server, entitled or not. Fixed by resolving the entitlement
set live through the one predicate instead.
- Dashboard.vue's loadActivitySummary was the one dashboard loader
missing the tenant guard its four siblings already carry, drawing a
spurious 403 against the now-allowlist-refused /activity/summary on
every tenant page load and 30s refresh.
Also corrected RELEASE_NOTICE.md's inverted claim about the access
block's default (absent = Shared-only, NOT deny-all; only a present-
but-empty block is deny-all) and widened the Playwright refused-route
smoke list with several named must-refuse examples.
Two findings verified and rejected: the nil-vs-empty access-block
collapse in ServerEditionAccessProjection is deliberate, pinned by an
existing config_hotreload_serveredition_test.go case; rejecting an
empty group_servers key is defensive config validation, not a spec
violation.
Appends the round 1 entry (4 genuine findings fixed, 2 rejected with reasons, full verification command list) under PR-C.
Cross-model review (opencode gpt-5.6-sol/terra) round 2 of PR-C: - entitledServerNamesFor read the admin-config servers once (round 1's fix) but the access block through a separate, independent live call, so a hot reload landing between the two reads could splice a servers snapshot from one configuration version to an access snapshot from another. A new EntitlementSnapshotProvider derives both values from one liveConfig() read, wired in setup.go. - The /events heartbeat branch never re-resolved the session principal, unlike its status/runtime-event siblings, so a disabled tenant's otherwise-idle stream never closed (FR-005). - The tenant profile projection only omitted a profile when scoping narrowed a non-empty effective set to empty, missing the case where the effective set was already empty. - Three tenant-dashboard call sites (Usage.vue's activity/usage poll, refreshSecurityScannerStatus, the ModeSwitcher control) and one Activity.vue fallback path issued or exposed admin-only core requests to a tenant session, contradicting FR-041; a doc paragraph misattributed an administrator-only capability to tenants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
T079/T090: real-instance verification against the dev-server-edition rig (quickstart §5-§7) - entitlement-filtered REST, non-disclosing by-name refusals matching a nonexistent server, "*" token materialisation to the entitled set, hot reload narrowing a live session/token with no restart or rotation, JWT self-renewal refused, live administrator parity. Web UI/Playwright run against the rig with a properly embedded frontend (make build-server, since the rig script's bare go build does not embed it): 4/5 pass; the 5th hit a pre-existing router deep-link/reload bug (title updates, body does not) unrelated to the PR-C diff, flagged as a follow-up rather than fixed inline. T093: full gate set re-run against HEAD (aac5053, after both cross-review rounds), since review landed code changes after the prior gate run. All 19 non-e2e gates green; isolated test-api-e2e.sh run twice (63/65 both times, same pre-existing launcher-lifecycle pair, unrelated to PR-C). Ticks T067-T095 in tasks.md; ROADMAP.md regenerated.
Deploying mcpproxy-docs with
|
| Latest commit: |
ff19749
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://805ed2c6.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://107-c-group-allowlist.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35204710838 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ture (PR-C CI) TestMCPGroupScope_TwoFixtureParity_GroupScopedToken failed in CI (not locally) because newGroupScopeFixture wrote directly to storage/upstream manager right after NewServer(), racing StartBackgroundInitialization's async LoadConfiguredServers — which treats cfg.Servers as the source of truth and unconditionally re-saves every entry (including the disabled shared-server placeholders this fixture registers) before flipping the runtime to PhaseReady. When that reconciliation write lands after the fixture's own Enabled:true write, the server reverts to Enabled:false in storage and isExactToolCallable drops its indexed tool from retrieve_tools — exactly the observed empty-vs-populated mismatch. Same race already fixed once in this codebase (see newLogsTestServer/server_logs_missing_file_test.go); this applies the same require.Eventually(PhaseReady) guard here.
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
2 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.
Summary
Stacked on #1292 → #1287 — merge in order.
Spec 107 PR-C (Phase C.1–C.4, T067–T095): one entitlement predicate with IdP-group server grants, owner resolution on every authentication, scoped index search, and a session-cookie tenant principal on core REST + the Web UI — so a tenant can use the product with nothing but their SSO session.
entitledServerNamesFor(user, isAdmin)+ wrapperentitledServerNames(userID, isAdmin),internal/serveredition/api/user_handlers.go): group grants (server_edition.access.group_servers/default_servers) read live; empty/absent access map = deny-all for non-administrators; administrators keep today's whole-configuration view byte-for-byte (SC-006). Seven previously-independent.Sharedreader sites collapsed onto it; an AST guard (user_handlers_shared_guard_test.go) pins that collapse. Two cross-review rounds closed a hot-reload snapshot race (servers and the access block are now read from one live snapshot per request,entitledServerNamesForSnapshot/EntitlementSnapshotProvider) and aGET /user/activitydisclosure bug (it was ignoring the entitlement predicate entirely, trusting a not-yet-populatedAuthContext.AllowedServers).storage.SetAgentTokenOwnerResolverreplacing the two old callbacks): oneGetUsercall per token authentication;AllowedServersnarrowed live through the entitlement predicate ("*"materialises to the entitled set for a tenant, stays literal for an administrator — FR-009);Email/Provider/Rolecarried as new non-persisted (json:"-") fields on the validated token, never written to BBolt.GET /api/v1/index/search): filters before the ranked cut, not after — an exhaustive paged scan of the same boolean query, cut to the top-limitof the filtered ranking, so a hidden high-ranking result can never displace an entitled one (internal/index/bleve.goSearchToolsScoped).internal/httpapi/session_principal.go): one credential source decides per request (FR-001 precedence — a valid cookie never rescues a wrong API key or bearer token); a fixed, non-disclosing 403 body on every route not in the FR-045 tenant allowlist, enforced before body parsing;chi.Walk-exhaustive Go test (tenant_allowlist_walk_test.go) plus a browser-level Playwright smoke check over a representative subset.principalKind;/profiles, SSE re-resolution per frame (including the heartbeat branch, closed in round 2), andGET /user/activityall give a tenant the same shape/masking as the core doors, scoped to their own records.Two cross-review rounds (chunked by area — predicate/storage, httpapi hook/allowlist, SSE/profiles/activity, frontend) found and fixed 9 genuine defects total, most of them real information-disclosure bugs against a live reproduction (not just unit assertions): an encoded-path allowlist bypass exposing up to 100 cross-user tool-call records, two entitlement/config snapshot races, an activity-log door that ignored the predicate outright, and five frontend gating gaps that drew spurious or (for
ModeSwitcher) genuinely reachable 403s for a tenant session. Details, live reproduction and the exact fix commit for each:verification.md→## PR-C→### Cross-review.Access block (example)
Absent
access= unchangedShared-only behaviour (every shared server, like today). A present block — even{}— is deny-all for any group/default that doesn't match:default_serverscovers a user whose groups match no key.Verification gate table (short)
Full command-by-command log:
specs/107-server-edition-sso-hardening/verification.md→## PR-C.go vetboth tags@latestfallback)internal/server), 64 packagesinternal/serverrace (CI-skip regex)-tags server -racego test ./cmd/...,./tests/oauthserver/...make swagger-verifyvitest(1298 tests) +npm run buildgen-roadmap.py --checktest-api-e2e.sh, two runs"*"token materialisation, hot reload narrows a live session/token with no restart or rotation, JWT can't self-renew, admin parityserver-edition-tenant.spec.ts) against the riggit diffdoes not touchrouter/index.ts)