fix(security): harden remote-URL & runtime-arg validation; honest keychain notice (M4/M5/I1)#66
Merged
Merged
Conversation
…chain notice (M4/M5/I1) Pre-existing hardening surfaced by the post-ship security review: - M4a (MED): validateRemoteUrl accepted plaintext http:// to any host, which is interceptable once written to an IDE config. Now https is always allowed; http only for loopback (localhost / 127.0.0.1 / ::1 / *.localhost). Also wire validateRemoteUrl into the `mcpm up` URL-server path, which previously wrote stack-file `url:` servers to client configs unvalidated. - M4b (MED): validateRuntimeArgs' allowlist permits "." and "/" inside values, so a ".." path-traversal segment (e.g. --config=../secret) slipped through. Reject ".." segments up front (bare, after "=", or path-separated); a non-traversal double dot like --range=1..10 is left untouched. - M5/I1 (LOW/INFO): the keychain secret-storage notice claimed "protects against other-user/offline access" unconditionally — false when keychain mode silently falls back to the machine-derived key. Make the notice honest about both backends and recommend `mcpm secrets migrate`. tsc + tsup clean; full vitest suite green.
…L coverage) Multi-agent review follow-ups: - MEDIUM dry-run regression: validateRemoteUrl ran before the dryRun guard in processUrlServer, so `mcpm up --dry-run` on a bad stack URL threw and exited non-zero on a read-only pass. Capture the validation result instead of throwing: dry-run now previews it as a "would reject URL …" skip (exit zero), and a real run categorizes the bad URL as `blocked` (mirroring trust-policy blocks) rather than a generic `failed`. - HIGH coverage gap: the up-path URL validation had zero integration tests. Add four handleUp cases — valid https installs to Cursor, non-loopback http blocked, non-http(s) scheme blocked, and the dry-run regression (no throw, "would reject"). - Nits: test IPv6 bracket loopback `http://[::1]:8080` (the bracket-strip is load-bearing); document that isLoopbackHost intentionally over-rejects exotic loopback spellings (never a bypass). tsc + tsup clean; full suite green (1302).
Member
Author
Multi-agent review pass — addressed (commit 0984949)Verdict was request-changes (no security bypass, but a real regression + a coverage gap). Both fixed:
Note: the disputed "M4b regex character-class bug" was verified a false positive — the class includes a literal backslash, so Full suite green (1302), tsc + tsup clean. |
Merged
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
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.
Summary
Pre-existing hardening surfaced by the post-ship security review (companion to #65, which fixes the shipped-diff gaps). These are not regressions from #62/#61/#63/#64 — they predate them — but they're worth closing.
Findings fixed
validateRemoteUrlaccepted plaintexthttp://to any host; once written to an IDE config it's interceptable. Themcpm upURL-server path also wrote stack-fileurl:servers unvalidated.httponly for loopback (localhost/127.0.0.1/::1/*.localhost).validateRemoteUrlis now also called on theupURL path.validateRuntimeArgs' allowlist permits.and/in values, so a..traversal segment (e.g.--config=../secret) slipped through to the spawned server...path segment up front — bare, after=, or path-separated. A non-traversal double-dot like--range=1..10is left alone.mcpm secrets migrate.Tests
http://to non-loopback hosts rejected; loopback http + all https allowed...,a/../../b,--config=../secret,--config=../../secretrejected;--range=1..10allowed.tsc --noEmit+tsupclean.Deliberately NOT changed (with rationale)
security(1)has no clean non-interactive stdin path; the maintainer already documented this as an acceptable, narrow same-user window (os-keychain.ts). Overriding a reasoned decision with a breakage-prone change isn't warranted.