fix: UriTemplate.match() handles optional, out-of-order, and encoded query parameters

.changeset/fix-uri-template-query-params.md

@@ -0,0 +1,5 @@
+---
+'@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`.

packages/core/src/shared/uriTemplate.ts

@@ -7,6 +7,14 @@ const MAX_VARIABLE_LENGTH = 1_000_000; // 1MB
 const MAX_TEMPLATE_EXPRESSIONS = 10_000;
 const MAX_REGEX_LENGTH = 1_000_000; // 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.
@@ -35,6 +43,19 @@ export class UriTemplate {
         UriTemplate.validateLength(template, MAX_TEMPLATE_LENGTH, 'Template');
         this.template = template;
         this.parts = this.parse(template);
+
+        // Templates with a literal '?' in a string segment are incompatible
+        // with match()'s split-at-'?' query parsing — the path regex would
+        // include the escaped '?' but the URI is split before matching.
+        // Supporting this requires a fragile two-code-path implementation
+        // that has proven bug-prone, so reject at construction time.
+        const literalQueryPart = this.parts.find(part => typeof part === 'string' && part.includes('?'));
+        if (literalQueryPart !== undefined) {
+            throw new Error(
+                `UriTemplate: literal '?' in template string is not supported. ` +
+                    `Use {?param} to introduce query parameters instead. Template: "${template}"`
+            );
+        }
     }
 
     toString(): string {
@@ -97,7 +118,7 @@ export class UriTemplate {
         return expr
             .slice(operator.length)
             .split(',')
-            .map(name => name.replace('*', '').trim())
+            .map(name => name.replaceAll('*', '').trim())
             .filter(name => name.length > 0);
     }
 
@@ -254,12 +275,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) {
@@ -269,22 +297,50 @@ export class UriTemplate {
             }
         }
 
+        // Strip any URI fragment before splitting path/query.
+        const hashIndex = uri.indexOf('#');
+        const uriNoFrag = hashIndex === -1 ? uri : uri.slice(0, hashIndex);
+
+        // Split URI into path and query parts at the first '?'
+        const queryIndex = uriNoFrag.indexOf('?');
+        const pathPart = queryIndex === -1 ? uriNoFrag : uriNoFrag.slice(0, queryIndex);
+        const queryPart = queryIndex === -1 ? '' : uriNoFrag.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 (const [i, name_] of names.entries()) {
-            const { name, exploded } = name_!;
-            const value = match[i + 1]!;
-            const cleanName = name.replace('*', '');
 
+        for (const [i, { name, exploded }] of names.entries()) {
+            const value = match[i + 1]!;
+            const cleanName = name.replaceAll('*', '');
             result[cleanName] = exploded && value.includes(',') ? value.split(',') : value;
         }
 
+        if (queryParts.length > 0) {
+            const queryParams = new Map<string, string>();
+            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.replaceAll('*', '');
+                const value = queryParams.get(cleanName);
+                if (value === undefined) continue;
+                result[cleanName] = exploded && value.includes(',') ? value.split(',') : value;
+            }
+        }
+
         return result;
     }
 }

packages/core/test/shared/uriTemplate.test.ts

@@ -191,11 +191,97 @@ 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 reject templates with literal ? in a string segment', () => {
+            expect(() => new UriTemplate('/path?fixed=1')).toThrow(/literal '\?'/);
+            expect(() => new UriTemplate('/path?static=1{?dynamic}')).toThrow(/literal '\?'/);
+            expect(() => new UriTemplate('/path?static=1{&dynamic}')).toThrow(/literal '\?'/);
+            expect(() => new UriTemplate('/api?v=2{&key,page}')).toThrow(/literal '\?'/);
+            expect(() => new UriTemplate('/api/{version}?format=json{&key}')).toThrow(/literal '\?'/);
+        });
+
+        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', () => {