From 5e52f24490cbd64b663c9ead8d23a7bdac3d73ec Mon Sep 17 00:00:00 2001 From: JR Conlin Date: Wed, 19 May 2021 15:28:34 -0700 Subject: [PATCH] bug: Add additional verification checks. (#80) bug: Add additional verification checks. * add req and opt query param check to click_url validator * add 'id' as req field for click_url Closes #22 --- src/adm.rs | 127 +++++++++++++++++++++++--------- src/server/location.rs | 1 - src/web/mock_adm_response1.json | 2 +- src/web/test.rs | 80 +++++++++++++++++--- 4 files changed, 163 insertions(+), 47 deletions(-) diff --git a/src/adm.rs b/src/adm.rs index 5d4076a1..b3378b71 100644 --- a/src/adm.rs +++ b/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 { + 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::>(); + 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::>(); + 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: diff --git a/src/server/location.rs b/src/server/location.rs index c8a1662d..30ced840 100644 --- a/src/server/location.rs +++ b/src/server/location.rs @@ -49,7 +49,6 @@ impl From<&RequestHead> for LocationResult { ..Default::default() }; } - dbg!("No Google header found"); Self::default() } } diff --git a/src/web/mock_adm_response1.json b/src/web/mock_adm_response1.json index 74d59217..b7c1d2b8 100644 --- a/src/web/mock_adm_response1.json +++ b/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, diff --git a/src/web/test.rs b/src/web/test.rs index c7276e15..e8c7b067 100644 --- a/src/web/test.rs +++ b/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| { 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()