balazsbotond / urlcat

A URL builder library for JavaScript.
https://urlcat.org
MIT License
1.82k stars 57 forks source link

Passing an invalid string as a URL to `pathTemplate` causes an exception. #7

Closed kako-jun closed 4 years ago

kako-jun commented 4 years ago

To reproduce the exception:

in:

urlcat("http://example.com", "//");

out:

If a parameter appears twice in the path, it is substituted twice:
     TypeError [ERR_INVALID_URL]: Invalid URL: //

Users of urlcat may not know that new URL is called internally. So users don't expect exceptions.

The problem is when pathTemplate is variable. Users don't know what string cause an exception until they give it to urlcat. :cold_sweat:

It forces users to add try. Or users need to read the urlcat code to prevent this.

How is adding try internally :question:

balazsbotond commented 4 years ago

Nice catch, thank you!

This is indeed very unexpected behavior. I'll release a fix later today.

balazsbotond commented 4 years ago

This is due to how the URL constructor handles relative URLs:

new URL('/foo', 'http://example.com').toString() // ==> "http://example.com/foo"
new URL('', 'http://example.com').toString() // ==> "http://example.com/"
new URL('/', 'http://example.com').toString() // ==> "http://example.com/"
new URL('//foo', 'http://example.com').toString() // ==> "http://foo/"
new URL('//', 'http://example.com').toString() // ==> ERROR

To be honest I don't think this is useful at all - users would expect urlcat to do simple concatenation. This error goes against the principle of least surprise. We already have a well-tested join function that does the same so I'm going to replace the URL constructor with a simple join call.

balazsbotond commented 4 years ago

@kako-jun thanks again. I've fixed the bug and published a new patch release v2.0.2.

balazsbotond commented 3 years ago

@all-contributors please add @kako-jun for bug

allcontributors[bot] commented 3 years ago

@balazsbotond

I've put up a pull request to add @kako-jun! :tada: