req.params includes port number, regression 0.35.0-0.36.0

[AriPerkkio]

Describe the bug

When upgrading msw@0.35.0 to latest I noticed one of my utility method started to fail. I narrowed this issue to msw@0.36.0.

Some background. Not necessary for the bug but gives some insight.

I have a utility method which validates that all req.params values are encrypted. I have a specific pattern which can be utilized to identify such fields. For now lets call this pattern /somepattern/.

function validateQueryParametersAreEncrypted(request) {
    Object.entries(request.params).forEach(([name, value]) => {
        if (!/somepattern/.test(value)) {
            throw new Error(`URL parameter (${name}) was not encrypted: (${value})`);
        }
    });
}

This validation is applied to all my MSW handlers. It is now failing due to :3000 not matching /somepattern/.

Environment

• msw: 0.36.3
• nodejs: 16.13.1
• npm: 8.3.0

Please also provide your browser version.

• Google Chrome 96

To Reproduce

Steps to reproduce the behavior:

1. Setup https://github.com/mswjs/examples/tree/master/examples/rest-react
2. Add new handler:

rest.get("/example/:id", (req, res, ctx) => {
    console.log("req.params", req.params);
    return res(ctx.status(200));
})

3. Add new request: fetch("/example/someid");.

Expected behavior

Below is logs from different MSW versions. Version 0.35.0 works as expected.

rest.get("/example/:id", (req, res, ctx) => {
    console.log("req.params", req.params);
    // ^^ msw@0.35.0 req.params {id: 'someid'}
    // ^^ msw@0.36.0 req.params {3000: ':3000', id: 'someid'}
    // ^^ msw@0.36.1 req.params {3000: ':3000', id: 'someid'}
    // ^^ msw@0.36.2 req.params {3000: ':3000', id: 'someid'}
    // ^^ msw@0.36.3 req.params {3000: ':3000', id: 'someid'}

    return res(ctx.status(200));
});

[AriPerkkio]

Debugged the request on browser and built minimal repro for unit testing:

import { match } from 'path-to-regexp'

test('match should not include port number in params', () => {
  const out = match('http\\://localhost:3000/example/:id', {
    decode: decodeURIComponent,
  })('http://localhost:3000/example/someid')

  expect(out).toHaveProperty('params', { id: 'someid' })
})

    - Expected value  - 0
    + Received value  + 1

      Object {
    +   "3000": ":3000",
        "id": "someid",
      }

    > 12 |   expect(out).toHaveProperty('params', { id: 'someid' })

I got these input arguments logged out from these lines in browser:

msw/src/handlers/RestHandler.ts

Lines 153 to 159 in b3c34e1

	parse(request: RequestType, resolutionContext?: ResponseResolutionContext) {
	return matchRequestUrl(
	request.url,
	this.info.path,
	resolutionContext?.baseUrl,
	)
	}

msw/src/utils/matching/matchRequestUrl.ts

Line 64 in b3c34e1

const result = match(cleanPath, { decode: decodeURIComponent })(cleanUrl)

Do these argument look alright or is this a bug in path-to-regexp?

[kettanaito]

Hey, @AriPerkkio. Thank you for reporting and investigating this!

This is a regression likely related to our internal migration to path-to-regexp, which only supports relative URL paths. To accommodate for that, we are transforming absolute paths to properly escaped strings so that path-to-regexp could match them.

Here's where we're escaping the port number:

msw/src/utils/matching/matchRequestUrl.ts

Lines 39 to 43 in 527add2

	/**
	* Escape the port so that "path-to-regexp" can match
	* absolute URLs including port numbers.
	*/
	.replace(/([^\/])(:)(?=\d+)/, '$1\\$2')

So a path string like /user/:id becomes this:

https\\://localhost\\:3000/user/:id

Which gets matched correctly given to path-to-regexp. That's what fails the unit test reproduction you've written: the port number is not escaped. Take a look at our current unit tests for the matchRequestUrl function to see the input path string that we provide to path-to-regexp:

msw/src/utils/matching/matchRequestUrl.test.ts

Lines 77 to 90 in 527add2

	test('escapes the colon before the port number', () => {
	expect(coercePath('localhost:8080')).toEqual('localhost\\:8080')
	expect(coercePath('http://127.0.0.1:8080')).toEqual(
	'http\\://127.0.0.1\\:8080',
	)
	expect(coercePath('https://example.com:1234')).toEqual(
	'https\\://example.com\\:1234',
	)
	expect(coercePath('localhost:8080/:5678')).toEqual('localhost\\:8080/:5678')
	expect(coercePath('https://example.com:8080/:5678')).toEqual(
	'https\\://example.com\\:8080/:5678',
	)
	})

[kettanaito]

There must be something else at play as I cannot reproduce the issue following the steps you've mentioned. Here's what I got:

// src/mocks/handlers.js
import { rest } from 'msw'

export const handlers = [
  rest.get('/example/:id', (req, res, ctx) => {
    return res(ctx.text(req.params.id))
  }),
]

The actual request:

useEffect(() => {
  fetch('/example/abc-123')
}, [])

The response:

200 OK
text/plain

abc-123

@AriPerkkio, may you please double-check that you're indeed using the latest msw? I recommend uninstalling it and installing it again:

$ npm remove msw
$ npm install msw -D

If the issue still persists, may I ask you to isolate it to a standalone repo (you can use CRA or any other starter)? Thanks.

[AriPerkkio]

As the current handler is not returning all parameters could you try with following changes:

export const handlers = [
  rest.get('/example/:id', (req, res, ctx) => {
-    return res(ctx.text(req.params.id))
+    return res(ctx.json(req.params))
  }),
]

useEffect(() => {
-  fetch('/example/abc-123')
+  fetch('/example/abc-123').then(r => r.json()).then(console.log)
}, [])

Now the handler should also be returning the port number in the response.

I quickly verified the version of MSW with following logging using the same mswjs/examples repository:

  rest.get('/example/:id', (req, res, ctx) => {
    console.log('msw/package.json version', require('msw/package.json').version)
    // ^^ msw/package.json version 0.36.0

    console.log('req.params', req.params)
    // ^^  req.params {3001: ':3001', id: 'someid'}
    return res(ctx.json(req.params))
  }),

I'm happy to build a separate minimal repro repository if needed but I think these will be enough - let me know if following changes help.

[kettanaito]

Oh, that's a mistake on my side. Thanks for pointing it out 👍
I'll update the issue with the req.params change.

[AriPerkkio]

I was comparing the bundled coercePath and the one from source, and was able to find the likely root cause. It seems main branch contains unreleased changes which might have fixed these, #1028 (👋 @tdeekens).

Below is comparison between 0.36.3 and main.

coercePath msw@0.36.3

function coercePath(path) {
  return (
    path
      /**
       * Replace wildcards ("*") with unnamed capturing groups
       * because "path-to-regexp" doesn't support wildcards.
       * Ignore path parameter' modifiers (i.e. ":name*").
       */
      .replace(
        /([:a-zA-Z_-]*)(\*{1,2})+/g,
        (_, parameterName, wildcard) => {
          const expression = '(.*)'

          if (!parameterName) {
            return expression
          }

          return parameterName.startsWith(':')
            ? `${parameterName}${wildcard}`
            : `${parameterName}${expression}`
        },
      )
      /**
       * Escape the protocol so that "path-to-regexp" could match
       * absolute URL.
       * @see https://github.com/pillarjs/path-to-regexp/issues/259
       */
      .replace(/^([^\/]+)(:)(?=\/\/)/g, '$1\\$2')
  )
}

> coercePath('http://localhost:3000/example/:id')
> 'http\\://localhost:3000/example/:id'
//                   ^^ Incorrect

coercePath msw@main, unreleased

function coercePath(path) {
  return (
    path
      /**
       * Replace wildcards ("*") with unnamed capturing groups
       * because "path-to-regexp" doesn't support wildcards.
       * Ignore path parameter' modifiers (i.e. ":name*").
       */
      .replace(
        /([:a-zA-Z_-]*)(\*{1,2})+/g,
        (_, parameterName, wildcard) => {
          const expression = '(.*)'

          if (!parameterName) {
            return expression
          }

          return parameterName.startsWith(':')
            ? `${parameterName}${wildcard}`
            : `${parameterName}${expression}`
        },
      )
      /**
       * Escape the port so that "path-to-regexp" can match
       * absolute URLs including port numbers.
       */
      .replace(/([^\/])(:)(?=\d+)/, '$1\\$2')
      /**
       * Escape the protocol so that "path-to-regexp" could match
       * absolute URL.
       * @see https://github.com/pillarjs/path-to-regexp/issues/259
       */
      .replace(/^([^\/]+)(:)(?=\/\/)/, '$1\\$2')
  )
}

> coercePath('http://localhost:3000/example/:id')
> 'http\\://localhost\\:3000/example/:id'
//                   ^^ Correct

So it seems coercePath will be buggy in releases 0.36.0 - 0.36.3 only.
Once new version is released I can do the final verification.

[kettanaito]

That's a great find, @AriPerkkio! I'll publish the latest main so this issue would be resolved.

Update: published in 0.36.4.

Could you please update to that version and let me know if the issue is gone? Thanks.

[AriPerkkio]

Fix verified with version 0.36.4 - port number is no longer included in the req.params. Thanks!