From e6dac66a20ac9597e7f639d0c4301b6e3ee30f05 Mon Sep 17 00:00:00 2001 From: d fei Date: Tue, 1 Sep 2026 08:07:49 -0700 Subject: [PATCH] feat(remote): add the password field to the remote-host form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend could already authenticate with a password, but only the API could supply the field — the UI had no way in. It goes in "Advanced SSH" next to Identity File: the two are answers to the same question, and seeing them together is what makes the choice obvious. The hint says outright to prefer a key when one exists. Three details, each of which breaks something if skipped: - the field is type="password" and is never populated from the server. GET is redacted, so any code writing a value into this box can only be writing a placeholder — and the next save would store that placeholder as the real password. - the value is deliberately not trimmed: leading or trailing spaces may be part of the password. - it is added to the remoteFields clearing list. Without that, a typed password persists across forms and the next new host silently inherits it — credentials from two different machines bleeding together. Both submit paths are wired: the standalone "add remote host", and the one that creates a host as part of the remote-case flow. Wiring only one leaves the other silently key-only. Guard tests pin each of the above (including "must be both paths" and "must never repopulate"). --- src/web/public/index.html | 5 ++++ src/web/public/session-ui.js | 9 +++++++ test/remote-ssh-password.test.ts | 43 ++++++++++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/web/public/index.html b/src/web/public/index.html index 7f54a277..f13f538c 100644 --- a/src/web/public/index.html +++ b/src/web/public/index.html @@ -2818,6 +2818,11 @@ Optional. Path to a private key on this machine (passed to ssh -i). Never the key contents. +
+ + + Optional, for hosts that accept no key. Stored on this machine only (remote-hosts.json, mode 0600), never sent back to the browser, and handed to ssh through sshpass's SSHPASS variable rather than the command line. Needs sshpass installed here. Prefer a key when the host allows one. +
diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 3011c015..973cb9c0 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -2466,6 +2466,9 @@ Object.assign(CodemanApp.prototype, { 'remoteHostPort', 'remoteHostCodexCommand', 'remoteHostIdentityFile', + // ⚠️ Must be cleared with the rest: a password left in the field would be + // silently inherited by the NEXT host created from this form. + 'remoteHostPassword', 'remoteHostSocksProxy', 'remoteHostJumpHost', 'remoteHostExtraSshOptions', @@ -3049,6 +3052,8 @@ Object.assign(CodemanApp.prototype, { // COD-107 — port + advanced SSH connection options. const portRaw = document.getElementById('remoteHostPort').value.trim(); const identityFile = document.getElementById('remoteHostIdentityFile').value.trim(); + // Deliberately NOT trimmed: leading/trailing spaces can be part of a password. + const password = document.getElementById('remoteHostPassword').value; const socksProxy = document.getElementById('remoteHostSocksProxy').value.trim(); const jumpHost = document.getElementById('remoteHostJumpHost').value.trim(); const extraSshOptions = document.getElementById('remoteHostExtraSshOptions').value @@ -3085,6 +3090,7 @@ Object.assign(CodemanApp.prototype, { username, ...(port ? { port } : {}), ...(identityFile ? { identityFile } : {}), + ...(password ? { password } : {}), ...(socksProxy ? { socksProxy } : {}), ...(jumpHost ? { jumpHost } : {}), ...(extraSshOptions.length ? { extraSshOptions } : {}), @@ -3417,6 +3423,8 @@ Object.assign(CodemanApp.prototype, { const username = document.getElementById('remoteHostUsername').value.trim(); const portRaw = document.getElementById('remoteHostPort').value.trim(); const identityFile = document.getElementById('remoteHostIdentityFile').value.trim(); + // Deliberately NOT trimmed: leading/trailing spaces can be part of a password. + const password = document.getElementById('remoteHostPassword').value; const socksProxy = document.getElementById('remoteHostSocksProxy').value.trim(); const jumpHost = document.getElementById('remoteHostJumpHost').value.trim(); const codexCommand = document.getElementById('remoteHostCodexCommand').value.trim(); @@ -3436,6 +3444,7 @@ Object.assign(CodemanApp.prototype, { username, ...(port ? { port } : {}), ...(identityFile ? { identityFile } : {}), + ...(password ? { password } : {}), ...(socksProxy ? { socksProxy } : {}), ...(jumpHost ? { jumpHost } : {}), ...(extraSshOptions.length ? { extraSshOptions } : {}), diff --git a/test/remote-ssh-password.test.ts b/test/remote-ssh-password.test.ts index 5670f839..0143987e 100644 --- a/test/remote-ssh-password.test.ts +++ b/test/remote-ssh-password.test.ts @@ -5,10 +5,10 @@ * environment variable, never argv), and the variable itself is injected into * the pane with socket-scoped `tmux setenv` like every other secret here. */ -import { promises as fs } from 'node:fs'; +import { promises as fs, readFileSync } from 'node:fs'; import { mkdtempSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join, resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; import { buildSshConnectionArgs, @@ -108,3 +108,42 @@ describe('remote-hosts.json holds a secret, so it is 0600', () => { expect((await fs.stat(path)).mode & 0o777).toBe(0o600); }); }); + +describe('the remote-host form wires the password without leaking it', () => { + const html = readFileSync(resolve(import.meta.dirname, '../src/web/public/index.html'), 'utf8'); + const ui = readFileSync(resolve(import.meta.dirname, '../src/web/public/session-ui.js'), 'utf8'); + + it('renders a masked field, never a plain text input', () => { + const field = /]*id="remoteHostPassword"[^>]*>/.exec(html)?.[0] ?? ''; + expect(field, 'remoteHostPassword must exist').not.toBe(''); + expect(field).toContain('type="password"'); + }); + + it('sends the password on BOTH submit paths', () => { + // The form is reachable from "add a remote host" AND from the remote-case + // create flow, which builds its own host payload; wiring only one leaves the + // other silently key-only. + expect(ui.match(/\.\.\.\(password \? \{ password \} : \{\}\),/g) ?? []).toHaveLength(2); + expect(ui.match(/getElementById\('remoteHostPassword'\)/g) ?? []).toHaveLength(2); + }); + + it('does not trim the value, since spaces can be part of a password', () => { + for (const m of ui.matchAll(/const password = document\.getElementById\('remoteHostPassword'\)\.value([^;]*);/g)) { + expect(m[1]).toBe(''); + } + }); + + it('clears the field with the rest of the form', () => { + // Left behind, a typed password would be inherited by the NEXT host created + // from this form — a real leak between two different machines' credentials. + const list = /const remoteFields = \[([\s\S]*?)\];/.exec(ui)?.[1] ?? ''; + expect(list, 'remoteFields list not found').not.toBe(''); + expect(list).toContain("'remoteHostPassword'"); + }); + + it('never populates the field from a server response', () => { + // GET redacts the password, so anything assigning to this field would be + // writing a placeholder that a later save would persist as the real value. + expect(ui).not.toMatch(/getElementById\('remoteHostPassword'\)\.value\s*=/); + }); +});