plugin: cross-repo-search best-practices skill (#123) - #126
Conversation
IceRhymers
left a comment
There was a problem hiding this comment.
Security review — risk LOW, nothing blocks merge
Fresh security pass over the diff (0 critical / 0 high / 0 medium / 3 low / 2 info). Posting the findings and taking all three LOW items in a follow-up commit.
Clean areas, with the reasoning that mattered:
- Supply chain — inherited, not introduced.
uc-mcp-proxywas verified on PyPI (0.5.0, author-owned,Repository: github.com/IceRhymers/uc-mcp-proxy) rather than assumed — no dependency-confusion or name-squat.master's README has shipped the same unpinneduvx uc-mcp-proxyinvocation since before this PR, and a persistedclaude mcp addentry re-runsuvxevery launch exactly as the plugin's.mcp.jsondoes. Pinning is feasible but not clearly desirable: runbook §4.4 records that the plugin has no defined release process, so a hard pin creates a bump obligation nobody owns. APP_URLis correctly NOTsensitive. Marking it sensitive would directly conflict with the documented recovery procedure — §1's whole diagnostic (blank → no server; trailing slash → wrong server) depends on the operator being able to read the value back, and inspectability is the stated reasonuserConfigwas chosen over an env var. Masking it would destroy the only diagnostic for a silent failure mode.<your-org>guidance is safe. The placeholder is a shell metacharacter — pasted unedited it parses as an input redirect and dies loudly rather than silently resolving to someone else's repo. The upstream slug appears only inside a#comment, never in an executable position.- No committed infrastructure. Zero real hostnames, workspace IDs, or SP client IDs. The
example.databricksapps.comprobe host is safe for a more precise reason than the doc gives: not RFC-2606 (that reservesexample.com, not this subdomain), but because Databricks controls the wholedatabricksapps.comzone, so nobody can stand it up to catch a stray request.
LOW findings, all being fixed:
L1 — --scope project would commit the app URL. claude plugin install --scope project writes pluginConfigs into the repo's .claude/settings.json — a file this very repo commits, and a widespread team convention. This is the one gap test_t3 structurally cannot cover: the leak happens in the consumer's repo via a consumer action, which also falsifies that test's docstring claim that the value "never reaches a committed file."
L2 — the remedy command is a blessed template with an attacker-fillable hole (highest blast radius here). SKILL.md puts a ready-made claude mcp add ... --url https://<app-url>/mcp into the model's context with exactly one placeholder, framed as "the remedy". If an agent fills <app-url> from surrounding context rather than asking, the result persists at user scope — a permanent MCP server whose tool descriptions poison every future session, with uc-mcp-proxy forwarding the user's Databricks OAuth credential to that host.
L3 — corpus results aren't framed as untrusted data. search_code/get_file pipe raw source from many unreviewed repositories into the model, and neither SERVER_INSTRUCTIONS nor the skill says those bytes are data rather than instructions. Pre-existing and unchanged by this PR — raised because SKILL.md is now the artifact whose job is routing requests into those tools, making it the cheapest insertion point.
INFO worth recording: claude-plugin/AGENTS.md calls the plugin "packaging, not a new capability." That is accurate about functionality, but the trust relationship is genuinely new — before this PR the repo could not push executable configuration to consumers; after it, it can. Not a defect and no change requested; noted so the phrase isn't later cited as a security property.
|
Review pass 1 addressed — all three LOW findings fixed in 4579101.
Both skill additions land in the tail by design — the untrusted-data framing and the placeholder warning only matter once the agent is already routing or already stuck — so the head cap is unchanged at 1933. The whole-file cap moves 5000 → 5500 on a re-measured 3879, derivation recorded in the docstring as before. Not taken, deliberately: pinning Gates after the fix: |
133bbff to
a7531b2
Compare
Closes #123
Refs #124
What changed
This is now a skill-only Claude Code plugin. It installs
cross-repo-search, which routes cross-workspace questions to an already configuredcode-searchMCP server and explains the right tool choice:list_reposfor an unknown project,search_codefor exact/structural matches, andsemantic_searchfor behavior questions.find_referencesandlist_importsfor callers and imports, including their grep-shaped candidate-set—not LSP-binding—limitation.The plugin deliberately contains no
.mcp.json,userConfig, App URL, or automatic MCP registration. Server setup remains explicit and client-owned through the README’s “Connecting a client” instructions.Scope
#124 is intentionally not closed: its automatic MCP-registration design has been removed. The marketplace/plugin packaging remains solely as the installable delivery mechanism for the optional skill.
Verification
uv run pytest tests/unit/test_claude_plugin.py -q— 5 passedmake plugin-validate— both plugin and marketplace manifests pass--strictmake lint— greenmake test— 1327 passed, 274 deselected