fix: harden auth and input handling

Author: Finesssee

README.md

@@ -414,11 +414,14 @@ Browser-based Authorization Code + PKCE flow with automatic token refresh.
 
 ```bash
 linear-cli auth oauth          # Opens browser for authorization
+linear-cli auth oauth --secure # Store OAuth tokens in OS keyring (best on official release builds)
 linear-cli auth status         # Show auth type, token expiry
 linear-cli auth revoke         # Revoke OAuth tokens
 linear-cli auth logout         # Remove stored credentials
 ```
 
+> On macOS, `--secure` works best with an official signed release binary. Locally built or frequently rebuilt CLI binaries can trigger repeated Keychain prompts and may fail keychain readback verification. If that happens, use plain `linear-cli auth oauth` or `LINEAR_API_KEY` instead.
+
 **Auth priority:** `LINEAR_API_KEY` env var > OS keyring > OAuth tokens > config file API key.
 
 ## Configuration

src/api.rs

@@ -1,7 +1,8 @@
 use anyhow::{Context, Result};
 use futures::StreamExt;
 use reqwest::header::HeaderMap;
-use reqwest::{Client, StatusCode};
+use reqwest::redirect::Policy;
+use reqwest::{Client, StatusCode, Url};
 use serde_json::{json, Value};
 use std::sync::Arc;
 use std::time::Duration;
@@ -16,6 +17,7 @@ use crate::text::is_uuid;
 use std::sync::OnceLock;
 
 const LINEAR_API_URL: &str = "https://api.linear.app/graphql";
+const LINEAR_UPLOADS_HOST: &str = "uploads.linear.app";
 
 /// Configuration for generic ID resolution
 struct ResolverConfig<'a> {
@@ -135,6 +137,47 @@ fn http_error(status: StatusCode, headers: &HeaderMap, context: &str) -> CliErro
     err.with_details(details)
 }
 
+pub fn parse_linear_upload_url(raw: &str) -> Result<Url> {
+    let url = Url::parse(raw).context("Failed to parse upload URL")?;
+
+    if url.scheme() != "https" {
+        anyhow::bail!("Invalid upload URL: expected https");
+    }
+    if url.host_str() != Some(LINEAR_UPLOADS_HOST) {
+        anyhow::bail!(
+            "Invalid upload URL: expected exact host '{}'",
+            LINEAR_UPLOADS_HOST
+        );
+    }
+    if !url.username().is_empty() || url.password().is_some() {
+        anyhow::bail!("Invalid upload URL: credentials are not allowed");
+    }
+    if let Some(port) = url.port() {
+        if port != 443 {
+            anyhow::bail!("Invalid upload URL: non-default ports are not allowed");
+        }
+    }
+
+    Ok(url)
+}
+
+fn upload_redirect_policy() -> Policy {
+    Policy::custom(|attempt| {
+        let url = attempt.url();
+        let is_allowed = url.scheme() == "https"
+            && url.host_str() == Some(LINEAR_UPLOADS_HOST)
+            && url.username().is_empty()
+            && url.password().is_none()
+            && url.port_or_known_default() == Some(443);
+
+        if is_allowed {
+            attempt.follow()
+        } else {
+            attempt.stop()
+        }
+    })
+}
+
 /// Resolves a team key (like "SCW") or name to a team UUID.
 /// If the input is already a UUID (36 characters with dashes), returns it as-is.
 pub async fn resolve_team_id(
@@ -618,10 +661,15 @@ impl LinearClient {
         url: &str,
         writer: &mut impl std::io::Write,
     ) -> Result<u64> {
+        let url = parse_linear_upload_url(url)?;
         let auth_header = self.ensure_fresh_auth().await?;
+        let upload_client = Client::builder()
+            .timeout(Duration::from_secs(30))
+            .redirect(upload_redirect_policy())
+            .build()
+            .context("Failed to build upload client")?;
 
-        let response = self
-            .client
+        let response = upload_client
             .get(url)
             .header("Authorization", &auth_header)
             .send()
@@ -699,7 +747,7 @@ impl LinearClient {
                 let new_tokens = crate::oauth::refresh_tokens(client_id, refresh_token).await?;
 
                 // Persist the new tokens
-                let scopes = if let Ok(Some(existing)) = config::get_oauth_config(profile) {
+                let scopes = if let Ok(Some(existing)) = config::get_oauth_metadata(profile) {
                     existing.scopes
                 } else {
                     vec![]
@@ -713,7 +761,17 @@ impl LinearClient {
                     token_type: new_tokens.token_type.clone(),
                     scopes,
                 };
-                if let Err(e) = config::save_oauth_config(profile, &oauth_config) {
+                #[cfg(feature = "secure-storage")]
+                let persist_result = if config::oauth_uses_secure_storage(profile).unwrap_or(false) {
+                    config::save_oauth_config_secure(profile, &oauth_config)
+                } else {
+                    config::save_oauth_config(profile, &oauth_config)
+                };
+
+                #[cfg(not(feature = "secure-storage"))]
+                let persist_result = config::save_oauth_config(profile, &oauth_config);
+
+                if let Err(e) = persist_result {
                     eprintln!("Warning: Failed to persist refreshed OAuth tokens: {}", e);
                 }
 

src/commands/auth.rs

@@ -1,4 +1,4 @@
-use anyhow::Result;
+use anyhow::{Context, Result};
 use clap::Subcommand;
 use dialoguer::{Confirm, Password};
 use serde_json::json;
@@ -108,6 +108,11 @@ async fn login(
     #[cfg(feature = "secure-storage")]
     if secure {
         crate::keyring::set_key(&profile, &key)?;
+        let saved = crate::keyring::get_key(&profile)?
+            .context("API key was written to keyring but could not be read back")?;
+        if saved != key {
+            anyhow::bail!("API key stored in keyring could not be verified after saving");
+        }
 
         if output.is_json() || output.has_template() {
             print_json_owned(
@@ -168,8 +173,10 @@ async fn logout(force: bool, output: &OutputOptions) -> Result<()> {
     #[cfg(feature = "secure-storage")]
     {
         let _ = crate::keyring::delete_key(&profile); // Ignore errors (may not exist)
+        let _ = crate::keyring::delete_oauth_tokens(&profile); // Ignore errors (may not exist)
     }
 
+    config::clear_oauth_config(&profile)?;
     config::workspace_remove(&profile)?;
 
     if output.is_json() || output.has_template() {
@@ -197,34 +204,50 @@ async fn status(validate: bool, output: &OutputOptions) -> Result<()> {
         .ok()
         .filter(|p| !p.is_empty());
 
-    let configured = profile
+    let config_file_configured = profile
         .as_ref()
         .and_then(|p| config_data.workspaces.get(p))
         .map(|w| !w.api_key.is_empty())
         .unwrap_or(false);
 
     // Check keyring storage
     #[cfg(feature = "secure-storage")]
-    let keyring_configured = profile
+    let api_key_keyring_configured = profile
         .as_ref()
         .and_then(|p| crate::keyring::get_key(p).ok())
         .flatten()
         .is_some();
     #[cfg(not(feature = "secure-storage"))]
-    let keyring_configured = false;
+    let api_key_keyring_configured = false;
+
+    #[cfg(feature = "secure-storage")]
+    let oauth_keyring_configured = profile
+        .as_ref()
+        .and_then(|p| crate::keyring::get_oauth_tokens(p).ok())
+        .flatten()
+        .is_some();
+    #[cfg(not(feature = "secure-storage"))]
+    let oauth_keyring_configured = false;
 
     #[cfg(feature = "secure-storage")]
     let keyring_available = crate::keyring::is_available();
     #[cfg(not(feature = "secure-storage"))]
     let keyring_available = false;
 
+    let keyring_configured = api_key_keyring_configured || oauth_keyring_configured;
+
     // Check OAuth config
+    let oauth_metadata = profile
+        .as_ref()
+        .and_then(|p| config::get_oauth_metadata(p).ok())
+        .flatten();
+    let oauth_configured = oauth_metadata.is_some();
     let oauth_config = profile
         .as_ref()
         .and_then(|p| config::get_oauth_config(p).ok())
         .flatten();
-    let oauth_configured = oauth_config.is_some();
     let auth_type = if oauth_configured { "oauth" } else { "api_key" };
+    let configured = config_file_configured || keyring_configured || oauth_configured;
 
     let mut validated = None;
     if validate {
@@ -271,11 +294,12 @@ async fn status(validate: bool, output: &OutputOptions) -> Result<()> {
                 "profile": profile,
                 "configured": configured,
                 "keyring_configured": keyring_configured,
+                "oauth_keyring_configured": oauth_keyring_configured,
                 "keyring_available": keyring_available,
                 "auth_type": auth_type,
                 "oauth_configured": oauth_configured,
-                "oauth_scopes": oauth_config.as_ref().map(|o| &o.scopes),
-                "oauth_expires_at": oauth_config.as_ref().and_then(|o| o.expires_at),
+                "oauth_scopes": oauth_metadata.as_ref().map(|o| &o.scopes),
+                "oauth_expires_at": oauth_metadata.as_ref().and_then(|o| o.expires_at),
                 "env_api_key": env_key.is_some(),
                 "env_profile": env_profile,
                 "validated": validated,
@@ -295,6 +319,10 @@ async fn status(validate: bool, output: &OutputOptions) -> Result<()> {
         "Keyring available: {}",
         if keyring_available { "yes" } else { "no" }
     );
+    println!(
+        "OAuth keyring: {}",
+        if oauth_keyring_configured { "yes" } else { "no" }
+    );
     println!(
         "Env API key override: {}",
         if env_key.is_some() { "yes" } else { "no" }
@@ -303,7 +331,7 @@ async fn status(validate: bool, output: &OutputOptions) -> Result<()> {
         println!("Validated: {}", if validated { "yes" } else { "no" });
     }
     println!("Auth type: {}", auth_type);
-    if let Some(ref oauth) = oauth_config {
+    if let Some(ref oauth) = oauth_metadata {
         println!("OAuth scopes: {:?}", oauth.scopes);
         if let Some(expires) = oauth.expires_at {
             let dt = chrono::DateTime::from_timestamp(expires, 0)
@@ -503,18 +531,9 @@ async fn oauth_login(
 
     #[cfg(feature = "secure-storage")]
     if secure {
-        let json = serde_json::to_string(&oauth_config)?;
-        crate::keyring::set_oauth_tokens(&profile, &json)?;
-        // Save only metadata in config (no secrets)
-        let metadata_only = config::OAuthConfig {
-            client_id: oauth_config.client_id.clone(),
-            access_token: String::new(),
-            refresh_token: None,
-            expires_at: oauth_config.expires_at,
-            token_type: oauth_config.token_type.clone(),
-            scopes: oauth_config.scopes.clone(),
-        };
-        config::save_oauth_config(&profile, &metadata_only)?;
+        config::save_oauth_config_secure(&profile, &oauth_config).context(
+            "Secure OAuth storage failed. On macOS, locally built or unsigned binaries may not be able to read back Keychain items. Try the official signed release or fall back to plain `linear-cli auth oauth`.",
+        )?;
 
         if output.is_json() || output.has_template() {
             print_json_owned(

src/commands/doctor.rs

@@ -18,12 +18,34 @@ pub async fn run(output: &OutputOptions, check_api: bool, fix: bool) -> Result<(
         .filter(|p| !p.is_empty());
     let cache_dir = cache::cache_dir_path()?;
 
-    let configured = profile
+    let config_file_configured = profile
         .as_ref()
         .and_then(|p| config_data.workspaces.get(p))
         .map(|w| !w.api_key.is_empty())
         .unwrap_or(false);
 
+    #[cfg(feature = "secure-storage")]
+    let keyring_available = crate::keyring::is_available();
+    #[cfg(not(feature = "secure-storage"))]
+    let keyring_available = false;
+
+    #[cfg(feature = "secure-storage")]
+    let oauth_keyring_configured = profile
+        .as_ref()
+        .and_then(|p| crate::keyring::get_oauth_tokens(p).ok())
+        .flatten()
+        .is_some();
+    #[cfg(not(feature = "secure-storage"))]
+    let oauth_keyring_configured = false;
+
+    let oauth_configured = profile
+        .as_ref()
+        .and_then(|p| config::get_oauth_metadata(p).ok())
+        .flatten()
+        .is_some();
+
+    let configured = config_file_configured || oauth_configured || oauth_keyring_configured;
+
     let mut api_ok = None;
     let mut api_error = None;
     if check_api || fix {
@@ -141,6 +163,9 @@ pub async fn run(output: &OutputOptions, check_api: bool, fix: bool) -> Result<(
                 "config_path": config_path.to_string_lossy(),
                 "profile": profile,
                 "configured": configured,
+                "keyring_available": keyring_available,
+                "oauth_configured": oauth_configured,
+                "oauth_keyring_configured": oauth_keyring_configured,
                 "env_api_key": env_key.is_some(),
                 "env_profile": env_profile,
                 "cache_dir": cache_dir.to_string_lossy(),
@@ -156,6 +181,18 @@ pub async fn run(output: &OutputOptions, check_api: bool, fix: bool) -> Result<(
     println!("Config path: {}", config_path.display());
     println!("Profile: {}", profile.unwrap_or_else(|| "none".to_string()));
     println!("Configured: {}", if configured { "yes" } else { "no" });
+    println!(
+        "Keyring available: {}",
+        if keyring_available { "yes" } else { "no" }
+    );
+    println!(
+        "OAuth configured: {}",
+        if oauth_configured { "yes" } else { "no" }
+    );
+    println!(
+        "OAuth keyring configured: {}",
+        if oauth_keyring_configured { "yes" } else { "no" }
+    );
     println!(
         "Env API key override: {}",
         if env_key.is_some() { "yes" } else { "no" }