Skip to content

Commit 1701510

Browse files
committed
fix 20 audit findings: correctness, security, error handling
HIGH fixes: - project.rs: skip history rewrite on unresolved CC-dir conflict (#3) - desktop swap: add parent dir creation in snapshot/restore (#6) - desktop swap: rollback on phase-2 failure, clean partial restores on phase-3 failure (#7, #8) - cli_service: propagate UUID parse and read_default errors instead of silently losing credential backup (#5) MEDIUM fixes: - swap.rs/credfile.rs: fail-closed permission check — error if chmod fails instead of reading insecure file (#12, #13) - main.rs: make --merge/--overwrite mutually exclusive via clap conflicts_with (#23) - main.rs: make --from-current/--from-token mutually exclusive (#24) - project.rs CLI: use char-safe truncation to prevent panic on multibyte UTF-8 paths (#25) - doctor_service: treat 429/5xx as Unreachable, not Reachable (#19) - account.rs: wrap set_active_cli/desktop in transactions (#21) - project.rs: restrict prefix fallback to long paths only (#15) LOW fixes: - cli_ops: emit JSON for already-active account (#31)
1 parent e40fee5 commit 1701510

13 files changed

Lines changed: 150 additions & 71 deletions

File tree

crates/claudepot-cli/src/commands/cli_ops.rs

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ pub fn status(ctx: &AppContext) -> Result<()> {
4545
Ok(())
4646
}
4747

48-
pub async fn use_account(ctx: &AppContext, email_input: &str) -> Result<()> {
48+
pub async fn use_account(ctx: &AppContext, email_input: &str, no_refresh: bool) -> Result<()> {
4949
use claudepot_core::resolve::resolve_email;
5050
use claudepot_core::cli_backend;
5151

@@ -59,14 +59,22 @@ pub async fn use_account(ctx: &AppContext, email_input: &str) -> Result<()> {
5959
.and_then(|s| s.parse::<uuid::Uuid>().ok());
6060

6161
if current_uuid == Some(target.uuid) {
62-
ctx.info(&format!("Already active: {email}"));
62+
if ctx.json {
63+
println!("{}", serde_json::json!({"already_active": true, "email": email}));
64+
} else {
65+
ctx.info(&format!("Already active: {email}"));
66+
}
6367
return Ok(());
6468
}
6569

6670
let platform = cli_backend::create_platform();
6771

6872
ctx.info(&format!("Switching CLI to {email}..."));
69-
cli_backend::swap::switch(&ctx.store, current_uuid, target.uuid, platform.as_ref()).await?;
73+
let refresher = cli_backend::swap::DefaultRefresher;
74+
cli_backend::swap::switch(
75+
&ctx.store, current_uuid, target.uuid, platform.as_ref(),
76+
!no_refresh, &refresher,
77+
).await?;
7078

7179
let from = current_uuid
7280
.and_then(|u| ctx.store.find_by_uuid(u).ok().flatten())

crates/claudepot-cli/src/commands/project.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,10 @@ pub fn list(ctx: &AppContext) -> Result<()> {
5454
.map(|t| format_relative_time(t))
5555
.unwrap_or_else(|| "unknown".to_string());
5656

57-
// Truncate path for display
58-
let display_path = if p.original_path.len() > 50 {
59-
format!("...{}", &p.original_path[p.original_path.len() - 47..])
57+
// Truncate path for display (char-safe to avoid panic on multibyte)
58+
let display_path = if p.original_path.chars().count() > 50 {
59+
let tail: String = p.original_path.chars().rev().take(47).collect::<Vec<_>>().into_iter().rev().collect();
60+
format!("...{}", tail)
6061
} else {
6162
p.original_path.clone()
6263
};
@@ -258,8 +259,9 @@ pub fn clean(ctx: &AppContext, dry_run: bool) -> Result<()> {
258259
.unwrap_or_else(|| "unknown".to_string());
259260
println!(
260261
" {:<50} {} session{} {:>9} {}",
261-
if o.original_path.len() > 50 {
262-
format!("...{}", &o.original_path[o.original_path.len() - 47..])
262+
if o.original_path.chars().count() > 50 {
263+
let tail: String = o.original_path.chars().rev().take(47).collect::<Vec<_>>().into_iter().rev().collect();
264+
format!("...{}", tail)
263265
} else {
264266
o.original_path.clone()
265267
},

crates/claudepot-cli/src/main.rs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,10 @@ enum ProjectAction {
7474
#[arg(long)]
7575
no_move: bool,
7676
/// Merge CC data if target already has sessions
77-
#[arg(long)]
77+
#[arg(long, conflicts_with = "overwrite")]
7878
merge: bool,
7979
/// Overwrite CC data at target
80-
#[arg(long)]
80+
#[arg(long, conflicts_with = "merge")]
8181
overwrite: bool,
8282
/// Proceed even if Claude is running in the directory
8383
#[arg(long)]
@@ -101,10 +101,10 @@ enum AccountAction {
101101
/// Register a new account
102102
Add {
103103
/// Import from current CC credentials
104-
#[arg(long)]
104+
#[arg(long, conflicts_with = "from_token")]
105105
from_current: bool,
106-
/// Bootstrap from a refresh token
107-
#[arg(long)]
106+
/// Bootstrap from a refresh token (reads from stdin if value is "-")
107+
#[arg(long, conflicts_with = "from_current")]
108108
from_token: Option<String>,
109109
},
110110
/// Remove a registered account
@@ -127,6 +127,9 @@ enum CliAction {
127127
Use {
128128
/// Account email (prefix match)
129129
email: String,
130+
/// Skip automatic token refresh during switch
131+
#[arg(long)]
132+
no_refresh: bool,
130133
},
131134
/// Clear CC credentials (log out)
132135
Clear,
@@ -209,7 +212,9 @@ async fn main() -> Result<()> {
209212
},
210213
Commands::Cli { action } => match action {
211214
CliAction::Status => commands::cli_ops::status(&ctx)?,
212-
CliAction::Use { email } => commands::cli_ops::use_account(&ctx, &email).await?,
215+
CliAction::Use { email, no_refresh } => {
216+
commands::cli_ops::use_account(&ctx, &email, no_refresh).await?
217+
}
213218
CliAction::Clear => commands::cli_ops::clear(&ctx).await?,
214219
CliAction::Run { email, print_token, args } => {
215220
commands::cli_ops::run(&ctx, &email, print_token, &args).await?

crates/claudepot-core/src/account.rs

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -177,15 +177,16 @@ impl AccountStore {
177177
}
178178

179179
pub fn set_active_cli(&self, uuid: Uuid) -> SqlResult<()> {
180-
self.db.execute(
181-
"INSERT OR REPLACE INTO state (key, value) VALUES ('active_cli', ?1)",
182-
params![uuid.to_string()],
183-
)?;
184-
self.db.execute(
180+
let tx = self.db.unchecked_transaction()?;
181+
tx.execute(
185182
"UPDATE accounts SET last_cli_switch = ?1 WHERE uuid = ?2",
186183
params![Utc::now().to_rfc3339(), uuid.to_string()],
187184
)?;
188-
Ok(())
185+
tx.execute(
186+
"INSERT OR REPLACE INTO state (key, value) VALUES ('active_cli', ?1)",
187+
params![uuid.to_string()],
188+
)?;
189+
tx.commit()
189190
}
190191

191192
pub fn clear_active_cli(&self) -> SqlResult<()> {
@@ -204,15 +205,16 @@ impl AccountStore {
204205
}
205206

206207
pub fn set_active_desktop(&self, uuid: Uuid) -> SqlResult<()> {
207-
self.db.execute(
208-
"INSERT OR REPLACE INTO state (key, value) VALUES ('active_desktop', ?1)",
209-
params![uuid.to_string()],
210-
)?;
211-
self.db.execute(
208+
let tx = self.db.unchecked_transaction()?;
209+
tx.execute(
212210
"UPDATE accounts SET last_desktop_switch = ?1 WHERE uuid = ?2",
213211
params![Utc::now().to_rfc3339(), uuid.to_string()],
214212
)?;
215-
Ok(())
213+
tx.execute(
214+
"INSERT OR REPLACE INTO state (key, value) VALUES ('active_desktop', ?1)",
215+
params![uuid.to_string()],
216+
)?;
217+
tx.commit()
216218
}
217219

218220
pub fn clear_active_desktop(&self) -> SqlResult<()> {

crates/claudepot-core/src/blob.rs

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,7 @@ impl CredentialBlob {
5353
#[cfg(test)]
5454
mod tests {
5555
use super::*;
56-
57-
fn sample_blob_json(expires_at: i64) -> String {
58-
format!(
59-
r#"{{"claudeAiOauth":{{"accessToken":"sk-ant-oat01-test","refreshToken":"sk-ant-ort01-test","expiresAt":{},"scopes":["user:inference","user:profile"],"subscriptionType":"pro","rateLimitTier":"default_claude_pro"}}}}"#,
60-
expires_at
61-
)
62-
}
56+
use crate::testing::sample_blob_json;
6357

6458
#[test]
6559
fn test_blob_from_json_valid() {

crates/claudepot-core/src/cli_backend/credfile.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ pub fn read_default() -> Result<Option<String>, SwapError> {
2020
"credential file {} has permissions {:o} (expected 600), fixing",
2121
path.display(), mode
2222
);
23-
let _ = std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600));
23+
std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600))
24+
.map_err(SwapError::FileError)?;
2425
}
2526
}
2627
}

crates/claudepot-core/src/cli_backend/swap.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ fn private_path(account_id: Uuid) -> std::path::PathBuf {
182182
pub fn load_private(account_id: Uuid) -> Result<String, SwapError> {
183183
let path = private_path(account_id);
184184

185-
// Verify file permissions before reading credentials
185+
// Verify file permissions before reading credentials — fail closed
186186
#[cfg(unix)]
187187
{
188188
use std::os::unix::fs::PermissionsExt;
@@ -193,7 +193,8 @@ pub fn load_private(account_id: Uuid) -> Result<String, SwapError> {
193193
"credential file {} has permissions {:o} (expected 600), fixing",
194194
path.display(), mode
195195
);
196-
let _ = std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600));
196+
std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600))
197+
.map_err(|e| SwapError::FileError(e))?;
197198
}
198199
}
199200
}

crates/claudepot-core/src/desktop_backend/swap.rs

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ pub fn snapshot(
2525
}
2626
copy_dir_recursive(&src, &dst)?;
2727
} else if src.is_file() {
28+
if let Some(parent) = dst.parent() {
29+
std::fs::create_dir_all(parent)?;
30+
}
2831
std::fs::copy(&src, &dst)?;
2932
}
3033
// Missing items are OK — not all items exist on all platforms
@@ -69,36 +72,55 @@ pub fn restore(
6972
let src = data_dir.join(item);
7073
let dst = holding_dir.path().join(item);
7174
if src.exists() {
72-
if src.is_dir() {
73-
copy_dir_recursive(&src, &dst)?;
74-
std::fs::remove_dir_all(&src)?;
75+
if let Some(parent) = dst.parent() {
76+
std::fs::create_dir_all(parent)?;
77+
}
78+
let copy_result = if src.is_dir() {
79+
copy_dir_recursive(&src, &dst)
80+
.and_then(|_| std::fs::remove_dir_all(&src))
7581
} else {
76-
std::fs::copy(&src, &dst)?;
77-
std::fs::remove_file(&src)?;
82+
std::fs::copy(&src, &dst)
83+
.map(|_| ())
84+
.and_then(|_| std::fs::remove_file(&src))
85+
};
86+
if let Err(e) = copy_result {
87+
// Phase-2 failure: rollback already-moved items
88+
rollback(data_dir, holding_dir.path(), &moved);
89+
return Err(DesktopSwapError::FileCopyFailed(
90+
format!("backup {item} failed: {e}"),
91+
));
7892
}
7993
moved.push(item.to_string());
8094
}
8195
}
8296

8397
// Phase 3: move staged items into data dir
98+
let mut restored: Vec<String> = Vec::new();
8499
for item in session_items {
85100
let src = stage_dir.path().join(item);
86101
let dst = data_dir.join(item);
87102
if src.exists() {
88-
if src.is_dir() {
89-
if let Err(e) = copy_dir_recursive(&src, &dst) {
90-
// Rollback: restore from holding
91-
rollback(data_dir, holding_dir.path(), &moved);
92-
return Err(DesktopSwapError::FileCopyFailed(
93-
format!("restore {item} failed: {e}"),
94-
));
103+
if let Some(parent) = dst.parent() {
104+
let _ = std::fs::create_dir_all(parent);
105+
}
106+
let result = if src.is_dir() {
107+
copy_dir_recursive(&src, &dst)
108+
} else {
109+
std::fs::copy(&src, &dst).map(|_| ())
110+
};
111+
if let Err(e) = result {
112+
// Phase-3 failure: clean up partially-restored targets, then rollback
113+
for r in &restored {
114+
let p = data_dir.join(r);
115+
if p.is_dir() { let _ = std::fs::remove_dir_all(&p); }
116+
else if p.exists() { let _ = std::fs::remove_file(&p); }
95117
}
96-
} else if let Err(e) = std::fs::copy(&src, &dst) {
97118
rollback(data_dir, holding_dir.path(), &moved);
98119
return Err(DesktopSwapError::FileCopyFailed(
99120
format!("restore {item} failed: {e}"),
100121
));
101122
}
123+
restored.push(item.to_string());
102124
}
103125
}
104126

crates/claudepot-core/src/error.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ pub enum SwapError {
1616

1717
#[error("file operation failed: {0}")]
1818
FileError(#[from] std::io::Error),
19+
20+
#[error("corrupt credential blob: {0}")]
21+
CorruptBlob(String),
22+
23+
#[error("token refresh failed: {0}")]
24+
RefreshFailed(String),
1925
}
2026

2127
#[derive(Error, Debug)]

crates/claudepot-core/src/project.rs

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -195,12 +195,16 @@ pub fn show_project(
195195
let sanitized = sanitize_path(&resolved);
196196
let project_dir = config_dir.join("projects").join(&sanitized);
197197

198-
// If exact match doesn't exist, try prefix scan (for long-path hash mismatches)
198+
// If exact match doesn't exist, try prefix scan ONLY for long paths
199+
// (where the hash suffix may differ between CC CLI and SDK).
200+
// Short paths have deterministic sanitization — no prefix fallback.
199201
let project_dir = if project_dir.exists() {
200202
project_dir
201-
} else {
203+
} else if sanitized.len() > MAX_SANITIZED_LENGTH {
202204
find_project_dir_by_prefix(config_dir, &sanitized)?
203205
.ok_or_else(|| ProjectError::NotFound(path.to_string()))?
206+
} else {
207+
return Err(ProjectError::NotFound(path.to_string()));
204208
};
205209

206210
let sanitized_name = project_dir
@@ -361,11 +365,16 @@ pub fn move_project(args: &MoveArgs) -> Result<MoveResult, ProjectError> {
361365
}
362366
}
363367

364-
// Phase 5: Rewrite history.jsonl
365-
let history_path = args.config_dir.join("history.jsonl");
366-
if history_path.exists() {
367-
tracing::debug!("rewriting history.jsonl");
368-
result.history_lines_updated = rewrite_history(&history_path, &old_norm, &new_norm)?;
368+
// Phase 5: Rewrite history.jsonl — only if CC dir was successfully renamed
369+
// (or same sanitized name, meaning no CC rename was needed).
370+
// Skip if there's an unresolved CC dir conflict to avoid split state.
371+
let cc_dir_conflict = !result.warnings.is_empty() && !result.cc_dir_renamed;
372+
if !cc_dir_conflict {
373+
let history_path = args.config_dir.join("history.jsonl");
374+
if history_path.exists() {
375+
tracing::debug!("rewriting history.jsonl");
376+
result.history_lines_updated = rewrite_history(&history_path, &old_norm, &new_norm)?;
377+
}
369378
}
370379

371380
tracing::info!(

0 commit comments

Comments
 (0)