Skip to content
New issue

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

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

Already on GitHub? # to your account

Got doesn't throw on leading slashes #1057

Closed
szmarczak opened this issue Feb 8, 2020 · 4 comments · Fixed by #1051
Closed

Got doesn't throw on leading slashes #1057

szmarczak opened this issue Feb 8, 2020 · 4 comments · Fixed by #1051
Labels
bug Something does not work as it should ✭ help wanted ✭ regression Something does not work anymore

Comments

@szmarczak
Copy link
Collaborator

As per the readme:

Note: Leading slashes in input are disallowed when using this option to enforce consistency and avoid confusion. For example, when the prefix URL is https://example.com/foo and the input is /bar, there's ambiguity whether the resulting URL would become https://example.com/foo/bar or https://example.com/bar. The latter is used by browsers.

But it does not throw:

if (is.string(options.url)) {
options.url = (options.prefixUrl as string) + options.url;
options.url = options.url.replace(/^unix:/, 'http://$&');
if (options.searchParams || options.search) {
options.url = options.url.split('?')[0];
}
// @ts-ignore URL is not URL
options.url = optionsToUrl({
origin: options.url,
...options
});
} else if (!is.urlInstance(options.url)) {
// @ts-ignore URL is not URL
options.url = optionsToUrl({origin: options.prefixUrl as string, ...options});
}

@szmarczak szmarczak added bug Something does not work as it should ✭ help wanted ✭ regression Something does not work anymore labels Feb 8, 2020
@szmarczak szmarczak changed the title Got doesn't deduplicate leading slashes Got doesn't throw on leading slashes Feb 8, 2020
@szmarczak szmarczak mentioned this issue Feb 8, 2020
18 tasks
@azz
Copy link

azz commented Feb 22, 2020

I don't think it should throw if prefixUrl is just an origin:

const myApi = got.extend({ prefixUrl: 'https://api.example.com' });

myApi.get('/things'); // this should be fine as it is not ambiguous.

@szmarczak
Copy link
Collaborator Author

@azz The readme says that the ending / will be added automatically if not present, thus it should throw if the input starts with /.

@azz
Copy link

azz commented Feb 22, 2020

That's what's currently documented but I'm challenging whether that behaviour is appropriate, especially in the case where it is non-ambiguous.

@szmarczak
Copy link
Collaborator Author

The documentation provides a non-ambiguous behavior, which has been intensively discussed. please see #829 and #943 for more information.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something does not work as it should ✭ help wanted ✭ regression Something does not work anymore
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants