Block credential material in MCP tool arguments - #436
Conversation
27edda5 to
60bbab4
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27edda5431
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const [key, child] of Object.entries(value)) { | ||
| const childPath = path ? `${path}.${key}` : key; |
There was a problem hiding this comment.
When an authenticated or compromised Bot puts credential material in a property name, such as { "sk-<token>": "ordinary" }, only the ordinary value is visited, so the inspection reports safe: true and forwards the token to the vendor. If a nested value is also rejected, the same uninspected key is copied into findings[].path and persisted in the audit trail. Inspect keys as content and use a redacted structural path rather than embedding unsafe key text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in edc4ea7. Keys are now inspected as content, credential-bearing keys are detected, and arbitrary/unsafe property names are replaced with an audit-safe structural marker. Added tests proving both detection and non-disclosure.
|
@codex review The branch has been rebuilt from the complete latest upstream tree, and the argument-key bypass plus bounded-scan cases are addressed in |
davidmckayv
left a comment
davidmckayv
left a comment
There was a problem hiding this comment.
Deep-reviewed against live code (correctness, governance, no vendor/secret/scale issues). Composed build+tests green. CHANGELOG/format rebase on CI-validated substance.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edc4ea79a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| findings.push({ category: keyCategory, path: childPath }); | ||
| } | ||
| if ( | ||
| sensitiveFieldNames.has(normalizedFieldName(key)) && |
There was a problem hiding this comment.
When a tool accepts a standard compound credential field such as x-api-key or aws_secret_access_key, normalization produces x_api_key or leaves aws_secret_access_key, neither of which exactly matches this set. Because these credentials are commonly opaque strings with no recognizable provider prefix, the value inspection also returns no finding and the credential is forwarded to the vendor. Match credential-bearing components/suffixes rather than only the current exact names.
Useful? React with 👍 / 👎.
| if (/-----BEGIN (?:[A-Z ]+ )?PRIVATE KEY-----/.test(value)) { | ||
| return "private_key"; | ||
| } | ||
| if (/^\s*(?:Basic|Bearer)\s+\S+/i.test(value)) { |
There was a problem hiding this comment.
When a credential appears inside an ordinary argument, such as curl -H 'Authorization: Bearer <opaque-token>', the start anchor prevents this expression from recognizing it. An opaque OAuth token need not match any provider-specific pattern, so prepending explanatory text turns a blocked bearer token into a safe: true result and sends it to the vendor.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
|
|
||
| for (const [key, child] of Object.entries(value)) { |
There was a problem hiding this comment.
On an authenticated call containing a very wide object, Object.entries(value) materializes every property before the loop can stop at MAX_NODES, so the advertised node bound does not cap either enumeration work or the temporary allocation. The rebuilt tree still performs this eager enumeration (and the caller also shallow-copies top-level arguments before inspection), allowing a large argument object to consume resources well beyond the 2,000-node limit; iterate lazily and apply the limit before collecting entries.
Useful? React with 👍 / 👎.
| payload: { | ||
| ...decided, | ||
| refusal: "sensitive_tool_arguments", |
There was a problem hiding this comment.
When a policy deny rule is in dry-run mode and the arguments also contain a credential, decided.decision.carriedOut is true because the policy would forward, but content inspection then prevents any vendor call. Spreading decided unchanged into this second rejection row makes the audit UI display “dry-run: recorded, not enforced” for a call that was actually enforced by content governance; override carriedOut to false for this refusal.
Useful? React with 👍 / 👎.
2 participants
Summary
This is the narrow secret-leak-prevention slice of #86. It deliberately does not attempt broad PII detection, prompt-injection classification, or cost accounting in the same change.
Validation
bun test server/tests/content-governance.test.ts— 8 passedbun run format:check— passedbun run lint— passedbun run typecheck— passedThe PostgreSQL-backed integration assertion is included for repository CI; the local execution environment did not provide Docker/PostgreSQL.