security: harden local defaults, viewer CSP, mesh auth, and export confinement#108
Merged
rohitg00 merged 3 commits intorohitg00:mainfrom Apr 12, 2026
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR tightens local network bindings, hardens viewer CSP using per-response nonces and delegated event handling, requires AGENTMEMORY_SECRET for mesh sync and includes it on outbound mesh requests, enforces an export root for Obsidian exports, and removes the automatic iii-engine installer from the CLI. Changes
Sequence Diagram(s)sequenceDiagram
participant Local as Local AgentMemory
participant HTTP as HTTP Fetch
participant Peer as Remote Peer (https://peer)
Local->>Local: mem::mesh-sync invoked
Local->>Local: require AGENTMEMORY_SECRET check
alt secret missing
Local-->>Local: return { success: false, error: "mesh sync requires AGENTMEMORY_SECRET" }
else secret present
Local->>HTTP: POST https://peer/agentmemory/mesh/receive
Note right of HTTP: Authorization: Bearer <AGENTMEMORY_SECRET>\nContent-Type: application/json
HTTP->>Peer: deliver push payload
Peer-->>HTTP: 200 OK
HTTP-->>Local: response
Local->>HTTP: GET https://peer/agentmemory/mesh/export?...
Note right of HTTP: Authorization: Bearer <AGENTMEMORY_SECRET>
HTTP->>Peer: request export
Peer-->>HTTP: export payload
HTTP-->>Local: response (process results)
Local-->>Local: aggregate results and return success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/viewer/server.ts (1)
86-100:⚠️ Potential issue | 🟠 MajorDo not serve
/agentmemory/vieweras an unauthenticated local alias.Line 90 makes the exact
/agentmemory/viewerroute bypass the authenticated API handler and return HTML directly on the viewer port. WhenAGENTMEMORY_SECRETis set, that path is still public on127.0.0.1:${port}even though the REST API returns 401 for the same route.Proposed fix
- if ( - method === "GET" && - (pathname === "/" || - pathname === "/viewer" || - pathname === "/agentmemory/viewer") - ) { + if (method === "GET" && (pathname === "/" || pathname === "/viewer")) { const rendered = renderViewerDocument(); if (rendered.found) { res.writeHead(200, { "Content-Type": "text/html; charset=utf-8", "Content-Security-Policy": rendered.csp,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/viewer/server.ts` around lines 86 - 100, The route check in the HTTP handler is incorrectly allowing the exact path "/agentmemory/viewer" to bypass authentication and serve renderViewerDocument() HTML directly; update the condition in the request handling logic (the GET path check that currently references pathname === "/agentmemory/viewer") to only allow "/" and "/viewer" (keep renderViewerDocument() usage intact for those) and remove "/agentmemory/viewer" so that requests to "/agentmemory/viewer" are handled by the authenticated API flow/authorization checks already used for other agentmemory endpoints (ensure any authentication logic that runs for other agentmemory routes also runs for "/agentmemory/viewer").src/cli.ts (1)
100-140:⚠️ Potential issue | 🟠 MajorCheck known local
iiilocations before falling back to Docker.Lines 100-115 start Docker before Lines 117-140 scan
~/.local/bin/iiiand/usr/local/bin/iii, so a valid local install outsidePATHis ignored whenever Docker is available. That reverses the intended precedence.Proposed fix
- const dockerBin = whichBinary("docker"); - const dockerCompose = join(__dirname, "..", "docker-compose.yml"); - const dcExists = existsSync(dockerCompose) || existsSync(join(process.cwd(), "docker-compose.yml")); - - if (dockerBin && dcExists) { - const composeFile = existsSync(dockerCompose) ? dockerCompose : join(process.cwd(), "docker-compose.yml"); - const s = p.spinner(); - s.start("Starting iii-engine via Docker..."); - const child = spawn(dockerBin, ["compose", "-f", composeFile, "up", "-d"], { - detached: true, - stdio: "ignore", - }); - child.unref(); - s.stop("Docker compose started"); - return true; - } - const iiiPaths = [ join(process.env["HOME"] || "", ".local", "bin", "iii"), "/usr/local/bin/iii", ]; for (const iiiPath of iiiPaths) { @@ if (iiiBin && configPath) { const s = p.spinner(); s.start(`Starting iii-engine: ${iiiBin}`); const child = spawn(iiiBin, ["--config", configPath], { detached: true, stdio: "ignore", }); child.unref(); s.stop("iii-engine process started"); return true; } + + const dockerBin = whichBinary("docker"); + const dockerCompose = join(__dirname, "..", "docker-compose.yml"); + const dcExists = + existsSync(dockerCompose) || + existsSync(join(process.cwd(), "docker-compose.yml")); + + if (dockerBin && dcExists) { + const composeFile = existsSync(dockerCompose) + ? dockerCompose + : join(process.cwd(), "docker-compose.yml"); + const s = p.spinner(); + s.start("Starting iii-engine via Docker..."); + const child = spawn(dockerBin, ["compose", "-f", composeFile, "up", "-d"], { + detached: true, + stdio: "ignore", + }); + child.unref(); + s.stop("Docker compose started"); + return true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 100 - 140, The start logic currently prefers Docker when dockerBin && dcExists, which causes local installs in iiiPaths (~/.local/bin/iii and /usr/local/bin/iii) to be ignored; move or run the iiiPaths discovery before the Docker branch (or change the condition to prefer iiiBin when found) so that the loop that sets iiiBin (checking iiiPaths, logging via p.log.info and updating process.env.PATH) executes before deciding to spawn Docker; then if iiiBin && configPath spawn the local iii process (using the same spinner/child.unref flow) and only fall back to the docker compose spawn (dockerBin + dcExists) if no local iiiBin was found.
🧹 Nitpick comments (5)
test/privacy.test.ts (1)
65-69: Add explicitghu_regression coverageLine [65] adds
ghs_coverage, but the regex insrc/functions/privacy.tsLine [11] also targetsghu_. Add oneghu_...case to lock that behavior.➕ Suggested test addition
it("strips GitHub fine-grained service tokens", () => { expect( stripPrivateData("ghs_1234567890abcdefghijklmnopqrstuvwxyzAB"), ).toBe("[REDACTED_SECRET]"); }); + + it("strips GitHub user tokens", () => { + expect( + stripPrivateData("ghu_1234567890abcdefghijklmnopqrstuvwxyzAB"), + ).toBe("[REDACTED_SECRET]"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/privacy.test.ts` around lines 65 - 69, Add an explicit test case exercising the `ghu_` prefix to prevent a regression: in the `privacy.test.ts` suite add an `it` case (similar to the existing `ghs_` test) that calls `stripPrivateData("ghu_1234567890abcdefghijklmnopqrstuvwxyzAB")` and asserts it returns `"[REDACTED_SECRET]"`; this ensures the `stripPrivateData` behavior (the regex in `src/functions/privacy.ts` that also targets `ghu_`) stays covered.src/viewer/document.ts (1)
10-23: Consider caching the template after first load.
loadViewerTemplate()usesreadFileSyncon every call. For high-traffic scenarios, consider caching the template content after the first successful read.💡 Optional: Cache template after first load
+let cachedTemplate: string | null = null; + function loadViewerTemplate(): string | null { + if (cachedTemplate !== null) { + return cachedTemplate; + } const base = dirname(fileURLToPath(import.meta.url)); const candidates = [ join(base, "..", "src", "viewer", "index.html"), join(base, "..", "viewer", "index.html"), join(base, "viewer", "index.html"), ]; for (const path of candidates) { try { - return readFileSync(path, "utf-8"); + cachedTemplate = readFileSync(path, "utf-8"); + return cachedTemplate; } catch {} } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/viewer/document.ts` around lines 10 - 23, The loadViewerTemplate function currently calls readFileSync on each invocation; add a module-level cache (e.g., a let cachedViewerTemplate: string | null = undefined) and modify loadViewerTemplate to return the cached value if defined, otherwise read the file once, assign the result to cachedViewerTemplate, and return it; keep the existing candidates loop and null return behavior if no file found. Ensure the cache variable is exported or visible to the module scope so subsequent calls to loadViewerTemplate reuse the cached content.test/obsidian-export.test.ts (1)
123-132: Consider cleaning up the environment variable after tests.Setting
process.env.AGENTMEMORY_EXPORT_ROOTwithout cleanup could affect other test suites if they run in the same process.💡 Optional: Clean up env in afterEach
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; ... describe("Obsidian Export", () => { let sdk: ReturnType<typeof mockSdk>; let kv: ReturnType<typeof mockKV>; const exportRoot = "/tmp/agentmemory-export-root"; + let originalExportRoot: string | undefined; beforeEach(() => { + originalExportRoot = process.env.AGENTMEMORY_EXPORT_ROOT; process.env.AGENTMEMORY_EXPORT_ROOT = exportRoot; ... }); + afterEach(() => { + if (originalExportRoot === undefined) { + delete process.env.AGENTMEMORY_EXPORT_ROOT; + } else { + process.env.AGENTMEMORY_EXPORT_ROOT = originalExportRoot; + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/obsidian-export.test.ts` around lines 123 - 132, The test sets process.env.AGENTMEMORY_EXPORT_ROOT in the beforeEach block but doesn't restore it; add an afterEach that deletes or restores process.env.AGENTMEMORY_EXPORT_ROOT to avoid leaking into other tests. Specifically, in the same test file where beforeEach sets exportRoot and calls registerObsidianExportFunction(sdk, kv), add an afterEach that does delete process.env.AGENTMEMORY_EXPORT_ROOT (or restores a saved previous value) and retains existing cleanup of writtenFiles and createdDirs so the environment is fully cleaned between tests.package.json (1)
13-13: Build script correctly updated for Docker configuration.The additional copy step for
iii-config.docker.yamlaligns with the new Docker-specific config. For consistency, consider addingiii-config.docker.yamlto thefilesarray (lines 33-41) alongsideiii-config.yamlanddocker-compose.yml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 13, The package.json build script copies iii-config.docker.yaml into dist but the package "files" array does not include it; update the "files" array to include "iii-config.docker.yaml" alongside "iii-config.yaml" and "docker-compose.yml" so the Docker-specific config is published with the package (locate the "files" array in package.json and add the filename).test/viewer-security.test.ts (1)
17-24: Make the inline-handler assertion generic.This only guards three attribute names, so
onload=,onerror=, etc. can slip back in without failing the test. A single regex keeps the regression aligned with the XSS goal.Proposed fix
- expect(rendered.html).not.toContain("onclick="); - expect(rendered.html).not.toContain("onmouseover="); - expect(rendered.html).not.toContain("onmouseout="); + expect(rendered.html).not.toMatch(/\son[a-z]+\s*=/i);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/viewer-security.test.ts` around lines 17 - 24, The test for inline DOM event handlers should be made generic: in the "does not contain inline DOM event handlers" case (using renderViewerDocument() and the rendered.html result), replace the three specific toContain checks with a single assertion that rendered.html does not match a regex for any on* attribute (e.g. use a case-insensitive pattern like /\bon\w+\s*=/i or similar) so any inline event handlers (onload, onerror, onclick, etc.) will fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/mesh.test.ts`:
- Around line 204-233: The test currently stubs global fetch with
vi.stubGlobal("fetch", fetchMock) but only calls vi.unstubAllGlobals() at the
end of the test body so a failing expectation can leave fetch stubbed; fix by
guaranteeing cleanup: wrap the test body that sets fetchMock and calls
authedSdk.trigger(...) in a try/finally where vi.unstubAllGlobals() is invoked
in finally, or move the cleanup into an afterEach hook that calls
vi.unstubAllGlobals(); update references to fetchMock/vi.stubGlobal and ensure
the finally/afterEach runs regardless of test failures.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 100-140: The start logic currently prefers Docker when dockerBin
&& dcExists, which causes local installs in iiiPaths (~/.local/bin/iii and
/usr/local/bin/iii) to be ignored; move or run the iiiPaths discovery before the
Docker branch (or change the condition to prefer iiiBin when found) so that the
loop that sets iiiBin (checking iiiPaths, logging via p.log.info and updating
process.env.PATH) executes before deciding to spawn Docker; then if iiiBin &&
configPath spawn the local iii process (using the same spinner/child.unref flow)
and only fall back to the docker compose spawn (dockerBin + dcExists) if no
local iiiBin was found.
In `@src/viewer/server.ts`:
- Around line 86-100: The route check in the HTTP handler is incorrectly
allowing the exact path "/agentmemory/viewer" to bypass authentication and serve
renderViewerDocument() HTML directly; update the condition in the request
handling logic (the GET path check that currently references pathname ===
"/agentmemory/viewer") to only allow "/" and "/viewer" (keep
renderViewerDocument() usage intact for those) and remove "/agentmemory/viewer"
so that requests to "/agentmemory/viewer" are handled by the authenticated API
flow/authorization checks already used for other agentmemory endpoints (ensure
any authentication logic that runs for other agentmemory routes also runs for
"/agentmemory/viewer").
---
Nitpick comments:
In `@package.json`:
- Line 13: The package.json build script copies iii-config.docker.yaml into dist
but the package "files" array does not include it; update the "files" array to
include "iii-config.docker.yaml" alongside "iii-config.yaml" and
"docker-compose.yml" so the Docker-specific config is published with the package
(locate the "files" array in package.json and add the filename).
In `@src/viewer/document.ts`:
- Around line 10-23: The loadViewerTemplate function currently calls
readFileSync on each invocation; add a module-level cache (e.g., a let
cachedViewerTemplate: string | null = undefined) and modify loadViewerTemplate
to return the cached value if defined, otherwise read the file once, assign the
result to cachedViewerTemplate, and return it; keep the existing candidates loop
and null return behavior if no file found. Ensure the cache variable is exported
or visible to the module scope so subsequent calls to loadViewerTemplate reuse
the cached content.
In `@test/obsidian-export.test.ts`:
- Around line 123-132: The test sets process.env.AGENTMEMORY_EXPORT_ROOT in the
beforeEach block but doesn't restore it; add an afterEach that deletes or
restores process.env.AGENTMEMORY_EXPORT_ROOT to avoid leaking into other tests.
Specifically, in the same test file where beforeEach sets exportRoot and calls
registerObsidianExportFunction(sdk, kv), add an afterEach that does delete
process.env.AGENTMEMORY_EXPORT_ROOT (or restores a saved previous value) and
retains existing cleanup of writtenFiles and createdDirs so the environment is
fully cleaned between tests.
In `@test/privacy.test.ts`:
- Around line 65-69: Add an explicit test case exercising the `ghu_` prefix to
prevent a regression: in the `privacy.test.ts` suite add an `it` case (similar
to the existing `ghs_` test) that calls
`stripPrivateData("ghu_1234567890abcdefghijklmnopqrstuvwxyzAB")` and asserts it
returns `"[REDACTED_SECRET]"`; this ensures the `stripPrivateData` behavior (the
regex in `src/functions/privacy.ts` that also targets `ghu_`) stays covered.
In `@test/viewer-security.test.ts`:
- Around line 17-24: The test for inline DOM event handlers should be made
generic: in the "does not contain inline DOM event handlers" case (using
renderViewerDocument() and the rendered.html result), replace the three specific
toContain checks with a single assertion that rendered.html does not match a
regex for any on* attribute (e.g. use a case-insensitive pattern like
/\bon\w+\s*=/i or similar) so any inline event handlers (onload, onerror,
onclick, etc.) will fail the test.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb278fb2-3322-4e21-9c82-bde3e02aa535
📒 Files selected for processing (20)
README.mddocker-compose.ymliii-config.docker.yamliii-config.yamlpackage.jsonsrc/auth.tssrc/cli.tssrc/functions/mesh.tssrc/functions/obsidian-export.tssrc/functions/privacy.tssrc/index.tssrc/triggers/api.tssrc/viewer/document.tssrc/viewer/index.htmlsrc/viewer/server.tstest/integration.test.tstest/mesh.test.tstest/obsidian-export.test.tstest/privacy.test.tstest/viewer-security.test.ts
Comment on lines
+204
to
+233
| const fetchMock = vi.fn(async () => | ||
| new Response(JSON.stringify({ accepted: 0 }), { | ||
| status: 200, | ||
| headers: { "Content-Type": "application/json" }, | ||
| }), | ||
| ); | ||
| vi.stubGlobal("fetch", fetchMock); | ||
|
|
||
| const regResult = (await authedSdk.trigger("mem::mesh-register", { | ||
| url: "https://peer2.example.com", | ||
| name: "peer-2", | ||
| })) as { success: boolean; peer: MeshPeer }; | ||
|
|
||
| const result = (await authedSdk.trigger("mem::mesh-sync", { | ||
| peerId: regResult.peer.id, | ||
| direction: "push", | ||
| })) as { success: boolean; results: Array<{ errors: string[] }> }; | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.results[0].errors).toEqual([]); | ||
| expect(fetchMock).toHaveBeenCalledWith( | ||
| "https://peer2.example.com/agentmemory/mesh/receive", | ||
| expect.objectContaining({ | ||
| headers: expect.objectContaining({ | ||
| Authorization: "Bearer mesh-secret", | ||
| }), | ||
| }), | ||
| ); | ||
|
|
||
| vi.unstubAllGlobals(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full test file structure and the specific test in question
cat -n test/mesh.test.ts | head -250Repository: rohitg00/agentmemory
Length of output: 9632
🏁 Script executed:
# Check for other uses of vi.stubGlobal in the file to see patterns
rg "vi\.stubGlobal|vi\.unstubAllGlobals" test/mesh.test.ts -n -B 2 -A 2Repository: rohitg00/agentmemory
Length of output: 295
🏁 Script executed:
# Check the test/crystallize.test.ts pattern mentioned in guidelines to understand expected patterns
rg "vi\.stubGlobal|try.*finally|afterEach" test/crystallize.test.ts -n -B 1 -A 1Repository: rohitg00/agentmemory
Length of output: 46
🏁 Script executed:
# Check test/crystallize.test.ts for patterns
head -100 test/crystallize.test.tsRepository: rohitg00/agentmemory
Length of output: 3176
🏁 Script executed:
# Check if there are other vi.stubGlobal usages in the test suite
rg "vi\.stubGlobal" test/ -nRepository: rohitg00/agentmemory
Length of output: 126
🏁 Script executed:
# Check the broader test structure - look for other tests using mocks or stubs
rg "afterEach|try.*finally" test/mesh.test.ts -nRepository: rohitg00/agentmemory
Length of output: 46
🏁 Script executed:
# Check if the test suite uses afterEach anywhere for cleanup
grep -n "afterEach" test/mesh.test.tsRepository: rohitg00/agentmemory
Length of output: 46
🏁 Script executed:
# Check the broader vitest setup and patterns in the codebase
find test -name "*.test.ts" -type f | head -5Repository: rohitg00/agentmemory
Length of output: 174
🏁 Script executed:
# See if there are any examples of proper global stub cleanup in the test suite
rg "unstubAllGlobals|afterEach" test/ -B 3 -A 1Repository: rohitg00/agentmemory
Length of output: 1564
🏁 Script executed:
# Let's check if there are any other tests in mesh.test.ts that might be affected by the fetch stub
wc -l test/mesh.test.tsRepository: rohitg00/agentmemory
Length of output: 85
🏁 Script executed:
# Look at the complete afterEach pattern in other test files for consistency
rg -A 5 "afterEach" test/index-persistence.test.ts test/embedding-provider.test.ts test/circuit-breaker.test.ts | head -30Repository: rohitg00/agentmemory
Length of output: 1883
🏁 Script executed:
# Verify the actual test structure - is the mesh-sync test the last test or are there tests after it?
sed -n '183,250p' test/mesh.test.tsRepository: rohitg00/agentmemory
Length of output: 2316
Always restore the global fetch stub with guaranteed cleanup.
If any expectation before line 233 fails, fetch stays stubbed and can cascade failures into the subsequent mesh-receive test suite. Wrap the test body in try/finally or move cleanup to afterEach.
Proposed fix
vi.stubGlobal("fetch", fetchMock);
-
- const regResult = (await authedSdk.trigger("mem::mesh-register", {
- url: "https://peer2.example.com",
- name: "peer-2",
- })) as { success: boolean; peer: MeshPeer };
-
- const result = (await authedSdk.trigger("mem::mesh-sync", {
- peerId: regResult.peer.id,
- direction: "push",
- })) as { success: boolean; results: Array<{ errors: string[] }> };
-
- expect(result.success).toBe(true);
- expect(result.results[0].errors).toEqual([]);
- expect(fetchMock).toHaveBeenCalledWith(
- "https://peer2.example.com/agentmemory/mesh/receive",
- expect.objectContaining({
- headers: expect.objectContaining({
- Authorization: "Bearer mesh-secret",
- }),
- }),
- );
-
- vi.unstubAllGlobals();
+ try {
+ const regResult = (await authedSdk.trigger("mem::mesh-register", {
+ url: "https://peer2.example.com",
+ name: "peer-2",
+ })) as { success: boolean; peer: MeshPeer };
+
+ const result = (await authedSdk.trigger("mem::mesh-sync", {
+ peerId: regResult.peer.id,
+ direction: "push",
+ })) as { success: boolean; results: Array<{ errors: string[] }> };
+
+ expect(result.success).toBe(true);
+ expect(result.results[0].errors).toEqual([]);
+ expect(fetchMock).toHaveBeenCalledWith(
+ "https://peer2.example.com/agentmemory/mesh/receive",
+ expect.objectContaining({
+ headers: expect.objectContaining({
+ Authorization: "Bearer mesh-secret",
+ }),
+ }),
+ );
+ } finally {
+ vi.unstubAllGlobals();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const fetchMock = vi.fn(async () => | |
| new Response(JSON.stringify({ accepted: 0 }), { | |
| status: 200, | |
| headers: { "Content-Type": "application/json" }, | |
| }), | |
| ); | |
| vi.stubGlobal("fetch", fetchMock); | |
| const regResult = (await authedSdk.trigger("mem::mesh-register", { | |
| url: "https://peer2.example.com", | |
| name: "peer-2", | |
| })) as { success: boolean; peer: MeshPeer }; | |
| const result = (await authedSdk.trigger("mem::mesh-sync", { | |
| peerId: regResult.peer.id, | |
| direction: "push", | |
| })) as { success: boolean; results: Array<{ errors: string[] }> }; | |
| expect(result.success).toBe(true); | |
| expect(result.results[0].errors).toEqual([]); | |
| expect(fetchMock).toHaveBeenCalledWith( | |
| "https://peer2.example.com/agentmemory/mesh/receive", | |
| expect.objectContaining({ | |
| headers: expect.objectContaining({ | |
| Authorization: "Bearer mesh-secret", | |
| }), | |
| }), | |
| ); | |
| vi.unstubAllGlobals(); | |
| const fetchMock = vi.fn(async () => | |
| new Response(JSON.stringify({ accepted: 0 }), { | |
| status: 200, | |
| headers: { "Content-Type": "application/json" }, | |
| }), | |
| ); | |
| vi.stubGlobal("fetch", fetchMock); | |
| try { | |
| const regResult = (await authedSdk.trigger("mem::mesh-register", { | |
| url: "https://peer2.example.com", | |
| name: "peer-2", | |
| })) as { success: boolean; peer: MeshPeer }; | |
| const result = (await authedSdk.trigger("mem::mesh-sync", { | |
| peerId: regResult.peer.id, | |
| direction: "push", | |
| })) as { success: boolean; results: Array<{ errors: string[] }> }; | |
| expect(result.success).toBe(true); | |
| expect(result.results[0].errors).toEqual([]); | |
| expect(fetchMock).toHaveBeenCalledWith( | |
| "https://peer2.example.com/agentmemory/mesh/receive", | |
| expect.objectContaining({ | |
| headers: expect.objectContaining({ | |
| Authorization: "Bearer mesh-secret", | |
| }), | |
| }), | |
| ); | |
| } finally { | |
| vi.unstubAllGlobals(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/mesh.test.ts` around lines 204 - 233, The test currently stubs global
fetch with vi.stubGlobal("fetch", fetchMock) but only calls
vi.unstubAllGlobals() at the end of the test body so a failing expectation can
leave fetch stubbed; fix by guaranteeing cleanup: wrap the test body that sets
fetchMock and calls authedSdk.trigger(...) in a try/finally where
vi.unstubAllGlobals() is invoked in finally, or move the cleanup into an
afterEach hook that calls vi.unstubAllGlobals(); update references to
fetchMock/vi.stubGlobal and ensure the finally/afterEach runs regardless of test
failures.
eng-pf
force-pushed
the
codex/upstream-security-pr
branch
from
bb8ced2 to
8843485
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
720-725:⚠️ Potential issue | 🟡 MinorUpdate the
/agentmemory/healthauth docs.The table still says
/agentmemory/healthis “always public”, butsrc/triggers/api.tsrunscheckAuth(req, secret)inapi::health, so it returns401onceAGENTMEMORY_SECRETis configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 720 - 725, The README table incorrectly states `/agentmemory/health` is always public; update the documentation entry for the `GET /agentmemory/health` row to reflect that `api::health` calls `checkAuth(req, secret)` and therefore requires `Authorization: Bearer <secret>` when `AGENTMEMORY_SECRET` is configured (otherwise remains public). Edit the table description to something like: "Health check with metrics; requires Authorization when AGENTMEMORY_SECRET is set" so it matches the behavior implemented in `api::health`/`checkAuth`.src/triggers/api.ts (1)
1434-1446:⚠️ Potential issue | 🟠 MajorWhitelist mesh payloads before calling
sdk.trigger().These handlers still pass
req.body/req.body || {}straight through to the internal functions. Since mesh endpoints are peer-exposed system boundaries, construct explicit payloads from known fields/scopes here and validate their types before dispatching.As per coding guidelines "REST endpoint handlers must validate inputs, whitelist request body fields, and never pass raw request body directly to sdk.trigger()" and "Input validation must occur at system boundaries (MCP handlers, REST endpoints) in TypeScript".
Also applies to: 1473-1482, 1491-1499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/triggers/api.ts` around lines 1434 - 1446, The api::mesh-register REST handler currently forwards req.body directly into sdk.trigger("mem::mesh-register"); instead, validate and whitelist the expected fields (url: string, name: string, sharedScopes?: string[]), coerce/verify types, build a new payload object (e.g., { url, name, sharedScopes }) and pass that to sdk.trigger; keep the existing secret/auth checks (requireConfiguredSecret and checkAuth) but do not pass req.body or allow extra properties through. Apply the same pattern to the other mesh-related handlers that call sdk.trigger for mesh operations to ensure all inputs are validated and only whitelisted fields are sent to mem::mesh-* triggers.src/viewer/server.ts (1)
70-108:⚠️ Potential issue | 🔴 CriticalDon't turn the viewer port into an auth-bypassing REST proxy.
This server no longer authenticates proxied requests, but
proxyToRestApi()still injectsAuthorization: Bearer ${secret}upstream. That makes port 3113 an unauthenticated gateway to every protected/agentmemory/*route for any local caller, including browser-driven CSRF-style requests to loopback services. Keep/vieweritself local-friendly, but require auth or proxy only a tight allowlist for the rest.Also applies to: 145-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/viewer/server.ts` around lines 70 - 108, The server currently proxies arbitrary requests to proxyToRestApi(resolvedRestPort, pathname, qs, method, req, res, secret) and always injects Authorization: Bearer ${secret}, turning the viewer port into an auth-bypassing gateway; fix by enforcing auth or an allowlist before calling proxyToRestApi: either (A) require the incoming request to present valid credentials (e.g., check req.headers.authorization matches the expected secret) and only then forward using proxyToRestApi, or (B) restrict proxied paths to a small explicit allowlist (e.g., only specific safe routes) and refuse/404 everything else; do not call proxyToRestApi with the secret for unauthenticated requests and do not blindly add Authorization when the incoming request lacks valid auth. Ensure the checks are implemented in the createServer handler around the proxyToRestApi invocation and reference the symbols proxyToRestApi, resolvedRestPort, secret, pathname, and req.headers.authorization.
🧹 Nitpick comments (1)
test/obsidian-export.test.ts (1)
234-241: Assert that the rejected export is side-effect free.This regression only checks the returned error. Add assertions that no directories or files were created, so the test fails if a future change performs partial writes before returning.
Suggested assertion additions
expect(result.success).toBe(false); expect(result.error).toContain(exportRoot); + expect(createdDirs.size).toBe(0); + expect(writtenFiles.size).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/obsidian-export.test.ts` around lines 234 - 241, The test "rejects vaultDir outside the export root" currently only checks the returned error; update it to also assert the operation is side-effect free by verifying no files or directories were created under the export root after calling sdk.trigger("mem::obsidian-export"). After the expect(result.success).toBe(false) line, add assertions that the exportRoot (and any expected child export path used by this test) either does not exist or remains empty/unmodified (use fs.existsSync/fs.readdirSync or the test suite's FS helpers) so the test will fail if mem::obsidian-export performs any partial writes before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/functions/obsidian-export.ts`:
- Around line 20-27: The current resolveVaultDir allows escaping via symlinks (a
path under export root that is a symlink to outside), so update resolveVaultDir
to validate real filesystem paths: compute realRoot =
realpathSync(getExportRoot()) and realResolved = realpathSync(resolved) and
ensure realResolved === realRoot or realResolved.startsWith(realRoot + sep);
alternatively, explicitly walk each path segment from root to resolved using
lstatSync and reject if any segment is a symlink before permitting later
writeFile() operations. Use resolveVaultDir as the single place to enforce this
check so writeFile() never receives an approved but symlinked path.
- Around line 16-18: The getExportRoot function should expand a leading "~" in
the AGENTMEMORY_EXPORT_ROOT env var before resolving so "~/.agentmemory" maps to
the user's home; update getExportRoot to check
process.env["AGENTMEMORY_EXPORT_ROOT"], and if it starts with "~" replace that
leading "~" with os.homedir() (or homedir()) then call resolve on the resulting
path; keep fallback to DEFAULT_EXPORT_ROOT unchanged so existing behavior using
DEFAULT_EXPORT_ROOT and homedir() remains intact.
---
Outside diff comments:
In `@README.md`:
- Around line 720-725: The README table incorrectly states `/agentmemory/health`
is always public; update the documentation entry for the `GET
/agentmemory/health` row to reflect that `api::health` calls `checkAuth(req,
secret)` and therefore requires `Authorization: Bearer <secret>` when
`AGENTMEMORY_SECRET` is configured (otherwise remains public). Edit the table
description to something like: "Health check with metrics; requires
Authorization when AGENTMEMORY_SECRET is set" so it matches the behavior
implemented in `api::health`/`checkAuth`.
In `@src/triggers/api.ts`:
- Around line 1434-1446: The api::mesh-register REST handler currently forwards
req.body directly into sdk.trigger("mem::mesh-register"); instead, validate and
whitelist the expected fields (url: string, name: string, sharedScopes?:
string[]), coerce/verify types, build a new payload object (e.g., { url, name,
sharedScopes }) and pass that to sdk.trigger; keep the existing secret/auth
checks (requireConfiguredSecret and checkAuth) but do not pass req.body or allow
extra properties through. Apply the same pattern to the other mesh-related
handlers that call sdk.trigger for mesh operations to ensure all inputs are
validated and only whitelisted fields are sent to mem::mesh-* triggers.
In `@src/viewer/server.ts`:
- Around line 70-108: The server currently proxies arbitrary requests to
proxyToRestApi(resolvedRestPort, pathname, qs, method, req, res, secret) and
always injects Authorization: Bearer ${secret}, turning the viewer port into an
auth-bypassing gateway; fix by enforcing auth or an allowlist before calling
proxyToRestApi: either (A) require the incoming request to present valid
credentials (e.g., check req.headers.authorization matches the expected secret)
and only then forward using proxyToRestApi, or (B) restrict proxied paths to a
small explicit allowlist (e.g., only specific safe routes) and refuse/404
everything else; do not call proxyToRestApi with the secret for unauthenticated
requests and do not blindly add Authorization when the incoming request lacks
valid auth. Ensure the checks are implemented in the createServer handler around
the proxyToRestApi invocation and reference the symbols proxyToRestApi,
resolvedRestPort, secret, pathname, and req.headers.authorization.
---
Nitpick comments:
In `@test/obsidian-export.test.ts`:
- Around line 234-241: The test "rejects vaultDir outside the export root"
currently only checks the returned error; update it to also assert the operation
is side-effect free by verifying no files or directories were created under the
export root after calling sdk.trigger("mem::obsidian-export"). After the
expect(result.success).toBe(false) line, add assertions that the exportRoot (and
any expected child export path used by this test) either does not exist or
remains empty/unmodified (use fs.existsSync/fs.readdirSync or the test suite's
FS helpers) so the test will fail if mem::obsidian-export performs any partial
writes before returning.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32a08d2c-2e84-4d75-b8c3-3abf6c35ceed
📒 Files selected for processing (20)
README.mddocker-compose.ymliii-config.docker.yamliii-config.yamlpackage.jsonsrc/auth.tssrc/cli.tssrc/functions/mesh.tssrc/functions/obsidian-export.tssrc/functions/privacy.tssrc/index.tssrc/triggers/api.tssrc/viewer/document.tssrc/viewer/index.htmlsrc/viewer/server.tstest/integration.test.tstest/mesh.test.tstest/obsidian-export.test.tstest/privacy.test.tstest/viewer-security.test.ts
✅ Files skipped from review due to trivial changes (5)
- package.json
- iii-config.yaml
- test/privacy.test.ts
- docker-compose.yml
- iii-config.docker.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- src/index.ts
- test/integration.test.ts
- test/viewer-security.test.ts
- src/viewer/document.ts
- src/functions/privacy.ts
- src/functions/mesh.ts
- src/cli.ts
- src/viewer/index.html
Comment on lines
+16
to
+18
| function getExportRoot(): string { | ||
| return resolve(process.env["AGENTMEMORY_EXPORT_ROOT"] || DEFAULT_EXPORT_ROOT); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const { resolve, join } = require("node:path");
const { homedir } = require("node:os");
console.log("path.resolve('~/.agentmemory') =", resolve("~/.agentmemory"));
console.log("expected home path =", join(homedir(), ".agentmemory"));
NODERepository: rohitg00/agentmemory
Length of output: 190
🏁 Script executed:
cat -n src/functions/obsidian-export.ts | head -50Repository: rohitg00/agentmemory
Length of output: 1895
🏁 Script executed:
grep -n "getExportRoot\|AGENTMEMORY_EXPORT_ROOT" src/functions/obsidian-export.tsRepository: rohitg00/agentmemory
Length of output: 287
🏁 Script executed:
rg "AGENTMEMORY_EXPORT_ROOT" --type md --type txtRepository: rohitg00/agentmemory
Length of output: 236
Expand ~ before resolving AGENTMEMORY_EXPORT_ROOT.
The README documents AGENTMEMORY_EXPORT_ROOT=~/.agentmemory as the recommended value, but path.resolve("~/.agentmemory") treats ~ literally, resolving to a literal ~ directory under the current working directory instead of the user's home. When AGENTMEMORY_EXPORT_ROOT is not set, DEFAULT_EXPORT_ROOT correctly uses homedir(), but explicit env var values with ~ will not expand.
Suggested fix
function getExportRoot(): string {
- return resolve(process.env["AGENTMEMORY_EXPORT_ROOT"] || DEFAULT_EXPORT_ROOT);
+ const configured =
+ process.env["AGENTMEMORY_EXPORT_ROOT"] || DEFAULT_EXPORT_ROOT;
+ const expanded =
+ configured === "~"
+ ? homedir()
+ : configured.startsWith(`~${sep}`)
+ ? join(homedir(), configured.slice(2))
+ : configured;
+ return resolve(expanded);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/functions/obsidian-export.ts` around lines 16 - 18, The getExportRoot
function should expand a leading "~" in the AGENTMEMORY_EXPORT_ROOT env var
before resolving so "~/.agentmemory" maps to the user's home; update
getExportRoot to check process.env["AGENTMEMORY_EXPORT_ROOT"], and if it starts
with "~" replace that leading "~" with os.homedir() (or homedir()) then call
resolve on the resulting path; keep fallback to DEFAULT_EXPORT_ROOT unchanged so
existing behavior using DEFAULT_EXPORT_ROOT and homedir() remains intact.
Comment on lines
+20
to
+27
| function resolveVaultDir(vaultDir?: string): string | null { | ||
| const root = getExportRoot(); | ||
| const resolved = resolve(vaultDir || join(root, "vault")); | ||
| if (resolved === root || resolved.startsWith(root + sep)) { | ||
| return resolved; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
vaultDir confinement is still bypassable via symlinks.
A path like <exportRoot>/link passes this prefix check even when link is a symlink to a directory outside the export root, so the later writeFile() calls can still escape the intended sandbox. Revalidate against real paths, or reject symlinked path segments before writing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/functions/obsidian-export.ts` around lines 20 - 27, The current
resolveVaultDir allows escaping via symlinks (a path under export root that is a
symlink to outside), so update resolveVaultDir to validate real filesystem
paths: compute realRoot = realpathSync(getExportRoot()) and realResolved =
realpathSync(resolved) and ensure realResolved === realRoot or
realResolved.startsWith(realRoot + sep); alternatively, explicitly walk each
path segment from root to resolved using lstatSync and reject if any segment is
a symlink before permitting later writeFile() operations. Use resolveVaultDir as
the single place to enforce this check so writeFile() never receives an approved
but symlinked path.
Resolved README.md conflicts by taking the new benchmarks/competitor table layout from main and re-applying the security-related text changes from this branch: - "starts with local iii-engine or Docker" in npx command - From source section: local iii-engine or Docker fallback note - Real-Time Viewer: 127.0.0.1 binding + nonce-based CSP description - AGENTMEMORY_EXPORT_ROOT env var - API section: 127.0.0.1 default + mesh AGENTMEMORY_SECRET requirement - Prerequisites: Node.js + iii-engine or Docker (no curl|sh) src/cli.ts auto-merged cleanly: install_iii() function and curl|sh removed by this branch, token savings status command preserved from main.
6 tasks
This was referenced Apr 12, 2026
This was referenced Apr 13, 2026
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
2 participants
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
This PR addresses a set of security issues in agentmemory’s default deployment, viewer, mesh sync, export path, and startup flow.
It specifically fixes:
Findings Addressed
1. Default REST/stream exposure on
0.0.0.0Previously, the default
iii-config.yamlexposed REST and stream services on all interfaces.Changes:
127.0.0.1iii-config.docker.yamlfor Docker usageSecurity impact:
2. Stored XSS via inline viewer handlers
The viewer previously rendered attacker-controlled values into HTML that used inline event handlers, while CSP allowed inline scripts.
Changes:
data-actionevent handlingscript-src-attr 'none'Security impact:
unsafe-inlinescript execution3. Arbitrary path writes in Obsidian export
vaultDirwas previously accepted directly, allowing export to arbitrary filesystem paths.Changes:
AGENTMEMORY_EXPORT_ROOT~/.agentmemoryvaultDirvalues outside the allowed rootSecurity impact:
4. Mesh sync without peer auth
Mesh push/pull requests previously ran without authorization headers, and mesh endpoints were usable without a configured shared secret.
Changes:
AGENTMEMORY_SECRETfor mesh endpointsAGENTMEMORY_SECRETfor mesh sync operationsAuthorization: Bearer <secret>on mesh push/pull requestsSecurity impact:
5. Secret redaction misses modern token formats
The privacy filter did not catch several current credential shapes.
Changes:
sk-proj-*sk/pk/rk/aktoken variantsghs_/ghu_style tokensSecurity impact:
6. CLI executes remote shell script directly
The CLI startup path previously supported
curl ... | shinstallation foriii-engine.Changes:
iii-enginebinary if availableSecurity impact:
Additional Hardening
/agentmemory/viewerfollow normal bearer-token auth rules whenAGENTMEMORY_SECRETis setValidation
Ran successfully:
npm testnpm run buildNotes
This branch was rebased/cherry-picked onto current
upstream/mainand conflicts were resolved against recent upstream README/CLI/viewer changes.Summary by CodeRabbit
Security
New Features
Documentation
Chores
Tests