Auto merge of #2065 - jtgeibel:prod/make-2-errors-user-facing, r=smarnach

Convert pagination ISE into a Bad Request response

Errors in parsing the pagination query string parameters can be safely returned to the client and do not need to propagate up to the logging middleware.

The following have been observed in production logs:

* error="cannot parse integer from empty string" for `/api/v1/crates?page=&per_page=50&sort=downloads` (empty `page=`)
* error="invalid digit found in string" for `/api/v1/crates?page=1&per_page=100%22%EF%BC%8Cexception"` (very invalid `per_page`)

Additionally, 2 existing pagination errors are converted to use `bad_request()` in place of `cargo_err()`.  It is not necessary to send a status=200 response to support old versions of cargo for pagination errors.

Fixes:  #1928

r? @smarnach

Author: bors

src/controllers/helpers/pagination.rs

@@ -1,5 +1,5 @@
 use crate::models::helpers::with_count::*;
-use crate::util::errors::{cargo_err, AppResult};
+use crate::util::errors::{bad_request, AppResult};
 use diesel::pg::Pg;
 use diesel::prelude::*;
 use diesel::query_builder::*;
@@ -16,9 +16,9 @@ pub(crate) enum Page {
 impl Page {
     fn new(params: &IndexMap<String, String>) -> AppResult<Self> {
         if let Some(s) = params.get("page") {
-            let numeric_page = s.parse()?;
+            let numeric_page = s.parse().map_err(|e| bad_request(&e))?;
             if numeric_page < 1 {
-                return Err(cargo_err(&format_args!(
+                return Err(bad_request(&format_args!(
                     "page indexing starts from 1, page {} is invalid",
                     numeric_page,
                 )));
@@ -44,11 +44,11 @@ impl PaginationOptions {
 
         let per_page = params
             .get("per_page")
-            .map(|s| s.parse())
+            .map(|s| s.parse().map_err(|e| bad_request(&e)))
             .unwrap_or(Ok(DEFAULT_PER_PAGE))?;
 
         if per_page > MAX_PER_PAGE {
-            return Err(cargo_err(&format_args!(
+            return Err(bad_request(&format_args!(
                 "cannot request more than {} items",
                 MAX_PER_PAGE,
             )));
@@ -182,3 +182,30 @@ where
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{Page, PaginationOptions};
+    use indexmap::IndexMap;
+
+    #[test]
+    fn page_must_be_a_number() {
+        let mut params = IndexMap::new();
+        params.insert(String::from("page"), String::from("not a number"));
+        let page_error = Page::new(&params).unwrap_err().response().unwrap();
+
+        assert_eq!(page_error.status, (400, "Bad Request"));
+    }
+
+    #[test]
+    fn per_page_must_be_a_number() {
+        let mut params = IndexMap::new();
+        params.insert(String::from("per_page"), String::from("not a number"));
+        let per_page_error = PaginationOptions::new(&params)
+            .unwrap_err()
+            .response()
+            .unwrap();
+
+        assert_eq!(per_page_error.status, (400, "Bad Request"));
+    }
+}

src/tests/krate.rs

@@ -2364,3 +2364,31 @@ fn pagination_links_included_if_applicable() {
     assert_eq!(None, page4.meta.next_page);
     assert_eq!(Some("?page=2&per_page=1".to_string()), page3.meta.prev_page);
 }
+
+#[test]
+fn pagination_parameters_only_accept_integers() {
+    let (app, anon, user) = TestApp::init().with_user();
+    let user = user.as_model();
+
+    app.db(|conn| {
+        CrateBuilder::new("pagination_links_1", user.id).expect_build(conn);
+        CrateBuilder::new("pagination_links_2", user.id).expect_build(conn);
+        CrateBuilder::new("pagination_links_3", user.id).expect_build(conn);
+    });
+
+    let invalid_per_page_json = anon
+        .get_with_query::<()>("/api/v1/crates", "page=1&per_page=100%22%EF%BC%8Cexception")
+        .bad_with_status(400);
+    assert_eq!(
+        invalid_per_page_json.errors[0].detail,
+        "invalid digit found in string"
+    );
+
+    let invalid_page_json = anon
+        .get_with_query::<()>("/api/v1/crates", "page=100%22%EF%BC%8Cexception&per_page=1")
+        .bad_with_status(400);
+    assert_eq!(
+        invalid_page_json.errors[0].detail,
+        "invalid digit found in string"
+    );
+}

src/tests/user.rs

@@ -296,7 +296,7 @@ fn following() {
     assert_eq!(r.meta.more, false);
 
     user.get_with_query::<()>("/api/v1/me/updates", "page=0")
-        .bad_with_status(200); // TODO: Should be 500
+        .bad_with_status(400);
 }
 
 #[test]