INTER-2499 fix: url encode path parameters (api-v3)

.changeset/encode-path-parameters.md

@@ -0,0 +1,5 @@
+---
+'@fingerprintjs/fingerprintjs-pro-server-api': patch
+---
+
+URL-encode path parameters; reject "." and ".." path parameters with a TypeError.

src/urlUtils.ts

@@ -52,6 +52,39 @@ function serializeQueryStringParams(params: QueryStringParameters): string {
   return urlSearchParams.toString()
 }
 
+/**
+ * Confines a value to a single URL path segment. `.` is deliberately left unencoded because
+ * the Server API does not decode path parameters, and valid parameter values can contain dots.
+ *
+ * A value of `.` or `..` is rejected: `new URL()` drops such a segment even when the dots are
+ * encoded, so it cannot be expressed. See https://url.spec.whatwg.org/#double-dot-path-segment
+ */
+function encodePathParam(placeholder: string, value: unknown): string {
+  // Coerce before comparing, because an untyped caller can pass something that is not a string
+  // but stringifies to one. Both conversions throw on values only such a caller could pass:
+  // `String` when the value has no primitive representation, `encodeURIComponent` on a lone
+  // surrogate. Neither should escape as its own error type.
+  let param: string
+  let encoded: string
+  try {
+    // eslint-disable-next-line @typescript-eslint/no-base-to-string -- runtime validation
+    param = String(value ?? '')
+    encoded = encodeURIComponent(param)
+  } catch (cause) {
+    throw new TypeError(`Invalid path parameter for ${placeholder}`, { cause })
+  }
+
+  if (param === '') {
+    throw new TypeError(`Missing path parameter for ${placeholder}`)
+  }
+
+  if (param === '.' || param === '..') {
+    throw new TypeError(`Invalid path parameter for ${placeholder}: ${param}`)
+  }
+
+  return encoded
+}
+
 function getServerApiUrl(region: Region): string {
   switch (region) {
     case Region.EU:
@@ -123,18 +156,18 @@ export function getRequestPath<Path extends keyof paths, Method extends keyof pa
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   method: _,
 }: GetRequestPathOptions<Path, Method>): string {
-  // Step 1: Extract the path parameters (placeholders) from the path
-  const placeholders = Array.from(path.matchAll(/{(.*?)}/g)).map((match) => match[1])
-
-  // Step 2: Replace the placeholders with provided pathParams
-  let formattedPath: string = path
-  placeholders.forEach((placeholder, index) => {
-    if (pathParams?.[index]) {
-      formattedPath = formattedPath.replace(`{${placeholder}}`, pathParams[index])
-    } else {
-      throw new Error(`Missing path parameter for ${placeholder}`)
-    }
-  })
+  // `url.pathname` below is always slash-prefixed, so the normalization check needs a path
+  // that is too. Every generated path key is; this guards a caller that casts past the type.
+  if (!path.startsWith('/')) {
+    throw new TypeError(`Invalid path: ${path} does not start with a slash`)
+  }
+
+  // Replace each `{placeholder}` with its path parameter. The replacement is a function so
+  // that `$&` and friends in a parameter are not read as replacement patterns.
+  let index = 0
+  const formattedPath: string = path.replace(/{(.*?)}/g, (_, placeholder: string) =>
+    encodePathParam(placeholder, pathParams?.[index++])
+  )
 
   const queryStringParameters: QueryStringParameters = {
     ...(queryParams ?? {}),
@@ -148,5 +181,9 @@ export function getRequestPath<Path extends keyof paths, Method extends keyof pa
   url.pathname = formattedPath
   url.search = serializeQueryStringParams(queryStringParameters)
 
+  if (url.pathname !== formattedPath) {
+    throw new TypeError('Invalid path: path changed during normalization')
+  }
+
   return url.toString()
 }

tests/mocked-responses-tests/pathParamEncodingTests.spec.ts

@@ -0,0 +1,86 @@
+import { Region } from '../../src/types'
+import { FingerprintJsServerApiClient } from '../../src/serverApiClient'
+import { getIntegrationInfo } from '../../src'
+
+jest.spyOn(global, 'fetch')
+
+const mockFetch = fetch as unknown as jest.Mock
+
+/**
+ * A path parameter must never be able to change which endpoint or which host the SDK talks
+ * to, no matter what the caller passes in.
+ */
+describe('[Mocked response] Path parameter encoding', () => {
+  const apiKey = 'dummy_api_key'
+  const ii = `ii=${encodeURIComponent(getIntegrationInfo())}`
+
+  const client = new FingerprintJsServerApiClient({ region: Region.EU, apiKey })
+
+  const emptyResponse = () => new Response(undefined, { headers: { 'content-length': '0' } })
+
+  beforeEach(() => {
+    mockFetch.mockClear()
+  })
+
+  const operations = [
+    {
+      name: 'getEvent',
+      prefix: 'events',
+      placeholder: 'request_id',
+      missingParamMessage: 'requestId is not set',
+      call: (param: string) => client.getEvent(param),
+    },
+    {
+      name: 'updateEvent',
+      prefix: 'events',
+      placeholder: 'request_id',
+      missingParamMessage: 'requestId is not set',
+      call: (param: string) => client.updateEvent({ suspect: true }, param),
+    },
+    {
+      name: 'getVisits',
+      prefix: 'visitors',
+      placeholder: 'visitor_id',
+      missingParamMessage: 'VisitorId is not set',
+      call: (param: string) => client.getVisits(param),
+    },
+    {
+      name: 'deleteVisitorData',
+      prefix: 'visitors',
+      placeholder: 'visitor_id',
+      missingParamMessage: 'VisitorId is not set',
+      call: (param: string) => client.deleteVisitorData(param),
+    },
+  ] as const
+
+  describe.each(operations)('$name', ({ prefix, placeholder, missingParamMessage, call }) => {
+    // The full encoding table lives in the unit tests; these are the cases INTER-2499 asks to
+    // be pinned at the wire level for every method.
+    it.each([
+      ['../events', '..%2Fevents'],
+      ['evil.com', 'evil.com'],
+      ['//evil.com', '%2F%2Fevil.com'],
+    ])('requests a single path segment for %j', async (param, encoded) => {
+      mockFetch.mockReturnValue(Promise.resolve(emptyResponse()))
+
+      // The empty body makes some methods reject; only the requested URL matters here.
+      await call(param).catch(() => undefined)
+
+      const requestedUrl = mockFetch.mock.calls[0]?.[0] as string
+      expect(requestedUrl).toEqual(`https://eu.api.fpjs.io/${prefix}/${encoded}?${ii}`)
+      expect(new URL(requestedUrl).host).toEqual('eu.api.fpjs.io')
+    })
+
+    it.each(['.', '..'])('does not send a request for %j', async (param) => {
+      await expect(call(param)).rejects.toThrow(new TypeError(`Invalid path parameter for ${placeholder}: ${param}`))
+
+      expect(mockFetch).not.toHaveBeenCalled()
+    })
+
+    it('does not send a request for an empty parameter', async () => {
+      await expect(call('')).rejects.toThrow(new TypeError(missingParamMessage))
+
+      expect(mockFetch).not.toHaveBeenCalled()
+    })
+  })
+})

tests/unit-tests/urlUtilsTests.spec.ts

@@ -237,3 +237,113 @@ describe('Delete visitor path', () => {
     expect(actualPath).toEqual(expectedPath)
   })
 })
+
+describe('getRequestPath', () => {
+  it('disallows normalized path segments', () => {
+    expect(() => {
+      getRequestPath({
+        path: '/visitors/../events' as '/events/{request_id}',
+        method: 'get',
+        pathParams: [requestId],
+        region: Region.Global,
+      })
+    }).toThrow('Invalid path: path changed during normalization')
+  })
+
+  it('disallows a path without a leading slash', () => {
+    expect(() => {
+      getRequestPath({
+        path: 'events/{request_id}' as '/events/{request_id}',
+        method: 'get',
+        pathParams: [requestId],
+        region: Region.Global,
+      })
+    }).toThrow('Invalid path: events/{request_id} does not start with a slash')
+  })
+})
+
+// Encoding does not depend on which parameter is being replaced, so these run against one
+// path. That every operation routes through it is covered by the mocked-response tests.
+describe('path parameter encoding', () => {
+  const eventPath = (param: unknown) =>
+    getRequestPath({
+      path: '/events/{request_id}',
+      method: 'get',
+      pathParams: [param] as string[],
+      region: Region.Global,
+    })
+
+  it.each([
+    ['../events', '..%2Fevents'],
+    ['../', '..%2F'],
+    ['/../../events', '%2F..%2F..%2Fevents'],
+    ['evil.com', 'evil.com'],
+    ['//evil.com', '%2F%2Fevil.com'],
+    ['https://evil.com', 'https%3A%2F%2Fevil.com'],
+    ['a b#c?d', 'a%20b%23c%3Fd'],
+    ['%2e%2e', '%252e%252e'],
+    ['..%2fevents', '..%252fevents'],
+    ['..\\..', '..%5C..'],
+    ['$&', '%24%26'],
+    ['...', '...'],
+    // A placeholder in a parameter must not reach the next replacement, which would matter
+    // for a path with two placeholders.
+    ['{request_id}', '%7Brequest_id%7D'],
+    ['1626550679751.cVc5Pm', '1626550679751.cVc5Pm'],
+    // An untyped caller can pass something that is not a string but stringifies to one.
+    [new String('../events'), '..%2Fevents'],
+  ])('keeps %j inside a single path segment', (param, encoded) => {
+    expect(eventPath(param)).toEqual(`https://api.fpjs.io/events/${encoded}?${ii}`)
+  })
+
+  it.each([
+    ['.', '.', 'Invalid path parameter for request_id: .'],
+    ['..', '..', 'Invalid path parameter for request_id: ..'],
+    ['a String object', new String('..'), 'Invalid path parameter for request_id: ..'],
+    ['an object with a toString', { toString: () => '..' }, 'Invalid path parameter for request_id: ..'],
+    ['an array', ['..'], 'Invalid path parameter for request_id: ..'],
+    // A lone surrogate makes `encodeURIComponent` throw a `URIError`
+    ['a lone surrogate', '\ud800', 'Invalid path parameter for request_id'],
+    // These have no primitive representation, so `String` itself throws
+    ['an object without a prototype', Object.create(null), 'Invalid path parameter for request_id'],
+    [
+      'an object whose toString throws',
+      {
+        toString: () => {
+          throw new Error('boom')
+        },
+      },
+      'Invalid path parameter for request_id',
+    ],
+  ])('rejects %s', (_, param, message) => {
+    // The message is matched through `objectContaining` because some of these carry a
+    // `cause`, which `toThrow(new TypeError(...))` would compare too.
+    expect(() => eventPath(param)).toThrow(TypeError)
+    expect(() => eventPath(param)).toThrow(expect.objectContaining({ message }))
+  })
+
+  it('preserves the cause when string coercion fails', () => {
+    const cause = new Error('boom')
+    const param = {
+      toString: () => {
+        throw cause
+      },
+    }
+
+    expect(() => eventPath(param)).toThrow(
+      expect.objectContaining({
+        message: 'Invalid path parameter for request_id',
+        cause,
+      })
+    )
+  })
+
+  it.each([
+    ['an empty string', ''],
+    ['an empty String object', new String('')],
+    ['null', null],
+    ['undefined', undefined],
+  ])('rejects %s as missing', (_, param) => {
+    expect(() => eventPath(param)).toThrow(new TypeError('Missing path parameter for request_id'))
+  })
+})