fix(openai): accept standard OAuth callback parameters

Accept standard OAuth callback parameters while keeping one-shot flow and duplicate security parameter rejection.

Classify unavailable identity verification correctly and add redacted OAuth/import failure diagnostics.

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
zyycn
2026-07-27 22:39:01 +08:00
co-authored by Codex
parent 9758c420fa
commit 6901e0ed45
3 changed files with 214 additions and 6 deletions
+73 -1
View File
@@ -250,11 +250,22 @@ impl ProviderAdmin for OpenAiAdminProvider {
command.document.into_provider_data().into_inner(),
))
.await
.inspect_err(|error| {
log_import_failure("prepare_document", credential_admin_error_code(*error));
})
.map_err(map_credential_admin_error)?;
let observed_at = Utc::now();
let mut credentials = Vec::with_capacity(prepared.accounts().len());
for account in prepared.into_accounts() {
let account = self.preserve_existing_installation_id(account).await?;
let account = self
.preserve_existing_installation_id(account)
.await
.inspect_err(|error| {
log_import_failure(
"preserve_installation_id",
provider_admin_error_code(error.kind()),
);
})?;
credentials.push(prepared_create(account, observed_at)?);
}
Ok(PreparedCredentialImport {
@@ -286,6 +297,7 @@ impl ProviderAdmin for OpenAiAdminProvider {
&self,
command: CompleteAuthorization,
) -> Result<PreparedAuthorizationCommit, ProviderAdminError> {
let request_id = command.context.request_id.clone();
let binding = AuthorizationOwnerBinding::from_context(&command.context);
let completed = self
.oauth
@@ -295,6 +307,14 @@ impl ProviderAdmin for OpenAiAdminProvider {
callback_url: SecretString::from(command.callback_url),
})
.await
.inspect_err(|error| {
tracing::warn!(
request_id = %request_id,
oauth_stage = "complete_authorization",
oauth_error = oauth_error_code(error),
"OpenAI OAuth authorization completion failed"
);
})
.map_err(map_oauth_error)?;
let credential = match completed.credential {
CompletedCodexOAuthCredential::Create(credential) => {
@@ -1211,6 +1231,44 @@ fn map_credential_admin_error(error: CodexCredentialAdminError) -> ProviderAdmin
})
}
const fn credential_admin_error_code(error: CodexCredentialAdminError) -> &'static str {
match error {
CodexCredentialAdminError::InvalidInput => "invalid_input",
CodexCredentialAdminError::IdentityMismatch => "identity_mismatch",
CodexCredentialAdminError::InvalidCredential => "invalid_credential",
CodexCredentialAdminError::NotFound => "not_found",
CodexCredentialAdminError::RevisionConflict => "revision_conflict",
CodexCredentialAdminError::StoreUnavailable => "store_unavailable",
CodexCredentialAdminError::MissingRefreshToken => "missing_refresh_token",
CodexCredentialAdminError::RefreshLeaseUnavailable => "refresh_lease_unavailable",
CodexCredentialAdminError::RefreshRejected => "refresh_rejected",
CodexCredentialAdminError::AccountBanned => "account_banned",
CodexCredentialAdminError::RefreshUnavailable => "refresh_unavailable",
CodexCredentialAdminError::RefreshAmbiguous => "refresh_ambiguous",
CodexCredentialAdminError::IdentityRejected => "identity_rejected",
CodexCredentialAdminError::IdentityUnavailable => "identity_unavailable",
}
}
const fn provider_admin_error_code(kind: ProviderAdminErrorKind) -> &'static str {
match kind {
ProviderAdminErrorKind::Invalid => "invalid",
ProviderAdminErrorKind::Unsupported => "unsupported",
ProviderAdminErrorKind::NotFound => "not_found",
ProviderAdminErrorKind::Conflict => "conflict",
ProviderAdminErrorKind::Unavailable => "unavailable",
ProviderAdminErrorKind::Internal => "internal",
}
}
fn log_import_failure(stage: &'static str, error: &'static str) {
tracing::warn!(
import_stage = stage,
import_error = error,
"OpenAI credential import preparation failed"
);
}
fn map_oauth_error(error: CodexOAuthAdminError) -> ProviderAdminError {
use CodexOAuthAdminError as Error;
provider_admin_error(match error {
@@ -1225,6 +1283,20 @@ fn map_oauth_error(error: CodexOAuthAdminError) -> ProviderAdminError {
})
}
const fn oauth_error_code(error: &CodexOAuthAdminError) -> &'static str {
match error {
CodexOAuthAdminError::InvalidInput => "invalid_input",
CodexOAuthAdminError::NotFound => "not_found",
CodexOAuthAdminError::Conflict => "conflict",
CodexOAuthAdminError::FlowExpired => "flow_expired",
CodexOAuthAdminError::UpstreamRejected => "upstream_rejected",
CodexOAuthAdminError::UpstreamUnavailable => "upstream_unavailable",
CodexOAuthAdminError::Ambiguous => "ambiguous",
CodexOAuthAdminError::StorageUnavailable => "storage_unavailable",
CodexOAuthAdminError::Credential => "credential",
}
}
fn map_quota_error(error: CodexCredentialQuotaError) -> ProviderAdminError {
use CodexCredentialQuotaError as Error;
provider_admin_error(match error {
@@ -635,9 +635,19 @@ fn callback_parts(value: &str) -> Result<(SecretString, SecretString), CodexOAut
let mut state = None;
for (key, value) in url.query_pairs() {
match key.as_ref() {
"code" if code.is_none() => code = Some(value.into_owned()),
"state" if state.is_none() => state = Some(value.into_owned()),
_ => return Err(CodexOAuthAdminError::UpstreamRejected),
"code" => {
if code.replace(value.into_owned()).is_some() {
return Err(CodexOAuthAdminError::UpstreamRejected);
}
}
"state" => {
if state.replace(value.into_owned()).is_some() {
return Err(CodexOAuthAdminError::UpstreamRejected);
}
}
// OAuth 服务可能附加 `scope`、`iss` 等标准参数。
// 固定回调地址与唯一的 `code`/`state` 才是安全边界,无关参数不参与校验。
_ => {}
}
}
let code = code.filter(|value| valid_secret(value));
@@ -718,8 +728,11 @@ fn map_exchange_error(error: AuthorizationCodeExchangeError) -> CodexOAuthAdminE
}
}
fn map_identity_error(_: CodexIdentityVerificationError) -> CodexOAuthAdminError {
CodexOAuthAdminError::UpstreamRejected
fn map_identity_error(error: CodexIdentityVerificationError) -> CodexOAuthAdminError {
match error {
CodexIdentityVerificationError::Rejected => CodexOAuthAdminError::UpstreamRejected,
CodexIdentityVerificationError::Unavailable => CodexOAuthAdminError::UpstreamUnavailable,
}
}
fn map_admin_error(_: CodexCredentialAdminError) -> CodexOAuthAdminError {
@@ -265,6 +265,129 @@ async fn completion_returns_verified_core_account_without_writing_store() {
);
}
#[tokio::test]
async fn completion_accepts_unrelated_oauth_callback_parameters() {
let service = service();
let started = started(&service).await;
let state = Url::parse(&started.authorization_url)
.expect("authorization URL")
.query_pairs()
.find_map(|(key, value)| (key == "state").then(|| value.into_owned()))
.expect("state");
let prepared = service
.complete_authorization(CompleteCodexOAuthAuthorization {
owner_ref: owner_ref(),
flow_id: started.flow_id,
callback_url: SecretString::from(format!(
"http://localhost:1455/auth/callback?code=authorization-code&state={state}&scope=openid%20profile&iss=https%3A%2F%2Fauth.openai.com"
)),
})
.await
.expect("complete OAuth with standard callback parameters");
assert!(matches!(
prepared.credential,
CompletedCodexOAuthCredential::Create(_)
));
}
#[tokio::test]
async fn completion_rejects_repeated_code_callback_parameter() {
let service = service();
let started = started(&service).await;
let state = Url::parse(&started.authorization_url)
.expect("authorization URL")
.query_pairs()
.find_map(|(key, value)| (key == "state").then(|| value.into_owned()))
.expect("state");
let error = service
.complete_authorization(CompleteCodexOAuthAuthorization {
owner_ref: owner_ref(),
flow_id: started.flow_id,
callback_url: SecretString::from(format!(
"http://localhost:1455/auth/callback?code=authorization-code&code=another-code&state={state}"
)),
})
.await
.expect_err("repeated code");
assert_eq!(error, CodexOAuthAdminError::UpstreamRejected);
}
#[tokio::test]
async fn completion_rejects_repeated_state_callback_parameter() {
let service = service();
let started = started(&service).await;
let state = Url::parse(&started.authorization_url)
.expect("authorization URL")
.query_pairs()
.find_map(|(key, value)| (key == "state").then(|| value.into_owned()))
.expect("state");
let error = service
.complete_authorization(CompleteCodexOAuthAuthorization {
owner_ref: owner_ref(),
flow_id: started.flow_id,
callback_url: SecretString::from(format!(
"http://localhost:1455/auth/callback?code=authorization-code&state={state}&state=another-state-value"
)),
})
.await
.expect_err("repeated state");
assert_eq!(error, CodexOAuthAdminError::UpstreamRejected);
}
struct UnavailableVerifier;
#[async_trait]
impl CodexAccountIdentityVerifier for UnavailableVerifier {
async fn verify(
&self,
_: &CodexOAuthSecret,
_: &CodexIdentityExpectation,
) -> Result<CodexIdentityVerification, CodexIdentityVerificationError> {
Err(CodexIdentityVerificationError::Unavailable)
}
async fn verify_authorization(
&self,
_: &CodexOAuthSecret,
_: &SecretString,
_: &SecretString,
_: &CodexIdentityExpectation,
) -> Result<CodexIdentityVerification, CodexIdentityVerificationError> {
Err(CodexIdentityVerificationError::Unavailable)
}
}
#[tokio::test]
async fn completion_classifies_unavailable_identity_verification_as_upstream_unavailable() {
let store = Arc::new(MemoryAccountStore::default());
let service = service_with_store(store, Arc::new(UnavailableVerifier));
let started = started(&service).await;
let state = Url::parse(&started.authorization_url)
.expect("authorization URL")
.query_pairs()
.find_map(|(key, value)| (key == "state").then(|| value.into_owned()))
.expect("state");
let error = service
.complete_authorization(CompleteCodexOAuthAuthorization {
owner_ref: owner_ref(),
flow_id: started.flow_id,
callback_url: SecretString::from(format!(
"http://localhost:1455/auth/callback?code=authorization-code&state={state}"
)),
})
.await
.expect_err("identity verification unavailable");
assert_eq!(error, CodexOAuthAdminError::UpstreamUnavailable);
}
#[tokio::test]
async fn callback_state_mismatch_fails_closed_and_consumes_flow() {
let service = service();