Skip to main content

max / audiofiles

Harden export filename sanitization and move API key to the keychain (ultra-fuzz Security) - resolve_output_names sanitized computed names but returned plugin-supplied name_overrides verbatim — an untrusted Rhai transform_filename hook could inject a traversal component. Sanitize overrides through the same primitive; strip_traversal now correctly defuses only a bare `.`/`..` (after separators are stripped a name is a single in-dir component) instead of the prior ineffective stem check. The call-site sanitizer also defuses a bare `..`. - SyncKit API key now lives in the OS keychain (keyring), with a one-time migration off the legacy plaintext file (read, store, delete) and graceful fallback to env/bundled when no keychain provider is present. save_api_key writes to the keychain, not a cleartext file. Tests: override traversal payloads are neutralized; strip_traversal payload table; keychain load/migration factored into pure read_key_file / env-or-bundled helpers so unit tests stay off the global keychain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-18 21:16 UTC
Commit: d0aeb6d22e398d10c42073e54e3ed19a00c57fe9
Parent: 670c669
5 files changed, +186 insertions, -64 deletions
M Cargo.lock +1
@@ -381,6 +381,7 @@ dependencies = [
381 381 "dirs",
382 382 "eframe",
383 383 "gtk",
384 + "keyring",
384 385 "midir",
385 386 "open",
386 387 "parking_lot",
M crates/audiofiles-app/Cargo.toml +1
@@ -26,6 +26,7 @@ chrono = { workspace = true }
26 26 midir = { workspace = true }
27 27 rfd = { workspace = true }
28 28 toml = { workspace = true }
29 + keyring = { version = "3", features = ["apple-native", "linux-native", "windows-native"] }
29 30
30 31 [dev-dependencies]
31 32 tempfile = "3"
M crates/audiofiles-app/src/main.rs +83 -54
@@ -164,33 +164,73 @@ fn parse_synckit_toml_key() -> Option<String> {
164 164 table.get("api_key").filter(|s| !s.is_empty()).cloned()
165 165 }
166 166
167 - /// Load a saved API key from the data directory, falling back to env vars and bundled toml.
168 - fn load_api_key(data_dir: &Path) -> Option<String> {
169 - // Saved key file takes priority
170 - let key_path = data_dir.join("sync_api_key");
171 - if let Ok(key) = std::fs::read_to_string(&key_path) {
172 - let key = key.trim().to_string();
173 - if !key.is_empty() {
174 - tracing::info!("Loaded SyncKit API key from {}", key_path.display());
175 - return Some(key);
176 - }
177 - }
178 - // Fall back to env vars (for development / CI)
167 + /// OS keychain entry for the SyncKit API key. The key is a credential, not a
168 + /// regenerable cache, so it belongs in the keychain rather than a plaintext
169 + /// file. Returns None when no keychain provider is available (headless Linux
170 + /// without secret-service, CI) — callers fall back to file/env/bundled.
171 + fn api_key_keychain_entry() -> Option<keyring::Entry> {
172 + keyring::Entry::new("audiofiles", "sync_api_key").ok()
173 + }
174 +
175 + /// Read a trimmed API key from the legacy plaintext file. None if absent/empty.
176 + fn read_key_file(data_dir: &Path) -> Option<String> {
177 + let key = std::fs::read_to_string(data_dir.join("sync_api_key")).ok()?;
178 + let key = key.trim().to_string();
179 + (!key.is_empty()).then_some(key)
180 + }
181 +
182 + /// API key from env vars (dev/CI) or the bundled synckit.toml (alpha shared key).
183 + fn api_key_from_env_or_bundled() -> Option<String> {
179 184 if let (Ok(_url), Ok(key)) = (
180 185 std::env::var("AF_SYNC_SERVER_URL"),
181 186 std::env::var("AF_SYNC_API_KEY"),
182 187 ) {
183 188 return Some(key);
184 189 }
185 - // Fall back to bundled synckit.toml
186 - parse_synckit_toml_key()}
190 + parse_synckit_toml_key()
191 + }
187 192
188 - /// Save an API key to the data directory for future launches.
189 - #[cfg(test)]
190 - fn save_api_key(data_dir: &Path, api_key: &str) {
191 - let key_path = data_dir.join("sync_api_key");
192 - if let Err(e) = std::fs::write(&key_path, api_key) {
193 - tracing::error!("Failed to save API key to {}: {e}", key_path.display());
193 + /// Load a saved API key. Order: OS keychain, then a legacy plaintext file
194 + /// (migrated into the keychain and deleted on read), then env vars, then
195 + /// bundled toml.
196 + fn load_api_key(data_dir: &Path) -> Option<String> {
197 + // Keychain takes priority.
198 + if let Some(entry) = api_key_keychain_entry() {
199 + match entry.get_password() {
200 + Ok(key) if !key.trim().is_empty() => {
201 + tracing::info!("Loaded SyncKit API key from OS keychain");
202 + return Some(key.trim().to_string());
203 + }
204 + Ok(_) | Err(keyring::Error::NoEntry) => {}
205 + Err(e) => tracing::warn!("Keychain read failed, falling back: {e}"),
206 + }
207 + }
208 +
209 + // Legacy plaintext file: migrate into the keychain, then remove it so the
210 + // credential stops living on disk in cleartext.
211 + if let Some(key) = read_key_file(data_dir) {
212 + tracing::info!("Migrating SyncKit API key into the keychain");
213 + if save_api_key(&key) {
214 + let _ = std::fs::remove_file(data_dir.join("sync_api_key"));
215 + }
216 + return Some(key);
217 + }
218 +
219 + api_key_from_env_or_bundled()
220 + }
221 +
222 + /// Persist an API key to the OS keychain. Returns true on success; best-effort,
223 + /// so a missing keychain provider degrades gracefully rather than failing.
224 + fn save_api_key(api_key: &str) -> bool {
225 + let Some(entry) = api_key_keychain_entry() else {
226 + return false;
227 + };
228 + match entry.set_password(api_key) {
229 + Ok(()) => true,
230 + Err(e) => {
231 + tracing::warn!("Failed to store API key in keychain: {e}");
232 + false
233 + }
194 234 }
195 235 }
196 236
@@ -950,67 +990,56 @@ fn reveal_in_file_manager(path: &Path) {
950 990 mod tests {
951 991 use super::*;
952 992
993 + // The plaintext-file path is tested via `read_key_file` so the tests stay
994 + // deterministic — `load_api_key` reads the real OS keychain first, whose
995 + // state is global and not test-controlled.
996 +
953 997 #[test]
954 - fn load_api_key_from_file() {
998 + fn read_key_file_returns_key() {
955 999 let dir = tempfile::tempdir().unwrap();
956 1000 std::fs::write(dir.path().join("sync_api_key"), "test-key-123").unwrap();
957 - let result = load_api_key(dir.path());
958 - assert_eq!(result, Some("test-key-123".to_string()));
1001 + assert_eq!(read_key_file(dir.path()), Some("test-key-123".to_string()));
959 1002 }
960 1003
961 1004 #[test]
962 - fn load_api_key_trims_whitespace() {
1005 + fn read_key_file_trims_whitespace() {
963 1006 let dir = tempfile::tempdir().unwrap();
964 1007 std::fs::write(dir.path().join("sync_api_key"), " key-with-spaces \n").unwrap();
965 - let result = load_api_key(dir.path());
966 - assert_eq!(result, Some("key-with-spaces".to_string()));
1008 + assert_eq!(read_key_file(dir.path()), Some("key-with-spaces".to_string()));
967 1009 }
968 1010
969 1011 #[test]
970 - fn load_api_key_empty_file_falls_back_to_bundled() {
1012 + fn read_key_file_empty_is_none() {
971 1013 let dir = tempfile::tempdir().unwrap();
972 1014 std::fs::write(dir.path().join("sync_api_key"), "").unwrap();
973 - // Empty file → falls through to bundled synckit.toml key
974 - if std::env::var("AF_SYNC_API_KEY").is_err() {
975 - let key = load_api_key(dir.path());
976 - assert_eq!(key, parse_synckit_toml_key());
977 - }
1015 + assert_eq!(read_key_file(dir.path()), None);
978 1016 }
979 1017
980 1018 #[test]
981 - fn load_api_key_whitespace_only_falls_back_to_bundled() {
1019 + fn read_key_file_whitespace_only_is_none() {
982 1020 let dir = tempfile::tempdir().unwrap();
983 1021 std::fs::write(dir.path().join("sync_api_key"), " \n ").unwrap();
984 - if std::env::var("AF_SYNC_API_KEY").is_err() {
985 - let key = load_api_key(dir.path());
986 - assert_eq!(key, parse_synckit_toml_key());
987 - }
1022 + assert_eq!(read_key_file(dir.path()), None);
988 1023 }
989 1024
990 1025 #[test]
991 - fn load_api_key_no_file_falls_back_to_bundled() {
1026 + fn read_key_file_missing_is_none() {
992 1027 let dir = tempfile::tempdir().unwrap();
993 - if std::env::var("AF_SYNC_API_KEY").is_err() {
994 - let key = load_api_key(dir.path());
995 - assert_eq!(key, parse_synckit_toml_key());
996 - }
1028 + assert_eq!(read_key_file(dir.path()), None);
997 1029 }
998 1030
999 1031 #[test]
1000 - fn save_api_key_creates_file() {
1001 - let dir = tempfile::tempdir().unwrap();
1002 - save_api_key(dir.path(), "saved-key");
1003 - let content = std::fs::read_to_string(dir.path().join("sync_api_key")).unwrap();
1004 - assert_eq!(content, "saved-key");
1032 + fn env_or_bundled_matches_bundled_without_env() {
1033 + if std::env::var("AF_SYNC_API_KEY").is_err() {
1034 + assert_eq!(api_key_from_env_or_bundled(), parse_synckit_toml_key());
1035 + }
1005 1036 }
1006 1037
1007 - #[test]
1008 - fn save_and_load_roundtrip() {
1009 - let dir = tempfile::tempdir().unwrap();
1010 - save_api_key(dir.path(), "roundtrip-key");
1011 - let result = load_api_key(dir.path());
1012 - assert_eq!(result, Some("roundtrip-key".to_string()));
1013 - }
1038 + // No save/load round-trip test: `save_api_key` writes to the real OS
1039 + // keychain under a fixed namespace, so exercising it in unit tests would
1040 + // pollute (or depend on) the developer's actual keychain. The migration and
1041 + // fallback ordering are covered by the `read_key_file` / env-or-bundled
1042 + // tests above.
1014 1043
1015 1044 // ── Initial screen resolution ──
1016 1045
M crates/audiofiles-browser/src/backend/direct.rs +8 -2
@@ -237,9 +237,15 @@ impl DirectBackend {
237 237 stem,
238 238 ctx,
239 239 ) {
240 - // Sanitize: strip path separators and NUL bytes to prevent traversal
240 + // Sanitize: strip path separators, NUL bytes, and a bare `..`
241 + // to prevent traversal. resolve_output_names re-sanitizes
242 + // overrides too, so this is a first layer, not the only one.
241 243 let safe_stem = new_stem.replace(['/', '\\', '\0'], "_");
242 - let safe_stem = if safe_stem.is_empty() { "untitled".to_string() } else { safe_stem };
244 + let safe_stem = if safe_stem.is_empty() || safe_stem == ".." || safe_stem == "." {
245 + "untitled".to_string()
246 + } else {
247 + safe_stem
248 + };
243 249 *name = if ext.is_empty() {
244 250 safe_stem
245 251 } else {
M crates/audiofiles-core/src/export/resolve.rs +93 -8
@@ -18,10 +18,12 @@ pub fn resolve_output_names(
18 18 config: &ExportConfig,
19 19 pattern: Option<&RenamePattern>,
20 20 ) -> Vec<String> {
21 - // If the backend pre-computed names (e.g. via transform_filename hook), use them directly.
21 + // If the backend pre-computed names (e.g. via transform_filename hook), use them.
22 + // These come from an untrusted Rhai plugin, so they get the SAME traversal
23 + // sanitization as computed names — never trust the caller to have done it.
22 24 if let Some(ref overrides) = config.name_overrides
23 25 && overrides.len() == items.len() {
24 - return overrides.clone();
26 + return overrides.iter().map(|n| strip_traversal(n)).collect();
25 27 }
26 28
27 29 let output_ext = match config.format {
@@ -98,12 +100,7 @@ pub fn resolve_output_names(
98 100 // Strip path separators and NUL bytes from all filenames to prevent path traversal,
99 101 // even when no NamingRules are configured.
100 102 for name in &mut names {
101 - *name = name.replace(['/', '\\', '\0'], "_");
102 - // Reject . and .. as bare stems (after extension split these would be just dots)
103 - let stem_part = name.split('.').next().unwrap_or("");
104 - if stem_part == ".." {
105 - *name = name.replacen("..", "_", 1);
106 - }
103 + *name = strip_traversal(name);
107 104 }
108 105
109 106 // Deduplicate: same-named files (from different VFS dirs, or after sanitization)
@@ -121,3 +118,91 @@ pub fn resolve_output_names(
121 118
122 119 names
123 120 }
121 +
122 + #[cfg(test)]
123 + mod tests {
124 + use super::*;
125 + use crate::export::{ExportChannels, ExportFormat};
126 + use std::path::PathBuf;
127 +
128 + fn item(name: &str) -> ExportItem {
129 + ExportItem {
130 + hash: crate::SampleHash::new("a".repeat(64)),
131 + ext: "wav".to_string(),
132 + relative_path: PathBuf::from(name),
133 + name: name.to_string(),
134 + bpm: None,
135 + musical_key: None,
136 + classification: None,
137 + duration: None,
138 + tags: Vec::new(),
139 + source_path: None,
140 + }
141 + }
142 +
143 + fn config_with_overrides(overrides: Vec<String>) -> ExportConfig {
144 + ExportConfig {
145 + format: ExportFormat::Original,
146 + sample_rate: None,
147 + bit_depth: None,
148 + channels: ExportChannels::Original,
149 + naming_pattern: None,
150 + flatten: true,
151 + metadata_sidecar: false,
152 + destination: PathBuf::from("/tmp/export"),
153 + device_profile: None,
154 + naming_rules: None,
155 + max_file_size_bytes: None,
156 + name_overrides: Some(overrides),
157 + }
158 + }
159 +
160 + #[test]
161 + fn override_names_are_sanitized_against_traversal() {
162 + // Untrusted plugin-supplied overrides must not escape the export dir.
163 + let items = vec![item("a.wav"), item("b.wav"), item("c.wav")];
164 + let overrides = vec![
165 + "../escape.wav".to_string(),
166 + "sub/dir.wav".to_string(),
167 + "..".to_string(),
168 + ];
169 + let out = resolve_output_names(&items, &config_with_overrides(overrides), None);
170 + assert_eq!(out.len(), 3);
171 + for name in &out {
172 + // With separators stripped, each name is a single path component
173 + // that cannot escape the export dir; a bare `.`/`..` is defused.
174 + assert!(!name.contains('/'), "no path separator: {name}");
175 + assert!(!name.contains('\\'), "no backslash: {name}");
176 + assert!(!name.contains('\0'), "no NUL: {name}");
177 + assert_ne!(name, ".", "bare . must be defused");
178 + assert_ne!(name, "..", "bare .. must be defused");
179 + }
180 + }
181 +
182 + #[test]
183 + fn strip_traversal_defuses_payloads() {
184 + assert_eq!(strip_traversal("a/b\\c\0d.wav"), "a_b_c_d.wav");
185 + assert_eq!(strip_traversal(".."), "__");
186 + assert_eq!(strip_traversal("."), "_");
187 + // Benign names with dots are left intact.
188 + assert_eq!(strip_traversal("kick..wav"), "kick..wav");
189 + assert_eq!(strip_traversal("../x.wav"), ".._x.wav");
190 + }
191 + }
192 +
193 + /// Neutralize path-traversal in a single output filename: strip separators and
194 + /// NUL bytes, and defuse a bare `..` stem. Applied to every output name —
195 + /// including untrusted plugin-supplied overrides — so the primitive is safe
196 + /// regardless of caller.
197 + fn strip_traversal(name: &str) -> String {
198 + // Strip separators and NUL first: with those gone, the name is a single
199 + // path component, so `dir.join(name)` cannot climb out of `dir`.
200 + let out = name.replace(['/', '\\', '\0'], "_");
201 + // The only remaining escape is a component that IS `.` or `..` — those
202 + // resolve to the current/parent directory on join. Defuse them; every other
203 + // string (including a benign `..foo` or `.._x.wav`) stays in-directory.
204 + if out == "." || out == ".." {
205 + return "_".repeat(out.len());
206 + }
207 + out
208 + }