Skip to content

fix(memory): block symlink escapes in memory_get#68597

Open
GaosCode wants to merge 2 commits into
openclaw:mainfrom
GaosCode:fix/memory-get-symlink-boundary
Open

fix(memory): block symlink escapes in memory_get#68597
GaosCode wants to merge 2 commits into
openclaw:mainfrom
GaosCode:fix/memory-get-symlink-boundary

Conversation

@GaosCode

Copy link
Copy Markdown
Contributor

Summary

  • Problem: memory_get could accept a path that lexically stayed under the workspace or an allowed extra path, but escaped through an intermediate directory symlink.
  • Why it matters: an agent using memory_get could 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.
  • What changed: resolve target files and allowed roots to real paths before reading, then reject reads whose canonical path falls outside the allowed root; applied to both builtin memory reads and QMD-backed reads.
  • What did NOT change (scope boundary): no changes to memory search ranking, indexing behavior, prompt logic, or non-memory file access surfaces.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Memory / storage
  • API / contracts

Linked Issue/PR

  • Closes #
  • Related #
  • This PR fixes a bug or regression

Root Cause (if applicable)

  • Root cause: the read path checks only validated the lexical path prefix and direct symlink files, but did not verify that the resolved real path still stayed inside the allowed workspace / extraPath / QMD collection root.
  • Missing detection / guardrail: no regression test covered an intermediate directory symlink that escaped the allowed root.
  • Contributing context (if known): builtin and QMD read paths each enforced path rules separately, so both needed the same canonical-path containment check.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file:
    • extensions/memory-core/src/memory/manager.read-file.test.ts
    • extensions/memory-core/src/memory/qmd-manager.test.ts
  • Scenario the test should lock in: reject memory_get reads when a path under memory/, an allowed extra path, or a QMD collection crosses the boundary through an intermediate directory symlink.
  • Why this is the smallest reliable guardrail: the bug lives in path resolution and file-read authorization, so direct read-path tests cover it without requiring full indexing or end-to-end setup.
  • Existing test that already covers this (if any): existing tests already covered direct symlink files; this PR extends coverage to directory symlink escapes.
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

memory_get now rejects markdown paths that escape the allowed workspace / extra path / QMD collection through intermediate directory symlinks.

Diagram (if applicable)

Before:
memory_get("memory/linked/secret.md")
-> lexical prefix check passes
-> linked/ resolves outside workspace
-> file is read

After:
memory_get("memory/linked/secret.md")
-> lexical prefix check passes
-> realpath resolves outside allowed root
-> request fails with "path required"

Security Impact (required)

  • New permissions/capabilities? No
  • Secrets/tokens handling changed? No
  • New/changed network calls? No
  • Command/tool execution surface changed? No
  • Data access scope changed? Yes
  • If any Yes, explain risk + mitigation:
    • This narrows the readable file scope for memory_get to the canonical allowed roots and blocks previously unintended cross-boundary reads through directory symlinks.

Repro + Verification

Environment

  • OS: macOS
  • Runtime/container: local Node/Bun repo test runner
  • Model/provider: N/A
  • Integration/channel (if any): memory core / QMD
  • Relevant config (redacted): default local test config

Steps

  1. Create a markdown file outside the workspace or outside an allowed extra path / QMD collection root.
  2. Add an intermediate directory symlink inside memory/, an allowed extra path, or a QMD collection that points to that outside directory.
  3. Run memory_get on the symlinked markdown path.

Expected

  • The read is rejected with path required.

Actual

  • Before this fix, the read could succeed because only the lexical path prefix was checked.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

  • Verified scenarios:
    • builtin memory reads reject directory symlink escapes
    • extra-path memory reads reject nested directory symlink escapes
    • QMD reads reject directory symlink escapes
  • Edge cases checked:
    • direct symlink-file rejection still holds
    • transient file disappearance path still returns empty text correctly after switching reads to canonical paths
  • What you did not verify:
    • full pnpm test
    • full pnpm check

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? Yes
  • Config/env changes? No
  • Migration needed? No
  • If yes, exact upgrade steps: N/A

Risks and Mitigations

  • Risk: setups that used a directory symlink inside memory/, an allowed extra path, or a QMD collection as an implicit way to reach files outside the configured memory roots will now fail on memory_get.
    • Mitigation: supported outside-workspace reads should be configured explicitly through memorySearch.extraPaths or QMD collections; this change aligns runtime reads with the documented memory-root contract and adds regression coverage for the escaped-symlink case.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/memory-host-sdk/host/fs-utils.ts Outdated
@greptile-apps

greptile-apps Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a path traversal vulnerability in memory_get by adding a canonical real-path containment check (resolveContainedRegularFile) that resolves both the target file and the allowed roots via fs.realpath before verifying the file is within bounds. The fix is applied consistently to both the builtin memory read path (read-file.ts) and the QMD-backed read path (qmd-manager.ts), and regression tests are added for directory-symlink escapes in all three read contexts (workspace memory, extra paths, and QMD collections).

Confidence Score: 5/5

Safe 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 AI
This 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

Comment thread extensions/memory-core/src/memory/qmd-manager.test.ts Outdated
@clawsweeper

clawsweeper Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

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 details

Best 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.

  • [medium] QMD search/read boundary can diverge — extensions/memory-core/src/memory/qmd-manager.ts:1197
    QMD may still index or surface symlink-followed files while this patch rejects memory_get for those same paths, so the data-access boundary remains inconsistent unless QMD indexing or docs are updated with the read change.
    Confidence: 0.82

AGENTS.md: found and applied where relevant.

What I checked:

  • stale F-rated PR: PR was opened 2026-04-18T14:25:27Z, is older than 30 days, and the latest review rated it F.
  • proof blocker: real behavior proof is insufficient and proof tier is F, so this branch is not merge-ready without contributor follow-up.
  • no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

  • vincentkoc: Current main blame/history for the moved memory host SDK and QMD files is dominated by recent package/baseline commits from Vincent Koc. (role: recent area contributor; confidence: medium; commits: ecec1b9a59df, 2e08f0f4221f; files: packages/memory-host-sdk/src/host/read-file.ts, packages/memory-host-sdk/src/host/fs-utils.ts, extensions/memory-core/src/memory/qmd-manager.ts)
  • Takhoffman: PR fix(context-window): Tighten context limits and bound memory excerpts #67277 changed memory read-file and QMD read behavior shortly before this PR. (role: recent memory read refactor author; confidence: high; commits: 4f00b769251d; files: packages/memory-host-sdk/src/host/read-file.ts, extensions/memory-core/src/memory/qmd-manager.ts, extensions/memory-core/src/memory/manager.read-file.test.ts)
  • eleqtrizit: PR Align QMD memory reads with canonical memory paths #66026 aligned QMD memory reads with canonical memory paths and touched the central QMD read path. (role: QMD read path contributor; confidence: high; commits: 37d5971db364; files: extensions/memory-core/src/memory/qmd-manager.ts, extensions/memory-core/src/memory/qmd-manager.test.ts)
  • samzong: PR [Fix] Block memory extra path symlink traversal #80331 merged the related extra-path symlink traversal hardening now present on main. (role: adjacent symlink traversal fix author; confidence: medium; commits: 1a7efcad4b20; files: packages/memory-host-sdk/src/host/read-file.ts, packages/memory-host-sdk/src/host/read-file.test.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 1d2bebbb41bf.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 19, 2026
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 19, 2026
@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels May 20, 2026
@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jun 6, 2026
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 6, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Jun 7, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extensions: memory-core Extension: memory-core merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant