Validate settings imports before writing secrets; disclose base URLs
Secret Scan / scan (push) Successful in 14s
Build App (Preview) / compute-version (pull_request) Successful in 7s
Secret Scan / scan (pull_request) Successful in 6s
Build App (Preview) / create-release (pull_request) Successful in 2s
Build App (Preview) / build-macos (pull_request) Successful in 2m43s
Build App (Preview) / build-windows (pull_request) Successful in 4m59s
Build App (Preview) / build-linux (pull_request) Successful in 7m29s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
Secret Scan / scan (push) Successful in 14s
Build App (Preview) / compute-version (pull_request) Successful in 7s
Secret Scan / scan (pull_request) Successful in 6s
Build App (Preview) / create-release (pull_request) Successful in 2s
Build App (Preview) / build-macos (pull_request) Successful in 2m43s
Build App (Preview) / build-windows (pull_request) Successful in 4m59s
Build App (Preview) / build-linux (pull_request) Successful in 7m29s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
A rejected import (bad env var name, disallowed host path) used to leave keychain secrets already overwritten while the settings themselves stayed unchanged. apply_settings_import now runs update_settings's validation (extracted into validate_settings_update) before any secret write. Also from this review round: sharpened two format-version tests that previously passed against the pre-fix code too, added a direct test for split_settings_and_secrets, warned on a dormant web terminal token even when the terminal import leaves it off, matched the password-length check to the frontend's unit of measure, zeroized the export plaintext buffer, and surfaced non-blank Ollama/llama.cpp/OpenAI-compatible/gateway base URLs in the import preview so a traffic redirect isn't silent.
This commit is contained in:
@@ -601,6 +601,15 @@ deliberately out of scope — this is not a project backup.
|
|||||||
gateway key). Secrets are restored *before* the settings replace runs, not after — replacing
|
gateway key). Secrets are restored *before* the settings replace runs, not after — replacing
|
||||||
settings is what triggers `reconcile_gateway`, and restoring the other way round leaves a real
|
settings is what triggers `reconcile_gateway`, and restoring the other way round leaves a real
|
||||||
window where a gateway recreation happens against the destination's old keys.
|
window where a gateway recreation happens against the destination's old keys.
|
||||||
|
- **The imported settings are validated *before* any secret is written, not just before the
|
||||||
|
settings replace.** `apply_settings_import` calls
|
||||||
|
`settings_commands::validate_settings_update(¤t, &settings)` — the same checks
|
||||||
|
`update_settings` runs internally, pulled out into its own function specifically so this caller
|
||||||
|
can run them first — and only proceeds to the three keychain writes if that passes. A review
|
||||||
|
caught the earlier ordering: writing secrets first meant a rejected import (a bad env var name, a
|
||||||
|
disallowed host path) still left the keychain overwritten with the file's secrets while the
|
||||||
|
settings themselves stayed unchanged, a silently half-applied state the error message gave no
|
||||||
|
hint of.
|
||||||
- **`read_and_decrypt` checks `format_version` before attempting to parse the full payload, not
|
- **`read_and_decrypt` checks `format_version` before attempting to parse the full payload, not
|
||||||
after.** A version bump that isn't deserialize-compatible is exactly the case that check exists
|
after.** A version bump that isn't deserialize-compatible is exactly the case that check exists
|
||||||
for, and parsing the full struct first would fail on the shape mismatch before the version check
|
for, and parsing the full struct first would fail on the shape mismatch before the version check
|
||||||
@@ -610,10 +619,21 @@ deliberately out of scope — this is not a project backup.
|
|||||||
- **The 8-character password minimum is enforced in `export_settings` itself, not only in the
|
- **The 8-character password minimum is enforced in `export_settings` itself, not only in the
|
||||||
export modal.** The frontend minimum is a UX nudge; the Rust command is the actual boundary a
|
export modal.** The frontend minimum is a UX nudge; the Rust command is the actual boundary a
|
||||||
weak password has to cross, and Argon2id's memory-hardness buys little against an attacker who
|
weak password has to cross, and Argon2id's memory-hardness buys little against an attacker who
|
||||||
can just try a short password directly. The derived key and the decrypted plaintext are both
|
can just try a short password directly. Measured with `.chars().count()` (Unicode scalar values)
|
||||||
wrapped in `zeroize::Zeroizing` for the same reason every other secret in this codebase gets
|
rather than `.len()` (bytes), to stay as close as this pair of languages allows to the frontend's
|
||||||
handled carefully — cheap insurance (`zeroize` is already pulled in transitively via `aes-gcm`)
|
`.length` check (UTF-16 code units) — the two only diverge on astral-plane characters. The
|
||||||
for material that exists only to hold or produce live credentials.
|
derived key and both plaintext buffers — the payload built for export, and whatever `decrypt`
|
||||||
|
recovers on import — are wrapped in `zeroize::Zeroizing` for the same reason every other secret
|
||||||
|
in this codebase gets handled carefully — cheap insurance (`zeroize` is already pulled in
|
||||||
|
transitively via `aes-gcm`) for material that exists only to hold or produce live credentials.
|
||||||
|
- **The preview also discloses non-blank custom base URLs** (`global_ollama`, `global_llamacpp`,
|
||||||
|
`global_openai_compatible`, `gateway.api_base`) so an import that would redirect model traffic to
|
||||||
|
a different server is visible in the confirmation dialog rather than discovered later — these are
|
||||||
|
endpoints, not secrets, so `SettingsImportPreview` carries and `describeImport` renders the actual
|
||||||
|
URL rather than just a presence flag. `describeImportWarnings` additionally calls out a web
|
||||||
|
terminal token that arrives with the terminal left *off*: `start_web_terminal` only mints a fresh
|
||||||
|
token when none is already set, so a planted token would otherwise activate silently the next
|
||||||
|
time someone turns the terminal on, with no import-time signal that it wasn't freshly generated.
|
||||||
|
|
||||||
## Testing
|
## Testing
|
||||||
|
|
||||||
|
|||||||
@@ -10,19 +10,24 @@ pub async fn get_settings(state: State<'_, AppState>) -> Result<AppSettings, Str
|
|||||||
Ok(state.settings_store.get())
|
Ok(state.settings_store.get())
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tauri::command]
|
/// Everything `update_settings` refuses a save over, run against the store's
|
||||||
pub async fn update_settings(
|
/// *current* value and the incoming one.
|
||||||
settings: AppSettings,
|
///
|
||||||
state: State<'_, AppState>,
|
/// Pulled out so a caller that does other, harder-to-undo work alongside a
|
||||||
) -> Result<AppSettings, String> {
|
/// settings save — `settings_export_commands::apply_settings_import`
|
||||||
let before = state.settings_store.get();
|
/// restores three keychain secrets in the same command — can run this
|
||||||
|
/// *first* and bail before touching anything, rather than discovering the
|
||||||
|
/// rejection only when `update_settings` itself runs partway through.
|
||||||
|
pub fn validate_settings_update(
|
||||||
|
before: &AppSettings,
|
||||||
|
incoming: &AppSettings,
|
||||||
|
) -> Result<(), String> {
|
||||||
// The global half of the same rule the project half gets in
|
// The global half of the same rule the project half gets in
|
||||||
// `update_project`: a global custom env var is merged into every project's
|
// `update_project`: a global custom env var is merged into every project's
|
||||||
// container environment, so an unchecked name here reaches all of them.
|
// container environment, so an unchecked name here reaches all of them.
|
||||||
crate::models::validate_env_vars_update(
|
crate::models::validate_env_vars_update(
|
||||||
&before.global_custom_env_vars,
|
&before.global_custom_env_vars,
|
||||||
&settings.global_custom_env_vars,
|
&incoming.global_custom_env_vars,
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
// The same for the two host paths this struct owns. `update_project`
|
// The same for the two host paths this struct owns. `update_project`
|
||||||
@@ -40,14 +45,26 @@ pub async fn update_settings(
|
|||||||
crate::commands::project_commands::validate_mounted_host_path(
|
crate::commands::project_commands::validate_mounted_host_path(
|
||||||
"SSH key path",
|
"SSH key path",
|
||||||
before.default_ssh_key_path.as_deref(),
|
before.default_ssh_key_path.as_deref(),
|
||||||
settings.default_ssh_key_path.as_deref(),
|
incoming.default_ssh_key_path.as_deref(),
|
||||||
)?;
|
)?;
|
||||||
crate::commands::project_commands::validate_mounted_host_path(
|
crate::commands::project_commands::validate_mounted_host_path(
|
||||||
"CA certificate path",
|
"CA certificate path",
|
||||||
before.ca_cert_path.as_deref(),
|
before.ca_cert_path.as_deref(),
|
||||||
settings.ca_cert_path.as_deref(),
|
incoming.ca_cert_path.as_deref(),
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tauri::command]
|
||||||
|
pub async fn update_settings(
|
||||||
|
settings: AppSettings,
|
||||||
|
state: State<'_, AppState>,
|
||||||
|
) -> Result<AppSettings, String> {
|
||||||
|
let before = state.settings_store.get();
|
||||||
|
|
||||||
|
validate_settings_update(&before, &settings)?;
|
||||||
|
|
||||||
let saved = state.settings_store.update(settings)?;
|
let saved = state.settings_store.update(settings)?;
|
||||||
|
|
||||||
// Persisting a setting is not the same as applying it. The gateway is the
|
// Persisting a setting is not the same as applying it. The gateway is the
|
||||||
@@ -122,7 +139,10 @@ async fn reconcile_gateway(before: &GatewaySettings, after: &GatewaySettings) {
|
|||||||
GatewayAction::StopIfRunning => {
|
GatewayAction::StopIfRunning => {
|
||||||
log::info!("Model gateway disabled in settings — stopping the container");
|
log::info!("Model gateway disabled in settings — stopping the container");
|
||||||
if let Err(e) = docker::gateway::stop_gateway_container().await {
|
if let Err(e) = docker::gateway::stop_gateway_container().await {
|
||||||
log::error!("Failed to stop the model gateway after it was disabled: {}", e);
|
log::error!(
|
||||||
|
"Failed to stop the model gateway after it was disabled: {}",
|
||||||
|
e
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
GatewayAction::RestartIfRunning => {
|
GatewayAction::RestartIfRunning => {
|
||||||
@@ -138,10 +158,7 @@ async fn reconcile_gateway(before: &GatewaySettings, after: &GatewaySettings) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[tauri::command]
|
#[tauri::command]
|
||||||
pub async fn pull_image(
|
pub async fn pull_image(image_name: String, app_handle: tauri::AppHandle) -> Result<(), String> {
|
||||||
image_name: String,
|
|
||||||
app_handle: tauri::AppHandle,
|
|
||||||
) -> Result<(), String> {
|
|
||||||
use tauri::Emitter;
|
use tauri::Emitter;
|
||||||
docker::pull_image(&image_name, move |msg| {
|
docker::pull_image(&image_name, move |msg| {
|
||||||
let _ = app_handle.emit("image-pull-progress", msg);
|
let _ = app_handle.emit("image-pull-progress", msg);
|
||||||
@@ -334,7 +351,10 @@ mod tests {
|
|||||||
let before = enabled_gateway();
|
let before = enabled_gateway();
|
||||||
let mut after = before.clone();
|
let mut after = before.clone();
|
||||||
after.enabled = false;
|
after.enabled = false;
|
||||||
assert_eq!(gateway_action(&before, &after), GatewayAction::StopIfRunning);
|
assert_eq!(
|
||||||
|
gateway_action(&before, &after),
|
||||||
|
GatewayAction::StopIfRunning
|
||||||
|
);
|
||||||
// Still true when it was already off — a stray running container is
|
// Still true when it was already off — a stray running container is
|
||||||
// still a container that shouldn't be up.
|
// still a container that shouldn't be up.
|
||||||
assert_eq!(gateway_action(&after, &after), GatewayAction::StopIfRunning);
|
assert_eq!(gateway_action(&after, &after), GatewayAction::StopIfRunning);
|
||||||
|
|||||||
@@ -33,6 +33,7 @@ use std::path::{Path, PathBuf};
|
|||||||
|
|
||||||
use tauri::State;
|
use tauri::State;
|
||||||
use tauri_plugin_dialog::DialogExt;
|
use tauri_plugin_dialog::DialogExt;
|
||||||
|
use zeroize::Zeroizing;
|
||||||
|
|
||||||
use crate::models::{
|
use crate::models::{
|
||||||
AppSettings, ExportedSecrets, SettingsExportPayload, SettingsImportPreview,
|
AppSettings, ExportedSecrets, SettingsExportPayload, SettingsImportPreview,
|
||||||
@@ -122,7 +123,11 @@ pub async fn export_settings(
|
|||||||
window: tauri::Window,
|
window: tauri::Window,
|
||||||
state: State<'_, AppState>,
|
state: State<'_, AppState>,
|
||||||
) -> Result<bool, String> {
|
) -> Result<bool, String> {
|
||||||
if password.len() < MIN_PASSWORD_LEN {
|
// `.chars().count()` — Unicode scalar values, not bytes — to stay as
|
||||||
|
// close as this pair of languages allows to the frontend's `.length`
|
||||||
|
// check (UTF-16 code units); the two only diverge on astral-plane
|
||||||
|
// characters, which no reasonable password touches.
|
||||||
|
if password.chars().count() < MIN_PASSWORD_LEN {
|
||||||
return Err(format!(
|
return Err(format!(
|
||||||
"Use a password of at least {} characters.",
|
"Use a password of at least {} characters.",
|
||||||
MIN_PASSWORD_LEN
|
MIN_PASSWORD_LEN
|
||||||
@@ -146,8 +151,10 @@ pub async fn export_settings(
|
|||||||
secrets,
|
secrets,
|
||||||
};
|
};
|
||||||
|
|
||||||
let plaintext = serde_json::to_vec(&payload)
|
let plaintext = Zeroizing::new(
|
||||||
.map_err(|e| format!("Failed to prepare settings for export: {}", e))?;
|
serde_json::to_vec(&payload)
|
||||||
|
.map_err(|e| format!("Failed to prepare settings for export: {}", e))?,
|
||||||
|
);
|
||||||
let encrypted = settings_crypto::encrypt(&plaintext, &password)?;
|
let encrypted = settings_crypto::encrypt(&plaintext, &password)?;
|
||||||
|
|
||||||
std::fs::write(&dest, &encrypted).map_err(|e| format!("Failed to write export file: {}", e))?;
|
std::fs::write(&dest, &encrypted).map_err(|e| format!("Failed to write export file: {}", e))?;
|
||||||
@@ -200,18 +207,33 @@ pub async fn preview_settings_import(
|
|||||||
/// on import." A user who wants to clear a secret already has dedicated UI
|
/// on import." A user who wants to clear a secret already has dedicated UI
|
||||||
/// for that (signing out of shared auth, clearing the gateway key).
|
/// for that (signing out of shared auth, clearing the gateway key).
|
||||||
///
|
///
|
||||||
/// Order matters here: secrets are restored **before** the settings replace
|
/// Order matters here, twice over.
|
||||||
/// runs (which is what triggers `reconcile_gateway`), so a gateway
|
|
||||||
/// recreation that replace provokes sees the final key material rather than
|
|
||||||
/// racing it — restoring the other way round left a real window where the
|
|
||||||
/// running gateway and the keychain briefly disagreed.
|
|
||||||
///
|
///
|
||||||
/// The pending path is only cleared on success. A failure here (a rejected
|
/// First: the imported settings are **validated before any secret is
|
||||||
/// host path, a keychain write failure surfaced some other way) leaves the
|
/// written**, using the same checks `update_settings` itself runs
|
||||||
/// import pending so the frontend can let the user retry `apply` without
|
/// (`settings_commands::validate_settings_update`). Restoring a secret is
|
||||||
/// making them pick the file and re-enter the password again — the
|
/// hard to undo unnoticed — a stale env-var-name rejection or a disallowed
|
||||||
/// preview's job was confirming *what* to import, not spending the one
|
/// host path used to be caught only when `update_settings` ran, by which
|
||||||
/// attempt at applying it.
|
/// point the three keychain secrets below were already overwritten with the
|
||||||
|
/// file's, each with a fresh rotation id, silently flagging every project
|
||||||
|
/// container for recreation — while the error the user saw talked only
|
||||||
|
/// about the rejected setting and said nothing about the credentials that
|
||||||
|
/// had already moved. Failing this check first makes a rejected import
|
||||||
|
/// leave nothing touched, matching what "the import failed" is supposed to
|
||||||
|
/// mean.
|
||||||
|
///
|
||||||
|
/// Second, among the things that *do* get written: secrets are restored
|
||||||
|
/// **before** the settings replace runs (which is what triggers
|
||||||
|
/// `reconcile_gateway`), so a gateway recreation that replace provokes sees
|
||||||
|
/// the final key material rather than racing it — restoring the other way
|
||||||
|
/// round left a real window where the running gateway and the keychain
|
||||||
|
/// briefly disagreed.
|
||||||
|
///
|
||||||
|
/// The pending path is only cleared on success. A failure here (rejected by
|
||||||
|
/// the validation above, or some other error) leaves the import pending so
|
||||||
|
/// the frontend can let the user retry `apply` without making them pick the
|
||||||
|
/// file and re-enter the password again — the preview's job was confirming
|
||||||
|
/// *what* to import, not spending the one attempt at applying it.
|
||||||
#[tauri::command]
|
#[tauri::command]
|
||||||
pub async fn apply_settings_import(
|
pub async fn apply_settings_import(
|
||||||
password: String,
|
password: String,
|
||||||
@@ -230,21 +252,7 @@ pub async fn apply_settings_import(
|
|||||||
|
|
||||||
let payload = read_and_decrypt(&path, &password)?;
|
let payload = read_and_decrypt(&path, &password)?;
|
||||||
|
|
||||||
if let Some(token) = non_blank(payload.secrets.claude_oauth_token) {
|
let current = state.settings_store.get();
|
||||||
if let Err(e) = secure::store_claude_oauth_token(&token) {
|
|
||||||
log::warn!("Settings import: could not restore the shared Claude login: {}", e);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
if let Some(key) = non_blank(payload.secrets.gateway_api_key) {
|
|
||||||
if let Err(e) = secure::store_gateway_api_key(&key) {
|
|
||||||
log::warn!("Settings import: could not restore the gateway provider API key: {}", e);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
if let Some(key) = non_blank(payload.secrets.gateway_master_key) {
|
|
||||||
if let Err(e) = secure::store_gateway_master_key(&key) {
|
|
||||||
log::warn!("Settings import: could not restore the gateway master key: {}", e);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// The web-terminal token lives inside `AppSettings` itself rather than
|
// The web-terminal token lives inside `AppSettings` itself rather than
|
||||||
// the keychain, so "leave an absent secret alone" has to be done by
|
// the keychain, so "leave an absent secret alone" has to be done by
|
||||||
@@ -254,9 +262,37 @@ pub async fn apply_settings_import(
|
|||||||
// `split_settings_and_secrets`).
|
// `split_settings_and_secrets`).
|
||||||
let mut settings = payload.settings;
|
let mut settings = payload.settings;
|
||||||
settings.web_terminal.access_token = non_blank(payload.secrets.web_terminal_access_token)
|
settings.web_terminal.access_token = non_blank(payload.secrets.web_terminal_access_token)
|
||||||
.or_else(|| state.settings_store.get().web_terminal.access_token);
|
.or_else(|| current.web_terminal.access_token.clone());
|
||||||
|
|
||||||
let saved = crate::commands::settings_commands::update_settings(settings, state.clone()).await?;
|
crate::commands::settings_commands::validate_settings_update(¤t, &settings)?;
|
||||||
|
|
||||||
|
if let Some(token) = non_blank(payload.secrets.claude_oauth_token) {
|
||||||
|
if let Err(e) = secure::store_claude_oauth_token(&token) {
|
||||||
|
log::warn!(
|
||||||
|
"Settings import: could not restore the shared Claude login: {}",
|
||||||
|
e
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if let Some(key) = non_blank(payload.secrets.gateway_api_key) {
|
||||||
|
if let Err(e) = secure::store_gateway_api_key(&key) {
|
||||||
|
log::warn!(
|
||||||
|
"Settings import: could not restore the gateway provider API key: {}",
|
||||||
|
e
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if let Some(key) = non_blank(payload.secrets.gateway_master_key) {
|
||||||
|
if let Err(e) = secure::store_gateway_master_key(&key) {
|
||||||
|
log::warn!(
|
||||||
|
"Settings import: could not restore the gateway master key: {}",
|
||||||
|
e
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
let saved =
|
||||||
|
crate::commands::settings_commands::update_settings(settings, state.clone()).await?;
|
||||||
|
|
||||||
state.pending_settings_import.lock().await.take();
|
state.pending_settings_import.lock().await.take();
|
||||||
|
|
||||||
@@ -289,7 +325,8 @@ struct FormatVersionProbe {
|
|||||||
/// so neither error path below ever interpolates what `serde_json`
|
/// so neither error path below ever interpolates what `serde_json`
|
||||||
/// actually says — only a fixed, generic message.
|
/// actually says — only a fixed, generic message.
|
||||||
fn read_and_decrypt(path: &Path, password: &str) -> Result<SettingsExportPayload, String> {
|
fn read_and_decrypt(path: &Path, password: &str) -> Result<SettingsExportPayload, String> {
|
||||||
let encrypted = std::fs::read(path).map_err(|e| format!("Failed to read export file: {}", e))?;
|
let encrypted =
|
||||||
|
std::fs::read(path).map_err(|e| format!("Failed to read export file: {}", e))?;
|
||||||
let plaintext = settings_crypto::decrypt(&encrypted, password)?;
|
let plaintext = settings_crypto::decrypt(&encrypted, password)?;
|
||||||
|
|
||||||
let probe: FormatVersionProbe = serde_json::from_slice(&plaintext)
|
let probe: FormatVersionProbe = serde_json::from_slice(&plaintext)
|
||||||
@@ -302,8 +339,9 @@ fn read_and_decrypt(path: &Path, password: &str) -> Result<SettingsExportPayload
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
serde_json::from_slice(&plaintext)
|
serde_json::from_slice(&plaintext).map_err(|_| {
|
||||||
.map_err(|_| "This file doesn't look like a valid settings export (unexpected shape).".to_string())
|
"This file doesn't look like a valid settings export (unexpected shape).".to_string()
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
@@ -318,14 +356,54 @@ mod tests {
|
|||||||
assert_eq!(non_blank(Some(" a ".to_string())), Some(" a ".to_string()));
|
assert_eq!(non_blank(Some(" a ".to_string())), Some(" a ".to_string()));
|
||||||
}
|
}
|
||||||
|
|
||||||
fn write_export(dir: &std::path::Path, name: &str, payload: &SettingsExportPayload, password: &str) -> PathBuf {
|
fn write_export(
|
||||||
let plaintext = serde_json::to_vec(payload).unwrap();
|
dir: &std::path::Path,
|
||||||
|
name: &str,
|
||||||
|
payload: &SettingsExportPayload,
|
||||||
|
password: &str,
|
||||||
|
) -> PathBuf {
|
||||||
|
write_raw_export(dir, name, &serde_json::to_value(payload).unwrap(), password)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Like `write_export`, but takes an arbitrary `serde_json::Value` rather
|
||||||
|
/// than a real `SettingsExportPayload` — for fixtures that are
|
||||||
|
/// deliberately not shape-compatible, which the typed helper above can't
|
||||||
|
/// produce at all.
|
||||||
|
fn write_raw_export(
|
||||||
|
dir: &std::path::Path,
|
||||||
|
name: &str,
|
||||||
|
value: &serde_json::Value,
|
||||||
|
password: &str,
|
||||||
|
) -> PathBuf {
|
||||||
|
let plaintext = serde_json::to_vec(value).unwrap();
|
||||||
let encrypted = settings_crypto::encrypt(&plaintext, password).unwrap();
|
let encrypted = settings_crypto::encrypt(&plaintext, password).unwrap();
|
||||||
let path = dir.join(name);
|
let path = dir.join(name);
|
||||||
std::fs::write(&path, &encrypted).unwrap();
|
std::fs::write(&path, &encrypted).unwrap();
|
||||||
path
|
path
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn splitting_settings_moves_the_web_terminal_token_out_rather_than_copying_it() {
|
||||||
|
let mut settings = AppSettings::default();
|
||||||
|
settings.web_terminal.access_token = Some("super-secret-token".to_string());
|
||||||
|
|
||||||
|
let (settings, secrets) = split_settings_and_secrets(settings);
|
||||||
|
|
||||||
|
assert_eq!(settings.web_terminal.access_token, None);
|
||||||
|
assert_eq!(
|
||||||
|
secrets.web_terminal_access_token,
|
||||||
|
Some("super-secret-token".to_string())
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn splitting_settings_with_no_token_leaves_it_absent_on_both_sides() {
|
||||||
|
let (settings, secrets) = split_settings_and_secrets(AppSettings::default());
|
||||||
|
|
||||||
|
assert_eq!(settings.web_terminal.access_token, None);
|
||||||
|
assert_eq!(secrets.web_terminal_access_token, None);
|
||||||
|
}
|
||||||
|
|
||||||
fn sample_payload(format_version: u32) -> SettingsExportPayload {
|
fn sample_payload(format_version: u32) -> SettingsExportPayload {
|
||||||
SettingsExportPayload {
|
SettingsExportPayload {
|
||||||
format_version,
|
format_version,
|
||||||
@@ -348,11 +426,24 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_file_from_a_newer_format_is_refused_before_the_full_shape_is_parsed() {
|
fn a_file_from_a_newer_format_is_refused_before_the_full_shape_is_parsed() {
|
||||||
|
// Shape-incompatible with the *current* `SettingsExportPayload` (a
|
||||||
|
// future version could easily have changed `settings` from an object
|
||||||
|
// to something else) as well as newer — so this only passes under
|
||||||
|
// the probe-first ordering. Parsing the full struct first (the old
|
||||||
|
// behavior) would fail on the shape mismatch and never reach the
|
||||||
|
// version check, producing the "unexpected shape" message instead of
|
||||||
|
// "newer version" / "Update Triple-C".
|
||||||
let dir = temp_dir("newer-format");
|
let dir = temp_dir("newer-format");
|
||||||
let path = write_export(
|
let path = write_raw_export(
|
||||||
&dir,
|
&dir,
|
||||||
"export.triplec",
|
"export.triplec",
|
||||||
&sample_payload(SETTINGS_EXPORT_FORMAT_VERSION + 1),
|
&serde_json::json!({
|
||||||
|
"format_version": SETTINGS_EXPORT_FORMAT_VERSION + 1,
|
||||||
|
"exported_at": "2026-08-27T00:00:00Z",
|
||||||
|
"app_version": "9.9.9",
|
||||||
|
"settings": "this-app-version-stores-settings-differently",
|
||||||
|
"secrets": {},
|
||||||
|
}),
|
||||||
"correct password",
|
"correct password",
|
||||||
);
|
);
|
||||||
|
|
||||||
@@ -381,18 +472,35 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_malformed_payload_produces_a_generic_error_not_a_raw_serde_message() {
|
fn a_malformed_payload_produces_a_generic_error_not_a_raw_serde_message() {
|
||||||
// Encrypt something that decrypts fine but isn't a valid payload
|
// A `format_version` the probe accepts, but a `settings` field of
|
||||||
// shape at all — this must not happen in practice (only this app
|
// the wrong *type* rather than just a missing field — this is what
|
||||||
// ever writes these files), but the error path must still never
|
// makes `serde_json` produce an "invalid type: string `...`, expected
|
||||||
// echo back plaintext content, generic malformed-shape or not.
|
// struct AppSettings" error that quotes the offending value
|
||||||
|
// verbatim. That value here stands in for plaintext that, in a real
|
||||||
|
// export, could be a live credential — the assertion below is only
|
||||||
|
// meaningful against a fixture that actually exercises serde's
|
||||||
|
// value-quoting behavior, which a merely-missing-field fixture does
|
||||||
|
// not.
|
||||||
let dir = temp_dir("malformed");
|
let dir = temp_dir("malformed");
|
||||||
let plaintext = b"{\"not\": \"a real export\"}".to_vec();
|
let path = write_raw_export(
|
||||||
let encrypted = settings_crypto::encrypt(&plaintext, "correct password").unwrap();
|
&dir,
|
||||||
let path = dir.join("export.triplec");
|
"export.triplec",
|
||||||
std::fs::write(&path, &encrypted).unwrap();
|
&serde_json::json!({
|
||||||
|
"format_version": SETTINGS_EXPORT_FORMAT_VERSION,
|
||||||
|
"exported_at": "2026-08-27T00:00:00Z",
|
||||||
|
"app_version": "0.4.14",
|
||||||
|
"settings": "NOT-A-REAL-CREDENTIAL-abc123",
|
||||||
|
"secrets": {},
|
||||||
|
}),
|
||||||
|
"correct password",
|
||||||
|
);
|
||||||
|
|
||||||
let err = read_and_decrypt(&path, "correct password").unwrap_err();
|
let err = read_and_decrypt(&path, "correct password").unwrap_err();
|
||||||
assert!(!err.contains("not a real export"), "leaked plaintext into the error: {}", err);
|
assert!(
|
||||||
|
!err.contains("NOT-A-REAL-CREDENTIAL-abc123"),
|
||||||
|
"leaked plaintext into the error: {}",
|
||||||
|
err
|
||||||
|
);
|
||||||
assert!(err.contains("doesn't look like a valid settings export"));
|
assert!(err.contains("doesn't look like a valid settings export"));
|
||||||
|
|
||||||
std::fs::remove_dir_all(&dir).ok();
|
std::fs::remove_dir_all(&dir).ok();
|
||||||
@@ -409,7 +517,11 @@ mod tests {
|
|||||||
);
|
);
|
||||||
|
|
||||||
let err = read_and_decrypt(&path, "wrong password").unwrap_err();
|
let err = read_and_decrypt(&path, "wrong password").unwrap_err();
|
||||||
assert!(err.contains("Wrong password"), "unexpected message: {}", err);
|
assert!(
|
||||||
|
err.contains("Wrong password"),
|
||||||
|
"unexpected message: {}",
|
||||||
|
err
|
||||||
|
);
|
||||||
|
|
||||||
std::fs::remove_dir_all(&dir).ok();
|
std::fs::remove_dir_all(&dir).ok();
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -110,11 +110,24 @@ pub struct SettingsImportPreview {
|
|||||||
/// bury in a generic "settings replaced" line — see the module doc
|
/// bury in a generic "settings replaced" line — see the module doc
|
||||||
/// comment on why this field exists at all.
|
/// comment on why this field exists at all.
|
||||||
pub enables_web_terminal: bool,
|
pub enables_web_terminal: bool,
|
||||||
|
/// Non-blank custom base URLs the import would set, so a redirect of
|
||||||
|
/// model traffic to somewhere other than the usual provider is visible
|
||||||
|
/// at import time rather than discovered later. These are endpoints, not
|
||||||
|
/// secrets — safe to show verbatim, unlike everything above.
|
||||||
|
#[serde(default)]
|
||||||
|
pub ollama_base_url: Option<String>,
|
||||||
|
#[serde(default)]
|
||||||
|
pub llamacpp_base_url: Option<String>,
|
||||||
|
#[serde(default)]
|
||||||
|
pub openai_compatible_base_url: Option<String>,
|
||||||
|
#[serde(default)]
|
||||||
|
pub gateway_api_base: Option<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl SettingsImportPreview {
|
impl SettingsImportPreview {
|
||||||
pub fn from_payload(payload: &SettingsExportPayload) -> Self {
|
pub fn from_payload(payload: &SettingsExportPayload) -> Self {
|
||||||
let non_blank = |s: &Option<String>| s.as_deref().is_some_and(|v| !v.trim().is_empty());
|
let non_blank = |s: &Option<String>| s.as_deref().is_some_and(|v| !v.trim().is_empty());
|
||||||
|
let non_blank_value = |s: &Option<String>| s.clone().filter(|v| !v.trim().is_empty());
|
||||||
Self {
|
Self {
|
||||||
exported_at: payload.exported_at.clone(),
|
exported_at: payload.exported_at.clone(),
|
||||||
app_version: payload.app_version.clone(),
|
app_version: payload.app_version.clone(),
|
||||||
@@ -126,6 +139,12 @@ impl SettingsImportPreview {
|
|||||||
has_gateway_master_key: non_blank(&payload.secrets.gateway_master_key),
|
has_gateway_master_key: non_blank(&payload.secrets.gateway_master_key),
|
||||||
has_web_terminal_access_token: non_blank(&payload.secrets.web_terminal_access_token),
|
has_web_terminal_access_token: non_blank(&payload.secrets.web_terminal_access_token),
|
||||||
enables_web_terminal: payload.settings.web_terminal.enabled,
|
enables_web_terminal: payload.settings.web_terminal.enabled,
|
||||||
|
ollama_base_url: non_blank_value(&payload.settings.global_ollama.base_url),
|
||||||
|
llamacpp_base_url: non_blank_value(&payload.settings.global_llamacpp.base_url),
|
||||||
|
openai_compatible_base_url: non_blank_value(
|
||||||
|
&payload.settings.global_openai_compatible.base_url,
|
||||||
|
),
|
||||||
|
gateway_api_base: non_blank_value(&payload.settings.gateway.api_base),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -138,8 +157,14 @@ mod tests {
|
|||||||
fn payload_with(secrets: ExportedSecrets) -> SettingsExportPayload {
|
fn payload_with(secrets: ExportedSecrets) -> SettingsExportPayload {
|
||||||
let mut settings = AppSettings::default();
|
let mut settings = AppSettings::default();
|
||||||
settings.global_custom_env_vars = vec![
|
settings.global_custom_env_vars = vec![
|
||||||
crate::models::EnvVar { key: "A".to_string(), value: "1".to_string() },
|
crate::models::EnvVar {
|
||||||
crate::models::EnvVar { key: "B".to_string(), value: "2".to_string() },
|
key: "A".to_string(),
|
||||||
|
value: "1".to_string(),
|
||||||
|
},
|
||||||
|
crate::models::EnvVar {
|
||||||
|
key: "B".to_string(),
|
||||||
|
value: "2".to_string(),
|
||||||
|
},
|
||||||
];
|
];
|
||||||
SettingsExportPayload {
|
SettingsExportPayload {
|
||||||
format_version: SETTINGS_EXPORT_FORMAT_VERSION,
|
format_version: SETTINGS_EXPORT_FORMAT_VERSION,
|
||||||
@@ -203,6 +228,26 @@ mod tests {
|
|||||||
assert!(!preview.has_web_terminal_access_token);
|
assert!(!preview.has_web_terminal_access_token);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn custom_base_urls_are_surfaced_but_blank_ones_read_as_absent() {
|
||||||
|
let mut payload = payload_with(ExportedSecrets::default());
|
||||||
|
payload.settings.global_ollama.base_url = Some("http://attacker.example:11434".to_string());
|
||||||
|
payload.settings.global_llamacpp.base_url = Some(" ".to_string());
|
||||||
|
payload.settings.gateway.api_base = Some("https://gateway.example/v1".to_string());
|
||||||
|
|
||||||
|
let preview = SettingsImportPreview::from_payload(&payload);
|
||||||
|
assert_eq!(
|
||||||
|
preview.ollama_base_url.as_deref(),
|
||||||
|
Some("http://attacker.example:11434")
|
||||||
|
);
|
||||||
|
assert_eq!(preview.llamacpp_base_url, None);
|
||||||
|
assert_eq!(preview.openai_compatible_base_url, None);
|
||||||
|
assert_eq!(
|
||||||
|
preview.gateway_api_base.as_deref(),
|
||||||
|
Some("https://gateway.example/v1")
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn counts_reflect_the_real_settings() {
|
fn counts_reflect_the_real_settings() {
|
||||||
let payload = payload_with(ExportedSecrets::default());
|
let payload = payload_with(ExportedSecrets::default());
|
||||||
|
|||||||
@@ -26,6 +26,10 @@ const samplePreview: SettingsImportPreview = {
|
|||||||
has_gateway_master_key: false,
|
has_gateway_master_key: false,
|
||||||
has_web_terminal_access_token: false,
|
has_web_terminal_access_token: false,
|
||||||
enables_web_terminal: false,
|
enables_web_terminal: false,
|
||||||
|
ollama_base_url: null,
|
||||||
|
llamacpp_base_url: null,
|
||||||
|
openai_compatible_base_url: null,
|
||||||
|
gateway_api_base: null,
|
||||||
};
|
};
|
||||||
|
|
||||||
describe("ImportSettingsModal", () => {
|
describe("ImportSettingsModal", () => {
|
||||||
|
|||||||
@@ -14,6 +14,10 @@ function preview(overrides: Partial<SettingsImportPreview> = {}): SettingsImport
|
|||||||
has_gateway_master_key: false,
|
has_gateway_master_key: false,
|
||||||
has_web_terminal_access_token: false,
|
has_web_terminal_access_token: false,
|
||||||
enables_web_terminal: false,
|
enables_web_terminal: false,
|
||||||
|
ollama_base_url: null,
|
||||||
|
llamacpp_base_url: null,
|
||||||
|
openai_compatible_base_url: null,
|
||||||
|
gateway_api_base: null,
|
||||||
...overrides,
|
...overrides,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
@@ -59,6 +63,19 @@ describe("describeImport", () => {
|
|||||||
const items = describeImport(preview({ has_web_terminal_access_token: true }));
|
const items = describeImport(preview({ has_web_terminal_access_token: true }));
|
||||||
expect(items).toContain("The web terminal access token");
|
expect(items).toContain("The web terminal access token");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("names custom base URLs verbatim, since they're endpoints rather than secrets", () => {
|
||||||
|
const items = describeImport(
|
||||||
|
preview({
|
||||||
|
ollama_base_url: "http://10.0.0.5:11434",
|
||||||
|
gateway_api_base: "https://gateway.example/v1",
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
expect(items).toContain("Ollama server: http://10.0.0.5:11434");
|
||||||
|
expect(items).toContain("Gateway upstream: https://gateway.example/v1");
|
||||||
|
expect(items.some((i) => i.includes("llama.cpp"))).toBe(false);
|
||||||
|
expect(items.some((i) => i.includes("OpenAI-compatible"))).toBe(false);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("describeImportWarnings", () => {
|
describe("describeImportWarnings", () => {
|
||||||
@@ -79,7 +96,9 @@ describe("describeImportWarnings", () => {
|
|||||||
).toHaveLength(1);
|
).toHaveLength(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("does not warn just because a web terminal token is present but the terminal is off", () => {
|
it("warns about a dormant web terminal token even while the terminal stays off", () => {
|
||||||
expect(describeImportWarnings(preview({ has_web_terminal_access_token: true }))).toEqual([]);
|
expect(describeImportWarnings(preview({ has_web_terminal_access_token: true }))).toEqual([
|
||||||
|
"Includes a web terminal access token that will activate the next time the web terminal is turned on.",
|
||||||
|
]);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -19,21 +19,37 @@ export function describeImport(preview: SettingsImportPreview): string[] {
|
|||||||
if (preview.has_gateway_api_key) items.push("The gateway provider API key");
|
if (preview.has_gateway_api_key) items.push("The gateway provider API key");
|
||||||
if (preview.has_gateway_master_key) items.push("The gateway master key");
|
if (preview.has_gateway_master_key) items.push("The gateway master key");
|
||||||
if (preview.has_web_terminal_access_token) items.push("The web terminal access token");
|
if (preview.has_web_terminal_access_token) items.push("The web terminal access token");
|
||||||
|
if (preview.ollama_base_url) items.push(`Ollama server: ${preview.ollama_base_url}`);
|
||||||
|
if (preview.llamacpp_base_url) items.push(`llama.cpp server: ${preview.llamacpp_base_url}`);
|
||||||
|
if (preview.openai_compatible_base_url) {
|
||||||
|
items.push(`OpenAI-compatible server: ${preview.openai_compatible_base_url}`);
|
||||||
|
}
|
||||||
|
if (preview.gateway_api_base) items.push(`Gateway upstream: ${preview.gateway_api_base}`);
|
||||||
return items;
|
return items;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Things about an import that deserve more attention than a bullet in a
|
* Things about an import that deserve more attention than a bullet in a
|
||||||
* long list — currently just the one, but deliberately its own function
|
* long list — deliberately its own function rather than a flag inside
|
||||||
* rather than a flag inside `describeImport`: a setting that turns on a
|
* `describeImport`: a setting that turns on a network-listening service is
|
||||||
* network-listening service is exactly the kind of change a "your settings
|
* exactly the kind of change a "your settings were replaced" summary is bad
|
||||||
* were replaced" summary is bad at surfacing, on purpose or (if the file
|
* at surfacing, on purpose or (if the file came from someone else) not.
|
||||||
* came from someone else) not.
|
*
|
||||||
|
* A token that arrives with the terminal left *off* gets its own warning
|
||||||
|
* too, distinct from the "enables it now" one: `start_web_terminal` only
|
||||||
|
* mints a fresh token when none is already set, so a planted token here
|
||||||
|
* would silently become live the next time someone flips the terminal on
|
||||||
|
* through the UI, with no import-time signal that it wasn't freshly
|
||||||
|
* generated.
|
||||||
*/
|
*/
|
||||||
export function describeImportWarnings(preview: SettingsImportPreview): string[] {
|
export function describeImportWarnings(preview: SettingsImportPreview): string[] {
|
||||||
const warnings: string[] = [];
|
const warnings: string[] = [];
|
||||||
if (preview.enables_web_terminal) {
|
if (preview.enables_web_terminal) {
|
||||||
warnings.push("Enables the remote web terminal, which listens on your network.");
|
warnings.push("Enables the remote web terminal, which listens on your network.");
|
||||||
|
} else if (preview.has_web_terminal_access_token) {
|
||||||
|
warnings.push(
|
||||||
|
"Includes a web terminal access token that will activate the next time the web terminal is turned on.",
|
||||||
|
);
|
||||||
}
|
}
|
||||||
return warnings;
|
return warnings;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -310,6 +310,12 @@ export interface SettingsImportPreview {
|
|||||||
* "this enables a service that listens on your network" must not hide
|
* "this enables a service that listens on your network" must not hide
|
||||||
* inside a generic "settings replaced" summary. */
|
* inside a generic "settings replaced" summary. */
|
||||||
enables_web_terminal: boolean;
|
enables_web_terminal: boolean;
|
||||||
|
/** Non-blank custom base URLs the import would set — endpoints, not
|
||||||
|
* secrets, so shown verbatim to disclose a redirect of model traffic. */
|
||||||
|
ollama_base_url: string | null;
|
||||||
|
llamacpp_base_url: string | null;
|
||||||
|
openai_compatible_base_url: string | null;
|
||||||
|
gateway_api_base: string | null;
|
||||||
}
|
}
|
||||||
|
|
||||||
/** What `inspect_ca_cert_path` reports about a corporate CA path. Errors ride
|
/** What `inspect_ca_cert_path` reports about a corporate CA path. Errors ride
|
||||||
|
|||||||
Reference in New Issue
Block a user