fix(viewer): allow non-loopback bind via AGENTMEMORY_VIEWER_HOST#442
fix(viewer): allow non-loopback bind via AGENTMEMORY_VIEWER_HOST#442wyh0626 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR makes the viewer bind host configurable (via AGENTMEMORY_VIEWER_HOST), enforces inbound Bearer auth and Host-header allowlisting for non-loopback binds, updates the frontend to prompt/save tokens on 401, and adds Fly entrypoint/docs to enable ChangesViewer host binding and authentication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Hey @wyh0626 — appreciate the contribution, but blocking on a security regression before this can land. The viewer has no inbound auth — its only defense beyond loopback bind is the Host header allowlist in Specifically:
This PR also currently conflicts with main on |
wyh0626
force-pushed
the
fix/434-fly-viewer-bind
branch
from
4b35165 to
8ad0794
Compare
|
@wyh0626 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/viewer/server.ts (1)
17-30: 💤 Low valueConsider caching the favicon buffer at module load time.
loadViewerFavicon()performs synchronous disk I/O on every/favicon.svgrequest. Since the favicon is static, loading it once at module initialization would avoid repeated disk reads.♻️ Optional: cache favicon at module level
+// Cache favicon at module load — it's static and avoids repeated disk I/O. +const VIEWER_FAVICON: Buffer | null = loadViewerFavicon(); + function loadViewerFavicon(): Buffer | null { const base = dirname(fileURLToPath(import.meta.url)); // ... rest unchanged }Then in the request handler:
if (method === "GET" && pathname === "/favicon.svg") { - const favicon = loadViewerFavicon(); + const favicon = VIEWER_FAVICON; if (favicon) {🤖 Prompt for 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. In `@src/viewer/server.ts` around lines 17 - 30, loadViewerFavicon currently does synchronous disk I/O on every request; change to load the favicon once at module initialization and reuse it: call loadViewerFavicon() at module top-level and store the result in a const (e.g., VIEWER_FAVICON or viewerFaviconBuffer), then update the request handler that serves /favicon.svg to return that cached buffer (handle null by returning 404 or falling back). Keep the existing loadViewerFavicon function but ensure the module-level constant is used instead of calling the function per-request.
🤖 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.
Nitpick comments:
In `@src/viewer/server.ts`:
- Around line 17-30: loadViewerFavicon currently does synchronous disk I/O on
every request; change to load the favicon once at module initialization and
reuse it: call loadViewerFavicon() at module top-level and store the result in a
const (e.g., VIEWER_FAVICON or viewerFaviconBuffer), then update the request
handler that serves /favicon.svg to return that cached buffer (handle null by
returning 404 or falling back). Keep the existing loadViewerFavicon function but
ensure the module-level constant is used instead of calling the function
per-request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36a33852-fedc-4568-9534-f15321657d21
📒 Files selected for processing (6)
deploy/fly/README.mddeploy/fly/entrypoint.shsrc/viewer/index.htmlsrc/viewer/server.tstest/viewer-host.test.tstest/viewer-session-id.test.ts
✅ Files skipped from review due to trivial changes (1)
- deploy/fly/README.md
`startViewerServer` hardcoded `server.listen(port, "127.0.0.1")`, so Fly's `fly-local-6pn` traffic was RST and `fly proxy 3113:3113` reported "Recv failure: Connection reset by peer" (rohitg00#434). Bind host is now read from `AGENTMEMORY_VIEWER_HOST` (default `127.0.0.1`). To keep the viewer's bearer-authorized proxy from silently exposing itself on non-loopback binds, the same code path now: - drops loopback hostnames from the default Host allowlist when bind is non-loopback; - refuses to start unless both `VIEWER_ALLOWED_HOSTS` and `AGENTMEMORY_SECRET` are set in that mode; - requires `Authorization: Bearer $AGENTMEMORY_SECRET` (timing-safe compare from src/auth.ts) on every /agentmemory/* request; HTML and the favicon stay open. `deploy/fly/Dockerfile` no longer bakes `AGENTMEMORY_VIEWER_HOST=::`. The entrypoint exports it (and a `VIEWER_ALLOWED_HOSTS` default scoped to the Host headers `fly proxy` emits) only when `FLY_APP_NAME` or `FLY_ALLOC_ID` is present, so a `docker run -p 3113:3113` of the image elsewhere stays loopback-only. Tests: 29 cases in test/viewer-host.test.ts.
wyh0626
force-pushed
the
fix/434-fly-viewer-bind
branch
from
8ad0794 to
f80e9a7
Compare
|
Updated this PR against the latest Changes made:
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/viewer/server.ts (1)
323-340:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog the actual bound port after
listen(0).If
requestedPortis0,currentPortnever changes, so the startup messages printhttp://localhost:0even thoughserver.address()already exposes the real port. That makes the log misleading for ephemeral-port callers and for these tests, which all start the server with0.Proposed fix
server.on("listening", () => { const addr = server.address(); - boundViewerPort = + const actualPort = addr && typeof addr === "object" && "port" in addr ? addr.port : currentPort; + boundViewerPort = actualPort; viewerSkipped = false; if (inboundSecret !== null) { const allowedHosts = readAllowedHostsOverride().join(", "); console.log( - `[agentmemory] Viewer: http://localhost:${currentPort} (bound to ${host}; inbound Bearer required; allowed Host headers: ${allowedHosts})`, + `[agentmemory] Viewer: http://localhost:${actualPort} (bound to ${host}; inbound Bearer required; allowed Host headers: ${allowedHosts})`, ); return; } if (currentPort === requestedPort) { - console.log(`[agentmemory] Viewer: http://localhost:${currentPort}`); + console.log(`[agentmemory] Viewer: http://localhost:${actualPort}`); } else { console.log( - `[agentmemory] Viewer started on http://localhost:${currentPort} (fallback from ${requestedPort})`, + `[agentmemory] Viewer started on http://localhost:${actualPort} (fallback from ${requestedPort})`, ); } });🤖 Prompt for 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. In `@src/viewer/server.ts` around lines 323 - 340, The startup logs use currentPort which stays 0 when listen(0) is used; after you compute boundViewerPort from server.address() (the existing addr check around server.address()), propagate that real port into the logging path by either assigning currentPort = boundViewerPort when boundViewerPort is defined or by replacing currentPort with boundViewerPort in the console.log calls (including the inboundSecret branch and both requestedPort comparisons) so the actual ephemeral port returned by server.address() is printed.
🤖 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 `@deploy/fly/README.md`:
- Line 90: Typo in the README description: replace the word "preseeds" with
"pre-seeds" (or "prepopulates") in the sentence that references
VIEWER_ALLOWED_HOSTS
(`VIEWER_ALLOWED_HOSTS=localhost:3113,127.0.0.1:3113,[::1]:3113`) so the
allowlist description reads clearly (e.g., "pre-seeds VIEWER_ALLOWED_HOSTS..."
or "prepopulates VIEWER_ALLOWED_HOSTS...").
In `@src/viewer/index.html`:
- Around line 1217-1224: The unlock password input (id="viewer-auth-token")
lacks an accessible name; add one by either adding a visible <label> that
references the input id or by adding an aria-label/aria-labelledby that conveys
"Viewer authorization token" (or similar) so screen readers can announce the
field; update the host.innerHTML where the input is created and ensure any
visually-hidden label uses an appropriate class if you want it hidden, and keep
the button with data-action="save-viewer-token" intact to preserve behavior.
- Around line 3792-3803: After successfully saving the viewer token in the
'save-viewer-token' handler (around setViewerToken, hideViewerAuthPrompt,
loadTab(state.activeTab)), also invoke fetchFlags() to re-fetch config/flags and
then refresh the footer/version and flag banner UI (e.g., call existing
functions like refreshFooterVersion() and refreshFlagBanners(), or implement
small functions to re-render those parts if they don't exist) so the page state
updates immediately without a full reload.
---
Outside diff comments:
In `@src/viewer/server.ts`:
- Around line 323-340: The startup logs use currentPort which stays 0 when
listen(0) is used; after you compute boundViewerPort from server.address() (the
existing addr check around server.address()), propagate that real port into the
logging path by either assigning currentPort = boundViewerPort when
boundViewerPort is defined or by replacing currentPort with boundViewerPort in
the console.log calls (including the inboundSecret branch and both requestedPort
comparisons) so the actual ephemeral port returned by server.address() is
printed.
🪄 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: a55f2fe6-d173-4a91-80c2-814f12c2160f
📒 Files selected for processing (6)
deploy/fly/README.mddeploy/fly/entrypoint.shsrc/viewer/index.htmlsrc/viewer/server.tstest/viewer-host.test.tstest/viewer-session-id.test.ts
| Fly's runtime variables** (`FLY_APP_NAME` / `FLY_ALLOC_ID`). That makes | ||
| the viewer listen on the machine's `fly-local-6pn` WireGuard interface | ||
| as well as loopback so `fly proxy` can reach it. The same branch | ||
| preseeds `VIEWER_ALLOWED_HOSTS=localhost:3113,127.0.0.1:3113,[::1]:3113`, |
There was a problem hiding this comment.
Fix wording typo in allowlist description.
Line 90 uses “preseeds”; replace with “pre-seeds” (or “prepopulates”) for clearer docs text.
✏️ Proposed doc tweak
-The same branch preseeds `VIEWER_ALLOWED_HOSTS=localhost:3113,127.0.0.1:3113,[::1]:3113`,
+The same branch pre-seeds `VIEWER_ALLOWED_HOSTS=localhost:3113,127.0.0.1:3113,[::1]:3113`,🤖 Prompt for 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.
In `@deploy/fly/README.md` at line 90, Typo in the README description: replace the
word "preseeds" with "pre-seeds" (or "prepopulates") in the sentence that
references VIEWER_ALLOWED_HOSTS
(`VIEWER_ALLOWED_HOSTS=localhost:3113,127.0.0.1:3113,[::1]:3113`) so the
allowlist description reads clearly (e.g., "pre-seeds VIEWER_ALLOWED_HOSTS..."
or "prepopulates VIEWER_ALLOWED_HOSTS...").
| host.innerHTML = | ||
| '<div class="viewer-auth-panel">' + | ||
| '<div>' + | ||
| '<div class="viewer-auth-title">Viewer authorization required</div>' + | ||
| '<div class="viewer-auth-desc">Enter <code>AGENTMEMORY_SECRET</code> to unlock viewer API access.</div>' + | ||
| '</div>' + | ||
| '<input id="viewer-auth-token" type="password" autocomplete="off" spellcheck="false" placeholder="AGENTMEMORY_SECRET" />' + | ||
| '<button class="btn" data-action="save-viewer-token">Unlock</button>' + |
There was a problem hiding this comment.
Add an accessible name to the unlock field.
This input is the only path through the non-loopback auth gate, but it currently relies on placeholder text alone. Screen readers won't get a reliable label here, which makes the unlock flow much harder to complete.
♿ Proposed fix
- '<div class="viewer-auth-desc">Enter <code>AGENTMEMORY_SECRET</code> to unlock viewer API access.</div>' +
+ '<div id="viewer-auth-desc" class="viewer-auth-desc">Enter <code>AGENTMEMORY_SECRET</code> to unlock viewer API access.</div>' +
'</div>' +
- '<input id="viewer-auth-token" type="password" autocomplete="off" spellcheck="false" placeholder="AGENTMEMORY_SECRET" />' +
+ '<input id="viewer-auth-token" type="password" autocomplete="off" spellcheck="false" aria-label="AGENTMEMORY_SECRET" aria-describedby="viewer-auth-desc" placeholder="AGENTMEMORY_SECRET" />' +
'<button class="btn" data-action="save-viewer-token">Unlock</button>' +📝 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.
| host.innerHTML = | |
| '<div class="viewer-auth-panel">' + | |
| '<div>' + | |
| '<div class="viewer-auth-title">Viewer authorization required</div>' + | |
| '<div class="viewer-auth-desc">Enter <code>AGENTMEMORY_SECRET</code> to unlock viewer API access.</div>' + | |
| '</div>' + | |
| '<input id="viewer-auth-token" type="password" autocomplete="off" spellcheck="false" placeholder="AGENTMEMORY_SECRET" />' + | |
| '<button class="btn" data-action="save-viewer-token">Unlock</button>' + | |
| host.innerHTML = | |
| '<div class="viewer-auth-panel">' + | |
| '<div>' + | |
| '<div class="viewer-auth-title">Viewer authorization required</div>' + | |
| '<div id="viewer-auth-desc" class="viewer-auth-desc">Enter <code>AGENTMEMORY_SECRET</code> to unlock viewer API access.</div>' + | |
| '</div>' + | |
| '<input id="viewer-auth-token" type="password" autocomplete="off" spellcheck="false" aria-label="AGENTMEMORY_SECRET" aria-describedby="viewer-auth-desc" placeholder="AGENTMEMORY_SECRET" />' + | |
| '<button class="btn" data-action="save-viewer-token">Unlock</button>' + |
🤖 Prompt for 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.
In `@src/viewer/index.html` around lines 1217 - 1224, The unlock password input
(id="viewer-auth-token") lacks an accessible name; add one by either adding a
visible <label> that references the input id or by adding an
aria-label/aria-labelledby that conveys "Viewer authorization token" (or
similar) so screen readers can announce the field; update the host.innerHTML
where the input is created and ensure any visually-hidden label uses an
appropriate class if you want it hidden, and keep the button with
data-action="save-viewer-token" intact to preserve behavior.
| if (action === 'save-viewer-token') { | ||
| var tokenInput = document.getElementById('viewer-auth-token'); | ||
| var token = tokenInput ? tokenInput.value.trim() : ''; | ||
| if (token) { | ||
| setViewerToken(token); | ||
| hideViewerAuthPrompt(); | ||
| if (state[state.activeTab] && typeof state[state.activeTab] === 'object') { | ||
| state[state.activeTab].loaded = false; | ||
| } | ||
| loadTab(state.activeTab); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Retry the viewer config fetch after unlocking.
fetchFlags() only runs during bootstrap. If config/flags is the request that first 401s, this handler unlocks the active tab but leaves the footer version and flag banners stale until the user fully reloads the page.
🔁 Proposed fix
if (token) {
setViewerToken(token);
hideViewerAuthPrompt();
+ fetchFlags();
if (state[state.activeTab] && typeof state[state.activeTab] === 'object') {
state[state.activeTab].loaded = false;
}
loadTab(state.activeTab);
}📝 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.
| if (action === 'save-viewer-token') { | |
| var tokenInput = document.getElementById('viewer-auth-token'); | |
| var token = tokenInput ? tokenInput.value.trim() : ''; | |
| if (token) { | |
| setViewerToken(token); | |
| hideViewerAuthPrompt(); | |
| if (state[state.activeTab] && typeof state[state.activeTab] === 'object') { | |
| state[state.activeTab].loaded = false; | |
| } | |
| loadTab(state.activeTab); | |
| } | |
| return; | |
| if (action === 'save-viewer-token') { | |
| var tokenInput = document.getElementById('viewer-auth-token'); | |
| var token = tokenInput ? tokenInput.value.trim() : ''; | |
| if (token) { | |
| setViewerToken(token); | |
| hideViewerAuthPrompt(); | |
| fetchFlags(); | |
| if (state[state.activeTab] && typeof state[state.activeTab] === 'object') { | |
| state[state.activeTab].loaded = false; | |
| } | |
| loadTab(state.activeTab); | |
| } | |
| return; |
🤖 Prompt for 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.
In `@src/viewer/index.html` around lines 3792 - 3803, After successfully saving
the viewer token in the 'save-viewer-token' handler (around setViewerToken,
hideViewerAuthPrompt, loadTab(state.activeTab)), also invoke fetchFlags() to
re-fetch config/flags and then refresh the footer/version and flag banner UI
(e.g., call existing functions like refreshFooterVersion() and
refreshFlagBanners(), or implement small functions to re-render those parts if
they don't exist) so the page state updates immediately without a full reload.
2 participants
Summary
Closes #434.
startViewerServerhardcodesserver.listen(port, "127.0.0.1"). On Fly, traffic fromfly proxyenters via thefly-local-6pnWireGuard interface, not loopback, so the kernel RSTs the connection. That's thecurl: (56) Recv failure: Connection reset by peerthe issue reports.Fix
A two-line helper:
startViewerServercalls it once and uses the result. The default stays127.0.0.1, so every local install is unaffected and the v0.9.12 plaintext-bearer guard (which assumes bearer tokens only ride loopback) still holds.The Fly image opts in by setting
AGENTMEMORY_VIEWER_HOST=::indeploy/fly/Dockerfile. That makes the viewer listen onfly-local-6pn(IPv6) as well as loopback.fly.tomlis untouched: port 3113 is still not declared as a Fly service, so the viewer remains reachable only throughfly proxy(mTLS over WireGuard).Verify
Unit:
npx vitest run test/viewer-host.test.ts # 6/6 passEnd-to-end on Fly. I built the patched image locally with
npm pack+ a tarball install in the Dockerfile, deployed to a Singapore machine, then ran the exact reproducer from the issue:HEAD on
/returns 404 because the viewer only registers a GET handler there; GET serves the dashboard HTML. The signal is that the TCP+HTTP handshake completes at all, no RST. App and volume torn down after.Tests
6 cases in
test/viewer-host.test.ts:resolveViewerHostdefaults to127.0.0.1when the env var is unsetstartViewerServeractually binds to127.0.0.1by defaultstartViewerServerbinds to::1whenAGENTMEMORY_VIEWER_HOST=::1Summary by CodeRabbit
New Features
Documentation
Tests