refactor(serveredition): freeze the latent credential-injection chain, normalise removed config keys, per-owner token cap (Spec 107 PR-A) - #1287
Conversation
…al IdPs Generic OIDC provider, IdP-group → server allowlist, attributable JSONL audit line, and a freeze/cut of the latent multiuser/credential-injection code, split into four PRs (A freeze/cut, B OIDC + front door + telemetry, C entitlement predicate + tenant session, D audit line). Judge-panel drafted; cross-model reviewed over nine rounds (codex gpt-5.6-sol), ~130 findings resolved. Un-parks roadmap epic sso.
… and normalise removed config keys (Spec 107 PR-A) Related #1177
…nt (Spec 107 PR-A) Closes #1177
Resolves the add/add conflict in .github/RELEASE_NOTICE.md by keeping the branch copy, whose first section is byte-identical to main's Spec 105 notice (#1279) and which appends the Spec 107 PR-A entries after it.
Deploying mcpproxy-docs with
|
| Latest commit: |
c741f82
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5f88cdb4.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://107-a-freeze-cut.mcpproxy-docs.pages.dev |
|
CI note: the umbrella |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35100447480 --repo smart-mcp-proxy/mcpproxy-go
|
|
CodeQL check: pre-existing baseline alert, not a regression from this PR. The umbrella This is CodeQL's documented large-diff re-attribution (its own summary: "Alerts not introduced by this pull request might have been detected because the code changes were too large") — PR-A's cut removes ~5.8k LOC across the package, which triggers dataflow re-analysis that re-flags unrelated pre-existing alerts. The real per-language jobs ( No code change needed here. #125 is tracked separately as an existing repo-wide finding, unrelated to Spec 107. |
2 participants
Summary
Spec 107 PR-A (
specs/107-server-edition-sso-hardening, phases A.1–A.4). Three things, no new behaviour promised:Cut / frozen (FR-031, FR-033, FR-035). The latent credential-injection chain had zero production callers and is deleted with its tests:
multiuser.Router/ToolFilter, packageinternal/serveredition/workspace,broker.TokenExchanger/CredentialResolver/HeaderInjector/ConnectionKeyand the dead audit constants,auth.GetValidIDPSubjectToken+ErrReauthRequired,OAuthProvider.RefreshAccessToken,OAuthConnector.Refresh,ConnectorProvider/ConnectorFor, and the brokered seam the personal binary compiled (internal/transport/broker_auth.go,BrokeredAuthplumbing intransport/http.go,core.(*Client).SetBrokeredAuthand every brokered branch inconnection_http.go/client.go). Thestore_idp_tokenswriter, its reader and the offline-access scope/params are gone; the first server-edition boot after upgrade purges the rows an earlier release stored (by key, before any fallible setup step). An AST guard test (internal/config/latent_symbols_guard_test.go, both tags) fails if any of these declarations comes back. What stays: theoauth_connectconnect flow, its REST routes, the encrypted credential store,mcpproxy credential …— now labelled honestly: stored, not injected (credential list|statusopen with that line; docs rewritten,idp-token-storage.mdtombstoned; a release-gate test fails on any surviving injection claim).Normalised (FR-032, FR-039, FR-040).
server_edition.max_user_servers,workspace_idle_timeout, per-serverauth_broker.header/header_formatand the never-implementedauth_broker.mode: token_exchange|entra_oboare removed. Server build: a raw-map normaliser beside theteamsalias drops them before the typed decode and records oneLoadDiagnosticper key/mode, emitted as one WARN each byconfig.LogLoadDiagnosticsafter the logger exists (boot and every hot reload; the CLI twin printswarning:to stderr);PATCH /api/v1/configand/config/applyrefuse them with the same text viaValidateRemovedKeyson the raw document.store_idp_tokens: truestill loads and warns once.ServerEditionConfig.Validateis now non-mutating with the defaults (TTLs, Microsoft tenant,MCPPROXY_CRED_KEYfallback) inApplyDefaults, applied to a clone at boot, andConfig.Validate()/ValidateDetailed()reach it under the server build. Personal build:server_editionand eachauth_brokerblock are opaquejson.RawMessagecarriers preserved key-for-key through load → save → PATCH → save (withUseNumber; the legacyteamsalias re-marshal no longer rounds2^53+1or long fractions) — earlier releases wrote both blocks back as{}.Per-owner agent-token cap (FR-037, US6). Carries the design
mainmerged in #1286 —auth.MaxTokensPerOwner = 25per signed-in user inside the unchanged 100-record deployment cap — and adds the owner-cap tests (internal/storage/agent_tokens_owner_cap_test.go: the count is a full bucket walk overUserID, never a row cap, with >25 stranger rows seeded ahead of the target owner in key order), the 409 wording that names only the caller's own quota on both doors, and the docs. Ownerless operator tokens keep the personal edition's 100-token behaviour.Also in this PR: the lint job runs a second pass with
--build-tags serverand theserver-editionrace job widens to./internal/server/... ./internal/httpapi/... ./internal/storage/...with the CI-skipregex (FR-047 part 1; the server tag exposed zero pre-existing lint findings);fields.tsloses theserver_edition.max_user_serversrow;.github/RELEASE_NOTICE.mdgains the entries below. No OAS change, nocontracts.tschange, no new module dependency, frozen tool-surface goldens pass unregenerated (FR-044).Closes #1177 — already closed by #1286; this PR merges that quota, adds its tests and the per-owner 409 wording, and documents it.
Related #1281.
Retired security-regression tests (FR-031)
Deleted with the code they guarded. Every one of them exercised a path that is
nilin production — nothing constructed aRouter,CredentialResolveror brokered client outside tests — so a future "wire the broker" PR starts by re-homing them:TestRouter_BrokeredConnectionKey_AgentTokenNeverPoolsOntoItsOwner(internal/serveredition/multiuser/router_broker_agent_test.go)TestRouter_BrokeredConnectionKey_AdminUserSessionKeysAsItself(internal/serveredition/multiuser/router_broker_agent_test.go)TestResolve_NoStaticFallback_OnExchangeFailure(internal/serveredition/broker/credential_resolver_test.go)TestResolve_CrossUserIsolation_NeverReturnsAnotherUsersCredential(internal/serveredition/broker/credential_resolver_test.go)TestClient_BrokeredConnection_FailsClosed_OnlyHeadersStrategy(internal/upstream/core/connection_http_broker_test.go)TestOAuthClients_RefuseBrokeredAuth(internal/transport/oauth_static_headers_test.go)TestOAuthClients_CarryStaticHeadersandTestIntegration_ActivityIsolation*stay green.Release notice (
.github/RELEASE_NOTICE.md)server_edition.max_user_servers,server_edition.workspace_idle_timeout,auth_broker.header,auth_broker.header_formatandauth_broker.mode: token_exchange|entra_oboare removed — an old file still loads with one warning per key (atoken_exchange/entra_oboserver loses its wholeauth_brokerblock), writes throughPATCH /config//config/applyare refused, and the next write-back omits them.server_editionandauth_brokerblocks pass through as opaque JSON — no warning, no validation, no more{}write-back erasing a team's configuration.server_edition.store_idp_tokensis a no-op:truewarns once; the first server-edition boot after upgrading deletes the IdP tokens an earlier release stored; logins no longer requestoffline_access.mcpproxy credential list|statussay so; the docs' "Credential resolution", "Header injection" and "Per-(user, server) connection keying" sections are gone.409names the caller's own quota; revoked tokens keep their slot until permanently deleted.Verification
Full record, real-instance transcript and the five cross-review rounds (opencode
gpt-5.6-sol, closed CLEAN, 5/10 used) inspecs/107-server-edition-sso-hardening/verification.md. Gate set (plan.md §Gates) re-run on the merged head:go buildpersonal +-tags server;go vetboth tagsgo test -race -tags serverserveredition/config/oauth/storage; server+httpapi with the CI-skipregexgo test -race -skip <CI regex> ./internal/...(personal)go test -race ./cmd/mcpproxyboth tags;go test ./cmd/...;go test ./cmd/release-gate/--build-tags serverscripts/test-api-e2e.sh(isolated, high port)make swagger-verify;TestContractsInSync;gen-roadmap.py --check; settings parityfrontend: npx vitest runswift testtray-lifecycle.jsonlcase; nonative/file touchedEnabledstill true, personal round trip byte-stable,credential listbanner, Alice 26th mint 409 / Bob 201 / operator 201