sindresorhus / ky

🌳 Tiny & elegant JavaScript HTTP client based on the Fetch API
MIT License
13.61k stars 364 forks source link

Change default request mode to same-origin #602

Closed sholladay closed 1 month ago

sholladay commented 3 months ago

Closes #406

Note: This is a breaking change.

This PR sets same-origin as the default for the request mode option. (And tweaks how the credentials default is derived, which will also be same-origin, as it always has been.)

Previously, we weren't setting a default request mode, which meant that the mode was either cors or no-cors depending on the environment.

The no-cors mode is specifically discouraged by the fetch spec, but is often the default on the web for legacy reasons.

https://fetch.spec.whatwg.org/#concept-request-mode

Even though the default request mode is "no-cors", standards are highly discouraged from using it for new features. It is rather unsafe.

I would be okay with cors as the default mode if this proves to be unpopular. But same-origin is the most secure default.

sholladay commented 3 months ago

Thoughts on this @sindresorhus @szmarczak?

sindresorhus commented 3 months ago

This is a breaking change, correct?

sholladay commented 3 months ago

Yes, definitely a breaking change.

People who are using Ky for same-origin requests (such as calling their own API) won't notice anything. But those using Ky for cross-origin requests (such as calling Google Maps or another 3rd party API) will have to do something like:

ky(url, { mode : 'cors' })
sindresorhus commented 3 months ago

I would be okay with cors as the default mode if this proves to be unpopular.

Setting it to cors would fail the network request if the server cannot handle cors though, which may not be a good default.

sindresorhus commented 3 months ago

But same-origin is the most secure default.

How does Node.js handle same-origin. I would assume it just ignores it?

sholladay commented 3 months ago

From what I can tell (by experimentation and poking around the source code), Node seems to just ignore it.

However, interestingly, it does validate the value. Passing mode : 'foo' results in an error:

TypeError: Dictionary: foo is not an accepted type. Expected one of navigate, same-origin, no-cors, cors.

Deno also seems to just ignore it and, in fact, not even validate the value. Which is strange, because Deno actually has a mechanism to set the value of window.location, which would make it possible for fetch to run the cross origin check. I guess it's just not implemented.

DavidAnson commented 2 months ago

Copying https://mastodon.social/@DavidAnson/112799398210589472

Can one of you explain the below comment in the spec? I do not understand it and it seems backwards? Same origin seems like a fine default in browser, but as you point out has no meaning under Node. I feel you should have a VERY good, clear reason to do something different than fetch.

Even though the default request mode is "no-cors", standards are highly discouraged from using it for new features. It is rather unsafe.

sholladay commented 2 months ago

Can one of you explain the below comment in the spec?

No problem. The web has a long history of security vulnerabilities that mostly stem from behaviors that were designed before the rise of JavaScript and other technologies that allow arbitrary code to run. Early on, it was fine that you could load an image from any server regardless of whether you trust it or not, for example. But today, SVGs can actually do malicious things and someone could leave one in a comment here. GitHub does a good job of sanitizing such input, but not all websites do.

There's a particularly bad type of attack called CSRF where, for example, I get your browser to run some code that sends a request to your bank to send me money. Assuming you happen to be logged in to your bank, it will work, because from the bank's perspective, it's you who is requesting it.

The obvious solution to this problem is to treat the bank's origin as a security boundary and not allow requests from outside of it. But while that would probably be appropriate for all banks, it wouldn't be appropriate for every type of website, like those providing sports scores, advertising, maps, and useful third-party APIs.

Hence the creation of the Cross Origin Resource Sharing (CORS) protocol. It's basically a way for servers to opt-in to allowing cross-origin requests and announce what kind of requests are safe to make.

And for situations where the server doesn't opt-in to CORS? Well, it's messy. Modern browsers try to restrict what JavaScript is allowed to do with those requests. For example, by returning "opaque" responses, among other security measures. But they also didn't want to break lots of existing websites from before CORS was invented. It's a balancing act, on a fundamentally flawed, legacy security model.

When you use fetch with no-cors mode, that's the legacy security model. You can make requests to any website you want, but the browser won't let you read the response body, among other things.

When you use cors mode, you are using the modern security model. You get to read the response body and in some cases you get more descriptive errors. But unlike with no-cors. the request will fail if the server doesn't opt-in.

Now here's the weird part. Request and fetch actually have different default modes. Reauest uses cors. I only learned about this recently and it surprised me. Since Ky is using Request internally, we are unintentionally already using different defaults than fetch.

So where do we go from here? I think we should either codify cors as our default or switch to same-origin for its inherent security benefit. I don't think we should align with fetch in this case.

DavidAnson commented 2 months ago

This is very clear, thank you! I see why avoiding the legacy no-cors model makes sense and I think that modern web developers are already aware of CORS and will not be surprised by the behavior of cors mode. However, same-origin feels to me like a step too far. I appreciate that it is a more secure default, but I worry that it will be surprising for many and could lead to additional support burden for you and this project. What's more, because the current behavior is already cors, codifying that is not a breaking change for existing users. So, for whatever it's worth, my vote would be to use cors and to highlight same-origin in the documentation as an even more secure option.

sholladay commented 1 month ago

Changing the default mode to same-origin doesn't seem to have elicited much of a response. I'm going to close this.