Skip to content

Commit 8788546

Browse files
committed
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.
1 parent 7699bc0 commit 8788546

File tree

1 file changed

+13
-0
lines changed

1 file changed

+13
-0
lines changed

src/controllers/util.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,19 @@ impl AuthenticatedUser {
2828
impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
2929
/// Obtain `AuthenticatedUser` for the request or return an `Unauthorized` error
3030
fn authenticate(&self, conn: &PgConnection) -> AppResult<AuthenticatedUser> {
31+
let origin_headers = self.headers().get_all(header::ORIGIN);
32+
let expected_origin = match (self.scheme(), self.host()) {
33+
(conduit::Scheme::Http, conduit::Host::Name(host)) => format!("http://{}", host),
34+
(conduit::Scheme::Https, conduit::Host::Name(host)) => format!("https://{}", host),
35+
_ => "".to_string(),
36+
};
37+
if origin_headers
38+
.iter()
39+
.any(|h| h.as_bytes() != expected_origin.as_bytes())
40+
{
41+
return Err(internal("only same-origin requests can be authenticated"))
42+
.chain_error(|| Box::new(Unauthorized) as Box<dyn AppError>);
43+
}
3144
if let Some(id) = self.extensions().find::<TrustedUserId>() {
3245
// A trusted user_id was provided by a signed cookie (or a test `MockCookieUser`)
3346
Ok(AuthenticatedUser {

0 commit comments

Comments
 (0)