From 44aeb47b56037890f49c05464a973b8447f7c0ff Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 25 Mar 2019 15:19:07 -0600 Subject: [PATCH] Require a User-Agent header, take 2 We deployed this back in October, and quickly realized that old versions of Cargo (Rust 1.15 and earlier) didn't set a User-Agent header. Notably, the version of Cargo used for bootstrapping the compiler back then was this old, so we accidentally broke the build for rustc. We still see build traffic for these old versions, and we likely will never be willing to break `cargo build` for old versions of Cargo. However, as we discussed back in early November, we're fine with doing this for all other endpoints. This change will break versions of Cargo prior to Rust 1.16 for all operations that talk to crates.io, except for `cargo build`. We no longer receive any traffic for publish, yank, unyank, or owners from these old versions. (`cargo search` hits an endpoint that is also hit by bots, so I can't say for sure if it's coming from cargo or random crawlers, which is kinda the point of this requirement in the first place) --- src/middleware.rs | 6 ++-- src/middleware/require_user_agent.rs | 3 +- src/tests/server.rs | 44 +++++++++++++++++----------- src/tests/util.rs | 30 ++++++++++++------- 4 files changed, 50 insertions(+), 33 deletions(-) diff --git a/src/middleware.rs b/src/middleware.rs index e34cb38dc0f..38936d8fb69 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -93,10 +93,8 @@ pub fn build_middleware(app: Arc, endpoints: R404) -> MiddlewareBuilder { let ips = ip_list.split(',').map(String::from).collect(); m.around(block_ips::BlockIps::new(ips)); } - // Note: Temporarily disabled because cargo-vendor doesn't include a - // User-Agent header and Rust's CI broke. If this is still commented out - // by Nov 7, 2018 ping the crates.io team. - // m.around(require_user_agent::RequireUserAgent::default()); + + m.around(require_user_agent::RequireUserAgent::default()); if env != Env::Test { m.around(log_request::LogRequests::default()); diff --git a/src/middleware/require_user_agent.rs b/src/middleware/require_user_agent.rs index 0228f748182..8a24067e28c 100644 --- a/src/middleware/require_user_agent.rs +++ b/src/middleware/require_user_agent.rs @@ -22,7 +22,8 @@ impl AroundMiddleware for RequireUserAgent { impl Handler for RequireUserAgent { fn call(&self, req: &mut dyn Request) -> Result> { let has_user_agent = request_header(req, "User-Agent") != ""; - if !has_user_agent { + let is_download = req.path().ends_with("download"); + if !has_user_agent && !is_download { let body = format!( include_str!("no_user_agent_message.txt"), request_header(req, "X-Request-Id"), diff --git a/src/tests/server.rs b/src/tests/server.rs index bf5cd3edf4e..6ffd1e5de58 100644 --- a/src/tests/server.rs +++ b/src/tests/server.rs @@ -1,18 +1,28 @@ -// Note: Temporarily disabled because cargo-vendor doesn't include a -// User-Agent header and Rust's CI broke. If this is still commented out -// by Nov 7, 2018 ping the crates.io team. +use conduit::Method; -// use conduit::{Handler, Method}; -// -// use {app, req}; -// -// -// #[test] -// fn user_agent_is_required() { -// let (_b, _app, middle) = app(); -// -// let mut req = req(Method::Get, "/api/v1/crates"); -// req.header("User-Agent", ""); -// let resp = t!(middle.call(&mut req)); -// assert_eq!(resp.status.0, 403); -// } +use crate::builders::*; +use crate::util::*; + +#[test] +fn user_agent_is_required() { + let (_app, anon) = TestApp::init().empty(); + + let mut req = anon.request_builder(Method::Get, "/api/v1/crates"); + req.header("User-Agent", ""); + let resp = anon.run::<()>(req); + resp.assert_status(403); +} + +#[test] +fn user_agent_is_not_required_for_download() { + let (app, anon, user) = TestApp::init().with_user(); + + app.db(|conn| { + CrateBuilder::new("dl_no_ua", user.as_model().id).expect_build(conn); + }); + + let mut req = anon.request_builder(Method::Get, "/api/v1/crates/dl_no_ua/0.99.0/download"); + req.header("User-Agent", ""); + let resp = anon.run::<()>(req); + resp.assert_status(302); +} diff --git a/src/tests/util.rs b/src/tests/util.rs index 623b8474e77..4f1f32ee7ee 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -145,13 +145,21 @@ pub trait RequestHelper { fn request_builder(&self, method: Method, path: &str) -> MockRequest; fn app(&self) -> &TestApp; + /// Run a request + fn run(&self, mut request: MockRequest) -> Response + where + T: serde::de::DeserializeOwned, + { + Response::new(self.app().0.middle.call(&mut request)) + } + /// Issue a GET request fn get(&self, path: &str) -> Response where for<'de> T: serde::Deserialize<'de>, { - let mut request = self.request_builder(Method::Get, path); - Response::new(self.app().0.middle.call(&mut request)) + let request = self.request_builder(Method::Get, path); + self.run(request) } /// Issue a GET request that includes query parameters @@ -161,7 +169,7 @@ pub trait RequestHelper { { let mut request = self.request_builder(Method::Get, path); request.with_query(query); - Response::new(self.app().0.middle.call(&mut request)) + self.run(request) } /// Issue a PUT request @@ -169,9 +177,9 @@ pub trait RequestHelper { where for<'de> T: serde::Deserialize<'de>, { - let mut builder = self.request_builder(Method::Put, path); - let request = builder.with_body(body); - Response::new(self.app().0.middle.call(request)) + let mut request = self.request_builder(Method::Put, path); + request.with_body(body); + self.run(request) } /// Issue a DELETE request @@ -179,8 +187,8 @@ pub trait RequestHelper { where for<'de> T: serde::Deserialize<'de>, { - let mut request = self.request_builder(Method::Delete, path); - Response::new(self.app().0.middle.call(&mut request)) + let request = self.request_builder(Method::Delete, path); + self.run(request) } /// Issue a DELETE request with a body... yes we do it, for crate owner removal @@ -188,9 +196,9 @@ pub trait RequestHelper { where for<'de> T: serde::Deserialize<'de>, { - let mut builder = self.request_builder(Method::Delete, path); - let request = builder.with_body(body); - Response::new(self.app().0.middle.call(request)) + let mut request = self.request_builder(Method::Delete, path); + request.with_body(body); + self.run(request) } /// Search for crates matching a query string