diff --git a/.planning/milestones/v1.0-phases/03-typed-ipc-error-contract/03-SECURITY.md b/.planning/milestones/v1.0-phases/03-typed-ipc-error-contract/03-SECURITY.md new file mode 100644 index 00000000..32e6c6f2 --- /dev/null +++ b/.planning/milestones/v1.0-phases/03-typed-ipc-error-contract/03-SECURITY.md @@ -0,0 +1,66 @@ +--- +phase: 03 +slug: typed-ipc-error-contract +status: verified +threats_open: 0 +asvs_level: 1 +created: 2026-08-28 +--- + +# Phase 3 Security + +> Retroactive verification of the Phase 3 threat register after the ERR-06 +> post-close hardening. + +## Trust Boundaries + +| Boundary | Description | Data Crossing | +|----------|-------------|---------------| +| Rust command to Tauri bridge | `IpcError` is serialized instead of flattening reserved conflict codes into text | Conflict code and human-readable message | +| Tauri rejection to frontend normalizer | `todayInvoke`, `saveDocument`, and `updateFrontmatterField` normalize unknown rejection values before branch sites read them | Untrusted rejection object | +| Source inventory to regression guard | The ERR-06 test reads every Rust source file and checks code-emitting paths for typed-error flattening | Repository source text only | + +## Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation | Status | +|-----------|----------|-----------|----------|-------------|------------|--------| +| T-03-01 | Tampering / Spoofing | `normalizeIpcError` and its consumers | medium | mitigate | String-shape checks and closed union membership reject forged codes; `ipcError.test.ts`, `today.test.ts`, and `api.test.ts` pin fail-safe normalization | closed | +| T-03-02 | Information Disclosure | `IpcError` wire shape | low | accept | Conflict codes were already present in legacy error strings and the binary; the structured field adds no new sensitive detail | closed | +| T-03-03 | Information Disclosure | `From` and `Display` | low | mitigate | Legacy text remains the complete message for empty-code errors; Rust Display tests and the frontmatter API test pin unchanged user-visible text | closed | +| T-03-04 | Tampering | Conflict-emitting command paths | low | mitigate | ERR-06 removes string flattening from three commands; the recursive source guard detects new Rust modules, while the sole internal `apply_receipt` exception is function-scoped and documented | closed | +| T-03-05 | Information Disclosure | E2E fixture messages | low | accept | Fixtures carry a separate code and prefix-free message; normalization restores the same display text without adding internal data | closed | +| T-03-06 | Tampering | Rename and source-discovery drills | low | mitigate | Drill edits are reverted before commit; the source-discovery probe failed on the injected offender and passed after removal | closed | +| T-03-SC | Tampering | Dependency supply chain | low | accept | No dependency was added; the recursive guard uses the repository's existing `walkdir` dependency | closed | + +## Accepted Risks Log + +| Risk ID | Threat Ref | Rationale | Accepted By | Date | +|---------|------------|-----------|-------------|------| +| AR-03-01 | T-03-02 | The structured code exposes no information beyond the previous string prefix | Phase 3 design decision | 2026-08-28 | +| AR-03-02 | T-03-05 | Test fixtures preserve the production display surface and contain no secrets | Phase 3 design decision | 2026-08-28 | +| AR-03-03 | T-03-SC | No new package or version was introduced | Phase 3 design decision | 2026-08-28 | + +## Security Audit Trail + +| Audit Date | Threats Total | Closed | Open | Run By | +|------------|---------------|--------|------|--------| +| 2026-08-28 | 7 | 7 | 0 | Codex inline audit | + +## Verification Evidence + +- `make verify`: passed, including 1,945 frontend tests, 1,235 Rust tests, + fmt, clippy with warnings denied, typecheck, lint, and production build. +- ERR-06 recursive guard: passed on the final tree. +- Red probe: a newly added unregistered Rust module was detected as + `err06_guard_probe.rs:8`, then removed and the guard returned green. +- `Result<_, String>` measurement: 1,146 before ERR-06 and 1,142 after the + four intended signature changes. + +## Sign-Off + +- [x] All threats have a disposition. +- [x] Accepted risks are documented. +- [x] `threats_open: 0` confirmed. +- [x] `status: verified` set in frontmatter. + +**Approval:** verified 2026-08-28 diff --git a/src-tauri/src/document.rs b/src-tauri/src/document.rs index 485ef562..df391d29 100644 --- a/src-tauri/src/document.rs +++ b/src-tauri/src/document.rs @@ -208,7 +208,7 @@ pub fn update_frontmatter_field( key: String, value: Option, expected_revision: Option, -) -> Result { +) -> Result { let path = resolve_inside_vault(&vault_path, &document_path)?; let is_html = path .extension() @@ -216,13 +216,15 @@ pub fn update_frontmatter_field( .map(|value| matches!(value.to_ascii_lowercase().as_str(), "html" | "htm")) .unwrap_or(false); if is_html { - return Err("frontmatter editing is not supported for HTML documents".to_string()); + return Err("frontmatter editing is not supported for HTML documents" + .to_string() + .into()); } assert_document_owner(&vault_path, &path)?; assert_maru_can_write(&vault_path, WorkspaceWriteAction::Modify)?; let original = fs::read_to_string(&path).map_err(|err| format!("Cannot read document: {err}"))?; - assert_expected_revision(&original, expected_revision.as_deref()).map_err(|e| e.to_string())?; + assert_expected_revision(&original, expected_revision.as_deref())?; let mapped = value.map(FrontmatterValue::from); let updated = update_frontmatter_content(&original, &key, mapped)?; if updated != original { @@ -242,7 +244,7 @@ pub fn update_frontmatter_field( } write_atomic(&path, updated.as_bytes())?; } - read_document(vault_path, path.to_string_lossy().to_string()) + read_document(vault_path, path.to_string_lossy().to_string()).map_err(Into::into) } /// Optional Hub-driven prefill values. When the user picks a template + @@ -1211,7 +1213,7 @@ mod tests { .unwrap_err(); assert_eq!( - error, + error.to_string(), "frontmatter editing is not supported for HTML documents" ); } diff --git a/src-tauri/src/ipc_error.rs b/src-tauri/src/ipc_error.rs index c0646e97..b81833b6 100644 --- a/src-tauri/src/ipc_error.rs +++ b/src-tauri/src/ipc_error.rs @@ -55,6 +55,138 @@ impl From for IpcError { #[cfg(test)] mod tests { use super::*; + use std::fs; + use std::path::{Path, PathBuf}; + use walkdir::WalkDir; + + /// Name of the function a line sits in, found by walking back to the + /// nearest `fn` declaration. Used both to match the allowlist precisely and + /// to name the offender in the failure message. + fn enclosing_fn_name<'a>(lines: &[&'a str], index: usize) -> Option<&'a str> { + lines[..=index].iter().rev().find_map(|line| { + let trimmed = line.trim_start(); + let rest = trimmed + .strip_prefix("pub ") + .unwrap_or(trimmed) + .strip_prefix("pub(crate) ") + .unwrap_or_else(|| trimmed.strip_prefix("pub ").unwrap_or(trimmed)); + let rest = rest.strip_prefix("async ").unwrap_or(rest); + let rest = rest.strip_prefix("fn ")?; + Some(rest.split(['(', '<']).next().unwrap_or(rest).trim()) + }) + } + + /// True only for `map_err(|x| x.to_string())`, where the closure's own + /// argument is what gets stringified. That is the shape that takes a typed + /// error and throws the code away. It deliberately does not match + /// `map_err(|_| "literal".to_string())`, which mints a fresh error rather + /// than flattening one, nor an unrelated `.to_string()` elsewhere on the line. + fn flattens_the_closure_argument(line: &str) -> bool { + let Some((_, rest)) = line.split_once("map_err(|") else { + return false; + }; + let Some((param, body)) = rest.split_once('|') else { + return false; + }; + let param = param.trim(); + if param.is_empty() || param == "_" { + return false; + } + body.trim_start() + .starts_with(&format!("{param}.to_string())")) + } + + /// Read every Rust source under this crate's `src/` tree. Keeping source + /// discovery automatic means a new module that calls an existing emitter + /// is covered without a second, easy-to-forget inventory edit. + fn rust_sources() -> Vec<(String, String)> { + let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut paths: Vec = WalkDir::new(&root) + .into_iter() + .map(|entry| entry.expect("Rust source tree must be readable")) + .filter(|entry| { + entry.file_type().is_file() + && entry.path().extension().and_then(|value| value.to_str()) == Some("rs") + }) + .map(|entry| entry.into_path()) + .collect(); + paths.sort(); + + paths + .into_iter() + .map(|path| { + let name = path + .strip_prefix(&root) + .expect("walked source must stay below the crate src directory") + .to_string_lossy() + .replace('\\', "/"); + let source = fs::read_to_string(&path) + .unwrap_or_else(|err| panic!("Cannot read {}: {err}", path.display())); + (name, source) + }) + .collect() + } + + /// ERR-06: a path that reaches a contract-code emitter must not flatten its + /// error back to `String`. `.map_err(|e| e.to_string())` compiles fine and + /// silently strips the code, so the frontend's `err instanceof IpcError` + /// recovery branch stops matching while every build stays green. The + /// compiler cannot catch that; only an inventory can. + /// + /// Extend EMITTERS when a new code-producing helper lands. Add to ALLOWED + /// only with a reason, and only when the error genuinely never crosses IPC. + #[test] + fn no_code_emitting_path_flattens_its_error_to_string() { + const EMITTERS: &[&str] = &[ + "today_mutate(", + "task_transition(", + "task_trash(", + "save_document(", + "evidence_binder_mutate(", + "assert_expected_revision(", + "check_revision(", + "load_context(", + ]; + // (file, line) pairs that flatten deliberately, each with its reason. + const ALLOWED: &[(&str, &str)] = &[( + "web_actions.rs", + // apply_receipt is an internal helper, not a command: web_actions_apply + // consumes its Err as the reason string on a retry marker, so the value + // never crosses the IPC boundary and no frontend branch can read a code. + "apply_receipt", + )]; + let mut offenders = Vec::new(); + for (name, source) in rust_sources() { + let lines: Vec<&str> = source.lines().collect(); + for (index, line) in lines.iter().enumerate() { + if !flattens_the_closure_argument(line) { + continue; + } + // Look back a short window for the emitter call this flattens. + let start = index.saturating_sub(15); + let window = lines[start..=index].join("\n"); + let Some(emitter) = EMITTERS.iter().find(|e| window.contains(**e)) else { + continue; + }; + let enclosing = enclosing_fn_name(&lines, index).unwrap_or(""); + let allowed = ALLOWED + .iter() + .any(|(file, func)| *file == name && *func == enclosing); + if !allowed { + offenders.push(format!( + "{name}:{} in {enclosing}() flattens {emitter}", + index + 1 + )); + } + } + } + + assert!( + offenders.is_empty(), + "these paths reach a contract-code emitter and flatten its error to String, \ + stripping the code the frontend branches on: {offenders:#?}" + ); + } #[test] fn ipc_error_codes_are_stable() { diff --git a/src-tauri/src/today_ai.rs b/src-tauri/src/today_ai.rs index 454610e4..ee14aa25 100644 --- a/src-tauri/src/today_ai.rs +++ b/src-tauri/src/today_ai.rs @@ -7,6 +7,7 @@ // own revision/day/plan checks remain the final authority. use crate::agent_host::contracts::{TODAY_CAPTURE_SCHEMA_VERSION, TODAY_PLAN_SCHEMA_VERSION}; +use crate::ipc_error::IpcError; use crate::today::{ block_crosses_sleep, parse_sleep_start, parse_timezone, CalendarSyncStatus, CapacitySummary, CaptureCandidate, DailyPlanV1, PlanItemRef, TodayMutation, TodaySnapshot, TOP_LANE_MAX, @@ -274,7 +275,7 @@ pub fn today_apply_plan_result( output_json: String, valid_refs: Vec, sleep_start: String, -) -> Result { +) -> Result { let raw: JsonValue = serde_json::from_str(&output_json) .map_err(|err| format!("today_ai_invalid_payload: {err}"))?; let valid: HashSet = valid_refs.into_iter().collect(); @@ -296,7 +297,6 @@ pub fn today_apply_plan_result( expected_revision, TodayMutation::SetPlan { plan }, ) - .map_err(|e| e.to_string()) } #[cfg(test)] @@ -542,7 +542,7 @@ mod tests { SLEEP.to_string(), ) .unwrap_err(); - assert!(err.starts_with("today_conflict")); + assert_eq!(err.code, crate::ipc_error::TODAY_CONFLICT); // A draft computed against the newer revision but handed in with the // old expected_revision is caught by validation itself. let fresh = plan_output_json(&applied.revision, vec![top_item("a")], vec![]); @@ -555,7 +555,7 @@ mod tests { SLEEP.to_string(), ) .unwrap_err(); - assert!(err.starts_with("today_ai_stale_revision")); + assert!(err.to_string().starts_with("today_ai_stale_revision")); } fn plan_validation_err(output: String, snapshot: &TodaySnapshot, refs: &[&str]) -> String { @@ -801,7 +801,7 @@ mod tests { SLEEP.to_string(), ) .unwrap_err(); - assert!(err.starts_with("today_ai_invalid_payload")); + assert!(err.to_string().starts_with("today_ai_invalid_payload")); // deny_unknown_fields on the output wrapper: unexpected top-level // key is an invalid payload, not silently ignored. let mut raw: JsonValue = serde_json::from_str(&plan_output_json( diff --git a/src-tauri/src/today_calendar.rs b/src-tauri/src/today_calendar.rs index 63422b0b..a2f01da3 100644 --- a/src-tauri/src/today_calendar.rs +++ b/src-tauri/src/today_calendar.rs @@ -293,7 +293,7 @@ pub fn task_calendar_set_sync( item_ref: PlanItemRef, selected: bool, destination: Option, -) -> Result { +) -> Result { crate::today_store::today_mutate( work_path, logical_day, @@ -304,7 +304,6 @@ pub fn task_calendar_set_sync( destination, }, ) - .map_err(|e| e.to_string()) } // --- Publish ------------------------------------------------------------------ @@ -648,7 +647,7 @@ mod tests { snapshot: &TodaySnapshot, task_id: &str, selected: bool, - ) -> Result { + ) -> Result { task_calendar_set_sync( work(tmp), snapshot.logical_day.clone(), @@ -787,9 +786,9 @@ mod tests { // Stale revision conflicts; unknown item errors. let err = select(&tmp, &snapshot, "a", true).unwrap_err(); - assert!(err.starts_with("today_conflict")); + assert_eq!(err.code, crate::ipc_error::TODAY_CONFLICT); let err = select(&tmp, &cleared, "nope", true).unwrap_err(); - assert!(err.starts_with("today_plan_item_missing")); + assert!(err.to_string().starts_with("today_plan_item_missing")); } // --- Publish ----------------------------------------------------------------- diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index 537a353e..a0c2e98d 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { invoke } from "@tauri-apps/api/core"; vi.mock("@tauri-apps/api/core", () => ({ @@ -10,7 +10,12 @@ vi.mock("@tauri-apps/plugin-dialog", () => ({ save: vi.fn(), })); -import { applyInboxDecisions, scanInboxProcessedSnapshot } from "./api"; +import { applyInboxDecisions, scanInboxProcessedSnapshot, updateFrontmatterField } from "./api"; +import { IpcError } from "./ipcError"; + +beforeEach(() => { + vi.mocked(invoke).mockReset(); +}); describe("applyInboxDecisions fallback", () => { it("returns the done item directory for accepted decisions", async () => { @@ -90,3 +95,53 @@ describe("scanInboxProcessedSnapshot", () => { } }); }); + +describe("updateFrontmatterField", () => { + it("normalizes a document conflict into an IpcError", async () => { + (globalThis as { window?: unknown }).window = { __TAURI_INTERNALS__: {} }; + vi.mocked(invoke).mockRejectedValueOnce({ + code: "document_conflict", + message: "expected revision a, found b", + }); + + try { + let caught: unknown; + try { + await updateFrontmatterField("/workspace", "note.md", "status", "done", "rev-a"); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(IpcError); + expect(caught).toMatchObject({ + code: "document_conflict", + message: "document_conflict: expected revision a, found b", + }); + expect(invoke).toHaveBeenCalledWith("update_frontmatter_field", { + vaultPath: "/workspace", + documentPath: "note.md", + key: "status", + value: "done", + expectedRevision: "rev-a", + }); + } finally { + delete (globalThis as { window?: unknown }).window; + } + }); + + it("preserves the message of an uncoded legacy error", async () => { + (globalThis as { window?: unknown }).window = { __TAURI_INTERNALS__: {} }; + vi.mocked(invoke).mockRejectedValueOnce({ + code: "", + message: "frontmatter editing is not supported for HTML documents", + }); + + try { + await expect( + updateFrontmatterField("/workspace", "page.html", "status", "done"), + ).rejects.toThrow("frontmatter editing is not supported for HTML documents"); + } finally { + delete (globalThis as { window?: unknown }).window; + } + }); +}); diff --git a/src/lib/api.ts b/src/lib/api.ts index 66f7b78a..91f409e5 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -1118,13 +1118,20 @@ export async function updateFrontmatterField( const doc = readMockDocument(documentPath); return doc; } - return invoke("update_frontmatter_field", { - vaultPath, - documentPath, - key, - value, - expectedRevision: expectedRevision ?? null, - }); + try { + return await invoke("update_frontmatter_field", { + vaultPath, + documentPath, + key, + value, + expectedRevision: expectedRevision ?? null, + }); + } catch (err) { + // The command returns IpcError (ERR-06), so its rejection is a + // { code, message } object rather than a string. Without this funnel a + // caller doing String(err) would render "[object Object]". + throw normalizeIpcError(err); + } } /**