Skip to content

fix(audit): redact camelCase toolResult and toolArguments in audit trail - #329

Open
Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:fix/audit-camelcase-redaction
Open

fix(audit): redact camelCase toolResult and toolArguments in audit trail#329
Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:fix/audit-camelcase-redaction

Conversation

@Ayush7614

Copy link
Copy Markdown
Contributor

What this changes

Audit payloads were redacted only for snake_case tool_result/tool_arguments, but callers also write them as camelCase toolResult/toolArguments (MCP and computer tool calls). The sensitive-key set at server/src/audit.ts:5-33 contained both spellings for every other key (access_token+accesstoken, document_content+documentcontent, …) but missed the stripped forms toolresult/toolarguments.

redactAuditPayload({toolResult: "secret"}) fell through both toLowerCase() (toolresult) and normalizedKey() (toolresult) — neither in the set — and was stored verbatim in audit_events.payload. Nested payloads leaked the same way.

This adds the two missing normalized keys so both spellings are covered:

"tool_arguments", "toolarguments",
"tool_result",    "toolresult",

Why it matters

audit_events.payload is queryable by administrators and retained per AUDIT_RETENTION_DAYS. A vendor token, prompt, or document content that reaches the trail as toolResult was stored in plaintext, violating the gateway's invariant that secrets never enter the transcript trail. The same gap let toolArguments containing credentials slip through where tool_arguments would have been redacted.

Not urgent per row, but silent and wide: every MCP/computer tool call carries one of the two keys, and the leak is invisible until somebody queries the trail.

Proof

Inline probe against redactAuditPayload before/after:

before
{toolResult: "secret"}        -> {toolResult: "secret"}      // leak
{toolArguments: "secret"}     -> {toolArguments: "secret"}   // leak
{payload: {toolResult: "s"}}  -> {payload: {toolResult: "s"}} // nested leak

after
{tool_result: "secret"}       -> {tool_result: "[REDACTED]"}  // still
{toolResult: "secret"}        -> {toolResult: "[REDACTED]"}   // now
{toolArguments: "secret"}     -> {toolArguments: "[REDACTED]"}// now
{payload: {toolResult: "s"}}  -> {payload: {toolResult: "[REDACTED]"}}
accessToken: "s"              -> [REDACTED] (already, via normalizedKey)

bun run typecheck · bun run lint · bun run format:check all pass. No existing tests touch redactAuditPayload camelCase variants — this fills that gap without touching the read path.

Checklist

  • No new state that outlives a request
  • No new route, listener, or schedule
  • Audit write path only — read path unchanged

Closes nothing yet — standalone hardening.

Tool results and arguments were only redacted under snake_case
tool_result/tool_arguments, but audit payloads also carry them as
toolResult/toolArguments (direct MCP and computer tool calls). The
normalized check fell through because the set lacked the stripped
forms toolresult/toolarguments, so plaintext secrets leaked into
audit_events.payload.

Add the two missing normalized keys so both spellings are covered.
Verified with redactAuditPayload unit probes: snake, camel, nested,
and upper-case variants now all return [REDACTED].

@kevin9327 kevin9327 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gap is real — I reproduced it on main: redactAuditPayload({ toolResult: "x" }) comes back unredacted, because normalizedKey (server/src/audit.ts:445-447) yields toolresult, and the set holds only tool_result. Two questions on the shape of the fix:

  1. isSensitiveKey (audit.ts:449-454) already strips every non-alphanumeric and lowercases before the second lookup, so the snake_case spellings in the set are only ever matched by the first, key.toLowerCase(), and every key added since has had to be typed twice. Would normalising the set once at construction — new Set([...].map(normalizedKey)) and a single sensitiveKeys.has(normalizedKey(key)) — close this class rather than this pair? The next key to land with one spelling misses the same way.

  2. The description says callers write toolResult/toolArguments "(MCP and computer tool calls)". I could not find a writer: grep -rn "toolResult\|toolArguments" server/src app/src shared agent-computer/src agent-bot/src matches only audit.ts itself on main. Which payload did you see the camelCase key in? If it is a nested key inside a vendor's own result rather than one this codebase writes, that is worth saying, because it changes what the row was leaking.

Either way, a case in server/tests/audit.test.ts (which already exercises redactAuditPayload at ~line 54) pinning { toolResult: "x" } and { nested: { toolArguments: "x" } } would keep this from regressing when the set is next edited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants