Redact secret-named values from the tool-call audit

[dt2patel]

Business summary

Every MCP tools/call and every agent tool call writes an audit row (moqui.ai.AiToolCall), and an approval-gated agent call also writes a pending AiToolCallRequest. These rows stored the tool arguments and the full tool result as plain-text JSON. As a result, a credential passed to a tool (an API client secret, an access token) or returned by one was persisted in clear text, readable by anyone who can read those entities.

With this change, the value under any credential-like key is stored as ***redacted***. The rest of the row is unchanged, and the service, the MCP client and the model still get the real values.

What changed

• New org.moqui.ai.AuditRedactor builds the masked copy for the audit:

  • It first normalizes the value the way the audit serializes it (JsonOutput, read back). That way bean properties, iterator items, Expando and Map.Entry values are covered exactly as they would be written.
  • At any depth, it replaces with ***redacted*** the value under every key that matches the secret-name pattern, and the value of a {name|key: <secret name>, value: …} pair (HTTP headers, metafields).
  • A null stays null, and the input is never modified. Containers nested more than 32 levels deep are masked whole.
  • A value that cannot be serialized (a cyclic graph, a failing getter) is stored as the marker instead of throwing, so the audit write still happens.

• The built-in names always apply: pass(?:word|wd|phrase)|pwd|secret|token(?-i:(?!s))|api[-_]?key|private[-_]?key|access[-_]?key|credential|authorization, matched case-insensitively anywhere in the key. The token(?-i:(?!s)) part skips only the lowercase plural, so LLM usage counts stay readable (tokensIn, totalTokensOut, maxTokens, such as those in get_ai_spend results), while accessToken, tokenId and tokenString are still masked.

• New ai_audit_redact_pattern default-property (empty) in MoquiConf.xml. A deployment sets it to a regex of extra names (for example, consumerKey|signingKey), and those names are added to the built-in list. The property can only add names:

  • A blank value, a regex that does not compile (which is logged), or one that matches nothing all leave the built-in list in force.
  • If the property replaced the list instead, a blank value would switch masking off, and an operator adding one name would silently drop all the others. The adversarial probe below found the first of these in an earlier draft.

• script/ai/mcp/ExecTool.groovy: the audit row now records the arguments the backing service actually ran with (filtered to the exposed inputSchema, fixed parameters applied) instead of the raw client input. Both arguments and result go through AuditRedactor. The filtering now happens before the authentication gate, so refused calls are recorded the same way. The authenticated execution path is unchanged.

• AgentRunner.groovy: all four audit writes are masked:

  • dispatchTool: arguments and result.
  • rememberFact: arguments.
  • the rejected-call row in resume(): arguments.
  • AiToolCallRequest.arguments at the approval gate.

  resume() still dispatches from AiAgentRun.pendingState, so an approved call runs with the real values.

• Docs: design decision 19 in the MCP design spec, ai_audit_redact_pattern in the configuration reference, a new §6 in the security model on what masking does and does not cover, field descriptions on AiToolCall/AiToolCallRequest, and AGENTS.md check 3.

Why the MCP row now stores the filtered arguments

The raw input can include keys that never reach the service, and a client's attempted override of a fixed parameter. That means the row could disagree with what actually ran. For example, on main a call to the fixture tool echo_fixed with repeat: 5 is audited as repeat: 5, although the service ran with the fixed repeat=2. Recording the map the service actually received makes the row accurate. It also limits the stored keys to declared parameter names, which is where name-based masking is reliable. The trade-off is that out-of-schema keys a client sends are no longer recorded.

Not covered (by design, see security model §6)

• AiAgentRun.pendingState and AiConversationMessage keep the real values because both are load-bearing. resume() dispatches from pendingState (which is cleared on resume), and the conversation transcript is replayed to the model.
• Free text: errorText, and a secret inside a string value (for example, a JSON document returned as a single string).
• Other name/value shapes: attribute-style rows such as {settingTypeEnumId, settingValue}. Only {name|key, value} pairs are recognized.
• Unlisted names: names outside the built-in list, such as cookie, sessionId, jwt, consumerKey, signingKey or encryptionKey, and the plural accessTokens. A deployment adds the ones its tools use through ai_audit_redact_pattern.
• Existing rows: rows written before this change are not rewritten. Purging or re-masking them is a separate data operation on each instance.

Validation

This followed TDD twice:

1. The 7 call-level tests were written first. They failed on the unfixed code because the rows held the raw values.
2. The 12 hardening cases (normalization, name/value pairs, the additive property, blank and never-matching values, passphrase/pwd, marker-on-failure) were also written first. They failed against the first version of the helper, then passed.

All runs used ./gradlew :runtime:component:moqui-ai:test in an isolated copy of the dev runtime (its own transaction journal, with runtime/component/moqui-ai pointing at this branch) against a local MySQL dev database:

Run	Tests	Failed	Skipped
Baseline: origin/main b485c64	214	7	8
New call-level tests, without the fix	221	14: the baseline 7 plus all 7 new call-level tests	8
This branch	268	7: the same baseline set	8
This branch with masking disabled (mutant)	268	49: the baseline 7 plus 42 of the 54 new tests	8

• 54 new tests: 47 AuditRedactorTests cases and 7 call-level tests. The call-level tests assert on the persisted rows:
  • McpCallTests ×3: secret-named argument and result fields at the top level and nested in a Map and a List, where the row holds the marker instead of the value while structuredContent keeps the real value; the refused call; the filtered arguments.
  • AgentRunnerTests: the model still receives the real result.
  • AiApprovalTests ×2: the pending AiToolCallRequest is masked and the approved call still runs with the real value; the rejected-call row is masked.
  • AiContextTests: a remember call with an extra secret-named key.
• Mutant survivors: the 12 new tests that still pass under the mutant do not depend on masking. They cover kept keys, no mutation, the depth cap, marker-on-failure and the filtered arguments.
• Adversarial probe: an independent reviewer's probe of the compiled helper found the gaps fixed here: a blank or never-matching property switched masking off; beans, Expando, iterators and Map.Entry values leaked; and {name, value} headers leaked. After the fix, a rerun shows none of those leak, no property value switches masking off, and 8 threads flipping the property concurrently produce no unmasked value.
• Baseline failures: the 7 are environmental in this runtime and unrelated to this change. Six are in AiComposerTests (catalog content absent) and one is in NotNakedSeedTests (it needs another component's seed data). The 8 skips are live provider tests that need API keys.
• One flaky run: AiApprovalTests "A1 regression…" failed once in 9 runs and passed on rerun. This is pre-existing and unrelated to this change. run() starts generate#ConversationTitle asynchronously for an untitled conversation, and that thread can take the scripted response from the shared static MockProvider queue before the agent loop does. Nothing changed here runs before that first model call.
• Test values: all are synthetic (fake-secret-<nanoTime>). No real credentials were used.
• Not run: a live HTTP call to /mcp/json on a running server. The transport screen is unchanged; the tests drive dispatch#Request and exec#Tool and read the persisted rows back from the database.

Deploy note

This change adds a class under src/main (AuditRedactor) and changes AgentRunner. Rebuild the component jar (./gradlew :runtime:component:moqui-ai:jar) before restarting a server.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.