From 5d1ce879ae0e01824ae25c243770d8418b9efef6 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 31 Aug 2026 11:35:38 +0200 Subject: [PATCH 1/4] feat(deps)!: upgrade js-yaml to v5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit js-yaml v5 removes the ESM default export, so `import YAML from 'js-yaml'` now throws at module-load time. Both call sites switch to a namespace import, which keeps the `YAML.load(...)` call sites unchanged: - lib/services/express.js (OpenAPI spec parsing at boot) - modules/tasks/tests/tasks.openapi-operationid.unit.tests.js v5 also switches load() from DEFAULT_SCHEMA to CORE_SCHEMA, dropping merge keys, implicit timestamps, !!binary, !!omap, !!pairs and !!set. The stack's own YAML specs use none of these, so nothing changes stack-side — verified by the operationId test, which parses modules/tasks/doc/tasks.yml for real rather than through a mock. BREAKING CHANGE: downstream projects using `import yaml from 'js-yaml'` must switch to a namespace or named import, and any project YAML relying on merge keys, unquoted dates or !! tags changes meaning under CORE_SCHEMA. See MIGRATIONS.md for the full downstream checklist. --- MIGRATIONS.md | 29 ++++++++++++++++ lib/services/express.js | 2 +- .../tasks.openapi-operationid.unit.tests.js | 2 +- package-lock.json | 33 ++++++++++++++++--- package.json | 2 +- 5 files changed, 60 insertions(+), 8 deletions(-) diff --git a/MIGRATIONS.md b/MIGRATIONS.md index 124dfcf0d..c62410854 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -4,6 +4,35 @@ Breaking changes and upgrade notes for downstream projects. --- +## js-yaml upgraded to v5 — no default export, and `load()` now uses CORE_SCHEMA (2026-08-31) + +`js-yaml` moves from `^4.3.2` to `^5`. Two things change for downstream projects. + +**1. There is no ESM default export any more.** `import YAML from 'js-yaml'` throws +`SyntaxError: The requested module 'js-yaml' does not provide an export named 'default'` +at module-load time — which breaks application boot, not just tests. The stack's own +call sites now use a namespace import (`import * as YAML from 'js-yaml'`), which keeps +`YAML.load(...)` call sites unchanged. + +**2. `load()` defaults to `CORE_SCHEMA` (YAML 1.2) instead of `DEFAULT_SCHEMA` (YAML 1.1).** +Dropped from the default schema: merge keys (`<<:`), implicit timestamps, `!!binary`, +`!!omap`, `!!pairs`, `!!set`. A bare `2024-01-01` now loads as a string rather than a +`Date`, and a `<<:` merge key raises instead of merging. `load('')` also throws now +instead of returning `undefined`. + +**Action required:** + +- Grep your project for `from 'js-yaml'` and switch any default import to + `import * as yaml from 'js-yaml'` (or named: `import { load } from 'js-yaml'`). +- Grep your own OpenAPI/config YAML for `<<:`, `!!`, and unquoted `YYYY-MM-DD` values. + Any hit changes meaning under v5 — quote the dates, and expand merge keys by hand. + Alternatively pass `{ schema: DEFAULT_SCHEMA }` to `load()` to keep the old behaviour. +- If any YAML file you load can legitimately be empty, guard it: `load('')` throws in v5. + +The stack's own specs use none of these features, so no stack-side behaviour changed. + +--- + ## Unmatched routes return the same content-negotiated 404 on every HTTP verb (2026-08-03, closes gap from #3975) An earlier change (#3975) replaced the implicit 200 previously returned for any unmatched **GET** with a content-negotiated 404 (JSON `{ error: 'not_found' }` for API paths/JSON-accepting clients, minimal HTML otherwise) — but the catch-all was registered with `app.get` only. Unmatched POST/PUT/PATCH/DELETE were unaffected by that change and already 404'd via Express's own default finalhandler, just as unstructured HTML (`Cannot POST /...`) rather than the negotiated shape. The catch-all is now registered with `app.all`, so every unmatched verb gets the same negotiated 404 body/content-type as GET. `OPTIONS` is unaffected: the `cors` middleware answers preflight requests before this route is ever reached. diff --git a/lib/services/express.js b/lib/services/express.js index e866331b8..58aa93691 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -14,7 +14,7 @@ import lusca from 'lusca'; import cors from 'cors'; import morgan from 'morgan'; import fs from 'fs'; -import YAML from 'js-yaml'; +import * as YAML from 'js-yaml'; import config from '../../config/index.js'; import configHelper from '../helpers/config.js'; diff --git a/modules/tasks/tests/tasks.openapi-operationid.unit.tests.js b/modules/tasks/tests/tasks.openapi-operationid.unit.tests.js index 1d5a766e6..4c6fbbc56 100644 --- a/modules/tasks/tests/tasks.openapi-operationid.unit.tests.js +++ b/modules/tasks/tests/tasks.openapi-operationid.unit.tests.js @@ -1,5 +1,5 @@ import { describe, test, expect } from '@jest/globals'; -import yaml from 'js-yaml'; +import * as yaml from 'js-yaml'; import fs from 'fs'; import path from 'path'; import { fileURLToPath } from 'url'; diff --git a/package-lock.json b/package-lock.json index d3245552b..aa0da60d3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,7 +25,7 @@ "handlebars": "^4.7.9", "helmet": "~8.2.0", "inquirer": "^14.0.2", - "js-yaml": "^4.3.2", + "js-yaml": "^5.4.1", "jsonwebtoken": "^9.0.3", "lodash": "^4.18.1", "lusca": "^1.7.0", @@ -7549,6 +7549,29 @@ "typescript": ">=5" } }, + "node_modules/cosmiconfig/node_modules/js-yaml": { + "version": "4.3.2", + "resolved": "https://registry.npmjs.org/js-yaml/-/js-yaml-4.3.2.tgz", + "integrity": "sha512-SFNOvSJ+Dgf/9An904Yx+CgSlIPCkIpao4qo51lpee25TIRejdH3rhR4EZMGoNx3/TP3O+wzWuiTFl4sqbltzA==", + "dev": true, + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/puzrin" + }, + { + "type": "github", + "url": "https://github.com/sponsors/nodeca" + } + ], + "license": "MIT", + "dependencies": { + "argparse": "^2.0.1" + }, + "bin": { + "js-yaml": "bin/js-yaml.js" + } + }, "node_modules/cross-env": { "version": "10.1.0", "resolved": "https://registry.npmjs.org/cross-env/-/cross-env-10.1.0.tgz", @@ -13575,9 +13598,9 @@ "license": "MIT" }, "node_modules/js-yaml": { - "version": "4.3.2", - "resolved": "https://registry.npmjs.org/js-yaml/-/js-yaml-4.3.2.tgz", - "integrity": "sha512-SFNOvSJ+Dgf/9An904Yx+CgSlIPCkIpao4qo51lpee25TIRejdH3rhR4EZMGoNx3/TP3O+wzWuiTFl4sqbltzA==", + "version": "5.4.1", + "resolved": "https://registry.npmjs.org/js-yaml/-/js-yaml-5.4.1.tgz", + "integrity": "sha512-28R/k+NAjeuf7+CKlTxWZVExJGwVVLwY06DgEnOMz2gEpfNkDcD7QvyiVPT0xy0XXhU8vHsd4Ot42OOPdJG7dQ==", "funding": [ { "type": "github", @@ -13593,7 +13616,7 @@ "argparse": "^2.0.1" }, "bin": { - "js-yaml": "bin/js-yaml.js" + "js-yaml": "bin/js-yaml.mjs" } }, "node_modules/jsdom": { diff --git a/package.json b/package.json index 09f9b7004..b373488d8 100644 --- a/package.json +++ b/package.json @@ -65,7 +65,7 @@ "handlebars": "^4.7.9", "helmet": "~8.2.0", "inquirer": "^14.0.2", - "js-yaml": "^4.3.2", + "js-yaml": "^5.4.1", "jsonwebtoken": "^9.0.3", "lodash": "^4.18.1", "lusca": "^1.7.0", From 76f16981df69342e7eac944b0a5d7a4647fc7b65 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 31 Aug 2026 11:43:40 +0200 Subject: [PATCH 2/4] fix(openapi): keep an empty spec file a skipped warning under js-yaml v5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review gate caught two things the v5 migration missed. 1. `load('')` throws in v5 where v4 returned undefined, so an empty (or whitespace-only) OpenAPI file went from 'skip with a warning' to a rethrown error — i.e. a boot failure for any downstream project with an empty spec. express.js now checks the raw content before calling load(), which routes the empty case back through the existing not-a-plain-object guard. Locked by a test that fails exactly the way boot would if the guard is removed. 2. MIGRATIONS.md pointed downstream at `{ schema: DEFAULT_SCHEMA }` to restore YAML 1.1 behaviour, but v5 removed that export. The replacement is YAML11_SCHEMA. Verified by executing the package, not by reading docs. Also drops the now-dead `default:` keys from the three js-yaml jest mocks: v5 has no default export, so a mock that supplies one would let a reintroduced default import pass tests and still fail at boot. --- MIGRATIONS.md | 21 ++++++++++----- lib/services/express.js | 5 +++- lib/services/tests/express.docs.unit.tests.js | 27 +++++++++++++++++-- .../tests/express.docsEnvGate.unit.tests.js | 1 - 4 files changed, 43 insertions(+), 11 deletions(-) diff --git a/MIGRATIONS.md b/MIGRATIONS.md index c62410854..684e605ce 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -14,11 +14,14 @@ at module-load time — which breaks application boot, not just tests. The stack call sites now use a namespace import (`import * as YAML from 'js-yaml'`), which keeps `YAML.load(...)` call sites unchanged. -**2. `load()` defaults to `CORE_SCHEMA` (YAML 1.2) instead of `DEFAULT_SCHEMA` (YAML 1.1).** +**2. `load()` defaults to `CORE_SCHEMA` (YAML 1.2) instead of the old YAML 1.1 default.** Dropped from the default schema: merge keys (`<<:`), implicit timestamps, `!!binary`, `!!omap`, `!!pairs`, `!!set`. A bare `2024-01-01` now loads as a string rather than a -`Date`, and a `<<:` merge key raises instead of merging. `load('')` also throws now -instead of returning `undefined`. +`Date`, and a `<<:` merge key raises instead of merging. Where `!!set` still is loaded +explicitly it now produces a native `Set` rather than an object of nulls. + +**3. `load('')` throws.** Empty or whitespace-only input raises +`expected a document, but the input is empty` where v4 returned `undefined`. **Action required:** @@ -26,10 +29,14 @@ instead of returning `undefined`. `import * as yaml from 'js-yaml'` (or named: `import { load } from 'js-yaml'`). - Grep your own OpenAPI/config YAML for `<<:`, `!!`, and unquoted `YYYY-MM-DD` values. Any hit changes meaning under v5 — quote the dates, and expand merge keys by hand. - Alternatively pass `{ schema: DEFAULT_SCHEMA }` to `load()` to keep the old behaviour. -- If any YAML file you load can legitimately be empty, guard it: `load('')` throws in v5. - -The stack's own specs use none of these features, so no stack-side behaviour changed. + To keep the old YAML 1.1 behaviour instead, pass `{ schema: YAML11_SCHEMA }` to `load()`. + Note `DEFAULT_SCHEMA` no longer exists in v5; `YAML11_SCHEMA` is its replacement. +- If any YAML file you load can legitimately be empty, guard it before calling `load()` + (`raw.trim() ? load(raw) : null`) — otherwise an empty file turns into a thrown error. + +The stack's own specs use none of the dropped YAML 1.1 features, so no stack-side parsing +behaviour changed. `lib/services/express.js` does guard the empty-file case, so an empty +OpenAPI spec stays a skipped-with-warning file rather than a boot failure. --- diff --git a/lib/services/express.js b/lib/services/express.js index 58aa93691..425bd0012 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -63,7 +63,10 @@ const initApiSpec = (app) => { const contents = config.files.openapi .map((filePath) => { try { - const doc = YAML.load(fs.readFileSync(filePath).toString()); + // js-yaml v5 throws on an empty document where v4 returned undefined. An empty + // spec file must stay a skippable warning, not a boot failure. + const raw = fs.readFileSync(filePath).toString(); + const doc = raw.trim() ? YAML.load(raw) : null; if (!doc || typeof doc !== 'object' || Array.isArray(doc)) { logger.warn(`[openapi] skipping ${filePath}: YAML did not parse to a plain object`); return null; diff --git a/lib/services/tests/express.docs.unit.tests.js b/lib/services/tests/express.docs.unit.tests.js index ad3dc62d1..d5e90c6ee 100644 --- a/lib/services/tests/express.docs.unit.tests.js +++ b/lib/services/tests/express.docs.unit.tests.js @@ -87,7 +87,6 @@ describe('express initApiSpec — core/doc/index.yml OpenAPI baseline (T13 Fix B default: { isProd: jest.fn().mockReturnValue(false), isDevEnv: jest.fn().mockReturnValue(true) }, })); jest.unstable_mockModule('js-yaml', () => ({ - default: { load: jest.fn().mockReturnValue(mockCoreYamlDoc) }, load: jest.fn().mockReturnValue(mockCoreYamlDoc), })); jest.unstable_mockModule('../../helpers/guides.js', () => ({ @@ -229,7 +228,6 @@ describe('express initApiSpec — OpenAPI JSON spec endpoint:', () => { default: { isProd: jest.fn().mockReturnValue(false), isDevEnv: jest.fn().mockReturnValue(true) }, })); jest.unstable_mockModule('js-yaml', () => ({ - default: { load: jest.fn().mockReturnValue(mockYamlDoc) }, load: jest.fn().mockReturnValue(mockYamlDoc), })); @@ -260,6 +258,31 @@ describe('express initApiSpec — OpenAPI JSON spec endpoint:', () => { expect(spec.servers[0].url).toBe('https://example.com'); }); + test('an empty spec file is skipped with a warning, never a boot failure', async () => { + // js-yaml v5 throws on an empty document (v4 returned undefined), so express.js + // guards on the raw content before calling load(). Drop that guard and this test + // fails the way boot would: load() is reached and throws. + const load = jest.fn(() => { + throw new Error('expected a document, but the input is empty'); + }); + jest.unstable_mockModule('fs', () => ({ + default: { readFileSync: jest.fn().mockReturnValue(' \n') }, + readFileSync: jest.fn().mockReturnValue(' \n'), + })); + jest.unstable_mockModule('js-yaml', () => ({ load })); + jest.unstable_mockModule('../../helpers/guides.js', () => ({ + default: { loadGuides: jest.fn().mockReturnValue([]), mergeGuidesIntoSpec: jest.fn() }, + })); + jest.unstable_mockModule('../../../config/index.js', () => ({ default: baseConfig })); + + const { default: expressService } = await import('../express.js'); + const app = buildMockApp(); + + expect(() => expressService.initApiSpec(app)).not.toThrow(); + expect(load).not.toHaveBeenCalled(); + expect(app._routes['/api/spec.json']).toBeUndefined(); + }); + test('merges markdown guides into info.description', async () => { // The real guides helper runs so guide markdown lands in info.description — // this is the documentation surface now that the Redoc UI is gone. diff --git a/lib/services/tests/express.docsEnvGate.unit.tests.js b/lib/services/tests/express.docsEnvGate.unit.tests.js index 4aafd88d5..827b1e48d 100644 --- a/lib/services/tests/express.docsEnvGate.unit.tests.js +++ b/lib/services/tests/express.docsEnvGate.unit.tests.js @@ -46,7 +46,6 @@ describe('express initApiSpec — env gate (spec off in non-dev envs):', () => { readFileSync: jest.fn().mockReturnValue('mocked'), })); jest.unstable_mockModule('js-yaml', () => ({ - default: { load: jest.fn().mockReturnValue(mockYamlDoc) }, load: jest.fn().mockReturnValue(mockYamlDoc), })); jest.unstable_mockModule('../../helpers/guides.js', () => ({ From 513d548964a31f6412809bf0dd81b6f2bd37e71d Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 31 Aug 2026 12:09:47 +0200 Subject: [PATCH 3/4] fix(openapi): cover comment-only spec files and correct the v5 migration notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review (CodeRabbit was rate-limited on this public repo) found the previous commit's guard incomplete and three factual errors in MIGRATIONS.md. Every claim below was checked by executing js-yaml 5.4.1. The guard: `raw.trim()` is truthy for a file containing only a comment, but v5 raises 'expected a document, but the input is empty' for that too — as it does for a BOM-only file. Since config.files.openapi globs modules/*/doc/*.yml, a downstream module shipping a stubbed or commented-out spec still bricked boot. express.js now lets load() run and discriminates on err.reason, so all four content-free shapes skip with a warning while real parse errors still propagate. The tests: the mocked case is replaced by express.docsEmptySpec.unit.tests.js, which runs the REAL parser over zero-byte, whitespace-only, comment-only and BOM-only input, asserts the warning is emitted, and adds two controls — a malformed document must still fail loudly, a valid one must still be served. A mock could only ever throw for the inputs its author imagined, which is exactly how the comment-only case slipped through. Mutation-checked: removing the guard fails all four cases. MIGRATIONS.md corrections: - a '<<:' merge key does NOT raise under CORE_SCHEMA, it is silently kept as a literal '<<' key. Saying it raises would send a downstream maintainer looking for a failure that never comes. - YAML11_SCHEMA is not a drop-in for v4's DEFAULT_SCHEMA. It also restores YAML 1.1 scalar rules v4 did not apply (12:30 -> 750, 014 -> 12, 0o14 -> '0o14', key y -> true). Documented as a last resort with the divergences listed. - '!!set' does not load as a native Set under the default schema, it throws unknown tag; the Set only appears under YAML11_SCHEMA, and serialises to {}. - the content-free trigger set and the recommended guard now match the code. --- MIGRATIONS.md | 65 ++++++++--- lib/services/express.js | 13 ++- lib/services/tests/express.docs.unit.tests.js | 25 ----- .../tests/express.docsEmptySpec.unit.tests.js | 105 ++++++++++++++++++ 4 files changed, 165 insertions(+), 43 deletions(-) create mode 100644 lib/services/tests/express.docsEmptySpec.unit.tests.js diff --git a/MIGRATIONS.md b/MIGRATIONS.md index 684e605ce..7f310f26b 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -6,7 +6,8 @@ Breaking changes and upgrade notes for downstream projects. ## js-yaml upgraded to v5 — no default export, and `load()` now uses CORE_SCHEMA (2026-08-31) -`js-yaml` moves from `^4.3.2` to `^5`. Two things change for downstream projects. +`js-yaml` moves from `^4.3.2` to `^5`. Three things change for downstream projects. +All behaviours below were checked by executing `js-yaml@5.4.1`, not read from its docs. **1. There is no ESM default export any more.** `import YAML from 'js-yaml'` throws `SyntaxError: The requested module 'js-yaml' does not provide an export named 'default'` @@ -15,28 +16,62 @@ call sites now use a namespace import (`import * as YAML from 'js-yaml'`), which `YAML.load(...)` call sites unchanged. **2. `load()` defaults to `CORE_SCHEMA` (YAML 1.2) instead of the old YAML 1.1 default.** -Dropped from the default schema: merge keys (`<<:`), implicit timestamps, `!!binary`, -`!!omap`, `!!pairs`, `!!set`. A bare `2024-01-01` now loads as a string rather than a -`Date`, and a `<<:` merge key raises instead of merging. Where `!!set` still is loaded -explicitly it now produces a native `Set` rather than an object of nulls. +Merge keys, implicit timestamps, `!!binary`, `!!omap`, `!!pairs` and `!!set` are no longer +in the default schema. Two of these fail quietly, so read carefully: -**3. `load('')` throws.** Empty or whitespace-only input raises -`expected a document, but the input is empty` where v4 returned `undefined`. +| Input | v4 default | v5 `CORE_SCHEMA` | +|---|---|---| +| `<<: *anchor` | merged into the mapping | **silently kept as a literal `"<<"` key** — no error | +| `d: 2024-01-01` | `Date` object | string `"2024-01-01"` | +| `!!set`, `!!binary`, `!!omap`, `!!pairs` | loaded | throws `unknown … tag` | + +The merge-key row is the dangerous one: nothing fails, you just get a spec with a `<<` +key in it. Grep for it; do not expect CI or boot to catch it. + +**3. `load()` throws on a document with no content.** Empty, whitespace-only, +**comment-only** and BOM-only input all raise `expected a document, but the input is +empty`, where v4 returned `undefined`. A file containing only `---` still returns `null`. **Action required:** - Grep your project for `from 'js-yaml'` and switch any default import to `import * as yaml from 'js-yaml'` (or named: `import { load } from 'js-yaml'`). -- Grep your own OpenAPI/config YAML for `<<:`, `!!`, and unquoted `YYYY-MM-DD` values. - Any hit changes meaning under v5 — quote the dates, and expand merge keys by hand. - To keep the old YAML 1.1 behaviour instead, pass `{ schema: YAML11_SCHEMA }` to `load()`. - Note `DEFAULT_SCHEMA` no longer exists in v5; `YAML11_SCHEMA` is its replacement. -- If any YAML file you load can legitimately be empty, guard it before calling `load()` - (`raw.trim() ? load(raw) : null`) — otherwise an empty file turns into a thrown error. +- Grep your own OpenAPI/config YAML for `<<:`, `!!` tags and unquoted `YYYY-MM-DD` + values, and fix each per the table above. +- Check every globbed spec/config directory for files that are empty or contain only + comments. This is the one change that is a hard crash rather than a value change. +- If any YAML file you load can legitimately be content-free, do not pre-test the string + (`raw.trim()` is truthy for a comment-only file, so it does not cover this). Let `load()` + run and discriminate on the error, which keeps real parse errors propagating: + + ```js + let doc = null; + try { + doc = load(raw); + } catch (err) { + if (err?.reason !== 'expected a document, but the input is empty') throw err; + } + ``` + +**On `YAML11_SCHEMA` — it is not a drop-in for v4.** `DEFAULT_SCHEMA` was removed; the +nearest analogue is `YAML11_SCHEMA`, but passing it is a last resort, not an equivalence. +It restores merge keys and the `!!` tags, and also brings back YAML 1.1 scalar rules that +v4's default did **not** apply: + +| Input | v4 default | v5 `YAML11_SCHEMA` | +|---|---|---| +| `v: 12:30` | `"12:30"` | `750` (sexagesimal) | +| `old: 014` | `14` | `12` (legacy octal) | +| `o: 0o14` | `12` | `"0o14"` (string) | +| `y: 2` | key `"y"` | key `true` — and a document with both `y:` and `yes:` now throws `duplicated mapping key` | + +`!!set` under `YAML11_SCHEMA` also yields a native `Set`, which serialises to `{}` through +`res.json` — worth knowing if a loaded document is served as JSON. The stack's own specs use none of the dropped YAML 1.1 features, so no stack-side parsing -behaviour changed. `lib/services/express.js` does guard the empty-file case, so an empty -OpenAPI spec stays a skipped-with-warning file rather than a boot failure. +behaviour changed. `lib/services/express.js` handles the content-free case with the +snippet above, so an empty or comment-only OpenAPI spec stays a skipped-with-warning file +rather than a boot failure. --- diff --git a/lib/services/express.js b/lib/services/express.js index 425bd0012..5ed9a5d40 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -63,10 +63,17 @@ const initApiSpec = (app) => { const contents = config.files.openapi .map((filePath) => { try { - // js-yaml v5 throws on an empty document where v4 returned undefined. An empty - // spec file must stay a skippable warning, not a boot failure. const raw = fs.readFileSync(filePath).toString(); - const doc = raw.trim() ? YAML.load(raw) : null; + let doc = null; + try { + doc = YAML.load(raw); + } catch (err) { + // js-yaml v5 raises on a document with no content — empty, whitespace-only, + // comment-only or BOM-only — where v4 returned undefined. Those must stay a + // skippable warning, not a boot failure. Discriminate on the reason so real + // parse errors ('deficient indentation' etc.) still propagate. + if (err?.reason !== 'expected a document, but the input is empty') throw err; + } if (!doc || typeof doc !== 'object' || Array.isArray(doc)) { logger.warn(`[openapi] skipping ${filePath}: YAML did not parse to a plain object`); return null; diff --git a/lib/services/tests/express.docs.unit.tests.js b/lib/services/tests/express.docs.unit.tests.js index d5e90c6ee..705af42c4 100644 --- a/lib/services/tests/express.docs.unit.tests.js +++ b/lib/services/tests/express.docs.unit.tests.js @@ -258,31 +258,6 @@ describe('express initApiSpec — OpenAPI JSON spec endpoint:', () => { expect(spec.servers[0].url).toBe('https://example.com'); }); - test('an empty spec file is skipped with a warning, never a boot failure', async () => { - // js-yaml v5 throws on an empty document (v4 returned undefined), so express.js - // guards on the raw content before calling load(). Drop that guard and this test - // fails the way boot would: load() is reached and throws. - const load = jest.fn(() => { - throw new Error('expected a document, but the input is empty'); - }); - jest.unstable_mockModule('fs', () => ({ - default: { readFileSync: jest.fn().mockReturnValue(' \n') }, - readFileSync: jest.fn().mockReturnValue(' \n'), - })); - jest.unstable_mockModule('js-yaml', () => ({ load })); - jest.unstable_mockModule('../../helpers/guides.js', () => ({ - default: { loadGuides: jest.fn().mockReturnValue([]), mergeGuidesIntoSpec: jest.fn() }, - })); - jest.unstable_mockModule('../../../config/index.js', () => ({ default: baseConfig })); - - const { default: expressService } = await import('../express.js'); - const app = buildMockApp(); - - expect(() => expressService.initApiSpec(app)).not.toThrow(); - expect(load).not.toHaveBeenCalled(); - expect(app._routes['/api/spec.json']).toBeUndefined(); - }); - test('merges markdown guides into info.description', async () => { // The real guides helper runs so guide markdown lands in info.description — // this is the documentation surface now that the Redoc UI is gone. diff --git a/lib/services/tests/express.docsEmptySpec.unit.tests.js b/lib/services/tests/express.docsEmptySpec.unit.tests.js new file mode 100644 index 000000000..9bd6df215 --- /dev/null +++ b/lib/services/tests/express.docsEmptySpec.unit.tests.js @@ -0,0 +1,105 @@ +/** + * Module dependencies. + */ +import { jest, beforeEach, afterEach, describe, test, expect } from '@jest/globals'; + +/** + * Unit tests — initApiSpec must tolerate a content-free OpenAPI spec file. + * + * js-yaml v5 raises `expected a document, but the input is empty` for a document + * with no content, where v4 returned `undefined`. Since config.files.openapi is + * globbed from `modules/*/doc/*.yml`, a downstream module that ships a stubbed or + * fully commented-out spec would otherwise take the whole application down at boot. + * + * These tests deliberately DO NOT mock js-yaml: a hand-written mock can only throw + * for the inputs its author thought of, and the comment-only case was missed exactly + * that way. Only the real parser can say what it actually rejects. + */ +describe('express initApiSpec — content-free spec files (real js-yaml):', () => { + const baseConfig = { + openapi: { enable: true }, + files: { openapi: ['/fake/openapi.yaml'], guides: [] }, + app: { title: 'Test API', description: 'Test', url: 'https://example.com' }, + domain: 'https://example.com', + }; + + /** + * Build a minimal mock Express app that records registered GET routes. + * @returns {{ get: Function, _routes: Object }} mock app + */ + const buildMockApp = () => { + const routes = {}; + return { get: (path, handler) => { routes[path] = handler; }, _routes: routes }; + }; + + /** + * Mock everything except js-yaml, then run initApiSpec over a single spec file + * whose raw content is `raw`. + * @param {string} raw - file content served by the mocked fs.readFileSync + * @returns {Promise<{app: object, logger: object, run: Function}>} harness + */ + const harnessFor = async (raw) => { + jest.unstable_mockModule('fs', () => ({ + default: { readFileSync: jest.fn().mockReturnValue(raw) }, + readFileSync: jest.fn().mockReturnValue(raw), + })); + jest.unstable_mockModule('../../helpers/config.js', () => ({ + default: { isProd: jest.fn().mockReturnValue(false), isDevEnv: jest.fn().mockReturnValue(true) }, + })); + jest.unstable_mockModule('../../helpers/guides.js', () => ({ + default: { loadGuides: jest.fn().mockReturnValue([]), mergeGuidesIntoSpec: jest.fn() }, + })); + jest.unstable_mockModule('../logger.js', () => ({ + default: { warn: jest.fn(), info: jest.fn(), error: jest.fn() }, + })); + jest.unstable_mockModule('../../../config/index.js', () => ({ default: baseConfig })); + + const { default: expressService } = await import('../express.js'); + const { default: logger } = await import('../logger.js'); + const app = buildMockApp(); + return { app, logger, run: () => expressService.initApiSpec(app) }; + }; + + beforeEach(() => { + jest.resetModules(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + // Every one of these makes real js-yaml v5 raise the empty-document error. + test.each([ + ['zero-byte', ''], + ['whitespace-only', ' \n\n '], + ['comment-only', '# TODO: document this module\n'], + ['BOM-only', '\uFEFF'], + ])('%s spec file is skipped with a warning, never a boot failure', async (_label, raw) => { + const { app, logger, run } = await harnessFor(raw); + + expect(run).not.toThrow(); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('skipping')); + expect(app._routes['/api/spec.json']).toBeUndefined(); + }); + + test('a genuinely malformed spec file still fails loudly', async () => { + // The empty-document guard must not become a catch-all: a real syntax error + // carries a different reason and has to keep propagating. + const { run } = await harnessFor('a:\n\t- [unclosed\n'); + + expect(run).toThrow(/failed to load/); + }); + + test('a valid spec file is still parsed and served', async () => { + const { app, run } = await harnessFor( + 'openapi: "3.0.0"\ninfo:\n title: Test API\n version: "1.0.0"\npaths: {}\n', + ); + + run(); + const handler = app._routes['/api/spec.json']; + expect(handler).toBeDefined(); + let spec = null; + handler({}, { json: (body) => { spec = body; } }); + expect(spec.openapi).toBe('3.0.0'); + }); +}); From 3ad5792a356afc1ebf3b632a6d8acddcc76867b3 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 31 Aug 2026 12:19:57 +0200 Subject: [PATCH 4/4] test(openapi): add the multi-document control to the empty-spec guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up nits from the independent review. A multi-document file is the closest sibling of the guarded condition: v5 raises 'expected a single document in the stream, but found more' from the same function over the same documents array, two lines below the empty-document throw. It is therefore the input any future loosening of the reason check would swallow first, so it earns an explicit control alongside the malformed-syntax one. Also records why loadAll() was not used. It looks like the tidier fix — it returns [] for content-free input and drops the message-string dependency entirely — but it accepts a multi-document file that both v4 and v5 reject, trading a loud failure for a silent partial parse. Drops an HTML entity from the test file's JSDoc that only existed to avoid closing the block comment. --- lib/services/express.js | 3 +++ .../tests/express.docsEmptySpec.unit.tests.js | 14 ++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/services/express.js b/lib/services/express.js index 5ed9a5d40..632313a5d 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -72,6 +72,9 @@ const initApiSpec = (app) => { // comment-only or BOM-only — where v4 returned undefined. Those must stay a // skippable warning, not a boot failure. Discriminate on the reason so real // parse errors ('deficient indentation' etc.) still propagate. + // Do NOT swap this for loadAll(), which returns [] for content-free input and + // looks like the tidier fix: it also accepts a multi-document file that both + // v4 and v5 reject, turning a loud failure into a silent partial parse. if (err?.reason !== 'expected a document, but the input is empty') throw err; } if (!doc || typeof doc !== 'object' || Array.isArray(doc)) { diff --git a/lib/services/tests/express.docsEmptySpec.unit.tests.js b/lib/services/tests/express.docsEmptySpec.unit.tests.js index 9bd6df215..b71fde9a6 100644 --- a/lib/services/tests/express.docsEmptySpec.unit.tests.js +++ b/lib/services/tests/express.docsEmptySpec.unit.tests.js @@ -8,8 +8,9 @@ import { jest, beforeEach, afterEach, describe, test, expect } from '@jest/globa * * js-yaml v5 raises `expected a document, but the input is empty` for a document * with no content, where v4 returned `undefined`. Since config.files.openapi is - * globbed from `modules/*/doc/*.yml`, a downstream module that ships a stubbed or - * fully commented-out spec would otherwise take the whole application down at boot. + * globbed from every module's doc directory, a downstream module that ships a + * stubbed or fully commented-out spec would otherwise take the application down + * at boot. * * These tests deliberately DO NOT mock js-yaml: a hand-written mock can only throw * for the inputs its author thought of, and the comment-only case was missed exactly @@ -90,6 +91,15 @@ describe('express initApiSpec — content-free spec files (real js-yaml):', () = expect(run).toThrow(/failed to load/); }); + test('a multi-document spec file still fails loudly', async () => { + // Closest sibling of the guarded condition: this throws from the same function + // over the same `documents` array, two lines below the empty-document throw. It + // is the input a future loosening of the reason check would swallow first. + const { run } = await harnessFor('a: 1\n---\nb: 2\n'); + + expect(run).toThrow(/failed to load/); + }); + test('a valid spec file is still parsed and served', async () => { const { app, run } = await harnessFor( 'openapi: "3.0.0"\ninfo:\n title: Test API\n version: "1.0.0"\npaths: {}\n',