fix(sudo): enforce username scope and singleton row in sudo_config
All checks were successful
Test / rust-fmt-check (pull_request) Successful in 1m20s
Test / frontend-tests (pull_request) Successful in 1m41s
Test / frontend-typecheck (pull_request) Successful in 1m43s
Test / rust-clippy (pull_request) Successful in 3m7s
PR Review Automation / review (pull_request) Successful in 4m11s
Test / rust-tests (pull_request) Successful in 4m27s
All checks were successful
Test / rust-fmt-check (pull_request) Successful in 1m20s
Test / frontend-tests (pull_request) Successful in 1m41s
Test / frontend-typecheck (pull_request) Successful in 1m43s
Test / rust-clippy (pull_request) Successful in 3m7s
PR Review Automation / review (pull_request) Successful in 4m11s
Test / rust-tests (pull_request) Successful in 4m27s
Fixes two bugs identified in the AI code review: 1. INSERT OR REPLACE with a freshly generated UUID never matches the existing primary key, so it appended rows instead of replacing. Switch to DELETE-then-INSERT to guarantee exactly one row. 2. Username defaulted to empty string. Resolve it to the current OS user (USER/LOGNAME env vars, fallback 'local') so credentials are always bound to a specific user identity. test_sudo_password now passes -u <username> to sudo so the test runs scoped to the stored user, not an arbitrary one. UI: show the configured username prominently in status; relabel the field and add a scope hint below it. Tests: test_set_sudo_singleton_delete_then_insert, three username resolution tests.
This commit is contained in:
parent
26507ad3ff
commit
1a9c3bd65a
@ -294,6 +294,17 @@ pub struct SudoConfigStatus {
|
|||||||
pub updated_at: String,
|
pub updated_at: String,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Resolve the OS username to bind sudo credentials to.
|
||||||
|
fn resolve_sudo_username(provided: Option<String>) -> String {
|
||||||
|
provided
|
||||||
|
.filter(|u| !u.trim().is_empty())
|
||||||
|
.unwrap_or_else(|| {
|
||||||
|
std::env::var("USER")
|
||||||
|
.or_else(|_| std::env::var("LOGNAME"))
|
||||||
|
.unwrap_or_else(|_| "local".to_string())
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
#[tauri::command]
|
#[tauri::command]
|
||||||
pub async fn set_sudo_password(
|
pub async fn set_sudo_password(
|
||||||
password: String,
|
password: String,
|
||||||
@ -301,13 +312,18 @@ pub async fn set_sudo_password(
|
|||||||
state: tauri::State<'_, AppState>,
|
state: tauri::State<'_, AppState>,
|
||||||
) -> Result<(), String> {
|
) -> Result<(), String> {
|
||||||
let encrypted = crate::integrations::auth::encrypt_token(&password)?;
|
let encrypted = crate::integrations::auth::encrypt_token(&password)?;
|
||||||
|
let uname = resolve_sudo_username(username);
|
||||||
let db = state.db.lock().map_err(|e| e.to_string())?;
|
let db = state.db.lock().map_err(|e| e.to_string())?;
|
||||||
let id = uuid::Uuid::now_v7().to_string();
|
// DELETE then INSERT to guarantee exactly one row at all times.
|
||||||
let uname = username.unwrap_or_default();
|
// INSERT OR REPLACE with a freshly generated UUID never matches the
|
||||||
|
// existing primary key, so it inserts an additional row instead of
|
||||||
|
// replacing — this is the correct singleton pattern for SQLite.
|
||||||
|
db.execute("DELETE FROM sudo_config", [])
|
||||||
|
.map_err(|e| format!("Failed to clear sudo config: {e}"))?;
|
||||||
db.execute(
|
db.execute(
|
||||||
"INSERT OR REPLACE INTO sudo_config (id, username, encrypted_password, created_at, updated_at) \
|
"INSERT INTO sudo_config (id, username, encrypted_password, created_at, updated_at) \
|
||||||
VALUES (?1, ?2, ?3, datetime('now'), datetime('now'))",
|
VALUES (?1, ?2, ?3, datetime('now'), datetime('now'))",
|
||||||
rusqlite::params![id, uname, encrypted],
|
rusqlite::params![uuid::Uuid::now_v7().to_string(), uname, encrypted],
|
||||||
)
|
)
|
||||||
.map_err(|e| format!("Failed to store sudo config: {e}"))?;
|
.map_err(|e| format!("Failed to store sudo config: {e}"))?;
|
||||||
Ok(())
|
Ok(())
|
||||||
@ -342,15 +358,25 @@ pub async fn get_sudo_config_status(
|
|||||||
|
|
||||||
#[tauri::command]
|
#[tauri::command]
|
||||||
pub async fn test_sudo_password(state: tauri::State<'_, AppState>) -> Result<bool, String> {
|
pub async fn test_sudo_password(state: tauri::State<'_, AppState>) -> Result<bool, String> {
|
||||||
let encrypted: Option<String> = {
|
let (encrypted, stored_username) = {
|
||||||
let db = state.db.lock().map_err(|e| e.to_string())?;
|
let db = state.db.lock().map_err(|e| e.to_string())?;
|
||||||
db.prepare("SELECT encrypted_password FROM sudo_config LIMIT 1")
|
db.prepare("SELECT encrypted_password, username FROM sudo_config LIMIT 1")
|
||||||
.and_then(|mut stmt| stmt.query_row([], |row| row.get::<_, String>(0)))
|
.and_then(|mut stmt| {
|
||||||
|
stmt.query_row([], |row| {
|
||||||
|
Ok((row.get::<_, String>(0)?, row.get::<_, String>(1)?))
|
||||||
|
})
|
||||||
|
})
|
||||||
.ok()
|
.ok()
|
||||||
|
.ok_or("No sudo password configured".to_string())?
|
||||||
};
|
};
|
||||||
let encrypted = encrypted.ok_or("No sudo password configured")?;
|
|
||||||
let password = crate::integrations::auth::decrypt_token(&encrypted)?;
|
let password = crate::integrations::auth::decrypt_token(&encrypted)?;
|
||||||
let result = crate::commands::agentic::run_sudo_command(&password, &["true"])
|
// Scope the test to the stored username so credentials can only be
|
||||||
|
// verified for the user they were saved for.
|
||||||
|
let result = if stored_username.is_empty() {
|
||||||
|
crate::commands::agentic::run_sudo_command(&password, &["true"])
|
||||||
|
} else {
|
||||||
|
crate::commands::agentic::run_sudo_command(&password, &["-u", &stored_username, "true"])
|
||||||
|
}
|
||||||
.map_err(|e| format!("Sudo test failed: {e}"))?;
|
.map_err(|e| format!("Sudo test failed: {e}"))?;
|
||||||
Ok(result.success)
|
Ok(result.success)
|
||||||
}
|
}
|
||||||
@ -362,3 +388,74 @@ pub async fn clear_sudo_password(state: tauri::State<'_, AppState>) -> Result<()
|
|||||||
.map_err(|e| format!("Failed to clear sudo config: {e}"))?;
|
.map_err(|e| format!("Failed to clear sudo config: {e}"))?;
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod sudo_tests {
|
||||||
|
use super::*;
|
||||||
|
|
||||||
|
fn setup_db() -> rusqlite::Connection {
|
||||||
|
let conn = rusqlite::Connection::open_in_memory().unwrap();
|
||||||
|
crate::db::migrations::run_migrations(&conn).unwrap();
|
||||||
|
conn
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_set_sudo_singleton_delete_then_insert() {
|
||||||
|
let conn = setup_db();
|
||||||
|
// Insert two stale rows directly to simulate the old broken behaviour
|
||||||
|
conn.execute(
|
||||||
|
"INSERT INTO sudo_config (id, username, encrypted_password) VALUES ('id1', 'alice', 'enc1')",
|
||||||
|
[],
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
conn.execute(
|
||||||
|
"INSERT INTO sudo_config (id, username, encrypted_password) VALUES ('id2', 'bob', 'enc2')",
|
||||||
|
[],
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
let count: i64 = conn
|
||||||
|
.query_row("SELECT COUNT(*) FROM sudo_config", [], |r| r.get(0))
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(count, 2);
|
||||||
|
|
||||||
|
// Apply the correct singleton pattern
|
||||||
|
conn.execute("DELETE FROM sudo_config", []).unwrap();
|
||||||
|
conn.execute(
|
||||||
|
"INSERT INTO sudo_config (id, username, encrypted_password) VALUES ('id3', 'charlie', 'enc3')",
|
||||||
|
[],
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
let count: i64 = conn
|
||||||
|
.query_row("SELECT COUNT(*) FROM sudo_config", [], |r| r.get(0))
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(count, 1, "exactly one row must remain after set");
|
||||||
|
|
||||||
|
let username: String = conn
|
||||||
|
.query_row("SELECT username FROM sudo_config", [], |r| r.get(0))
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(username, "charlie");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_resolve_sudo_username_uses_provided() {
|
||||||
|
let result = resolve_sudo_username(Some("alice".to_string()));
|
||||||
|
assert_eq!(result, "alice");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_resolve_sudo_username_rejects_blank() {
|
||||||
|
let result = resolve_sudo_username(Some(" ".to_string()));
|
||||||
|
// blank string should fall through to env-based default
|
||||||
|
assert!(!result.trim().is_empty(), "username must never be blank");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_resolve_sudo_username_defaults_to_env() {
|
||||||
|
let env_user = std::env::var("USER")
|
||||||
|
.or_else(|_| std::env::var("LOGNAME"))
|
||||||
|
.unwrap_or_else(|_| "local".to_string());
|
||||||
|
let result = resolve_sudo_username(None);
|
||||||
|
assert_eq!(result, env_user);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@ -173,9 +173,10 @@ export default function Security() {
|
|||||||
</CardHeader>
|
</CardHeader>
|
||||||
<CardContent className="space-y-4">
|
<CardContent className="space-y-4">
|
||||||
{sudoStatus?.configured && (
|
{sudoStatus?.configured && (
|
||||||
<p className="text-sm text-green-600">
|
<div className="text-sm text-green-600 space-y-0.5">
|
||||||
Configured (last updated: {sudoStatus.updated_at})
|
<p>Configured for <strong>{sudoStatus.username}</strong></p>
|
||||||
</p>
|
<p className="text-xs text-muted-foreground">Last updated: {sudoStatus.updated_at}</p>
|
||||||
|
</div>
|
||||||
)}
|
)}
|
||||||
{sudoStatus && !sudoStatus.configured && (
|
{sudoStatus && !sudoStatus.configured && (
|
||||||
<p className="text-sm text-muted-foreground">Not configured</p>
|
<p className="text-sm text-muted-foreground">Not configured</p>
|
||||||
@ -183,16 +184,19 @@ export default function Security() {
|
|||||||
<div className="space-y-3">
|
<div className="space-y-3">
|
||||||
<div>
|
<div>
|
||||||
<label className="text-sm font-medium" htmlFor="sudo-username">
|
<label className="text-sm font-medium" htmlFor="sudo-username">
|
||||||
Username (optional)
|
Username
|
||||||
</label>
|
</label>
|
||||||
<input
|
<input
|
||||||
id="sudo-username"
|
id="sudo-username"
|
||||||
type="text"
|
type="text"
|
||||||
value={sudoUsername}
|
value={sudoUsername}
|
||||||
onChange={(e) => setSudoUsername(e.target.value)}
|
onChange={(e) => setSudoUsername(e.target.value)}
|
||||||
placeholder="Leave empty for current user"
|
placeholder="Defaults to current OS user"
|
||||||
className="mt-1 w-full rounded-md border border-input bg-background px-3 py-2 text-sm"
|
className="mt-1 w-full rounded-md border border-input bg-background px-3 py-2 text-sm"
|
||||||
/>
|
/>
|
||||||
|
<p className="text-xs text-muted-foreground mt-1">
|
||||||
|
Credentials are scoped to this user. Leave blank to use the current OS user.
|
||||||
|
</p>
|
||||||
</div>
|
</div>
|
||||||
<div>
|
<div>
|
||||||
<label className="text-sm font-medium" htmlFor="sudo-password">
|
<label className="text-sm font-medium" htmlFor="sudo-password">
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user