Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot match absolute URLs with port number #1027

Closed
tdeekens opened this issue Dec 12, 2021 · 5 comments · Fixed by #1028
Closed

Cannot match absolute URLs with port number #1027

tdeekens opened this issue Dec 12, 2021 · 5 comments · Fixed by #1028
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@tdeekens
Copy link
Contributor

tdeekens commented Dec 12, 2021

Describe the bug

Recently in version 0.36.0 path-to-regexp was used over node-match-path to have similar matching behaviour as react-router and express. However, path-to-regexp doesn't seem to match absolute URLs as node-match-path does. It feels that using path-to-regexp is a super valuable step forward but can more easily break mocks than anticipated.

Environment

  • msw: 0.36.3
  • nodejs: 17.x.x

To Reproduce

Example 1

  1. Define a handler with an absolute url such as rest.get('http://localhost:1234/*')
  2. Execute a request against http://localhost:1234/foo/bar

Example 2

  1. Define a handler with an absolute url such as rest.get('/:firstSegment/:secondSegment')
  2. Execute a request against http://localhost:1234/foo/bar

Example 3

  1. Define a handler with an absolute url such as rest.get('http\\://localhost\\:3456/:project/:application')
  2. Execute a request against http://localhost:1234/foo/bar

In all examples nothing will be matched or an error will be thrown.

Expected behavior

The URL to be matched or improved documentation how to deal with absolute URLs. It seems that path-to-regexp requires some escaping while not working with "catch-all" handers as well. Given that path-to-regexp will continue to be used a few more practical examples for migrating handlers with full urls could help the community if I am not the only one having this issue.

@tdeekens tdeekens added the bug Something isn't working label Dec 12, 2021
@tdeekens
Copy link
Contributor Author

tdeekens commented Dec 12, 2021

Just learning that msw tries to coerce the path to a path-to-regexp compatible version e.g. converting a http://localhost:3456/*" to a "http\\://localhost:3456/(.*). Still unsure why my handler stopped matching.

I just assumed it wouldn't do that when debugging and ended up with double escaping.

Update: I assume cause the port is not escaped.

@tdeekens
Copy link
Contributor Author

tdeekens commented Dec 12, 2021

Still even with http://localhost\\:1234/* I am unable to match http://localhost:1234/foo/bar and I am unsure why. Also cause this example works:

const { match } = require("path-to-regexp");

console.log(match("http\\://localhost\\:3456/(.*)")('http://localhost:3456/foo/bar'))

Returning

{ path: "http://localhost:3456/foo/bar", index: 0, params: Object {0: "foo/bar"} }

@tdeekens
Copy link
Contributor Author

Resolved the issue. It was the unescaped port. So one needs a handler as in http://localhost\\:1234/*. Then msw escapes the protocol : and alters the * so that the matcher becomes http\\://localhost\\:1234/(*)..

All of which is quite hard to debug and know. I only learned by reading code (which is always good) and debugging msw code during test runs. Otherwise it's very hard to know why things don't match. Especially, as msw patches the matcher passed to path-to-regex in parts. So reading path-to-regex's documentation easily leads to passing escaped matchers which will then be double escaped.

Hope this helps somebody after me.

@kettanaito
Copy link
Member

Hey, @tdeekens. Thanks for reporting this.

I still treat this as an issue on our side. The library should escape the port the same as it automatically escapes the protocol. I believe we can achieve that by changing the logic of this coercing function:

export function coercePath(path: string): string {
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: string | undefined, wildcard: string) => {
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')
)
}

You can see that it escapes the protocol already. We can add another regular expression to handle the port number. Would you be interested in opening a pull request with this fix?

@kettanaito kettanaito reopened this Dec 12, 2021
@kettanaito kettanaito changed the title Adoption of path-to-regexp breaks matching absolute urls Cannot match absolute URLs with port number Dec 12, 2021
@kettanaito kettanaito added the good first issue Good for newcomers label Dec 12, 2021
@tdeekens
Copy link
Contributor Author

Hi @kettanaito,

sure. I however don't really feel good about fiddling with URLs using Regular Expressions. There are so many edge cases we might not think of. Like URLs which are IPs or contain - and + signs (which I recall can happen). In essence, I struggle a bit with trying to coerce URLs at all mainly cause edge cases and side-effects are silent to users. Obviously my personal opinion but wanted to share it regardless.

A first attempt at a minimal RegExp which aren't my strong suit 😄 would be ((?::))(?:[0-9]+). Do you have something else in mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants