FxA: Track location when parsing URLs (#6981)

I'm hoping this will help us track down https://bugzilla.mozilla.org/show_bug.cgi?id=1991756

Author: bendk

components/fxa-client/src/error.rs

@@ -167,8 +167,13 @@ pub enum Error {
     #[error("Network error: {0}")]
     RequestError(#[from] viaduct::ViaductError),
 
-    #[error("Malformed URL error: {0}")]
-    MalformedUrl(#[from] url::ParseError),
+    #[error("Malformed URL: {sanitized_url} ({when})")]
+    MalformedUrl {
+        /// URL without any query or fragment parts.
+        /// This is safe to send in an error report because there's no auth information.
+        sanitized_url: String,
+        when: String,
+    },
 
     #[error("Unexpected HTTP status: {0}")]
     UnexpectedStatus(#[from] viaduct::UnexpectedStatus),

components/fxa-client/src/internal/config.rs

@@ -3,7 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use super::http_client;
-use crate::{FxaConfig, Result};
+use crate::{internal::util, FxaConfig, Result};
 use serde_derive::{Deserialize, Serialize};
 use std::{cell::RefCell, sync::Arc};
 use url::Url;
@@ -77,11 +77,11 @@ impl Config {
     }
 
     pub fn content_url(&self) -> Result<Url> {
-        Url::parse(&self.content_url).map_err(Into::into)
+        util::parse_url(&self.content_url, "content_url")
     }
 
     pub fn content_url_path(&self, path: &str) -> Result<Url> {
-        self.content_url()?.join(path).map_err(Into::into)
+        util::join_url(&self.content_url()?, path, "content_url_path")
     }
 
     pub fn client_config_url(&self) -> Result<Url> {
@@ -117,44 +117,57 @@ impl Config {
     }
 
     pub fn auth_url(&self) -> Result<Url> {
-        Url::parse(&self.remote_config()?.auth_url).map_err(Into::into)
+        util::parse_url(&self.remote_config()?.auth_url, "auth_url")
     }
 
     pub fn auth_url_path(&self, path: &str) -> Result<Url> {
-        self.auth_url()?.join(path).map_err(Into::into)
+        util::join_url(&self.auth_url()?, path, "auth_url_path")
     }
 
     pub fn oauth_url(&self) -> Result<Url> {
-        Url::parse(&self.remote_config()?.oauth_url).map_err(Into::into)
+        util::parse_url(&self.remote_config()?.oauth_url, "oauth_url")
     }
 
     pub fn oauth_url_path(&self, path: &str) -> Result<Url> {
-        self.oauth_url()?.join(path).map_err(Into::into)
+        util::join_url(&self.oauth_url()?, path, "oauth_url_path")
     }
 
     pub fn token_server_endpoint_url(&self) -> Result<Url> {
         if let Some(token_server_url_override) = &self.token_server_url_override {
-            return Ok(Url::parse(token_server_url_override)?);
+            return util::parse_url(
+                token_server_url_override,
+                "token_server_endpoint_url (override)",
+            );
         }
-        Ok(Url::parse(
+        util::parse_url(
             &self.remote_config()?.token_server_endpoint_url,
-        )?)
+            "token_server_endpoint_url",
+        )
     }
 
     pub fn authorization_endpoint(&self) -> Result<Url> {
-        Url::parse(&self.remote_config()?.authorization_endpoint).map_err(Into::into)
+        util::parse_url(
+            &self.remote_config()?.authorization_endpoint,
+            "authorization_endpoint",
+        )
     }
 
     pub fn token_endpoint(&self) -> Result<Url> {
-        Url::parse(&self.remote_config()?.token_endpoint).map_err(Into::into)
+        util::parse_url(&self.remote_config()?.token_endpoint, "token_endpoint")
     }
 
     pub fn introspection_endpoint(&self) -> Result<Url> {
-        Url::parse(&self.remote_config()?.introspection_endpoint).map_err(Into::into)
+        util::parse_url(
+            &self.remote_config()?.introspection_endpoint,
+            "introspection_endpoint",
+        )
     }
 
     pub fn userinfo_endpoint(&self) -> Result<Url> {
-        Url::parse(&self.remote_config()?.userinfo_endpoint).map_err(Into::into)
+        util::parse_url(
+            &self.remote_config()?.userinfo_endpoint,
+            "userinfo_endpoint",
+        )
     }
 
     fn normalize_token_server_url(token_server_url_override: &str) -> String {

components/fxa-client/src/internal/mod.rs

@@ -133,11 +133,15 @@ impl FirefoxAccount {
     /// a computer).
     pub fn get_pairing_authority_url(&self) -> Result<String> {
         // Special case for the production server, we use the shorter firefox.com/pair URL.
-        if self.state.config().content_url()? == Url::parse(config::CONTENT_URL_RELEASE)? {
+        let content_url_release = Url::parse(config::CONTENT_URL_RELEASE)
+            .expect("hardcoded CONTENT_URL_RELEASE failed to parse");
+        if self.state.config().content_url()? == content_url_release {
             return Ok("https://firefox.com/pair".to_owned());
         }
         // Similarly special case for the China server.
-        if self.state.config().content_url()? == Url::parse(config::CONTENT_URL_CHINA)? {
+        let content_url_china = Url::parse(config::CONTENT_URL_CHINA)
+            .expect("hardcoded CONTENT_URL_CHINA failed to parse");
+        if self.state.config().content_url()? == content_url_china {
             return Ok("https://firefox.com.cn/pair".to_owned());
         }
         Ok(self.state.config().pair_url()?.into())

components/fxa-client/src/internal/oauth.rs

@@ -137,7 +137,7 @@ impl FirefoxAccount {
     ) -> Result<String> {
         let mut url = self.state.config().pair_supp_url()?;
         url.query_pairs_mut().append_pair("entrypoint", entrypoint);
-        let pairing_url = Url::parse(pairing_url)?;
+        let pairing_url = util::parse_url(pairing_url, "begin_pairing_flow")?;
         if url.host_str() != pairing_url.host_str() {
             let fxa_server = FxaServer::from(&url);
             let pairing_fxa_server = FxaServer::from(&pairing_url);

components/fxa-client/src/internal/util.rs

@@ -52,3 +52,52 @@ impl Xorable for [u8] {
         }
     }
 }
+
+pub fn parse_url(url: &str, when: impl Into<String>) -> Result<url::Url> {
+    url::Url::parse(url).map_err(|_| Error::MalformedUrl {
+        sanitized_url: sanitized_url(url),
+        when: when.into(),
+    })
+}
+
+pub fn join_url(url: &url::Url, path: &str, when: impl Into<String>) -> Result<url::Url> {
+    url.join(path).map_err(|_| Error::MalformedUrl {
+        sanitized_url: sanitized_url(url.as_str()),
+        when: when.into(),
+    })
+}
+
+fn sanitized_url(url: &str) -> String {
+    // Remove everything after the `?` char, this is the URL params where all the auth data
+    // goes.
+    match url.split_once(['?', '#']) {
+        Some((before_qmark, _)) => before_qmark,
+        None => url,
+    }
+    .to_string()
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_sanitized_url() {
+        assert_eq!(
+            sanitized_url("https://mozilla.com/foo/bar"),
+            "https://mozilla.com/foo/bar"
+        );
+        assert_eq!(
+            sanitized_url("https://mozilla.com/foo/bar?password=1234"),
+            "https://mozilla.com/foo/bar"
+        );
+        assert_eq!(
+            sanitized_url("https://mozilla.com/foo/bar?password=1234#key=4321"),
+            "https://mozilla.com/foo/bar"
+        );
+        assert_eq!(
+            sanitized_url("https://mozilla.com/foo/bar#key=4321"),
+            "https://mozilla.com/foo/bar"
+        );
+    }
+}