Repository navigation
fix(secrets_detection): detect quoted AWS secret keys - #138
Conversation
Signed-off-by: lucarlig <luca.carlig@ibm.com>
msureshkumar88
left a comment
There was a problem hiding this comment.
Review
Verified locally on this branch (rebuilt the Rust extension, ran the full suite): cargo test --lib 80/80 passed, pytest plugins/tests/secrets_detection/ 100/100 passed, cargo fmt --check clean. The fix itself is correct for the case it targets, no breaking change, no unrelated diff, DCO sign-off present.
Requesting changes: quoted-secret detection gap (JSON/YAML colon form)
The new regex only extends the key = "value" assignment form:
aws_secret_access_key = "FAKE..." # now detected ✅
It does not cover colon-delimited quoted assignment, which is a very common shape for the payloads this plugin actually scans (JSON tool responses, YAML configs, Terraform state):
"aws_secret_access_key": "FAKE..." # NOT detected
aws_secret_access_key: "FAKE..." # NOT detected
Confirmed by testing the equivalent pattern directly — neither matches. Since this plugin's entire purpose is preventing secret leakage through tool responses, and JSON/YAML are extremely common response formats, this is a real residual false-negative gap (CWE-200-adjacent: sensitive data can pass through undetected in these formats).
Suggested fix: support [:=] as the separator, consistent with the existing generic_api_key_assignment pattern (src/patterns.rs) which already handles both. Add regression tests for the colon form alongside the existing quote-style tests.
Non-blocking follow-up (does not need to hold this PR)
aws_secret_access_key isn't registered in CAPTURE_PATTERNS (src/patterns.rs), so redaction replaces the entire match — including the aws_secret_access_key = prefix and quotes — rather than just the 40-char secret value, unlike base64_24 which is capture-aware. Pre-existing behavior, not introduced by this PR, but worth a follow-up ticket since this PR is already touching the same regex.
Signed-off-by: lucarlig <luca.carlig@ibm.com>
|
@msureshkumar88 Addressed the requested JSON/YAML gap in
Validation:
I left the non-blocking |
msureshkumar88
left a comment
There was a problem hiding this comment.
Real-gateway e2e verification (commit f0fd6d1)
The follow-up commit f0fd6d1 (support colon assignments) resolves the blocking gap from my earlier CHANGES_REQUESTED review. I re-verified end-to-end against a real running mcp-context-forge gateway (not just the plugin's own unit tests), since that's the actual consumer of this package.
Setup
- Built the plugin as a real wheel from this PR's tip commit (
f0fd6d1) viamaturin build --release, installed into a local mcp-context-forge gateway's venv (uv pip install --force-reinstall --no-deps <wheel>), replacing the stale package that was there. - Started the real gateway (
uvicorn mcpgateway.main:app --port 8000) plus a real downstream MCP tool server (output_schema_test_server, providingecho-echo-back) registered as a federated gateway. plugins/config.yaml→SecretsDetectionblock temporarily set tomode: enforce(frompermissive) to exercise real enforcement; restored to the original value afterward.
Test payload
The exact shape this PR's fix targets — a JSON/YAML colon-delimited, double-quoted AWS secret — sent through tools/call on echo-echo-back via the real /rpc JSON-RPC endpoint:
"aws_secret_access_key": "FAKESecretAccessKeyForTestingEXAMPLE0000"
Results (post-fix build, commit f0fd6d1)
- Block mode (
block_on_detection: true,redact: false): gateway returned-32602 Plugin Violation: Potential secrets detected in tool result(plugin_error_code: SECRETS_DETECTED) — call blocked, secret never reached the caller. - Redact mode (
redact: true,block_on_detection: false): gateway returned the tool content with the secret replaced by***REDACTED***; raw secret value absent from the response.
Regression control (parent commit 65a41b1, pre-colon-fix)
Rebuilt the plugin wheel from the parent commit (65a41b1, before this fix), reinstalled into the same gateway, same config, same request:
- Response came back with the secret verbatim:
"aws_secret_access_key": "FAKESecretAccessKeyForTestingEXAMPLE0000"— confirming the colon-delimited gap was real and reachable through the actual gateway request path, not just a unit-test artifact, and that this PR's fix is what closes it.
Gateway checkout and config were restored to their original state afterward (plugin reinstalled to this PR's build, plugins/config.yaml reverted, temp processes stopped).
Approving — the fix is verified correct in a real gateway end-to-end, in addition to the unit/integration tests already passing (81 Rust, 108 Python).
See separate comment for one non-blocking process item.
Summary of changes still required before mergeNot re-blocking the approval on this, but flagging per this repo's own
Please bump both (and let Non-blocking, cosmetic, pre-existing (not introduced by this PR): redaction still consumes the assignment prefix/quotes rather than just the secret value ( |
Correction: version bump claim above was wrongMy mistake — I only diffed the version between this PR's two commits ( Checked against the actual merge-base with |
Summary
Root cause
The
aws_secret_access_keyregex required the 40-character value to begin immediately after whitespace following=. Common quoted assignments therefore produced zero findings, so neither redaction nor blocking was applied.Impact
Tool responses containing quoted AWS Secret Access Key assignments are now detected and follow the configured Redact or Block enforcement action.
Validation
make ciinplugins/rust/python-package/secrets_detectionTracking