bug: Address insecure sub header fetch. (#803)

When insecurely getting the `sub` for tracking and errors, we were not
properly decoding. This was resulting in a VAPID error being generated.
This should not impact processing, but was generating a lot of logging
messages.

* FCM would reject TTLS > that 4 weeks, which is less than
our 30 day max. Added a specific filter for that.

* fixed some spelling mistakes.

Closes: [SYNC-4514](https://mozilla-hub.atlassian.net/browse/SYNC-4514)

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:[email protected]".to_owned(),
+            aud: Some(domain.to_owned()),
+            sub: Some("mailto:[email protected]".to_owned()),
         };
         let token = jsonwebtoken::encode(&jwk_header, &claims, &enc_key).unwrap();
         // try standard form with padding

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<String>,
+    pub sub: Option<String>,
 }
 
 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<String, VapidError> {
-        let data: HashMap<String, Value> = 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<String, VapidError> {
+        // 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:[email protected]".to_string()
+                aud: Some("https://push.services.mozilla.com".to_owned()),
+                sub: Some("mailto:[email protected]".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:[email protected]".to_owned())
         )
     }
 }

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

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(),

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, &notification.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

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(),
                 )

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;