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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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