fix(memory): block symlink escapes in memory_get#68597
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c28f73200
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR fixes a path traversal vulnerability in Confidence Score: 5/5Safe to merge; the fix is correctly scoped and all remaining findings are P2. The core security logic in resolveContainedRegularFile and isWithinRealRoot is correct and covers the stated attack surface. The only finding is a missing EPERM/EACCES guard on one new symlink creation in the QMD test — a P2 robustness concern that does not affect production behavior. extensions/memory-core/src/memory/qmd-manager.test.ts — symlink creation needs an EPERM/EACCES guard to match the pattern used in the parallel tests. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-core/src/memory/qmd-manager.test.ts
Line: 3569
Comment:
**Missing EPERM/EACCES guard for symlink creation**
The two new tests in `manager.read-file.test.ts` both wrap `fs.symlink` in a try/catch and skip the assertion when the OS denies symlink creation (`EPERM`/`EACCES`). This QMD test does not, so it would surface as a hard failure on Windows without Developer Mode or in any CI runner where symlink creation is restricted.
```suggestion
let symlinkOk = true;
try {
await fs.symlink(outsideDir, nestedLink, "dir");
} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
if (code === "EPERM" || code === "EACCES") {
symlinkOk = false;
} else {
throw err;
}
}
if (symlinkOk) {
await expect(
manager.readFile({ relPath: "qmd/workspace-main/linked-dir/secret.md" }),
).rejects.toThrow("path required");
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory): block symlink escapes in me..." | Re-trigger Greptile |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open: the PR targets a real memory_get symlink-boundary problem, but it is not merge-ready because current main has moved and partially rehardened the builtin path, the QMD read path still needs a current-main fix, and the branch is dirty with insufficient real behavior proof. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence reproduction path: on current main QMD readFile resolves qmd paths lexically, statRegularFile only rejects the final path if it is itself a symlink, and readFullText/readPartialText then read the resolved path through intermediate symlink components. I did not run a filesystem test because this review is read-only. Is this the best way to solve the issue? No, not as-is. The useful fix should be rebased onto current main's fs-safe memory host SDK, then QMD search/index/read policy should be made coherent instead of only rejecting reads after QMD may have indexed the symlinked file. Security review: Security review needs attention: The diff is security hardening, but QMD search/read boundary consistency needs attention before merge.
AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 1d2bebbb41bf. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
Summary
memory_getcould accept a path that lexically stayed under the workspace or an allowed extra path, but escaped through an intermediate directory symlink.memory_getcould be tricked into pulling content from outside the workspace or QMD memory boundary, which means normal memory lookups could unexpectedly surface unrelated local files instead of only the notes the user intended memory tools to access.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
Regression Test Plan (if applicable)
extensions/memory-core/src/memory/manager.read-file.test.tsextensions/memory-core/src/memory/qmd-manager.test.tsmemory_getreads when a path undermemory/, an allowed extra path, or a QMD collection crosses the boundary through an intermediate directory symlink.User-visible / Behavior Changes
memory_getnow rejects markdown paths that escape the allowed workspace / extra path / QMD collection through intermediate directory symlinks.Diagram (if applicable)
Security Impact (required)
NoNoNoNoYesYes, explain risk + mitigation:memory_getto the canonical allowed roots and blocks previously unintended cross-boundary reads through directory symlinks.Repro + Verification
Environment
Steps
memory/, an allowed extra path, or a QMD collection that points to that outside directory.memory_geton the symlinked markdown path.Expected
path required.Actual
Evidence
Human Verification (required)
pnpm testpnpm checkReview Conversations
Compatibility / Migration
YesNoNoRisks and Mitigations
memory/, an allowed extra path, or a QMD collection as an implicit way to reach files outside the configured memory roots will now fail onmemory_get.memorySearch.extraPathsor QMD collections; this change aligns runtime reads with the documented memory-root contract and adds regression coverage for the escaped-symlink case.