bug: Add additional verification checks.

src/adm.rs

@@ -62,35 +62,17 @@ impl From<&Settings> for AdmSettings {
 }
 
 /// Check that a given URL is valid according to it's corresponding filter
-fn check_url(
-    url: &str,
-    species: &'static str,
-    filter: &[String],
-    tags: &mut Tags,
-) -> HandlerResult<()> {
-    let parsed: Url = match url.parse() {
-        Ok(v) => v,
-        Err(e) => {
-            tags.add_tag("type", species);
-            tags.add_extra("parse_error", &e.to_string());
-            tags.add_extra("url", &url);
-            return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
-        }
-    };
-    let host = match parsed.host() {
+fn check_url(url: Url, species: &'static str, filter: &[String]) -> HandlerResult<bool> {
+    let host = match url.host() {
         Some(v) => v.to_string(),
         None => {
-            tags.add_tag("type", species);
-            tags.add_extra("url", &url);
-            return Err(HandlerErrorKind::MissingHost(species, parsed.to_string()).into());
+            return Err(HandlerErrorKind::MissingHost(species, url.to_string()).into());
         }
     };
     if !filter.contains(&host) {
-        tags.add_tag("type", species);
-        tags.add_extra("url", &url);
         return Err(HandlerErrorKind::UnexpectedHost(species, host).into());
     }
-    Ok(())
+    Ok(true)
 }
 
 impl AdmFilter {
@@ -108,22 +90,80 @@ impl AdmFilter {
         tile: &mut AdmTile,
         tags: &mut Tags,
     ) -> HandlerResult<()> {
-        check_url(
-            &tile.advertiser_url,
-            "Advertiser",
-            &filter.advertiser_hosts,
-            tags,
-        )
+        let url = &tile.advertiser_url;
+        let species = "Advertiser";
+        tags.add_tag("type", species);
+        tags.add_extra("url", &url);
+        let parsed: Url = match url.parse() {
+            Ok(v) => v,
+            Err(e) => {
+                tags.add_extra("parse_error", &e.to_string());
+                return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
+            }
+        };
+        check_url(parsed, species, &filter.advertiser_hosts)?;
+        Ok(())
     }
 
     /// Check the click URL
+    ///
+    /// Internally, this will use the hard-coded `req_keys` and `opt_keys` to specify
+    /// the required and optional query parameter keys that can appear in the click_url
     fn check_click(
         &self,
         filter: &AdmAdvertiserFilterSettings,
         tile: &mut AdmTile,
         tags: &mut Tags,
     ) -> HandlerResult<()> {
-        check_url(&tile.click_url, "Click", &filter.click_hosts, tags)
+        let url = &tile.click_url;
+        let species = "Click";
+        tags.add_tag("type", species);
+        tags.add_extra("url", &url);
+
+        // Check the required fields are present for the `click_url`
+        // pg 15 of 5.7.21 spec. (sort for efficiency)
+        // The list of sorted required query param keys for click_urls
+        let req_keys = vec!["aespFlag", "ci", "ctag", "key", "version"];
+        // the list of optionally appearing query param keys
+        let opt_keys = vec!["click-status"];
+
+        let mut all_keys = req_keys.clone();
+        all_keys.extend(opt_keys.clone());
+
+        let parsed: Url = match url.parse() {
+            Ok(v) => v,
+            Err(e) => {
+                tags.add_extra("parse_error", &e.to_string());
+                return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
+            }
+        };
+        let mut query_keys = parsed
+            .query_pairs()
+            .map(|p| p.0.to_string())
+            .collect::<Vec<String>>();
+        query_keys.sort();
+
+        // run the gauntlet of checks.
+        if !check_url(parsed, "Click", &filter.click_hosts)? {
+            dbg!("bad url", url.to_string());
+            tags.add_extra("reason", "bad host");
+            return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
+        }
+        for key in req_keys {
+            if !query_keys.contains(&key.to_owned()) {
+                dbg!("missing param", key, url.to_string());
+                tags.add_extra("reason", "missing required query param");
+                return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
+            }
+        }
+        for key in query_keys {
+            if !all_keys.contains(&key.as_str()) {
+                dbg!("invalid param", key, url.to_string());
+                tags.add_extra("reason", "invalid query param");
+                return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
+            }
+        }
+        Ok(())
     }
 
     /// Check the impression URL to see if it's valid.
@@ -135,12 +175,29 @@ impl AdmFilter {
         tile: &mut AdmTile,
         tags: &mut Tags,
     ) -> HandlerResult<()> {
-        check_url(
-            &tile.impression_url,
-            "Impression",
-            &filter.impression_hosts,
-            tags,
-        )
+        let url = &tile.impression_url;
+        let species = "Impression";
+        tags.add_tag("type", species);
+        tags.add_extra("url", &url);
+        let parsed: Url = match url.parse() {
+            Ok(v) => v,
+            Err(e) => {
+                tags.add_extra("parse_error", &e.to_string());
+                return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
+            }
+        };
+        let mut query_keys = parsed
+            .query_pairs()
+            .map(|p| p.0.to_string())
+            .collect::<Vec<String>>();
+        query_keys.sort();
+        if query_keys != vec!["id"] {
+            dbg!("missing param", "id", url.to_string());
+            tags.add_extra("reason", "invalid query param");
+            return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
+        }
+        check_url(parsed, species, &filter.impression_hosts)?;
+        Ok(())
     }
 
     /// Filter and process tiles from ADM:

src/server/location.rs

@@ -49,7 +49,6 @@ impl From<&RequestHead> for LocationResult {
                 ..Default::default()
             };
         }
-        dbg!("No Google header found");
         Self::default()
     }
 }

src/web/mock_adm_response1.json

@@ -14,7 +14,7 @@
             "click_url": "https://example.com/ctp?version=16.0.0&key=7.2&ci=8.9&ctag=E1DE38C8972D0281F5556659A&aespFlag=altinst",
             "image_url": "https://cdn.example.com/703.jpg",
             "advertiser_url": "https://www.dunderm.biz/?tag=bar&ref=baz",
-            "impression_url": "https://example.net/static?id=DEADB33F"
+            "impression_url": "https://example.com/static?id=DEADB33F"
         },
         {
             "id": 805,

src/web/test.rs

@@ -56,11 +56,11 @@ macro_rules! init_app {
 }
 
 /// Bind a mock of the AdM Tiles API to a random port on localhost
-fn init_mock_adm() -> (dev::Server, SocketAddr) {
-    let server = HttpServer::new(|| {
-        App::new().route(
+fn init_mock_adm(response: String) -> (dev::Server, SocketAddr) {
+    let server = HttpServer::new(move || {
+        App::new().data(response.clone()).route(
             "/",
-            web::get().to(|req: HttpRequest| {
+            web::get().to(|req: HttpRequest, resp: web::Data<String>| {
                 trace!(
                     "mock_adm: {:#?} {:#?} {:#?}",
                     req.path(),
@@ -69,7 +69,7 @@ fn init_mock_adm() -> (dev::Server, SocketAddr) {
                 );
                 HttpResponse::Ok()
                     .content_type("application/json")
-                    .body(MOCK_RESPONSE1)
+                    .body(resp.get_ref())
             }),
         )
     });
@@ -98,7 +98,7 @@ fn adm_settings() -> AdmSettings {
             advertiser_hosts: ["www.dunderm.biz".to_owned()].to_vec(),
             position: Some(1),
             include_regions: vec![],
-            impression_hosts: ["example.com".to_owned()].to_vec(),
+            impression_hosts: [].to_vec(),
             click_hosts: vec![],
         },
     );
@@ -128,7 +128,7 @@ fn adm_settings() -> AdmSettings {
 
 #[actix_rt::test]
 async fn basic() {
-    let (_, addr) = init_mock_adm();
+    let (_, addr) = init_mock_adm(MOCK_RESPONSE1.to_owned());
     let settings = Settings {
         adm_endpoint_url: format!("http://{}:{}/?partner=foo&sub1=bar", addr.ip(), addr.port()),
         adm_settings: json!(adm_settings()).to_string(),
@@ -161,9 +161,69 @@ async fn basic() {
     }
 }
 
+#[actix_rt::test]
+async fn basic_bad_reply() {
+    let missing_aespflag = r#"{
+        "tiles": [
+            {
+                "id": 601,
+                "name": "Acme",
+                "click_url": "https://example.com/ctp?version=16.0.0&key=22.1&ci=6.2&ctag=1612376952400200000",
+                "image_url": "https://cdn.example.com/601.jpg",
+                "advertiser_url": "https://www.acme.biz/?foo=1&device=Computers&cmpgn=123601",
+                "impression_url": "https://example.net/static?id=0000"
+            },
+            {
+                "id": 703,
+                "name": "Dunder Mifflin",
+                "click_url": "https://example.com/ctp?version=16.0.0&key=7.2&ci=8.9&ctag=E1DE38C8972D0281F5556659A&aespFlag=altinst",
+                "image_url": "https://cdn.example.com/703.jpg",
+                "advertiser_url": "https://www.dunderm.biz/?tag=bar&ref=baz",
+                "impression_url": "https://example.net/static?id=DEADB33F"
+            }
+        ]}"#;
+    let (_, addr) = init_mock_adm(missing_aespflag.to_owned());
+    let settings = Settings {
+        adm_endpoint_url: format!("http://{}:{}/?partner=foo&sub1=bar", addr.ip(), addr.port()),
+        adm_settings: json!(adm_settings()).to_string(),
+        ..get_test_settings()
+    };
+    let mut app = init_app!(settings).await;
+
+    let req = test::TestRequest::get()
+        .uri("/v1/tiles?country=UK&placement=newtab")
+        .header(header::USER_AGENT, UA)
+        .to_request();
+    let resp = test::call_service(&mut app, req).await;
+    assert_eq!(resp.status(), StatusCode::OK);
+
+    let content_type = resp.headers().get(header::CONTENT_TYPE);
+    assert!(content_type.is_some());
+    assert_eq!(
+        content_type
+            .unwrap()
+            .to_str()
+            .expect("Couldn't parse Content-Type"),
+        "application/json"
+    );
+
+    let result: Value = test::read_body_json(resp).await;
+    let tiles = result["tiles"].as_array().expect("!tiles.is_array()");
+    assert!(tiles.len() == 1);
+    assert!(
+        &tiles[0]
+            .as_object()
+            .unwrap()
+            .get("name")
+            .unwrap()
+            .to_string()
+            == "\"Dunder Mifflin\""
+    );
+}
+
 #[actix_rt::test]
 async fn basic_filtered() {
-    let (_, addr) = init_mock_adm();
+    let (_, addr) = init_mock_adm(MOCK_RESPONSE1.to_owned());
 
     let mut adm_settings = adm_settings();
     adm_settings.insert(
@@ -218,7 +278,7 @@ async fn basic_filtered() {
 
 #[actix_rt::test]
 async fn basic_default() {
-    let (_, addr) = init_mock_adm();
+    let (_, addr) = init_mock_adm(MOCK_RESPONSE1.to_owned());
 
     let adm_settings = adm_settings();
 
@@ -264,7 +324,7 @@ async fn basic_default() {
 
 #[actix_rt::test]
 async fn invalid_placement() {
-    let (_, addr) = init_mock_adm();
+    let (_, addr) = init_mock_adm(MOCK_RESPONSE1.to_owned());
     let settings = Settings {
         adm_endpoint_url: format!("http://{}:{}/?partner=foo&sub1=bar", addr.ip(), addr.port()),
         ..get_test_settings()