Feature Request: include credentials

[xinbenlv]

No description provided.

[ricmoo]

This is a potential security concern, since including the credentials can leak them to unauthorized third parties.

The current settings were all selected as they are they default for fetch.

[xinbenlv]

Wow, that was a fast response, thank you @ricmoo . I further discuss this idea in the #3097

Yes, it's only a proof of concept. and I agree with you there are security concerns. We would like to check with you if you are open to allow this to be configurable at least. Given that even basic authentication is allowed allowInsecureAuthentication

[ricmoo]

It could be made a configurable option, but this would be a non-backwards compatible change and would be a bit more work than is likely to be prioritized in the near future for v5.

This change, while simple in the browser case, requires some additional code for the node case, where it would need to be implemented. I want to make sure the fetch api works identically in both node and browser, so an option like this needs to be added as the node http library isn’t a matter of a simple property.

In v6, the entire fetch library will be able to be swapped out in the same way the crypto library can, so there will be fewer options directly within the library, with the expectation that adding the new abilities is easier to do by replacing the fetch.

[xinbenlv]

Can I describe my understanding of what you said in my own words, please check if it's correct:

1. Due to this is non-backwards compatibile change, if we add this feature, it will have to fall into the V5 of ethers.js. But we aren't likely to prioritize this particular feature in V5.

2. Because of the different behaviors between fetch API vs node-http API, if we want to introduce the includeCredentials, we need to also implement the node-http side to ensure the same option is honored in the same way for both browser and server-side.

3. Looking ahead, V6 intends to be http API agnostic, i.e. be agnostic of whether it's using fetch, node-http or something else (e.g. axios). In that case, users will call the http library of their choice directly, hence the option to define whether to include credentials will not be defined by ethers.js v6 but rather the http library of users' choice.

This it a good understanding?

[xinbenlv]

2. Because of the different behaviors between fetch API vs node-http API, if we want to introduce the includeCredentials, we need to also implement the node-http side to ensure the same option is honored in the same way for both browser and server-side.

QQ: if my understanding of this sentence is correct, does it currently honor skipFetchSetup in the server-side?
Since the credentials being included is more of a browser-only concept, how we call it includeCredentialsInFetch to make it more obvious a browser-only option?

[xinbenlv]

friendly ping to continue discussion~

[ricmoo]

The v6 branch won't need this, since you can just swap out the fetch operation and make any adjustments manually. If it is added to v5, it needs to occur in a minor version change. I am planning one though.

What I may opt for is a fetchOptions which would only affect browsers, then you could specify fetchOptions: { credentials: "include" }. And like skipFetchSetup, would only added browsers (i.e. fetch); which is a no-op in node.

I'll mark this for a minor bump to look more into it as I prepare that.

[xinbenlv]

That would work, thank you @ricmoo !

[xinbenlv]

With c309df8 merged this is PR is considered done

[ricmoo]

The fetchOptions has been included in v5.7.0. Try it out and let me know if there are any problems. :)

Thanks! :)

[xinbenlv]

That is great news!