Environment variable overrides

.github/workflows/ci.yml

@@ -0,0 +1,42 @@
+name: CI
+
+on:
+  pull_request:
+  push:
+    branches:
+      - main
+
+# `build.rs` requires these three or it panics, and they normally come from
+# Doppler (or a local .env, which is gitignored). CI only compiles and runs unit
+# tests — nothing here resolves a host or opens a socket — so placeholders keep
+# the job hermetic and secret-free, which also means it runs on fork PRs. If a
+# test ever needs to reach a real deployment, add a DOPPLER_TOKEN secret and put
+# `doppler run --` in front of the cargo commands instead.
+env:
+  SITE_HOST: https://ci.invalid
+  PLAYSITE_HOST: https://play.ci.invalid
+  CONVEX_HTTP_URL: https://api.ci.invalid
+  CARGO_TERM_COLOR: always
+
+jobs:
+  test:
+    name: test and lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          persist-credentials: false
+
+      - uses: dtolnay/rust-toolchain@stable
+        with:
+          components: clippy
+
+      - uses: Swatinem/rust-cache@v2
+
+      # --all-targets so the test code is linted too, and -D warnings so a lint
+      # that everyone ignores locally can't accumulate.
+      - name: cargo clippy
+        run: cargo clippy --all-targets -- -D warnings
+
+      - name: cargo test
+        run: cargo test

Cargo.toml

@@ -70,6 +70,7 @@ self_update = { version = "0.44", features = ["archive-tar", "archive-zip", "com
 dotenvy = "0.15"
 
 [dev-dependencies]
+tempfile = "3.27.0"
 
 # The profile that 'dist' will build with
 [profile.dist]

src/achievements.rs

@@ -1,4 +1,4 @@
-use crate::auth::{AuthManager, AuthSource};
+use crate::auth::require_api_key;
 use crate::config;
 use anyhow::{Context, Result};
 use serde::Deserialize;
@@ -80,17 +80,6 @@ async fn upload_achievement_image(
     Ok(presigned.r2_key)
 }
 
-fn require_api_key() -> Result<String> {
-    let auth_manager = AuthManager::new()?;
-    let auth_info = auth_manager.get_auth_info();
-    match auth_info.source {
-        AuthSource::None => {
-            anyhow::bail!("Not authenticated. Run `wavedash auth login` first.")
-        }
-        _ => Ok(auth_info.api_key.unwrap()),
-    }
-}
-
 pub struct CreateAchievementArgs<'a> {
     pub game_id: &'a str,
     pub identifier: &'a str,

src/auth.rs

@@ -1,5 +1,5 @@
 use crate::config;
-use anyhow::{bail, Result};
+use anyhow::{anyhow, bail, Result};
 use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _};
 use ring::{
     digest,
@@ -27,6 +27,7 @@ struct Credentials {
     email: Option<String>,
 }
 
+#[derive(Debug, PartialEq, Eq)]
 pub enum AuthSource {
     Environment,
     File,
@@ -82,6 +83,14 @@ impl AuthManager {
     }
 
     pub fn store_credentials(&self, api_key: &str, email: Option<&str>) -> Result<()> {
+        // [`Self::resolve_auth`] treats a blank stored key as a corrupt file and
+        // reports it as unauthenticated, so writing one would leave `auth login`
+        // announcing success over a key `auth status` then disowns. Every way of
+        // supplying a key funnels through here, which makes this the one place
+        // that can promise the file only ever holds a usable one.
+        let api_key = config::non_blank(api_key.to_string())
+            .ok_or_else(|| anyhow::anyhow!("Refusing to store a blank API key."))?;
+
         let path = config::credentials_path()?;
 
         // Create parent directory if it doesn't exist
@@ -95,7 +104,7 @@ impl AuthManager {
         }
 
         let credentials = Credentials {
-            api_key: api_key.to_string(),
+            api_key,
             email: email.map(|s| s.to_string()),
         };
         let json = serde_json::to_string(&credentials)?;
@@ -118,26 +127,44 @@ impl AuthManager {
     }
 
     pub fn get_auth_info(&self) -> AuthInfo {
-        // Check environment first
-        if let Ok(api_key) = std::env::var("WAVEDASH_TOKEN") {
-            if !api_key.is_empty() {
+        Self::resolve_auth(
+            std::env::var(config::ENV_TOKEN).ok(),
+            self.read_file_credentials(),
+        )
+    }
+
+    /// Precedence and blank handling, split out from [`Self::get_auth_info`] so
+    /// it's testable without mutating the process environment or reading the
+    /// real credentials file.
+    ///
+    /// Blank counts as unset, the same rule every other `WAVEDASH_*` variable
+    /// follows: an unpopulated CI secret falls back to stored credentials rather
+    /// than being sent as a bare `Bearer `, which returns a 401 telling the user
+    /// to run an interactive login they can't run. Trimming matters just as
+    /// much — a `WAVEDASH_TOKEN=$(cat key)` trailing newline is not a valid
+    /// header value, and reqwest rejects it as "failed to parse header value"
+    /// with nothing to say it was the token.
+    fn resolve_auth(env_token: Option<String>, file: Option<Credentials>) -> AuthInfo {
+        if let Some(api_key) = env_token.and_then(config::non_blank) {
+            return AuthInfo {
+                source: AuthSource::Environment,
+                api_key: Some(api_key),
+                email: None, // No email available from env var
+            };
+        }
+
+        // A blank key on disk is a corrupt or half-written credentials file, and
+        // is no more usable than a blank variable.
+        if let Some(creds) = file {
+            if let Some(api_key) = config::non_blank(creds.api_key) {
                 return AuthInfo {
-                    source: AuthSource::Environment,
+                    source: AuthSource::File,
                     api_key: Some(api_key),
-                    email: None, // No email available from env var
+                    email: creds.email,
                 };
             }
         }
 
-        // Check file
-        if let Some(creds) = self.read_file_credentials() {
-            return AuthInfo {
-                source: AuthSource::File,
-                api_key: Some(creds.api_key),
-                email: creds.email,
-            };
-        }
-
         AuthInfo {
             source: AuthSource::None,
             api_key: None,
@@ -158,6 +185,15 @@ impl AuthManager {
     }
 }
 
+pub(crate) fn require_api_key() -> Result<String> {
+    AuthManager::new()?.get_auth_info().api_key.ok_or_else(|| {
+        anyhow!(
+            "Not authenticated. Set {}, or run `wavedash auth login`.",
+            config::ENV_TOKEN
+        )
+    })
+}
+
 pub(crate) fn generate_state() -> String {
     use std::time::{SystemTime, UNIX_EPOCH};
     SystemTime::now()
@@ -496,6 +532,81 @@ pub async fn login_with_browser() -> Result<LoginResult> {
 mod tests {
     use super::*;
 
+    fn stored(api_key: &str) -> Option<Credentials> {
+        Some(Credentials {
+            api_key: api_key.to_string(),
+            email: Some("dev@wavedash.com".to_string()),
+        })
+    }
+
+    #[test]
+    fn env_token_wins_over_stored_credentials() {
+        let info = AuthManager::resolve_auth(Some("from_env".into()), stored("from_file"));
+
+        assert_eq!(info.source, AuthSource::Environment);
+        assert_eq!(info.api_key.as_deref(), Some("from_env"));
+    }
+
+    /// The CI shape: an unpopulated secret expands to "", which must not shadow
+    /// the stored credentials or be sent as a bare `Bearer `.
+    #[test]
+    fn a_blank_env_token_falls_back_to_stored_credentials() {
+        for blank in ["", "   ", "\t", "\n"] {
+            let info = AuthManager::resolve_auth(Some(blank.into()), stored("from_file"));
+
+            assert_eq!(info.source, AuthSource::File, "blank: {:?}", blank);
+            assert_eq!(info.api_key.as_deref(), Some("from_file"));
+        }
+    }
+
+    #[test]
+    fn a_blank_env_token_with_nothing_stored_is_unauthenticated() {
+        let info = AuthManager::resolve_auth(Some("   ".into()), None);
+
+        assert_eq!(info.source, AuthSource::None);
+        assert!(info.api_key.is_none());
+    }
+
+    /// `WAVEDASH_TOKEN=$(cat key.txt)` keeps the trailing newline, which is not a
+    /// legal header value — trim it here rather than fail opaquely at send time.
+    #[test]
+    fn an_env_token_is_trimmed_so_it_survives_becoming_a_header() {
+        let info = AuthManager::resolve_auth(Some("  wdcli_abc123\n".into()), None);
+
+        assert_eq!(info.api_key.as_deref(), Some("wdcli_abc123"));
+        assert!(reqwest::header::HeaderValue::from_str(&format!(
+            "Bearer {}",
+            info.api_key.unwrap()
+        ))
+        .is_ok());
+    }
+
+    #[test]
+    fn a_blank_stored_key_is_unauthenticated() {
+        let info = AuthManager::resolve_auth(None, stored("  "));
+
+        assert_eq!(info.source, AuthSource::None);
+        assert!(info.api_key.is_none());
+    }
+
+    /// The other half of the rule above: what the read path disowns, the write
+    /// path must never produce. Refused before the credentials path is touched,
+    /// so this test writes nothing.
+    #[test]
+    fn a_blank_api_key_is_never_stored() {
+        for blank in ["", "   ", "\t", "\n"] {
+            let err = AuthManager
+                .store_credentials(blank, None)
+                .expect_err(&format!("stored blank key {:?}", blank));
+
+            assert!(
+                err.to_string().contains("blank API key"),
+                "unexpected error: {}",
+                err
+            );
+        }
+    }
+
     #[test]
     fn derives_rfc7636_s256_pkce_challenge() {
         let challenge = pkce_challenge_from_verifier("dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk");

src/builds.rs

@@ -147,7 +147,7 @@ pub async fn handle_build_push(
     let config_dir = config_path
         .parent()
         .ok_or_else(|| anyhow::anyhow!("Config file has no parent directory"))?;
-    let upload_dir = config_dir.join(&wavedash_config.upload_dir);
+    let upload_dir = config_dir.join(wavedash_config.upload_dir()?);
 
     // Verify source directory exists
     if !upload_dir.exists() {
@@ -170,11 +170,11 @@ pub async fn handle_build_push(
     let engine_kind = wavedash_config.engine_type()?;
     let creds = get_temp_credentials(
         BuildUploadInfo {
-            game_id: &wavedash_config.game_id,
+            game_id: wavedash_config.game_id()?,
             engine: engine_kind.map(|e| e.as_label()),
-            engine_version: wavedash_config.engine_version(),
-            entrypoint: wavedash_config.entrypoint(),
-            entrypoint_params: wavedash_config.executable_entrypoint_params(),
+            engine_version: wavedash_config.engine_version()?,
+            entrypoint: wavedash_config.entrypoint()?,
+            entrypoint_params: wavedash_config.executable_entrypoint_params()?,
             message: message.as_deref(),
             build_size_bytes: total_bytes,
         },
@@ -198,7 +198,7 @@ pub async fn handle_build_push(
 
     // Notify the server that upload is complete
     let result =
-        notify_upload_complete(&wavedash_config.game_id, &creds.game_build_id, &api_key).await?;
+        notify_upload_complete(wavedash_config.game_id()?, &creds.game_build_id, &api_key).await?;
 
     // Print the play URL
     let site_host = config::get("open_browser_website_host")?;