From 09db6b2bff634abd287b69b0f5f37327218e48e6 Mon Sep 17 00:00:00 2001 From: Aldrich_CC <109075336+Chen17-sq@users.noreply.github.com> Date: Sat, 16 May 2026 04:47:48 +0800 Subject: [PATCH] refactor(utils): consolidate isPlainObject into shared type-guards module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two functionally identical ``isPlainObject`` helpers existed: * ``src/utils/frontmatter.ts:6-10`` — private module-level, predates the prototype-pollution hardening work. * ``src/features/mcp/codexcli-mcp.ts:24-28`` — added by #1630 to harden the codexcli MCP converters; delegates to ``isRecord``. Both check ``Object.getPrototypeOf(value) === Object.prototype || === null`` after excluding arrays/null. Per the suggestion in #1638, hoist the canonical version into ``src/utils/type-guards.ts`` alongside the existing ``isRecord`` and import from there. The hoisted version uses the codexcli-mcp form (delegates to ``isRecord``) because it's slightly cleaner and already proven on the prototype-pollution path. Both callers (``frontmatter.ts`` for YAML deep-clean, ``codexcli-mcp.ts`` for MCP config conversion) get the same behaviour via the shared import. Closes #1638. Test plan --------- - ``pnpm run build`` (tsup + DTS): clean. - ``npx vitest run src/utils/frontmatter.test.ts src/features/mcp/codexcli-mcp.test.ts``: 2 files / 89 tests pass — both modules exercise the helper on realistic inputs (object literals, arrays, null, class instances, ``Object.create(null)`` bags, prototype-pollution payloads). - No behaviour change: the hoisted helper is byte-identical to the codexcli-mcp definition, and the frontmatter definition was functionally equivalent (array-rejection was implicit via the prototype check; now explicit via ``isRecord``). --- src/features/mcp/codexcli-mcp.ts | 8 +------- src/utils/frontmatter.ts | 7 +------ src/utils/type-guards.ts | 17 +++++++++++++++++ 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/features/mcp/codexcli-mcp.ts b/src/features/mcp/codexcli-mcp.ts index e034e9c9c..65b4328d0 100644 --- a/src/features/mcp/codexcli-mcp.ts +++ b/src/features/mcp/codexcli-mcp.ts @@ -6,7 +6,7 @@ import { ValidationResult } from "../../types/ai-file.js"; import { McpServers } from "../../types/mcp.js"; import { readFileContentOrNull, readOrInitializeFileContent } from "../../utils/file.js"; import { warnWithFallback } from "../../utils/logger.js"; -import { isRecord } from "../../utils/type-guards.js"; +import { isPlainObject, isRecord } from "../../utils/type-guards.js"; import { RulesyncMcp } from "./rulesync-mcp.js"; import { ToolMcp, @@ -21,12 +21,6 @@ const MAX_REMOVE_EMPTY_ENTRIES_DEPTH = 32; const PROTOTYPE_POLLUTION_KEYS = new Set(["__proto__", "constructor", "prototype"]); -function isPlainObject(value: unknown): value is Record { - if (!isRecord(value)) return false; - const proto = Object.getPrototypeOf(value); - return proto === null || proto === Object.prototype; -} - function convertFromCodexFormat(codexMcp: Record): McpServers { const result: McpServers = {}; diff --git a/src/utils/frontmatter.ts b/src/utils/frontmatter.ts index e8ed0949a..2b8441e96 100644 --- a/src/utils/frontmatter.ts +++ b/src/utils/frontmatter.ts @@ -2,12 +2,7 @@ import matter from "gray-matter"; import { dump, load } from "js-yaml"; import { formatError } from "./error.js"; - -function isPlainObject(value: unknown): value is Record { - if (value === null || typeof value !== "object") return false; - const prototype = Object.getPrototypeOf(value); - return prototype === Object.prototype || prototype === null; -} +import { isPlainObject } from "./type-guards.js"; function deepRemoveNullishValue(value: unknown): unknown { if (value === null || value === undefined) { diff --git a/src/utils/type-guards.ts b/src/utils/type-guards.ts index b87d8a42a..ab8ea94b7 100644 --- a/src/utils/type-guards.ts +++ b/src/utils/type-guards.ts @@ -5,3 +5,20 @@ export function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } + +/** + * Stricter sibling of {@link isRecord}: narrows to *plain* objects whose + * prototype is either ``Object.prototype`` or ``null`` — i.e. object + * literals, ``Object.create(null)`` bags, and the output of ``JSON.parse``. + * + * Rejects class instances even though they pass ``isRecord``. This is the + * check needed for prototype-pollution hardening: anything walking + * arbitrary user-supplied keys (frontmatter parsing, MCP config + * conversion, etc.) should reject inputs whose prototype could carry + * malicious accessor descriptors. + */ +export function isPlainObject(value: unknown): value is Record { + if (!isRecord(value)) return false; + const proto = Object.getPrototypeOf(value); + return proto === null || proto === Object.prototype; +}