diff --git a/MIGRATIONS.md b/MIGRATIONS.md index 124dfcf0d..7f310f26b 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -4,6 +4,77 @@ 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`. 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'` +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 the old YAML 1.1 default.** +Merge keys, implicit timestamps, `!!binary`, `!!omap`, `!!pairs` and `!!set` are no longer +in the default schema. Two of these fail quietly, so read carefully: + +| 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 `<<:`, `!!` 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` 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. + +--- + ## 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..632313a5d 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'; @@ -63,7 +63,20 @@ const initApiSpec = (app) => { const contents = config.files.openapi .map((filePath) => { try { - const doc = YAML.load(fs.readFileSync(filePath).toString()); + const raw = fs.readFileSync(filePath).toString(); + 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. + // 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)) { 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..705af42c4 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), })); 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..b71fde9a6 --- /dev/null +++ b/lib/services/tests/express.docsEmptySpec.unit.tests.js @@ -0,0 +1,115 @@ +/** + * 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 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 + * 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 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', + ); + + 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'); + }); +}); 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', () => ({ 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",