feat(security): block hidden paths in all vault operations - #428
Conversation
Shared hasHiddenPathSegment predicate (consolidates three duplicate inline implementations); resolveSafePath rejects any dot-prefixed segment on the resolved vault-relative path before filesystem access; memory file names reject leading dots (memory paths bypass resolveSafePath via direct join). Guardrail tests for every read and write surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rity docs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
|
umm-actually re-reviewed at 1 new finding(s) posted (8 tracked finding(s) across all runs). Context notes
umm-actually · deepseek/deepseek-v4-flash-0731 |
vault_delete_memory goes through memoryFilePath() which now rejects dot-prefixed names, but the tool description's Errors section was missing this error — unlike vault_get_memory and vault_update_memory which both had it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test used a substring match (toThrow("hidden path blocked")) on a
deterministic error message. Replace with the exact message including
the original input path, matching the project's assertion conventions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y surfaces Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pre-existing: new-directory rescan tests accept a condition where hasHiddenPathSegment is not the rescan's filter
The rescan tests in the new-directory rescan suite use the watcher's real addDir handler and report their own Failure scenario: A developer removes the Suggested fixAdd a test (or assertion in an existing test) that seeds a hidden file inside a newly-added directory and confirms the rescan does NOT index it — specifically that `upsertNote` is never called for the hidden path. This proves the hidden-segment filter inside rescanNewDirectory actually excludes entries, not just that the chokidar ignored callback does. |
|
Pre-existing: deduplication of duplicate add events is untested for the serialization guard
The file-watcher code in Failure scenario: A developer removes the Suggested fixAdd a test that fires duplicate `add` events for the same file path (two rapid chokidar events) and verifies that both embedding calls complete, and that the second call's embedding is the one ultimately stored. Alternatively, mock `embedNote` to track invocation order and verify serialization. |
|
Addressing the two umm-actually findings from the 1. "new-directory rescan tests accept a condition where hasHiddenPathSegment is not the rescan's filter" — false positive. The test the suggested fix asks for already exists: 2. "deduplication of duplicate add events is untested for the serialization guard" — valid, pre-existing, tracked. The 🔍 ship-check · pr-monitor · claude-fable-5 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughChangesThe PR adds shared hidden-path detection and blocks dot-prefixed paths across vault filesystem operations, memory files, listings, watching, and indexing. Tests and documentation cover rejection behavior, preserved files, visible dot-containing names, and security boundaries. Hidden path protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟠 High · up to Hidden memory files can still be returned through enumeration, allowing dot-prefixed content to be exposed despite the new blocking behavior, and path-validation errors may disclose sensitive user path data. The PR is not merge-ready until these security issues are addressed. Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vault-mcp/vault-operations/memory-store.ts`:
- Around line 320-333: Update the no-file enumeration filters in getMemory,
listMemoryFiles, and listMemoryFileNames to exclude .md entries whose basename
starts with a dot, matching the hidden-file restriction enforced by
memoryFilePath. Add regression coverage for each surface confirming dot-prefixed
memory files are omitted while normal memory files remain available.
In `@src/vault-mcp/vault-operations/vault-filesystem.ts`:
- Around line 73-76: Update resolveSafePath so both hidden-path and traversal
violations throw the same stable generic client-facing error without including
notePath, resolved paths, or other implementation details; retain any detailed
path information only in internal diagnostics if already supported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 78060a5a-33e3-4ae3-9b60-2d736a31b9cf
📒 Files selected for processing (19)
AGENTS.mdARCHITECTURE.mdREADME.mdSECURITY.mdsrc/utils/__tests__/has-hidden-path-segment.test.tssrc/utils/has-hidden-path-segment.tssrc/vault-mcp/mcp-core/tools/asset-tools.tssrc/vault-mcp/mcp-core/tools/memory-tools.tssrc/vault-mcp/mcp-core/tools/task-tools.tssrc/vault-mcp/mcp-core/tools/vault-crud-tools.tssrc/vault-mcp/search/file-watcher.tssrc/vault-mcp/search/search-index.tssrc/vault-mcp/vault-operations/__tests__/memory-store.test.tssrc/vault-mcp/vault-operations/__tests__/note-mover.test.tssrc/vault-mcp/vault-operations/__tests__/task-updater.test.tssrc/vault-mcp/vault-operations/__tests__/vault-filesystem.test.tssrc/vault-mcp/vault-operations/__tests__/vault-patcher.test.tssrc/vault-mcp/vault-operations/memory-store.tssrc/vault-mcp/vault-operations/vault-filesystem.ts
A pre-existing dot-prefixed .md in the memory folder leaked through the all-files read and both list surfaces; filter now mirrors the write-side rejection in memoryFilePath. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same rationale, fewer words — the shared-definition and resolved-path constraints stay; restated justification goes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Scope README Files bullet to visible folders
README's Files section still says an agent can Failure scenario: A user with a non-root Suggested fixRewrite the Files bullet to say 'search any visible folder' or similar. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Runtime Hardening section describes system properties; the lone imperative bullet now matches that register. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ync claim The Files section promised listing any folder — hidden folders now error, and the Data Integrity bullet says so, making the doc self-contradictory. SECURITY.md's symlink note loses the Obsidian Sync sentence: symlink handling under Sync is officially unsupported and undefined, so the claim wasn't defensible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressing the three PR-level findings: Scope README Files bullet to visible folders — fixed in e05582f: the Browse bullet now reads "list any visible folder's files", and DOCKERHUB.md is regenerated. The contradiction with the Data Integrity bullet is gone. New-directory rescan tests accept a condition where hasHiddenPathSegment is not the rescan's filter — refuted: the dedicated guardrail test creates a hidden Deduplication of duplicate add events is untested for the serialization guard — acknowledged as a pre-existing gap, deliberately not addressed in this PR: testing the pendingEmbeds serialization properly needs an ordering seam rather than mocked scheduler internals, and it is tracked as follow-up work. |
The server reads exactly two config files — daily-notes.json (core-plugin-data) and the Tasks plugin's data.json (community-plugin-data, powering vault_update_task's write-format detection) — so both categories default on; the desktop's per-device Vault configuration sync toggles remain the real gate. #428 blocks all tool reads of .obsidian/, and the deploy docs note plainly that plugin settings can carry API keys and sit unread on the config volume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The server reads exactly two config files — daily-notes.json (core-plugin-data) and the Tasks plugin's data.json (community-plugin-data, powering vault_update_task's write-format detection) — so both categories default on; the desktop's per-device Vault configuration sync toggles remain the real gate. #428 blocks all tool reads of .obsidian/, and the deploy docs note plainly that plugin settings can carry API keys and sit unread on the config volume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Document the memory dot-file message that memory tools actually emit
The PR's behavior-change list states that vault_get_memory / vault_update_memory with a dot-prefixed file name return Failure scenario: An operator upgrading and then verifying the fix greps tool logs for the documented string Suggested fixIn the Behavior changes section, split the memory tools out of the `hidden path blocked` bullet and document their actual message: `vault_update_memory` / `vault_get_memory` with a dot-prefixed `file` name return `memory file must not start with a dot: "..." would be a hidden file`. |
… default-on SYNC_CONFIGS (#429) Closes #427. Stacked on #428 (`feat(security): block hidden paths in all vault operations`), now merged — this PR targets `main` and its diff shows only the daily-notes work. ## What this fixes Three gaps from #427: 1. **Daily-notes settings were unreachable on standard remote deployments.** `SYNC_CONFIGS` was supported by the init chain but passed through by no shipped compose file and documented nowhere, so `.obsidian/daily-notes.json` never reached the server — daily notes worked only when the vault's settings happened to match the defaults. 2. **A failed config read was cached for the process lifetime.** On a fresh remote deploy the server boots before the initial sync delivers `.obsidian/`, and `server.ts` reads the config at startup — so the boot race was guaranteed, and the defaults stayed locked in until a restart. 3. **Periodic Notes users had no way to point the server at the right folder** — the core `daily-notes.json` file (which vault-cortex reads) isn't updated by the Periodic Notes plugin. ## Changes - **`DAILY_NOTES_FOLDER` / `DAILY_NOTES_FORMAT` env settings** with per-field precedence: env setting → `.obsidian/daily-notes.json` → the fallbacks (`Daily Notes`, `YYYY-MM-DD`). The format keeps Obsidian's moment tokens; validation at startup is structural fail-fast (probe-render a fixed date; reject traversal, leading or trailing separators, and empty renders) — unknown tokens are not rejected — tokens without a Luxon mapping fall into Luxon's own token grammar and may render differently than Obsidian, so only structurally unsafe results are blocked. When both fields are overridden, the config file is not read at all. The env settings are threaded as params (`DailyNotesEnvSettings`) from `VaultConfig` through `vault_get_daily_note`, the daily-review prompt, and the startup read — no module-level setter. - **Config caches hold only successful reads.** `daily-notes.ts` and `task-format-config.ts` (same race class) no longer cache the missing/malformed-file fallback — it's recomputed per call, so a config file delivered by sync after boot is picked up without a restart. Cost while the file is absent: one failed `readFile` per call. Once a read succeeds it's cached for the process lifetime, as before. The broken-link forward-reference exclusion no longer relies on a startup-seeded copy: `getOutgoingLinks` and `brokenLinkCount` take the daily notes folder as a parameter, resolved fresh per call by their callers, so a config file that arrives after boot corrects the annotation and count without a restart. - **`SYNC_CONFIGS` defaults to `core-plugin-data,community-plugin-data`** — the two categories the server reads: daily-notes settings, and community plugin settings (the Tasks plugin's format, which `vault_update_task`'s write-format detection uses). Applied in the remote compose files, the init script (so plain `docker run`, where compose interpolation doesn't apply, behaves the same), and CI deploys (optional repo-variable passthrough, `PROTECTED_PATHS`-style). Pulling a category is a no-op unless the desktop pushes it via Settings → Sync → "Vault configuration sync" — those per-device toggles are the real gate. Disabling is the explicit `none` sentinel, which actively clears the category list (`ob sync-config --configs ""`) rather than skipping — on a persistent config volume, skipping would leave a previously enabled category on across boots. Community plugin settings can carry API keys for plugins that use external services; #428 blocks all tool reads of `.obsidian/` and the server opens only the two config files it needs, so synced settings otherwise sit unread in the config volume (noted in the deploy docs). - **`moment-format.ts` extracted** from `daily-notes.ts` as a pure zero-import module so `config.ts` can validate formats without pulling in filesystem/logger deps. Token conversion runs only outside [literal] escapes, so literal text containing token letters ("[Week A]") is preserved verbatim — the previous convert-after-quoting order corrupted it (pre-existing, surfaced by review). - **Docs**: README Configuration rows + a "Daily notes" section (precedence, remote sync mechanics, Periodic Notes guidance); deploy/remote README "Daily notes" section + config table rows; DEPLOY.md CI variables; ARCHITECTURE.md resolution chain; `.env.example` × 3; synced CLI env blocks. - **DOCKERHUB generator**: the new table rows pushed the generated README past Docker Hub's 25000-byte cap (it was at 24785), so the generator now collapses markdown table padding (renders identically) and drops H3 subsections inside compact sections instead of emitting an empty heading. Output is now 18873 bytes. ## Behavior change Remote deployments that previously left `SYNC_CONFIGS` unset now sync the `core-plugin-data` and `community-plugin-data` categories by default. This only has an effect when the desktop pushes those categories; set `SYNC_CONFIGS=none` to keep config sync fully off. ## Tests - Precedence matrix (folder-only / format-only / both-vs-conflicting-file / both-no-file / folder-only-no-file), ENOENT-retry and malformed-then-fixed for both config readers, success-cache behavior pinned, `getDailyNotePath` exact path with overrides. - `loadConfig`: defaults, empty/whitespace-as-unset, raw moment string preserved, nested `YYYY/MM/DD` accepted, exact failure messages for traversal (raw and rendered), leading and trailing separators (raw and rendered), and empty render. - Exact-form drift pin: `SYNC_CONFIGS: ${SYNC_CONFIGS:-core-plugin-data,community-plugin-data}` in both remote compose files; existing compose ↔ .env.example ↔ CLI-block consistency tests cover the new vars. - Mutation-verified: reverting the cache fix makes exactly the three retry tests fail; all four `SYNC_CONFIGS` states (unset / empty / `none` / custom) exercised against the init script's branch logic with stubbed commands under `sh -e`. - `npm run lint` clean, both builds green, full suite 2527 tests passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable daily-note folders and filename formats, with environment overrides and vault/Obsidian fallbacks. * Added Moment-to-Luxon date-format compatibility, including literal text handling. * Added configurable Obsidian settings synchronization, defaulting to core plugin data. * **Bug Fixes** * Configuration files now retry after missing or malformed reads while retaining valid cached settings. * Added validation to prevent unsafe or invalid daily-note paths and filenames. * **Documentation** * Expanded deployment, configuration, and daily-notes documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Hidden paths (any dot-prefixed segment —
.obsidian/,.trash/, dotfiles) were excluded from listings, search, and the file watcher, but explicit paths were still served:vault_read_filewould read.obsidian/plugins/<plugin>/data.jsondirectly, and any write tool could create or modify.mdfiles inside hidden folders. Community plugins often store their own third-party API keys indata.jsonfiles, so a leaked MCP token also exposed those keys. Reported in #427.This PR blocks hidden paths in every client-facing vault operation, matching Obsidian, which ignores dot-prefixed paths entirely.
Changes
src/utils/has-hidden-path-segment.ts— consolidates three previously duplicated inline implementations (listing filter, file watcher, index rebuild), so the definition of "hidden" can't drift between layers.resolveSafePath— after the traversal check, the resolved vault-relative path is rejected if any segment is dot-prefixed. Checking the resolved path meansnotes/./plan.mdanda/../b.mdnormalize cleanly whilea/../.obsidian/xis caught. The guard fires before any filesystem access — it rejects on path shape alone and reveals nothing about what exists. Every read, write, patch, move, delete, and task update routes through it.daily-notes.tsandtask-format-config.tsread.obsidian/config via directreadFileand deliberately bypass the guard.join(notresolveSafePath), somemoryFilePathgains its own check: a name like.secretwould have created a hidden, unindexed file.Errors:bullets on every affected tool description, SECURITY.md (new Hidden paths section + database-placement note), README data-integrity bullet, ARCHITECTURE.md path-safety section.Behavior changes
Previously-succeeding calls that now return
hidden path blocked: "<path>" targets a hidden file or folder:vault_read_note(all modes) /vault_read_fileon paths inside hidden foldersvault_write_note,vault_patch_note,vault_replace_in_note,vault_delete_span,vault_update_properties,vault_delete_note,vault_update_taskon hidden pathsvault_move_notewith a hiddenold_pathornew_path(previously a note could be moved into.obsidian/and vanish from the index)vault_list_notes/vault_list_fileswith an explicitly hiddenfolder(previously returned an empty list; an explicit error beats a silent[]that reads as "folder is empty")vault_update_memory/vault_get_memorywith a dot-prefixedfilenameGraph and search tools are unchanged — they query the index, which has always excluded hidden paths.
Tests
a/../.obsidian/…rejected;notes/./plan.mdandnotes/version.2/file.mdaccepted with content returned.includes(".")fails the accepted-path tests.Verification
npm run lint(0 errors),npm run build,npm test(2484 passed). DOCKERHUB.md regeneration produced no diff (the changed README section is not part of it).Refs #427
BREAKING CHANGE: paths containing dot-prefixed segments (
.obsidian/,.trash/, dotfiles) are now rejected by all vault read, write, move, delete, and listing operations. Clients that read or wrote files inside hidden folders must stop, or the operator should relocate that content into visible folders. This matches Obsidian, which does not surface hidden paths at all.🤖 Generated with Claude Code
Summary by CodeRabbit
Security
Documentation
Tests