Skip to content
This repository has been archived by the owner on Jul 26, 2024. It is now read-only.

Commit

Permalink
feat: only include hostname in InvalidHost's err message (#327)
Browse files Browse the repository at this point in the history 
to reduce the sentry event cardinality from paths/their query parameters

also include the full audience key & adm_url as extras

Closes #322
Closes #323
  • Loading branch information
@pjenvey
pjenvey authored Nov 29, 2021
1 parent 6f9afc3 commit 7727827
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 51 deletions.
79 changes: 36 additions & 43 deletions src/adm/filter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,24 @@ pub struct AdmFilter {
pub legacy_list: HashSet<String>,
}

/// Parse &str into a `Url`
fn parse_url(
url: &str,
species: &'static str,
tile_name: &str,
tags: &mut Tags,
) -> HandlerResult<Url> {
Url::parse(url).map_err(|e| {
tags.add_tag("type", species);
tags.add_extra("tile", tile_name);
tags.add_extra("url", url);
tags.add_extra("parse_error", &e.to_string());
HandlerErrorKind::InvalidHost(species, "Url::parse failed".to_owned()).into()
})
}

/// Extract the host from Url
fn get_host(url: Url, species: &'static str) -> HandlerResult<String> {
fn get_host(url: &Url, species: &'static str) -> HandlerResult<String> {
url.host()
.map(|host| host.to_string())
.ok_or_else(|| HandlerErrorKind::MissingHost(species, url.to_string()).into())
Expand All @@ -69,7 +85,7 @@ fn get_host(url: Url, species: &'static str) -> HandlerResult<String> {
/// "com"]) allows "foo.example.com" and "quux.bar.example.com" (["quux",
/// "bar", "example", "com"])
fn check_url(url: Url, species: &'static str, filter: &[Vec<String>]) -> HandlerResult<bool> {
let host = get_host(url, species)?;
let host = get_host(&url, species)?;
let domains: Vec<_> = host.split('.').collect();
for allowed in filter {
let begin = domains.len() - allowed.len().min(domains.len());
Expand Down Expand Up @@ -100,26 +116,17 @@ impl AdmFilter {
) -> HandlerResult<()> {
let url = &tile.advertiser_url;
let species = "Advertiser";
let parsed: Url = match url.parse() {
Ok(v) => v,
Err(e) => {
tags.add_tag("type", species);
tags.add_extra("tile", &tile.name);
tags.add_extra("url", url);
tags.add_extra("parse_error", &e.to_string());
return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
}
};
let parsed = parse_url(url, species, &tile.name, tags)?;
let host = get_host(&parsed, species)?;

if parsed.scheme().to_lowercase() != "https" {
return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
tags.add_tag("type", species);
tags.add_extra("tile", &tile.name);
tags.add_extra("url", url);
return Err(HandlerErrorKind::InvalidHost(species, host).into());
}

// do a quick string comparison between the supplied host and the provided filter.
let host = parsed
.host()
.ok_or_else(|| HandlerErrorKind::MissingHost(species, url.to_string()))?
.to_string();
let mut path = Cow::from(parsed.path());
if !path.ends_with('/') {
path.to_mut().push('/');
Expand All @@ -145,7 +152,10 @@ impl AdmFilter {
};
}

Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into())
tags.add_tag("type", species);
tags.add_extra("tile", &tile.name);
tags.add_extra("url", url);
Err(HandlerErrorKind::InvalidHost(species, host).into())
}

/// Check the click URL
Expand All @@ -162,17 +172,8 @@ impl AdmFilter {
let species = "Click";
// Check the required fields are present for the `click_url` pg 15 of
// 5.7.21 spec
let parsed: Url = match url.parse() {
Ok(v) => v,
Err(e) => {
tags.add_tag("type", species);
tags.add_extra("tile", &tile.name);
tags.add_extra("url", url);

tags.add_extra("parse_error", &e.to_string());
return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
}
};
let parsed = parse_url(url, species, &tile.name, tags)?;
let host = get_host(&parsed, species)?;
let query_keys = parsed
.query_pairs()
.map(|p| p.0.to_string())
Expand All @@ -186,7 +187,7 @@ impl AdmFilter {
tags.add_extra("url", url);

tags.add_extra("reason", "bad host");
return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
return Err(HandlerErrorKind::InvalidHost(species, host).into());
}
for key in &*REQ_CLICK_PARAMS {
if !query_keys.contains(*key) {
Expand All @@ -197,7 +198,7 @@ impl AdmFilter {

tags.add_extra("reason", "missing required query param");
tags.add_extra("param", key);
return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
return Err(HandlerErrorKind::InvalidHost(species, host).into());
}
}
for key in query_keys {
Expand All @@ -209,7 +210,7 @@ impl AdmFilter {

tags.add_extra("reason", "invalid query param");
tags.add_extra("param", &key);
return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
return Err(HandlerErrorKind::InvalidHost(species, host).into());
}
}
Ok(())
Expand All @@ -226,16 +227,7 @@ impl AdmFilter {
) -> HandlerResult<()> {
let url = &tile.impression_url;
let species = "Impression";
let parsed: Url = match url.parse() {
Ok(v) => v,
Err(e) => {
tags.add_tag("type", species);
tags.add_extra("tile", &tile.name);
tags.add_extra("url", url);
tags.add_extra("parse_error", &e.to_string());
return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
}
};
let parsed = parse_url(url, species, &tile.name, tags)?;
let mut query_keys = parsed
.query_pairs()
.map(|p| p.0.to_string())
Expand All @@ -248,7 +240,8 @@ impl AdmFilter {
tags.add_extra("url", url);
tags.add_extra("reason", "invalid query param");
tags.add_extra("param", "id");
return Err(HandlerErrorKind::InvalidHost(species, url.to_string()).into());
let host = get_host(&parsed, species)?;
return Err(HandlerErrorKind::InvalidHost(species, host).into());
}
check_url(parsed, species, &filter.impression_hosts)?;
Ok(())
Expand Down
1 change: 1 addition & 0 deletions src/adm/tiles.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ pub async fn get_tiles(
.into_string()
.unwrap_or_else(|_| "Unknown".to_owned()),
);
tags.add_extra("adm_url", adm_url);

info!("adm::get_tiles GET {}", adm_url);
metrics.incr("tiles.adm.request");
Expand Down
15 changes: 7 additions & 8 deletions src/web/handlers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,6 @@ pub async fn get_tiles(
return Ok(response);
}

let mut tags = Tags::default();
{
tags.add_extra("country", &location.country());
tags.add_extra("region", &location.region());
// Add/modify the existing request tags.
// tags.clone().commit(&mut request.extensions_mut());
}

let audience_key = cache::AudienceKey {
country_code: location.country(),
region_code: if location.region() != "" {
Expand All @@ -88,6 +80,13 @@ pub async fn get_tiles(
legacy_only: device_info.legacy_only(),
};

let mut tags = Tags::default();
{
tags.add_extra("audience_key", &format!("{:#?}", audience_key));
// Add/modify the existing request tags.
// tags.clone().commit(&mut request.extensions_mut());
}

let mut expired = false;
if !settings.test_mode {
// First make a cheap read from the cache
Expand Down

0 comments on commit 7727827

Please sign in to comment.