From df843b67fe16a60d93258ecc2c39d4cb33b50f2f Mon Sep 17 00:00:00 2001 From: Yunfei He Date: Sun, 14 Jun 2026 19:44:27 +0800 Subject: [PATCH] fix(runtime): validate custom borderStyle object shape at render (#172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A malformed custom borderStyle OBJECT (e.g. `{ topLeft, topRight }` missing `top`) — or a truthy non-string non-object value (e.g. a number from a JS caller) — bypassed assertBoxValid's render-time check, which only shape-checked the STRING form. It reached drawBorder, passed the `if (!chars)` guard, and threw `Cannot read properties of undefined (reading 'repeat')` deep in the post-flush PAINT pass — wedging Vue's scheduler instead of surfacing a recoverable error, exactly the failure mode box-validate.ts exists to prevent. Resolve borderStyle to a BoxStyle the same way paint's drawBorder does (string -> cliBoxes[name], object -> directly), then shape-check the result: every one of the 8 glyphs paint reads (top/bottom/left/right + the four corners) must be a string. Any invalid value now throws a clean error AT RENDER, caught by the error boundary — like the existing unknown-string case. The string case keeps its "Unknown borderStyle:" wording; the object/non-string case uses "Invalid borderStyle:". Test-first: borders.test.tsx now asserts a malformed object, a number, each individually-missing glyph, and a present-but-non-string glyph all reject at render (not the opaque paint TypeError), and a complete custom object still paints a border. Co-authored-by: Claude Opus 4.8 (1M context) --- .../integration/components/borders.test.tsx | 135 ++++++++++++++++++ .../runtime/src/components/box-validate.ts | 94 ++++++++---- 2 files changed, 205 insertions(+), 24 deletions(-) diff --git a/packages/runtime-tests/integration/components/borders.test.tsx b/packages/runtime-tests/integration/components/borders.test.tsx index 6b9e7fd..395e87c 100644 --- a/packages/runtime-tests/integration/components/borders.test.tsx +++ b/packages/runtime-tests/integration/components/borders.test.tsx @@ -1675,6 +1675,141 @@ test("non-throwing borderStyle (false/undefined/valid/object) renders normally", expect(objectFrame).toContain("H"); }); +// A MALFORMED custom BoxStyle OBJECT (missing a glyph paint reads, e.g. `top`) +// must REJECT at RENDER with a clean error — exactly like an unknown STRING — +// rather than slipping past validation and throwing an opaque TypeError +// (`Cannot read properties of undefined (reading 'repeat')`) deep in the +// post-flush PAINT pass, where a throw wedges Vue's scheduler. Before the fix, +// assertBoxValid only shape-checked STRING borderStyles, so an object form +// bypassed the check entirely and reached `chars.top.repeat(...)` in +// drawBorder. (companion of audit 2.3 — same paint-wedge failure mode.) +test("MALFORMED borderStyle OBJECT (missing glyph) rejects at render", async ({ expect }) => { + await expect( + render( + defineComponent(() => () => ( + // Missing `top` (and the other edges/glyphs paint reads). Reachable in + // JS or via a partial object the consumer believes is a full BoxStyle. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + + Hi + + )), + { columns: 100 }, + ), + ).rejects.toThrow(/Invalid borderStyle/); + // It must NOT surface the opaque paint-phase TypeError. + await expect( + render( + defineComponent(() => () => ( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + + Hi + + )), + { columns: 100 }, + ), + ).rejects.not.toThrow(/Cannot read properties of undefined/); +}); + +// A truthy NON-STRING NON-OBJECT borderStyle (e.g. a number from a JS caller) +// hits the same validation gap and must also reject cleanly at render rather +// than crash in paint. +test("non-string non-object borderStyle (number) rejects at render", async ({ expect }) => { + await expect( + render( + defineComponent(() => () => ( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + + Hi + + )), + { columns: 100 }, + ), + ).rejects.toThrow(/Invalid borderStyle/); +}); + +// Exercise the FULL "every one of the 8 glyphs paint reads must be a string" +// contract: an object that is complete EXCEPT one glyph (each, table-driven) +// must reject at render — paint reads top/bottom/left/right unconditionally and +// the four corners when an adjacent side is drawn, so any one missing glyph can +// reach a `.repeat(...)`/concat on a non-string mid-commit. +const COMPLETE_GLYPHS = { + topLeft: "A", + top: "B", + topRight: "C", + right: "D", + bottomRight: "E", + bottom: "F", + bottomLeft: "G", + left: "H", +} as const; +for (const glyph of Object.keys(COMPLETE_GLYPHS)) { + test(`borderStyle OBJECT missing only "${glyph}" rejects at render`, async ({ expect }) => { + const partial = { ...COMPLETE_GLYPHS } as Record; + delete partial[glyph]; + await expect( + render( + defineComponent(() => () => ( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + + Hi + + )), + { columns: 100 }, + ), + ).rejects.toThrow(/Invalid borderStyle/); + }); +} + +// A glyph that is PRESENT but NON-STRING (e.g. a number) must also reject — the +// shape check requires `typeof === "string"`, not mere presence, since paint +// calls `.repeat(...)`/concatenates the glyph as a string. +test("borderStyle OBJECT with a non-string glyph rejects at render", async ({ expect }) => { + await expect( + render( + defineComponent(() => () => ( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + + Hi + + )), + { columns: 100 }, + ), + ).rejects.toThrow(/Invalid borderStyle/); +}); + +// Regression guard: a COMPLETE custom BoxStyle object (all 8 glyphs paint reads, +// as strings) must still render its border, never rejecting. This pins that the +// shape check accepts a genuinely valid object. +test("COMPLETE custom borderStyle OBJECT renders a border (regression)", async ({ expect }) => { + const { lastFrame } = await render( + defineComponent(() => () => ( + + Hi + + )), + { columns: 100 }, + ); + const frame = stripAnsi(lastFrame()!); + const lines = frame.split("\n"); + // Top row: A + B...B + C ; content row: H...D ; bottom row: G + F...F + E + expect(lines[0]).toMatch(/^AB+C$/); + expect(lines[1]).toMatch(/^H.*D$/); + expect(lines[2]).toMatch(/^GF+E$/); +}); + // borderDimColor should not dim styled child Text touching left edge test("borderDimColor does not dim styled child Text touching left edge", async ({ expect }) => { const { lastFrame } = await render( diff --git a/packages/runtime/src/components/box-validate.ts b/packages/runtime/src/components/box-validate.ts index 6b503ec..c6a0646 100644 --- a/packages/runtime/src/components/box-validate.ts +++ b/packages/runtime/src/components/box-validate.ts @@ -2,6 +2,35 @@ import cliBoxes from "cli-boxes"; import { assertValidBackgroundColor, assertValidForegroundColor } from "../paint/text-style.ts"; import type { BoxProps, BoxStyle } from "./box-props.ts"; +// The exact glyph keys paint's drawBorder reads off a resolved BoxStyle: +// `top`/`bottom`/`left`/`right` are read unconditionally for any drawn edge, and +// the four corners are read when their adjacent sides are drawn (edges default to +// drawn). Validating ALL of them here guarantees no malformed object can reach +// paint and throw `.repeat(...)` / string-concat a non-string mid-commit, +// regardless of which per-edge toggles are set. Keep in sync with drawBorder. +const BOX_STYLE_GLYPHS = [ + "top", + "bottom", + "left", + "right", + "topLeft", + "topRight", + "bottomLeft", + "bottomRight", +] as const; + +/** + * True only when `value` is a real BoxStyle: an object carrying every glyph + * paint reads, each a string. Rejects undefined (unknown preset name), the + * cli-boxes `default` self-key / prototype members (objects without string + * glyphs), partial custom objects, and truthy non-objects (e.g. a number). + */ +function isValidBoxStyleShape(value: unknown): value is BoxStyle { + if (typeof value !== "object" || value === null) return false; + const box = value as Record; + return BOX_STYLE_GLYPHS.every((glyph) => typeof box[glyph] === "string"); +} + /** * Eager render-time validation for ``. Runs every render and throws into the * error boundary on invalid input — exactly as box.ts's render fn did. Returns @@ -106,41 +135,58 @@ export function assertBoxValid(props: BoxProps): true { // rather than throwing (a throw in the post-flush paint pass wedges Vue's // scheduler). Same accepted limitation as the borderStyle fix (#124). - // Validate borderStyle during RENDER so an unknown name is caught by + // Validate borderStyle during RENDER so an invalid value is caught by // vue-tui's error boundary (onErrorCaptured → ErrorOverview), exactly like // any other component render error. Ink crashes on an unknown borderStyle // with a raw TypeError during paint (render-border.ts reads box.topLeft off - // cliBoxes[name] === undefined); we align to that "throw on unknown" contract - // but do it here rather than in paint, where a throw would unwind through - // Vue's post-flush commit and wedge its scheduler. Only a NON-EMPTY unknown - // STRING throws: a falsy value (false/undefined/"" = no border) and a custom - // BoxStyle OBJECT are both valid and pass through. (audit 2.3) - // `borderStyle.length > 0` (not `!== ""`): once `typeof === "string"` narrows - // the prop to the BorderStyle keyof union, an `!== ""` literal comparison has - // "no overlap" per TS (the union has no `""` member). The empty string is only - // reachable via a TS-bypass; `.length > 0` excludes it without tripping that. + // cliBoxes[name] === undefined); we align to that "throw on bad input" + // contract but do it here rather than in paint, where a throw would unwind + // through Vue's post-flush commit and wedge its scheduler. A falsy value + // (false/undefined/"" = no border) is valid and passes through. (audit 2.3) // - // We validate the RESOLVED box has a real BoxStyle SHAPE rather than testing - // `borderStyle in cliBoxes`, because `in` has two false-accept holes that let - // a non-box value reach paint (which then reads `.top`/`.topLeft` glyph - // strings off it): + // borderStyle has TWO valid prop forms (Ink types it `keyof Boxes | BoxStyle`): + // a preset-name STRING, or a custom BoxStyle OBJECT. We resolve the value to a + // BoxStyle exactly the way paint's drawBorder does — string → cliBoxes[name], + // object → use directly — then shape-check the RESULT. Both forms must produce + // a real BoxStyle so paint never reads a glyph off a malformed value. + // + // Why shape-check the RESOLVED box rather than `borderStyle in cliBoxes`: `in` + // has two false-accept holes that let a non-box value reach paint (which then + // reads `.top`/`.topLeft` glyph strings off it): // 1. cli-boxes' default export carries a CJS-interop `default` self-key, so // `"default" in cliBoxes` is true — but cliBoxes.default is the WHOLE // boxes object, not a BoxStyle (it has no string `top`). // 2. `in` walks the prototype chain, so Object.prototype members // ("toString", "constructor", "hasOwnProperty", …) report as "in // cliBoxes" while resolving to a function/undefined — never a BoxStyle. - // Resolving `cliBoxes[name]` and requiring an object with a string `top` - // rejects unknown names (undefined), "default" (whole object, no string - // `top`), and inherited props, while accepting every real preset. + // Resolving and requiring a real BoxStyle shape rejects unknown names + // (undefined), "default" (whole object), and inherited props. + // + // Why this also covers the OBJECT form: previously only the STRING form was + // shape-checked, so a malformed custom OBJECT (missing `top`, or a truthy + // non-string non-object like a number from a JS caller) bypassed validation, + // reached drawBorder, and threw `Cannot read properties of undefined (reading + // 'repeat')` in the post-flush PAINT pass — wedging Vue's scheduler. Resolving + // both forms and shape-checking here routes every invalid value to a clean + // render-time error instead. const borderStyle = props.borderStyle; - if (typeof borderStyle === "string" && borderStyle.length > 0) { - // Cast via `unknown` to add a string index signature: cliBoxes is typed as - // the `Boxes` keyof object (no index signature), so a direct cast is a TS2352 - // "insufficient overlap" error. Same cast paint.ts uses to look up by name. - const resolved = (cliBoxes as unknown as Record)[borderStyle]; - if (typeof resolved !== "object" || resolved === null || typeof resolved.top !== "string") { - throw new Error(`Unknown borderStyle: ${JSON.stringify(borderStyle)}`); + // Truthy gate: empty string / undefined / false ("no border") pass through + // unchanged, matching paint's `if (!style) return`. + if (borderStyle) { + if (typeof borderStyle === "string") { + // Cast via `unknown` to add a string index signature: cliBoxes is typed as + // the `Boxes` keyof object (no index signature), so a direct cast is a + // TS2352 "insufficient overlap" error. Same cast paint.ts uses by name. + const resolved = (cliBoxes as unknown as Record)[borderStyle]; + // Preserve the existing "Unknown borderStyle" wording for the string case. + if (!isValidBoxStyleShape(resolved)) { + throw new Error(`Unknown borderStyle: ${JSON.stringify(borderStyle)}`); + } + } else if (!isValidBoxStyleShape(borderStyle)) { + // Object form (or a truthy non-string non-object, e.g. a number from a JS + // caller): if it isn't a real BoxStyle, reject at render rather than let + // paint read a missing glyph and throw mid-commit. + throw new Error(`Invalid borderStyle: ${JSON.stringify(borderStyle)}`); } }