Skip to content

Commit

Permalink
Record more details for OriginMismatch
Browse files Browse the repository at this point in the history
We're getting a higher rate of these than expected, let's try to figure
out what's happening.
  • Loading branch information
bendk committed Nov 23, 2023
1 parent 78810d7 commit 993ab57
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 6 deletions.
6 changes: 3 additions & 3 deletions components/fxa-client/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ pub enum Error {
#[error("Cannot xor arrays with different lengths: {0} and {1}")]
XorLengthMismatch(usize, usize),

#[error("Origin mismatch")]
OriginMismatch,
#[error("Origin mismatch: {0}")]
OriginMismatch(String),

#[error("Remote key and local key mismatch")]
MismatchedKeys,
Expand Down Expand Up @@ -213,7 +213,7 @@ impl GetErrorHandling for Error {
Error::BackoffError(_) => {
ErrorHandling::convert(FxaError::Other).report_error("fxa-client-backoff")
}
Error::OriginMismatch => ErrorHandling::convert(FxaError::OriginMismatch),
Error::OriginMismatch(_) => ErrorHandling::convert(FxaError::OriginMismatch),
_ => ErrorHandling::convert(FxaError::Other).report_error("fxa-client-other-error"),
}
}
Expand Down
8 changes: 6 additions & 2 deletions components/fxa-client/src/internal/oauth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use super::{
scoped_keys::ScopedKeysFlow,
util, FirefoxAccount,
};
use crate::{AuthorizationParameters, Error, MetricsParams, Result, ScopedKey};
use crate::{AuthorizationParameters, Error, FxaServer, MetricsParams, Result, ScopedKey};
use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine};
use jwcrypto::{EncryptionAlgorithm, EncryptionParameters};
use rate_limiter::RateLimiter;
Expand Down Expand Up @@ -134,7 +134,11 @@ impl FirefoxAccount {
}
let pairing_url = Url::parse(pairing_url)?;
if url.host_str() != pairing_url.host_str() {
return Err(Error::OriginMismatch);
let fxa_server = FxaServer::from(&url);
let pairing_fxa_server = FxaServer::from(&pairing_url);
return Err(Error::OriginMismatch(format!(
"fxa-server: {fxa_server}, pairing-url-fxa-server: {pairing_fxa_server}"
)));
}
url.set_fragment(pairing_url.fragment());
self.oauth_flow(url, scopes)
Expand Down
72 changes: 71 additions & 1 deletion components/fxa-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ mod storage;
mod telemetry;
mod token;

use std::fmt;

pub use sync15::DeviceType;
use url::Url;

pub use auth::{AuthorizationInfo, FxaRustAuthState, MetricsParams};
pub use device::{AttachedClient, Device, DeviceCapability, LocalDevice};
Expand Down Expand Up @@ -109,7 +112,7 @@ pub struct FxaConfig {
pub token_server_url_override: Option<String>,
}

#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum FxaServer {
Release,
Stable,
Expand All @@ -132,6 +135,47 @@ impl FxaServer {
}
}

impl From<&Url> for FxaServer {
fn from(url: &Url) -> Self {
let origin = url.origin();
// Note: we can call unwrap() below because parsing content_url for known servers should
// never fail and `test_from_url` tests this.
if origin == Url::parse(Self::Release.content_url()).unwrap().origin() {
Self::Release
} else if origin == Url::parse(Self::Stable.content_url()).unwrap().origin() {
Self::Stable
} else if origin == Url::parse(Self::Stage.content_url()).unwrap().origin() {
Self::Stage
} else if origin == Url::parse(Self::China.content_url()).unwrap().origin() {
Self::China
} else if origin == Url::parse(Self::LocalDev.content_url()).unwrap().origin() {
Self::LocalDev
} else {
Self::Custom {
url: origin.ascii_serialization(),
}
}
}
}

/// Display impl
///
/// This identifies the variant, without recording the URL for custom servers. It's good for
/// Sentry reports when we don't want to give away any PII.
impl fmt::Display for FxaServer {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let variant_name = match self {
Self::Release => "Release",
Self::Stable => "Stable",
Self::Stage => "Stage",
Self::China => "China",
Self::LocalDev => "LocalDev",
Self::Custom { .. } => "Custom",
};
write!(f, "{variant_name}")
}
}

impl FxaConfig {
pub fn release(client_id: impl ToString, redirect_uri: impl ToString) -> Self {
Self {
Expand Down Expand Up @@ -180,3 +224,29 @@ impl FxaConfig {
}

uniffi::include_scaffolding!("fxa_client");

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_from_url() {
let test_cases = [
("https://accounts.firefox.com", FxaServer::Release),
("https://stable.dev.lcip.org", FxaServer::Stable),
("https://accounts.stage.mozaws.net", FxaServer::Stage),
("https://accounts.firefox.com.cn", FxaServer::China),
("http://127.0.0.1:3030", FxaServer::LocalDev),
(
"http://my-fxa-server.com",
FxaServer::Custom {
url: "http://my-fxa-server.com".to_owned(),
},
),
];
for (content_url, expected_result) in test_cases {
let url = Url::parse(content_url).unwrap();
assert_eq!(FxaServer::from(&url), expected_result);
}
}
}

0 comments on commit 993ab57

Please sign in to comment.