mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Expose onboarding hints in login completion notifications (#36635)
## What changed - Accept the allowlisted `.onboarding_entrypoint=life_sciences` suffix on a valid OAuth state while continuing to reject unknown or malformed suffixes. - Return the parsed callback metadata from the login server without including the suffix in the token exchange redirect URI. - Add the optional `onboardingEntrypoint` field and `life_sciences` value to the v2 `account/login/completed` notification and generated protocol exports. ## Testing - Cover valid, invalid, and repeated OAuth state suffixes. - Verify the end-to-end login callback result, token redirect URI, and app-server notification payload. GitOrigin-RevId: c2ee146328ddd43c03a503260d5d492e186fa889
This commit is contained in:
1 parent
9949245d1d
commit
5157493c23
20 files changed
+292
-42
No files matched your search
@@ -0,0 +1,29 @@
|
||||
const LIFE_SCIENCES_OAUTH_STATE_SUFFIX: &str = ".onboarding_entrypoint=life_sciences";
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
pub enum LoginOnboardingEntrypoint {
|
||||
LifeSciences,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
|
||||
pub struct LoginCallbackResult {
|
||||
pub onboarding_entrypoint: Option<LoginOnboardingEntrypoint>,
|
||||
}
|
||||
|
||||
pub(crate) fn login_callback_result_from_state(
|
||||
callback_state: &str,
|
||||
expected_state: &str,
|
||||
) -> Option<LoginCallbackResult> {
|
||||
if callback_state == expected_state {
|
||||
return Some(LoginCallbackResult::default());
|
||||
}
|
||||
|
||||
(callback_state.strip_suffix(LIFE_SCIENCES_OAUTH_STATE_SUFFIX) == Some(expected_state))
|
||||
.then_some(LoginCallbackResult {
|
||||
onboarding_entrypoint: Some(LoginOnboardingEntrypoint::LifeSciences),
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "callback_params_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,49 @@
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn accepts_the_original_oauth_state() {
|
||||
assert_eq!(
|
||||
login_callback_result_from_state("expected-state", "expected-state"),
|
||||
Some(LoginCallbackResult::default())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn accepts_the_allowlisted_life_sciences_suffix() {
|
||||
assert_eq!(
|
||||
login_callback_result_from_state(
|
||||
"expected-state.onboarding_entrypoint=life_sciences",
|
||||
"expected-state",
|
||||
),
|
||||
Some(LoginCallbackResult {
|
||||
onboarding_entrypoint: Some(LoginOnboardingEntrypoint::LifeSciences),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_a_suffix_when_the_nonce_does_not_match() {
|
||||
assert_eq!(
|
||||
login_callback_result_from_state(
|
||||
"different-state.onboarding_entrypoint=life_sciences",
|
||||
"expected-state",
|
||||
),
|
||||
None
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_unrecognized_or_repeated_suffixes() {
|
||||
for callback_state in [
|
||||
"expected-state.onboarding_entrypoint=unknown",
|
||||
"expected-state.onboarding_entrypoint=life_sciences.onboarding_entrypoint=life_sciences",
|
||||
"expected-state.extra=value.onboarding_entrypoint=life_sciences",
|
||||
] {
|
||||
assert_eq!(
|
||||
login_callback_result_from_state(callback_state, "expected-state"),
|
||||
None,
|
||||
"unexpectedly accepted {callback_state}",
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -3,12 +3,15 @@ pub mod auth_env_telemetry;
|
||||
pub mod test_support;
|
||||
pub mod token_data;
|
||||
|
||||
mod callback_params;
|
||||
mod device_code_auth;
|
||||
mod outbound_proxy;
|
||||
mod pkce;
|
||||
mod server;
|
||||
mod success_page;
|
||||
|
||||
pub use callback_params::LoginCallbackResult;
|
||||
pub use callback_params::LoginOnboardingEntrypoint;
|
||||
pub use codex_config::types::AuthCredentialsStoreMode;
|
||||
pub use codex_http_client::BuildCustomCaTransportError as BuildLoginHttpClientError;
|
||||
pub use device_code_auth::DeviceCode;
|
||||
|
||||
@@ -27,6 +27,8 @@ use std::time::Duration;
|
||||
use crate::auth::AuthDotJson;
|
||||
use crate::auth::AuthKeyringBackendKind;
|
||||
use crate::auth::save_auth;
|
||||
use crate::callback_params::LoginCallbackResult;
|
||||
use crate::callback_params::login_callback_result_from_state;
|
||||
use crate::default_client::create_raw_auth_client;
|
||||
use crate::default_client::originator;
|
||||
use crate::outbound_proxy::AuthRouteConfig;
|
||||
@@ -111,13 +113,20 @@ impl ServerOptions {
|
||||
pub struct LoginServer {
|
||||
pub auth_url: String,
|
||||
pub actual_port: u16,
|
||||
server_handle: tokio::task::JoinHandle<io::Result<()>>,
|
||||
server_handle: tokio::task::JoinHandle<io::Result<LoginCallbackResult>>,
|
||||
shutdown_handle: ShutdownHandle,
|
||||
}
|
||||
|
||||
impl LoginServer {
|
||||
/// Waits for the login callback loop to finish.
|
||||
pub async fn block_until_done(self) -> io::Result<()> {
|
||||
self.block_until_done_with_callback_result()
|
||||
.await
|
||||
.map(|_| ())
|
||||
}
|
||||
|
||||
/// Waits for login to finish and returns allowlisted callback metadata.
|
||||
pub async fn block_until_done_with_callback_result(self) -> io::Result<LoginCallbackResult> {
|
||||
self.server_handle
|
||||
.await
|
||||
.map_err(|err| io::Error::other(format!("login server thread panicked: {err:?}")))?
|
||||
@@ -201,6 +210,7 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result<LoginServer> {
|
||||
let shutdown_notify = shutdown_notify.clone();
|
||||
let server = server;
|
||||
tokio::spawn(async move {
|
||||
let mut callback_result = LoginCallbackResult::default();
|
||||
let result = loop {
|
||||
tokio::select! {
|
||||
_ = shutdown_notify.notified() => {
|
||||
@@ -228,7 +238,8 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result<LoginServer> {
|
||||
let _ = tokio::task::spawn_blocking(move || req.respond(response)).await;
|
||||
None
|
||||
}
|
||||
HandledRequest::RedirectWithHeader(header) => {
|
||||
HandledRequest::RedirectWithHeader { header, result } => {
|
||||
callback_result = result;
|
||||
let redirect = Response::empty(302).with_header(header);
|
||||
let _ = tokio::task::spawn_blocking(move || req.respond(redirect)).await;
|
||||
None
|
||||
@@ -247,9 +258,9 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result<LoginServer> {
|
||||
)
|
||||
})
|
||||
.await;
|
||||
Some(result)
|
||||
Some(result.map(|()| callback_result))
|
||||
}
|
||||
HandledRequest::RedirectAndExit(header) => {
|
||||
HandledRequest::RedirectAndExit { header, result } => {
|
||||
match tokio::task::spawn_blocking(move || {
|
||||
send_response_with_disconnect(
|
||||
req,
|
||||
@@ -268,7 +279,7 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result<LoginServer> {
|
||||
warn!("hosted login redirect task failed: {err}");
|
||||
}
|
||||
}
|
||||
Some(Ok(()))
|
||||
Some(Ok(result))
|
||||
}
|
||||
};
|
||||
|
||||
@@ -297,8 +308,14 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result<LoginServer> {
|
||||
/// Internal callback handling outcome.
|
||||
enum HandledRequest {
|
||||
Response(Response<Cursor<Vec<u8>>>),
|
||||
RedirectWithHeader(Header),
|
||||
RedirectAndExit(Header),
|
||||
RedirectWithHeader {
|
||||
header: Header,
|
||||
result: LoginCallbackResult,
|
||||
},
|
||||
RedirectAndExit {
|
||||
header: Header,
|
||||
result: LoginCallbackResult,
|
||||
},
|
||||
ResponseAndExit {
|
||||
headers: Vec<Header>,
|
||||
body: Vec<u8>,
|
||||
@@ -332,7 +349,10 @@ async fn process_request(
|
||||
let has_code = params.get("code").is_some_and(|code| !code.is_empty());
|
||||
let has_state = params.get("state").is_some_and(|state| !state.is_empty());
|
||||
let has_error = params.get("error").is_some_and(|error| !error.is_empty());
|
||||
let state_valid = params.get("state").map(String::as_str) == Some(state);
|
||||
let callback_result = params
|
||||
.get("state")
|
||||
.and_then(|callback_state| login_callback_result_from_state(callback_state, state));
|
||||
let state_valid = callback_result.is_some();
|
||||
info!(
|
||||
path = %path,
|
||||
has_code,
|
||||
@@ -380,6 +400,7 @@ async fn process_request(
|
||||
);
|
||||
}
|
||||
};
|
||||
let callback_result = callback_result.unwrap_or_default();
|
||||
|
||||
match exchange_code_for_tokens(
|
||||
&opts.issuer,
|
||||
@@ -446,12 +467,14 @@ async fn process_request(
|
||||
};
|
||||
match tiny_http::Header::from_bytes(&b"Location"[..], url.as_bytes()) {
|
||||
Ok(header) => match redirect {
|
||||
LoginSuccessRedirect::Local(_) => {
|
||||
HandledRequest::RedirectWithHeader(header)
|
||||
}
|
||||
LoginSuccessRedirect::Hosted(_) => {
|
||||
HandledRequest::RedirectAndExit(header)
|
||||
}
|
||||
LoginSuccessRedirect::Local(_) => HandledRequest::RedirectWithHeader {
|
||||
header,
|
||||
result: callback_result,
|
||||
},
|
||||
LoginSuccessRedirect::Hosted(_) => HandledRequest::RedirectAndExit {
|
||||
header,
|
||||
result: callback_result,
|
||||
},
|
||||
},
|
||||
Err(_) => login_error_response(
|
||||
"Sign-in completed but redirecting back to Codex failed.",
|
||||
|
||||
@@ -11,6 +11,8 @@ use base64::Engine;
|
||||
use codex_config::types::AuthCredentialsStoreMode;
|
||||
use codex_http_client::HttpClientBuilder;
|
||||
use codex_login::AuthKeyringBackendKind;
|
||||
use codex_login::LoginCallbackResult;
|
||||
use codex_login::LoginOnboardingEntrypoint;
|
||||
use codex_login::LoginSuccessPage;
|
||||
use codex_login::LoginSuccessPageBrand;
|
||||
use codex_login::ServerOptions;
|
||||
@@ -149,7 +151,9 @@ async fn end_to_end_login_flow_persists_auth_json() -> Result<()> {
|
||||
let client = HttpClientBuilder::new()
|
||||
.without_redirects()
|
||||
.build_direct()?;
|
||||
let url = format!("http://127.0.0.1:{login_port}/auth/callback?code=abc&state=test_state_123");
|
||||
let url = format!(
|
||||
"http://127.0.0.1:{login_port}/auth/callback?code=abc&state=test_state_123.onboarding_entrypoint=life_sciences"
|
||||
);
|
||||
let resp = client.get(&url).send().await?;
|
||||
assert_eq!(resp.status(), 302);
|
||||
let success_url = resp.headers()["location"].to_str()?;
|
||||
@@ -161,7 +165,13 @@ async fn end_to_end_login_flow_persists_auth_json() -> Result<()> {
|
||||
assert!(success_resp.status().is_success());
|
||||
|
||||
// Wait for server shutdown
|
||||
server.block_until_done().await?;
|
||||
let callback_result = server.block_until_done_with_callback_result().await?;
|
||||
assert_eq!(
|
||||
callback_result,
|
||||
LoginCallbackResult {
|
||||
onboarding_entrypoint: Some(LoginOnboardingEntrypoint::LifeSciences),
|
||||
}
|
||||
);
|
||||
|
||||
// Validate auth.json
|
||||
let auth_path = codex_home.join("auth.json");
|
||||
|
||||
Reference in new issue
Block a user