Skip to content

Commit 993ab57

Browse files
committed
Record more details for OriginMismatch
We're getting a higher rate of these than expected, let's try to figure out what's happening.
1 parent 78810d7 commit 993ab57

File tree

3 files changed

+80
-6
lines changed

3 files changed

+80
-6
lines changed

components/fxa-client/src/error.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,8 @@ pub enum Error {
106106
#[error("Cannot xor arrays with different lengths: {0} and {1}")]
107107
XorLengthMismatch(usize, usize),
108108

109-
#[error("Origin mismatch")]
110-
OriginMismatch,
109+
#[error("Origin mismatch: {0}")]
110+
OriginMismatch(String),
111111

112112
#[error("Remote key and local key mismatch")]
113113
MismatchedKeys,
@@ -213,7 +213,7 @@ impl GetErrorHandling for Error {
213213
Error::BackoffError(_) => {
214214
ErrorHandling::convert(FxaError::Other).report_error("fxa-client-backoff")
215215
}
216-
Error::OriginMismatch => ErrorHandling::convert(FxaError::OriginMismatch),
216+
Error::OriginMismatch(_) => ErrorHandling::convert(FxaError::OriginMismatch),
217217
_ => ErrorHandling::convert(FxaError::Other).report_error("fxa-client-other-error"),
218218
}
219219
}

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use super::{
1111
scoped_keys::ScopedKeysFlow,
1212
util, FirefoxAccount,
1313
};
14-
use crate::{AuthorizationParameters, Error, MetricsParams, Result, ScopedKey};
14+
use crate::{AuthorizationParameters, Error, FxaServer, MetricsParams, Result, ScopedKey};
1515
use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine};
1616
use jwcrypto::{EncryptionAlgorithm, EncryptionParameters};
1717
use rate_limiter::RateLimiter;
@@ -134,7 +134,11 @@ impl FirefoxAccount {
134134
}
135135
let pairing_url = Url::parse(pairing_url)?;
136136
if url.host_str() != pairing_url.host_str() {
137-
return Err(Error::OriginMismatch);
137+
let fxa_server = FxaServer::from(&url);
138+
let pairing_fxa_server = FxaServer::from(&pairing_url);
139+
return Err(Error::OriginMismatch(format!(
140+
"fxa-server: {fxa_server}, pairing-url-fxa-server: {pairing_fxa_server}"
141+
)));
138142
}
139143
url.set_fragment(pairing_url.fragment());
140144
self.oauth_flow(url, scopes)

components/fxa-client/src/lib.rs

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,10 @@ mod storage;
4747
mod telemetry;
4848
mod token;
4949

50+
use std::fmt;
51+
5052
pub use sync15::DeviceType;
53+
use url::Url;
5154

5255
pub use auth::{AuthorizationInfo, FxaRustAuthState, MetricsParams};
5356
pub use device::{AttachedClient, Device, DeviceCapability, LocalDevice};
@@ -109,7 +112,7 @@ pub struct FxaConfig {
109112
pub token_server_url_override: Option<String>,
110113
}
111114

112-
#[derive(Clone, Debug)]
115+
#[derive(Clone, Debug, PartialEq, Eq)]
113116
pub enum FxaServer {
114117
Release,
115118
Stable,
@@ -132,6 +135,47 @@ impl FxaServer {
132135
}
133136
}
134137

138+
impl From<&Url> for FxaServer {
139+
fn from(url: &Url) -> Self {
140+
let origin = url.origin();
141+
// Note: we can call unwrap() below because parsing content_url for known servers should
142+
// never fail and `test_from_url` tests this.
143+
if origin == Url::parse(Self::Release.content_url()).unwrap().origin() {
144+
Self::Release
145+
} else if origin == Url::parse(Self::Stable.content_url()).unwrap().origin() {
146+
Self::Stable
147+
} else if origin == Url::parse(Self::Stage.content_url()).unwrap().origin() {
148+
Self::Stage
149+
} else if origin == Url::parse(Self::China.content_url()).unwrap().origin() {
150+
Self::China
151+
} else if origin == Url::parse(Self::LocalDev.content_url()).unwrap().origin() {
152+
Self::LocalDev
153+
} else {
154+
Self::Custom {
155+
url: origin.ascii_serialization(),
156+
}
157+
}
158+
}
159+
}
160+
161+
/// Display impl
162+
///
163+
/// This identifies the variant, without recording the URL for custom servers. It's good for
164+
/// Sentry reports when we don't want to give away any PII.
165+
impl fmt::Display for FxaServer {
166+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
167+
let variant_name = match self {
168+
Self::Release => "Release",
169+
Self::Stable => "Stable",
170+
Self::Stage => "Stage",
171+
Self::China => "China",
172+
Self::LocalDev => "LocalDev",
173+
Self::Custom { .. } => "Custom",
174+
};
175+
write!(f, "{variant_name}")
176+
}
177+
}
178+
135179
impl FxaConfig {
136180
pub fn release(client_id: impl ToString, redirect_uri: impl ToString) -> Self {
137181
Self {
@@ -180,3 +224,29 @@ impl FxaConfig {
180224
}
181225

182226
uniffi::include_scaffolding!("fxa_client");
227+
228+
#[cfg(test)]
229+
mod tests {
230+
use super::*;
231+
232+
#[test]
233+
fn test_from_url() {
234+
let test_cases = [
235+
("https://accounts.firefox.com", FxaServer::Release),
236+
("https://stable.dev.lcip.org", FxaServer::Stable),
237+
("https://accounts.stage.mozaws.net", FxaServer::Stage),
238+
("https://accounts.firefox.com.cn", FxaServer::China),
239+
("http://127.0.0.1:3030", FxaServer::LocalDev),
240+
(
241+
"http://my-fxa-server.com",
242+
FxaServer::Custom {
243+
url: "http://my-fxa-server.com".to_owned(),
244+
},
245+
),
246+
];
247+
for (content_url, expected_result) in test_cases {
248+
let url = Url::parse(content_url).unwrap();
249+
assert_eq!(FxaServer::from(&url), expected_result);
250+
}
251+
}
252+
}

0 commit comments

Comments
 (0)