Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/app/start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
isAllowedWebviewUrl,
} from "../security";
import { registerIpcHandlers } from "../ipc/register";
import { setGatewayPromptParent } from "../gatewayPrompt";
import { showChatContextMenu } from "./context-menu";
import { buildMenu } from "./menu";
import { setupUpdater } from "./updater";
Expand Down Expand Up @@ -159,6 +160,10 @@ function createWindow(): void {
});

mainWindow.on("ready-to-show", () => mainWindow?.show());

// Let mid-turn gateway sudo/secret prompts parent their modal to this window.
setGatewayPromptParent(() => mainWindow);

mainWindow.webContents.on("render-process-gone", (_event, details) => {
console.error("[CRASH] Renderer process gone:", details.reason, details.exitCode);
});
Expand Down
34 changes: 24 additions & 10 deletions src/main/askpass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,21 @@ exit 1
};
}

async function showPasswordDialog(
/**
* Show a hardened, modal password/secret prompt and resolve with the entered
* value (or null on cancel). CSP-locked (`default-src 'none'`), sandboxed,
* ephemeral data-URL — the value is never persisted. Reused by both the
* installer's sudo askpass bridge and the mid-turn gateway sudo/secret prompts
* (see `gatewayPrompt.ts`). `title` / `heading` default to the installer's
* wording so existing callers are unchanged.
*/
export async function showPasswordDialog(
parent: BrowserWindow | null,
prompt: string,
opts: { title?: string; heading?: string } = {},
): Promise<string | null> {
const title = opts.title ?? "Administrator Password Required";
const heading = opts.heading ?? "The installer needs your password";
return new Promise((resolve) => {
const win = new BrowserWindow({
width: 460,
Expand All @@ -131,7 +142,7 @@ async function showPasswordDialog(
minimizable: false,
maximizable: false,
fullscreenable: false,
title: "Administrator Password Required",
title,
webPreferences: {
preload: join(__dirname, "../preload/askpass.js"),
nodeIntegration: false,
Expand Down Expand Up @@ -173,20 +184,23 @@ async function showPasswordDialog(
event.preventDefault(),
);

const html = buildDialogHtml(prompt);
const html = buildDialogHtml(prompt, heading);
win.loadURL(
"data:text/html;charset=UTF-8;base64," +
Buffer.from(html).toString("base64"),
);
});
}

function buildDialogHtml(prompt: string): string {
const safePrompt = prompt
.replace(/&/g, "&amp;")
.replace(/</g, "&lt;")
.replace(/>/g, "&gt;")
.replace(/"/g, "&quot;");
function buildDialogHtml(prompt: string, heading: string): string {
const esc = (s: string): string =>
s
.replace(/&/g, "&amp;")
.replace(/</g, "&lt;")
.replace(/>/g, "&gt;")
.replace(/"/g, "&quot;");
const safePrompt = esc(prompt);
const safeHeading = esc(heading);
return `<!doctype html>
<html><head><meta charset="utf-8">
<meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src 'unsafe-inline'; script-src 'none'; img-src 'none'; connect-src 'none'; frame-src 'none'; base-uri 'none'; form-action 'none'">
Expand All @@ -203,7 +217,7 @@ function buildDialogHtml(prompt: string): string {
button:hover { opacity:0.9; }
</style></head>
<body>
<div class="title">The installer needs your password</div>
<div class="title">${safeHeading}</div>
<div class="prompt">${safePrompt}</div>
<input id="pw" type="password" autofocus autocomplete="off" />
<div class="row">
Expand Down
69 changes: 69 additions & 0 deletions src/main/gatewayPrompt.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import type { BrowserWindow } from "electron";
import { showPasswordDialog } from "./askpass";

/**
* Mid-turn gateway credential prompts (`sudo.request` / `secret.request`).
*
* Unlike `clarify.request` — which renders an inline card in the chat
* transcript — a sudo password or a secret value is sensitive and must NEVER
* land in scrollback. So these reuse the installer's hardened askpass modal
* (`showPasswordDialog`): CSP-locked `default-src 'none'`, sandboxed, ephemeral
* data-URL, the value never persisted.
*
* Gateway protocol (NousResearch/hermes-agent, tui_gateway/server.py), keyed by
* request_id:
* sudo.request {} -> sudo.respond { request_id, password }
* secret.request { prompt, env_var, ... } -> secret.respond { request_id, value }
* An empty answer is a safe "skip": the gateway treats secret.request as
* skipped and lets a terminal sudo prompt fail cleanly, so cancel maps to "".
*/

let parentWindowGetter: () => BrowserWindow | null = () => null;

/** Wire the provider that returns the window to parent the modal to. Called
* once from index.ts after the main window is created. */
export function setGatewayPromptParent(
getter: () => BrowserWindow | null,
): void {
parentWindowGetter = getter;
}

/**
* Prompt for the sudo password. Resolves with the password, or "" if the user
* cancels (safe skip — terminal sudo then fails cleanly rather than hanging).
*/
export async function promptSudoPassword(): Promise<string> {
const parent = parentWindowGetter();
const value = await showPasswordDialog(
parent,
"An agent command needs administrator (sudo) access to continue. " +
"Your password is sent only to the local sudo prompt and is never stored.",
{
title: "Administrator Password Required",
heading: "Hermes needs your sudo password",
},
);
return value ?? "";
}

/**
* Prompt for a named secret the agent requested (e.g. an API key it needs to
* store). Resolves with the value, or "" if the user cancels (safe skip — the
* gateway records the secret as skipped).
*/
export async function promptSecretValue(
envVar: string,
prompt: string,
): Promise<string> {
const parent = parentWindowGetter();
const detail =
(prompt && prompt.trim()) ||
`The agent is requesting a value for ${envVar || "a secret"}.`;
const value = await showPasswordDialog(parent, detail, {
title: "Secret Required",
heading: envVar
? `Hermes needs a value for ${envVar}`
: "Hermes needs a secret value",
});
return value ?? "";
}
Comment on lines +54 to +69

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 envVar used verbatim in the modal heading without sanitisation. envVar comes directly from event.payload.env_var in the gateway event stream — an untrusted, agent-controlled string. It flows into the heading option and is rendered as <div class="title">${safeHeading}</div> in buildDialogHtml, which does HTML-escape it, so the XSS risk in the rendered DOM is mitigated. However, the raw value also appears in the title option, which is used as the OS-level window title (win.title), where HTML escaping is irrelevant. A length cap and simple printable-chars validation on envVar would close the spoofing surface entirely.

67 changes: 59 additions & 8 deletions src/main/hermes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ import {
getActiveProfileNameSync,
} from "./utils";
import { getProfilePort } from "./gateway-ports";
import { promptSudoPassword, promptSecretValue } from "./gatewayPrompt";
import { getSecret } from "./secrets";
import { readModels } from "./models";
import { providerListSafe } from "./secrets";
import { HIDDEN_SUBPROCESS_OPTIONS } from "./process-options";
Expand Down Expand Up @@ -1984,14 +1986,63 @@ async function sendMessageViaTuiGateway(
}

if (event.type === "sudo.request" || event.type === "secret.request") {
// Out of scope for the inline-clarify change: a desktop sudo/secret prompt
// carries its own security-review surface and is a deliberate follow-up.
void client
.request("session.interrupt", { session_id: activeSessionId }, 5_000)
.catch(() => undefined);
finish(
`Hermes requested ${event.type.replace(".request", "")} input, but Hermes One does not yet expose that gateway dialog.`,
);
const isSudo = event.type === "sudo.request";
const requestId =
typeof event.payload?.request_id === "string"
? event.payload.request_id
: "";
if (!requestId) {
void client
.request("session.interrupt", { session_id: activeSessionId }, 5_000)
.catch(() => undefined);
finish(
`Hermes requested ${event.type.replace(".request", "")} input, but the gateway provided no request_id to answer.`,
);
return;
}
// A sudo password / secret value is sensitive — collect it in the
// hardened askpass modal (never the chat transcript) and forward it to
// the gateway. Cancel maps to "" (a safe skip the gateway handles).
//
// For secret.request: try the configured security provider first. If the
// vault already holds the key, answer silently without prompting the user.
const payload = event.payload as
| { prompt?: string; env_var?: string }
| undefined;
const envVar = String(payload?.env_var ?? "");

// Vault-first resolution for secret.request: attempt a provider lookup
// before falling back to the interactive modal. sudo.request always needs
// an interactive password — no vault lookup applies.
const vaultValue =
!isSudo && envVar ? getSecret(envVar, profile) : null;

const collect: Promise<string> =
vaultValue != null
? Promise.resolve(vaultValue)
: isSudo
? promptSudoPassword()
: promptSecretValue(envVar, String(payload?.prompt ?? ""));

void collect
.then((answer) => {
if (finished) return; // turn was cancelled while modal was open
const method = isSudo ? "sudo.respond" : "secret.respond";
const params = isSudo
? { request_id: requestId, password: answer }
: { request_id: requestId, value: answer };
return client.request(method, params, 300_000);
})
Comment on lines +2027 to +2035

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Missing finished guard before forwarding credentials to the gateway. If cancel() or finish() is called while the askpass modal is still open (e.g. the user aborts the chat turn), finished becomes true but the modal stays open with no way to dismiss it programmatically. When the user eventually submits the modal, collect resolves and the .then() fires unconditionally — forwarding the sudo password or secret value to the gateway on a session that has already been torn down. The clarify path avoids this by clearing pendingClarify in finish()/cancel(), making stale responses a no-op; the same protection is missing here.

Suggested change
void collect
.then((answer) => {
const method = isSudo ? "sudo.respond" : "secret.respond";
const params = isSudo
? { request_id: requestId, password: answer }
: { request_id: requestId, value: answer };
return client.request(method, params, 300_000);
})
void collect
.then((answer) => {
if (finished) return;
const method = isSudo ? "sudo.respond" : "secret.respond";
const params = isSudo
? { request_id: requestId, password: answer }
: { request_id: requestId, value: answer };
return client.request(method, params, 300_000);
})

.catch((error) => {
const message =
error instanceof Error ? error.message : String(error);
if (!hasGatewayOutput) {
startApiFallback(message);
return;
}
finish(message);
});
return;
}
});

Expand Down
38 changes: 38 additions & 0 deletions tests/askpass-security.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ import { ASKPASS_SUBMIT_CHANNEL } from "../src/shared/askpass";
const ROOT = join(__dirname, "..");
const askpassMainSrc = readFileSync(join(ROOT, "src/main/askpass.ts"), "utf-8");
const sudoCredsSrc = readFileSync(join(ROOT, "src/main/sudoCreds.ts"), "utf-8");
const gatewayPromptSrc = readFileSync(
join(ROOT, "src/main/gatewayPrompt.ts"),
"utf-8",
);
const hermesSrc = readFileSync(join(ROOT, "src/main/hermes.ts"), "utf-8");
const askpassPreloadSrc = readFileSync(
join(ROOT, "src/preload/askpass.ts"),
"utf-8",
Expand Down Expand Up @@ -92,3 +97,36 @@ describe("askpass Electron hardening", () => {
expect(electronViteConfigSrc).toContain("src/preload/askpass.ts");
});
});

describe("gateway sudo/secret prompt handling", () => {
it("collects sudo/secret via the hardened askpass modal, not the chat transcript", () => {
// Reuses the shared hardened dialog rather than a bespoke window.
expect(gatewayPromptSrc).toContain(
'import { showPasswordDialog } from "./askpass"',
);
expect(gatewayPromptSrc).toContain("showPasswordDialog(");
// Sensitive values must never be rendered as a chat message / clarify card.
expect(gatewayPromptSrc).not.toMatch(/onClarify|ClarifyMessage|safeSend/);
});

it("never logs or persists the collected secret", () => {
expect(gatewayPromptSrc).not.toMatch(/console\.(log|warn|error|info)/);
expect(gatewayPromptSrc).not.toMatch(/writeFile|appendFile|setEnvValue/);
});

it("forwards answers with the gateway's exact respond shapes", () => {
// sudo.respond carries `password`; secret.respond carries `value`.
expect(hermesSrc).toContain('"sudo.respond"');
expect(hermesSrc).toContain('"secret.respond"');
expect(hermesSrc).toContain("password: answer");
expect(hermesSrc).toContain("value: answer");
// Both keyed by request_id, mirroring clarify.respond.
expect(hermesSrc).toContain("request_id: requestId, password");
expect(hermesSrc).toContain("request_id: requestId, value");
});

it("maps cancel to an empty answer (safe skip), never blocking the turn", () => {
// showPasswordDialog resolves null on cancel; the prompt helpers coerce to "".
expect(gatewayPromptSrc).toContain('return value ?? "";');
});
});
22 changes: 16 additions & 6 deletions tests/ssh-remote.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,28 @@ const sshConfig: SshConfig = {

function runWithHermesShim(command: string): Buffer {
const home = mkdtempSync(join(tmpdir(), "hermes-ssh-cmd-home-"));
const bin = join(home, "bin");
mkdirSync(bin, { recursive: true });
const hermes = join(bin, "hermes");
// Install the shim at a path buildRemoteHermesCmd PROBES BY ABSOLUTE PATH
// ($HOME/.local/bin/hermes), not just on PATH. The command runs under
// `bash -lc` (a login shell), which re-sources /etc/profile and RESETS PATH —
// so a shim reachable only via a prepended PATH entry is dropped, and the
// final `command -v hermes` fallback then finds either nothing (clean CI
// container → the test fails) or a real host hermes (dev box → the test
// passes for the wrong reason). Placing the shim at the probed absolute
// location makes the `[ -x $HOME/.local/bin/hermes ]` branch fire first,
// independent of login-shell PATH behavior and of whether a real hermes
// exists on the host. PATH is still prepended as belt-and-suspenders.
const localBin = join(home, ".local", "bin");
mkdirSync(localBin, { recursive: true });
const hermes = join(localBin, "hermes");
writeFileSync(
hermes,
[
"#!/usr/bin/env bash",
'if [ "$1" = "doctor" ]; then',
' printf "doctor stderr preserved\\n" >&2',
' printf "doctor stderr preserved\\\\n" >&2',
" exit 0",
"fi",
'printf "%s\\0" "$@"',
'printf "%s\\\\0" "$@"',
"",
].join("\n"),
);
Expand All @@ -53,7 +63,7 @@ function runWithHermesShim(command: string): Buffer {
env: {
...process.env,
HOME: home,
PATH: `${bin}:${process.env.PATH || ""}`,
PATH: `${localBin}:${process.env.PATH || ""}`,
},
});
}
Expand Down
Loading