From 7878c98e9a32e0721bb02100dd461eb1ee8fbdc1 Mon Sep 17 00:00:00 2001 From: james-crowley <42618816+james-crowley@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:37:03 -0400 Subject: [PATCH] fix(audit): name the profile rule that refused a call in the authz reason A call refused by a profile's tier cap, deny rule or unannotated-tool policy was written to the audit log as authz deny / reason other, while the activity log carried block_reason profile_tier|profile_rule| profile_unannotated. The typed cause reached the funnel but not auditAuthz. Pass it through: the three tool-policy causes are now authz reasons (profile_tier, profile_rule, profile_unannotated, disclosed:true), in the top-level gates, direct mode and replay (one funnel) and in the nested code_execution path, which used to write token_scope / disclosed:false for a refusal the script reads in full. profile_scope is unchanged; profile_code_execution and profile_management are not audit reasons (built-in invocations are not audited per call). Schema enum, audit-log docs, the Spec 107 contract and CHANGELOG updated. Fixes #1703 --- CHANGELOG.md | 2 + docs/features/audit-log.md | 13 +- docs/schemas/audit-line-v1.schema.json | 2 +- internal/server/audit_funnel.go | 35 ++++- internal/server/audit_profile_reason_test.go | 147 ++++++++++++++++++ internal/server/mcp.go | 2 +- .../contracts/audit-line-events.md | 2 +- .../contracts/audit-line.schema.json | 2 +- 8 files changed, 194 insertions(+), 11 deletions(-) create mode 100644 internal/server/audit_profile_reason_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b3eb0f269..e81b86684 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -175,6 +175,8 @@ Releases follow [Semantic Versioning](https://semver.org/). ### Bug Fixes +- **audit: a profile tool-policy refusal now names its rule (#1703).** The `authz` audit line for a call refused by a profile's tier cap, deny rule or unannotated-tool policy used to read `reason: other`; it now reads `profile_tier`, `profile_rule` or `profile_unannotated` (`disclosed: true`), matching the `block_reason` of the same call in the activity log. A refusal inside a `code_execution` script used to be written as `token_scope` with `disclosed: false` and now uses the same three reasons. `profile_scope` is unchanged, and `profile_code_execution` / `profile_management` are not audit reasons because those built-in tools are not audited per call. The `reason` enum in `docs/schemas/audit-line-v1.schema.json` gains the three members, so a consumer that validates audit lines against the enum must accept them. (refs #1703) + - **Upgrade no longer admits a server an older release held in quarantine (RC-UPG-001).** A v0.69 restart (including the baseline security scan shortly after startup) wrote a keyless server's un-gated config over its recorded quarantine, while v0.69 kept holding it in memory. On upgrade the config-load admission gate read that record as already admitted and auto-baseline approved all of its tools. The gate now also requires an approval baseline: a server recorded as unquarantined whose tools were never approved is quarantined again for review. Servers with approved tools are unaffected; vetted servers with no tool records (zero-tool servers, servers that never connected while live, servers last vetted before v0.21) are also held once — approve them, or set `"quarantined": false` before upgrading. - **A server added while mcpproxy runs is no longer re-quarantined before its tools are discovered.** The RC-UPG-001 rule now applies only to servers `config.db` already held at startup; adding a server saves it to `config.db` before publishing its config, so an OAuth server added with `quarantined: false` was held again before sign-in (v0.70.0-rc.3 release gate). Adding a server with an explicit `quarantined` value — REST, the `upstream_servers` tool, `upstream add --no-quarantine` without a running daemon, or an import with `skip_quarantine` — now records it as an operator statement in `mcp_config.json`, so it is honoured after a restart. diff --git a/docs/features/audit-log.md b/docs/features/audit-log.md index 56c074349..f37eccd59 100644 --- a/docs/features/audit-log.md +++ b/docs/features/audit-log.md @@ -163,10 +163,19 @@ entry for this page — see `verification.md`.) #### `authz` reason vocabulary -`intent_invalid` · `intent_rejected` · `profile_scope` · `token_scope` · -`token_permission` · `server_quarantined` · `tool_pending_approval` · +`intent_invalid` · `intent_rejected` · `profile_scope` · `profile_tier` · +`profile_rule` · `profile_unannotated` · `token_scope` · `token_permission` · `server_quarantined` · `tool_pending_approval` · `tool_changed_approval` · `tool_not_callable` · `other` +`profile_scope` is a profile refusing a server outside its scope (non-disclosing, +`disclosed: false`). `profile_tier`, `profile_rule` and `profile_unannotated` are +a profile's tool policy refusing a tool inside its scope: the tier cap, a deny +rule, or an unannotated tool under a fail-closed policy. They are disclosed +refusals (`disclosed: true`) and match the `block_reason` of the same call in the +activity log. The same reasons are written for a refusal inside a +`code_execution` script. `profile_code_execution` and `profile_management` are +not audit reasons: those built-in tools are not audited per call. + ### `tool_call` — one per `authz allow`, at completion | Key | Required | Notes | diff --git a/docs/schemas/audit-line-v1.schema.json b/docs/schemas/audit-line-v1.schema.json index 6ef131d6e..b2a036ba6 100644 --- a/docs/schemas/audit-line-v1.schema.json +++ b/docs/schemas/audit-line-v1.schema.json @@ -102,7 +102,7 @@ "required": ["request_id", "surface", "server", "tool", "operation", "decision", "reason", "args_sha256", "args_bytes"], "properties": { "surface": { "enum": ["call_tool_read", "call_tool_write", "call_tool_destructive", "direct", "code_execution", "rest"] }, - "reason": { "enum": ["none", "intent_invalid", "intent_rejected", "profile_scope", "token_scope", "token_permission", "server_quarantined", "tool_pending_approval", "tool_changed_approval", "tool_not_callable", "other"] }, + "reason": { "enum": ["none", "intent_invalid", "intent_rejected", "profile_scope", "profile_tier", "profile_rule", "profile_unannotated", "token_scope", "token_permission", "server_quarantined", "tool_pending_approval", "tool_changed_approval", "tool_not_callable", "other"] }, "caller": { "properties": { "kind": { "enum": ["api_key", "socket", "stdio", "anonymous", "agent_token", "session_admin", "internal"] } }, "not": { "required": ["email_hash"] } } }, "not": { "anyOf": [ diff --git a/internal/server/audit_funnel.go b/internal/server/audit_funnel.go index ab792368d..5fc7e5238 100644 --- a/internal/server/audit_funnel.go +++ b/internal/server/audit_funnel.go @@ -33,6 +33,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/callerr" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" "github.com/smart-mcp-proxy/mcpproxy-go/internal/jsruntime" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" "github.com/smart-mcp-proxy/mcpproxy-go/internal/reqcontext" "github.com/smart-mcp-proxy/mcpproxy-go/internal/telemetry" "github.com/smart-mcp-proxy/mcpproxy-go/internal/transport" @@ -292,6 +293,22 @@ func auditCallerFromContext(ctx context.Context) audit.Caller { // the refusal was non-disclosing to the caller (Spec 105 FR-010 shapes: // scope refusals reveal nothing about whether the server exists). func auditReasonFromBlockKey(reasonKey string) (reason string, disclosed bool) { + return auditReasonFromBlockKeyAndProfileReason(reasonKey, "") +} + +// auditReasonFromBlockKeyAndProfileReason is auditReasonFromBlockKey for a +// refusal that may also carry the typed cause of a profile tool-policy +// refusal (profile.BlockReason). The three tool-policy causes are their own +// authz reasons so the audit stream says which rule refused the call without +// a join against the activity log. They are disclosed refusals: the caller's +// own error text already names the cause. profile_scope is unchanged, and +// profile_code_execution / profile_management are deliberately not members: +// those built-in invocations never reach the audit stream. +func auditReasonFromBlockKeyAndProfileReason(reasonKey, profileReason string) (reason string, disclosed bool) { + switch profile.BlockReason(profileReason) { + case profile.BlockReasonTier, profile.BlockReasonRule, profile.BlockReasonUnannotated: + return profileReason, true + } switch reasonKey { case telemetry.BlockReasonTokenScope, telemetry.BlockReasonProfileScope: return reasonKey, false @@ -331,6 +348,13 @@ func (p *MCPProxyServer) auditWrite(line audit.Line, err error) { // reasonKey is the gate's telemetry.BlockReason* (ignored for allow). A // second call for the same attempt is a no-op. func (p *MCPProxyServer) auditAuthz(ctx context.Context, decision, reasonKey string) { + p.auditAuthzWithProfileReason(ctx, decision, reasonKey, "") +} + +// auditAuthzWithProfileReason is auditAuthz for a gate that also knows the +// typed cause of a profile tool-policy refusal (profile.BlockReason, "" for +// every other refusal). +func (p *MCPProxyServer) auditAuthzWithProfileReason(ctx context.Context, decision, reasonKey, profileReason string) { d := auditDispatchFromContext(ctx) if d == nil || p.auditSink == nil { return @@ -351,7 +375,7 @@ func (p *MCPProxyServer) auditAuthz(ctx context.Context, decision, reasonKey str Reason: "none", } if decision == "deny" { - reason, disclosed := auditReasonFromBlockKey(reasonKey) + reason, disclosed := auditReasonFromBlockKeyAndProfileReason(reasonKey, profileReason) in.Reason = reason in.Disclosed = &disclosed } @@ -528,9 +552,10 @@ func (o *nestedAuthzObserver) ObserveAuthzGate(report jsruntime.AuthzGateReport) if o.proxy == nil || o.proxy.auditSink == nil { return } - // Note: the audit reason below stays the Spec 107 vocabulary even for a - // profile refusal (report.BlockReason is not mapped here); aligning it - // with the top-level gate's `other` is a Spec 107/108 follow-up. + // A profile tool-policy refusal (tier cap, deny rule, unannotated tool) + // carries its typed cause in report.BlockReason and is audited under that + // reason, as at the top-level gates. Every other refusal keeps the Spec 107 + // nested vocabulary below. reasonKey := telemetry.BlockReasonTokenScope switch report.Code { case jsruntime.ErrorCodeServerNotAllowed: @@ -581,5 +606,5 @@ func (o *nestedAuthzObserver) ObserveAuthzGate(report jsruntime.AuthzGateReport) Args: report.Arguments, Caller: &caller, }) - o.proxy.auditAuthz(ctx, "deny", reasonKey) + o.proxy.auditAuthzWithProfileReason(ctx, "deny", reasonKey, report.BlockReason) } diff --git a/internal/server/audit_profile_reason_test.go b/internal/server/audit_profile_reason_test.go new file mode 100644 index 000000000..fc14519e6 --- /dev/null +++ b/internal/server/audit_profile_reason_test.go @@ -0,0 +1,147 @@ +package server + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/profile" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/telemetry" +) + +// A profile's tool policy refuses a tool in three ways (tier cap, deny rule, +// unannotated tool). The audit stream names which one, instead of "other". + +func TestAuditReasonFromBlockKeyAndProfileReason(t *testing.T) { + cases := []struct { + name string + reasonKey string + profileReason string + wantReason string + wantDisclosed bool + }{ + {"tier", telemetry.BlockReasonOther, string(profile.BlockReasonTier), "profile_tier", true}, + {"rule", telemetry.BlockReasonOther, string(profile.BlockReasonRule), "profile_rule", true}, + {"unannotated", telemetry.BlockReasonOther, string(profile.BlockReasonUnannotated), "profile_unannotated", true}, + // The typed cause wins over a gate key a nested refusal may carry. + {"tier over token_permission", telemetry.BlockReasonTokenPermission, string(profile.BlockReasonTier), "profile_tier", true}, + // Not audit reasons: built-in invocations never reach the audit stream. + {"code_execution stays other", telemetry.BlockReasonOther, string(profile.BlockReasonCodeExecution), "other", true}, + {"management stays other", telemetry.BlockReasonOther, string(profile.BlockReasonManagement), "other", true}, + // Server scope keeps its own non-disclosing reason via the gate key. + {"profile_scope key", telemetry.BlockReasonProfileScope, "", "profile_scope", false}, + {"server scope cause alone is not a tool-policy reason", telemetry.BlockReasonTokenScope, string(profile.BlockReasonServerScope), "token_scope", false}, + {"no profile cause", telemetry.BlockReasonToolNotCallable, "", "tool_not_callable", true}, + {"unknown stays other", "nonsense", "", "other", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + reason, disclosed := auditReasonFromBlockKeyAndProfileReason(tc.reasonKey, tc.profileReason) + assert.Equal(t, tc.wantReason, reason) + assert.Equal(t, tc.wantDisclosed, disclosed) + }) + } + // The one-argument form is unchanged for callers with no profile cause. + reason, disclosed := auditReasonFromBlockKey(telemetry.BlockReasonTokenScope) + assert.Equal(t, "token_scope", reason) + assert.False(t, disclosed) +} + +// profileAuditFixture is one server with a read, a denied-by-rule read, a +// write and an unannotated tool behind a read-capped profile "ro". +func profileAuditFixture(t *testing.T) (*MCPProxyServer, *recordingAuditSink, *countingUpstream) { + t.Helper() + proxy, rt := createTestProxyWithRuntimeCfg(t, nil, func(cfg *config.Config) { + cfg.Servers = []*config.ServerConfig{{Name: "fx", Enabled: true}} + cfg.EnableCodeExecution = true + cfg.Profiles = []config.ProfileConfig{{ + Name: "ro", Servers: []string{"fx"}, MaxTier: config.ProfileTierRead, + Unannotated: config.ProfileUnannotatedDeny, + CodeExecution: boolPtr(true), + Tools: &config.ProfileToolRules{Deny: []string{"fx:secret_read"}}, + }} + }) + sink := &recordingAuditSink{} + proxy.auditSink = sink + up := startCountingUpstream(t, proxy, rt, "fx", + readSpec("note_get"), readSpec("secret_read"), writeSpec("note_set"), toolSpec{Name: "mystery", Description: "no annotations"}) + return proxy, sink, up +} + +func TestAuditFunnel_ProfileToolPolicyRefusalsNameTheRule(t *testing.T) { + cases := []struct { + tool string + reason string + }{ + {"note_set", "profile_tier"}, + {"secret_read", "profile_rule"}, + {"mystery", "profile_unannotated"}, + } + for _, tc := range cases { + t.Run(tc.reason, func(t *testing.T) { + proxy, sink, up := profileAuditFixture(t) + + result, err := proxy.handleCallToolVariant(pinnedProfileCtx("ro"), + auditCallToolRequest("fx:"+tc.tool, map[string]interface{}{}), contracts.ToolVariantRead) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, resultText(t, result), "blocked by profile") + assert.Equal(t, int64(0), up.count.Load(), "a refused call never reaches the upstream") + + lines := sink.decoded(t) + require.Len(t, lines, 1, "a refusal is exactly one authz line") + assert.Equal(t, "authz", lines[0]["event"]) + assert.Equal(t, "deny", lines[0]["decision"]) + assert.Equal(t, tc.reason, lines[0]["reason"]) + assert.Equal(t, true, lines[0]["disclosed"], "the caller's own error text already names the cause") + assert.Equal(t, "ro", callerOf(t, lines[0])["profile_pin"]) + }) + } +} + +func TestAuditFunnel_ProfileAllowedCallIsStillAllow(t *testing.T) { + proxy, sink, up := profileAuditFixture(t) + + result, err := proxy.handleCallToolVariant(pinnedProfileCtx("ro"), + auditCallToolRequest("fx:note_get", map[string]interface{}{}), contracts.ToolVariantRead) + require.NoError(t, err) + require.False(t, result.IsError, resultText(t, result)) + assert.Equal(t, int64(1), up.count.Load()) + + lines := sink.decoded(t) + require.Len(t, lines, 2) + assert.Equal(t, "allow", lines[0]["decision"]) + assert.Equal(t, "none", lines[0]["reason"]) +} + +func TestAuditFunnel_NestedProfileToolPolicyRefusalNamesTheRule(t *testing.T) { + cases := []struct { + tool string + reason string + }{ + {"note_set", "profile_tier"}, + {"secret_read", "profile_rule"}, + {"mystery", "profile_unannotated"}, + } + for _, tc := range cases { + t.Run(tc.reason, func(t *testing.T) { + proxy, sink, up := profileAuditFixture(t) + + call := runSandboxCallTool(t, proxy, pinnedProfileCtx("ro"), "fx", tc.tool) + require.False(t, call.OK) + assert.Equal(t, int64(0), up.count.Load()) + + lines := sink.decoded(t) + require.Len(t, lines, 1, "the nested refusal writes exactly one authz line") + assert.Equal(t, "deny", lines[0]["decision"]) + assert.Equal(t, tc.reason, lines[0]["reason"]) + assert.Equal(t, true, lines[0]["disclosed"], + "the script reads the refusal in full, so the line must not claim it was concealed") + assert.Equal(t, "code_execution", lines[0]["surface"]) + assert.NotEmpty(t, lines[0]["parent_id"]) + }) + } +} diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 71e7ec97a..9e63f3894 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -995,7 +995,7 @@ func (p *MCPProxyServer) emitActivityPolicyDecisionWithBlockReason(ctx context.C if isPostDispatchBlockKey(reasonKey) { p.auditToolCall(ctx, "blocked", reasonKey, "", auditDurationMs(ctx), nil, nil) } else { - p.auditAuthz(ctx, "deny", reasonKey) + p.auditAuthzWithProfileReason(ctx, "deny", reasonKey, blockReason) } } if p.mainServer != nil && p.mainServer.runtime != nil { diff --git a/specs/107-server-edition-sso-hardening/contracts/audit-line-events.md b/specs/107-server-edition-sso-hardening/contracts/audit-line-events.md index 3631eaada..11d8b6421 100644 --- a/specs/107-server-edition-sso-hardening/contracts/audit-line-events.md +++ b/specs/107-server-edition-sso-hardening/contracts/audit-line-events.md @@ -26,7 +26,7 @@ Machine-readable form: [`audit-line.schema.json`](audit-line.schema.json) (draft | `surface` | ✔ | | `call_tool_read|write|destructive` \| `direct` \| `code_execution` (nested child) \| `rest` | | `server`, `tool`, `operation` | ✔ | | canonical pair; recorded even when the refusal was non-disclosing | | `decision` | ✔ | | `allow` written after the last gate and before the upstream call; `deny` at the refusing gate | -| `reason` | ✔ | | `none` iff `allow`; else one pre-dispatch gate: `intent_invalid`, `intent_rejected`, `profile_scope`, `token_scope`, `token_permission`, `server_quarantined`, `tool_pending_approval`, `tool_changed_approval`, `tool_not_callable`, `other` (= `telemetry.BlockReason*` minus `output_sanitisation|output_schema`) | +| `reason` | ✔ | | `none` iff `allow`; else one pre-dispatch gate: `intent_invalid`, `intent_rejected`, `profile_scope`, `profile_tier`, `profile_rule`, `profile_unannotated` (added after Spec 108: a profile tool-policy refusal, `disclosed:true`), `token_scope`, `token_permission`, `server_quarantined`, `tool_pending_approval`, `tool_changed_approval`, `tool_not_callable`, `other` (= `telemetry.BlockReason*` minus `output_sanitisation|output_schema`) | | `disclosed` | ✔ when `deny` | | `false` for non-disclosing refusals (Spec 105 FR-010 shapes) | | `args_sha256`, `args_bytes` | ✔ | | from the attempt record | | `parent_id` | opt | | nested children only | diff --git a/specs/107-server-edition-sso-hardening/contracts/audit-line.schema.json b/specs/107-server-edition-sso-hardening/contracts/audit-line.schema.json index 6ef131d6e..b2a036ba6 100644 --- a/specs/107-server-edition-sso-hardening/contracts/audit-line.schema.json +++ b/specs/107-server-edition-sso-hardening/contracts/audit-line.schema.json @@ -102,7 +102,7 @@ "required": ["request_id", "surface", "server", "tool", "operation", "decision", "reason", "args_sha256", "args_bytes"], "properties": { "surface": { "enum": ["call_tool_read", "call_tool_write", "call_tool_destructive", "direct", "code_execution", "rest"] }, - "reason": { "enum": ["none", "intent_invalid", "intent_rejected", "profile_scope", "token_scope", "token_permission", "server_quarantined", "tool_pending_approval", "tool_changed_approval", "tool_not_callable", "other"] }, + "reason": { "enum": ["none", "intent_invalid", "intent_rejected", "profile_scope", "profile_tier", "profile_rule", "profile_unannotated", "token_scope", "token_permission", "server_quarantined", "tool_pending_approval", "tool_changed_approval", "tool_not_callable", "other"] }, "caller": { "properties": { "kind": { "enum": ["api_key", "socket", "stdio", "anonymous", "agent_token", "session_admin", "internal"] } }, "not": { "required": ["email_hash"] } } }, "not": { "anyOf": [