client: Parse auth callback query parameters before showing sign-in success page (#36440)

Marshall Bowers created 3 months ago

This PR fixes an issue where we would redirect the user's browser to the
sign-in success page even if the OAuth callback was malformed.

We now parse the OAuth callback parameters from the query string and
only redirect to the sign-in success page when they are valid.

Release Notes:

- Updated the sign-in flow to not show the sign-in success page
prematurely.

Change summary

Cargo.lock                  |  1 +
Cargo.toml                  |  1 +
crates/client/Cargo.toml    |  1 +
crates/client/src/client.rs | 24 +++++++++++++-----------
4 files changed, 16 insertions(+), 11 deletions(-)

Detailed changes

Cargo.lock

@@ -3070,6 +3070,7 @@ dependencies = [
  "schemars",
  "serde",
  "serde_json",
+ "serde_urlencoded",
  "settings",
  "sha2",
  "smol",

Cargo.toml

@@ -582,6 +582,7 @@ serde_json_lenient = { version = "0.2", features = [
     "raw_value",
 ] }
 serde_repr = "0.1"
+serde_urlencoded = "0.7"
 sha2 = "0.10"
 shellexpand = "2.1.0"
 shlex = "1.3.0"

crates/client/Cargo.toml

@@ -44,6 +44,7 @@ rpc = { workspace = true, features = ["gpui"] }
 schemars.workspace = true
 serde.workspace = true
 serde_json.workspace = true
+serde_urlencoded.workspace = true
 settings.workspace = true
 sha2.workspace = true
 smol.workspace = true

crates/client/src/client.rs

@@ -1410,6 +1410,12 @@ impl Client {
 
                     open_url_tx.send(url).log_err();
 
+                    #[derive(Deserialize)]
+                    struct CallbackParams {
+                        pub user_id: String,
+                        pub access_token: String,
+                    }
+
                     // Receive the HTTP request from the user's browser. Retrieve the user id and encrypted
                     // access token from the query params.
                     //
@@ -1420,17 +1426,13 @@ impl Client {
                             for _ in 0..100 {
                                 if let Some(req) = server.recv_timeout(Duration::from_secs(1))? {
                                     let path = req.url();
-                                    let mut user_id = None;
-                                    let mut access_token = None;
                                     let url = Url::parse(&format!("http://example.com{}", path))
                                         .context("failed to parse login notification url")?;
-                                    for (key, value) in url.query_pairs() {
-                                        if key == "access_token" {
-                                            access_token = Some(value.to_string());
-                                        } else if key == "user_id" {
-                                            user_id = Some(value.to_string());
-                                        }
-                                    }
+                                    let callback_params: CallbackParams =
+                                        serde_urlencoded::from_str(url.query().unwrap_or_default())
+                                            .context(
+                                                "failed to parse sign-in callback query parameters",
+                                            )?;
 
                                     let post_auth_url =
                                         http.build_url("/native_app_signin_succeeded");
@@ -1445,8 +1447,8 @@ impl Client {
                                     )
                                     .context("failed to respond to login http request")?;
                                     return Ok((
-                                        user_id.context("missing user_id parameter")?,
-                                        access_token.context("missing access_token parameter")?,
+                                        callback_params.user_id,
+                                        callback_params.access_token,
                                     ));
                                 }
                             }