From 2ff2d33130206717b9634fda7c241e3edff02486 Mon Sep 17 00:00:00 2001 From: os-elon Date: Sat, 22 Aug 2026 21:24:18 +0000 Subject: [PATCH] fix(cli): refuse a relative `plugins: [...]` entry, naming the two spellings that work (#10944) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A string entry that is not a bare package name was handed straight to `import()`, which ESM resolves against `serve.ts` itself — the installed CLI's directory. The served app's root never entered the resolution, so a relative path could never address anything the app owns; measured, `'..'` even loaded this package's own command barrel. The boot loop caught the failure, printed one red line naming a path inside the CLI, and served the app without the plugin. Ruled at triage on #10944: refuse the spelling at load, naming the two that do resolve from the app (a declared bare package name, an absolute/`file://` path). No accepted set widens — the spelling has never loaded an app's file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r --- ...refuse-relative-config-plugin-specifier.md | 74 ++++++ ...erve-config-plugin-host-resolution.test.ts | 27 ++- ...rve-config-plugin-relative-refusal.test.ts | 221 ++++++++++++++++++ packages/cli/src/commands/serve.ts | 87 +++++++ 4 files changed, 401 insertions(+), 8 deletions(-) create mode 100644 .changeset/refuse-relative-config-plugin-specifier.md create mode 100644 packages/cli/src/commands/serve-config-plugin-relative-refusal.test.ts diff --git a/.changeset/refuse-relative-config-plugin-specifier.md b/.changeset/refuse-relative-config-plugin-specifier.md new file mode 100644 index 0000000000..51ad82c40f --- /dev/null +++ b/.changeset/refuse-relative-config-plugin-specifier.md @@ -0,0 +1,74 @@ +--- +"@objectstack/cli": minor +--- + +`os serve` refuses a relative `plugins: [...]` entry, naming the two spellings +that do work (#10944) + +A string entry in the served app's own `objectstack.config.ts` that is not a +bare package name was handed straight to `import()`, which ESM resolves against +the file containing the call — `@objectstack/cli/dist/commands/serve.js`. The +served app's root never entered the resolution, so a relative path could not +address anything the app owns. Measured on `origin/main`, from a fixture app +that really did carry `local-plugin.js` beside its `package.json`: + +``` +'./local-plugin.js' -> Cannot find module '/src/commands/local-plugin.js' + imported from '/src/commands/serve.ts' +'../local-plugin.js' -> Cannot find module '/src/local-plugin.js' +'..' -> LOADED — the CLI's own command barrel + (CompileCommand, ValidateCommand, ServeCommand, …) +``` + +The app's own file was never seen in any of them. The third row is the same +fact stated as a positive: a relative entry can load a module, but only ever +one belonging to the CLI. The boot loop then caught the failure, printed one +red `✗ Failed to load plugin:` line naming a path inside the CLI's install +directory, and served the app **without** the plugin — a deployment that looks +healthy while quietly missing the extension its config declared. + +Such an entry is now refused before any import is attempted, with a message +that names both spellings that resolve from the app: + +``` +Refused the plugin entry './local-plugin.js' in `plugins: [...]`: a RELATIVE path there is +resolved against the CLI's own installation directory, never against your app — so +it can never load a file from your project. This spelling has never worked; it used +to fail with a "Cannot find module" naming a path inside the CLI's install directory. + +Use one of the two spellings that resolve from your app: + + 1. a package name your app DECLARES in its own package.json: + plugins: ['@mycompany/crm'] (then: pnpm add @mycompany/crm) + + 2. an absolute path, or a file:// URL the config computes for itself: + plugins: [new URL('./local-plugin.js', import.meta.url).href] +``` + +**What changes for a config that carries such an entry.** The plugin was never +loaded before and is not loaded now, and the boot still continues past the +failed entry exactly as it did — the observable difference at boot is the text +of the one red line. The entry has *never* worked, but it failed **quietly +enough to be missed**: an author who read `Cannot find module …/@objectstack/ +cli/dist/commands/local-plugin.js` had no way to tell that the spelling itself +was the problem, so a config could carry a dead `plugins:` entry indefinitely +while the deployment appeared fine. That is the silence this closes. + +`minor` rather than `patch` because one shape does change what it loads: +`plugins: ['.']` / `plugins: ['..']` resolved into the CLI's own package and +could register the CLI's command barrel as a plugin. Those are now refused. +Nothing in `content/docs`, `examples/` or the test suite writes any relative +`plugins:` spelling, so no documented usage moves. + +Scope is deliberately narrow, and every other shape that reaches this line was +measured rather than assumed. Untouched: bare package names (declared or not, +scoped or not — including a bare `local-plugin.js`, which ESM reads as a +package name and which keeps its existing "declare it in that app's +package.json" answer), absolute POSIX paths, `file://` URLs, Windows drive +paths (`C:\app\plugin.js`), `node:` builtins and `data:` URLs. All of those are +base-independent — they resolve to the same module whoever imports them — which +is exactly why they are none of this rule's business. + +Resolving a relative entry against the **served app's root** instead is a +capability addition with no measured pull today; it stays a maintainer +decision, and this refusal is the collection point for such a request. diff --git a/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts b/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts index 581da9bbb5..37d44d6b7c 100644 --- a/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts +++ b/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts @@ -154,21 +154,32 @@ describe('os serve → the branches that must NOT move (#10908 supersedes nothin expect(mod.default ?? mod).toBeTruthy(); }); - it('keeps a RELATIVE specifier anchored to serve.ts, not to @objectstack/types', async () => { + // ⚠️ REPLACED, not reworded (#10944). This slot used to pin that a RELATIVE + // specifier stayed anchored to serve.ts rather than being re-based under + // `@objectstack/types/dist/` — i.e. it pinned the exact branch #10944 has + // since removed. #10944 ruled that neither base is the served app's root, so + // the spelling is now refused at load instead of resolved anywhere; keeping + // the old assertion would have pinned a resolution that no longer runs. + // The refusal, the two spellings that do work, and the full shape matrix live + // in `serve-config-plugin-relative-refusal.test.ts`. What remains here is the + // half that is still this file's business: the refusal must not have widened + // to the base-independent spellings this branch exists to protect. + it('refuses a RELATIVE specifier without touching the base-independent ones (#10944)', async () => { const root = makeApp(APP_ONLY, { declare: false, install: false }); const missing = './__no_such_config_plugin_10908__.js'; const err = await Serve.importConfigPlugin(missing, root).catch((e: unknown) => e as Error); expect(err).toBeInstanceOf(Error); - expect(err.message).toContain(`Failed to import plugin '${missing}':`); - // The base is what this pins: the directory holding serve.ts. Routing this - // spelling through the host importer would silently re-base it under - // `@objectstack/types/dist/`, which is the regression this branch prevents. - // Neither base is the served app's root — whether a relative entry SHOULD - // resolve there is #10944, deliberately left open by this card. - expect(err.message).toContain('commands'); + expect(err.message).toBe(Serve.relativePluginSpecifierRefusal(missing)); + // The refusal replaces a resolution attempt, so no base is reported at all. expect(err.message).not.toContain('types/dist'); + // …and an absolute path — the spelling the refusal points at — is + // untouched, which is what makes the line above a narrowing and not a ban. + const file = join(root, 'still-loads.js'); + writeFileSync(file, 'export default { name: "still-loads" };\n'); + const mod: any = await Serve.importConfigPlugin(file, root); + expect(mod.default).toEqual({ name: 'still-loads' }); }); it('loads an absolute path and a file:// URL unchanged (base-independent spellings)', async () => { diff --git a/packages/cli/src/commands/serve-config-plugin-relative-refusal.test.ts b/packages/cli/src/commands/serve-config-plugin-relative-refusal.test.ts new file mode 100644 index 0000000000..9f1fea7b07 --- /dev/null +++ b/packages/cli/src/commands/serve-config-plugin-relative-refusal.test.ts @@ -0,0 +1,221 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A RELATIVE `plugins: [...]` entry is refused at load, naming the two + * spellings that work (#10944). + * + * ## The defect, measured before the fix + * + * `plugins: [...]` in the served app's own `objectstack.config.ts` is THE + * documented way to extend a deployment. A string entry that is not a bare + * package name was handed straight to `import()` — and ESM resolves a relative + * specifier against the file CONTAINING the call, which is + * `@objectstack/cli/dist/commands/serve.js`. The served app's root never + * entered the resolution at all. + * + * Measured on `origin/main` from a fixture app that really did carry + * `local-plugin.js` next to its `package.json`: + * + * './local-plugin.js' -> Cannot find module '/src/commands/local-plugin.js' + * imported from '/src/commands/serve.ts' + * '../local-plugin.js' -> Cannot find module '/src/local-plugin.js' + * '..' -> LOADED — this package's own command barrel + * (CompileCommand, ValidateCommand, ServeCommand, …) + * + * The app's own file was never seen in any of them, and the last row is the + * whole point stated as a positive: a relative entry CAN load a module — it can + * only ever load one belonging to the CLI. The boot loop then catches the + * failure, prints one red line naming a path inside the CLI's install + * directory, and serves the app WITHOUT the plugin, so the deployment looks + * healthy while quietly missing the extension its config declared. + * + * ## Why refusal and not app-root resolution + * + * Ruled at triage on #10944. Refusing expands no accepted set — the spelling + * has never loaded an app's file — and converts a diagnostic about the CLI's + * internals into an answer the author can act on. Resolving relative entries + * against the served app's root is a capability addition with no measured pull + * (nothing in `content/docs`, `examples/` or the test suite writes the + * spelling); it stays a maintainer decision, and this refusal is where such a + * request would come from. + * + * ## What the tests below are shaped to catch + * + * ⚠️ A suite that only asserts "the relative entry is refused" passes just as + * green on an implementation that refuses `plugins: [...]` ENTIRELY. So the + * headline test asserts the refusal and BOTH working spellings in one body — + * remove the narrowing from `Serve.isRelativePluginSpecifier` and that test + * reddens on the loads, not on the refusal. + */ + +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { afterAll, describe, expect, it } from 'vitest'; +import Serve from './serve.js'; + +const roots: string[] = []; +afterAll(() => { + for (const r of roots) rmSync(r, { recursive: true, force: true }); +}); + +/** A name no workspace package can satisfy, so nothing passes by accident. */ +const APP_ONLY = '@os-fixture/relative-refusal-probe'; + +/** + * An app root that carries `local-plugin.js` NEXT TO its `package.json` — the + * exact layout an author writing `plugins: ['./local-plugin.js']` has in mind — + * and, when asked, declares and installs `APP_ONLY` as well. + */ +function makeApp(opts: { declare: boolean } = { declare: false }): string { + const root = mkdtempSync(join(tmpdir(), 'os-10944-')); + roots.push(root); + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'fixture-app', + version: '1.0.0', + type: 'module', + ...(opts.declare ? { dependencies: { [APP_ONLY]: '1.0.0' } } : {}), + }), + ); + writeFileSync(join(root, 'local-plugin.js'), 'export default { name: "app-local-plugin" };\n'); + if (opts.declare) { + const dir = join(root, 'node_modules', ...APP_ONLY.split('/')); + mkdirSync(dir, { recursive: true }); + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ name: APP_ONLY, version: '1.0.0', type: 'module', main: 'index.js' }), + ); + writeFileSync(join(dir, 'index.js'), 'export default { name: "declared-package" };\n'); + } + return root; +} + +describe('os serve → a relative `plugins: [...]` entry is refused (#10944)', () => { + it('refuses the relative entry AND keeps both working spellings loading — one body, on purpose', async () => { + const root = makeApp({ declare: true }); + const absolute = join(root, 'local-plugin.js'); + + // (1) The shape the card was filed against. The file really is there, in + // the app root, and it is still refused — because the app root was never + // the base and this spelling cannot be made to mean it. + const err = await Serve.importConfigPlugin('./local-plugin.js', root).catch( + (e: unknown) => e as Error, + ); + expect(err).toBeInstanceOf(Error); + expect(err.message).toBe(Serve.relativePluginSpecifierRefusal('./local-plugin.js')); + + // (2) A DECLARED bare package name — spelling (a) the refusal names. + const declared: any = await Serve.importConfigPlugin(APP_ONLY, root); + expect(declared.default).toEqual({ name: 'declared-package' }); + + // (3) An absolute path and a `file://` URL — spelling (b) the refusal + // names. Both are base-independent, and both load the app's OWN file: the + // very module (1) refused to reach by the relative spelling. + for (const spelling of [absolute, pathToFileURL(absolute).href]) { + const mod: any = await Serve.importConfigPlugin(spelling, root); + expect(mod.default, spelling).toEqual({ name: 'app-local-plugin' }); + } + }); + + it('names spelling (a) — a bare package name the app DECLARES — in the message', () => { + const message = Serve.relativePluginSpecifierRefusal('./local-plugin.js'); + expect(message).toContain('DECLARES in its own package.json'); + expect(message).toContain("plugins: ['@mycompany/crm']"); + }); + + it('names spelling (b) — an absolute path or `file://` URL — in the message', () => { + const message = Serve.relativePluginSpecifierRefusal('./local-plugin.js'); + expect(message).toContain('absolute path'); + expect(message).toContain('file://'); + // The form an author writing an app-local plugin file actually wants: the + // CONFIG computes the absolute URL from its own location. Echoing the + // author's own specifier makes the line copy-pasteable. + expect(message).toContain("new URL('./local-plugin.js', import.meta.url).href"); + }); + + it('says WHY, and does not promise app-root resolution is coming', () => { + const message = Serve.relativePluginSpecifierRefusal('./local-plugin.js'); + // The consequence, concretely — this is the half that lets an author stop + // hunting for a missing file. + expect(message).toMatch(/CLI's own installation directory/); + expect(message).toMatch(/never against your app/); + // …and it does not claim an import was attempted, because none was. + expect(message).not.toContain('Failed to import plugin'); + // Option A stays a maintainer decision; the message must not pre-announce + // it (triage on #10944 is explicit about this). + expect(message).not.toMatch(/coming soon|will be supported|in a future release/i); + }); + + it('classifies every specifier shape that reaches this line — measured, not assumed', async () => { + // ⚠️ REFUSED and ALLOWED below are the boundary of this card. Each entry + // was run through `Serve.importConfigPlugin` on `origin/main` first; the + // ALLOWED ones all resolve to the SAME module regardless of which file + // imports them, which is exactly why they are none of this rule's business. + const REFUSED = [ + './local-plugin.js', // the filed shape + '../local-plugin.js', // climbing out of the CLI's commands dir + './nested/plugin.js', + '.', // resolves to the CLI's own commands directory + '..', // MEASURED: loaded the CLI's own command barrel + '.\\local-plugin.js', // Node's URL resolution normalises `\` to `/` + '..\\local-plugin.js', + './', + ]; + const ALLOWED = [ + '/abs/local-plugin.js', // absolute POSIX path + 'file:///abs/local-plugin.js', // file:// URL + 'C:\\app\\local-plugin.js', // Windows drive path — absolute + 'C:/app/local-plugin.js', + 'node:path', // builtin; MEASURED: loads + 'data:text/javascript,export default 1', // MEASURED: loads + '@mycompany/crm', // scoped bare package name + 'chalk', // unscoped bare package name + // NOT relative under ESM: a bare specifier, even one that looks like a + // filename. It keeps the #4719 "declare it in that app's package.json" + // answer, which is the right one for a bare name — deliberately left + // alone rather than folded into this refusal. + 'local-plugin.js', + '.hidden-not-relative', // `.` not followed by a separator + ]; + + for (const s of REFUSED) { + expect(Serve.isRelativePluginSpecifier(s), `REFUSED: ${JSON.stringify(s)}`).toBe(true); + } + for (const s of ALLOWED) { + expect(Serve.isRelativePluginSpecifier(s), `ALLOWED: ${JSON.stringify(s)}`).toBe(false); + } + + // The predicate is what the loader consults, so prove the two agree rather + // than trusting the wiring: every REFUSED shape throws the refusal text, + // and no ALLOWED shape ever does. + const root = makeApp(); + for (const s of REFUSED) { + const err = await Serve.importConfigPlugin(s, root).catch((e: unknown) => e as Error); + expect(err, s).toBeInstanceOf(Error); + expect(err.message, s).toBe(Serve.relativePluginSpecifierRefusal(s)); + } + for (const s of ALLOWED) { + // Most of these cannot resolve from a temp fixture, and that is fine — + // what must never happen is the REFUSAL text. + const outcome = await Serve.importConfigPlugin(s, root).catch((e: unknown) => e as Error); + const message = outcome instanceof Error ? outcome.message : ''; + expect(message, s).not.toContain('Refused the plugin entry'); + } + }); +}); + +describe('os serve → the refusal is wired into the load, not just exported (#10944)', () => { + it('the boot loop still routes every string entry through the refusing helper', async () => { + const { readFileSync } = await import('node:fs'); + const source = readFileSync(new URL('./serve.ts', import.meta.url), 'utf8'); + // Without this the behavioural tests above could all pass while the boot + // loop bare-imported the entry and never reached the helper. + expect(source).toContain('await Serve.importConfigPlugin(plugin, hostRoot)'); + const helper = source.slice(source.indexOf('static async importConfigPlugin')); + expect(helper).toContain('Serve.isRelativePluginSpecifier(pluginSpecifier)'); + expect(helper).toContain('Serve.relativePluginSpecifierRefusal(pluginSpecifier)'); + }); +}); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 15add70083..ba779d8282 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -398,6 +398,65 @@ export default class Serve extends Command { return 'off'; } + /** + * Is this `plugins: [...]` entry a RELATIVE module specifier — `./x.js`, + * `../x.js`, `.`, `..` (and their backslash spellings, which Node's URL + * resolution normalises to the same thing)? + * + * Deliberately NARROWER than `packageNameFromSpecifier(s) === undefined`, + * which also answers "not a package" for every BASE-INDEPENDENT spelling — + * an absolute POSIX path, a `file://` URL, a Windows drive path, `node:`, + * `data:`. Those resolve to the same module no matter who imports them, so + * they are none of this predicate's business; only a specifier whose meaning + * depends on WHICH FILE does the importing is. + * + * A bare `local-plugin.js` is NOT relative and is not matched: ESM reads it + * as a package name, and so does this file — it reaches the declaration + * branches below and gets #4719's "declare it in that app's package.json" + * answer, which is the correct one for a bare specifier. + */ + static isRelativePluginSpecifier(pluginSpecifier: string): boolean { + return /^\.\.?(?:[\\/]|$)/.test(pluginSpecifier); + } + + /** + * The refusal text for a relative `plugins: [...]` entry — one owner, so the + * string a user reads is CHOSEN and pinned rather than assembled at a call + * site (the same discipline `importConfigPlugin`'s failure wrapper follows). + * + * It names both spellings that DO resolve from the served app, because a + * refusal that only says "no" leaves the author exactly as stuck as the + * silence it replaced. `new URL(spec, import.meta.url).href` is the second + * one written the way an author actually wants it: the CONFIG computes the + * absolute URL from its own location, so a plugin file sitting next to + * `objectstack.config.ts` loads without publishing it as a package — using + * only resolution that already works today. + */ + static relativePluginSpecifierRefusal(pluginSpecifier: string): string { + // Quoted the way a config author writes it — single quotes unless the + // specifier itself would break them, in which case JSON's escaping is + // correct JS too. A backslash spelling (`.\\x.js`) takes that branch, so + // the suggested line stays copy-pasteable rather than silently dropping + // the escape. + const q = /^[^'\\\r\n]*$/.test(pluginSpecifier) + ? `'${pluginSpecifier}'` + : JSON.stringify(pluginSpecifier); + return [ + `Refused the plugin entry ${q} in \`plugins: [...]\`: a RELATIVE path there is`, + `resolved against the CLI's own installation directory, never against your app — so`, + `it can never load a file from your project. This spelling has never worked; it used`, + `to fail with a "Cannot find module" naming a path inside the CLI's install directory.`, + '', + 'Use one of the two spellings that resolve from your app:', + '', + " 1. a package name your app DECLARES in its own package.json:", + " plugins: ['@mycompany/crm'] (then: pnpm add @mycompany/crm)", + '', + ' 2. an absolute path, or a file:// URL the config computes for itself:', + ` plugins: [new URL(${q}, import.meta.url).href]`, + ].join('\n'); + } + /** * Load one `plugins: [...]` entry of the served app's own config that is * written as a STRING (#10908). @@ -456,12 +515,40 @@ export default class Serve extends Command { * declared one resolves FROM. The #4719 declaration gate is untouched, and no * undeclared package gains a way in that it did not already have. * + * ── The relative branch is REFUSED, not resolved (#10944) ────────────────── + * + * A relative entry is the one spelling that can never mean what its author + * meant. It is resolved against THIS file's directory — the installed CLI's + * `@objectstack/cli/dist/commands/` — so the served app's root never enters + * the resolution at all. Measured from a fixture app that really does carry + * `local-plugin.js` next to its config: `'./local-plugin.js'` resolves to + * `/src/commands/local-plugin.js` and the app's own file is never seen, + * while `'..'` LOADS this package's own command barrel. The boot loop then + * catches the failure, prints one red line naming a path inside the CLI, and + * serves the app WITHOUT the plugin — so the deployment looks healthy and is + * quietly missing the extension it declared. + * + * Ruled at triage on #10944: refuse it, naming the two spellings that work. + * That expands no accepted set — the spelling has never loaded an app's file + * — and turns a diagnostic about the CLI's internals into an answer the + * author can act on. Resolving relative entries against the SERVED APP's root + * instead is a capability addition with no measured pull (no doc, example or + * test in this repo uses the spelling); it stays a decision for a maintainer, + * and this refusal is where that request would come from. + * * @param pluginSpecifier The string as the app wrote it in `plugins: [...]`. * @param hostRoot Root of the served app; defaults to the process CWD, the * same value `serve`'s boot path computes. */ static async importConfigPlugin(pluginSpecifier: string, hostRoot?: string): Promise { const root = hostRoot ?? process.cwd(); + // Refused BEFORE the try, and deliberately not wrapped in the + // `Failed to import plugin '': …` text below: nothing was imported, + // and calling a refusal an import failure sends the author looking for a + // missing file. The refusal names the specifier itself. + if (Serve.isRelativePluginSpecifier(pluginSpecifier)) { + throw new Error(Serve.relativePluginSpecifierRefusal(pluginSpecifier)); + } try { // `await` inside the `try` rather than a bare `return`: a returned promise // would settle OUTSIDE it and skip the diagnostic wrapper below.