hardening: storage permissions, ATTACH path escaping, project-config privilege stripping#143
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens Magic Context against local data leaks and untrusted project config, and fixes a SQLite ATTACH path escaping edge case.
Changes:
- Escape single quotes in the OpenCode DB path used in SQLite
ATTACH, with a regression test. - Enforce tighter filesystem permissions for the Magic Context storage dir, DB, and SQLite sidecars, with tests.
- Strengthen the project-config trust boundary by stripping additional user-only fields (embedding + hidden-agent blocks + git history indexing) and updating docs/tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugin/src/features/magic-context/tool-owner-backfill.ts | Escapes ' in interpolated SQLite ATTACH path. |
| packages/plugin/src/features/magic-context/tool-owner-backfill.test.ts | Adds regression test for quoted-path ATTACH. |
| packages/plugin/src/features/magic-context/storage-db.ts | Creates/locks down storage dir and DB file permissions (POSIX). |
| packages/plugin/src/features/magic-context/storage-db.test.ts | Adds POSIX permission enforcement test. |
| packages/plugin/src/features/magic-context/memory/embedding-local.ts | Creates model cache dir with owner-only perms (best-effort). |
| packages/plugin/src/config/project-security.ts | Expands stripping of unsafe project-config fields (embedding/agents/git indexing). |
| packages/plugin/src/config/project-security.test.ts | Updates tests to match stronger stripping behavior. |
| packages/plugin/src/config/index.test.ts | Updates config-merging/token tests to reflect new stripping rules. |
| CONFIGURATION.md | Documents project-config trust boundary and which fields are user-only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| closeQuietly(mc); | ||
| }); | ||
|
|
||
| test("ATTACH succeeds when the OpenCode DB path contains a single quote", () => { |
| const fs = require("node:fs"); | ||
| const base = createTempDir("mc-backfill-quote-"); | ||
| const dataHome = join(base, "o'brien"); | ||
| fs.mkdirSync(dataHome, { recursive: true }); | ||
| process.env.XDG_DATA_HOME = dataHome; |
| if (process.platform !== "win32") { | ||
| try { | ||
| chmodSync(modelCacheDir, 0o700); | ||
| } catch { | ||
| // Non-fatal — leave default perms if chmod is rejected. | ||
| } | ||
| } |
| if ("embedding" in projectRaw) { | ||
| delete projectRaw.embedding; | ||
| warnings.push( | ||
| "Ignoring embedding from project config (security: the embedding endpoint/provider/key is " + | ||
| "user-config-only; a repository must not redirect where memory text is sent).", | ||
| ); | ||
| } | ||
|
|
||
| for (const agentKey of HIDDEN_AGENT_KEYS) { | ||
| const block = projectRaw[agentKey]; | ||
| if (!isPlainObject(block)) continue; | ||
| const removed: string[] = []; | ||
| for (const field of AGENT_ESCALATION_FIELDS) { | ||
| if (field in block) { | ||
| delete block[field]; | ||
| removed.push(field); | ||
| } | ||
| } | ||
| if (removed.length > 0) { | ||
| if (agentKey in projectRaw) { | ||
| delete projectRaw[agentKey]; | ||
| warnings.push( | ||
| `Ignoring ${agentKey}.${removed.join("/")} from project config ` + | ||
| "(security: a repository cannot reprogram or re-permission hidden agents).", | ||
| `Ignoring ${agentKey} from project config (security: hidden agents are user-config-only; a ` + | ||
| "repository must not enable, reprogram, re-permission, or re-route the historian/dreamer/sidekick).", | ||
| ); | ||
| } | ||
| } |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/features/magic-context/memory/embedding-local.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/memory/embedding-local.ts:409">
P2: The chmod failure here is silently swallowed, unlike the equivalent hardening in `storage-db.ts` which logs the path and error message. Since this is a security hardening path (preventing group/world-readable model cache), a silent failure leaves sensitive data exposed with no operator visibility. Log the error to match the pattern used in `ensureSecureStorageDir`/`restrictDatabaseFilePermissions`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (process.platform !== "win32") { | ||
| try { | ||
| chmodSync(modelCacheDir, 0o700); | ||
| } catch { |
There was a problem hiding this comment.
P2: The chmod failure here is silently swallowed, unlike the equivalent hardening in storage-db.ts which logs the path and error message. Since this is a security hardening path (preventing group/world-readable model cache), a silent failure leaves sensitive data exposed with no operator visibility. Log the error to match the pattern used in ensureSecureStorageDir/restrictDatabaseFilePermissions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/memory/embedding-local.ts, line 409:
<comment>The chmod failure here is silently swallowed, unlike the equivalent hardening in `storage-db.ts` which logs the path and error message. Since this is a security hardening path (preventing group/world-readable model cache), a silent failure leaves sensitive data exposed with no operator visibility. Log the error to match the pattern used in `ensureSecureStorageDir`/`restrictDatabaseFilePermissions`.</comment>
<file context>
@@ -399,7 +399,17 @@ export class LocalEmbeddingProvider implements EmbeddingProvider {
+ if (process.platform !== "win32") {
+ try {
+ chmodSync(modelCacheDir, 0o700);
+ } catch {
+ // Non-fatal — leave default perms if chmod is rejected.
+ }
</file context>
|
Thanks — this is solid work. Parts 1 and 2 (escape the ATTACH path; tighten Could you drop part 3 (stripping the whole
Stripping those blocks wholesale would remove legitimate per-repo configurability, so we'd rather design the boundary deliberately (likely a more granular allow-list) in its own change. If you rebase this down to parts 1 + 2 we can merge it quickly. Appreciate the contribution. |
Three defense-in-depth hardenings, one commit each:
Escape SQLite ATTACH path (
tool-owner-backfill.ts): the OpenCode DB path was interpolated intoATTACH '...'unescaped; a path containing a single quote breaks the statement. Now escaped ('→''; ATTACH does not support bound parameters). Test covers a path containing a quote.Storage permissions (
storage-db.ts,embedding-local.ts):context.dbholds raw conversation history. The storage dir (and models cache dir) are now created/chmod'd0700andcontext.db/-wal/-shmchmod'd0600. Best-effort with try/catch, skipped on win32. Test asserts modes (skipped on win32).Project-config privilege stripping (
project-security.ts): project-level config merges over user config, so a repo-suppliedmagic-context.jsonccould redirectembeddingto an arbitrary endpoint (memory text is POSTed to${endpoint}/embeddings), enabledreamer/sidekick(agents allowed bash/write/edit), or overridehistorianrouting.stripUnsafeProjectConfigFields()now also stripsembedding,historian,dreamer,sidekick(whole objects) andmemory.git_commit_indexingfrom project config — these are user-config-only now, consistent with the existing strip ofauto_update/sqlite/hidden-agent fields. CONFIGURATION.md documents the trust boundary.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Hardens storage and config security and fixes a SQLite ATTACH path bug. Prevents local data leaks, blocks repo-driven privilege escalation, and makes backfill robust for paths with quotes.
0700;context.db/-wal/-shm0600(best-effort; skipped on Windows).embedding,historian,dreamer,sidekick(entire blocks), andmemory.git_commit_indexing; disable{env:}/{file:}token expansion in project config; document the trust boundary in CONFIGURATION.md.Written for commit 55e0bb6. Summary will update on new commits.
Greptile Summary
Three independent hardening commits: SQLite ATTACH path escaping, filesystem permission tightening on the storage dir and DB files, and a stronger project-config privilege strip that now drops entire
embedding/historian/dreamer/sidekickblocks (previously only dangerous sub-fields were removed). All changes are well-tested and the documentation is updated.tool-owner-backfill.ts):replaceAll("'", "''")is the correct SQL-literal escape for a context where bound parameters are unavailable; regression test covers an end-to-end path with a quote in the directory name.storage-db.ts,embedding-local.ts): storage dir created/chmoded to0700, DB file and WAL/SHM sidecars to0600; best-effort, Windows-skipped, chmod placed after WAL-mode init so sidecars exist before the call.project-security.ts): upgrading from field-level to block-level strip closes routing-override and enablement vectors the previous approach left open; however,dropInheritedEmbeddingKeyOnRedirectis now unreachable in production (see inline comment).Confidence Score: 4/5
Safe to merge — all three hardening changes behave correctly and the tests cover the key scenarios. One function in project-security.ts is now unreachable in practice and should be cleaned up in a follow-up.
The ATTACH escaping and permission tightening are straightforward and well-tested. The project-config strip is the most significant change and is correct in its effect, but it leaves
dropInheritedEmbeddingKeyOnRedirectas dead code: both call sites inconfig/index.tspass the already-strippedprojectRaw, so the function's early-return guard fires on every invocation and the key-drop logic never runs. This does not reopen any protection gap (the full-block strip is strictly stronger), but it creates a misleading safety net that future contributors may rely on or be confused by.packages/plugin/src/config/project-security.ts —
dropInheritedEmbeddingKeyOnRedirectand its two call sites inconfig/index.tscan be removed.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[loadPluginConfig called] --> B[Load user config\nexpand tokens] A --> C[Load project config\nNO token expansion] C --> D[stripUnsafeProjectConfigFields\nprojectRaw mutated in-place] D -->|deletes| E["auto_update\nsqlite\nembedding ← NEW\nhistorian ← NEW\ndreamer ← NEW\nsidekick ← NEW\nmemory.git_commit_indexing ← NEW"] D --> F[deepMergeRawConfig\nuser + project] B --> F F --> G[dropInheritedEmbeddingKeyOnRedirect\n⚠ now dead code:\nprojectRaw.embedding always absent] G --> H[parsePluginConfig / Zod] H --> I[MagicContextConfig returned] style E fill:#f9d,stroke:#c66 style G fill:#ffd,stroke:#aa0Comments Outside Diff (1)
packages/plugin/src/config/project-security.ts, line 119-161 (link)dropInheritedEmbeddingKeyOnRedirectis now dead code in every production call path. Both call sites inconfig/index.tscallstripUnsafeProjectConfigFields(projectRaw)first, which deletesprojectRaw.embeddingunconditionally. The very first guard in this function (if (!isPlainObject(projectRaw.embedding)) return []) will always short-circuit becauseembeddingis absent by the time the function is called. The function provides zero additional protection and its unit tests inproject-security.test.tstest it in isolation (without stripping first), so they do not surface the dead-code condition. Consider removing the function and its call sites now that the full-strip approach supersedes it.Reviews (1): Last reviewed commit: "plugin: strip agent and embedding overri..." | Re-trigger Greptile