fix: act on the 1.27.0 pre-release review

A Fable 5.1 reviewer read the whole release diff against 1.26.2 and returned SHIP WITH FIXES. These are its findings, verified before acting on each. **The changelog advertised a feature the code refuses (major).** The #401 changeset and docs/web-tabs.md both listed `*.localhost` in the loopback set. The follow-up in 02b0e278 moved it out of the auto-route set on security grounds and updated CLAUDE.md but neither of those, and that changeset becomes the 1.27.0 CHANGELOG entry: a user would have read the release notes, tapped `http://app.localhost:3000/` on a phone and got a connection error from a documented feature. Both corrected, and the user guide now says why it is excluded and that adding such a dashboard by hand still works. **Dictation delivered its text twice (minor, #388).** `keydownSnapshot` started `null`, so `keydownSnapshot ?? canonicalCount` at the input event read a counter xterm had ALREADY bumped: on a fresh page load with no keydown yet, xterm's own capture listener forwards the `insertText` itself (it is not gated behind a keydown), then the snapshot equals the bumped count, `count > snapshot` is false, and the controller emits the same text again. Reproduced directly against the module: it emitted `hello` for input xterm had already delivered. A `0` baseline restores that file's own invariant, that a missed recovery is acceptable and a duplicated keystroke is not. Two regression tests, covering both the xterm-already-delivered and genuinely-dropped halves. **The sorted rail's arrow-key walk followed the DOM (minor).** `_tabKeydownHandler` steps `querySelectorAll` order, which is `sessionOrder`, while a sorted rail paints its rows with the flex `order` property, so ArrowDown from the top card landed wherever that session happened to sit in the tab order. It now sorts its node list by the COMPUTED order first: computed rather than inline, because web tabs take their `order: 9999` from CSS and would otherwise read as 0 and lead the walk. This is the one place that follows the paint; the Alt+N badge, the drag model and the filter all still deliberately read the DOM. **A trusted dashboard was auto-reused by a tapped link (minor, #401).** The reuse loop skipped `managed` and direct-mode records but not `trusted`. A trusted frame is mounted with `allow-same-origin`, i.e. on Codeman's origin with the user's cookie, and these links come from agent output, which is the threat model the loopback allowlist was just narrowed for. An agent that can write into the dev server's tree could print a path that one tap opens inside that privileged frame. Excluded from auto-reuse, with a test; opening it from the Run dropdown is still an explicit action and unchanged. **Two documentation claims that were no longer true.** CLAUDE.md said test/location-overlay-commands.test.ts pins every remote pane command, but remote claude and remote omp now have their own arm in `buildRemoteLaunchCommand` and never reach `defaultRemoteCommandForMode`, which is what that test asserts, so it pins nothing for them and changing either arm will not fail it. Named the real pins instead. Also documented the arrow-key-walk exception in the rail paragraph. Left as follow-ups, deliberately: `POST /api/webviews` does not dedupe by URL server-side, so two devices tapping one link concurrently can still save two dashboards for one origin (pre-existing endpoint behaviour that #401 makes reachable by a tap), and the location-overlay golden should assert the real remote claude/omp commands rather than a branch neither reaches. Full gate green: 359 files, 6869 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-10-02 13:39:41 +02:00 · 2026-09-12 06:09:43 +02:00
parent 8b23f3e260
commit 65ddedd1d4
8 changed files with 92 additions and 9 deletions
@@ -6,7 +6,7 @@ feat(webview): open `localhost` links from the terminal and the Response Viewer

 An agent prints `http://localhost:5173/` and the user taps it on a phone: that address
 only exists on the Codeman box, so the link was a guaranteed connection error from any
-other device. A loopback link (`localhost`, `*.localhost`, 127/8, 0.0.0.0, ::1) clicked in
+other device. A loopback link (`localhost`, 127/8, 0.0.0.0, ::1) clicked in
 the terminal or in the Response Viewer now opens as a proxied web tab whenever the
 Codeman page itself is not on that box — reusing a saved proxied dashboard on the same
 origin (with the link's own path opened inside it) or saving one under its host:port.
@@ -59,12 +59,24 @@ served) and you tap it on your phone. That address only exists on the Codeman bo
 the phone's browser can never load it — but the web-tab proxy fetches from the server,
 where it works.

-So a **loopback** link (`localhost`, `*.localhost`, `127.0.0.0/8`, `0.0.0.0`, `::1`) clicked
+So a **loopback** link (`localhost`, `127.0.0.0/8`, `0.0.0.0`, `::1`) clicked
 in the terminal or in the Response Viewer opens as a **proxied web tab** whenever the
 Codeman page itself is not on that box. A saved proxied dashboard on the same origin is
-reused (one tab per dev server, with the link's own path opened inside it); otherwise
-one is saved under its `host:port` so it is in the Run dropdown next time. Sandboxed by
-default, like any other web tab.
+reused (one tab per dev server, with the link's own path opened inside it, and one tab
+per dev server rather than per host spelling, so `localhost:5173` and `127.0.0.1:5173`
+share it); otherwise one is saved under its `host:port` so it is in the Run dropdown
+next time, and a toast tells you it was saved. Sandboxed by default, like any other web
+tab.
+
+⚠️ **`*.localhost` is deliberately not auto-routed**, even though a browser treats it as
+loopback. Every other name in that list is an address literal that can only mean this
+box; a `*.localhost` DNS name is not one, and on a resolver with a search domain
+configured `evil.localhost` can be retried as `evil.localhost.<search domain>`, which
+someone else can control. Since the links come from agent output, one tap would then
+make Codeman fetch an agent-chosen origin server-side and save it. If you really run
+`api.localhost` dev hosts, add that dashboard by hand: doing so is an explicit action,
+which is the difference that matters here. A **trusted** (non-sandboxed) dashboard is
+likewise never auto-reused by a tapped link, for the same reason.

 Only loopback is routed this way. A LAN or tailnet address (`192.168.…`, `100.…`,
 `box.ts.net`) may well be reachable from the device — a VPN, the same Wi-Fi — and a
@@ -5183,6 +5183,18 @@ class CodemanApp {

      // Rows hidden by the sidebar filter must not be steppable.
      const tabs = [...container.querySelectorAll('.session-tab:not(.tab-filtered-out)')];
+      // ⚠️ A sorted rail paints its rows with the flex `order` property while the
+      // DOM stays in `sessionOrder` (that is what keeps the Alt+N badge and the
+      // drag model honest), so a DOM-order walk steps around the screen instead of
+      // down it: ArrowDown from the top card lands wherever that session happens to
+      // sit in the tab order. Walk what the eye sees. Read the COMPUTED order, not
+      // the inline one, or web tabs (pinned past the cards by a CSS `order: 9999`
+      // rather than an inline style) read as 0 and the walk starts on them. Array
+      // sort is stable, so equal orders keep DOM order, which is the unsorted case.
+      if (this.isTabRailSorted()) {
+        const orderOf = (el) => Number(getComputedStyle(el).order) || 0;
+        tabs.sort((a, b) => orderOf(a) - orderOf(b));
+      }
      const currentIndex = tabs.indexOf(document.activeElement);

      // Enter or Space activates the tab
@@ -54,7 +54,15 @@
    // Number of canonical data events xterm has emitted, bumped by the caller's
    // onData hook. Only its ORDER relative to a keydown matters.
    let canonicalCount = 0;
-    let keydownSnapshot = null;
+    // ⚠️ 0, never null. With `null` the `?? canonicalCount` fallback at the input
+    // event reads a count xterm has ALREADY bumped: on a fresh page load with no
+    // keydown yet (dictation, Android voice typing, any `insertText` with no key
+    // held) xterm's own capture listener runs first, forwards the text itself and
+    // bumps the counter, then this snapshot equals it, `count > snapshot` is false,
+    // and the text is emitted a SECOND time. A baseline of 0 makes that comparison
+    // true and stands the recovery down, which restores this file's invariant: a
+    // missed recovery is acceptable, a duplicated keystroke is not.
+    let keydownSnapshot = 0;
    let composing = false;
    const pending = [];

@@ -141,7 +141,13 @@ Object.assign(CodemanApp.prototype, {
    const wantedKey = webTabOriginKey(url);
    let existing = null;
    for (const webview of this.webviews.values()) {
-      if (webview.managed || (webview.embedMode ?? 'proxy') !== 'proxy') continue;
+      // ⚠️ `trusted` is excluded alongside `managed` and direct-mode records: a
+      // trusted frame runs with `allow-same-origin`, i.e. on Codeman's origin with
+      // the user's cookie, and the link being followed came from agent output. An
+      // agent that can write into the dev server's tree (it IS the workspace) could
+      // otherwise print a path that one tap navigates that privileged frame to.
+      // Opening such a dashboard from the Run dropdown is still an explicit action.
+      if (webview.managed || webview.trusted || (webview.embedMode ?? 'proxy') !== 'proxy') continue;
      try {
        if (webTabOriginKey(new URL(webview.url)) === wantedKey) {
          existing = webview;
@@ -298,6 +298,28 @@ describe('orphaned terminal input recovery', () => {
  });
 });

+describe('the first input event of a page load, with no keydown before it', () => {
+  it('stands down when xterm already delivered it, instead of duplicating the text', () => {
+    // Dictation (Android voice typing, desktop dictation, any `insertText` with
+    // no key held) reaches xterm with `_keyDownSeen` false, so xterm's OWN capture
+    // listener forwards it and bumps the canonical counter before this controller's
+    // listener runs. The snapshot baseline has to predate that bump, or the
+    // candidate reads "xterm stayed silent" and emits the text a second time.
+    const h = harness();
+    h.controller.notifyCanonicalData(); // xterm delivered it first
+    h.input('hello');
+    h.flushTimers();
+    expect(h.emitted, 'xterm already delivered this text').toEqual([]);
+  });
+
+  it('still recovers one that xterm genuinely dropped', () => {
+    const h = harness();
+    h.input('hello'); // nothing from xterm for it
+    h.flushTimers();
+    expect(h.emitted).toEqual(['hello']);
+  });
+});
+
 describe('terminal-ui wiring: what counts as "xterm spoke for this keystroke"', () => {
  const terminalSource = readFileSync(new URL('../src/web/public/terminal-ui.js', import.meta.url), 'utf8');

@@ -250,6 +250,29 @@ describe('openLinkThroughWebTabIfLoopback', () => {
    });
  });

+  it("does not reuse a TRUSTED dashboard, whose frame runs on Codeman's own origin", async () => {
+    // A trusted webview is mounted with `allow-same-origin`. The link being
+    // followed came from agent output, so auto-reusing that frame would let an
+    // agent-chosen path be opened inside a privileged origin on one tap. A fresh
+    // sandboxed record is saved instead.
+    const { win, app, calls } = boot();
+    app.webviews.set('trusted-dev', {
+      id: 'trusted-dev',
+      name: 'trusted',
+      url: 'http://localhost:5173/',
+      embedMode: 'proxy',
+      trusted: true,
+    } as never);
+    app.webviews.delete('dev');
+
+    await app.openUrlInWebTab('http://localhost:5173/admin');
+
+    const post = calls.find((c) => c.method === 'POST' && c.path === '/api/webviews');
+    expect(post, 'a trusted dashboard must not be reused for a tapped link').toBeTruthy();
+    expect(post?.body).toMatchObject({ url: 'http://localhost:5173/', trusted: false });
+    expect(frameSrc(win, 'trusted-dev')).toBeFalsy();
+  });
+
  it('does not reuse a direct-mode dashboard, which cannot show a loopback page from elsewhere', async () => {
    const { win, app, calls } = boot();
    expect(app.openLinkThroughWebTabIfLoopback('https://localhost:9443/admin')).toBe(true);