feat(server): list open sessions on the landing page - #310
Conversation
visibleSessions() in cli.js filtered out ended sessions with a private one-liner. The upcoming server-side landing list needs the exact same filter, and a second copy of `status !== "ended"` is exactly the kind of thing that quietly drifts between the two surfaces over time. Export it once from session-store.js so the CLI's home output and the server's landing page can never silently diverge on what "open" means.
GET / always rendered the same static "Lavish Editor is running" card,
regardless of how many sessions were actually open - useful only if you
already have the CLI to hand. On a phone via the tailnet root there is
no CLI at all, and even on desktop it's convenient to just open the
root URL and see what's live.
The route now reads the same session inventory the CLI already prints
(filterVisibleSessions() over store.listSessions()) and renders a
minimal read-only list when any are open: a status dot, the file path
with the home directory elided the same way the chrome overflow menu
already does (displayPathParts), and a link. The empty state is
byte-identical to today's placeholder.
Decisions worth recording:
- Read failures fail open to the placeholder rather than 500ing - this
route is unauthenticated and always reachable, so it must never hang
or error on a bad state.json read.
- Each link is built fresh as a relative /session/{key} at render time,
never the session's stored `url` field. That field is captured once
at POST /api/sessions time against whatever host resolved then, and
would go stale relative to whichever host actually served this page
once the tailnet root (kunchenguid#216) exists alongside plain loopback.
- No opt-out env var. This is a deliberate new-default change, accepted
as such, so no extra config surface was added for it.
- The populated list carries a 15s <meta http-equiv=refresh> so it
stays current without a new SSE surface; the empty placeholder never
carries it, since there is nothing there to go stale and reloading it
would disrupt someone leaving the tab open for later.
- session.file is a filesystem path an attacker could shape (this route
has no auth beyond the Host allowlist), so every user-controlled
string is escaped with the existing escapeHtml().
Only one existing test touched the landing route at all (the Tailscale dual-listener test), and it only ever saw the empty placeholder. Extend it to assert the session it already opens shows up in the list with a working relative link resolved against the MagicDNS host - proving tailnet-root compatibility rather than merely asserting it. New coverage: - empty state stays byte-identical to today's placeholder, no refresh tag - a single open session renders file name, status dot, working link, and the refresh tag - several sessions list correctly and an ended one is excluded - a file path containing markup-significant characters is escaped, not rendered raw - this route has no auth in front of it - filterVisibleSessions as a pure unit test: keeps open/feedback, drops ended, preserves order
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (3): Last reviewed commit: "test(server): keep XSS payload slashes o..." | Re-trigger Greptile |
|
Adding a couple of screenshots here to make this easier to review — here's what it looks like running locally: Empty state (no sessions open): Populated state (2 sessions open): Ran the full test suite locally as well and everything's green. Happy to adjust anything if you'd like it done differently! |
|
Speaking as Kun's firstmate: Diff-reviewed head This is a captain-decision hold, not waiting on the author. Do not merge from this pass. Why hold
VISION.md (per rule)
No auto-merge. No rebase. #216 left alone (do-not-re-flag). Flagging Firstmate for the security + new-default decision. |
|
Speaking as Kun's firstmate: Follow-up on CI after the fork approvals: Guard and Require no-mistakes are green. Ubuntu and macOS build-and-test are green. windows-latest is red on the new landing XSS test ( Until that is green, part of this PR is waiting on the author. Separately, the captain-decision hold in the previous comment still stands for new-default + the session-enumeration / handoff discoverability call — that is not waiting on you, and it is not cleared by greening CI. |
|
Speaking as Kun's firstmate. Captain accepted the new-default landing (security discoverability within the trust-the-network model). Do not treat that as merge approval yet. Still blocking merge:
CI otherwise: ubuntu/macos + Guard + no-mistakes are green on |
Windows forbids <>:"|?* in path names, so mkdir of a <script> directory fails on windows-latest. Open a safe artifact, then rewrite session.file in state.json to keep the escapeHtml regression without an illegal path. Co-authored-by: Kun Chen <kunchenguid@users.noreply.github.com>
path.join treats `/` in `</script>` as a separator on Windows, so the stored session.file became `...<\script>...` and the title-attribute escape assertion failed. Concatenate with path.sep so the literal payload is preserved on every OS. Co-authored-by: Kun Chen <kunchenguid@users.noreply.github.com>
|
Speaking as Kun's firstmate: Recheck of #310 head CI (fully green)
contract-class: new-default (captain already accepted; no opt-out env). Not re-flagging captain. Hold reason (unchanged policy) Since last stamp
Outcome: held-awaiting-firstmate-yes. Firstmate: CI is clear — please relay explicit yes (or no) after screenshot review before any merge. |
|
Speaking as Kun's firstmate. Captain skipped the merge card. Skip is not yes. This remains a captain-decision hold — not waiting on the author. CI is fully green on |
|
Independently re-verified at the actual current head (
|
|
Speaking as Kun's firstmate: Recheck after author comment (independent screenshot re-verify). Tip is still Standing captain-decision hold (FM-LA-310). Captain skipped the merge card earlier; skip is not yes. This is not waiting on the author. Do not merge, rebase, or re-flag from this pass. Will only merge on an explicit later yes from Firstmate after the captain. VISION (per-rule): Artifact stays the author's — aligns (landing chrome, not artifact mutation). Interaction beats prose — aligns (session list is the pointing surface for open reviews). Design chosen — aligns (same visual shell). Nothing interrupts — aligns (read-only list + meta refresh when populated). Every token on purpose — aligns. Instructions are the product — aligns. Scope — aligns (one person / local+tailnet server root; not multi-user collab). contract-class: new-default (unconfigured root landing gains a live session list; captain already accepted the product call earlier — merge still needs explicit yes). |
3 participants
Closes #308
Security note - reviewed and explicitly accepted, not auto-fixed
The no-mistakes review step flagged that a landing-page session link does more than reveal a file name: visiting
/session/:key(now one click away from an unauthenticated root page, where before it required already knowing that exact URL) immediately callsissueReviewerHandoffand silently supersedes whatever browser tab currently holds that review, with no confirmation step. This is not new behavior introduced by this PR -/session/:keyalready worked this way for anyone who already had the URL - but the landing page does meaningfully lower the bar by making every open session's URL discoverable with zero prior information, within the existing "unauthenticated server, trust the network" threat model this repo's README already documents (see the Network binding bullet).This was raised to the project owner as an explicit
ask-usergate (not resolved by the agent) and the decision was: ship as-is, as a discoverability change within an already-accepted threat model rather than a new class of vulnerability, with this note added specifically so the maintainer sees it during human review too. The full original finding is preserved verbatim in the collapsed Review section below.What Changed
/landing route now reads visible sessions from the store and renders each as a row (status dot, truncated file path, "Open" link) instead of the static placeholder card; the empty-list case is unchanged.filterVisibleSessionsinsrc/session-store.js(dropsstatus === "ended"sessions) and reused it from bothsrc/cli.js'svisibleSessions()and the new server route, so the CLI's home output and the landing page can't diverge on what counts as "open".<meta http-equiv="refresh">tag, but only when the session list is non-empty, so an idle empty page never reloads out from under a viewer.state.jsonread error falls back to an empty session list instead of a 500, since/is unauthenticated and must stay available.Risk Assessment
✅ Low: This fix round only added one accurate documentation line to README.md with no functional or source code changes; the prior security finding was already reviewed and explicitly accepted by the user in round 1.
Testing
Installed missing dependencies (chokidar etc. via
pnpm install, which the worktree lacked) then ran the three targeted test groups covering this change end-to-end: the landing page correctly renders the empty-state placeholder with no refresh tag when no sessions are open, lists open sessions with escaped file paths/status dots/working session links and a 15s refresh tag when populated, excludes ended sessions while keeping order, HTML-escapes a maliciously crafted file path, and the session link resolves correctly cross-host under simulated Tailscale binding (validating the relative-link design decision). The extractedfilterVisibleSessionshelper is also unit-tested directly. All 7 relevant tests passed; no code or test changes were needed, and the working tree is clean (node_modules/dist installed for testing are gitignored, not tracked).Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → 1 auto-fixed, 1 escalated and accepted as-is
src/server.js:530- The new/route is unauthenticated (protected only by the Host allowlist, not by session-key possession) and now lists every open session's real filesystem path plus a one-click/session/:keylink. Visiting that link immediately callsissueReviewerHandoff, which mints a fresh reviewer token and supersedes whatever browser tab currently holds the review - with no confirmation step. Previously an attacker/curious user needed to already know (or receive) the exact artifact path to compute or be given the session key; now anyone who can reach the bound interface (loopback-sharing users, or any device on a Tailscale-bound tailnet per the documented non-loopback warning) can enumerate every open review and hijack any of them in one click, with no opt-out env var. The commit message documents this as a deliberate accepted default, but that acceptance appears to have been made unilaterally rather than confirmed with the project owner, and this materially lowers the bar for reviewer-session takeover within the existing 'unauthenticated server, trust the network' threat model.README.md- The landing page session list is a new user-facing feature (file paths, status dots, refresh behavior, session links) but README.md's user-facing contract was not updated to describe it, contrary to AGENTS.md's documentation-ownership rule that README owns features and their behavior.🔧 Fix: docs(readme): document new landing page session list (applied to the first finding only; the second was escalated to the project owner and approved as-is - see the Security note above)
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
node --test --test-name-pattern "landing" test/server.test.js(5 tests: empty placeholder, single session listing, multiple sessions excluding ended, HTML-escaping of malicious file paths, health/landing responsiveness)node --test --test-name-pattern "Tailscale mode binds" test/server.test.js(verifies the session link on the landing page is relative/host-independent and resolves correctly when fetched via the MagicDNS host, per issue #216)node --test --test-name-pattern "filterVisibleSessions" test/session-store.test.js(unit test for the extracted shared filter used by both the CLI home output and the new landing page)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.