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

Change default request mode to same-origin #602

Closed
wants to merge 3 commits into from

Conversation

sholladay
Copy link
Collaborator

@sholladay sholladay commented Jun 26, 2024

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
Copy link
Collaborator Author

Thoughts on this @sindresorhus @szmarczak?

@sindresorhus
Copy link
Owner

This is a breaking change, correct?

@sholladay
Copy link
Collaborator Author

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' })

readme.md Outdated
@@ -136,6 136,7 @@ import ky from 'https://esm.sh/ky';
The `input` and `options` are the same as [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch), with some exceptions:

- The `credentials` option is `same-origin` by default, which is the default in the spec too, but not all browsers have caught up yet.
- The `mode` option is `same-origin` by default, as the `no-cors` default used by `fetch` is confusing and unsafe. If you need to make cross-origin requests, set the `mode` to `cors`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be able to defend this choice. A user will wonder why it's confusing and unsafe and we need to either answer that or link to a site that does answer it.

Copy link
Collaborator Author

@sholladay sholladay Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed that wording in favor of something that is hopefully more helpful, with specific advice about what mode to use, when, and why, with multiple links.

What do you think?

@sindresorhus
Copy link
Owner

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
Copy link
Owner

But same-origin is the most secure default.

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

@sholladay
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

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

@sholladay sholladay closed this Aug 10, 2024
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.

Headers are not sent when mode is no-cors
3 participants