Flag high-risk content before MCP tool calls - #448
Conversation
e1b03f7 to
269d7a4
Compare
|
Ready for review. The branch is rebased to current |
![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: 269d7a4b6a
ℹ️ 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".
| } | ||
|
|
||
| function hasValidPaymentCard(value: string): boolean { | ||
| const candidates = value.match(/(?<!\d)(?:\d[ -]?){12,18}\d(?!\d)/g) ?? []; |
There was a problem hiding this comment.
When a valid PAN is followed by a space-separated numeric field, such as 4242 4242 4242 4242 12 34, this greedy expression consumes up to 19 digits as one candidate; the Luhn check then runs against the combined value and returns no finding. Card data commonly appears beside an expiry date or ZIP, so the detector should identify bounded PAN candidates rather than treating every adjacent digit group as part of the card.
Useful? React with 👍 / 👎.
| if (category && findings.length < MAX_FINDINGS) { | ||
| findings.push({ category, path }); | ||
| findings.push({ category, path, action: "block" }); |
There was a problem hiding this comment.
When 20 earlier values match review-only signals and a credential appears later, mustBlock becomes true but this length guard prevents the credential finding from being added. The resulting rejected-call audit payload contains only action: "review" findings and no category or path explaining what caused the block; reserve or replace a slot for blocking findings so the bounded result remains consistent with the rejection.
Useful? React with 👍 / 👎.
| "mcp.tools_discovered": "Tools offered for one run", | ||
| "mcp.call_succeeded": "Called on this Bot's behalf", | ||
| "mcp.call_rejected": "Blocked", | ||
| "mcp.content_flagged": "Content needs review", |
There was a problem hiding this comment.
For every new mcp.content_flagged row, the admin page shows only this generic label: Row never reads payload.contentInspection.findings. Consequently payment-card, SSN, and prompt-injection events are indistinguishable and their audit-safe paths are inaccessible from the review UI, leaving administrators unable to determine what content needs review.
Useful? React with 👍 / 👎.
|
Thanks for this, and for the care in it. The engineering is genuinely solid: it's advisory-only (a flagged tool call still proceeds, so nothing legitimate is refused), it never puts a matched value in an audit row or error, the regexes are linear (no ReDoS), and the credential path stays fail-closed. No complaints on the code. We're going to close it on scope rather than quality. The credential firewall (#436, and #450 on top of it) is default-on in the template for a narrow reason: credential material in a model-supplied tool argument is unambiguously anomalous, because the gateway injects real auth separately, so blocking it can't refuse a legitimate call. Broad PII detection (credit card, SSN) and prompt-injection classification are a different kind of thing. They are heuristic, and shipping them default-on would turn on audit flagging for every deployment that forks this template, with false positives that are real: roughly one in ten random 16-digit numbers passes a Luhn check, and 'ignore previous instructions' is ordinary text in any document about prompt injection. The right policy there depends entirely on a given deployment's data and risk posture, which is exactly the kind of decision a template should leave to the fork rather than make for everyone. So this is something to build into a specific deployment, not into the template's shipped defaults. The content-governance module is the extension point for it: a fork can add precisely these categories, tuned to its own data, and decide block-vs-review for itself. If we later decide the template should offer optional, config-gated content policies, we'll design that deliberately as a product surface. Appreciate the contribution, and the thoroughness behind it. |
2 participants
Summary
mcp.content_flaggedaudit events and an admin audit label without recording matched valuesASIA) and require Luhn-valid payment-card candidatesThis is a focused follow-up to #436 and continues the content-governance slice of #86.
Validation
bun test server/tests/content-governance.test.ts app/tests/audit-outcome.test.ts server/tests/audit.test.ts— 32 passedbun run format:check— passedbun run lint— passedbun run typecheck— passedPostgreSQL-backed integration tests remain delegated to repository CI.