fix: UriTemplate.match() handles optional, out-of-order, and encoded query parameters#1785
fix: UriTemplate.match() handles optional, out-of-order, and encoded query parameters#1785felixweinberger wants to merge 16 commits intomainfrom
Conversation
Absent query parameters are now omitted from the result object rather
than set to empty string, so callers can use `vars.param ?? default`.
This also distinguishes 'param absent' from 'param present but empty'
(e.g. ?q= returns {q: ''}).
Also removes dead code: the isQuery field was always false since query
parts go to a separate array, and the (?:\\?.*)?$ regex suffix was
unreachable since pathPart already excludes the query string.
🦋 Changeset detectedLatest commit: 585c5f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…delcontextprotocol/typescript-sdk into fweinberger/uri-template-query-params
…agment handling
- Remove the hasLiteralQuery special-case branch from match(). Four rounds
of review found bugs in it; the two-code-path design was inherently fragile.
- Reject any literal '?' in template string segments at construction time
with a clear error pointing at the {?param} alternative. No real-world
MCP resource templates use this pattern.
- Fix fragment handling: strip #fragment before splitting path/query, so
/path?a=1#frag correctly extracts {a:'1'} instead of {a:'1#frag'}.
- Replace 7 hasLiteralQuery tests with construction-validation tests and
fragment-stripping tests.
Net: -33 lines of match() logic, one code path, correct by construction.
| 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; | ||
| } |
There was a problem hiding this comment.
🔴 Path variable captures in match() are returned as raw percent-encoded strings (lines 317-321), while query parameters are decoded via safeDecode() (lines 329-330) — an asymmetry newly introduced by this PR. This breaks the round-trip fidelity goal the PR describes: expand({ q: 'hello world' }) for {+q} yields /hello%20world, but match('/hello%20world') returns { q: 'hello%20world' }, so a second expand() double-encodes to /hello%2520world. The fix is to apply safeDecode to path variable captures in the names loop at lines 317-321, mirroring the query param handling.
Extended reasoning...
What the bug is
This PR adds safeDecode() calls when parsing query parameters (lines 329-330) so that {?q} matching ?q=hello%20world returns { q: 'hello world' }. However, path variable captures at lines 317-321 are returned without any decoding — the raw regex capture group value is stored directly in result[cleanName]. This creates a new inconsistency between the two handling paths.
The specific code path
In match(), path variable captures go through the names loop with no decoding applied. Query params go through safeDecode() at lines 329-330 before being stored. The path variable loop has no equivalent decoding step.
Why existing code does not prevent it
Before this PR, neither path variables nor query params were decoded — the behavior was symmetrically raw. This PR adds decoding only for query params, introducing a split where one code path decodes and the other does not. The path variable loop at lines 317-321 was not updated to match.
Impact and round-trip failure
The PR description explicitly advertises URL-encoded values are decoded and round-trip fidelity (expand(match(uri)) === uri) as goals. These goals hold for query params but fail for path operators:
- expand({ q: 'hello world' }) with template {+q} yields /hello%20world (via encodeURI)
- match('/hello%20world') returns { q: 'hello%20world' } (NOT decoded — returns raw)
- expand({ q: 'hello%20world' }) with template {+q} yields /hello%2520world (double-encoded!)
This also affects the simple {var} operator: template /users/{id} matching /users/john%20doe returns { id: 'john%20doe' }, while {?id} matching ?id=john%20doe returns { id: 'john doe' }.
Addressing the refutation
One verifier argues this is pre-existing because path variable non-decoding predates the PR and lines 317-321 are unchanged. This is technically accurate, but the PR creates a new user-visible inconsistency: prior to this PR, both operator types behaved the same (both raw). After this PR, the behavior diverges. The PR's own stated goals imply users should expect decoding to work uniformly, making this worth flagging as part of this PR rather than as a separate issue.
Fix
Apply safeDecode to path variable values in the names loop at lines 317-321, similar to the query param handling at lines 329-330. This mirrors the query param decoding and satisfies the round-trip guarantee for all operator types.
There was a problem hiding this comment.
Path-variable decoding is pre-existing behavior (base code never decoded either) and out of scope for this query-param fix. Will track applying safeDecode() to path captures separately.
… throw on literal ? at construction
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 The new match() routes {&} operators to queryParts (same as {?}) and splits the URI at ?, but a standalone {&} template (no preceding {?}) produces URIs with & but no ? — so the split finds nothing and the path regex (which omits the & part) fails to match. For template /path{&extra}, expand({extra: 'val'}) = '/path&extra=val', but match('/path&extra=val') returns null. Before this PR, partToRegExp for {&extra} generated \&extra=([^&]+) in the path regex, which matched correctly.
Extended reasoning...
What the bug is and how it manifests
The new match() implementation separates template parts into two buckets: path parts (used to build the regex) and query parts ({?}/{&} operators). For query parts, the URI is split at the first ? to extract the query string. However, RFC 6570's {&} (form-style query continuation) operator is specifically designed to add &key=val segments without a leading ?. When {&} is used standalone (no preceding {?} or literal ?), the expanded URI contains & but no ? delimiter.
The specific code path that triggers it
In match() at lines 274-280, part.operator === '&' routes the operator names into queryParts (same as {?}). At lines 302-307, queryParts.length > 0 causes the code to look for ? in the URI. For a standalone {&} template like /path{&extra}, the expanded URI /path&extra=val has no ?, so queryIndex = -1, pathPart = '/path&extra=val', queryPart = ''. The path regex is built as ^/path$ (the {&extra} part contributed nothing to the path regex, having been routed to queryParts). Matching '/path&extra=val' against ^/path$ fails and returns null.
Why existing code doesn't prevent it
Before this PR, partToRegExp was called for {&} operators and generated \&name=([^&]+) patterns added directly to the path regex. The full URI was matched against the full regex, so ^/path\&extra=([^&]+)$ correctly matched /path&extra=val. The new code short-circuits {?}/{&} before calling partToRegExp, routing them to queryParts instead — which only works when there is a ? to split at.
What the impact would be
Round-trip fidelity breaks: expand(match(uri)) \!== uri because match returns null for valid expanded URIs using standalone {&}. The expand side still works correctly (X{&var} expands to X&var=test per existing tests), making the asymmetry subtle. Any MCP resource template or client using {&} without a preceding {?} will silently fail to match.
How to fix it
Only route {&} operators to queryParts when the template also contains a {?} operator or a literal ? in a string segment (i.e., when there will actually be a ? in expanded URIs). Otherwise, fall back to calling partToRegExp for {&} parts (the pre-PR behavior), keeping them in the path regex where they can match the &key=val suffix directly.
Step-by-step proof
- Template:
/path{&extra}, URI:/path&extra=val - Parts loop: literal
'/path'→pattern = '^/path';{&extra}→ appended toqueryParts = [{name:'extra'}] hasHashOperator = false; no#in URI →working = '/path&extra=val'queryParts.length > 0→ search for?in'/path&extra=val'→queryIndex = -1(not found)pathPart = '/path&extra=val',queryPart = ''- Final
pattern = '^/path$';'/path&extra=val'.match(/^\/path$/)→null - Returns
null— regression from pre-PR behavior where^/path\&extra=([^&]+)$matched'/path&extra=val'and returned{extra: 'val'}
| // 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); | ||
| } |
There was a problem hiding this comment.
🔴 The fragment-stripping guard at line 289 only checks p.operator === '#', but the {+var} (reserved expansion) operator also uses (.+) in partToRegExp and is RFC 6570-defined to pass reserved characters including # — so 'http://e.com{+rest}'.match('http://e.com/path#section') returns { rest: '/path' } instead of { rest: '/path#section' }. The fix is to extend hasHashOperator to also include templates with + operators: this.parts.some(p => typeof p \!== 'string' && (p.operator === '#' || p.operator === '+')).
Extended reasoning...
What the bug is
The match() method introduced in this PR adds a pre-processing step that strips the URI fragment before regex matching (lines 287–294). The guard condition hasHashOperator is supposed to skip this stripping when a {#var} operator is present, because the fragment is part of what the path regex must capture. However, the {+var} (reserved expansion) operator also generates the pattern (.+) in partToRegExp (via case '+': case '#': pattern = '(.+)') and is RFC 6570-defined to pass reserved characters through unencoded — including #. The check only looks at p.operator === '#' and never considers p.operator === '+'.
The specific code path
For template 'http://e.com{+rest}' and URI 'http://e.com/path#section':
hasHashOperator = this.parts.some(p => typeof p \!== 'string' && p.operator === '#')→ false (only+operator present)- Fragment IS stripped:
hashIndexfinds#, soworking = 'http://e.com/path' queryParts.length === 0(no?/&operators) → no query split;pathPart = 'http://e.com/path'- Path regex
^http://e\.com(.+)$is matched against'http://e.com/path'→ captures'/path' - Result:
{ rest: '/path' }— fragment silently dropped
Why existing code doesn't prevent it
Both + and # operators generate identical (.+) patterns in partToRegExp. The hasHashOperator guard was added to protect # captures from fragment stripping, but the symmetric case for + was overlooked. The check is simply incomplete.
Impact
This is a regression introduced by this PR. Before the PR, match() applied the regex to the full URI string, so (.+) correctly captured '/path#section'. After the PR, the fragment is stripped first, silently breaking round-trip fidelity for {+var} templates. The round-trip guarantee advertised by the PR (expand(match(uri)) === uri) fails: expand({ rest: '/path#section' }) yields 'http://e.com/path#section', but match('http://e.com/path#section') returns { rest: '/path' }, so a second expand produces 'http://e.com/path'.
How to fix
Extend hasHashOperator to also cover the + operator:
const hasReservedOrHashOperator = this.parts.some(
p => typeof p \!== 'string' && (p.operator === '#' || p.operator === '+')
);Step-by-step proof
- Template:
'http://e.com{+rest}', parsed parts:['http://e.com', { operator: '+', names: ['rest'], ... }] hasHashOperator=false(the part's operator is'+', not'#')hashIndex = uri.indexOf('#') = 20→working = 'http://e.com/path'queryParts.length = 0→pathPart = 'http://e.com/path'- Regex
/^http:\/\/e\.com(.+)$/matches'http://e.com/path'→match[1] = '/path' result = { rest: '/path' }—'#section'is permanently lost
| 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; | ||
| } |
There was a problem hiding this comment.
🔴 The exploded query param handler decodes values via safeDecode() before checking for commas, so a percent-encoded comma (%2C) in a single array element gets turned into a literal comma and causes an incorrect split. For example, expand({tags: ['a,b']}) with {?tags*} produces ?tags=a%2Cb, but match('/path?tags=a%2Cb') returns {tags: ['a', 'b']} instead of {tags: ['a,b']}. Fix by splitting on commas in the raw encoded value first, then decoding each element individually.
Extended reasoning...
What the bug is and how it manifests
In match(), query parameter values are decoded via safeDecode() at line 330 and stored in the queryParams Map as decoded strings. Then at line 340, the exploded check exploded && value.includes(',') ? value.split(',') : value operates on the already-decoded value. For a single array element containing a literal comma (e.g., tag value "a,b"), encodeURIComponent encodes the comma to %2C during expansion, but safeDecode converts it back to , before the split check, incorrectly creating two array elements.
The specific code path
- Line 330 (in the query params parsing loop):
const value = safeDecode(pair.slice(equalIndex + 1));— this decodes the full value, including any%2Csequences into literal commas. - Line 340 (in the queryParts processing loop):
result[cleanName] = exploded && value.includes(',') ? value.split(',') : value;— this operates on the already-decoded string, so"a,b"(decoded from"a%2Cb") is split into['a', 'b'].
Why existing code does not prevent it
Prior to this PR, neither path variable captures nor query parameters were decoded — both returned raw encoded strings. The comma-split check was always performed on the raw encoded string, where %2C would not trigger a split. This PR introduces safeDecode() for query parameters but reuses the existing post-decode comma-split check, creating a new bug: commas that were intentionally percent-encoded to be part of a single value are now treated as array delimiters.
What the impact would be
Any {?name*} or {&name*} template whose elements contain literal commas will fail round-trip fidelity. Specifically, expand(match(uri)) \!== uri for any URI where an exploded array element contains %2C. This is a regression introduced by this PR — the pre-PR behavior was at least consistent (always returning raw strings).
How to fix it
Split on literal commas in the raw encoded value first, then decode each element individually:
// Instead of:
const value = safeDecode(pair.slice(equalIndex + 1));
queryParams.set(key, value);
// ...later...
result[cleanName] = exploded && value.includes(',') ? value.split(',') : value;
// Do:
const rawValue = pair.slice(equalIndex + 1);
queryParams.set(key, rawValue); // store raw, not decoded
// ...later...
if (exploded && rawValue.includes(',')) {
result[cleanName] = rawValue.split(',').map(safeDecode);
} else {
result[cleanName] = safeDecode(rawValue);
}Step-by-step proof
- Template:
{?tags*}, value:{tags: ['a,b', 'c']} expand({tags: ['a,b', 'c']})→?tags=a%2Cb,c(encodeURIComponent encodes the comma inside'a,b'to%2C, the inter-element comma is left literal)- In
match('/path?tags=a%2Cb,c'):- pair =
'tags=a%2Cb,c' safeDecode('a%2Cb,c')='a,b,c'(DECODED: now has two commas)- queryParams.set('tags', 'a,b,c')
exploded=true,'a,b,c'.includes(',') = true'a,b,c'.split(',') = ['a', 'b', 'c']— WRONG (should be['a,b', 'c'])
- pair =
- Correct approach: split
'a%2Cb,c'on literal commas →['a%2Cb', 'c'], then decode each →['a,b', 'c']
| // 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 When a template uses both {?param} and {#var} operators together (e.g., /path{?a}{#b}), match() always returns null for valid URIs like /path?a=1#section. The two new conditional preprocessing steps interact incorrectly: hasHashOperator=true prevents fragment stripping, but queryParts.length>0 then splits at '?', placing the fragment into queryPart ('a=1#section') and leaving pathPart='/path' which cannot satisfy the (.+) capture pattern that {#b} adds to the path regex. Additionally, even if matching succeeded, the fragment would leak into the query value for 'a'.
Extended reasoning...
What the bug is and how it manifests
When a template contains both a {?...} query operator and a {#var} fragment operator (e.g., /path{?a}{#b}), the match() method unconditionally returns null for any valid matching URI. This is caused by two new conditional preprocessing steps introduced by this PR that were designed independently but fail when combined.
The specific code path that triggers it
For template /path{?a}{#b} and URI /path?a=1#section:
- Parts loop: {?a} goes to queryParts; {#b} is sent to partToRegExp (operator # is not ?/&), which adds (.+) to pattern and b to names. Final path pattern: ^/path(.+)$.
- Fragment check: hasHashOperator = true, so the fragment is NOT stripped. working = /path?a=1#section.
- Query split: queryParts.length > 0, so split at ?: pathPart = /path, queryPart = a=1#section.
- Path match: /path matched against /^/path(.+)$/ returns null because (.+) requires at least one character after /path, but pathPart is exactly /path.
Result: null. Expected: {a: '1', b: '#section'}.
Why existing code does not prevent it
The hasHashOperator check was added to preserve the fragment for {#} capture; the queryParts.length > 0 check was added to split path from query for {?} parsing. Each works correctly in isolation. But when both operators appear in the same template, they interact destructively: fragment preservation keeps the fragment in working, but the query split then moves that fragment into queryPart rather than leaving it available for path regex capture by the {#} pattern.
Impact
Any URI template combining {?...} and {#...} operators always returns null from match(), even for perfectly valid matching URIs. Additionally, even if the path match were somehow forced to succeed, queryPart = a=1#section would assign value 1#section to variable a — meaning the fragment leaks into the query value.
How to fix
When both hasHashOperator and queryParts.length > 0 are true, the fragment must be separated before the ? split. One approach: always strip the fragment from working before splitting at ?, record the fragment separately, then pass it back to the path match when hasHashOperator is true (appending it to pathPart so the {#} pattern can capture it).
Step-by-step proof
- Template: /path{?a}{#b}, URI: /path?a=1#section
- After parts loop: pattern = '^/path(.+)$', names = [{name:'b'}], queryParts = [{name:'a'}]
- hasHashOperator = true -> working = /path?a=1#section (no stripping)
- queryParts.length > 0 -> split at ?: pathPart = /path, queryPart = a=1#section
- /path.match(/^/path(.+)$/) -> null
- match() returns null — BUG: should return {a: '1', b: '#section'}
- No test in the test suite covers this combined {?}+{#} case
3 participants
Supersedes #1396. Implementation by @mgyarmathy, with a refinement to how absent query parameters are represented.
Fixes
UriTemplate.match()to correctly handle query parameter templates per RFC 6570 semantics:Motivation and Context
Fixes #149
Fixes #1079
Previously, a resource template like
products{?page,limit}would fail to matchproductsorproducts?limit=10&page=1(wrong order), returningnulland causing a "Resource not found" error. This made query parameter templates effectively unusable.Design: absent parameters are omitted, not empty string
Absent query parameters are omitted from the result rather than set to
''. This differs from #1396, which returned empty strings. Rationale:Round-trip fidelity. The SDK's
expand()already distinguishes these cases:Omitting keys preserves
expand(match(uri)) === uri. Returning''would turn/itemsinto/items?page=&limit=on round-trip.RFC 6570 §2.3 explicitly distinguishes undefined from empty string ("A variable value that is a string of length zero is not considered undefined"). The RFC only specifies expansion, not matching, but this semantic distinction should be preserved in both directions.
Ecosystem convention. The reference JS implementation (uri-templates) omits keys for absent params.
Ergonomics. Enables the idiomatic
vars.page ?? defaultValuepattern. Empty string would silently break??defaulting.This means callers can distinguish "parameter absent" (
vars.page === undefined) from "parameter present but empty" (vars.page === '', e.g.?page=).How Has This Been Tested?
New test cases cover: partial matches, out-of-order params, omitted params, extra params, encoded values, and the absent-vs-empty distinction. All 461 core tests pass.
Breaking Changes
Resource templates with query parameters now match more liberally. If you relied on strict all-params-required matching to reject requests, you'll need to validate explicitly in your callback. See
docs/migration.md.Types of changes
Checklist
Additional context
Core implementation (path/query splitting, Map-based query parsing, URL decoding) by @mgyarmathy in #1396. This PR adds the absent-param-omission refinement, changeset, and migration note.