Add Access-Control-Allow-Origin: * header to allow CORS #2480
Conversation
|
I'm a little worried about allowing this generically for all requests since the API is under a subpath of crates.io, instead of a subdomain, and we use a cookie for auth. Did you check for the common scenarios of CORS attacks? |
|
My understanding is that as long as we do not send a As an extra precaution, I think we should add |
|
hmm, yeah, I guess it might be fine, as long as we only allow read access to the API 🤔 |
Turbo87
added
A-backend ⚙️
C-enhancement ✨
As I understand it, the guarantees are stronger than that. Adding this CORS header will only allow unauthenticated read-only access to the API. I've deployed this branch to my staging area, and the following script works as I expect. <!doctype html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Crates.io CORS Test</title>
</head>
<body><script>
async function run() {
let url = "https://jtgeibel-staging-crates-io.herokuapp.com/api/v1/summary";
let response = await fetch(url);
let body = await response.text();
alert(body);
response = await fetch(url, {credentials: "include"});
body = await response.text();
alert(body);
}
run()
</script></body>
</html>Opening this page (locally via If we wanted to allow authenticated access, the response would need to echo the request If I try to do something shady, like create a new token with async function run() {
let url = "https://jtgeibel-staging-crates-io.herokuapp.com/api/v1/me/tokens";
let response = await fetch(url, {credentials: "include", method: "PUT", body: '{api_token: {name: "bad"}}'});
let body = await response.text();
alert(body);
}then the browser first sends an Even if we returned a success status code and nginx was adding In any case, I would definitely like to give the team a bit more time to review this change because I have no prior experience with this web functionality. I've added it to the agenda for tomorrow. |
|
☔ The latest upstream changes (presumably #2519) made this pull request unmergeable. Please resolve the merge conflicts. |
|
We've had @bors r+ |
|
📌 Commit 6a49937 has been approved by |
|
☀️ Test successful - checks-travis |
In rust-lang#2480, Access-Control-Allow-Origin: * was added to all requests. The goal, as I understand it, was to allow other web sites to request data from the public API - for instance the download counts for crates. This does somewhat increase the risk of unwanted cross-site requests. There are two cases to be concerned about, per https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS: "Simple" requests: GET, HEAD, or POST, and "preflighted" requests (for our purposes, PUT). Simple requests execute without an OPTIONS preflight, and the browser decides whether the caller can read the response based on the Access-Control-Allow-Origin (ACAO) header in the response. For GET, the risk is that a third-party website could read private data - for instance the email address in /api/v1/me. For POST, the risk is that an important state mutation (like uploading a new crate) could happen even if the calling web page isn't allowed via ACAO to read the response. The above risk for GET is mitigated two ways: - Users who logged in recently get the SameSite setting on their session cookie, so their cookie is not sent on cross-site requests and they cannot be authenticated. - The CORS specification indicates that, on a request carrying cookies, the calling origin cannot access the response unless the ACAO header in the response explicitly names that origin; a wildcard is not sufficient. For POST, the risk is present regardless of whether ACAO: * is set. Since POSTs aren't preflighted, they can mutate state even if ACAO doesn't permit accessing the response. The risk for POST is mitigated by: - SameSite cookie setting - crates.io doesn't currently implement any POST endpoints, only PUT. But that's not a documented security feature and could change in the future. For preflighted PUT requests, the risk is that a third-party website could send a request that mutates state, like uploading a new crate. This is mitigated by: - SameSite cookie setting - Preflight request will fail because crates.io currently does not support OPTIONS, and so will return 404. Nginx will additionally not set ACAO on 404s. However, the fact that crates.io doesn't support OPTIONS isn't currently documented as a security feature. - If OPTIONS support was added at some point in the future, a browser that sent a credentialed request should stop after the preflight response if the ACAO header in the response did not explicitly include the calling origin (rather than a wildcard). However, the above is a somewhat complex analysis for an important piece of crates.io's security. I'm proposing to add another layer of protection: When authenticating a request, authentication is refused if the Origin header is set to anything other than the origin of crates.io itself. I started to write a test for this, but could not figure out how to instantiate something that implements RequestExt. I'd appreciate any tips on how best to test this.
Restrict authentication to same-origin requests. In #2480, Access-Control-Allow-Origin: * was added to all requests. The goal, as I understand it, was to allow other web sites to request data from the public API - for instance the download counts for crates. This does somewhat increase the risk of unwanted cross-site requests. There are two cases to be concerned about, per https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS: "Simple" requests: GET, HEAD, or POST, and "preflighted" requests (for our purposes, PUT). Simple requests execute without an OPTIONS preflight, and the browser decides whether the caller can read the response based on the Access-Control-Allow-Origin (ACAO) header in the response. For GET, the risk is that a third-party website could read private data - for instance the email address in /api/v1/me. For POST, the risk is that an important state mutation (like uploading a new crate) could happen even if the calling web page isn't allowed via ACAO to read the response. The above risk for GET is mitigated two ways: - Users who logged in recently get the SameSite setting on their session cookie, so their cookie is not sent on cross-site requests and they cannot be authenticated. - The CORS specification indicates that, on a request carrying cookies, the calling origin cannot access the response unless the ACAO header in the response explicitly names that origin; a wildcard is not sufficient. For POST, the risk is present regardless of whether ACAO: * is set. Since POSTs aren't preflighted, they can mutate state even if ACAO doesn't permit accessing the response. The risk for POST is mitigated by: - SameSite cookie setting - crates.io doesn't currently implement any POST endpoints, only PUT. But that's not a documented security feature and could change in the future. For preflighted PUT requests, the risk is that a third-party website could send a request that mutates state, like uploading a new crate. This is mitigated by: - SameSite cookie setting - Preflight request will fail because crates.io currently does not support OPTIONS, and so will return 404. Nginx will additionally not set ACAO on 404s. However, the fact that crates.io doesn't support OPTIONS isn't currently documented as a security feature. - If OPTIONS support was added at some point in the future, a browser that sent a credentialed request should stop after the preflight response if the ACAO header in the response did not explicitly include the calling origin (rather than a wildcard). However, the above is a somewhat complex analysis for an important piece of crates.io's security. I'm proposing to add another layer of protection: When authenticating a request, authentication is refused if the Origin header is set to anything other than the origin of crates.io itself. I started to write a test for this, but could not figure out how to instantiate something that implements RequestExt. I'd appreciate any tips on how best to test this.
Fixes #2191