From 69fb623ef8b7445e8c35468614b1deab782669f4 Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Sat, 3 Oct 2026 12:32:24 -0400 Subject: [PATCH] Added a read-only guard that makes config writes throw in dev and CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no ref Deep-freezing config makes it immutable but not loud. Ghost's .js files and its CommonJS dependencies are sloppy-mode, where a write to a frozen object is dropped without an error, so a caller that mutates config fails silently and no test can see it. `node --use-strict` does not help: it makes only the entry point strict, and a `require()`d CommonJS module keeps its own strictness, with or without tsx. Enforcing the freeze that way would take 'use strict' directives across 1330 files, 6 of which have one today. A proxy trap throws whatever mode the caller is in. So `development` and anything starting with `test` - the environments this repo runs itself, the same set that validates strictly - now get a proxy whose set, delete, defineProperty and setPrototypeOf traps throw and name the key path. Production keeps the frozen object: it is the cheaper read, and the guard's value is in catching writes before they ship rather than after. GHOST_CONFIG_GUARD overrides either way. The two cannot be combined. A proxy over a deep-frozen target cannot return a wrapped child from its get trap, because the invariant for non-writable, non-configurable properties forbids handing back anything other than the target's own value, so a guarded tree is left unfrozen and relies on the traps. Children are wrapped lazily and memoised per underlying object, which keeps `===` stable between reads of the same subtree and terminates on cycles. Symbols and functions are handed back unwrapped, or iterators, spreads and array methods break. Only plain objects and arrays are wrapped. Reads cost more - 4.5ns to 21ns for a scalar, 120ns to 2.4µs for a spread of a config object - which is the other reason production keeps the freeze. It makes no measurable difference to the test suite: 8.50s against 9.04s over 9340 tests. One change outside the guard: config.set() now clones overrides with cloneDeep rather than structuredClone, because under the guard the value may be a proxy and structuredClone throws on those. The tests drive a deliberately sloppy-mode fixture, because a test file is no use here - vitest compiles those to ESM, which is always strict, so a write would throw with or without the guard. They stub GHOST_CONFIG_GUARD rather than inheriting it, so they do not depend on what a developer has exported. Verified against the unit, integration and e2e suites on both MySQL and SQLite, and a development boot. The guard found exactly one real violation while being built, fixed in the preceding commit: knex-migrator mutating the database config it was handed. --- ghost/core/core/shared/config/SCHEMA.md | 33 ++-- ghost/core/core/shared/config/guard.ts | 124 +++++++++++++++ ghost/core/core/shared/config/validated.ts | 47 ++++-- .../test/unit/shared/config/guard.test.ts | 149 ++++++++++++++++++ .../test/unit/shared/config/validated.test.ts | 46 +++++- .../utils/fixtures/sloppy-config-writer.js | 16 ++ 6 files changed, 386 insertions(+), 29 deletions(-) create mode 100644 ghost/core/core/shared/config/guard.ts create mode 100644 ghost/core/test/unit/shared/config/guard.test.ts create mode 100644 ghost/core/test/utils/fixtures/sloppy-config-writer.js diff --git a/ghost/core/core/shared/config/SCHEMA.md b/ghost/core/core/shared/config/SCHEMA.md index d5986f3dba9e..13049cc77805 100644 --- a/ghost/core/core/shared/config/SCHEMA.md +++ b/ghost/core/core/shared/config/SCHEMA.md @@ -83,14 +83,7 @@ failure for feature config, which is the wrong behaviour for most of it. the schema changed a value rather than only checking it. Adding a key to the schema enforces its type: a caller that treated a string as -a number, or assumed a key is always present, stops compiling. That is the point -— the compiler is what keeps config honest, because the freeze on its own cannot. -Ghost's `.js` files are sloppy-mode CommonJS, where writing to a frozen object is -dropped rather than thrown, and `node --use-strict` does not change that: it makes -only the entry point strict, while a `require()`d CommonJS module keeps its own -strictness. Enforcing the freeze at runtime would take `'use strict'` directives -across the `.js` files, or a Proxy whose `set` trap throws, gated to the test -environment. +a number, or assumed a key is always present, stops compiling. ## Rules @@ -128,7 +121,29 @@ change. ## Don't mutate what `get()` returns -It is frozen, and it is shared: every reader of a key gets the same object. Build +In `development` and under test a write throws, naming the key path: + +``` +TypeError: Ghost config is read-only: attempted write to `paths:contentPath`. +``` + +In production it does not. The config is deep-frozen there, and Ghost's `.js` +files and its CommonJS dependencies are sloppy-mode, where a write to a frozen +object is dropped without an error. `node --use-strict` does not change that: it +makes only the entry point strict, and a `require()`d CommonJS module keeps its +own strictness. + +So the loud failure is a development and CI affordance - [`guard.ts`](guard.ts) +swaps the freeze for a proxy whose traps throw, which fires whatever mode the +caller is in. Production keeps the frozen object because it is the cheaper read; +the two cannot be combined, since a proxy over a deep-frozen target may not +return a wrapped child. `GHOST_CONFIG_GUARD` overrides in either direction. + +Guarded reads cost more - a scalar read goes from 4.5ns to 21ns and a spread of a +config object from 120ns to 2.4µs - which is why it is not on in production. It +makes no measurable difference to the test suite. + +Config is shared as well as read-only: every reader of a key gets the same object. Build a derived object instead. Three places got this wrong, each writing through into config for every later reader: [`configure-knex.ts`](../../server/data/db/configure-knex.ts) assembling knex's diff --git a/ghost/core/core/shared/config/guard.ts b/ghost/core/core/shared/config/guard.ts new file mode 100644 index 000000000000..11c0a139e5d1 --- /dev/null +++ b/ghost/core/core/shared/config/guard.ts @@ -0,0 +1,124 @@ +/** + * A read-only guard for config, for the environments this repo runs itself. + * + * Deep-freezing config makes it immutable but not loud: Ghost's `.js` files and + * its CommonJS dependencies are sloppy-mode, where a write to a frozen object is + * dropped without an error. `node --use-strict` does not help - it makes only the + * entry point strict, and a `require()`d CommonJS module keeps its own + * strictness. A proxy trap throws whatever the caller's mode, so development and + * CI fail on a write that production would merely lose. + * + * Production keeps the frozen object: it is the cheaper read, and the guard's + * value is in finding writes before they ship. The two are mutually exclusive - + * a proxy over a deep-frozen target cannot return a wrapped child, because the + * invariant for non-writable, non-configurable properties forbids handing back + * anything other than the target's own value - so a guarded tree is left + * unfrozen and relies on the traps instead. + */ + +const wrappers = new WeakMap(); + +/** + * Whether writes to config should throw rather than be dropped. + * + * Matches the environments ./validated.ts validates strictly, for the same + * reason: a mistake should fail a developer's boot or CI, not a live site. + * `GHOST_CONFIG_GUARD` overrides in either direction. + */ +export function shouldGuard(env: string): boolean { + const override = process.env.GHOST_CONFIG_GUARD; + + if (override !== undefined) { + return override === 'true'; + } + + return env === 'development' || env.startsWith('test'); +} + +function isWrappable(value: unknown): value is object { + if (value === null || typeof value !== 'object') { + return false; + } + + // leave class instances and anything else exotic alone - config is plain data, + // and wrapping something with its own internal slots breaks it + const proto = Object.getPrototypeOf(value); + + return Array.isArray(value) || proto === Object.prototype || proto === null; +} + +/** + * Wrap a config tree so that any write throws, naming the key path. + * + * Children are wrapped lazily from the get trap, so only the parts actually read + * pay for it, and memoised per underlying object, which keeps `===` stable + * between reads of the same subtree and terminates on cycles. + */ +export function guardReadOnly(tree: T, path = ''): T { + if (!isWrappable(tree)) { + return tree; + } + + const existing = wrappers.get(tree); + + if (existing !== undefined) { + return existing as T; + } + + const refuse = (prop: string | symbol): never => { + // A native TypeError on purpose: it is what a strict-mode write to a frozen + // object throws, so guarded and frozen config fail the same way. Config also + // must not depend on @tryghost/error. + // eslint-disable-next-line ghost/ghost-custom/ghost-error-usage + throw new TypeError( + `Ghost config is read-only: attempted write to \`${path ? `${path}:` : ''}${String(prop)}\`. ` + + 'Build a derived object instead - see core/shared/config/SCHEMA.md.', + ); + }; + + const proxy = new Proxy(tree, { + get(target, prop) { + const value = Reflect.get(target, prop); + + // symbols carry iterators and inspection hooks, and functions are array + // and object methods - wrapping either breaks them + if (typeof prop === 'symbol' || typeof value === 'function') { + return value; + } + + return guardReadOnly(value, path ? `${path}:${String(prop)}` : String(prop)); + }, + // without this, Object.getOwnPropertyDescriptor(guarded, k).value hands back + // the raw child and a write through it misses every trap. Safe to wrap: + // a guarded tree is left unfrozen, so its properties stay configurable and + // returning a different value does not break a proxy invariant. + getOwnPropertyDescriptor(target, prop) { + const descriptor = Reflect.getOwnPropertyDescriptor(target, prop); + + if (!descriptor || !('value' in descriptor) || typeof prop === 'symbol') { + return descriptor; + } + + return { + ...descriptor, + value: guardReadOnly(descriptor.value, path ? `${path}:${String(prop)}` : String(prop)), + }; + }, + set(_target, prop) { + return refuse(prop); + }, + defineProperty(_target, prop) { + return refuse(prop); + }, + deleteProperty(_target, prop) { + return refuse(prop); + }, + setPrototypeOf() { + return refuse('[[Prototype]]'); + }, + }); + + wrappers.set(tree, proxy); + + return proxy as T; +} diff --git a/ghost/core/core/shared/config/validated.ts b/ghost/core/core/shared/config/validated.ts index b3f84c1a5b3e..e01e96965eaf 100644 --- a/ghost/core/core/shared/config/validated.ts +++ b/ghost/core/core/shared/config/validated.ts @@ -1,7 +1,9 @@ import type { ReadonlyDeep } from 'type-fest'; +import _ from 'lodash'; import { z } from 'zod'; import { bindAll as bindUrlHelpers, type BoundHelpers } from '@tryghost/config-url-helpers'; import { bindAll as bindHelpers, type ConfigHelpers } from './helpers'; +import { guardReadOnly, shouldGuard } from './guard'; import { configSchema, type ConfigAt, type ConfigPath, type ValidatedConfig } from './schema'; /** @@ -49,17 +51,28 @@ function isStrict(env: string): boolean { } /** - * Validate a config tree and deep-freeze it. + * Make a validated tree read-only: frozen in production, proxied in the + * environments this repo runs itself so a write throws rather than being + * dropped. See ./guard.ts. + */ +function readOnly(tree: Record): ValidatedConfig { + return ( + shouldGuard(String(tree.env)) ? guardReadOnly(tree) : deepFreeze(tree) + ) as ValidatedConfig; +} + +/** + * Validate a config tree and make it read-only. * - * Freezes in place, so the caller must hand over a tree nothing else holds - - * zod passes keys the schema does not name straight through by reference, so a - * shared tree would be frozen out from under its other owner. + * Takes ownership of the tree: it is frozen in place, and zod passes keys the + * schema does not name straight through by reference, so a shared tree would be + * frozen out from under its other owner. */ export function validateConfig(tree: Record): ValidatedConfig { const result = configSchema.safeParse(tree); if (result.success) { - return deepFreeze(result.data); + return readOnly(result.data as Record); } const report = z.prettifyError(result.error); @@ -75,7 +88,7 @@ export function validateConfig(tree: Record): ValidatedConfig { `Ghost config failed validation (not enforced in the ${tree.env} environment):\n${report}`, ); - return deepFreeze(tree) as ValidatedConfig; + return readOnly(tree); } /** @@ -134,12 +147,14 @@ function lookup(root: unknown, key: string): unknown { } /** - * Write a key path into a tree, copying each level on the way down if it is - * frozen. + * Write a key path into a tree, copying each level on the way down. * - * An override's value is often something a test read back out of config, which - * is frozen, so a later override targeting a path inside it would otherwise be - * writing into a frozen object. + * The copy is unconditional, because an override's value is written into every + * later tree by reference and may be an object a caller still holds - usually + * one a test read back out of config. Writing a deeper path in place would + * reach back through both. Copying only when a level is frozen looks equivalent + * but is not: under the guard nothing is frozen, so the same nested override + * would mutate what an earlier `get()` returned. */ function writePath(tree: Record, key: string, value: unknown): void { const segments = key.split(':'); @@ -151,7 +166,7 @@ function writePath(tree: Record, key: string, value: unknown): if (child === null || typeof child !== 'object') { node[segment] = {}; - } else if (Object.isFrozen(child)) { + } else { node[segment] = Array.isArray(child) ? [...child] : { ...child }; } @@ -203,12 +218,14 @@ export function createConfig(sources: Record): GhostConfig { set(key: string, value: unknown): void { // Cloned first, before anything is recorded: cloning can throw - on a - // value it cannot handle - and doing it here means that throw cannot - // leave `overrides` half-updated. + // value with a throwing getter, say - and doing it here means that throw + // cannot leave `overrides` half-updated. cloneDeep rather than + // structuredClone, because under the guard this value may be a proxy and + // structuredClone rejects those. // // The clone itself is needed because rebuild() makes whatever ends up in // the tree read-only, and this value is the caller's own object. - const cloned = structuredClone(value); + const cloned = _.cloneDeep(value); const previous = new Map(overrides); // deleted first so the key moves to the end: overrides replay in diff --git a/ghost/core/test/unit/shared/config/guard.test.ts b/ghost/core/test/unit/shared/config/guard.test.ts new file mode 100644 index 000000000000..b54464aa7326 --- /dev/null +++ b/ghost/core/test/unit/shared/config/guard.test.ts @@ -0,0 +1,149 @@ +import assert from 'node:assert/strict'; +import _ from 'lodash'; +import { guardReadOnly, shouldGuard } from '../../../../core/shared/config/guard'; + +// Stands in for Ghost's production .js files and its CommonJS dependencies. +// A test file is no use here: vitest compiles those to ESM, which is always +// strict, so a write would throw with or without the guard. +const sloppy = require('../../../utils/fixtures/sloppy-config-writer'); + +describe('Config read-only guard', function () { + afterEach(function () { + vi.unstubAllEnvs(); + }); + + it('uses a sloppy-mode fixture, or it would prove nothing', function () { + assert.ok(sloppy.isSloppy()); + + // the baseline the guard exists to fix: frozen is silent here + const frozen = Object.freeze({ a: 1 }); + sloppy.write(frozen, 'a', 2); + assert.equal(frozen.a, 1); + }); + + describe('shouldGuard', function () { + beforeEach(function () { + // the default behaviour is what is under test here, so do not inherit a + // GHOST_CONFIG_GUARD a developer happens to have exported + vi.stubEnv('GHOST_CONFIG_GUARD', undefined); + }); + + it('covers the environments this repo runs itself', function () { + assert.ok(shouldGuard('development')); + assert.ok(shouldGuard('testing')); + assert.ok(shouldGuard('testing-mysql')); + }); + + it('leaves production and anything unrecognised to the freeze', function () { + assert.ok(!shouldGuard('production')); + assert.ok(!shouldGuard('staging')); + assert.ok(!shouldGuard('whatever-an-embedder-picked')); + }); + + it('is overridable in either direction', function () { + vi.stubEnv('GHOST_CONFIG_GUARD', 'true'); + assert.ok(shouldGuard('production')); + + vi.stubEnv('GHOST_CONFIG_GUARD', 'false'); + assert.ok(!shouldGuard('testing')); + }); + }); + + describe('refusing writes', function () { + it('throws on a write from sloppy-mode code, naming the key path', function () { + const config = guardReadOnly({ paths: { contentPath: '/a' } }); + + assert.throws( + () => sloppy.write(config.paths, 'contentPath', '/b'), + /read-only: attempted write to `paths:contentPath`/, + ); + assert.throws(() => sloppy.write(config, 'paths', {}), /write to `paths`/); + }); + + it('throws on delete, defineProperty and setPrototypeOf', function () { + const config = guardReadOnly({ paths: { contentPath: '/a' } }); + + assert.throws(() => sloppy.remove(config.paths, 'contentPath'), /write to/); + assert.throws(() => Object.defineProperty(config, 'x', { value: 1 }), /write to `x`/); + assert.throws(() => Object.setPrototypeOf(config, null), /\[\[Prototype\]\]/); + }); + + it('refuses a write through a property descriptor', function () { + const config = guardReadOnly({ paths: { contentPath: '/a' } }); + + // without a getOwnPropertyDescriptor trap this hands back the raw child, + // and a write through it misses every other trap + const descriptor = Object.getOwnPropertyDescriptor(config, 'paths') as { + value: { contentPath: string }; + }; + + assert.throws(() => sloppy.write(descriptor.value, 'contentPath', '/b'), /write to/); + assert.equal(config.paths.contentPath, '/a'); + }); + + it('refuses writes into an array', function () { + const config = guardReadOnly({ apps: { internal: ['a'] } }); + + assert.throws(() => sloppy.write(config.apps.internal, 0, 'b'), /write to `apps:internal:0`/); + }); + }); + + describe('leaving reads alone', function () { + const source = { + url: 'http://localhost:2368', + paths: { contentPath: '/a', nested: { deep: true } }, + apps: { internal: ['private-blogging', 'subscribers'] }, + }; + + it('reads through at every depth', function () { + const config = guardReadOnly(_.cloneDeep(source)); + + assert.equal(config.url, 'http://localhost:2368'); + assert.equal(config.paths.contentPath, '/a'); + assert.equal(config.paths.nested.deep, true); + assert.equal(config.apps.internal[1], 'subscribers'); + }); + + it('keeps the operations callers actually use working', function () { + const config = guardReadOnly(_.cloneDeep(source)); + + assert.deepEqual(Object.keys(config).sort(), ['apps', 'paths', 'url']); + assert.ok('paths' in config); + assert.deepEqual({ ...config.paths }, source.paths); + assert.deepEqual(JSON.parse(JSON.stringify(config)), source); + assert.deepEqual(_.cloneDeep(config), source); + assert.deepEqual([...config.apps.internal], source.apps.internal); + assert.deepEqual( + config.apps.internal.map((name: string) => name.length), + [16, 11], + ); + assert.equal(config.apps.internal.length, 2); + }); + + it('hands back the same wrapper for the same subtree', function () { + const config = guardReadOnly(_.cloneDeep(source)); + + // callers compare config objects by identity; a fresh proxy per read + // would break that, and would not terminate on a cycle + assert.equal(config.paths, config.paths); + }); + + it('terminates on a cycle', function () { + const cyclic: Record = { name: 'root' }; + cyclic.self = cyclic; + + const config = guardReadOnly(cyclic) as { name: string; self: { self: { name: string } } }; + + assert.equal(config.self.self.name, 'root'); + }); + + it('leaves values it must not wrap untouched', function () { + const date = new Date(); + const config = guardReadOnly({ when: date, scalar: 1, nothing: null }); + + assert.equal(config.when, date); + assert.equal(config.scalar, 1); + assert.equal(config.nothing, null); + }); + }); +}); diff --git a/ghost/core/test/unit/shared/config/validated.test.ts b/ghost/core/test/unit/shared/config/validated.test.ts index eddba8a6efaf..ec205a459ddf 100644 --- a/ghost/core/test/unit/shared/config/validated.test.ts +++ b/ghost/core/test/unit/shared/config/validated.test.ts @@ -6,6 +6,8 @@ import { z } from 'zod'; import { configSchema } from '../../../../core/shared/config/schema'; import { createConfig, deepFreeze, validateConfig } from '../../../../core/shared/config/validated'; +const sloppy = require('../../../utils/fixtures/sloppy-config-writer'); + const configDir = path.join(__dirname, '../../../../core/shared/config'); function readJson(...parts: string[]): Record { @@ -78,7 +80,19 @@ describe('Validated Config', function () { vi.restoreAllMocks(); }); - it('returns a deep-frozen view', function () { + it('refuses writes, by throwing under the guard', function () { + // stubbed rather than inherited: the guard is on by default under test, + // but a developer may have GHOST_CONFIG_GUARD exported - see ./guard.test.ts + vi.stubEnv('GHOST_CONFIG_GUARD', 'true'); + + const validated = validateConfig({ ...minimal, paths: { contentPath: '/a' } }); + + assert.throws(() => sloppy.write(validated.paths, 'contentPath', '/b'), /read-only/); + }); + + it('deep-freezes instead where the guard is off, as production does', function () { + vi.stubEnv('GHOST_CONFIG_GUARD', 'false'); + const validated = validateConfig({ ...minimal, paths: { contentPath: '/a' } }); assert.ok(Object.isFrozen(validated)); @@ -124,6 +138,10 @@ describe('Validated Config', function () { }); describe('createConfig', function () { + afterEach(function () { + vi.unstubAllEnvs(); + }); + it('reads key paths out of the frozen tree', function () { const config = createConfig({ ...minimal, paths: { contentPath: '/a' } }); @@ -132,13 +150,15 @@ describe('Validated Config', function () { assert.deepEqual(config.get('paths'), { contentPath: '/a' }); }); - it('freezes every key, not only the ones with a schema', function () { + it('makes every key read-only, not only the ones with a schema', function () { + vi.stubEnv('GHOST_CONFIG_GUARD', 'true'); + const config = createConfig({ ...minimal, paths: { contentPath: '/a' } }); - // the schema is loose, so an unlisted key is validated-and-frozen too - + // the schema is loose, so an unlisted key is validated and protected too - // that is what makes the whole config immutable - assert.ok(Object.isFrozen(config.get('paths'))); - assert.ok(Object.isFrozen(config.get())); + assert.throws(() => sloppy.write(config.get('paths'), 'contentPath', '/b'), /read-only/); + assert.throws(() => sloppy.write(config.get(), 'url', 'http://nope.test'), /read-only/); }); it('returns undefined for a path that is not there', function () { @@ -262,6 +282,22 @@ describe('Validated Config', function () { assert.equal(config.get('tinybird:tracker:endpoint'), '/e'); }); + // guarded and unguarded config must answer identically: the guard exists to + // make a write loud, not to change what config does + for (const guard of ['true', 'false']) { + it(`leaves an already-returned object alone on a nested set, guard=${guard}`, function () { + vi.stubEnv('GHOST_CONFIG_GUARD', guard); + const config = createConfig(minimal); + + config.set('paths', { contentPath: '/first' }); + const previous = config.get('paths'); + config.set('paths:contentPath', '/second'); + + assert.equal(previous.contentPath, '/first'); + assert.equal(config.get('paths:contentPath'), '/second'); + }); + } + it('drops every override on reset', function () { const config = createConfig(minimal); config.set('url', 'http://elsewhere.test'); diff --git a/ghost/core/test/utils/fixtures/sloppy-config-writer.js b/ghost/core/test/utils/fixtures/sloppy-config-writer.js new file mode 100644 index 000000000000..a087f91cc28d --- /dev/null +++ b/ghost/core/test/utils/fixtures/sloppy-config-writer.js @@ -0,0 +1,16 @@ +// Deliberately no 'use strict': this stands in for Ghost's production .js files +// and its CommonJS dependencies, where a write to a frozen object is dropped +// without an error. The read-only guard has to catch those. +module.exports.isSloppy = function isSloppy() { + return (function () { + return this; + })() !== undefined; +}; + +module.exports.write = function write(target, key, value) { + target[key] = value; +}; + +module.exports.remove = function remove(target, key) { + delete target[key]; +};