SEC-007: Harden backend process recovery - #295
Conversation
9a4b378 to
d5e7b6e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are verified security/correctness issues in the Unix record publication/cleanup path (name substitution + unbound deletion) and a concrete portability build break on non-Linux/macOS Unix targets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens Berd’s backend (Goose serve) lifecycle recovery by replacing path/PID-based recovery handling with a verified, bounded process-record store and process-identity-based signaling decisions across platforms (including special “fail closed” handling on macOS and hardened handle-relative operations on Windows).
Changes:
- Introduces a new
ProcessRecordStorethat publishes and cleans up recovery records using owner-private, no-replace, successor-safe filesystem operations (Unix + Windows-specific implementations). - Switches Goose serve startup/shutdown and stale-process cleanup to use verified record reads, exact-object deletion, and identity probes before signaling/escalation.
- Extends process identity plumbing on Unix (Linux + macOS) and updates shutdown flow to be asynchronous and bounded.
File summaries
| File | Description |
|---|---|
| src-tauri/src/services/process.rs | Expands process identity support beyond Windows and adds Unix identity capture/probe helpers. |
| src-tauri/src/services/acp/process_record_store.rs | Adds the verified, bounded, platform-specific recovery record store and its adversarial tests. |
| src-tauri/src/services/acp/mod.rs | Wires the new process_record_store module into ACP services. |
| src-tauri/src/services/acp/goose_serve.rs | Migrates Goose serve lifecycle + stale recovery to the new store and identity-based signaling/retention behavior. |
| src-tauri/src/lib.rs | Updates app-exit teardown to await async singleton shutdown. |
| src-tauri/Cargo.toml | Adds required windows-sys feature flags for the new Windows ACL / NT API usage. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d5e7b6e to
b6d12ca
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It’s a high-sensitivity, platform-specific lifecycle/security change with outstanding native Windows adversarial verification, and it includes at least one correctness/robustness issue to address (stored PR comment).
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
b6d12ca to
11cf7ca
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The Unix stale-process cleanup retains recovery records even when a follow-up identity probe could confirm the process is gone/mismatched after a SIGTERM/SIGKILL failure, which can leave behind stale records indefinitely.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src-tauri/src/services/acp/goose_serve.rs:889
- If SIGKILL fails, we unconditionally retain the record. When the failure is due to the process already being gone (or the PID having already moved to a mismatched identity), a follow-up identity probe would allow us to remove the exact verified record and avoid leaving stale recovery evidence indefinitely.
if !kill(pid) {
log::warn!(
"SIGKILL failed for exact goose serve {}; keeping {}",
identity.pid,
path.display()
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
11cf7ca to
3e40c33
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
This is a high-sensitivity, cross-platform lifecycle/security change with at least one confirmed correctness issue in identity stability that can undermine orphan cleanup behavior.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
3e40c33 to
de5008e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The Windows implementation has a compile-time issue (STATUS_OBJECT_NAME_NOT_FOUND is referenced but not imported/defined), which must be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
de5008e to
88bb792
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It’s a high-sensitivity, platform-specific security hardening change with an explicit merge-blocking requirement for native Windows adversarial verification still outstanding.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
88bb792 to
da966af
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unix record publication can leave a hardlinked record in an undeletable/invalid state if temp-name unlink fails after a successful publish, breaking cleanup guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
da966af to
2f36572
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new process-record store currently has at least one confirmed correctness issue (MAX_RECORD_BYTES boundary) and one handle-safety issue (missing CLOEXEC on dup’d directory fd) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
2f36572 to
7d16be3
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It is a high-sensitivity cross-platform security change and the PR description explicitly notes outstanding native Windows adversarial verification as merge-blocking.
Review details
Suppressed comments (7)
src-tauri/src/services/acp/goose_serve.rs:761
remove_verified()errors are ignored here; if exact deletion fails, the record will persist without any log signal. Logging the error will help diagnose recovery-record retention and potential directory pollution.
let _ = store.remove_verified(path, verified);
src-tauri/src/services/acp/goose_serve.rs:820
remove_verified()failures are silently ignored here; if exact deletion fails, stale or invalid records can accumulate without any diagnostic breadcrumb. Prefer logging the error (but still continuing) to keep startup recovery transparent.
let _ = store.remove_verified(path, verified);
src-tauri/src/services/acp/goose_serve.rs:825
- This exact-record deletion is best-effort, but errors are dropped. If identity-bound deletion fails, logging helps explain why a record continues to reappear on subsequent startups.
let _ = store.remove_verified(path, verified);
src-tauri/src/services/acp/goose_serve.rs:857
- When SIGTERM fails but a follow-up probe confirms
Gone/Mismatch, the record cleanup still ignoresremove_verified()errors. Please log failures so you can distinguish 'record intentionally retained' from 'cleanup failed'.
let _ = store.remove_verified(path, verified);
src-tauri/src/services/acp/goose_serve.rs:870
- Exact cleanup is attempted here, but any
remove_verified()error is discarded; that can silently leave behind a record even after aGone/Mismatchprobe. Logging failures would make this path diagnosable.
let _ = store.remove_verified(path, verified);
src-tauri/src/services/acp/goose_serve.rs:893
- If SIGKILL fails but the follow-up probe confirms
Gone/Mismatch, cleanup ignores anyremove_verified()error. Please log deletion failures to avoid silently retaining records due to substitution/permission races.
let _ = store.remove_verified(path, verified);
src-tauri/src/services/acp/goose_serve.rs:906
- This final best-effort cleanup drops
remove_verified()errors; if the record can't be deleted exactly, it will remain with no log. Logging the error keeps recovery-record retention explainable in the field.
let _ = store.remove_verified(path, verified);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
7d16be3 to
d063873
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive, cross-platform process/FS hardening changes still need manual validation (including the stated native Windows adversarial verification) and there are correctness issues to address.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
d063873 to
1f8c547
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
High-sensitivity, cross-platform process/FS hardening with a confirmed Unix enumeration error-handling gap plus outstanding native Windows adversarial verification called out as merge-blocking in the PR description.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
1f8c547 to
51a977a
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces high-sensitivity, cross-platform process/FS security changes and still depends on outstanding native Windows adversarial verification before merge.
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
Prevent stale identifiers and mutable record paths from authorizing process signals or cleanup of the wrong filesystem object. Co-authored-by: Fuzzy <644e8093c651dbf16ecec80095552ed2e9180ec9e3de2f5f4f0d86ecfbe9d5f5@buzz.block.builderlab.xyz> Signed-off-by: Olabode Olaoke <olabode@squareup.com>
51a977a to
58e6413
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved critical recovery races remain, and full native Windows adversarial verification is still merge-blocking.
Review details
Suppressed comments (4)
src-tauri/src/services/acp/goose_serve.rs:733
- [P1] If startup fails after publishing a record but before
GOOSE_SERVEis initialized (for example, retention or teardown times out), a retry runs in the same app process with this owner identity still matching. This branch leaves the record without checkingserve_identity, so a still-running failed child is never recovered and the record persists across retries. Treat a matching owner in an uninitialized singleton as a failed-startup record and run serve cleanup.
match crate::services::process::probe_process_identity(owner_identity) {
IdentityProbe::Matches => {
log::debug!(
"Process record {} is still owned by live exact process {}; leaving it alone",
path.display(),
record.owner_pid
);
return;
src-tauri/src/services/acp/process_record_store.rs:627
- [Security] Unix
remove_verifiedultimately reopens the pathname and performsfstatatfollowed byunlinkat; the identity check can race with a same-user rename-and-replace between those syscalls. In that windowunlinkatdeletes the successor atpath, not the retained object, so this is not exact-object deletion on Unix. Use an fd-bound deletion approach where available or fail closed instead of relying on this check-then-unlink sequence.
pub(super) fn remove_verified(
root_path: &Path,
root: &RootHandle,
path: &Path,
verified: &VerifiedRecord,
src-tauri/src/services/acp/process_record_store.rs:447
- After
linkatsucceeds, both cleanup calls still resolve names rather than the retained source object. A hostile same-user process can replacefrombeforeunlink(from), which deletes the successor and leaves the published object with two links; if rollback runs,unlink(to)has the same pathname race. That can delete an unrelated object or permanently fail the laternlink == 1validation. Bind rollback/unlink to object identity or use an atomic publication primitive.
if let Err(temp_error) = unlink(root, &from) {
// Publication created `to` as a second name for the retained source.
// Roll it back before the caller cleans the identity-bound temp name,
// otherwise both names retain nlink == 2 and fail metadata validation.
let rollback_error = unlink(root, &to).err();
src-tauri/src/services/acp/process_record_store.rs:1216
- When
NextEntryOffsetis nonzero it must point to another entry inside the returned buffer. Allowingnext == remaining.len()advances exactly to EOF and returns success even though no next entry exists, silently accepting a truncated directory result and potentially skipping stale records during recovery.
if next < minimum_next || !next.is_multiple_of(8) || next > remaining.len() {
- Files reviewed: 6/7 changed files
- Comments generated: 7
- Review effort level: Lite
| let process_record = | ||
| retain_published_record(&process_record_store, &process_record_path, &mut child) | ||
| .await?; |
| diagnostic_log::fields([("pid", (pid as i64).into())]), | ||
| ); | ||
| kill_process(pid); | ||
| if !kill(pid) { |
| let source_identity = file_identity(source)?; | ||
| let mut current = std::mem::MaybeUninit::<libc::stat>::uninit(); | ||
| // SAFETY: current is writable and from is a root-relative name. | ||
| if unsafe { | ||
| libc::fstatat( |
| super::ensure_direct_child(root_path, path)?; | ||
| let file = match relative_file( | ||
| root, | ||
| path.file_name().expect("direct child has a name"), | ||
| FILE_READ_DATA | FILE_READ_ATTRIBUTES | READ_CONTROL | SYNCHRONIZE | DELETE, | ||
| FILE_OPEN, | ||
| ) { | ||
| Ok(file) => file, | ||
| Err(RelativeFileError::NtStatus(STATUS_OBJECT_NAME_NOT_FOUND)) => return Ok(()), | ||
| Err(error) => return Err(error.to_string()), | ||
| }; | ||
| validate_metadata( | ||
| path, | ||
| &file | ||
| .metadata() | ||
| .map_err(|e| format!("failed to inspect {}: {e}", path.display()))?, | ||
| )?; |
| let process_record_dir = if let Some(dir) = | ||
| crate::services::e2e_mode::E2eMode::process_record_dir_for(&app_handle) | ||
| .unwrap_or_else(|| std::env::temp_dir().join(PROCESS_RECORD_DIR_NAME)); | ||
| kill_stale_serve_process(&process_record_dir).await; | ||
| { | ||
| dir | ||
| } else { |
| let pid = child.id().and_then(pid_t_from_u32).ok_or_else(|| { | ||
| "child has no valid process id for graceful termination".to_string() | ||
| })?; |
| for path in entries { | ||
| if !is_process_record_path(&path) { | ||
| continue; | ||
| } | ||
| cleanup_process_record(&path).await; | ||
| cleanup_process_record(store, &path).await; |
Summary
Berd’s backend process recovery and shutdown handling relied on mutable pathnames and process identifiers that could become stale. This could allow pathname substitution, unsafe record permissions, PID reuse, or cleanup races to cause Berd to signal an unrelated process or modify the wrong filesystem object.
This is a high-sensitivity change to Berd’s backend lifecycle and local recovery-record storage, with platform-specific behavior. No current architectural law directly governs backend process-record storage or teardown (
LAWS/README.md,LAWS/AGENTS.md,LAWS/CHAT.md).Full native Windows adversarial verification remains a merge blocker. GitHub’s native Windows/MSVC CI compiled the implementation and passed its 71 managed-service tests on exact head
da966afa688779903e9b8a04b8913bf53890a7cd. That CI evidence does not exercise the complete Windows adversarial suite. Before final QA or merge, an authorized native Windows/MSVC host must still validate the adversarial ACL, retained-root enumeration, handle-relative no-replace rename, exact-handle deletion, root-swap, reparse-child, pathname-substitution, and pathname-successor scenarios.Related issue
N/A — no public issue was opened because the underlying report is security-sensitive. No duplicate public issue or PR is cited.
Testing
Locally available macOS gates passed on the implementation snapshot:
just tauri-checkjust clippyTwo broader backend tests still fail because of known unrelated default-provider fixtures.
GitHub’s native Windows/MSVC CI compiled the implementation and passed 71 managed-service tests on exact head
da966afa688779903e9b8a04b8913bf53890a7cd; all GitHub checks on that head passed. Full native Windows adversarial execution remains outstanding and merge-blocking as described above.Generated with Goose