diff --git a/.changeset/fix-uri-template-query-params.md b/.changeset/fix-uri-template-query-params.md new file mode 100644 index 000000000..a3f91b89f --- /dev/null +++ b/.changeset/fix-uri-template-query-params.md @@ -0,0 +1,6 @@ +--- +'@modelcontextprotocol/core': patch +--- + +Fix `UriTemplate.match()` to correctly handle optional, out-of-order, and URL-encoded query parameters. Previously, query parameters had to appear in the exact order specified in the template and omitted parameters would cause match failures. Omitted query parameters are now +absent from the result (rather than set to `''`), so callers can use `vars.param ?? defaultValue`. diff --git a/src/shared/uriTemplate.ts b/src/shared/uriTemplate.ts index a47a64c97..ffb6f50cb 100644 --- a/src/shared/uriTemplate.ts +++ b/src/shared/uriTemplate.ts @@ -7,6 +7,14 @@ const MAX_VARIABLE_LENGTH = 1000000; // 1MB const MAX_TEMPLATE_EXPRESSIONS = 10000; const MAX_REGEX_LENGTH = 1000000; // 1MB +function safeDecode(s: string): string { + try { + return decodeURIComponent(s); + } catch { + return s; + } +} + export class UriTemplate { /** * Returns true if the given string contains any URI template expressions. @@ -97,7 +105,7 @@ export class UriTemplate { return expr .slice(operator.length) .split(',') - .map(name => name.replace('*', '').trim()) + .map(name => name.replace(/\*/g, '').trim()) .filter(name => name.length > 0); } @@ -247,12 +255,19 @@ export class UriTemplate { match(uri: string): Variables | null { UriTemplate.validateLength(uri, MAX_TEMPLATE_LENGTH, 'URI'); + + // Build regex pattern for path (non-query) parts let pattern = '^'; const names: Array<{ name: string; exploded: boolean }> = []; + const queryParts: Array<{ name: string; exploded: boolean }> = []; for (const part of this.parts) { if (typeof part === 'string') { pattern += this.escapeRegExp(part); + } else if (part.operator === '?' || part.operator === '&') { + for (const name of part.names) { + queryParts.push({ name, exploded: part.exploded }); + } } else { const patterns = this.partToRegExp(part); for (const { pattern: partPattern, name } of patterns) { @@ -262,23 +277,61 @@ export class UriTemplate { } } + // Only strip the URI fragment when the template has no {#var} operator; + // otherwise the fragment is part of what the path regex must capture. + const hasHashOperator = this.parts.some(p => typeof p !== 'string' && p.operator === '#'); + let working = uri; + if (!hasHashOperator) { + const hashIndex = working.indexOf('#'); + if (hashIndex !== -1) working = working.slice(0, hashIndex); + } + + // Only split path/query when the template actually has {?..}/{&..} + // operators. Otherwise match the path regex against the full URI so + // {+var} can capture across '?' as it did before query-param support. + let pathPart = working; + let queryPart = ''; + if (queryParts.length > 0) { + const queryIndex = working.indexOf('?'); + if (queryIndex !== -1) { + pathPart = working.slice(0, queryIndex); + queryPart = working.slice(queryIndex + 1); + } + } + pattern += '$'; UriTemplate.validateLength(pattern, MAX_REGEX_LENGTH, 'Generated regex pattern'); const regex = new RegExp(pattern); - const match = uri.match(regex); - + const match = pathPart.match(regex); if (!match) return null; const result: Variables = {}; - for (let i = 0; i < names.length; i++) { - const { name, exploded } = names[i]; - const value = match[i + 1]; - const cleanName = name.replace('*', ''); - if (exploded && value.includes(',')) { - result[cleanName] = value.split(','); - } else { - result[cleanName] = value; + for (const [i, { name, exploded }] of names.entries()) { + const value = match[i + 1]!; + const cleanName = name.replace(/\*/g, ''); + result[cleanName] = exploded && value.includes(',') ? value.split(',') : value; + } + + if (queryParts.length > 0) { + const queryParams = new Map(); + if (queryPart) { + for (const pair of queryPart.split('&')) { + const equalIndex = pair.indexOf('='); + if (equalIndex !== -1) { + const key = safeDecode(pair.slice(0, equalIndex)); + const value = safeDecode(pair.slice(equalIndex + 1)); + queryParams.set(key, value); + } + } + } + + for (const { name, exploded } of queryParts) { + const cleanName = name.replace(/\*/g, ''); + const value = queryParams.get(cleanName); + + if (value === undefined) continue; + result[cleanName] = exploded && value.includes(',') ? value.split(',') : value; } } diff --git a/test/shared/uriTemplate.test.ts b/test/shared/uriTemplate.test.ts index 5bd54d2cf..813597bc2 100644 --- a/test/shared/uriTemplate.test.ts +++ b/test/shared/uriTemplate.test.ts @@ -191,11 +191,105 @@ describe('UriTemplate', () => { expect(template.variableNames).toEqual(['q', 'page']); }); + it('should handle partial query parameter matches correctly', () => { + const template = new UriTemplate('/search{?q,page}'); + const match = template.match('/search?q=test'); + expect(match).toEqual({ q: 'test' }); + expect(template.variableNames).toEqual(['q', 'page']); + }); + + it('should match multiple query parameters if provided in a different order', () => { + const template = new UriTemplate('/search{?q,page}'); + const match = template.match('/search?page=1&q=test'); + expect(match).toEqual({ q: 'test', page: '1' }); + expect(template.variableNames).toEqual(['q', 'page']); + }); + + it('should still match if additional query parameters are provided', () => { + const template = new UriTemplate('/search{?q,page}'); + const match = template.match('/search?q=test&page=1&sort=desc'); + expect(match).toEqual({ q: 'test', page: '1' }); + expect(template.variableNames).toEqual(['q', 'page']); + }); + + it('should match omitted query parameters', () => { + const template = new UriTemplate('/search{?q,page}'); + const match = template.match('/search'); + expect(match).toEqual({}); + expect(template.variableNames).toEqual(['q', 'page']); + }); + + it('should distinguish absent from empty query parameters', () => { + const template = new UriTemplate('/search{?q,page}'); + const match = template.match('/search?q='); + expect(match).toEqual({ q: '' }); + }); + + it('should match nested path segments with query parameters', () => { + const template = new UriTemplate('/api/{version}/{resource}{?apiKey,q,p,sort}'); + const match = template.match('/api/v1/users?apiKey=testkey&q=user'); + expect(match).toEqual({ + version: 'v1', + resource: 'users', + apiKey: 'testkey', + q: 'user' + }); + expect(template.variableNames).toEqual(['version', 'resource', 'apiKey', 'q', 'p', 'sort']); + }); + it('should handle partial matches correctly', () => { const template = new UriTemplate('/users/{id}'); expect(template.match('/users/123/extra')).toBeNull(); expect(template.match('/users')).toBeNull(); }); + + it('should handle encoded query parameters', () => { + const template = new UriTemplate('/search{?q}'); + const match = template.match('/search?q=hello%20world'); + expect(match).toEqual({ q: 'hello world' }); + expect(template.variableNames).toEqual(['q']); + }); + + it('should not throw on malformed percent-encoding in query parameters', () => { + const template = new UriTemplate('/search{?q}'); + expect(template.match('/search?q=100%')).toEqual({ q: '100%' }); + expect(template.match('/search?q=%ZZ')).toEqual({ q: '%ZZ' }); + }); + + it('should not throw on literal ? in a string segment (expand-only usage)', () => { + expect(() => new UriTemplate('/path?fixed=1')).not.toThrow(); + expect(() => new UriTemplate('http://e.com/?literal').expand({})).not.toThrow(); + expect(new UriTemplate('http://e.com/?literal').expand({})).toBe('http://e.com/?literal'); + }); + + it('should let {+var} capture across ? when template has no query operators', () => { + const template = new UriTemplate('http://e.com{+rest}'); + expect(template.match('http://e.com/search?q=hello')).toEqual({ rest: '/search?q=hello' }); + }); + + it('should let {#var} capture the fragment', () => { + const template = new UriTemplate('/page{#section}'); + expect(template.match('/page#intro')).toEqual({ section: '#intro' }); + }); + + it('should accept templates using {?param} for query parameters', () => { + // The supported way to express query parameters + const template = new UriTemplate('/path{?fixed}'); + expect(template.match('/path?fixed=1')).toEqual({ fixed: '1' }); + expect(template.match('/path')).toEqual({}); + }); + + it('should strip fragments before matching query parameters', () => { + const template = new UriTemplate('/path{?a}'); + expect(template.match('/path?a=1#frag')).toEqual({ a: '1' }); + expect(template.match('/path?a=1&b=2#frag')).toEqual({ a: '1' }); + }); + + it('should strip fragments when the URI has no query string', () => { + const template = new UriTemplate('/path{?a}'); + expect(template.match('/path#frag')).toEqual({}); + expect(template.match('/path')).toEqual({}); + }); }); describe('security and edge cases', () => {