diff --git a/src/commands/login.ts b/src/commands/login.ts index 1adcba7..26469e8 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -31,9 +31,16 @@ export const loginCommand = new Command("login") // Step 2: best-effort daemon login. The hsh-tunneld daemon owns its // own token (it does its own gateway round-trip via IPC — we never // hand it the CLI's token, preserving the daemon-owns-token trust - // model). If the daemon isn't installed/running/configured this is - // a silent no-op, so a tunnel-less setup sees no change in behavior. + // model). + // + // If the daemon is genuinely not installed this is a silent no-op. + // But if the daemon IS installed and we fail to update its token + // (unreachable, unconfigured, login error), loginDaemon returns + // false and has already warned the user — we propagate a non-zero + // exit so the failure is visible in scripts and shells. The CLI + // login itself already succeeded, so this only flags the daemon leg. if (opts.tunnel) { - await loginDaemon({ browser: opts.browser, timeout: 180, optional: true }); + const ok = await loginDaemon({ browser: opts.browser, timeout: 180, optional: true }); + if (!ok) process.exitCode = 1; } }); diff --git a/src/commands/tunnel.ts b/src/commands/tunnel.ts index 3f3c0df..8e715cc 100644 --- a/src/commands/tunnel.ts +++ b/src/commands/tunnel.ts @@ -98,6 +98,18 @@ const statusSub = new Command("status") ...(s.last_error ? { "Last error": chalk.red(s.last_error) } : {}), }); console.log(); + + // The daemon reports "authenticated" whenever it merely HOLDS a + // token — it can't know the token is valid until it dials the + // gateway. When that dial 401s (expired/rejected token) the only + // signal is a buried last_error string. Detect it and surface a + // concrete next step, otherwise the user sees "authenticated" + + // an opaque "401 Unauthorized" and has no idea the fix is to log + // in again. + if (isAuthError(s.last_error)) { + warn("The daemon's saved login has expired or was rejected by the gateway."); + info("Re-authenticate the daemon with: hsh tunnel login"); + } } catch (err) { renderApiError(err); process.exitCode = 1; @@ -263,34 +275,96 @@ const loginSub = new Command("login") * auth method and run the matching OIDC or local-auth path, then report * the resulting tunnel state. * - * `optional` controls the contract for the "daemon not usable" cases - * (not installed, not running, no api_url configured): + * `optional` controls the contract for the "daemon not usable" cases: * - * - optional=false (interactive `hsh tunnel login`): these are hard - * errors — the user explicitly targeted the daemon, so we tell them - * exactly what to fix and return false. + * - optional=false (interactive `hsh tunnel login`): every failure is + * a hard error — the user explicitly targeted the daemon, so we + * tell them exactly what to fix and return false. * * - optional=true (best-effort follow-up from the unified - * `hsh login`): these are silently skipped and return true. The - * user asked to log the *CLI* in; the daemon is a bonus, not a - * requirement, and a missing daemon must not fail `hsh login`. + * `hsh login`): we ONLY skip silently when the daemon is genuinely + * absent (no IPC socket on disk). When the daemon is installed but + * unreachable, unconfigured, or its login fails, we WARN and return + * false — silently swallowing those cases was the bug that left the + * daemon on a stale/expired token while `hsh login` claimed success. + * + * Returns true only when the daemon login actually succeeded OR the + * daemon is genuinely absent (optional=true). Returns false on any real + * failure so the caller can set a non-zero exit code. + */ +/** Human-readable reason from a TunnelUnavailableError (or any error). */ +function unavailableReason(err: unknown): string { + if (err instanceof TunnelUnavailableError) { + return `${err.reason} (${err.message})`; + } + return errMessage(err); +} + +function errMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} + +/** + * Heuristic: does the daemon's last_error indicate the saved token was + * rejected by the gateway (as opposed to a network/DNS/route failure)? * - * Returns true when the daemon login succeeded OR was legitimately - * skipped (optional=true), false on a real failure. + * The daemon surfaces gateway HTTP failures verbatim in last_error + * (e.g. "bring up: fetch serverinfo: GET .../api/serverinfo returned + * 401 Unauthorized: {...access denied...}"). We match on the canonical + * auth signals — a 401 status or the gateway's "access denied" body — + * so `hsh tunnel status` can point the user at `hsh tunnel login` + * instead of leaving them staring at a raw HTTP error. */ +export function isAuthError(lastError: string | undefined): boolean { + if (!lastError) return false; + const e = lastError.toLowerCase(); + return ( + e.includes("401") || + e.includes("unauthorized") || + e.includes("access denied") || + e.includes("token is expired") || + e.includes("token expired") + ); +} + export async function loginDaemon(opts: { browser: boolean; timeout: number; optional: boolean; }): Promise { + // The `optional` (best-effort) contract distinguishes two very + // different "daemon not usable" situations: + // + // - Genuinely ABSENT: no IPC socket on disk → the daemon isn't + // installed/running. Skipping silently is correct; a tunnel-less + // setup must not see noise from `hsh login`. + // + // - Present but UNREACHABLE or its login fails: the socket exists + // (daemon installed) but we can't read its control token, can't + // connect, or the daemon-side login errors. Skipping silently + // here is the bug that let `hsh login` leave the daemon on a + // stale/expired token while reporting success — so we WARN and + // fail loudly, pointing the user at `hsh tunnel login`. + // + // The discriminator is whether the socket file exists. + const daemonPresent = resolveSocketPath().exists; + const skipOrFail = (reason: string): boolean => { + if (opts.optional && !daemonPresent) { + // Truly not installed — silent best-effort skip. + return true; + } + warn(`Tunnel daemon login was not completed: ${reason}`); + info("Your CLI is logged in, but the tunnel daemon still holds its previous token."); + info("Run `hsh tunnel login` to update the daemon."); + return false; + }; + let client: TunnelClient; try { client = TunnelClient.connect(); } catch (err) { if (opts.optional) { - // Daemon not installed/running — skip silently. The CLI login - // already succeeded; the tunnel is simply not part of this setup. - return true; + return skipOrFail(unavailableReason(err)); } renderUnavailable(err); return false; @@ -304,11 +378,12 @@ export async function loginDaemon(opts: { const cfg = await client.config(); if (!cfg.api_url) { if (opts.optional) { - // Best-effort: nudge but don't fail. The user can wire the - // daemon up later with `hsh tunnel config set api-url`. - info("Tunnel daemon detected but not configured — skipping tunnel login."); - info("Configure it with: hsh tunnel config set api-url "); - return true; + // Daemon is reachable but unconfigured. This is a real + // "couldn't update the daemon" case, not an absent daemon, so + // surface it rather than swallow it. + warn("Tunnel daemon is installed but has no api_url configured."); + info("Configure it with: hsh tunnel config set api-url , then run hsh tunnel login."); + return false; } error("Daemon has no api_url configured."); info("Set it first: hsh tunnel config set api-url "); @@ -316,7 +391,7 @@ export async function loginDaemon(opts: { } apiUrl = cfg.api_url; } catch (err) { - if (opts.optional) return true; + if (opts.optional) return skipOrFail(errMessage(err)); renderApiError(err); return false; } @@ -329,7 +404,7 @@ export async function loginDaemon(opts: { try { serverInfo = await getPublicServerInfo(apiUrl); } catch (err) { - if (opts.optional) return true; + if (opts.optional) return skipOrFail(errMessage(err)); error(`Could not detect auth method: ${(err as Error).message}`); info(`Verify the api-url: hsh tunnel config get`); return false; diff --git a/tests/login-daemon.test.ts b/tests/login-daemon.test.ts new file mode 100644 index 0000000..875e485 --- /dev/null +++ b/tests/login-daemon.test.ts @@ -0,0 +1,73 @@ +/** + * tests/login-daemon.test.ts — regression tests for the unified + * `hsh login` daemon leg (loginDaemon). + * + * The bug being guarded: with optional=true, loginDaemon used to skip + * silently whenever the daemon couldn't be reached — including when the + * daemon WAS installed but its control token was unreadable. That left + * the daemon on a stale/expired token while `hsh login` reported + * success, forcing users to run `hsh tunnel login` manually. + * + * The fix: only skip silently when the daemon is GENUINELY ABSENT (no + * IPC socket on disk). When the socket exists but the login can't be + * completed, loginDaemon must return false (caller exits non-zero). + * + * We drive the real loginDaemon and steer the "is the daemon present" + * discriminator via the HSH_TUNNELD_SOCKET env override that + * resolveSocketPath honors. No real daemon is ever contacted: when the + * socket path points at a non-socket file, TunnelClient.connect throws + * before any network I/O (no control token), which is exactly the + * "present but unusable" branch we want to assert on. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; + +import { loginDaemon } from "../src/commands/tunnel.ts"; + +let tmp: string; +const savedSocket = process.env.HSH_TUNNELD_SOCKET; +const savedToken = process.env.HSH_TUNNELD_TOKEN_FILE; + +beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), "hsh-login-daemon-")); +}); + +afterEach(() => { + if (savedSocket === undefined) delete process.env.HSH_TUNNELD_SOCKET; + else process.env.HSH_TUNNELD_SOCKET = savedSocket; + if (savedToken === undefined) delete process.env.HSH_TUNNELD_TOKEN_FILE; + else process.env.HSH_TUNNELD_TOKEN_FILE = savedToken; + rmSync(tmp, { recursive: true, force: true }); +}); + +describe("loginDaemon optional contract", () => { + test("genuinely absent daemon (no socket) → silent skip, returns true", async () => { + // Point at a socket path that does not exist on disk. + process.env.HSH_TUNNELD_SOCKET = join(tmp, "does-not-exist.sock"); + delete process.env.HSH_TUNNELD_TOKEN_FILE; + + const ok = await loginDaemon({ browser: false, timeout: 1, optional: true }); + // Absent daemon must NOT fail `hsh login`. + expect(ok).toBe(true); + }); + + test("installed daemon we can't authenticate against → returns false (no silent skip)", async () => { + // Create a real file at the socket path so resolveSocketPath reports + // exists=true (daemon "present"), but provide no readable control + // token so connect() throws — the exact "present but unusable" case + // that previously skipped silently. + const sockPath = join(tmp, "hsh.sock"); + writeFileSync(sockPath, ""); + process.env.HSH_TUNNELD_SOCKET = sockPath; + // Token file points at a missing path → connect() throws no-token. + process.env.HSH_TUNNELD_TOKEN_FILE = join(tmp, "missing-token"); + + const ok = await loginDaemon({ browser: false, timeout: 1, optional: true }); + // Present-but-unusable must surface as a failure so the caller + // exits non-zero and the user knows to run `hsh tunnel login`. + expect(ok).toBe(false); + }); +}); diff --git a/tests/tunnel-status-auth-hint.test.ts b/tests/tunnel-status-auth-hint.test.ts new file mode 100644 index 0000000..adb2bfb --- /dev/null +++ b/tests/tunnel-status-auth-hint.test.ts @@ -0,0 +1,43 @@ +/** + * tests/tunnel-status-auth-hint.test.ts — unit tests for isAuthError, + * the heuristic that decides whether `hsh tunnel status` should hint + * "re-run hsh tunnel login" based on the daemon's last_error. + * + * The daemon reports "authenticated" whenever it merely holds a token; + * a rejected/expired token only shows up as a buried HTTP error in + * last_error. isAuthError extracts the auth signal so the UI can give a + * concrete next step. + */ + +import { describe, expect, test } from "bun:test"; + +import { isAuthError } from "../src/commands/tunnel.ts"; + +describe("isAuthError", () => { + test("matches the real serverinfo 401 last_error", () => { + const real = + 'bring up: fetch serverinfo: GET https://sandbox.hoop.dev/api/serverinfo ' + + 'returned 401 Unauthorized: {"message":"access denied"}'; + expect(isAuthError(real)).toBe(true); + }); + + test("matches a bare 401 / unauthorized / access denied / expired", () => { + expect(isAuthError("got 401 from gateway")).toBe(true); + expect(isAuthError("Unauthorized")).toBe(true); + expect(isAuthError("access denied")).toBe(true); + expect(isAuthError("token is expired")).toBe(true); + expect(isAuthError("token expired")).toBe(true); + }); + + test("does NOT match non-auth failures", () => { + expect(isAuthError("dial tcp: connection refused")).toBe(false); + expect(isAuthError("configure routes: permission denied")).toBe(false); + expect(isAuthError("fetch serverinfo: context deadline exceeded")).toBe(false); + expect(isAuthError("no tunnelable connections found for this user")).toBe(false); + }); + + test("returns false for empty / undefined", () => { + expect(isAuthError(undefined)).toBe(false); + expect(isAuthError("")).toBe(false); + }); +});