fix(security,api,share): contain path traversal, stop item orphaning, surface global items, and share whole projects - #20
Merged
Conversation
Phase A diagnosis from docs/plans/issue-6-oversized-cursor-linux.md could not reach a verdict: this environment is WSL, and CLAUDE.local.md explicitly documents WSLg as unsupported for reproducing/verifying this bug (needs a real X11/Wayland desktop session). Static analysis ruled out the app-side candidates (no cursor: CSS rules, no zoom/transform/ image-rendering hacks, no set_zoom call, no custom cursor assets, no GTK/XCURSOR env vars set in lib.rs), but D1/D2/D3/D4/D6 all require a live compositor and remain unrun. No Phase B code change is warranted without a confirmed H3/H4/H5 verdict. Posted findings + needs-linux-verifier label on issue #6 instead of guessing a cause. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extend the custom titlebar controls to Linux (issue #5), matching the Windows gate. Replaces the isWindows boolean + useEffect with a platform-derived Chrome config computed via a lazy useState initializer (platform() is synchronous in Tauri v2, so the effect + one-frame no-controls flash is unnecessary). macOS behaviour is unchanged by design (see plan §4.3) and maximize stays out of scope (§4.4). Also adds focus-visible outline styling to the minimize/close buttons so keyboard focus is visible (§4.5) — previously only hover: classes existed. No Rust changes: existing capabilities (allow-minimize, allow-close, allow-start-dragging) are not platform-scoped and already apply on Linux. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…10) crypt_env_inject_environment and crypt_env_generate_example_env were the only two MCP tools naming the environment identifier `id`, diverging from every other environment-scoped tool's `environment_id`. An LLM caller that inferred the parameter name from the majority would pass `environment_id` to these two, have it silently ignored, and — when project/environment were also present — inject a different environment's full decrypted variable set with no error. Both schemas now advertise `environment_id` with the canonical wording used everywhere else. `resolve_environment_id` accepts `id` as an unadvertised, deprecated alias for the 1.0.x line only (removed in 1.1.0, marked DEPRECATED in-source); using it still succeeds but appends a fixed deprecation notice to the tool response. The ambiguity error in crypt_env_inject_env_by_name now names environment_id too. Adds an in-crate #[cfg(test)] mod tests (cargo test --bin crypt-env-mcp) asserting every environment-scoped tool declares environment_id and not a bare id, that the five item/category/workspace tools keep bare id, and that the resolver prefers environment_id over the alias. Updates docs/reference.md and adds a CHANGELOG 1.0.2 entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ect names Issue #12: adds idx_environments_name_nocase (project_id, name COLLATE NOCASE) and retrofits the same pre-check to idx_projects_name_nocase, so an install that already holds a case-colliding pair (e.g. MyApp/myapp, production/Production) is deterministically deduped (lowest id keeps its name, losers get -2/-3 suffixes, audit persisted under settings key env_name_dedup_v1) instead of bricking VaultDb::open. Unique-constraint violations are now detected via sqlx's is_unique_violation() and mapped to a stable "conflict:" sentinel string instead of substring-matching sqlx's error text, closing a raw-SQL leak in the 500 fallback path. project::resolve_environment now rejects ambiguous case-insensitive project/environment matches instead of silently taking the first one (SQLite NOCASE folds ASCII only, so non-ASCII collisions like PRODUCCIÓN/producción survive the index and need this Unicode-aware check) — the HTTP layer maps that to 409 AMBIGUOUS_SCOPE. New src-tauri/tests/environment_naming.rs covers T1-T8/T11 from the plan plus a projects-side dedup case; sqlx added to [dev-dependencies] to seed pre-collision state that's otherwise unreachable through the public API once the index exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r (issue #3, Phase 1) Adds a new `wsl` module (`wsl_list_distros`, `wsl_distro_home`) so Windows users with projects living inside a WSL2 distro can attach a project's .env by picking a distro instead of typing `\\wsl.localhost\...` UNC paths by hand. `project_pick_env_path` gains an optional `start_dir` to seed the native dialog there; the environment editor gets a "WSL" button beside the existing browse control, gated to Windows and non-empty distro lists. Also lands the data-loss gate this feature's own plan flagged as non-negotiable: `inject_environment`'s .env read now distinguishes `ErrorKind::NotFound` (start from empty) from every other error kind (abort with an error) instead of treating any read failure as "file is empty" and silently overwriting it — the most likely real failure mode once WSL/UNC paths are reachable from the picker. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…bering (#8) Adds a shared envfile module (std-only leaf: inspect/commit/marker policy) that every filesystem sink writing an env file now goes through before any secret is decrypted. A resolved path that already exists and doesn't carry the crypt-env marker is refused (409 TARGET_EXISTS) unless the caller opts into `overwrite: true`, which first preserves the prior bytes in `<path>.bak` (refusing again with 409 BACKUP_EXISTS if one is already there). - POST /fill, POST /environments/:id/example, POST /environments/:id/inject all gain `overwrite` and surface `backup`/`unmanagedPaths` in responses. - inject_environment restructures into resolve -> gate -> decrypt -> write, with the gate strictly before decryption; owner-configured environment.paths[] are grandfathered (reported, not blocked) per the upgrade-safety trade-off, while output_path/output_dir-derived paths are hard-gated. - TempEnvFile now delegates to envfile::commit (fixes the write-then-chmod window) and, on the error path, truncates rather than removes a pre-existing file it didn't create. - CLI `fill --force`, MCP `overwrite` param + 409 handling on the three affected tools, and a GUI confirm-before-overwrite dialog backed by a new environment_inject_preview command. - Replaces the `read_to_string(..).unwrap_or_default()` at the old project/mod.rs:328 (which turned any read error, not just NotFound, into "treat as empty") with envfile::inspect's explicit NotFound/other-error split — this also happens to be the fix for the silent-overwrite failure mode on a stopped-WSL-distro UNC path that issue #3 flags as blocking. Explicitly out of scope: path traversal/containment validation on output_dir (issue #7's territory — see envfile/mod.rs's doc comment on the boundary). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…overage baseline; drop stale Postman collection Implements docs/plans/issue-11-test-coverage-and-postman.md: a crate::test_support harness (TestVault fixture + router()/req() oneshot helpers) so every module's private/pub(crate) logic is unit-testable without a Tauri runtime or live socket, plus 119 new test cases across api/, project/, vault/, db/, share/, the CLI's scope resolver, and the MCP binary's environment-picking logic (17 -> 136 total). Two pure refactors enable the harness, both behavior-preserving: - api::build_router / ApiState::new extracted from start_server (pub(crate), not pub — zero external API widening). - vault::set_item_global's fork logic extracted the same way create_project_item already was, so the Tauri command becomes a thin delegating wrapper. - crypt-env-mcp's resolve_environment_id split into network-fetching + pure pick_environment_id, mirroring the plan's resolve_environment_id split. Deletes the stale Postman collection (asserted a removed /health field, every request 422'd without the mandatory scope params) and replaces it with curl examples + the executed api::tests::* suite in docs/reference.md. Adds .github/workflows/test.yml running cargo test on push/PR with an informational cargo-llvm-cov summary step. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lity, sentinel visibility
Addresses adversarial review of the issue-12 case-insensitive-uniqueness
work:
- Critical: next_free_{project,environment}_name folded the rename
candidate with Rust's to_lowercase() (full Unicode) but compared it
against SQLite's own LOWER(name) (ASCII-only). For a non-ASCII base
this made the two folds disagree, so a real collision against a
pre-existing suffixed sibling (e.g. producciÓn-2) was missed, the
"free" candidate was accepted, and the following UPDATE then hit the
table's exact-match UNIQUE constraint -- init_schema returned Err and
the vault could not be opened at all. Fixed by comparing via
LOWER(name) = LOWER(?) so both sides are folded by SQLite consistently.
Added a regression test reproducing the exact crash shape.
- env_name_dedup_v1 (the sole reversal path for an otherwise
irreversible rename) is now persisted per-rename, inside the dedup
loops themselves, instead of batched and written only after both
CREATE UNIQUE INDEX statements succeed -- a renamed row can no longer
land in the DB without also being recorded, even if a later migration
step fails or the process is killed mid-run.
- PROJECT_NAME_CONFLICT bumped from private to pub (matching
ENVIRONMENT_NAME_CONFLICT, also bumped from pub(crate) to pub): each
src/bin/* target is its own crate separate from crypt_env_lib, so
pub(crate) was invisible cross-crate. Exported
project::AMBIGUOUS_MATCH_PREFIX so the ambiguity sentinel is a named
constant instead of a bare string literal at every match site.
- persist_rename_report no longer silently discards prior rename
history on a JSON parse failure -- logs to stderr and preserves the
raw value verbatim under env_name_dedup_v1_corrupt before starting a
fresh list.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…points (issue #13) Global items not linked into any environment were invisible to GET /items, GET /commands, and every CLI/MCP surface built on them — indistinguishable from items that don't exist, and a known, undocumented-in-the-contract divergence between the GUI (which already filters isGlobal client-side) and every headless caller. Adds a tri-state include_global=true|false|only query param (default true) to the two discovery endpoints only; materialization endpoints (/fill, /environments/:id/inject, /environments/:id/example, /share/listen) are untouched and stay strictly linkage-based. Every returned item/command now carries isGlobal + a new linked discriminator via an API-response-only ScopedItem/ScopedCommand wrapper (not merged into VaultItem, which is what gets AES-GCM encrypted). Propagates to crypt-env search/list (--scope-globals flag + SCOPE column), crypt-env cmd/exec (linked-wins tie-break on name collision, warns on shadowed global), and the MCP list_items/search_items tools (include_global in inputSchema, forwarded to the REST call). GUI untouched — GlobalSecrets.tsx already filters client-side. 9 new tests: 7 pure-function unit tests (api::scope_tests, plain VaultItem values, no db/vault) covering the union/dedup/linked-flag logic and the invalid-value 422 path, plus 2 integration tests guarding that is_global round-trips through VaultDb::upsert_item and set_item_global. A 10th test (full HTTP GET /items assertion) is deferred to issue #11's test harness, which lives on a separate branch not present in this worktree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An environment `name` is stored, untrusted data reused by later requests — a name like `../../../tmp/pwned` created once via POST /environments (or the GUI/CLI/an imported template) would silently redirect a subsequent /fill's decrypted secrets, /example, or inject_environment's write outside the caller's own output_dir. create_dir_all on the interpolated join path was the amplifier: it materialized the `.env...` directory that gave the `..` segments something real to resolve against. Two independent layers, each sufficient alone: - project::validate_environment_name / validate_project_name reject hostile names at the single choke point (save_environment / save_project), so no new hostile name can ever be persisted regardless of caller (HTTP, Tauri command, imported .cryptenv-proj template, CLI). - New dependency-free fsguard::resolve_within(base, name) guarantees any name — including a legacy row that predates validation — resolves to a direct child of the canonicalized base or is rejected, with a symlink recheck for an already-existing target. Wired into all three sinks (/fill, /environments/:id/example, inject_environment's default-filename branch): the create_dir_all call that used to see the interpolated name is gone, replaced by fsguard's own base-only creation. /fill's target is now resolved and validated before any decryption happens, so a rejected request never produces plaintext. Both layers report 422 (VALIDATION_ERROR / PATH_NOT_CONTAINED) instead of 500, and neither echoes the offending name or resolved path. ProjectManager.tsx mirrors both name rules for inline UX feedback only — the server remains the sole enforcement point. New src-tauri/tests/path_containment.rs covers the malicious-name table (19+ entries: traversal, absolute/UNC/verbatim, reserved device names, NUL, Unicode look-alikes, over-length, etc.), the no-amplification invariant, the choke-point (save_environment with no HTTP involved), layer independence, the positive path, and legacy-row containment. Issue #11's HTTP harness hasn't landed on this branch, so the /fill-specific HTTP-level cases (secret-scan-on-rejection, error-shape-over-HTTP) are deferred to when it does; the non-HTTP cases already cover objectives 1, 2, 4 and 6 of the plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… key collision Implements issue #9: POST /items now takes ?on_conflict=update|replace|error (default update). On a key collision, an exclusive item (non-global, linked once, single owner) is re-encrypted in place (200); a shared item 409s SHARED_ITEM_CONFLICT instead of silently rewriting a value another environment/project also sees; replace always creates a new item and deletes the superseded one only once it's unreachable. The whole create-or-update- and-link sequence is one SQLite transaction (db::create_or_link_item), so a failure between create and link can no longer leave an item owned but unlinked. Also adds the orphan report/prune surface (GET /maintenance/orphans, vault_list_orphan_items/vault_prune_orphan_items) for cleaning up rows left behind by data written before this fix, or by the share/relay/backup import paths that still land unlinked. PRAGMA secure_delete=ON so every delete path (including the new ones) zeroes freed pages. CLI `add`'s existing "update conflicting keys?" prompt now does what it always claimed; declining sends on_conflict=error instead of silently dropping the key. MCP crypt_env_add_item gains on_conflict + an accurate description; crypt_env_import_env_file maps its overwrite bool onto it so bulk import can no longer mass-produce orphans. Tests 12-15 of the plan's matrix (pure VaultDb, no HTTP harness needed) are in vault_integration.rs. Tests 1-11 need issue #11's HTTP test harness, not yet in this worktree — left as a follow-up, documented in the test file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…#4) Phase 0 — delete the legacy workspace relay surface: removes WorkspaceBundle/encrypt_workspace/decrypt_workspace, the /workspaces/:id/relay/send and /workspaces/relay/receive REST endpoints, and the crypt_env_share_workspace_send/receive MCP tools. This code read and wrote the frozen workspaces/workspace_vars tables, so anything imported through it landed as ownerless items invisible to every project-scoped surface. The workspaces/* tables and their db accessors are kept — they remain the one-time migration backfill source. Phases 1-3 — build project relay sharing (no MCP tool, by design): - Protocol: ProjectBundle/EnvironmentBundle/ProjectBundleVar in share/relay.rs, items deduped by name and hoisted to the bundle root, version-checked on decrypt, no `paths` or `literal` field. - db::insert_received_project: one transaction, Unicode-aware case-insensitive project-name pre-check ("conflict: ..." prefixed error), ciphertext-only input (db never sees the vault key). - project::relay: build_project_bundle / receive_project_bundle orchestrators shared by HTTP and Tauri. - REST: POST /projects/:id/relay/send, POST /projects/relay/receive. - Tauri: project_relay_send, project_relay_receive. - CLI: crypt-env project share/receive, with a KEY -> item name manifest and y/N confirmation before anything leaves the machine. - GUI: SHARE PROJECT / RECEIVE PROJECT on ProjectManager.tsx, via new ProjectShareModal.tsx; extracted RelayCodeDisplay shared with ShareModal. Tests: 4 pure protocol tests in share/relay.rs, 7 integration tests in tests/project_relay.rs covering the plan's cases 5-11 (dedup, name collision + override, environment exclusion, dangling references, ownership, no leaked paths) against the db/project layer directly — issue #11's HTTP test harness isn't in this worktree yet, so these bypass the HTTP layer per the fallback the sibling issue branches used. Docs: reference.md rows replaced, MCP backward-compat notes removed; CHANGELOG Unreleased entry for both the removal and the new feature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # src-tauri/Cargo.toml
# Conflicts: # src-tauri/src/bin/crypt-env-mcp.rs
# Conflicts: # docs/reference.md # src-tauri/src/bin/crypt-env-mcp.rs # src-tauri/src/lib.rs # src-tauri/src/project/mod.rs
# Conflicts: # docs/reference.md # src-tauri/src/api/mod.rs # src-tauri/src/lib.rs # src-tauri/src/project/mod.rs
# Conflicts: # docs/reference.md # src-tauri/src/db/mod.rs # src-tauri/tests/vault_integration.rs
# Conflicts: # docs/reference.md # src-tauri/src/api/mod.rs # src-tauri/tests/vault_integration.rs
# Conflicts: # CHANGELOG.md # docs/reference.md # src-tauri/src/api/mod.rs # src-tauri/src/db/mod.rs # src-tauri/src/lib.rs # src/components/ProjectManager.tsx
The five branches in this batch add four integration targets under src-tauri/tests/ (environment_naming, path_containment, project_relay, vault_integration). `cargo test --lib --bins` builds none of them, so they would have been invisible to CI while appearing to be covered. Refs #11
CLAUDE.md forbids unwrap() in production code. The occupied-key branch re-unwrapped the `current` row to read is_global even though it had already been narrowed to `Some` via `current_item_id`. Carry (id, is_global) in one Option and match on that instead — same behaviour, no unwrap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-relay # Conflicts: # CHANGELOG.md # docs/reference.md # src-tauri/src/api/mod.rs # src-tauri/src/api/tests/items.rs # src-tauri/src/api/tests/scope.rs # src-tauri/src/db/mod.rs # src-tauri/src/lib.rs # src-tauri/src/project/mod.rs # src-tauri/tests/vault_integration.rs # src/components/ProjectManager.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The four changes that consume #11's harness and #12's collision contract.
Closes #7
Closes #9
Closes #13
Closes #4
Type of Change
What Changed
#7 — two independent layers. Layer 1:
environment.namemust match^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$;project.namegets a laxer deny-list (separators, control chars, NTFS-hostile chars, reserved device names, leading/trailing dot or space). Enforced insave_environment/save_project, so HTTP, Tauri, CLI and imported templates share one choke point. Layer 2: newfsguard::resolve_withinguarantees no write derived from an environment name — validated or legacy — lands outside the caller'soutput_dir, returning 422PATH_NOT_CONTAINED. Existing rows aren't migrated or rejected at read time; layer 2 covers them.#9 —
POST /itemson an existing environment-var key created a new item and repointed the link, leaving the old item in the vault: still owned, still readable via/items/:idand/items/:id/reveal, invisible to scoped listings. Now takes?on_conflict=update|replace|error(defaultupdate):updatere-encrypts in place when the item is exclusive, 409SHARED_ITEM_CONFLICTwhen it isn't;replacerepoints and deletes the superseded item only if it became unreachable;error409s. Create-or-link is one SQLite transaction. AddsGET /maintenance/orphans(read-only, redacted) and Tauri-only prune commands — deliberately no REST prune, so a static MCP token can't bulk-delete vault rows. EnablesPRAGMA secure_delete=ON.#13 — global items were invisible through every scoped list/search endpoint unless separately linked.
GET /itemsandGET /commandsnow takeinclude_global=true|false|only(defaulttrue), unioning unlinked globals in, deduped by id. Every item carriesisGlobalandlinkedso a caller can tell "reusable and exists" from "will be written by fill/inject". Materialization surfaces (/fill,/inject,/example,/share/listen) are deliberately unaffected — linking a global into an environment stays an explicit act.#4 — whole-project relay sharing: structure plus decrypted values for the selected environments in one encrypted round-trip, items deduped by name.
POST /projects/:id/relay/send+POST /projects/relay/receive, Tauri commands, CLIproject share/project receive, and a GUI flow showing a key manifest (KEY → item name, never values) before upload. Receive always creates a new project in one all-or-nothing transaction; a case-insensitive name collision is 409, retryable with an override. Not exposed to MCP by design. Removes the legacy workspace-relay REST endpoints and MCP tools, dead since the Projects/Environments migration — the underlying tables stay as migration backfill source.Integration fix
receive_project_bundlewrites project and environment names straight from the relay bundle intoinsert_received_project, bypassing #7's validators — a gap that exists only in the #4+#7 combination. Added both validators at the top of the receive path so remote names get layer 1 as well as layer 2.Known gaps
GET/PUT/DELETE /items/:idandPOST /items/:id/revealstill perform no scope check — see Item endpoints are unscoped: any valid token can read, modify, or reveal any item by id #17. Global items are invisible through every scoped list/search endpoint unless separately linked into the queried environment #13 does not close this; it changes list visibility only.#4'sinsert_received_projectcarries aTODO(#12)to reuse environments.name has no case-insensitive uniqueness, unlike the now-fixed projects.name #12's shared collision helper now that it has landed.Testing
cargo checkpassescargo test --lib --bins --tests— 221 passed, 0 failed (incl.path_containment9,project_relay7)tsc --noEmitcleanpnpm tauri dev(not run). Share a whole project (all environments) via relay, not just single items #4's GUI share/receive flow needs a real end-to-end relay test with a live Supabase project before merge.Security Implications
#7 is the security fix: a crafted environment name plus
output_dircould path-traverse outside the intended directory, becausecreate_dir_allon the joined path materialized the component that made..resolve — reachable by anything holding the static MCP token. Now defended twice, independently. #9 removes a path that accumulated fully-readable secret rows invisible to every list surface. #4 moves decrypted values across a relay: values are re-encrypted with the receiver's key, never cross as ciphertext, are burned after read, and received items are neverisGlobal.