Escape regex metacharacters in the variable key during substitution#269
Open
aosmcleod wants to merge 1 commit into
Open
Escape regex metacharacters in the variable key during substitution#269aosmcleod wants to merge 1 commit into
aosmcleod wants to merge 1 commit into
Conversation
replaceVariables built its match pattern with `new RegExp(\`\\$\\{${key}\\}\`)`,
interpolating the raw key. Keys contain "." (e.g. "user_config.cs_password")
and the manifest schema allows arbitrary user_config key names, so the pattern
over-matched ("." matched any character) and a key with a metacharacter (e.g.
"[", "(a+)+") threw a SyntaxError / ReDoS, aborting config generation for an
otherwise valid manifest. Escape the key before building the regex.
Distinct from modelcontextprotocol#258 (which concerns `$` in the replacement value). Adds tests
for literal-dot matching and metacharacter-key safety.
Fixes modelcontextprotocol#262
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
1 participant
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
replaceVariablesbuilds its match pattern withnew RegExp(\\$\{${key}\}`, "g")`, interpolating the raw key. Two consequences:.in keys likeuser_config.cs_passwordis an unescaped metacharacter, so it matches any character —${user_config_cs_password}(underscore) is wrongly substituted.user_configkey containing a metacharacter ([,(a+)+, …) throws aSyntaxError/ is a ReDoS vector, abortinggetMcpConfigForManifestfor an otherwise valid manifest. The schema allows arbitrary string keys.Distinct from #258, which concerns
$sequences in the replacement value. Fixes #262.Fix
Escape regex metacharacters in the key before constructing the pattern (
src/shared/config.ts).Test
Adds two cases to
test/config.test.ts:.matches only a literal dot, and a metacharacter key does not throw and matches its own literal placeholder. Both fail against the old code and pass with the fix.yarn lint+yarn testgreen.