INTER-2499 fix: url encode path parameters - #282
Conversation
Refs INTER-2499
🦋 Changeset detectedLatest commit: 66c2cb4 The changes in this PR will be included in the next version bump. 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 |
Coverage report
Test suite run success130 tests passing in 30 suites. Report generated by 🧪jest coverage report action from 66c2cb4 Show full coverage report
|
There was a problem hiding this comment.
🟡 Changes recommended
The changeset release note doesn’t mention the new dot-segment (./..) rejection behavior, which is user-visible and should be documented for consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens URL construction for the Fingerprint Server API client by ensuring path parameters cannot escape their intended single path segment, preventing path traversal/endpoint confusion during request building.
Changes:
- URL-encodes all path parameters in
getRequestPathvia a dedicatedencodePathParamhelper. - Rejects dot-segment path parameters (
.and..) by throwingTypeErrorto avoidURLnormalization changing the requested endpoint. - Adds unit and mocked wire-level tests asserting the exact URL passed to
fetchand that the host cannot be altered via path params.
File summaries
| File | Description |
|---|---|
src/urlUtils.ts |
Adds encodePathParam and applies it to placeholder replacement in getRequestPath. |
tests/unit-tests/urlUtilsTests.spec.ts |
Adds a table-driven unit suite validating path-param encoding and dot-segment rejection. |
tests/mocked-responses-tests/pathParamEncodingTests.spec.ts |
Adds integration-style tests ensuring client operations request the expected encoded URL and never change host. |
.changeset/encode-path-parameters.md |
Adds a changeset entry describing the release note for this behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
There is a validated bypass for the dot-segment rejection when non-primitive (truthy) path params are passed by untyped JS callers, which can reintroduce the normalization issue.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
5592f65 to
83e57e1
Compare
An untyped caller could pass a value that is not a string but stringifies to a dot segment, bypassing the rejection.
There was a problem hiding this comment.
🟢 Approval recommended
The change is localized to the URL construction choke point, aligns with the stated security intent, and is backed by both unit and wire-level tests covering key edge cases.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
JuroUhlar
changed the title
encodeURIComponent throws URIError on a lone surrogate, which escaped as a third error type at the public boundary. Also unifies the missing-parameter throw on TypeError.
Replace placeholders in one pass with a function replacement, move all validation into encodePathParam, and drop test rows that were duplicated across both paths.
There was a problem hiding this comment.
🟡 Changes recommended
encodePathParam can leak unexpected exceptions from String(value) coercion (e.g., Symbols or throwing toString), bypassing the intended consistent TypeError behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
String() throws for values with no primitive representation, which leaked as its own error type.
There was a problem hiding this comment.
🟢 Approval recommended
The security fix is localized to URL construction and is backed by both unit-level and wire-level tests, with only a minor doc-comment clarity note.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/urlUtils.ts:48
- The
encodePathParamdoc comment mentions “valid event IDs” even though the helper is generic (also used forvisitor_idetc.), which can be misleading. Consider wording it in a placeholder-agnostic way while still noting why dots are preserved.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
mcnulty-fp
left a comment
mcnulty-fp
left a comment
There was a problem hiding this comment.
Great work! Just a few minor comments about perhaps improving the tests and the implementation a bit.
Co-authored-by: Dan McNulty <212590662+mcnulty-fp@users.noreply.github.com>
Co-authored-by: Dan McNulty <212590662+mcnulty-fp@users.noreply.github.com>
🚀 Following releases will be created using changesets from this PR:@fingerprint/node-sdk@7.7.1Patch Changes
|
Fixes [INTER-2499](https://fingerprintjs.atlassian.net/browse/INTER-2499) on the `api-v3` line. Backport of #282. Path params were interpolated raw, so `getEvent('../events/search')` requested `/events/search`. - Encode path params with `encodeURIComponent` in `getRequestPath`, the choke point for `getEvent`, `updateEvent`, `getVisits` (and the deprecated `getVisitorHistory`) and `deleteVisitorData`. - Reject `.` and `..` with a `TypeError`. - Unit tests for the encoding table; wire-level tests per operation asserting the exact `fetch` URL and host. Real IDs are `encodeURIComponent` no-ops, so nothing changes on the wire for legitimate traffic. ### Differences from #282 - `getRequestPath` on this branch keeps its generic signature and has no `apiVersion` prefix, so the normalization guard compares `url.pathname` against the path as-is. - Tests are Jest, so the three cases carrying an error `cause` assert the type and the message separately — `toThrow(new TypeError(...))` compares `cause` too. - The wire-level table also covers `getVisits`, which does not exist on v4. ### Discussion point: why `.` and `..` throw instead of being encoded The URL Standard treats `%2e` as a dot when matching [dot-path segments](https://url.spec.whatwg.org/#double-dot-path-segment), so `new URL()` drops the segment for `..` and `%2E%2E` alike. A literal dot segment is unrepresentable, so throwing is the only option. [INTER-2499]: https://fingerprintjs.atlassian.net/browse/INTER-2499?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Fixes INTER-2499. Path params were interpolated raw, so
getEvent('../events')requested/v4/events.encodeURIComponentingetRequestPath, the choke point forgetEvent,updateEventanddeleteVisitorData..and..with aTypeError.fetchURL and host.Real IDs are
encodeURIComponentno-ops, so nothing changes on the wire for legitimate traffic.Discussion point: why
.and..throw instead of being encodedThe URL Standard treats
%2eas a dot when matching dot-path segments, sonew URL()drops the segment for..and%2E%2Ealike. A literal dot segment is unrepresentable, so throwing is the only option.