fix(security): shadowing check needs clone evidence, not a name coincidence - #951
Merged
Merged
Conversation
…idence Owner report against v0.53.0-rc.7: the TPA scanner raised a dangerous finding because ElevenLabs and kaggle both expose list_models. A name collision alone is the proxy's normal operating condition — mcpproxy unifies many servers, every tool is namespaced server:tool, and retrieve_tools' BM25 ranking is what disambiguates shared compound names. No fixed "distinctive name" heuristic separates those from attacks. - Collision branch now requires impersonation-clone evidence: the same name on another server AND a near-duplicate description (token-set containment; cosmetic case/punctuation/whitespace edits do not launder a copy). Bare coincidences are silent from both sides. - Reference branch no longer fires when the tool's OWN server also exposes the referenced name — "call list_models first" is ordinary self-documentation, whoever else exposes that name. - Corpus: the two shadowing entries now model the real clone attack (they were bare collisions with honestly different descriptions — the exact FP class); new hard-negative hn_name_coincidence encodes the ElevenLabs/kaggle case as benign. Unicode escapes preserved. - Docs updated (tool-scanner.md, security-quarantine.md).
Deploying mcpproxy-docs with
|
| Latest commit: |
daacd68
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c90bc976.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-tpa-shadowing-name-coinc.mcpproxy-docs.pages.dev |
… robustness - cmd/scan-eval gateFixture and the sh_collide_transfer corpus entry modelled bare collisions (or byte-identical dups); both now model the cosmetic-variant clone the check actually detects, so the gate measures the real attack shape. - hn_name_coincidence renamed hn_shadowing_name_coincidence per the corpus id-prefix convention (detect_corpus_test hardNegPrefix). - descTokens is unicode-aware: a Cyrillic/CJK description tokenizes to real words instead of an empty set that could never evidence a clone. - cloneDescriptions requires >=3 tokens per side: "Create" == "Create" carries no information, and sub-minimal identical descriptions no longer hard-flag a shared name. - The two accepted trade-offs codex argued (different-description collisions out of scope; own-server decoy silencing the reference branch) are now documented at the decision sites: both reduce to the name-coincidence policy the owner set (MCP-3520), whose defenses are admission quarantine, server:tool namespacing, and retrieve_tools provenance — not a collision alarm.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 30882676887 --repo smart-mcp-proxy/mcpproxy-go
|
…codex round 2) FieldsFunc finds no word boundaries in unspaced Han/kana/Hangul/Thai text, so an informative CJK description collapsed to ONE token and ducked under the three-token evidence floor — a cloned unspaced description could never be detected, and the round-1 test had cheated with artificial spaces. Runs in spaceless scripts now emit character bigrams (the standard segmentation-free indexing unit), which restores both the floor and the containment thresholds for those scripts. The boundary itself is now pinned: two identical tokens carry no clone evidence, three do.
Member
Author
|
Codex critic review — three rounds, final verdict CLEAN:
Codex also argued bare collisions remain "worth flagging"; that is the owner's explicit product decision going the other way (a colliding name with a different description is indistinguishable from ElevenLabs-vs-Kaggle |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Reported against
v0.53.0-rc.7: the TPA scanner raised a dangerous / HIGHdetect.shadowing.cross_serverfinding because ElevenLabs and Kaggle both exposelist_models.A name collision alone is mcpproxy's normal operating condition: the proxy exists to unify many servers, every tool is namespaced
server:tool, andretrieve_tools' BM25 ranking is what disambiguates shared compound names (list_models,search_issues, …). The "distinctive name" length heuristic cannot separate those from attacks.Change
list_modelsfirst" is ordinary self-documentation, whoever else exposes that name. Still requires a distinctive name and still ignores self-references.sh_collide_*malicious entries modelled bare collisions with honestly different descriptions — the exact FP class — and now model the real clone attack; newhn_name_coincidencehard-negative encodes the ElevenLabs/Kaggle case as benign. Unicode escapes in the smuggling entries preserved byte-for-byte.tool-scanner.md,security-quarantine.md) describe the new semantics.Tests
New: name-coincidence-with-distinct-descriptions is silent (both sides), cosmetic-edit clones still flag, own-server reference is silent. Full
./internal/security/...with-race, strict golangci-lint v2, and./scripts/test-api-e2e.shall green.