diff --git a/autoendpoint/src/extractors/subscription.rs b/autoendpoint/src/extractors/subscription.rs index c1d5f9b0e..767088d33 100644 --- a/autoendpoint/src/extractors/subscription.rs +++ b/autoendpoint/src/extractors/subscription.rs @@ -87,7 +87,7 @@ impl FromRequest for Subscription { if let Some(ref header) = vapid { let sub = header .vapid - .sub() + .insecure_sub() .map_err(|e: VapidError| { // Capture the type of error and add it to metrics. let mut tags = Tags::default(); @@ -332,6 +332,9 @@ fn validate_vapid_jwt( jsonwebtoken::errors::ErrorKind::InvalidAudience => { return Err(VapidError::InvalidAudience.into()); } + jsonwebtoken::errors::ErrorKind::MissingRequiredClaim(e) => { + return Err(VapidError::InvalidVapid(format!("Missing required {}", e)).into()); + } _ => { // Attempt to match up the majority of ErrorKind variants. // The third-party errors all defer to the source, so we can @@ -423,8 +426,8 @@ pub mod tests { let enc_key = jsonwebtoken::EncodingKey::from_ec_der(&PRIV_KEY); let claims = VapidClaims { exp, - aud: aud.to_string(), - sub: sub.to_string(), + aud: Some(aud.to_string()), + sub: Some(sub.to_string()), }; let token = jsonwebtoken::encode(&jwk_header, &claims, &enc_key).unwrap(); @@ -566,8 +569,8 @@ pub mod tests { let enc_key = jsonwebtoken::EncodingKey::from_ec_der(&PRIV_KEY); let claims = VapidClaims { exp: VapidClaims::default_exp() - 100, - aud: domain.to_owned(), - sub: "mailto:admin@example.com".to_owned(), + aud: Some(domain.to_owned()), + sub: Some("mailto:admin@example.com".to_owned()), }; let token = jsonwebtoken::encode(&jwk_header, &claims, &enc_key).unwrap(); // try standard form with padding diff --git a/autoendpoint/src/headers/vapid.rs b/autoendpoint/src/headers/vapid.rs index 349e9fc0e..e46bed6c8 100644 --- a/autoendpoint/src/headers/vapid.rs +++ b/autoendpoint/src/headers/vapid.rs @@ -3,7 +3,6 @@ use std::fmt; use base64::Engine; use serde::{Deserialize, Serialize}; -use serde_json::Value; use thiserror::Error; use crate::headers::util::split_key_value; @@ -15,23 +14,23 @@ pub const ALLOWED_SCHEMES: [&str; 3] = ["bearer", "webpush", "vapid"]; The Assertion block for the VAPID header. Please note: We require the `sub` claim in addition to the `exp` and `aud`. -See [HTTP Endpoints for Notficiations::Lexicon::{vapid_key}](https://mozilla-services.github.io/autopush-rs/http.html#lexicon-1) +See [HTTP Endpoints for Notifications::Lexicon::{vapid_key}](https://mozilla-services.github.io/autopush-rs/http.html#lexicon-1) for details. */ #[derive(Clone, Debug, Serialize, Deserialize, Eq, PartialEq)] pub struct VapidClaims { pub exp: u64, - pub aud: String, - pub sub: String, + pub aud: Option, + pub sub: Option, } impl Default for VapidClaims { fn default() -> Self { Self { exp: VapidClaims::default_exp(), - aud: "No audience".to_owned(), - sub: "No sub".to_owned(), + aud: None, + sub: None, } } } @@ -143,25 +142,27 @@ impl VapidHeader { } } - pub fn sub(&self) -> Result { - let data: HashMap = serde_json::from_str(&self.token).map_err(|e| { - warn!("πŸ” Vapid: {:?}", e); - VapidError::SubInvalid + /// Return the claimed `sub` after doing some minimal checks for validity. + /// WARNING: THIS FUNCTION DOES NOT VALIDATE THE VAPID HEADER AND SHOULD + /// ONLY BE USED FOR LOGGING AND METRIC REPORTING FUNCTIONS. + /// Proper validation should be done by [crate::extractors::subscription::validate_vapid_jwt()] + pub fn insecure_sub(&self) -> Result { + // This parses the VAPID header string + let data = VapidClaims::try_from(self.clone()).inspect_err(|e| { + warn!("πŸ” Vapid: {:?} {:?}", e, &self.token); })?; - if let Some(sub_candiate) = data.get("sub") { - if let Some(sub) = sub_candiate.as_str() { - if !sub.starts_with("mailto:") || !sub.starts_with("https://") { - info!("πŸ” Vapid: Bad Format {:?}", sub); - return Err(VapidError::SubBadFormat); - } - if sub.is_empty() { - info!("πŸ” Empty Vapid sub"); - return Err(VapidError::SubEmpty); - } - info!("πŸ” Vapid: sub: {:?}", sub); - return Ok(sub.to_owned()); + if let Some(sub) = data.sub { + if !sub.starts_with("mailto:") && !sub.starts_with("https://") { + info!("πŸ” Vapid: Bad Format {:?}", sub); + return Err(VapidError::SubBadFormat); } + if sub.is_empty() { + info!("πŸ” Empty Vapid sub"); + return Err(VapidError::SubEmpty); + } + info!("πŸ” Vapid: sub: {:?}", sub); + return Ok(sub.to_owned()); } Err(VapidError::SubMissing) } @@ -254,9 +255,16 @@ mod tests { returned_header.unwrap().claims(), Ok(VapidClaims { exp: 1713564872, - aud: "https://push.services.mozilla.com".to_string(), - sub: "mailto:admin@example.com".to_string() + aud: Some("https://push.services.mozilla.com".to_owned()), + sub: Some("mailto:admin@example.com".to_owned()) }) + ); + + // Ensure the parent `.sub()` returns a valid value. + let returned_header = VapidHeader::parse(VALID_HEADER); + assert_eq!( + returned_header.unwrap().insecure_sub(), + Ok("mailto:admin@example.com".to_owned()) ) } } diff --git a/autoendpoint/src/routers/common.rs b/autoendpoint/src/routers/common.rs index 34f68282d..325e675aa 100644 --- a/autoendpoint/src/routers/common.rs +++ b/autoendpoint/src/routers/common.rs @@ -153,7 +153,7 @@ pub async fn handle_error( if let Some(Ok(claims)) = vapid.map(|v| v.vapid.claims()) { let mut extras = err.extras.unwrap_or_default(); - extras.extend([("sub".to_owned(), claims.sub)]); + extras.extend([("sub".to_owned(), claims.sub.unwrap_or_default())]); err.extras = Some(extras); }; err diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs index 92e49bf64..5c09043eb 100644 --- a/autoendpoint/src/routers/fcm/client.rs +++ b/autoendpoint/src/routers/fcm/client.rs @@ -117,6 +117,7 @@ impl FcmClient { .http_client .post(self.endpoint.clone()) .header("Authorization", format!("Bearer {}", token)) + .header("Content-Type", "application/json") .json(&message) .timeout(self.timeout) .send() @@ -150,10 +151,13 @@ impl FcmClient { return Err(match (status, data.error) { (StatusCode::UNAUTHORIZED, _) => RouterError::Authentication, (StatusCode::NOT_FOUND, _) => RouterError::NotFound, - (_, Some(error)) => RouterError::Upstream { - status: error.status, - message: error.message, - }, + (_, Some(error)) => { + info!("πŸŒ‰Bridge Error: {:?}, {:?}", error.message, &self.endpoint); + RouterError::Upstream { + status: error.status, + message: error.message, + } + } (status, None) => RouterError::Upstream { status: status.to_string(), message: "Unknown reason".to_string(), diff --git a/autoendpoint/src/routers/fcm/router.rs b/autoendpoint/src/routers/fcm/router.rs index e497dd173..76f57a4e3 100644 --- a/autoendpoint/src/routers/fcm/router.rs +++ b/autoendpoint/src/routers/fcm/router.rs @@ -1,4 +1,4 @@ -use autopush_common::{db::client::DbClient, MAX_NOTIFICATION_TTL}; +use autopush_common::{db::client::DbClient, MAX_FCM_NOTIFICATION_TTL}; use crate::error::ApiResult; use crate::extractors::notification::Notification; @@ -153,8 +153,8 @@ impl Router for FcmRouter { let (routing_token, app_id) = self.routing_info(router_data, ¬ification.subscription.user.uaid)?; - let ttl = - MAX_NOTIFICATION_TTL.min(self.settings.min_ttl.max(notification.headers.ttl as u64)); + let ttl = MAX_FCM_NOTIFICATION_TTL + .min(self.settings.min_ttl.max(notification.headers.ttl as u64)); // Send the notification to FCM let client = self diff --git a/autoendpoint/src/routers/webpush.rs b/autoendpoint/src/routers/webpush.rs index ac14b31e3..bdcd915c5 100644 --- a/autoendpoint/src/routers/webpush.rs +++ b/autoendpoint/src/routers/webpush.rs @@ -168,7 +168,9 @@ impl WebPushRouter { let mut err = ApiError::from(error); if let Some(Ok(claims)) = vapid.map(|v| v.vapid.claims()) { let mut extras = err.extras.unwrap_or_default(); - extras.extend([("sub".to_owned(), claims.sub)]); + if let Some(sub) = claims.sub { + extras.extend([("sub".to_owned(), sub)]); + } err.extras = Some(extras); }; err @@ -209,12 +211,15 @@ impl WebPushRouter { self.handle_error( ApiErrorKind::Router(RouterError::SaveDb( e, - // try to extract the `sub` from the VAPID clamis. - notification - .subscription - .vapid - .as_ref() - .map(|vapid| vapid.vapid.claims().map(|c| c.sub).unwrap_or_default()), + // try to extract the `sub` from the VAPID claims. + notification.subscription.vapid.as_ref().map(|vapid| { + vapid + .vapid + .claims() + .ok() + .and_then(|c| c.sub) + .unwrap_or_default() + }), )), notification.subscription.vapid.clone(), ) diff --git a/autopush-common/src/lib.rs b/autopush-common/src/lib.rs index 8656e814c..f59243981 100644 --- a/autopush-common/src/lib.rs +++ b/autopush-common/src/lib.rs @@ -34,7 +34,7 @@ pub mod util; /// That gets back to the concept that Push messages are supposed to be "timely". /// A user may not appreciate that they have an undelivered calendar reminder from /// 58 days ago, nor should they be interested in a meeting alert that happened last -/// month. When a User Agent (UA) connects, it recieves all pending messages. If +/// month. When a User Agent (UA) connects, it receives all pending messages. If /// a user has not used the User Agent in more than /// [60 days](https://searchfox.org/mozilla-central/search?q=OFFER_PROFILE_RESET_INTERVAL_MS), /// the User Agent suggest "refreshing Firefox", which essentially throws away one's @@ -44,7 +44,10 @@ pub mod util; /// "abandoned" and any router info assigned to a User Agent that has not contacted /// Autopush in 60 days can be discarded. /// +const ONE_DAY_IN_SECONDS: u64 = 24 * 60 * 60; /// The maximum TTL for notifications, 30 days in seconds -pub const MAX_NOTIFICATION_TTL: u64 = 30 * 24 * 60 * 60; +pub const MAX_NOTIFICATION_TTL: u64 = 30 * ONE_DAY_IN_SECONDS; +/// FCM has a max TTL of 4 weeks. +pub const MAX_FCM_NOTIFICATION_TTL: u64 = 4 * 7 * ONE_DAY_IN_SECONDS; /// The maximum TTL for router records, 60 days in seconds pub const MAX_ROUTER_TTL: u64 = 2 * MAX_NOTIFICATION_TTL;