fix(secrets): unlock Secret Service items on retry

Closes #11052
This commit is contained in:
t8y2
2026-10-04 16:33:11 +00:00
parent 320e37aa42
commit 75c9122799
11 changed files with 220 additions and 23 deletions
@@ -31,6 +31,8 @@ describe("migration failure actions", () => {
button("migration.retryStatus").click();
await nextTick();
expect(backend.migrationStatus).toHaveBeenCalledTimes(2);
expect(backend.migrationStatus).toHaveBeenNthCalledWith(1, false);
expect(backend.migrationStatus).toHaveBeenNthCalledWith(2, true);
expect(backend.migrationRetry).not.toHaveBeenCalled();
} finally {
app.unmount();
@@ -148,7 +148,7 @@ async function diagnostic() {
<p>{{ t(errorAdviceKey) }}</p>
</div>
<div class="flex flex-wrap gap-3">
<button v-if="props.store.state.error === 'statusFailed' || (status?.keyProviderAvailable === false && !status?.keyCreationAllowed)" class="rounded-md bg-primary px-4 py-2 text-primary-foreground disabled:opacity-50" :disabled="props.store.state.busy" @click="props.store.initialize">
<button v-if="props.store.state.error === 'statusFailed' || (status?.keyProviderAvailable === false && !status?.keyCreationAllowed)" class="rounded-md bg-primary px-4 py-2 text-primary-foreground disabled:opacity-50" :disabled="props.store.state.busy" @click="props.store.retryStatus">
{{ t("migration.retryStatus") }}</button
><button
v-else
@@ -173,7 +173,7 @@ async function diagnostic() {
<p class="mt-3 text-sm text-muted-foreground">{{ props.store.state.error ? t(errorAdviceKey) : t("migration.progress") }}</p>
</div>
<div v-if="props.store.state.error" class="flex flex-wrap gap-3">
<button class="rounded-md bg-primary px-4 py-2 text-primary-foreground disabled:opacity-50" :disabled="props.store.state.busy" @click="props.store.state.error === 'statusFailed' ? props.store.initialize() : props.store.retry()">
<button class="rounded-md bg-primary px-4 py-2 text-primary-foreground disabled:opacity-50" :disabled="props.store.state.busy" @click="props.store.state.error === 'statusFailed' ? props.store.retryStatus() : props.store.retry()">
{{ props.store.state.busy ? t("migration.running") : props.store.state.error === "statusFailed" ? t("migration.retryStatus") : t("migration.retryMigration") }}</button
><button class="rounded-md border px-4 py-2 disabled:opacity-50" :disabled="props.store.state.busy" @click="diagnostic">{{ t("migration.exportDiagnostic") }}</button
><button class="rounded-md border px-4 py-2 disabled:opacity-50" :disabled="props.store.state.busy" @click="exitApp">{{ t("migration.exit") }}</button>
@@ -125,6 +125,17 @@ describe("WebDAV sync HTTP API", () => {
await expect(migrationCleanupBackups()).resolves.toBeUndefined();
expect(lastCall(fetchMock)).toEqual({ url: "/api/migration/cleanup-backups", body: {} });
});
it("forwards explicit migration status retries to the Web backend", async () => {
const fetchMock = stubFetch({});
const { migrationStatus } = await import("@/lib/backend/http");
await migrationStatus();
await migrationStatus(true);
expect(fetchMock).toHaveBeenNthCalledWith(1, "/api/migration/status");
expect(fetchMock).toHaveBeenNthCalledWith(2, "/api/migration/status?retry=true");
});
});
describe("GitLab snippet sync HTTP API", () => {
+1 -1
View File
@@ -205,7 +205,7 @@ import type { PluginToolPreview } from "@/types/pluginAiTools";
import type { CsvQuoteMode } from "@/lib/export/csvQuoteMode";
import type { MigrationPreflight, MigrationReport } from "./migration";
export type { MigrationPreflight, MigrationReport } from "./migration";
export const migrationStatus = (): Promise<MigrationPreflight> => get("/api/migration/status");
export const migrationStatus = (retry = false): Promise<MigrationPreflight> => get(`/api/migration/status${retry ? "?retry=true" : ""}`);
export const migrationStart = (): Promise<MigrationReport> => post("/api/migration/start", {});
export const migrationRetry = (): Promise<MigrationReport> => post("/api/migration/retry", {});
export const migrationCleanupBackups = (): Promise<void> => post("/api/migration/cleanup-backups", {});
+1 -1
View File
@@ -36,7 +36,7 @@ function invoke<T>(command: string, args?: Record<string, unknown>): Promise<T>
import type { MigrationPreflight, MigrationReport } from "./migration";
export type { MigrationPreflight, MigrationReport } from "./migration";
export const migrationStatus = (): Promise<MigrationPreflight> => invoke("migration_status");
export const migrationStatus = (retry = false): Promise<MigrationPreflight> => invoke("migration_status", { retry });
export const migrationStart = (): Promise<MigrationReport> => invoke("migration_start");
export const migrationRetry = (): Promise<MigrationReport> => invoke("migration_retry");
export const migrationCleanupBackups = (): Promise<void> => invoke("migration_cleanup_backups");
+4 -2
View File
@@ -74,9 +74,11 @@ describe("migration store", () => {
expect(store.state.error).toBe("statusFailed");
expect(store.state.errorCode).toBeNull();
expect(store.state.errorMessage).toBeNull();
await store.initialize();
await store.retryStatus();
expect(store.state.error).toBeNull();
expect(store.completed.value).toBe(true);
expect(status).toHaveBeenNthCalledWith(1, false);
expect(status).toHaveBeenNthCalledWith(2, true);
});
it("discards arbitrary transport exception values in state and diagnostics", async () => {
const secret = "password=DO_NOT_EXPOSE";
@@ -194,7 +196,7 @@ describe("migration store", () => {
await store.initialize();
await store.start();
expect(store.state.error).toBe("statusFailed");
await store.initialize();
await store.retryStatus();
expect(store.completed.value).toBe(true);
expect(store.state.report).toBeNull();
expect(store.blocking.value).toBe(true);
+7 -5
View File
@@ -32,18 +32,18 @@ export function useMigrationStore(backend: MigrationApi = api) {
state.errorCode = null;
state.errorMessage = null;
}
async function refreshStatus() {
async function refreshStatus(retry = false) {
// Only this endpoint returns classified, redacted migration errors. Transport
// exceptions may contain configuration values and must never enter UI state.
state.status = await backend.migrationStatus();
state.status = await backend.migrationStatus(retry);
state.errorCode = state.status.errorCode ?? null;
state.errorMessage = state.status.errorMessage ?? null;
}
async function initialize() {
async function checkStatus(retry: boolean) {
state.loading = true;
clearError();
try {
await refreshStatus();
await refreshStatus(retry);
state.step = completed.value ? 3 : 1;
// A completed migration only needs the success page in the session that
// performed it. On later launches, retained backups are recovery assets,
@@ -57,6 +57,8 @@ export function useMigrationStore(backend: MigrationApi = api) {
state.loading = false;
}
}
const initialize = () => checkStatus(false);
const retryStatus = () => checkStatus(true);
async function run(retry: boolean) {
if (state.busy) return;
state.busy = true;
@@ -114,5 +116,5 @@ export function useMigrationStore(backend: MigrationApi = api) {
function enter() {
if (completed.value && !state.busy) state.entered = true;
}
return { state, blocking, completed, initialize, start: () => run(false), retry: () => run(true), cleanup, diagnostic, enter };
return { state, blocking, completed, initialize, retryStatus, start: () => run(false), retry: () => run(true), cleanup, diagnostic, enter };
}
+107 -6
View File
@@ -345,6 +345,31 @@ where
worker.join().map_err(|_| "KEYRING_ACCESS_FAILED: secret service worker failed".to_string())?
}
#[cfg(any(test, all(feature = "os-keyring", target_os = "linux")))]
fn read_secret_service_item<I, U, R, E>(mut is_locked: I, mut unlock: U, mut read: R) -> Result<Vec<u8>, String>
where
I: FnMut() -> Result<bool, E>,
U: FnMut() -> Result<(), E>,
R: FnMut() -> Result<Vec<u8>, E>,
E: std::fmt::Display,
{
let locked =
is_locked().map_err(|error| format!("KEYRING_ACCESS_FAILED: secret lock status check failed: {error}"))?;
if locked {
// Unlock drives the provider's prompt flow (KWallet, GNOME Keyring,
// and other Secret Service implementations). Never treat a denied or
// failed prompt as a missing item: doing so could provision a
// replacement key and strand existing ciphertext.
unlock().map_err(|error| format!("KEYRING_ACCESS_FAILED: secret unlock failed: {error}"))?;
let still_locked = is_locked()
.map_err(|error| format!("KEYRING_ACCESS_FAILED: secret lock status check failed after unlock: {error}"))?;
if still_locked {
return Err("KEYRING_ACCESS_FAILED: secret remained locked after unlock".to_string());
}
}
read().map_err(|error| format!("KEYRING_ACCESS_FAILED: secret read failed: {error}"))
}
#[cfg(all(feature = "os-keyring", target_os = "linux"))]
fn secret_service_keyring_codec(allow_create: bool) -> Result<Option<SecretCodec>, String> {
use secret_service::{blocking::SecretService, EncryptionType, Error as SecretServiceError};
@@ -372,8 +397,7 @@ fn secret_service_keyring_codec(allow_create: bool) -> Result<Option<SecretCodec
let item = search.unlocked.into_iter().next().or_else(|| search.locked.into_iter().next());
if let Some(item) = item {
let secret =
item.get_secret().map_err(|error| format!("KEYRING_ACCESS_FAILED: secret read failed: {error}"))?;
let secret = read_secret_service_item(|| item.is_locked(), || item.unlock(), || item.get_secret())?;
let material = String::from_utf8(secret).map_err(|_| "SECRET_KEY_INVALID".to_string())?;
let codec = SecretCodec::from_key_material(material.trim()).map_err(|_| "SECRET_KEY_INVALID".to_string())?;
return Ok(Some(codec));
@@ -411,8 +435,7 @@ fn secret_service_keyring_codec(allow_create: bool) -> Result<Option<SecretCodec
let item = found.into_iter().next();
match item {
Some(item) => {
let secret =
item.get_secret().map_err(|error| format!("KEYRING_ACCESS_FAILED: secret read failed: {error}"))?;
let secret = read_secret_service_item(|| item.is_locked(), || item.unlock(), || item.get_secret())?;
let material = String::from_utf8(secret).map_err(|_| "SECRET_KEY_INVALID".to_string())?;
let codec =
SecretCodec::from_key_material(material.trim()).map_err(|_| "SECRET_KEY_INVALID".to_string())?;
@@ -599,8 +622,8 @@ mod tests {
}
use super::{
managed_key_path, read_key_file_with_retry, run_secret_service_operation, SecretCodec, SecretKeyPolicy,
SecretKeySource,
managed_key_path, read_key_file_with_retry, read_secret_service_item, run_secret_service_operation,
SecretCodec, SecretKeyPolicy, SecretKeySource,
};
use base64::Engine as _;
use std::sync::{Mutex, OnceLock};
@@ -699,6 +722,84 @@ mod tests {
assert!(!path.exists());
}
#[test]
fn locked_secret_service_item_is_unlocked_before_read() {
use std::cell::{Cell, RefCell};
let locked = Cell::new(true);
let calls = RefCell::new(Vec::new());
let secret = read_secret_service_item(
|| {
calls.borrow_mut().push("is_locked");
Ok::<_, &'static str>(locked.get())
},
|| {
calls.borrow_mut().push("unlock");
locked.set(false);
Ok::<_, &'static str>(())
},
|| {
calls.borrow_mut().push("read");
Ok::<_, &'static str>(b"secret".to_vec())
},
)
.unwrap();
assert_eq!(secret, b"secret");
assert_eq!(*calls.borrow(), ["is_locked", "unlock", "is_locked", "read"]);
}
#[test]
fn unlocked_secret_service_item_is_read_without_an_unlock_prompt() {
let unlock_called = std::cell::Cell::new(false);
let secret = read_secret_service_item(
|| Ok::<_, &'static str>(false),
|| {
unlock_called.set(true);
Ok::<_, &'static str>(())
},
|| Ok::<_, &'static str>(b"secret".to_vec()),
)
.unwrap();
assert_eq!(secret, b"secret");
assert!(!unlock_called.get());
}
#[test]
fn secret_service_unlock_rejection_or_provider_failure_is_not_treated_as_missing() {
for detail in ["prompt dismissed by user", "provider returned an error"] {
let read_called = std::cell::Cell::new(false);
let result = read_secret_service_item(
|| Ok::<_, &'static str>(true),
|| Err(detail),
|| {
read_called.set(true);
Ok::<_, &'static str>(Vec::new())
},
);
assert_eq!(result.unwrap_err(), format!("KEYRING_ACCESS_FAILED: secret unlock failed: {detail}"));
assert!(!read_called.get());
}
}
#[test]
fn secret_service_item_still_locked_after_unlock_is_an_access_error() {
let read_called = std::cell::Cell::new(false);
let result = read_secret_service_item(
|| Ok::<_, &'static str>(true),
|| Ok::<_, &'static str>(()),
|| {
read_called.set(true);
Ok::<_, &'static str>(Vec::new())
},
);
assert_eq!(result.unwrap_err(), "KEYRING_ACCESS_FAILED: secret remained locked after unlock");
assert!(!read_called.get());
}
#[cfg(all(feature = "os-keyring", target_os = "linux"))]
#[test]
fn linux_keyring_missing_bus_is_safe_for_sync_and_async_callers() {
+56 -2
View File
@@ -1348,7 +1348,15 @@ impl Storage {
}
fn resolve_secret_key(&self, allow_create: bool) -> Result<SecretKeyResolution, String> {
if !allow_create {
self.resolve_secret_key_with_error_cache(allow_create, true)
}
fn resolve_secret_key_with_error_cache(
&self,
allow_create: bool,
use_cached_error: bool,
) -> Result<SecretKeyResolution, String> {
if !allow_create && use_cached_error {
let key_files = self.key_file_digests();
let mut cache = self.secret_key_error_cache.lock().unwrap_or_else(|poisoned| poisoned.into_inner());
if let Some(cached) = cache.as_ref() {
@@ -1557,6 +1565,20 @@ impl Storage {
}
pub async fn inspect_data_migration(&self) -> Result<MigrationPreflight, String> {
self.inspect_data_migration_with_error_cache(true).await
}
/// Repeat the read-only migration probe after an explicit user action.
/// Only this call bypasses a cached provider failure; its success or error
/// replaces that cache before ordinary startup/background reads resume.
pub async fn retry_data_migration_inspection(&self) -> Result<MigrationPreflight, String> {
self.inspect_data_migration_with_error_cache(false).await
}
async fn inspect_data_migration_with_error_cache(
&self,
use_cached_key_error: bool,
) -> Result<MigrationPreflight, String> {
let files = self.legacy_json_files().await?;
let mut stored = self.load_migration_state().await?;
let failure = self.migration_failure.lock().unwrap_or_else(|p| p.into_inner()).clone();
@@ -1664,7 +1686,7 @@ impl Storage {
// This probe is deliberately read-only. It must not create a keyring
// entry, key file, or change permissions while displaying status.
let key_files_before = self.key_file_digests();
let key_probe = self.resolve_secret_key(false);
let key_probe = self.resolve_secret_key_with_error_cache(false, use_cached_key_error);
if let Err(error) = key_probe.as_ref() {
// Keep one startup probe from being repeated by each subsequent
// storage read while the desktop keyring is locked or unavailable.
@@ -9430,6 +9452,38 @@ mod tests {
assert!(storage.secret_codec(false).is_ok());
}
#[tokio::test]
async fn explicit_status_retry_bypasses_cached_error_and_keeps_the_success() {
let dir = tempfile::tempdir().unwrap();
let storage = storage_with_managed_key(dir.path()).await;
let cached_error = "KEYRING_ACCESS_FAILED: cached locked keyring";
storage.cache_secret_key_error(cached_error, storage.key_file_digests());
let cached = storage.inspect_data_migration().await.unwrap();
assert_eq!(cached.error_code.as_deref(), Some("KEYRING_ACCESS_FAILED"));
assert_eq!(storage.resolve_secret_key(false).err().unwrap(), cached_error);
let retried = storage.retry_data_migration_inspection().await.unwrap();
assert!(retried.key_provider_available);
assert_eq!(retried.key_status, super::MigrationKeyStatus::Ready);
assert_eq!(retried.key_source, "managed_data_dir");
assert!(storage.resolve_secret_key(false).is_ok());
}
#[tokio::test]
async fn failed_status_retry_replaces_the_cached_provider_error() {
let dir = tempfile::tempdir().unwrap();
let storage = storage_with_managed_key(dir.path()).await;
std::fs::write(managed_key_path(dir.path()), "\n").unwrap();
let cached_error = "KEYRING_ACCESS_FAILED: cached locked keyring";
storage.cache_secret_key_error(cached_error, storage.key_file_digests());
assert_eq!(storage.resolve_secret_key(false).err().unwrap(), cached_error);
let retried = storage.retry_data_migration_inspection().await.unwrap();
assert_eq!(retried.error_code.as_deref(), Some("SECRET_KEY_INVALID"));
assert_eq!(storage.resolve_secret_key(false).err().unwrap(), "SECRET_KEY_INVALID");
}
#[tokio::test]
async fn start_data_migration_invalidates_cached_secret_key_error() {
// Migration must re-probe the live provider instead of trusting a
+20 -2
View File
@@ -1,14 +1,32 @@
use axum::{extract::State, http::StatusCode, response::IntoResponse, Json};
use axum::{
extract::{Query, State},
http::StatusCode,
response::IntoResponse,
Json,
};
use dbx_core::storage::{MigrationPreflight, MigrationReport};
use serde::Deserialize;
use serde_json::json;
use std::sync::Arc;
use crate::state::WebState;
#[derive(Debug, Default, Deserialize)]
pub struct MigrationStatusQuery {
#[serde(default)]
retry: bool,
}
pub async fn status(
State(state): State<Arc<WebState>>,
Query(query): Query<MigrationStatusQuery>,
) -> Result<Json<MigrationPreflight>, (StatusCode, Json<serde_json::Value>)> {
state.app.storage.inspect_data_migration().await.map(Json).map_err(error_response)
let status = if query.retry {
state.app.storage.retry_data_migration_inspection().await
} else {
state.app.storage.inspect_data_migration().await
};
status.map(Json).map_err(error_response)
}
pub async fn start(
+9 -2
View File
@@ -39,8 +39,15 @@ pub struct SnippetDownloadResult {
}
#[tauri::command]
pub async fn migration_status(state: State<'_, Arc<AppState>>) -> Result<MigrationPreflight, String> {
state.storage.inspect_data_migration().await
pub async fn migration_status(
state: State<'_, Arc<AppState>>,
retry: Option<bool>,
) -> Result<MigrationPreflight, String> {
if retry.unwrap_or(false) {
state.storage.retry_data_migration_inspection().await
} else {
state.storage.inspect_data_migration().await
}
}
#[tauri::command]