fix(mcp): stateful regex lastIndex bug, RFC 3986 authority parsing

- Remove /g flag from module-level ENV_VAR_PATTERN to avoid lastIndex state
- Create fresh regex instances per call in server-side hasEnvVarInHostname
- Fix authority extraction to terminate at /, ?, or # per RFC 3986
- Prevents bypass via https://evil.com?token={{SECRET}} (no path)
- Add test cases for query-only and fragment-only env var URLs (53 total)
This commit is contained in:
waleed
2026-02-18 00:31:14 -08:00
parent 89b83df34c
commit eb2b5d6dab
3 changed files with 31 additions and 10 deletions

View File

@@ -113,17 +113,19 @@ const logger = createLogger('McpSettings')
* can't be determined until resolution — but env vars only in the path/query
* do NOT bypass the check.
*/
const ENV_VAR_PATTERN = /\{\{[^}]+\}\}/g
const ENV_VAR_PATTERN = /\{\{[^}]+\}\}/
function hasEnvVarInHostname(url: string): boolean {
// If the entire URL is an env var, hostname is unknown
if (url.trim().replace(ENV_VAR_PATTERN, '').trim() === '') return true
const globalPattern = new RegExp(ENV_VAR_PATTERN.source, 'g')
if (url.trim().replace(globalPattern, '').trim() === '') return true
const protocolEnd = url.indexOf('://')
if (protocolEnd === -1) return ENV_VAR_PATTERN.test(url)
// Extract authority per RFC 3986 (terminated by /, ?, or #)
const afterProtocol = url.substring(protocolEnd + 3)
const authorityEnd = afterProtocol.indexOf('/')
const authorityEnd = afterProtocol.search(/[/?#]/)
const authority = authorityEnd === -1 ? afterProtocol : afterProtocol.substring(0, authorityEnd)
return new RegExp(ENV_VAR_PATTERN.source).test(authority)
return ENV_VAR_PATTERN.test(authority)
}
function isDomainAllowed(url: string | undefined, allowedDomains: string[] | null): boolean {

View File

@@ -178,6 +178,14 @@ describe('isMcpDomainAllowed', () => {
false
)
})
it('rejects disallowed domain with env var in query but no path', () => {
expect(isMcpDomainAllowed('https://evil.com?token={{SECRET}}')).toBe(false)
})
it('rejects disallowed domain with env var in fragment but no path', () => {
expect(isMcpDomainAllowed('https://evil.com#{{SECTION}}')).toBe(false)
})
})
describe('env var security edge cases', () => {
@@ -276,6 +284,18 @@ describe('validateMcpDomain', () => {
it('does not throw for allowed URL with env var in path', () => {
expect(() => validateMcpDomain('https://allowed.com/{{PATH}}')).not.toThrow()
})
it('throws for disallowed URL with env var in query but no path', () => {
expect(() => validateMcpDomain('https://evil.com?token={{SECRET}}')).toThrow(
McpDomainNotAllowedError
)
})
it('throws for disallowed URL with env var in fragment but no path', () => {
expect(() => validateMcpDomain('https://evil.com#{{SECTION}}')).toThrow(
McpDomainNotAllowedError
)
})
})
})
})

View File

@@ -30,19 +30,18 @@ function checkMcpDomain(url: string): string | null {
* env vars in the path/query do NOT bypass the domain check.
*/
function hasEnvVarInHostname(url: string): boolean {
const envVarPattern = createEnvVarPattern()
// If the entire URL is an env var reference, hostname is unknown
if (url.trim().replace(envVarPattern, '').trim() === '') return true
if (url.trim().replace(createEnvVarPattern(), '').trim() === '') return true
try {
// Extract the authority portion (between :// and the next /)
// Extract the authority portion (between :// and the first /, ?, or # per RFC 3986)
const protocolEnd = url.indexOf('://')
if (protocolEnd === -1) return envVarPattern.test(url)
if (protocolEnd === -1) return createEnvVarPattern().test(url)
const afterProtocol = url.substring(protocolEnd + 3)
const authorityEnd = afterProtocol.indexOf('/')
const authorityEnd = afterProtocol.search(/[/?#]/)
const authority = authorityEnd === -1 ? afterProtocol : afterProtocol.substring(0, authorityEnd)
return createEnvVarPattern().test(authority)
} catch {
return envVarPattern.test(url)
return createEnvVarPattern().test(url)
}
}