From 8b65488a89e58bb3cbf4a013d776dfaa7d30b4b7 Mon Sep 17 00:00:00 2001 From: Jason Date: Thu, 16 Apr 2026 16:55:11 +0800 Subject: [PATCH] fix(proxy/gemini): narrow URL normalization + guard empty OAuth access_token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2a — Preserve opaque relay URLs that contain `/v1/models/` prefixes. `should_normalize_gemini_full_url` previously flagged any full URL whose path merely contained `/v1beta/models/` or `/v1/models/` as a structured Gemini endpoint, forcing rewrite to `.../v1beta/models/{model}:method`. This silently dropped legitimate relay route segments (e.g. `https://relay.example/v1/models/invoke` → `.../v1beta/models/...:generateContent`, losing `/invoke`) and sent traffic to the wrong upstream path. Replace the bare `contains(...)` checks with `matches_structured_gemini_models_path`, which requires the `/models/` segment to be followed by a canonical Gemini method call (`*:generateContent` or `*:streamGenerateContent`). The `matches_bare_gemini_models_path` helper is generalized (and renamed) to handle both `/v1beta/models/` and `/v1/models/` alongside the original bare `/models/` shape. P2b — Reject empty Gemini OAuth access_tokens before they reach the bearer header. `GeminiAdapter::parse_oauth_credentials` accepts refresh-token-only JSON (and surfaces `{"access_token": "", ...}` for expired credentials) with `access_token` defaulting to `""`. The Claude adapter's GeminiCli branch then called `AuthInfo::with_access_token(key, creds.access_token)` unconditionally, so the bearer-header builder at `AuthStrategy::GoogleOAuth` resolved to `Authorization: Bearer ` — a deterministic 401 from upstream. CC Switch does not currently exchange the refresh_token for a fresh access_token (`OAuthCredentials::needs_refresh` / `can_refresh` are annotated `#[allow(dead_code)]`). Until that exists, only attach `access_token` when it is non-empty; fall back to plain GoogleOAuth strategy with the raw key and log a warn pointing users at `~/.gemini/oauth_creds.json` so the failure mode is observable. Tests: - gemini_url.rs: three new regressions — opaque `/v1/models/invoke`, opaque `/v1beta/models/route`, and the positive counter-case where a structured `/v1/models/...:generateContent` path still normalizes. - claude.rs: three new `test_extract_auth_gemini_cli_*` tests covering refresh-only JSON, empty-string access_token JSON, and the valid-JSON pass-through. All 839 lib tests pass; cargo fmt + clippy -D warnings clean. Addresses Codex review P2 findings on #1918. --- src-tauri/src/proxy/gemini_url.rs | 83 ++++++++++++++--- src-tauri/src/proxy/providers/claude.rs | 118 ++++++++++++++++++++++-- 2 files changed, 184 insertions(+), 17 deletions(-) diff --git a/src-tauri/src/proxy/gemini_url.rs b/src-tauri/src/proxy/gemini_url.rs index f84fe095d..2c0407a46 100644 --- a/src-tauri/src/proxy/gemini_url.rs +++ b/src-tauri/src/proxy/gemini_url.rs @@ -63,9 +63,14 @@ fn should_normalize_gemini_full_url(base_url: &str) -> bool { } let path = path.trim_end_matches('/'); - path.contains("/v1beta/models/") - || path.contains("/v1/models/") - || matches_bare_gemini_models_path(path) + // Only classify a path as a "structured Gemini endpoint" — safe to + // rewrite to /v1beta/models/{model}:method — when the `/models/` + // segment is actually followed by a Gemini `*:generateContent` / + // `*:streamGenerateContent` method call. Bare `path.contains("/v1/models/")` + // would also match legitimate opaque relay routes like + // `/v1/models/invoke`, which are *not* Gemini-structured and must be + // preserved as-is. + matches_structured_gemini_models_path(path) || path.ends_with("/v1beta") || path.ends_with("/v1") || path.ends_with("/v1beta/models") @@ -80,8 +85,6 @@ fn should_normalize_gemini_full_url(base_url: &str) -> bool { || path.ends_with("/v1beta/openai/responses") || path.ends_with("/v1/openai/responses") || path.ends_with("/openai/responses") - || path.contains(":generateContent") - || path.contains(":streamGenerateContent") } fn split_query(input: &str) -> (&str, Option<&str>) { @@ -150,13 +153,25 @@ fn normalize_prefix(prefix: &str) -> String { } } -fn matches_bare_gemini_models_path(path: &str) -> bool { - if let Some(idx) = path.find("/models/") { - let after = &path[idx + "/models/".len()..]; - after.contains(":generateContent") || after.contains(":streamGenerateContent") - } else { - false +/// Returns true when `path` contains a `/models/` segment followed by a +/// canonical Gemini method call (`*:generateContent` or +/// `*:streamGenerateContent`). The `/models/` segment alone is not enough: +/// opaque relay routes such as `/v1/models/invoke` or `/custom/models/foo` +/// also contain `/models/` but are not Gemini-structured and must not be +/// rewritten. +fn matches_structured_gemini_models_path(path: &str) -> bool { + let mut cursor = path; + while let Some(idx) = cursor.find("/models/") { + let after = &cursor[idx + "/models/".len()..]; + if after.contains(":generateContent") || after.contains(":streamGenerateContent") { + return true; + } + // Advance past this `/models/` occurrence so a later Gemini-style + // segment in the same path (unusual but cheap to handle) can still + // match. + cursor = &cursor[idx + "/models/".len()..]; } + false } fn merge_queries(base_query: Option<&str>, endpoint_query: Option<&str>) -> Option { @@ -265,4 +280,50 @@ mod tests { assert_eq!(url, "https://relay.example/custom/models/invoke?alt=sse"); } + + /// Regression: a relay whose fixed path starts with `/v1/models/` is an + /// opaque route, not a Gemini-structured endpoint. The previous + /// heuristic matched any `contains("/v1/models/")` and rewrote it to + /// `/v1beta/models/{model}:generateContent`, dropping the relay's own + /// route component (`/invoke`) and sending traffic to the wrong place. + #[test] + fn preserves_opaque_full_url_with_v1_models_prefix() { + let url = resolve_gemini_native_url( + "https://relay.example/v1/models/invoke", + "/v1beta/models/gemini-2.5-flash:streamGenerateContent?alt=sse", + true, + ); + + assert_eq!(url, "https://relay.example/v1/models/invoke?alt=sse"); + } + + /// Same regression, `/v1beta/models/` variant — relays may use Google's + /// path layout defensively for routing while still being opaque. + #[test] + fn preserves_opaque_full_url_with_v1beta_models_prefix() { + let url = resolve_gemini_native_url( + "https://relay.example/v1beta/models/route", + "/v1beta/models/gemini-2.5-flash:streamGenerateContent?alt=sse", + true, + ); + + assert_eq!(url, "https://relay.example/v1beta/models/route?alt=sse"); + } + + /// Counter-case: a full URL that *does* carry a genuine Gemini method + /// segment (`:generateContent`) under `/v1/models/` should still be + /// recognized as structured and normalized to the requested model. + #[test] + fn normalizes_structured_v1_models_path_with_method_segment() { + let url = resolve_gemini_native_url( + "https://relay.example/v1/models/gemini-2.5-pro:generateContent", + "/v1beta/models/gemini-2.5-flash:streamGenerateContent?alt=sse", + true, + ); + + assert_eq!( + url, + "https://relay.example/v1beta/models/gemini-2.5-flash:streamGenerateContent?alt=sse" + ); + } } diff --git a/src-tauri/src/proxy/providers/claude.rs b/src-tauri/src/proxy/providers/claude.rs index 18f85bfb0..5fd49ddc0 100644 --- a/src-tauri/src/proxy/providers/claude.rs +++ b/src-tauri/src/proxy/providers/claude.rs @@ -427,12 +427,32 @@ impl ProviderAdapter for ClaudeAdapter { match provider_type { ProviderType::GeminiCli => { - if let Some(creds) = - super::gemini::GeminiAdapter::new().parse_oauth_credentials(&key) - { - Some(AuthInfo::with_access_token(key, creds.access_token)) - } else { - Some(AuthInfo::new(key, AuthStrategy::GoogleOAuth)) + // Parse stored OAuth JSON and only attach access_token when + // it's actually usable. `parse_oauth_credentials` accepts + // refresh-token-only JSON (which is legitimate before the + // first refresh) and also surfaces `{"access_token": "", ...}` + // for expired credentials. In both cases we would otherwise + // send `Authorization: Bearer ` to upstream and get a 401. + // + // CC Switch does not currently exchange the refresh_token for + // a fresh access_token. Until that path exists, degrade to + // plain GoogleOAuth strategy (which still sends the raw key + // as a fallback) and log loudly so users know to refresh + // their `~/.gemini/oauth_creds.json`. + match super::gemini::GeminiAdapter::new().parse_oauth_credentials(&key) { + Some(creds) if !creds.access_token.is_empty() => { + Some(AuthInfo::with_access_token(key, creds.access_token)) + } + Some(_) => { + log::warn!( + "[Gemini OAuth] access_token missing or empty for provider `{}`; \ + bearer auth will likely fail with 401. Refresh \ + ~/.gemini/oauth_creds.json via the gemini CLI to obtain a new token.", + provider.id + ); + Some(AuthInfo::new(key, AuthStrategy::GoogleOAuth)) + } + None => Some(AuthInfo::new(key, AuthStrategy::GoogleOAuth)), } } ProviderType::Gemini => Some(AuthInfo::new(key, AuthStrategy::Google)), @@ -752,6 +772,92 @@ mod tests { assert_eq!(auth.strategy, AuthStrategy::ClaudeAuth); } + /// Regression: a Gemini OAuth credential JSON that carries only a + /// refresh_token (no active access_token) must not be surfaced as an + /// `AuthInfo` whose bearer would be empty. Without the guard, downstream + /// header injection produces `Authorization: Bearer ` and a deterministic + /// 401 from upstream. + #[test] + fn test_extract_auth_gemini_cli_refresh_only_json_does_not_expose_empty_bearer() { + let adapter = ClaudeAdapter::new(); + let refresh_only_json = + r#"{"refresh_token":"rt-abc","client_id":"cid","client_secret":"cs"}"#; + let provider = create_provider_with_meta( + json!({ + "env": { + "ANTHROPIC_BASE_URL": "https://generativelanguage.googleapis.com", + "ANTHROPIC_API_KEY": refresh_only_json + } + }), + ProviderMeta { + api_format: Some("gemini_native".to_string()), + ..Default::default() + }, + ); + + let auth = adapter.extract_auth(&provider).unwrap(); + // access_token must not be surfaced as `Some("")` — the OAuth header + // builder uses `access_token.as_ref().unwrap_or(&api_key)`, so a + // `Some("")` would win over the raw key and emit `Bearer `. + assert!( + auth.access_token.as_deref().is_none_or(|t| !t.is_empty()), + "empty access_token leaked into AuthInfo" + ); + assert_eq!(auth.strategy, AuthStrategy::GoogleOAuth); + } + + /// Companion case: a JSON credential with an empty-string `access_token` + /// field (the shape an expired credential can take after partial writes) + /// must degrade the same way. + #[test] + fn test_extract_auth_gemini_cli_empty_access_token_degrades_to_raw_key() { + let adapter = ClaudeAdapter::new(); + let expired_json = r#"{"access_token":"","refresh_token":"rt-abc","client_id":"cid","client_secret":"cs"}"#; + let provider = create_provider_with_meta( + json!({ + "env": { + "ANTHROPIC_BASE_URL": "https://generativelanguage.googleapis.com", + "ANTHROPIC_API_KEY": expired_json + } + }), + ProviderMeta { + api_format: Some("gemini_native".to_string()), + ..Default::default() + }, + ); + + let auth = adapter.extract_auth(&provider).unwrap(); + assert!( + auth.access_token.as_deref().is_none_or(|t| !t.is_empty()), + "empty access_token leaked into AuthInfo" + ); + assert_eq!(auth.strategy, AuthStrategy::GoogleOAuth); + } + + /// Counter-case: a well-formed JSON credential with a non-empty + /// access_token must still flow through the OAuth path unchanged. + #[test] + fn test_extract_auth_gemini_cli_valid_json_keeps_access_token() { + let adapter = ClaudeAdapter::new(); + let valid_json = r#"{"access_token":"ya29.valid","refresh_token":"rt"}"#; + let provider = create_provider_with_meta( + json!({ + "env": { + "ANTHROPIC_BASE_URL": "https://generativelanguage.googleapis.com", + "ANTHROPIC_API_KEY": valid_json + } + }), + ProviderMeta { + api_format: Some("gemini_native".to_string()), + ..Default::default() + }, + ); + + let auth = adapter.extract_auth(&provider).unwrap(); + assert_eq!(auth.access_token.as_deref(), Some("ya29.valid")); + assert_eq!(auth.strategy, AuthStrategy::GoogleOAuth); + } + #[test] fn test_provider_type_detection() { let adapter = ClaudeAdapter::new();