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.
… rig (Spec 107 PR-B)
… binding and refusals (Spec 107 PR-B)
…ns, groups capture and subject binding (Spec 107 PR-B) Related #1169
…e security and forced MCP auth (Spec 107 PR-B)
…ed proxies, cookie security policy, forced MCP auth, provider probe (Spec 107 PR-B)
… keys; telemetry v13 server-edition fields (Spec 107 PR-B)
T062/T063: real-instance verification of the OIDC front door (happy-path login, groups, /auth/me, agent-token mint + /mcp initialize/tools-list, ErrorMode tamper matrix, cookie Secure matrix behind a simulated trusted vs untrusted proxy) and the telemetry v13 payload (server vs personal). T064: full gate set re-run on HEAD after cross-review round 6 touched the config load path, oauth_handler.go and telemetry.go — all green. T066: tick completed PR-B tasks (T032-T066); ROADMAP.md regenerated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…1288) This branch diverged from main before #1288 landed, so PR-B's CI hit the exact regression it fixed: handleAddFromRegistry's success path dereferenced a nil cfg, chi's recoverer turned the panic into a bare 500, and the recovered hardware fault corrupted the Go heap on windows/amd64 under Go 1.26 (golang/go#81238) — the internal/httpapi test binary crashed with "found pointer to free object" and took the whole Windows unit-test job (and the binaries job's httpapi/scanner test bundle) down with it. Cherry-picked the same fix main already carries: the nil-config guard with a JSON 500 (and its pinning test), plus the unrelated Pass-2 temp-dir teardown race fix in the scanner test this same upstream commit bundled. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
…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.
…on and the synchronous sink (Spec 107 PR-D)
…-validated builder, synchronous sink (Spec 107 PR-D)
…ation, nested observer and error classes (Spec 107 PR-D)
… nested code_execution observer, error classes (Spec 107 PR-D)
T096-T119: Edition-neutral audit line emission at authorization and tool-call decision points.
- internal/audit/error_class.go: ErrorClass enum (upstream_error, upstream_unavailable, validation, sanitisation, cancelled, internal) with errors.Is/As routing and ErrorClassOf() type switch
- internal/jsruntime/runtime.go: AuthzObserver interface (report cached decision), AuthzGateReport with ParentID, ExecutionOptions.{AuthzObserver, ParentID} for nested tracking, nestedAuthzObserver implementation
- internal/server/audit_funnel.go: auditDispatch (attempt wrapper), installAuditAttempt (RFC 8785 JCS arg hash, mount/source/origin/profile derivation), auditCallerFromContext (full table: stdio, socket, agent_token, session_admin, anonymous), auditAuthz (deny reason from telemetry.BlockReason, disclosed:false for scopes), auditToolCall (pairs authz, error_class routing), auditToolCallShed (rejected outcome)
- internal/server/mcp.go, mcp_routing.go, mcp_code_execution.go: ctx as first param to emit* activity functions; handleCallToolVariant surface/intent gates; code_execution bridge with parent_id + nested observer
- internal/server/server.go: ServerOption pattern, WithAuditSink, stdioAuthContext tags
- internal/server/serveredition_wire.go: AuditSink dependency wiring
- internal/server/audit_funnel_test.go: 11 tests covering allow/deny, hidden-server scope, intent rejection, nested calls, limiter shed, stdio/socket callers, schema validation
- activity_result_status_test.go, code_exec_activity_test.go, preflight_telemetry_test.go: statusArgIndex 5→6, ctx passed to emit functions
All audit lines validated against contracts/audit-line.schema.json; no secrets/tokens in clear; one authz per attempt, one tool_call per call.
…ults, doctor finding, write-failure metric, settings wiring (Spec 107 PR-D) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…sertion, release notice (Spec 107 PR-D)
T116: ran scripts/dev-server-edition.sh --phase d end to end, plus manual extensions for cases the script doesn't cover (sentinel in a caller-supplied tool name, native stdio transport rules, unwritable path). Fixed two rig bugs found along the way (not in the audited feature code): the scratch config left fixture servers quarantined, and the isError=null vs "false" assertion didn't accept the omitempty case. Documented a pre-existing, out-of-scope discrepancy: `--listen ""` doesn't actually reach native stdio through `mcpproxy serve` (Validate() resets it to the HTTP default); `--listen ":0"` does. T117: re-ran the full gate set since the adversarial-review commit changed code after the prior recording. All green; two transient flakes under heavy concurrent load (a Docker-status test, a launcher-lifecycle respawn test) confirmed non-reproducing / out of this PR's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Spec 107's four PRs (A, B, C, D) are all open; PR-D is #1296. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
6af4400
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0d538a74.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://107-d-audit-line.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 35371173901 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
opencode CLI (gpt-5.6-sol / gpt-5.6-terra) reviewed the PR-D diff in 5 chunks; all 5 returned findings (16 total). 8 verified genuine and fixed: NaN/Infinity silently hashed as JSON null instead of refused (canonical.go), client.version unmasked in the audit line, a short sink write not counted as a failure, a malformed args_json dispatch producing no audit line at all, work_session_id never stamped on the attempt, auth_event lines never carrying client.ip, two missing FR-014 startup notices, and a missing sidebars.js entry for the new audit-log doc. 3 rejected as false positives with evidence (redact.go patterns are a superset of the log sanitizer's, not narrower; no request_id==transport_request_id binding rule exists in the contracts doc; the unknown-server fail-open in mcp_code_execution.go is a documented deliberate design choice). 5 more confirmed genuine but deferred to a later round given their blast radius (JCS float formatting, per-kind caller-identity validation, auth_event flags vocabulary, Attempt.Operation sourced from the caller's variant instead of the resolved target tier, and a batch-cancellation gap in the tool_call/authz pairing) — all documented in verification.md. Full round detail, rejection evidence and verification commands in specs/107-server-edition-sso-hardening/verification.md under PR-D / Cross-review / Round 1.
Fixes 8 findings from round 2 (opencode gpt-5.6-sol/terra, 5 chunks), three of them round-1's own deferred list independently re-confirmed: - ES6 Number::toString fixed/exponential threshold in formatNumberJCS (args_sha256 could diverge from a real JCS reference implementation) - length-cap the per-field audit redaction pass (unbounded client.name could drop a required line at the sink's record-size limit) - Server.ReplayToolCall wrote zero audit lines (bypassed the funnel entirely via runtime.ReplayToolCall's direct managed-client call) - malformed args_json recorded authz allow before the remaining gates ran; now authz deny (reason: other) - audit line operation now reflects the target tool's real tier, not the caller's chosen call_tool_* door - batch dispatch no longer skips the audit-attempt bridge on an already-cancelled execution context - UpdateUserLogin returns the record a subject_mismatch/user_disabled refusal was decided against, closing a race that could silently drop auth_event's required user_id - HandleLogin now flags redirect_rejected on a pre-redirect failure, not only via the callback's stored pending state - EffectiveAuditLog now honours an explicit audit_log block on the personal edition; only the absent-block default is edition-keyed See specs/107-server-edition-sso-hardening/verification.md for the full per-finding table and verification commands.
# Conflicts: # .github/RELEASE_NOTICE.md # ROADMAP.md # cmd/mcpproxy/main.go # docs/configuration.md # frontend/src/stores/auth.ts # internal/config/config.go # internal/index/bleve.go # internal/runtime/config_hotreload.go # internal/runtime/config_hotreload_test.go # internal/runtime/lifecycle.go # internal/runtime/restart_gated.go # internal/server/serveredition_wire.go # internal/serveredition/auth/oauth_handler.go # internal/serveredition/registry.go # internal/serveredition/setup.go # internal/serveredition/users/store.go # internal/serveredition/users/store_test.go # oas/docs.go # roadmap.yaml # scripts/dev-server-edition.sh # specs/107-server-edition-sso-hardening/contracts/audit-line.schema.json # specs/107-server-edition-sso-hardening/tasks.md # specs/107-server-edition-sso-hardening/verification.md
…udget macos-latest and windows-latest Unit Tests jobs on PR #1296 failed with "list took too long: 157.821625ms (should be < 100ms)" while ubuntu-latest passed on the identical commit. handleUpstreamServers/handleListUpstreams and emitActivityInternalToolCall are untouched by this PR (the new audit funnel in audit_funnel.go only wires into makeDirectModeHandler and the call_tool_* funnels), and this test calls handleUpstreamServers in-process with no HTTP layer at all, so the audit-line feature cannot be the cause. The 100ms wall-clock bound had no slack for shared/loaded CI runners, matching this repo's documented history of macOS/Windows timing flakes. Replace it with a 500ms ceiling (upstreamServersListCeiling) that still catches an architectural regression but tolerates runner noise, following the same pattern already used by preflightBenchPerOpCeiling in internal/httpapi/preflight_bench_test.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Adds an attributable, edition-neutral JSONL audit line at every authorization
decision and tool-call funnel, plus a login/logout
auth_eventline for theserver edition's OAuth front door (Spec 107 PR-D, US3, FR-012–FR-019,
FR-039 part 4, FR-043(b,j), SC-003, SC-009).
internal/audit: JCS (RFC 8785) canonicalisation over sanitised args →args_sha256; three typed constructors (NewAuthz,NewToolCall,NewAuthEvent) so a forbidden field per event type doesn't compile; asynchronous, mutex-guarded sink (file via
lumberjack, or raw stdout)with an always-on write-failure counter; per-field sanitisation of every
caller/operator-controlled string (fixed-prefix credential patterns only —
never the generic high-entropy rule, which would mask
args_sha256/email_hashthemselves) plus a whole-line defence-in-depth pass that isasserted to be the identity on every constructor's output.
internal/server:authz+tool_calllines at thecall_tool_*variants, direct dispatch, the REST direct path, and the nested
code_executionwrapper (parent_id+surface:code_execution); calleridentity derived for every kind (API key, socket, anonymous, agent token
owned/ownerless,
session_admin, internal, and native stdio, whichneeded a new
transport.ConnectionSourceStdiotag since stdio installs anadmin context with no connection source today).
internal/serveredition/auth: oneauth_eventline per terminal loginattempt (every FR-013 reason) and one per logout, with stage-dependent
identity (
user_idonce the store is reached,email_hashfor averified-but-unresolved email, neither pre-identity).
internal/config:AuditLogConfig+EffectiveAuditLog(cfg, transport)encoding the stdio rule (FR-014: stdout can never double as the sink when
stdout carries JSON-RPC — absent block under stdio → disabled + WARN,
explicit stdout-only under stdio →
StartupErrorexit 4), validationreachable from boot/PATCH/apply alike, hot-reload restart-pinning, a
mcpproxy_audit_write_failures_totalcounter and a doctor finding.docs/features/audit-log.md,docs/schemas/audit-line-v1.schema.json,docs/operations/deploying-for-a-team.md, release notice bullets.Stacked on #1293 → #1292 → #1287 — merge in order.
Example lines
{"event":"authz","decision":"allow","server":"a","tool":"echo","reason":"none","operation":"read","caller":{"kind":"agent_token","user_id":"01M...","user_email":"alice@example.com","token_name":"t1","token_prefix":"mcp_agt_9f6b"},"request_id":"...-a-echo-1","schema_version":1} {"event":"tool_call","outcome":"success","server":"a","tool":"echo","args_sha256":"7804fe...","args_bytes":52,"caller":{"kind":"agent_token","user_id":"01M...","user_email":"alice@example.com"},"request_id":"...-a-echo-1","schema_version":1} {"event":"authz","decision":"deny","server":"b","tool":"echo","reason":"token_scope","disclosed":false,"caller":{"kind":"agent_token","...":"..."},"request_id":"...-b-echo-3","schema_version":1} {"event":"auth_event","surface":"login","reason":"ok","caller":{"kind":"session_user","user_id":"01M...","role":"user","provider":"oidc"},"schema_version":1}Config keys
{ "audit_log": { "enabled": true, "stdout": false, "path": "/var/log/mcpproxy/audit.jsonl", "max_size_mb": 50, "max_backups": 10, "max_age_days": 90, "compress": true } }Defaults: personal edition
{enabled:false}; server edition with the blockabsent
{enabled:true, stdout:true}— except under the native stdiotransport, where stdout carries JSON-RPC and the default resolves to
{enabled:false}with one startup WARN namingaudit_log.pathinstead. Anexplicit
stdout:trueunder stdio is refused at boot (StartupError,exit code 4), never silently disabled. An unwritable
pathis also exitcode 4, caught by a pre-flight open/close probe before the rotating writer
installs (
lumberjackopens lazily and would otherwise swallow it).Verification gate table (full detail in
verification.md)-tags server)go vet×2go test -racenon-server (excl.internal/server)internal/server, personal tags, CI-skip regex-tags server -racego test ./cmd/...,./tests/oauthserver/...make swagger-verify+TestContractsInSyncpython3 scripts/gen-roadmap.py --check./scripts/test-api-e2e.sh+ new audit assertions (T113)dev-server-edition.sh --phase d+ manual extensions)auth_event, schema validation viaTestExternalJSONLValidates, stdio WARN/exit-4 rules, unwritable-path exit 4Two rig bugs found and fixed while running T116 (in
scripts/dev-server-edition.sh,not in the audited feature code): the scratch config left fixture servers
quarantined, and an
isErrorassertion didn't accept theomitemptycase.One pre-existing, out-of-scope discrepancy documented but not fixed:
--listen ""does not actually reach native stdio through
mcpproxy servetoday(
Config.Validate()resets an emptyListenback to the HTTP default) —--listen ":0"is the only path that works; flagged for separate follow-up.Benchmark deltas (SC-009, same-tree audit-on vs audit-off, bound = max(10%, 5ms))
call_tool_read(admin)retrieve_tools(admin)tools/list(admin)retrieve_toolsscoped vs admin