Skip to content

Require a User-Agent header, take 2 #1696

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 2, 2019
Merged

Require a User-Agent header, take 2 #1696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/middleware.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,8 @@ pub fn build_middleware(app: Arc<App>, 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());
Expand Down
3 changes: 2 additions & 1 deletion src/middleware/require_user_agent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ impl AroundMiddleware for RequireUserAgent {
impl Handler for RequireUserAgent {
fn call(&self, req: &mut dyn Request) -> Result<Response, Box<dyn Error + Send>> {
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"),
Expand Down
44 changes: 27 additions & 17 deletions src/tests/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this testing when a User-Agent header exists, but is the empty string? If so, do we need a test where the header doesn't exist as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, conduit-test doesn't give us a way to unset a header. We're setting the user agent to a non-empty string when the requests are first constructed so we don't have to set it for every test besides these two. We could refactor things to apply the default user agent later, so we have a place to construct a request without it here, but I've left it this way for two reasons:

  • Explicitly setting req.header("User-Agent", "") makes it much more clear what this is actually testing
  • It'd be very odd if we were treating a header with no value differently than a header not being sent at all (the only case I'm aware of where there's a semantic difference is when sending an HTTP/1.1 -> HTTP/2 upgrade request, which specifies that an HTTP2-Settings header must be present)

With that in mind, do you still think it's worth the additional test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, I don't. 👍 LGTM.

If testing without the header was easy to do, I'd like to see it. But I'm with you that it would be very odd if the framework was treating a non-existent header differently than an empty header.

I just brought it up because I've been bitten in different places (not HTTP headers) where non-existent != empty string.

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);
}
30 changes: 19 additions & 11 deletions src/tests/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>(&self, mut request: MockRequest) -> Response<T>
where
T: serde::de::DeserializeOwned,
{
Response::new(self.app().0.middle.call(&mut request))
}

/// Issue a GET request
fn get<T>(&self, path: &str) -> Response<T>
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
Expand All @@ -161,36 +169,36 @@ 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
fn put<T>(&self, path: &str, body: &[u8]) -> Response<T>
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
fn delete<T>(&self, path: &str) -> Response<T>
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
fn delete_with_body<T>(&self, path: &str, body: &[u8]) -> Response<T>
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
Expand Down