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]; +};