Stop an untouched secret field from deleting the stored credential

Making `null` actually clear a secret — which it had to, since a blanked
token was previously never revoked — turned the config editors into a
credential shredder. Secrets are `#[serde(skip_serializing)]`, so the
inputs always render empty whether or not one is stored, and the blur
handlers sent `value || null` unconditionally. Focusing the Git token
field and tabbing away deleted it, with nothing shown and no undo.

`useSecretField` encodes the rule: only a field the user typed in may
speak about a secret. Untouched, `patch()` contributes no key at all, and
Rust already distinguishes an absent key from an explicit null.

`withoutUntouchedSecrets` covers the structural half. `saveBedrock`
spreads `{ ...bedrock, ...patch }`, and when that falls back to
DEFAULT_BEDROCK_CONFIG the literal spells every secret out as `null` — so
editing the AWS region would have wiped the credentials as a side effect.

Also here: `WorkspaceSection` no longer saves a half-filled folder row,
which `update_project`'s new validation would refuse on every keystroke
between the two inputs; `snapshots_skipped` is declared on the wire type
rather than widened locally; and `#[must_use]` on `ProjectGuard` and
`ScrubOutcome` — which immediately caught the migration path discarding
its scrub outcome, the one scrub whose silence is expensive because the
layer it declined to clean is about to be committed.

The capability test reads the real file and is mutation-verified: adding
`core:default` back makes it fail. That grant pulls in an unscoped
`std::fs::read` of any host path and went unnoticed for months, because
nothing in the suite read the file at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc
This commit is contained in:
2026-08-23 13:23:00 -07:00
co-authored by Claude Opus 5
parent 7bbb699e4e
commit 6a8972980d
11 changed files with 354 additions and 49 deletions
@@ -461,7 +461,17 @@ async fn fresh_migration(
// pre-swap commit is the single largest snapshot Triple-C ever takes, so
// letting this one path commit unscrubbed is what the scrub exists to
// prevent. Failure is swallowed inside; it must never block a migration.
docker::scrub_writable_layer(&container_id).await;
//
// The outcome is logged rather than discarded: this is the one scrub whose
// silence would be expensive, because the layer it declined to clean is
// about to be committed into a snapshot that outlives the migration.
log::info!(
"Pre-migration scrub of {}{}",
container_id,
docker::scrub_writable_layer(&container_id)
.await
.commit_log_suffix()
);
emit_progress(&app_handle, &project_id, "Stopping the container...");
let _ = state
+5 -1
View File
@@ -2371,6 +2371,10 @@ const SCRUB_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(120);
/// The distinction that earns this type is [`ScrubOutcome::NotRunning`] versus
/// [`ScrubOutcome::Failed`]: "there was nothing to exec into" is routine, while
/// "the exec ran and broke" is the one case worth a warning in the log.
/// `#[must_use]` because the variants mean different things to a caller and
/// three of the four are easy to ignore by accident: a `Failed` swallowed at a
/// call site is exactly how a scrub that stopped working would stay quiet.
#[must_use = "a scrub that was skipped, timed out or failed reads the same as one that worked unless the outcome is inspected"]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ScrubOutcome {
/// The scrub ran to completion and freed this many bytes — possibly zero,
@@ -2397,7 +2401,7 @@ impl ScrubOutcome {
/// nothing to exec into. A figure of zero reads as a scrub that ran and
/// found nothing, which is a different and more alarming thing than a
/// scrub that correctly had no work left.
fn commit_log_suffix(&self) -> String {
pub(crate) fn commit_log_suffix(&self) -> String {
match self {
Self::Reclaimed(bytes) => format!(
" ({:.2} MB dropped by the pre-commit scrub)",
+64
View File
@@ -727,4 +727,68 @@ mod tests {
lifecycle.settle_startup_tasks().await;
assert!(started.elapsed() <= STARTUP_CANCEL_BUDGET + Duration::from_secs(1));
}
/// The capability file is the app's entire IPC attack surface, and it is
/// data — nothing in `cargo test` reads it, so a widened grant lands with a
/// green suite. This is what noticing looks like.
///
/// It exists because `core:default` was granted for months. That alias
/// pulls in `core:image:default` → `allow-from-path`, which is an
/// unconditional `std::fs::read` of any host path with no scope check, and
/// nothing in the frontend has ever imported `@tauri-apps/api/image`.
#[test]
fn the_capability_grants_are_the_ones_that_were_reviewed() {
let raw = include_str!("../capabilities/default.json");
let parsed: serde_json::Value =
serde_json::from_str(raw).expect("capabilities/default.json must parse");
let listed: Vec<String> = parsed["permissions"]
.as_array()
.expect("a `permissions` array")
.iter()
.map(|p| match p {
// A scoped grant is an object; its identifier is what matters here.
serde_json::Value::Object(o) => o["identifier"]
.as_str()
.expect("a scoped grant needs an identifier")
.to_string(),
other => other.as_str().expect("a grant is a string or an object").to_string(),
})
.collect();
let mut sorted = listed.clone();
sorted.sort();
let mut expected = vec![
"core:event:allow-listen",
"core:event:allow-unlisten",
"core:webview:allow-internal-toggle-devtools",
"dialog:allow-open",
"dialog:allow-save",
"opener:allow-open-url",
"drag:allow-start-drag",
];
expected.sort();
assert_eq!(
sorted, expected,
"the capability set changed. That is allowed — but it is the IPC \
surface a compromised webview can call, so update this list \
deliberately rather than to make the test pass."
);
// Belt and braces: the `*:default` aliases are the specific trap here,
// because they expand to a set the file never spells out. `store:*` in
// particular was an arbitrary host-file read/write primitive.
for grant in &listed {
assert!(
!grant.ends_with(":default"),
"{} is an alias — it expands to permissions this file does not \
name. Enumerate them instead.",
grant
);
assert!(
!grant.starts_with("store:"),
"store:* is `PathBuf::push` against AppData, which an absolute \
path discards: an arbitrary host-file read/write."
);
}
}
}
+9 -2
View File
@@ -151,6 +151,10 @@ fn holders() -> &'static Mutex<HashMap<String, ProjectOp>> {
/// predecessor already learned: a plain release statement is skipped by an
/// early `?`, by a panic, and by the future simply being dropped. A guard is
/// not.
/// Dropping this releases the claim, so a caller that discards it has taken no
/// lock at all — `let _ = try_acquire(...)` drops immediately and reads as
/// success. `#[must_use]` makes that a compile warning rather than a race.
#[must_use = "the claim is released as soon as this guard is dropped; bind it for the whole operation"]
#[derive(Debug)]
pub struct ProjectGuard {
project_id: String,
@@ -245,8 +249,11 @@ mod tests {
assert!(err.contains("snapshot is being compacted"), "{}", err);
assert!(err.contains("starting or recreating"), "{}", err);
drop(first);
// And it has to be retakeable the moment the holder goes away.
try_acquire(&p, ProjectOp::Recreate).expect("released on drop");
// And it has to be retakeable the moment the holder goes away. Bound
// rather than discarded: `#[must_use]` is what stops a real caller
// writing `try_acquire(...)` and believing it holds something.
let retaken = try_acquire(&p, ProjectOp::Recreate).expect("released on drop");
drop(retaken);
}
#[test]