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

fix: Pass url string to http.request #1268

Merged

Conversation

serverwentdown
Copy link
Contributor

@serverwentdown serverwentdown commented Sep 2, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New feature
  • Other, please explain:

What changes did you make? (provide an overview)

The previous url.parse did remove brackets from hostname the new URL don't do it.

To fix this i Replaced https.request(options[, callback]) with https.request(url[, options][, callback]) and letting NodeJS take care of parsing the the url

  • This strips square brackets ([]) off hostnames that have them
  • implicit allow basic auth http://username:password@hostname that wasn't working before

Which issue (if any) does this pull request address?

Closes #1267

TODO:

  • Throw if url includes credentials in the constructor
    TypeError("Request cannot be constructed from a URL that includes credentials")
    • Add test for this

@serverwentdown
Copy link
Contributor Author

@serverwentdown serverwentdown commented Sep 2, 2021

I feel like there should be unit tests for this, but I don't know which file it should belong in.

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 2, 2021

[] feels like a very specific usecase

what if we just took the hostname straight from url.parse instead?

hostname = url.parse(parsedURL.toString()).hostname

I'm still wondering about just doing: https.request(url[, options][, callback]) instead of spreading out every single option
and remove the https.request(options[, callback]) solution that we use today


not fully rejecting this PR. but i like to have options before finalizing something and explore options

@serverwentdown
Copy link
Contributor Author

@serverwentdown serverwentdown commented Sep 3, 2021

This is exactly the implementation inside the Node.js standard library, so I'd say it is acceptable.

But I agree, using https.request(url, options, callback) is better.

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 3, 2021

This is exactly the implementation inside the Node.js standard library, so I'd say it is acceptable.

Oh, interesting find.

@serverwentdown
Copy link
Contributor Author

@serverwentdown serverwentdown commented Sep 3, 2021

Implementation of your proposal: main...serverwentdown:fix-ipv6-literal-parsing-2, though it needs some refactoring (parsedURL and options aren't named well)

I also added tests for IPv6 on both proposals.

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 5, 2021

Implementation of your proposal: main...serverwentdown:fix-ipv6-literal-parsing-2, though it needs some refactoring (parsedURL and options aren't named well)

Do you want to apply that changes to this PR?
( Also what do @Richienb, @LinusU, @gr2m (and anybody else) think? )

I also added tests for IPv6 on both proposals.

Great! Needed that!

Copy link
Collaborator

@gr2m gr2m left a comment

Change looks good to me! I'd approve the PR but not sure if there is anything else you want to add

src/request.js Outdated Show resolved Hide resolved
src/request.js Show resolved Hide resolved
LinusU
LinusU approved these changes Sep 6, 2021
Copy link
Member

@LinusU LinusU left a comment

I think that this seems like a great approach 👍

@gr2m
Copy link
Collaborator

@gr2m gr2m commented Sep 6, 2021

can you address the lint errors?

image

Running npx xo --fix should address them all

@jimmywarting jimmywarting changed the title fix: Strip brackets from IPv6 literals when parsing fix: Pass url string to http.request Sep 6, 2021
@serverwentdown
Copy link
Contributor Author

@serverwentdown serverwentdown commented Sep 7, 2021

Turns out the fetch() spec lists basic auth, but is not enabled (isAuthenticationFetch in spec) by any browsers for security reasons. But, allowing basic auth should be useful for majority of applications.

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 7, 2021

Started to get 2nd toughs on supporting basic auth...

Starting looking more into the isAuthenticationFetch in the spec and also trying it out in chromes devtool
image

Guess it have something to do with caching... it's bad to use cache storage and save the usr:psw in full plain text (url/key)

  • the usr/psw should be converted to a basic auth request header to avoid saving the headers to the cache storage.
  • It's also b/c the usr:psw should not be exposed in the response.url (for security reasons)

We could argue that we don't have the same level of restrictions and we don't have a cache storage.

I guess the isAuthenticationFetch flag is mostly set to true when working with mitm service workers and you get prompted with that builtin old basic auth login dialog in the browsers.

We also have a hole option with credentials that we haven't looked into much at all in node-fetch
new Request(url, { credentials: 'include' })

credentials: The request credentials you want to use for the request: omit, same-origin, or include. The default is same-origin.

we don't have a concept of origin either.

Either way i still think this PR looks good question is wether or not we shoud:

  • ...also throw an error when constructing the Request with a url that includes credentials (think this is for the best)
  • or if we should auto "converted to an Authorization value" as mention in the spec also and strip away the usr/psw from the url
  • or just simply allow it

we could also start having a look at what the credential option is

most ppl are already using auth headers cuz we have never supported http://username:password@hostname before - maybe best if it stays like that?

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 7, 2021

fyi. i tried deno's built in fetch api also... fetch('https://foo:bar@vecka.nu') worked out fine

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 8, 2021

@node-fetch/reviewers Should we throw an error for the time being on basic-auth-url?
it's easier to lift the restriction rather than removing/deprecating/discourage it.

Our ambition is that the code you write should be working everywhere the same way in Deno, Node & Browsers.
And if non of the browsers allow it then everyone should be using the solution that works cross all platforms. Meaning: using btoa and appending basic auth headers & throwing an error like browser dose.

@gr2m
Copy link
Collaborator

@gr2m gr2m commented Sep 8, 2021

Our ambition is that the code you write should be working everywhere the same way in Deno, Node & Browsers.
And if non of the browsers allow it then everyone should be using the solution that works cross all platforms

I concur

docs/v3-LIMITS.md Outdated Show resolved Hide resolved
@serverwentdown
Copy link
Contributor Author

@serverwentdown serverwentdown commented Sep 9, 2021

I also realised that the specification somewhat contradicts: when creating a Request with a URL that includes credentials, it should throw a TypeError.

@jimmywarting jimmywarting merged commit 8b54ab4 into node-fetch:main Sep 9, 2021
8 checks passed
@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 9, 2021

Thanks for your contribution to node fetch
It will be released in the next version.

I don't have the possibility to make a release yet until Sunday
I'm on a vacation now with no computer. Unless someone else dose it...

Would be nice to have that auto ci deployment now...

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 9, 2021

@serverwentdown If you would like to help out more with one other issue then i got something easy for you. (You don't have to if you don't want)

If a Request is constructed without a body then delete content-length request header #1249

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants