diff --git a/packages/runtime-tests/integration/lifecycle/error-overview.test.tsx b/packages/runtime-tests/integration/lifecycle/error-overview.test.tsx index 1663ac3..5052c9e 100644 --- a/packages/runtime-tests/integration/lifecycle/error-overview.test.tsx +++ b/packages/runtime-tests/integration/lifecycle/error-overview.test.tsx @@ -1,3 +1,4 @@ +import { cwd } from "node:process"; import { defineComponent, h } from "vue"; import { expect, test } from "vite-plus/test"; import stripAnsi from "strip-ansi"; @@ -181,6 +182,40 @@ test("non-Error throw: overview message and rejected Error message are identical expect(reject.message).toBe(overviewMessage(frame)); }); +test("non-Error throw: '[unserializable value]' is shown AND rejected with, and they agree", async () => { + // A pathological thrown value whose `.message` is a non-string AND whose + // primitive coercion throws: messageForNonError selects the String(value) + // branch, String(value) throws (Symbol.toPrimitive), and safeString() falls + // back to the fixed "[unserializable value]" placeholder. The SAME helper feeds + // the overview header and render.ts's reject-wrap, so the displayed and rejected + // messages must BOTH be "[unserializable value]" — they cannot drift (e17). + const Thrower = defineComponent(() => { + return () => { + // eslint-disable-next-line @typescript-eslint/only-throw-error -- non-Error value whose primitive coercion throws, exercising the unserializable placeholder (e17) + throw { + get message(): number { + return 42; + }, + [Symbol.toPrimitive](): never { + throw new Error("toPrimitive boom"); + }, + }; + }; + }); + + const { frame, reject } = await renderFrameAndReject(Thrower); + + // Display: the overview surfaces the placeholder. + expect(frame).toContain(" ERROR [unserializable value]"); + // Reject: the SAME placeholder string on a real Error. + expect(reject.kind).toBe("rejected"); + if (reject.kind !== "rejected") throw new Error("expected rejection"); + expect(reject.isError).toBe(true); + expect(reject.message).toBe("[unserializable value]"); + // Consistency: display === reject. + expect(reject.message).toBe(overviewMessage(frame)); +}); + test("non-Error throw: non-string .message falls back to String on BOTH paths and they agree", async () => { const Thrower = defineComponent(() => { return () => { @@ -258,6 +293,77 @@ test("unparsable stack frame falls back to literal backslash-t (not a real TAB)" expect(fallbackLine).not.toContain("\t"); }); +test("throwing .stack getter renders ERROR header with no synthetic stack (hardened read)", async () => { + // A pathological thrown value with a THROWING `.stack` getter. ErrorOverview + // reads `.stack` during render; an unguarded read would throw, Vue would catch + // it and re-route a NEW Error (with a real `.stack` pointing into Vue/dist + // internals), and the boundary would re-render an overview that LEAKS those + // internal frames — the same synthetic-stack corruption the primitive test + // guards against. With the guarded single `.stack` read, the throw is swallowed + // and the overview renders header-only (the primitive-throw path). + const StackGetterThrower = defineComponent(() => { + return () => { + // eslint-disable-next-line @typescript-eslint/only-throw-error -- non-Error value with a throwing .stack getter, exercising the hardened read + throw { + message: "stack getter boom", + get stack(): string { + throw new Error("inner stack boom"); + }, + }; + }; + }); + + const frame = await renderErrorFrame(StackGetterThrower); + + // The header shows the string .message (read via messageForNonError). + expect(frame).toContain(" ERROR stack getter boom"); + + // No synthetic stack leaked: the unguarded-read regression surfaces as + // dist/render-to-string/runtime-core frames and "- " stack lines. None appear. + expect(frame).not.toContain("render-to-string"); + expect(frame).not.toContain("runtime-core"); + expect(frame).not.toContain("dist/"); + expect(frame).not.toMatch(/^\s*- /m); +}); + +test("stack origin pointing at a DIRECTORY renders header-only without leaking EISDIR", async () => { + // A crafted/stale `.stack` can parse to an existing DIRECTORY path. Before the + // fix, ErrorOverview's excerpt block did `fs.existsSync(dir)` (true for a dir) → + // `fs.readFileSync(dir)` throws EISDIR DURING render. The boundary then re-faults + // and repaints an overview for the EISDIR error while waitUntilExit() rejects with + // the ORIGINAL message — a displayed-vs-rejected DISAGREEMENT (violates e17). With + // the excerpt read guarded, the read failure is swallowed (no excerpt) and the + // overview renders the header/origin for the original error. + // + // We point the first frame at process.cwd() (a real directory on disk) so + // fs.existsSync passes but fs.readFileSync throws EISDIR. + const dirPath = cwd(); + const DirStackThrower = defineComponent(() => { + return () => { + const e = new Error("Dir stack boom"); + const firstLine = (e.stack ?? "").split("\n")[0] ?? "Error: Dir stack boom"; + // A parseable frame whose file is an existing directory. + e.stack = `${firstLine}\n at someFn (${dirPath}:1:1)`; + throw e; + }; + }); + + const { frame, reject } = await renderFrameAndReject(DirStackThrower); + + // Display: header shows the ORIGINAL message, not an EISDIR error. + expect(frame).toContain(" ERROR Dir stack boom"); + expect(frame).not.toContain("EISDIR"); + expect(frame).not.toContain("illegal operation on a directory"); + + // Reject: waitUntilExit() rejects with the ORIGINAL Error. + expect(reject.kind).toBe("rejected"); + if (reject.kind !== "rejected") throw new Error("expected rejection"); + expect(reject.isError).toBe(true); + expect(reject.message).toBe("Dir stack boom"); + // Consistency: displayed message === rejected message (no EISDIR leakage). + expect(reject.message).toBe(overviewMessage(frame)); +}); + test("primitive (non-Error) throw renders ERROR header with no synthetic stack", async () => { const frame = await renderErrorFrame(PrimitiveThrower); diff --git a/packages/runtime-tests/integration/lifecycle/message-for-non-error.test.ts b/packages/runtime-tests/integration/lifecycle/message-for-non-error.test.ts new file mode 100644 index 0000000..fd7e8b3 --- /dev/null +++ b/packages/runtime-tests/integration/lifecycle/message-for-non-error.test.ts @@ -0,0 +1,67 @@ +import { expect, test } from "vite-plus/test"; +import { messageForNonError } from "@vue-tui/runtime/internal"; + +// messageForNonError feeds the error-display / reject path: render.ts's +// onErrorCaptured wraps `new Error(messageForNonError(err))` with NO surrounding +// try/catch (render.ts ~523, and the errorHandler exit path ~1276). Its docstring +// promises it "must not itself throw on a pathological thrown object", so every +// coercion inside it has to be throw-safe. +// +// Imported from the built `@vue-tui/runtime/internal` dist (not source): the +// source module imports box.vue/text.vue, and the runtime-tests vitest config +// has no @vitejs/plugin-vue, so a source-relative import fails to compile. + +test("normal values coerce to their expected message", () => { + expect(messageForNonError(42)).toBe("42"); + expect(messageForNonError("boom")).toBe("boom"); + expect(messageForNonError({ message: "hi" })).toBe("hi"); + expect(messageForNonError(null)).toBe("null"); + expect(messageForNonError(undefined)).toBe("undefined"); + expect(messageForNonError(new Error("x"))).toBe("x"); +}); + +test("a throwing .message getter falls back without throwing", () => { + // The `.message` READ throws; String(value) on the plain object then yields + // "[object Object]" safely (toString is the default). Exercises the catch + // branch's coercion on a value that DOES coerce. + const pathological = { + get message(): string { + throw new Error("message getter boom"); + }, + }; + expect(() => messageForNonError(pathological)).not.toThrow(); + expect(messageForNonError(pathological)).toBe("[object Object]"); +}); + +test("a value whose primitive coercion throws does not throw (the confirmed bug)", () => { + // HIGH-severity bug: `.message` is a non-string (so the typeof check selects + // the String(value) branch), and String(value) itself throws because + // Symbol.toPrimitive throws. Before the fix, messageForNonError re-throws here, + // wedging render.ts's onErrorCaptured hook (the app hangs, waitUntilExit() + // never settles). + const pathological = { + get message(): number { + return 42; + }, + [Symbol.toPrimitive](): never { + throw new Error("toPrimitive boom"); + }, + }; + expect(() => messageForNonError(pathological)).not.toThrow(); + expect(messageForNonError(pathological)).toBe("[unserializable value]"); +}); + +test("a value that throws in BOTH the message read and String() does not throw", () => { + // Both guarded spots fire: the message getter throws (catch branch), and the + // catch branch's String(value) also throws because toString throws. + const pathological = { + get message(): string { + throw new Error("message getter boom"); + }, + toString(): never { + throw new Error("toString boom"); + }, + }; + expect(() => messageForNonError(pathological)).not.toThrow(); + expect(messageForNonError(pathological)).toBe("[unserializable value]"); +}); diff --git a/packages/runtime-tests/integration/lifecycle/pathological-throw-no-wedge.test.tsx b/packages/runtime-tests/integration/lifecycle/pathological-throw-no-wedge.test.tsx new file mode 100644 index 0000000..44b7080 --- /dev/null +++ b/packages/runtime-tests/integration/lifecycle/pathological-throw-no-wedge.test.tsx @@ -0,0 +1,103 @@ +import { defineComponent } from "vue"; +import { expect, test } from "vite-plus/test"; +import { createApp } from "@vue-tui/runtime"; +import { makeFakeWritable, makeFakeStdin } from "./test-streams.ts"; + +// End-to-end proof that a PATHOLOGICAL non-Error throw never WEDGES the error +// boundary. The original bug: a thrown value with a throwing coercion/getter +// makes one of the three sibling throw sites in the error-exit/display path +// re-throw with NO surrounding try/catch, which wedges Vue's post-flush +// scheduler — the app hangs and waitUntilExit() never settles. We mount a real +// app whose user component throws each shape and assert waitUntilExit() SETTLES +// (rejects) rather than hanging. +// +// Non-sequential: these mount/unmount apps but assert ONLY on the app's own +// waitUntilExit() outcome, never on process-global state (yoga counts, +// listenerCount, fake timers), so file-level parallelism can't perturb them. +// +// A wedge means the promise NEVER settles, which would hang the test until +// vitest's timeout. To keep RED fast and the failure legible, race +// waitUntilExit() against a short timeout and assert the settle won. +const WEDGE_TIMEOUT = Symbol("timeout"); + +async function expectMountDoesNotWedge(thrown: unknown): Promise { + const App = defineComponent(() => () => { + throw thrown; + }); + + const stdout = makeFakeWritable(); + const stderr = makeFakeWritable(); + const { stream: stdin } = makeFakeStdin(); + + const app = createApp(App); + app.mount({ stdout, stdin, stderr, debug: true, exitOnCtrlC: false }); + + const outcome = await Promise.race([ + app + .waitUntilExit() + .then(() => "resolved" as const) + .catch(() => "rejected" as const), + new Promise((r) => setTimeout(() => r(WEDGE_TIMEOUT), 1500)), + ]); + + // Not wedged: the promise settled before the timeout fired. + expect(outcome).not.toBe(WEDGE_TIMEOUT); + // A throwing component routes through exit(err) → REJECTS waitUntilExit(). + expect(outcome).toBe("rejected"); + + app.unmount(); +} + +test("a throwing Symbol.toPrimitive (messageForNonError path) does not wedge the boundary", async () => { + // `.message` is a non-string, so messageForNonError selects the String(value) + // branch; String(value) then invokes the throwing Symbol.toPrimitive. Guarded + // by safeString() inside messageForNonError. + await expectMountDoesNotWedge({ + get message(): number { + return 42; + }, + [Symbol.toPrimitive](): never { + throw new Error("toPrimitive boom"); + }, + }); +}); + +test("a throwing Symbol.toStringTag getter (isErrorInput path) does not wedge the boundary", async () => { + // isErrorInput() does `Object.prototype.toString.call(value)`, which READS + // `value[Symbol.toStringTag]`. A throwing getter there makes isErrorInput + // re-throw on the error-exit path. Guarded by the try/catch in isErrorInput. + await expectMountDoesNotWedge({ + get [Symbol.toStringTag](): string { + throw new Error("toStringTag boom"); + }, + }); +}); + +test("a throwing .stack getter (ErrorOverview path) does not wedge the boundary", async () => { + // ErrorOverview reads `.stack` off the raw thrown value during render. A + // throwing getter there makes the overview render throw. Guarded by the + // try/catch around the single `.stack` read in ErrorOverview. + await expectMountDoesNotWedge({ + get stack(): string { + throw new Error("stack boom"); + }, + }); +}); + +test("a Proxy with a throwing getPrototypeOf trap (isErrorInput path) does not wedge the boundary", async () => { + // isErrorInput() does `value instanceof Error`, which invokes the value's + // [[GetPrototypeOf]]. A Proxy with a throwing getPrototypeOf trap makes that + // `instanceof` re-throw on the error-exit path — a GENUINE wedge, since + // isErrorInput runs in onErrorCaptured / resolveExit / appContext.exit with no + // outer guard. Guarded by wrapping the whole isErrorInput body in try/catch. + await expectMountDoesNotWedge( + new Proxy( + {}, + { + getPrototypeOf(): never { + throw new Error("proto boom"); + }, + }, + ), + ); +}); diff --git a/packages/runtime/src/components/error-overview.ts b/packages/runtime/src/components/error-overview.ts index 7c9e7c1..8247f8c 100644 --- a/packages/runtime/src/components/error-overview.ts +++ b/packages/runtime/src/components/error-overview.ts @@ -22,6 +22,20 @@ const stackUtils = new StackUtils({ internals: StackUtils.nodeInternals(), }); +// String() coercion that can NEVER throw. A pathological thrown value can carry +// a throwing Symbol.toPrimitive/toString/valueOf, so bare String(value) is not +// safe here — and this feeds the error-display / reject path (render.ts's +// onErrorCaptured wraps `new Error(messageForNonError(err))` with NO surrounding +// try/catch). If coercion throws, fall back to a fixed placeholder so the caller +// always gets a string. +const safeString = (value: unknown): string => { + try { + return String(value); + } catch { + return "[unserializable value]"; + } +}; + // The user-facing message for a NON-Error thrown value. SINGLE source of truth // shared by the ErrorOverview header (what the user SEES) and render.ts's // reject-wrap (`new Error(messageForNonError(value))`, what waitUntilExit() @@ -34,14 +48,17 @@ export function messageForNonError(value: unknown): string { // the SAME read, and the read is guarded because this feeds the error-display // / reject path — it must not itself throw on a pathological thrown object // (e.g. a `.message` getter that throws), which the old `String(value)` form - // never touched. Fall back to `String(value)` on any failure, as before. + // never touched. Both String() fallbacks go through safeString: the coercion + // itself can also throw (a throwing Symbol.toPrimitive/toString), so guarding + // only the `.message` read left two unguarded throw sites that wedged the + // error boundary. This function must NEVER throw, per its contract above. let message: unknown; try { message = (value as { message?: unknown })?.message; } catch { - return String(value); + return safeString(value); } - return typeof message === "string" ? message : String(value); + return typeof message === "string" ? message : safeString(value); } export const ErrorOverview = defineComponent({ @@ -59,10 +76,19 @@ export const ErrorOverview = defineComponent({ const error = props.error; // Pull `.stack`/`.message` defensively: the value may be a primitive. - const errorStack = - typeof (error as { stack?: unknown })?.stack === "string" - ? (error as { stack: string }).stack - : undefined; + // Read `.stack` exactly ONCE under try/catch (mirroring how + // messageForNonError reads `.message` once under guard): a pathological + // thrown value can carry a throwing `.stack` getter, and this render runs + // on the error-display path where a throw would re-fault the boundary. If + // the read throws or isn't a string, treat it as no-stack — the overview + // then renders header-only, which is already the primitive-throw path. + let errorStack: string | undefined; + try { + const rawStack = (error as { stack?: unknown })?.stack; + errorStack = typeof rawStack === "string" ? rawStack : undefined; + } catch { + errorStack = undefined; + } // Ink renders `{error.message}`. A cross-realm Error has a different // prototype and fails `instanceof Error`, so read a string `.message` // structurally; primitives still fall back to String(value). The same @@ -78,14 +104,25 @@ export const ErrorOverview = defineComponent({ let excerpt: CodeExcerpt[] | undefined; let lineWidth = 0; + // Guard the source read: a crafted/stale `.stack` can parse to an existing + // DIRECTORY (fs.existsSync true → fs.readFileSync throws EISDIR) or an + // unreadable path, and this runs on the error-DISPLAY path. An unguarded + // throw would re-fault the boundary and repaint the overview for THAT error + // while waitUntilExit() rejects with the original — a displayed-vs-rejected + // disagreement (e17). On any failure, treat it as "no excerpt": leave + // `excerpt` undefined so only the header/origin render (the no-excerpt path). if (filePath && origin?.line && fs.existsSync(filePath)) { - const sourceCode = fs.readFileSync(filePath, "utf8"); - excerpt = codeExcerpt(sourceCode, origin.line); + try { + const sourceCode = fs.readFileSync(filePath, "utf8"); + excerpt = codeExcerpt(sourceCode, origin.line); - if (excerpt) { - for (const { line } of excerpt) { - lineWidth = Math.max(lineWidth, String(line).length); + if (excerpt) { + for (const { line } of excerpt) { + lineWidth = Math.max(lineWidth, String(line).length); + } } + } catch { + excerpt = undefined; } } diff --git a/packages/runtime/src/internal.ts b/packages/runtime/src/internal.ts index 880d2eb..ddfdf5c 100644 --- a/packages/runtime/src/internal.ts +++ b/packages/runtime/src/internal.ts @@ -22,3 +22,7 @@ export { type KittyKeyboardController, } from "./io/kitty-keyboard.ts"; export { INTERNAL_FRAME_SINK, type FrameSink } from "./io/frame-sink.ts"; +// Exposed for unit testing: error-overview.ts imports .vue SFCs, which the +// runtime-tests vitest config does not compile (no @vitejs/plugin-vue), so a +// pure-function test of this helper must reach it through the built dist. +export { messageForNonError } from "./components/error-overview.ts"; diff --git a/packages/runtime/src/render.ts b/packages/runtime/src/render.ts index 654d8ad..1e6cb8a 100644 --- a/packages/runtime/src/render.ts +++ b/packages/runtime/src/render.ts @@ -203,7 +203,20 @@ const liveInstances = new WeakMap(); // result values (string/number/plain object) brand as e.g. `[object String]`, // so they still RESOLVE — exactly Ink's contract. function isErrorInput(value: unknown): value is Error { - return value instanceof Error || Object.prototype.toString.call(value) === "[object Error]"; + // Must NEVER throw: runs in the error-exit/capture path (onErrorCaptured, + // appContext.exit, resolveExit) with NO surrounding try/catch — an unguarded + // throw here wedges the boundary. BOTH checks can throw on a pathological thrown + // value, so BOTH are inside the try: `instanceof` invokes the value's + // [[GetPrototypeOf]] (a Proxy's throwing getPrototypeOf trap re-throws), and + // `Object.prototype.toString.call` READS a throwing `Symbol.toStringTag` getter. + // On any throw, treat the value as a non-Error (false) so it routes through + // messageForNonError. The brand check is the cross-realm-Error fallback (see + // the rationale above the function); `instanceof` short-circuits the common case. + try { + return value instanceof Error || Object.prototype.toString.call(value) === "[object Error]"; + } catch { + return false; + } } type MaybeWritableStream = NodeJS.WriteStream & {