diff --git a/.github/workflows/bin.yml b/.github/workflows/bin.yml index 3ddd71f..8e2c1c6 100644 --- a/.github/workflows/bin.yml +++ b/.github/workflows/bin.yml @@ -34,7 +34,6 @@ jobs: architecture: x86_64 binary-postfix: "" binary-name: vimstoat - use-cross: false - os: macos-latest os-name: macos @@ -42,7 +41,6 @@ jobs: architecture: arm64 binary-postfix: "" binary-name: vimstoat - use-cross: false - os: ubuntu-latest os-name: linux @@ -50,7 +48,6 @@ jobs: architecture: x86_64 binary-postfix: "" binary-name: vimstoat - use-cross: false - os: ubuntu-latest os-name: linux @@ -58,7 +55,6 @@ jobs: architecture: x86_64-musl binary-postfix: "" binary-name: vimstoat - use-cross: true - os: ubuntu-24.04-arm os-name: linux @@ -66,7 +62,6 @@ jobs: architecture: arm64 binary-postfix: "" binary-name: vimstoat - use-cross: false - os: ubuntu-24.04-arm os-name: linux @@ -74,7 +69,6 @@ jobs: architecture: arm64-musl binary-postfix: "" binary-name: vimstoat - use-cross: true - os: windows-latest os-name: windows @@ -82,7 +76,6 @@ jobs: architecture: x86_64 binary-postfix: ".exe" binary-name: vimstoat - use-cross: false - os: windows-latest os-name: windows @@ -90,7 +83,6 @@ jobs: architecture: arm64 binary-postfix: ".exe" binary-name: vimstoat - use-cross: true steps: - name: Checkout repository @@ -103,20 +95,13 @@ jobs: - uses: Swatinem/rust-cache@v2 - - name: Install cross - if: matrix.use-cross - uses: taiki-e/install-action@v2 - with: - tool: cross + - name: Install musl-tools + if: matrix.os-name == 'linux' && contains(matrix.target, 'musl') && !matrix.use-cross + run: sudo apt-get update && sudo apt-get install -y musl-tools - name: Cargo build shell: bash - run: | - if [ "${{ matrix.use-cross }}" = "true" ]; then - cross build --release --target ${{ matrix.target }} - else - cargo build --release --target ${{ matrix.target }} - fi + run: cargo build --release --target ${{ matrix.target }} - name: Packaging final binary shell: bash diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index c0d076c..bc94a49 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -57,8 +57,24 @@ jobs: - name: Lint with Clippy run: cargo clippy -- -D warnings - - name: Run Tests - run: cargo test + - name: Install Keyring & D-Bus dependencies + run: | + sudo apt-get update + sudo apt-get install -y gnome-keyring dbus dbus-x11 libsecret-tools + + - name: Run Tests with Keyring + env: + STOAT_TEST_TOKEN: ${{ secrets.STOAT_TEST_TOKEN }} + run: | + dbus-run-session -- bash -c ' + set -e + echo -n "ci-keyring-pwd" | gnome-keyring-daemon --components=secrets --unlock & + sleep 1 + test -n "$STOAT_TEST_TOKEN" || { echo "STOAT_TEST_TOKEN secret is not set"; exit 1; } + printf "%s" "$STOAT_TEST_TOKEN" | secret-tool store --label=vimstoat \ + service keyring-lib username vimstoat target default application rust-keyring + cargo test + ' publish-cargo: name: Publishing to Cargo diff --git a/src/api/auth.rs b/src/api/auth.rs index 1fa828f..0ffac48 100644 --- a/src/api/auth.rs +++ b/src/api/auth.rs @@ -51,6 +51,84 @@ impl Auth { mod tests { use super::*; + /// Reads the user's real session token from the keyring (read-only). + /// Panics if the keyring or a stored token is unavailable. + async fn real_token() -> String { + let auth = Auth::new().expect("keyring must be available to run auth tests"); + + let token = auth + .token_entry + .get_secret() + .await + .expect("a vimstoat session token must be stored in the keyring"); + + assert!(!token.is_empty(), "stored keyring token is empty"); + token + } + + /// Builds an `Auth` backed by a test-specific keyring entry. + /// Never touches the real vimstoat entry. + fn test_auth(id: &str) -> Auth { + Auth { + token_entry: KeyringEntry::try_new(id).expect("Failed to create test keyring entry"), + } + } + + #[tokio::test] + async fn test_validate_token_accepts_real_token() { + let token = real_token().await; + let auth = Auth::new().unwrap(); + + let client = auth + .validate_token(&token, None) + .await + .expect("real keyring token should validate against the Stoat API"); + + // Not assert_eq!: on failure it would print the real token. + assert!( + client.clone_token() == token, + "validated client token does not match the keyring token" + ); + } + + #[tokio::test] + async fn test_validate_token_rejects_invalid_token() { + let auth = test_auth("vimstoat_test_invalid_token"); + + // Not expect_err: it requires `ApiClient: Debug`, which is deliberately absent. + let Err(err) = auth + .validate_token("definitely_not_a_real_session_token", None) + .await + else { + panic!("bogus token should be rejected by the Stoat API"); + }; + + assert!( + matches!( + err.downcast_ref::(), + Some(AuthError::InvalidToken(_)) + ), + "unexpected error: {err:?}" + ); + } + + #[tokio::test] + async fn test_real_token_works_with_authenticated_endpoint() { + let token = real_token().await; + + let client = ApiClient::new(token, None); + let me = client + .get::(Endpoint::CurrentUser) + .await + .expect("GET /users/@me should succeed with the real token"); + + assert!(me.get("_id").is_some(), "no _id in response: {me}"); + assert!( + me.get("username").is_some(), + "no username in response: {me}" + ); + } + #[tokio::test] async fn test_keyring_store_and_get() { // Use a test-specific ID so we don't overwrite the actual vimstoat token during tests @@ -62,15 +140,9 @@ mod tests { let test_token = "test_secret_token_12345"; // Test storing the token - let store_result = auth.store_token(test_token).await; - // The test might fail on CI or headless systems without a secret service, so we handle it gracefully - if let Err(e) = store_result { - println!( - "Skipping keyring test because the environment doesn't support it: {}", - e - ); - return; - } + auth.store_token(test_token) + .await + .expect("keyring must be available to run auth tests"); // Test getting the token let retrieved_token = auth @@ -86,4 +158,27 @@ mod tests { // Clean up let _ = auth.token_entry.delete_secret().await; } + + #[tokio::test] + async fn test_keyring_store_overwrites_existing_token() { + let auth = test_auth("vimstoat_test_keyring_overwrite"); + + auth.store_token("first_token").await.unwrap(); + auth.store_token("second_token").await.unwrap(); + + let retrieved = auth.token_entry.get_secret().await.unwrap(); + assert_eq!(retrieved, "second_token"); + + let _ = auth.token_entry.delete_secret().await; + } + + #[tokio::test] + async fn test_keyring_get_after_delete_fails() { + let auth = test_auth("vimstoat_test_keyring_delete"); + + auth.store_token("temp_token").await.unwrap(); + auth.token_entry.delete_secret().await.unwrap(); + + assert!(auth.token_entry.get_secret().await.is_err()); + } } diff --git a/src/api/client.rs b/src/api/client.rs index 3d9a2d6..992f88d 100644 --- a/src/api/client.rs +++ b/src/api/client.rs @@ -44,7 +44,11 @@ impl Endpoint { } } -#[derive(Debug, Clone)] +// SECURITY: intentionally does NOT implement `Debug`. This type holds the user's +// session token, and a `{:?}`, `dbg!`, or derived `Debug` on a parent struct would +// write it to the log file or terminal. Do not add `Debug` here (or derive it on any +// type that contains an `ApiClient`) unless the token is redacted. +#[derive(Clone)] pub struct ApiClient { client: Client, token: String, diff --git a/src/api/events.rs b/src/api/events.rs index 55ea1dc..47d844e 100644 --- a/src/api/events.rs +++ b/src/api/events.rs @@ -1,7 +1,11 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; -#[derive(Debug, Clone, Serialize)] +// SECURITY: intentionally does NOT implement `Debug`. `Authenticate` carries the +// user's session token, and a `{:?}`, `dbg!`, or derived `Debug` would write it to +// the log file or terminal. Do not add `Debug` here (or derive it on any type that +// contains a `ClientEvent`) unless the token is redacted. +#[derive(Clone, Serialize)] #[serde(tag = "type")] #[allow(unused)] pub enum ClientEvent { diff --git a/src/logger.rs b/src/logger.rs index 0faafa9..5b9f425 100644 --- a/src/logger.rs +++ b/src/logger.rs @@ -1,20 +1,28 @@ -use std::{fs, path::PathBuf}; +use std::{ + fs, + path::{Path, PathBuf}, +}; use crate::Result; pub const LOG_FILE: &str = "logs"; -fn create_log_file() -> Result { - let mut path = if let Some(mut p) = dirs::cache_dir() { +fn log_path(cache_dir: Option) -> PathBuf { + let mut path = if let Some(mut p) = cache_dir { p.push(env!("CARGO_PKG_NAME")); p } else { PathBuf::new() }; - fs::create_dir_all(&path)?; - path.push(LOG_FILE); + path +} + +fn open_log_file(path: &Path) -> Result { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; + } Ok(fs::OpenOptions::new() .create(true) @@ -23,6 +31,10 @@ fn create_log_file() -> Result { .expect("Failed to open log file")) } +fn create_log_file() -> Result { + open_log_file(&log_path(dirs::cache_dir())) +} + pub fn init() -> Result<()> { let log_file = create_log_file()?; @@ -35,3 +47,66 @@ pub fn init() -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use std::io::Write; + + use tempfile::TempDir; + + use super::*; + + #[test] + fn log_path_uses_package_name_and_log_file() { + let base = PathBuf::from("/some/cache"); + let path = log_path(Some(base.clone())); + + assert_eq!(path, base.join(env!("CARGO_PKG_NAME")).join(LOG_FILE)); + } + + #[test] + fn log_path_without_cache_dir_is_relative() { + assert_eq!(log_path(None), PathBuf::from(LOG_FILE)); + } + + #[test] + fn open_log_file_creates_missing_directories() { + let tmp = TempDir::new().unwrap(); + let path = log_path(Some(tmp.path().join("nested").join("cache"))); + assert!(!path.parent().unwrap().exists()); + + open_log_file(&path).unwrap(); + + assert!(path.is_file()); + } + + #[test] + fn open_log_file_appends_across_opens() { + let tmp = TempDir::new().unwrap(); + let path = log_path(Some(tmp.path().to_path_buf())); + + let mut f = open_log_file(&path).unwrap(); + f.write_all(b"a").unwrap(); + drop(f); + + let mut f = open_log_file(&path).unwrap(); + f.write_all(b"b").unwrap(); + drop(f); + + assert_eq!(fs::read_to_string(&path).unwrap(), "ab"); + } + + #[test] + fn open_log_file_preserves_existing_content() { + let tmp = TempDir::new().unwrap(); + let path = log_path(Some(tmp.path().to_path_buf())); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(&path, "existing\n").unwrap(); + + let mut f = open_log_file(&path).unwrap(); + f.write_all(b"new\n").unwrap(); + drop(f); + + assert_eq!(fs::read_to_string(&path).unwrap(), "existing\nnew\n"); + } +}