diff --git a/Cargo.lock b/Cargo.lock index 4b8cb93..472ca09 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -180,7 +180,9 @@ name = "builderlab-auth" version = "0.1.0" dependencies = [ "anyhow", + "base64", "core-foundation", + "getrandom 0.4.3", "reqwest", "security-framework-sys", "serde", diff --git a/crates/builderlab-auth/Cargo.toml b/crates/builderlab-auth/Cargo.toml index fafbf7f..e56b1a9 100644 --- a/crates/builderlab-auth/Cargo.toml +++ b/crates/builderlab-auth/Cargo.toml @@ -8,10 +8,12 @@ publish = false [features] default = [] -blocking-client = ["dep:reqwest"] +blocking-client = ["dep:reqwest", "dep:base64", "dep:getrandom"] [dependencies] anyhow = "1" +base64 = { version = "0.22", optional = true } +getrandom = { version = "0.4", optional = true } reqwest = { version = "0.12", optional = true, default-features = false, features = ["blocking", "json", "rustls-tls"] } serde = { version = "1", features = ["derive"] } serde_json = "1" diff --git a/crates/builderlab-auth/src/auth_login.rs b/crates/builderlab-auth/src/auth_login.rs index f50e718..54bbf16 100644 --- a/crates/builderlab-auth/src/auth_login.rs +++ b/crates/builderlab-auth/src/auth_login.rs @@ -1,11 +1,13 @@ use std::time::Duration; use anyhow::{anyhow, Context, Result}; +use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine}; use reqwest::blocking::{Client, ClientBuilder}; use reqwest::header::{ACCEPT, USER_AGENT}; use reqwest::redirect::Policy; use reqwest::StatusCode as HttpStatusCode; use serde::{Deserialize, Serialize}; +use sha2::{Digest, Sha256}; use url::Url; use crate::auth::SESSION_CREDENTIAL_HEADER; @@ -13,6 +15,39 @@ use crate::auth_storage::StoredSessionCredential; pub const CLI_USER_AGENT: &str = "sq-kgoose-bl-auth-login"; +// Kept in memory for one browser login; never persist or log the verifier. +pub struct LoginAttempt { + state: String, + code_verifier: String, +} + +impl LoginAttempt { + pub fn new() -> Result { + Ok(Self { + state: random_token()?, + code_verifier: random_token()?, + }) + } + + pub fn state(&self) -> &str { + &self.state + } + + pub fn code_verifier(&self) -> &str { + &self.code_verifier + } + + fn code_challenge(&self) -> String { + URL_SAFE_NO_PAD.encode(Sha256::digest(self.code_verifier.as_bytes())) + } +} + +fn random_token() -> Result { + let mut bytes = [0; 32]; + getrandom::fill(&mut bytes).map_err(|_| anyhow!("generate secure login randomness"))?; + Ok(URL_SAFE_NO_PAD.encode(bytes)) +} + #[derive(Debug, Deserialize)] pub struct LoginExchangeResponse { pub session_credential: String, @@ -54,9 +89,10 @@ pub struct VerifiedLoginSession { pub me: AuthMeResponse, } -#[derive(Debug, Serialize)] +#[derive(Serialize)] struct LoginExchangeRequest<'a> { code: &'a str, + code_verifier: &'a str, } pub fn build_auth_http_client(timeout: Duration) -> Result { @@ -72,13 +108,17 @@ pub fn exchange_login_code( playpen: Option<&str>, server_url: &str, code: &str, + code_verifier: &str, ) -> Result { let url = auth_url(server_url, "/v1/auth/login/exchange")?; let mut request = client .post(url) .header(USER_AGENT, CLI_USER_AGENT) .header(ACCEPT, "application/json") - .json(&LoginExchangeRequest { code }); + .json(&LoginExchangeRequest { + code, + code_verifier, + }); if let Some(baggage) = playpen_baggage(playpen) { request = request.header("Baggage", baggage); } @@ -96,8 +136,9 @@ pub fn exchange_login_code_and_verify( playpen: Option<&str>, server_url: &str, code: &str, + code_verifier: &str, ) -> Result { - let exchange = exchange_login_code(client, playpen, server_url, code)?; + let exchange = exchange_login_code(client, playpen, server_url, code, code_verifier)?; let credential = StoredSessionCredential { session_credential: exchange.session_credential, expires_at: Some(exchange.expires_at), @@ -173,11 +214,14 @@ pub fn logout_session_credential( Ok(true) } -pub fn login_url(server_url: &str, callback_url: &str) -> Result { +pub fn login_url(server_url: &str, callback_url: &str, attempt: &LoginAttempt) -> Result { let mut url = auth_url(server_url, "/v1/auth/login")?; url.query_pairs_mut() .append_pair("type", "cli") - .append_pair("returnTo", callback_url); + .append_pair("returnTo", callback_url) + .append_pair("state", attempt.state()) + .append_pair("code_challenge", &attempt.code_challenge()) + .append_pair("code_challenge_method", "S256"); Ok(url) } @@ -202,16 +246,15 @@ pub fn playpen_baggage(playpen: Option<&str>) -> Option { #[cfg(test)] mod tests { - use std::io::{BufRead, BufReader, Write}; + use std::io::{BufRead, BufReader, Read, Write}; use std::net::{TcpListener, TcpStream}; - use std::sync::{Arc, Mutex}; use std::thread; use super::*; #[test] fn verify_session_credential_treats_unauthorized_as_invalid() { - let server = SingleResponseServer::start(401, r#"{}"#); + let server = TestServer::single(401, r#"{}"#); let client = build_auth_http_client(Duration::from_secs(5)).expect("client"); let credential = StoredSessionCredential { session_credential: "expired-session".to_string(), @@ -220,7 +263,7 @@ mod tests { let verified = verify_session_credential(&client, None, &server.base_url, &credential) .expect("verify session"); - let request = server.finish(); + let request = server.finish_one(); assert!(verified.is_none()); assert_eq!(request.path, "/v1/auth/me"); @@ -233,7 +276,7 @@ mod tests { #[test] fn verify_session_credential_does_not_echo_failure_body() { let secret = "reflected_session_credential_123456"; - let server = SingleResponseServer::start(500, secret); + let server = TestServer::single(500, secret); let client = build_auth_http_client(Duration::from_secs(5)).expect("client"); let credential = StoredSessionCredential { session_credential: secret.to_string(), @@ -242,7 +285,7 @@ mod tests { let error = verify_session_credential(&client, None, &server.base_url, &credential) .expect_err("reject failed session check"); - let request = server.finish(); + let request = server.finish_one(); let message = format!("{error:#}"); assert!(message.contains("/v1/auth/me failed with 500")); @@ -268,7 +311,7 @@ mod tests { #[test] fn logout_session_credential_posts_logout_and_accepts_success() { - let server = SingleResponseServer::start(200, r#"{}"#); + let server = TestServer::single(200, r#"{}"#); let client = build_auth_http_client(Duration::from_secs(5)).expect("client"); let credential = StoredSessionCredential { session_credential: "valid-session".to_string(), @@ -277,7 +320,7 @@ mod tests { let logged_out = logout_session_credential(&client, None, &server.base_url, &credential) .expect("logout session"); - let request = server.finish(); + let request = server.finish_one(); assert!(logged_out); assert_eq!(request.path, "/v1/auth/logout"); @@ -289,7 +332,7 @@ mod tests { #[test] fn logout_session_credential_treats_unauthorized_as_already_invalid() { - let server = SingleResponseServer::start(401, r#"{}"#); + let server = TestServer::single(401, r#"{}"#); let client = build_auth_http_client(Duration::from_secs(5)).expect("client"); let credential = StoredSessionCredential { session_credential: "expired-session".to_string(), @@ -298,7 +341,7 @@ mod tests { let logged_out = logout_session_credential(&client, None, &server.base_url, &credential) .expect("logout session"); - let request = server.finish(); + let request = server.finish_one(); assert!(!logged_out); assert_eq!(request.path, "/v1/auth/logout"); @@ -310,7 +353,7 @@ mod tests { #[test] fn verify_session_credential_accepts_successful_me_response() { - let server = SingleResponseServer::start( + let server = TestServer::single( 200, r#"{"subject":"auth0|user_123","email":"test@example.com","name":"Test User","expires_at":"2026-06-15T00:00:00Z","roles":["ROLE_USER"],"workspaces":{"active":[{"name":"Test Workspace"},{"name":"Other Workspace"}]}}"#, ); @@ -323,7 +366,7 @@ mod tests { let verified = verify_session_credential(&client, None, &server.base_url, &credential) .expect("verify session") .expect("authenticated"); - let request = server.finish(); + let request = server.finish_one(); assert_eq!(verified.subject.as_deref(), Some("auth0|user_123")); assert_eq!(verified.expires_at.as_deref(), Some("2026-06-15T00:00:00Z")); @@ -339,36 +382,70 @@ mod tests { } #[test] - fn login_url_uses_v1_route() { + fn login_attempts_have_independent_random_state_and_verifiers() { + let first = LoginAttempt::new().expect("first attempt"); + let second = LoginAttempt::new().expect("second attempt"); + for token in [ + first.state(), + first.code_verifier(), + second.state(), + second.code_verifier(), + ] { + assert_eq!(token.len(), 43); + assert_eq!( + URL_SAFE_NO_PAD + .decode(token) + .expect("base64url token") + .len(), + 32 + ); + } + assert_ne!(first.state(), first.code_verifier()); + assert_ne!(first.state(), second.state()); + assert_ne!(first.code_verifier(), second.code_verifier()); + } + + #[test] + fn exchange_failure_does_not_echo_secrets() { + let secret = "reflected-code-and-verifier"; + let server = TestServer::single(400, secret); + let client = build_auth_http_client(Duration::from_secs(5)).expect("client"); + let error = exchange_login_code(&client, None, &server.base_url, secret, secret) + .expect_err("reject failed exchange"); + server.finish_one(); + assert!(error.to_string().contains("400")); + assert!(!format!("{error:#}").contains(secret)); + } + + #[test] + fn login_url_includes_state_and_s256_challenge_without_verifier() { let url = login_url( "https://example.com/cash-app/goose", "http://127.0.0.1:1234/callback", + &LoginAttempt { + state: "client-state".to_string(), + code_verifier: "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk".to_string(), + }, ) .expect("login URL"); assert_eq!(url.path(), "/cash-app/goose/v1/auth/login"); - } - - #[test] - fn exchange_login_code_uses_v1_route() { - let server = SingleResponseServer::start( - 200, - r#"{"session_credential":"session","expires_at":"2026-06-15T00:00:00Z"}"#, + let query: std::collections::HashMap<_, _> = url.query_pairs().collect(); + assert_eq!(query["type"], "cli"); + assert_eq!(query["returnTo"], "http://127.0.0.1:1234/callback"); + assert_eq!(query["state"], "client-state"); + // RFC 7636 Appendix B S256 test vector. + assert_eq!( + query["code_challenge"], + "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" ); - let client = build_auth_http_client(Duration::from_secs(5)).expect("client"); - - let exchange = exchange_login_code(&client, None, &server.base_url, "one-time-code") - .expect("exchange"); - let request = server.finish(); - - assert_eq!(exchange.session_credential, "session"); - assert_eq!(exchange.expires_at, "2026-06-15T00:00:00Z"); - assert_eq!(request.path, "/v1/auth/login/exchange"); + assert_eq!(query["code_challenge_method"], "S256"); + assert!(!query.contains_key("code_verifier")); } #[test] - fn exchange_login_code_and_verify_checks_auth_me() { - let server = SequentialResponseServer::start(vec![ + fn login_exchange_sends_verifier_and_returns_verified_session() { + let server = TestServer::start(vec![ ( 200, r#"{"session_credential":"session","expires_at":"2026-06-15T00:00:00Z"}"#, @@ -380,9 +457,14 @@ mod tests { ]); let client = build_auth_http_client(Duration::from_secs(5)).expect("client"); - let verified = - exchange_login_code_and_verify(&client, None, &server.base_url, "one-time-code") - .expect("verified login"); + let verified = exchange_login_code_and_verify( + &client, + None, + &server.base_url, + "one-time-code", + "test-verifier", + ) + .expect("verified login"); let requests = server.finish(); assert_eq!(verified.credential.session_credential, "session"); @@ -400,6 +482,10 @@ mod tests { ); assert_eq!(requests.len(), 2); assert_eq!(requests[0].path, "/v1/auth/login/exchange"); + assert_eq!( + serde_json::from_str::(&requests[0].body).expect("exchange JSON"), + serde_json::json!({"code": "one-time-code", "code_verifier": "test-verifier"}) + ); assert_eq!(requests[1].path, "/v1/auth/me"); assert_eq!( requests[1].bl_session_credential.as_deref(), @@ -409,7 +495,7 @@ mod tests { #[test] fn exchange_login_code_and_verify_requires_auth_me_success() { - let server = SequentialResponseServer::start(vec![ + let server = TestServer::start(vec![ ( 200, r#"{"session_credential":"session","expires_at":"2026-06-15T00:00:00Z"}"#, @@ -418,9 +504,14 @@ mod tests { ]); let client = build_auth_http_client(Duration::from_secs(5)).expect("client"); - let error = - exchange_login_code_and_verify(&client, None, &server.base_url, "one-time-code") - .expect_err("auth/me rejection fails login"); + let error = exchange_login_code_and_verify( + &client, + None, + &server.base_url, + "one-time-code", + "test-verifier", + ) + .expect_err("auth/me rejection fails login"); let requests = server.finish(); assert!( @@ -435,42 +526,25 @@ mod tests { struct RecordedRequest { path: String, bl_session_credential: Option, + body: String, } - struct SingleResponseServer { + struct TestServer { base_url: String, - handle: thread::JoinHandle, + handle: thread::JoinHandle>, } - impl SingleResponseServer { - fn start(status: u16, body: &'static str) -> Self { - let listener = TcpListener::bind("127.0.0.1:0").expect("bind test server"); - let base_url = format!("http://{}", listener.local_addr().expect("local addr")); - let request = Arc::new(Mutex::new(None)); - let thread_request = Arc::clone(&request); - let handle = thread::spawn(move || { - let (stream, _) = listener.accept().expect("accept request"); - handle_connection(stream, status, body, &thread_request); - thread_request - .lock() - .expect("request mutex") - .take() - .expect("recorded request") - }); - Self { base_url, handle } + impl TestServer { + fn single(status: u16, body: &'static str) -> Self { + Self::start(vec![(status, body)]) } - fn finish(self) -> RecordedRequest { - self.handle.join().expect("join test server") + fn finish_one(self) -> RecordedRequest { + let mut requests = self.finish(); + assert_eq!(requests.len(), 1, "expected a single request"); + requests.pop().expect("recorded request") } - } - struct SequentialResponseServer { - base_url: String, - handle: thread::JoinHandle>, - } - - impl SequentialResponseServer { fn start(responses: Vec<(u16, &'static str)>) -> Self { let listener = TcpListener::bind("127.0.0.1:0").expect("bind test server"); let base_url = format!("http://{}", listener.local_addr().expect("local addr")); @@ -478,15 +552,7 @@ mod tests { let mut requests = Vec::new(); for (status, body) in responses { let (stream, _) = listener.accept().expect("accept request"); - let request = Arc::new(Mutex::new(None)); - handle_connection(stream, status, body, &request); - requests.push( - request - .lock() - .expect("request mutex") - .take() - .expect("recorded request"), - ); + requests.push(handle_connection(stream, status, body)); } requests }); @@ -498,12 +564,7 @@ mod tests { } } - fn handle_connection( - mut stream: TcpStream, - status: u16, - body: &str, - request: &Arc>>, - ) { + fn handle_connection(mut stream: TcpStream, status: u16, body: &str) -> RecordedRequest { let mut reader = BufReader::new(stream.try_clone().expect("clone stream")); let mut request_line = String::new(); reader.read_line(&mut request_line).expect("request line"); @@ -514,6 +575,7 @@ mod tests { .to_string(); let mut bl_session_credential = None; + let mut content_length = 0; loop { let mut line = String::new(); reader.read_line(&mut line).expect("request header"); @@ -521,16 +583,16 @@ mod tests { break; } if let Some((name, value)) = line.split_once(':') { + if name.eq_ignore_ascii_case("content-length") { + content_length = value.trim().parse().expect("content length"); + } if name.eq_ignore_ascii_case(SESSION_CREDENTIAL_HEADER) { bl_session_credential = Some(value.trim().to_string()); } } } - *request.lock().expect("request mutex") = Some(RecordedRequest { - path, - bl_session_credential, - }); - + let mut body_bytes = vec![0; content_length]; + reader.read_exact(&mut body_bytes).expect("request body"); let response = format!( "HTTP/1.1 {status} OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", body.len() @@ -538,5 +600,10 @@ mod tests { stream .write_all(response.as_bytes()) .expect("write response"); + RecordedRequest { + path, + bl_session_credential, + body: String::from_utf8(body_bytes).expect("UTF-8 body"), + } } } diff --git a/docs/bl-auth-flow.md b/docs/bl-auth-flow.md index 2f44ec2..3f5eeb6 100644 --- a/docs/bl-auth-flow.md +++ b/docs/bl-auth-flow.md @@ -10,8 +10,8 @@ sequenceDiagram participant Auth0 participant Store as Backend Data Store - CLI->>CLI: Start localhost callback server - CLI->>Browser: Open /v1/auth/login?type=cli&returnTo=http://127.0.0.1:/callback + CLI->>CLI: Generate random state and PKCE verifier; start localhost callback server + CLI->>Browser: Open /v1/auth/login?type=cli&returnTo=http://127.0.0.1:/callback&state=...&code_challenge=...&code_challenge_method=S256 Browser->>Backend: Begin CLI login Backend->>Store: Store AuthTransaction with type=cli and loopback returnTo Backend-->>Browser: Set oauth state cookie, 302 to Auth0 authorize @@ -21,10 +21,11 @@ sequenceDiagram Backend->>Auth0: Exchange code using stored PKCE verifier Auth0-->>Backend: ID/access token response Backend->>Store: Create short-lived one-time code for CLI login - Backend-->>Browser: 302 to localhost callback with code - Browser->>CLI: GET /callback?code=... - CLI->>Backend: POST /v1/auth/login/exchange { code } - Backend->>Store: Verify code is valid, unused, unexpired, and type=cli + Backend-->>Browser: 302 to localhost callback with code and client state + Browser->>CLI: GET /callback?code=...&state=... + CLI->>CLI: Require exactly one matching state before accepting code or error + CLI->>Backend: POST /v1/auth/login/exchange { code, code_verifier } + Backend->>Store: Verify code is valid, unused, unexpired, type=cli, and matches PKCE challenge Backend->>Store: Create cli session credential Backend-->>CLI: Return cli session credential CLI->>Backend: API request with X-BB-Session-Credential: @@ -37,3 +38,5 @@ Security constraints: - Durable session credentials are never placed in redirect URLs. - The localhost redirect carries only a short-lived, single-use exchange code. - CLI `returnTo` targets must be loopback URLs. +- Each login generates independent 256-bit random state and PKCE verifier values. The CLI sends only the S256 challenge in the login URL and keeps the verifier in memory until exchange. +- Callbacks with missing, mismatched, or duplicate state are rejected without consuming the pending login. diff --git a/docs/bl-auth-local-testing.md b/docs/bl-auth-local-testing.md index c52e88a..0a75bb7 100644 --- a/docs/bl-auth-local-testing.md +++ b/docs/bl-auth-local-testing.md @@ -4,7 +4,7 @@ This covers local CLI auth testing for BuilderLab identity. The important invariant is that the browser must hit `/v1/auth/login` and `/v1/auth/callback` on the same host that Auth0 redirects to. kgoose uses an HttpOnly state cookie between -those two requests before it redirects back to the CLI with a short-lived exchange code. +those two requests before it redirects back to the CLI with a short-lived exchange code and the CLI state. While BuilderLab staging is only reachable from a laptop through a Kubernetes port-forward, keep the backend Auth0 redirect URI as: @@ -61,7 +61,7 @@ Expected result: - kgoose redirects to Auth0 with `redirect_uri=http://localhost:5173/cash-app/goose/v1/auth/callback` - Auth0 redirects the browser back through the same port-forward - kgoose validates the state cookie, exchanges the Auth0 code server-side, and redirects to the CLI loopback callback with a one-time exchange code -- the CLI exchanges that code through kgoose and stores the returned session credential +- the CLI validates the returned state and exchanges that code with its PKCE verifier through kgoose and stores the returned session credential By default, the CLI stores browser auth sessions in the OS keyring. For local debugging without touching keyring state, use the `BL_AUTH_STORAGE=file` command above. diff --git a/src/bl/auth_login.rs b/src/bl/auth_login.rs index a6f8c49..a412fae 100644 --- a/src/bl/auth_login.rs +++ b/src/bl/auth_login.rs @@ -7,7 +7,7 @@ use std::time::Duration; use anyhow::{anyhow, Context, Result}; use builderlab_auth::auth_login::{ build_auth_http_client, exchange_login_code_and_verify, login_url, logout_session_credential, - verify_session_credential, AuthMeResponse, VerifiedLoginSession, + verify_session_credential, AuthMeResponse, LoginAttempt, VerifiedLoginSession, }; use builderlab_auth::auth_storage::StoredSessionCredential; use serde::Serialize; @@ -122,11 +122,13 @@ fn run_browser_login_inner( let server = Server::http("127.0.0.1:0") .map_err(|error| anyhow!("listen on loopback callback port: {error}"))?; let callback_url = format!("http://{}{}", server.server_addr(), CALLBACK_PATH); - let login_url = login_url(&service_url, &callback_url)?; + let attempt = LoginAttempt::new()?; + let login_url = login_url(&service_url, &callback_url, &attempt)?; + let expected_state = attempt.state().to_string(); let (tx, rx) = mpsc::channel(); thread::spawn(move || { - let result = receive_exchange_code(server); + let result = receive_exchange_code(server, &expected_state); let _ = tx.send(result); }); @@ -146,8 +148,13 @@ fn run_browser_login_inner( let code = rx .recv() .context("loopback auth server stopped before login completed")??; - let verified = - exchange_login_code_and_verify(&client, config.playpen.as_deref(), &service_url, &code)?; + let verified = exchange_login_code_and_verify( + &client, + config.playpen.as_deref(), + &service_url, + &code, + attempt.code_verifier(), + )?; let (stored, me, workspace_name) = validate_and_store_login_credential(storage, &storage_key, verified)?; auth_info( @@ -234,7 +241,7 @@ pub fn logout_stored_session( logout_session_credential(&client, config.playpen.as_deref(), &service_url, &stored) } -fn receive_exchange_code(server: Server) -> Result { +fn receive_exchange_code(server: Server, expected_state: &str) -> Result { for request in server.incoming_requests() { let url = request.url().to_string(); let parsed = @@ -248,23 +255,23 @@ fn receive_exchange_code(server: Server) -> Result { continue; } - let error = parsed + let mut states = parsed .query_pairs() - .find(|(key, _)| key == "error") - .map(|(_, value)| value.into_owned()); - if let Some(error) = error { + .filter(|(key, _)| key == "state") + .map(|(_, value)| value); + if states.next().as_deref() != Some(expected_state) || states.next().is_some() { respond_auth_page(request, StatusCode(400), AuthCallbackPage::Failure)?; - return Err(anyhow!("auth callback returned error: {error}")); + continue; } - let code = parsed - .query_pairs() - .find(|(key, _)| key == "code") - .map(|(_, value)| value.into_owned()) - .filter(|value| !value.trim().is_empty()) - .ok_or_else(|| anyhow!("auth callback did not include an exchange code"))?; - respond_auth_page(request, StatusCode(200), AuthCallbackPage::Success)?; - return Ok(code); + let result = callback_exchange_code(&parsed); + let (status, page) = if result.is_ok() { + (StatusCode(200), AuthCallbackPage::Success) + } else { + (StatusCode(400), AuthCallbackPage::Failure) + }; + respond_auth_page(request, status, page)?; + return result; } Err(anyhow!( @@ -272,6 +279,26 @@ fn receive_exchange_code(server: Server) -> Result { )) } +// Only parse the login result after the caller has validated the callback state. +fn callback_exchange_code(url: &Url) -> Result { + if url.query_pairs().any(|(key, _)| key == "error") { + return Err(anyhow!("auth callback returned an authentication error")); + } + + let mut codes = url + .query_pairs() + .filter(|(key, _)| key == "code") + .map(|(_, value)| value); + let code = codes + .next() + .filter(|value| !value.trim().is_empty()) + .ok_or_else(|| anyhow!("auth callback did not include an exchange code"))?; + if codes.next().is_some() { + return Err(anyhow!("auth callback included duplicate exchange codes")); + } + Ok(code.into_owned()) +} + fn respond_text(request: tiny_http::Request, status: StatusCode, body: &str) -> Result<()> { request .respond( @@ -373,10 +400,89 @@ mod tests { }; use super::{ - auth_callback_page, store_login_credential, validate_and_store_login_credential, - verify_stored_session_with, AuthCallbackPage, + auth_callback_page, receive_exchange_code, store_login_credential, + validate_and_store_login_credential, verify_stored_session_with, AuthCallbackPage, }; + fn callback_server() -> (String, std::thread::JoinHandle>) { + let server = tiny_http::Server::http("127.0.0.1:0").expect("callback server"); + let url = format!("http://{}/callback", server.server_addr()); + let handle = std::thread::spawn(move || receive_exchange_code(server, "expected-state")); + (url, handle) + } + + #[test] + fn invalid_state_cannot_complete_or_cancel_pending_login() { + let (url, handle) = callback_server(); + let client = + builderlab_auth::auth_login::build_auth_http_client(std::time::Duration::from_secs(5)) + .expect("client"); + for query in [ + "code=attacker-code", + "code=attacker-code&state=", + "code=attacker-code&state=wrong", + "error=denied&state=wrong", + "code=attacker-code&state=expected-state&state=wrong", + "code=attacker-code&state=expected-state&state=expected-state", + ] { + let response = client + .get(format!("{url}?{query}")) + .send() + .expect("callback"); + assert_eq!(response.status().as_u16(), 400, "callback query: {query}"); + assert!(response + .text() + .expect("failure page") + .contains("Sign-in didn’t finish")); + assert!(!handle.is_finished()); + } + let response = client + .get(format!("{url}?code=valid-code&state=expected-state")) + .send() + .expect("valid callback"); + assert_eq!(response.status().as_u16(), 200); + assert_eq!( + handle.join().expect("callback thread").expect("code"), + "valid-code" + ); + } + + #[test] + fn matching_state_rejects_malformed_codes_and_provider_errors() { + let client = + builderlab_auth::auth_login::build_auth_http_client(std::time::Duration::from_secs(5)) + .expect("client"); + for (query, expected_error) in [ + ("", "auth callback did not include an exchange code"), + ("code=", "auth callback did not include an exchange code"), + ( + "code=one&code=two", + "auth callback included duplicate exchange codes", + ), + ( + "error=private-provider-error&code=one", + "auth callback returned an authentication error", + ), + ] { + let (url, handle) = callback_server(); + let response = client + .get(format!("{url}?state=expected-state&{query}")) + .send() + .expect("invalid callback"); + assert_eq!(response.status().as_u16(), 400, "callback query: {query}"); + let error = handle + .join() + .expect("callback thread") + .expect_err("invalid callback"); + // The full error must describe the failure without reflecting provider details. + assert_eq!( + format!("{error:#}"), + expected_error, + "callback query: {query}" + ); + } + } + struct SwappingStorage { reads: RefCell>, read_count: Cell,