diff --git a/.changeset/22434-connector-provider-package-path.md b/.changeset/22434-connector-provider-package-path.md new file mode 100644 index 00000000000..b6df72d552f --- /dev/null +++ b/.changeset/22434-connector-provider-package-path.md @@ -0,0 +1,14 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-automation": minor +--- + +feat(spec): `ConnectorProviderContext.resolvePackagePath` — a connector provider factory can resolve a path against the declaring app's root + +Clause-②: yes (widening) + +- **What is new.** `ConnectorProviderContext` gains one optional, host-provided member, `resolvePackagePath(relativePath): Promise`, beside `loadPackageFile`. It resolves a relative path against the root of the stack or package that declared the connector entry and returns the absolute path. `'.'` returns the root itself. It refuses (throws on) an empty path, an absolute path, and a path that escapes the root after resolution, such as `../x` or `a/../../x`. That is the same rule `loadPackageFile` follows. It reads nothing and does not check that the path exists. +- **Who hands it.** The connector materializer in `@objectstack/service-automation` hands it to every provider factory. It is anchored at the plugin's `packageRoot` option, the same root `loadPackageFile` reads under, and falls back to `process.cwd()` the same way. The two members now share one confinement check, so they cannot disagree about what is inside the root. `loadPackageFile`'s behaviour and error messages are unchanged. +- **Who reads it.** Nothing yet. `@objectstack/connector-mcp` is next: a declarative stdio transport will use it as the launched process's working directory, so a relative command or argument resolves against the app's root instead of the directory the server was started from. +- **What does not change.** Which commands a declarative stdio transport may launch is untouched. A factory that does not read the member behaves as before. +- **Nothing to migrate.** A host that builds its own `ConnectorProviderContext` may leave the member out. A factory must then keep its existing behaviour, or fail with a clear message if it needs the root. diff --git a/packages/services/service-automation/src/connector-materialization.test.ts b/packages/services/service-automation/src/connector-materialization.test.ts index 26b93d50343..6a8e9b83ad1 100644 --- a/packages/services/service-automation/src/connector-materialization.test.ts +++ b/packages/services/service-automation/src/connector-materialization.test.ts @@ -24,6 +24,7 @@ import { ConnectorUpstreamUnavailableError } from '@objectstack/spec/integration import { AutomationServicePlugin, createPackageFileLoader, + createPackagePathResolver, DECLARATIVE_RETRY_BASE_MS, type CredentialResolver, } from './plugin.js'; @@ -542,6 +543,127 @@ describe('#3016 — file-ref materialization policy (fatal at boot, soft on relo }); }); +// ── #22434 — package path refs (resolvePackagePath) ───────────────────────── +// +// The sibling of `loadPackageFile` for a factory that needs a LOCATION — a +// launched process's working directory — rather than file contents. Same root, +// same confinement rule (one shared check), same `process.cwd()` default. It +// reads nothing, so a resolved path need not exist. + +/** The rejection a promise settles with, or a fail-loud marker if it resolved. */ +async function rejectionOf(p: Promise): Promise { + return p.then( + (value) => { throw new Error(`expected a refusal, got ${JSON.stringify(value)}`); }, + (err: unknown) => err as Error, + ); +} + +describe('#22434 — package path resolver (createPackagePathResolver)', () => { + const root = path.resolve(fixtureRoot); + const resolvePath = createPackagePathResolver(fixtureRoot); + + it('resolves a relative path against the package root, and `.` to the root itself', async () => { + await expect(resolvePath('.')).resolves.toBe(root); + await expect(resolvePath('./specs/billing.json')).resolves.toBe(path.join(root, 'specs', 'billing.json')); + await expect(resolvePath('specs')).resolves.toBe(path.join(root, 'specs')); + // Judged on the RESOLVED path: a traversal that lands back inside passes. + await expect(resolvePath('./specs/../specs')).resolves.toBe(path.join(root, 'specs')); + // A location, not a read: a path that does not exist yet still resolves. + await expect(resolvePath('./scripts/not-there.mjs')).resolves.toBe(path.join(root, 'scripts', 'not-there.mjs')); + }); + + it('refuses a path that escapes the root, naming the ref and the root it is confined to', async () => { + for (const ref of ['..', '../outside', 'specs/../../outside']) { + const err = await rejectionOf(resolvePath(ref)); + expect(err).toBeInstanceOf(Error); + expect(err).not.toBeInstanceOf(TypeError); + expect(err.message).toContain(`'${ref}' escapes the stack/package root`); + expect(err.message).toContain(`confined to '${root}'`); + } + }); + + it('refuses an absolute path (posix and windows-drive) and an empty ref', async () => { + for (const ref of [path.join(root, 'specs'), '/etc', 'C:\\evil']) { + const err = await rejectionOf(resolvePath(ref)); + expect(err).not.toBeInstanceOf(TypeError); + expect(err.message).toContain(`'${ref}' is absolute`); + } + for (const ref of ['', ' ']) { + expect((await rejectionOf(resolvePath(ref))).message).toContain('non-empty relative path'); + } + }); + + it('shares the loader\'s rule: a ref is refused for confinement by both members or by neither', async () => { + const load = createPackageFileLoader(fixtureRoot); + const confinement = /non-empty relative path|is absolute|escapes the stack\/package root/; + const refs = ['.', 'specs', './specs/billing.json', './specs/../specs/billing.json', '', '..', '../x', + 'specs/../../x', '/etc/hostname', 'C:/evil', 'D:\\evil']; + for (const ref of refs) { + const loaderRefused = await load(ref).then(() => false, (e: Error) => confinement.test(e.message)); + const resolverRefused = await resolvePath(ref).then(() => false, (e: Error) => confinement.test(e.message)); + expect(resolverRefused, `ref '${ref}'`).toBe(loaderRefused); + } + }); + + it('defaults to process.cwd() when no packageRoot is given, as the loader does', async () => { + await expect(createPackagePathResolver()('.')).resolves.toBe(path.resolve(process.cwd())); + }); +}); + +describe('#22434 — the materializer hands resolvePackagePath anchored to packageRoot', () => { + /** A provider whose factory resolves the refs in `providerConfig.paths` before materializing. */ + function makePathResolvingProvider() { + const resolved: string[] = []; + const factory: ConnectorProviderFactory = async (ctx) => { + for (const ref of (ctx.providerConfig.paths as string[]) ?? []) { + resolved.push(await ctx.resolvePackagePath!(ref)); + } + return { + def: { name: ctx.name, label: ctx.label, type: 'api', authentication: { type: 'none' }, actions: [{ key: 'ping', label: 'Ping' }] } as unknown as Connector, + handlers: { ping: async () => ({ ok: true }) }, + }; + }; + return { factory, resolved }; + } + + it('a factory resolving `.` and a relative path gets locations under packageRoot', async () => { + const { factory, resolved } = makePathResolvingProvider(); + const kernel = await boot( + [providerConnector('tools', { providerConfig: { paths: ['.', './scripts/fixture.mjs'] } })], + { providerFactory: factory, packageRoot: fixtureRoot }, + ); + expect(automationOf(kernel).getRegisteredConnectors()).toContain('tools'); + expect(resolved).toEqual([path.resolve(fixtureRoot), path.join(path.resolve(fixtureRoot), 'scripts', 'fixture.mjs')]); + await kernel.shutdown(); + }); + + it('fails boot loudly when a factory resolves a path that escapes the package root', async () => { + const { factory, resolved } = makePathResolvingProvider(); + await expect( + boot([providerConnector('tools', { providerConfig: { paths: ['../outside'] } })], { + providerFactory: factory, + packageRoot: fixtureRoot, + }), + ).rejects.toThrow(/failed to materialize connector instance 'tools'.*'\.\.\/outside' escapes the stack\/package root/s); + expect(resolved).toEqual([]); + }); + + it('a factory that never reads it materializes exactly as before', async () => { + const { factory, calls } = makeFakeProvider(); + const kernel = await boot([providerConnector('billing')], { providerFactory: factory, packageRoot: fixtureRoot }); + const engine = automationOf(kernel); + expect(engine.getRegisteredConnectors()).toContain('billing'); + // The member is handed (the factory simply ignores it) … + expect(typeof calls[0]?.resolvePackagePath).toBe('function'); + // … and the connector it materialized is listed exactly as it always was. + const desc = engine.getConnectorDescriptors().find((d) => d.name === 'billing'); + expect(desc?.actions.map((a) => a.key)).toEqual(['ping']); + expect(desc?.origin).toBe('declarative'); + expect(calls).toHaveLength(1); + await kernel.shutdown(); + }); +}); + // ── #3017 — upstream unavailable: degrade + retry, never a dead boot ───────── // // A provider factory that throws the CONNECTOR_UPSTREAM_UNAVAILABLE marker diff --git a/packages/services/service-automation/src/plugin.ts b/packages/services/service-automation/src/plugin.ts index e36e7077781..885704fb42e 100644 --- a/packages/services/service-automation/src/plugin.ts +++ b/packages/services/service-automation/src/plugin.ts @@ -183,21 +183,67 @@ export interface AutomationServicePluginOptions { * against (#3016, e.g. `providerConfig.spec: './billing-openapi.json'` for * `provider: 'openapi'`). The CLI passes the directory containing * `objectstack.config.ts`; embedders pass their stack root. Defaults to - * `process.cwd()`. Reads are confined to this root (see - * {@link createPackageFileLoader}). + * `process.cwd()`. Reads and resolved paths are confined to this root (see + * {@link createPackageFileLoader} and {@link createPackagePathResolver}). */ packageRoot?: string; } +/** + * The ONE confinement rule behind both package-anchored members of + * {@link ConnectorProviderContext} — `loadPackageFile` (#3016) and + * `resolvePackagePath` (#22434) — so the two can never disagree about what + * "inside the stack/package root" means. Resolves `relativePath` against + * `packageRoot` (default `process.cwd()`, read at call time) and returns the + * absolute result. Rejects an empty ref, an absolute ref (posix or Windows + * drive-letter), and any ref that escapes the root after resolution (`../…`, + * `a/../../…`). The check is on the normalized path, not on `realpath`: a + * symlink inside the root is judged by where it sits, not by where it points, + * for both members alike. + * + * `confined` only words the escape refusal for the member that raised it + * (`reads` for the loader, whose text is pinned verbatim by the platform + * checklist, `paths` for the resolver); the rule itself is identical. + * + * `node:path` is imported lazily so merely constructing either capability + * never touches it — hosts without a filesystem only fail if a factory + * actually dereferences a package ref. + */ +async function resolveInsidePackageRoot( + packageRoot: string | undefined, + relativePath: string, + confined: 'reads' | 'paths', +): Promise { + if (typeof relativePath !== 'string' || relativePath.trim().length === 0) { + throw new Error('package file ref must be a non-empty relative path.'); + } + const path = await import('node:path'); + // Windows drive-letter absolutes ('C:\…') are not `isAbsolute` on posix — + // reject them explicitly so the guard is platform-independent. + if (path.isAbsolute(relativePath) || /^[a-zA-Z]:[\\/]/.test(relativePath)) { + throw new Error( + `package file ref '${relativePath}' is absolute — file refs must be relative to the declaring stack/package root.`, + ); + } + const root = path.resolve(packageRoot ?? process.cwd()); + const resolved = path.resolve(root, relativePath); + const rel = path.relative(root, resolved); + if (rel.startsWith('..') || path.isAbsolute(rel)) { + throw new Error( + `package file ref '${relativePath}' escapes the stack/package root — ${confined} are confined to '${root}'.`, + ); + } + return resolved; +} + /** * Build the `loadPackageFile` capability handed to provider factories via * {@link ConnectorProviderContext} (#3016): read a UTF-8 text file resolved - * against `packageRoot`, **confined to that root**. Rejects absolute paths and - * any path that escapes the root after resolution (`../…`, `a/../../…`), so a - * declarative entry can never read outside the stack/package that declared it. - * A missing/unreadable file throws — the materializer's reconcile policy makes - * that fatal at boot and a skipped entry on reload, like every other ADR-0097 - * materialization failure. + * against `packageRoot`, **confined to that root** by + * {@link resolveInsidePackageRoot}, so a declarative entry can never read + * outside the stack/package that declared it. A missing/unreadable file throws + * — the materializer's reconcile policy makes that fatal at boot and a skipped + * entry on reload, like every other ADR-0097 materialization failure. * * Node builtins are imported lazily inside the returned closure so merely * constructing the capability never touches `node:fs`/`node:path` — hosts @@ -205,25 +251,7 @@ export interface AutomationServicePluginOptions { */ export function createPackageFileLoader(packageRoot?: string): (relativePath: string) => Promise { return async (relativePath: string) => { - if (typeof relativePath !== 'string' || relativePath.trim().length === 0) { - throw new Error('package file ref must be a non-empty relative path.'); - } - const path = await import('node:path'); - // Windows drive-letter absolutes ('C:\…') are not `isAbsolute` on posix — - // reject them explicitly so the guard is platform-independent. - if (path.isAbsolute(relativePath) || /^[a-zA-Z]:[\\/]/.test(relativePath)) { - throw new Error( - `package file ref '${relativePath}' is absolute — file refs must be relative to the declaring stack/package root.`, - ); - } - const root = path.resolve(packageRoot ?? process.cwd()); - const resolved = path.resolve(root, relativePath); - const rel = path.relative(root, resolved); - if (rel.startsWith('..') || path.isAbsolute(rel)) { - throw new Error( - `package file ref '${relativePath}' escapes the stack/package root — reads are confined to '${root}'.`, - ); - } + const resolved = await resolveInsidePackageRoot(packageRoot, relativePath, 'reads'); const { readFile } = await import('node:fs/promises'); try { return await readFile(resolved, 'utf8'); @@ -235,6 +263,20 @@ export function createPackageFileLoader(packageRoot?: string): (relativePath: st }; } +/** + * Build the `resolvePackagePath` capability handed to provider factories via + * {@link ConnectorProviderContext} (#22434): the absolute path of a ref + * resolved against `packageRoot` — `'.'` is the root itself — **confined to + * that root** by the same {@link resolveInsidePackageRoot} rule the loader + * uses, with the same `process.cwd()` default. It reads nothing and does not + * check existence; a factory that needs a location (a launched process's + * working directory) gets the declaring app's root instead of the server's + * current directory. + */ +export function createPackagePathResolver(packageRoot?: string): (relativePath: string) => Promise { + return (relativePath: string) => resolveInsidePackageRoot(packageRoot, relativePath, 'paths'); +} + /** * Resolves a declarative connector `credentialRef` to its plaintext secret, or * `undefined`/empty when unknown (which the materializer turns into a hard boot @@ -1975,6 +2017,9 @@ export class AutomationServicePlugin implements Plugin { // openapi's `providerConfig.spec: './billing-openapi.json'`), // confined to the stack/package root. loadPackageFile: createPackageFileLoader(this.options.packageRoot), + // #22434 — the same root as a location (e.g. a launched + // process's working directory), under the same confinement. + resolvePackagePath: createPackagePathResolver(this.options.packageRoot), }; let materialization; diff --git a/packages/spec/src/integration/connector-provider.test.ts b/packages/spec/src/integration/connector-provider.test.ts index 335ee01eafc..ba916f3cbee 100644 --- a/packages/spec/src/integration/connector-provider.test.ts +++ b/packages/spec/src/integration/connector-provider.test.ts @@ -5,12 +5,13 @@ // credentialRef-based instance auth (no inline secrets), and the authoring rules // enforced by DeclarativeConnectorEntrySchema. -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, expectTypeOf } from 'vitest'; import { ConnectorSchema, DeclarativeConnectorEntrySchema, ConnectorInstanceAuthSchema, } from './connector.zod'; +import type { ConnectorProviderContext } from './connector-provider'; describe('ADR-0097 connector schema evolution', () => { describe('ConnectorSchema — new fields', () => { @@ -167,3 +168,53 @@ describe('ADR-0097 connector schema evolution', () => { }); }); }); + +// --------------------------------------------------------------------------- +// [#22434] `ConnectorProviderContext.resolvePackagePath` — the host-provided +// package anchor, beside `loadPackageFile`. +// +// The pins here hold what the card decided at the CONTRACT layer: the member is +// OPTIONAL — a host that does not hand it, and every context built before it +// existed, still conforms, so its absence leaves a factory's behaviour as it +// was — and it has exactly the signature of its sibling `loadPackageFile`. The +// behaviour pins (resolves against the package root, `'.'` is the root, an +// escape is refused) live with the host that implements it: +// `packages/services/service-automation/src/connector-materialization.test.ts`. +// +// The optionality pin is a type alias, not `expectTypeOf`: `toEqualTypeOf<… +// | undefined>` cannot tell an optional member from a REQUIRED one typed +// `… | undefined`, while `{} extends Pick<…>` can. Exported for TS6196; +// `check:test-typecheck` compiles this file. +// --------------------------------------------------------------------------- + +type Eq = (() => T extends A ? 1 : 2) extends (() => T extends B ? 1 : 2) ? true : false; +type Assert = T; + +/** Optional: a host may omit it. A required member turns this alias red. */ +export type ResolvePackagePathIsOptional = Assert< + {} extends Pick ? true : false +>; + +/** One member family: the same `(relativePath) => Promise` shape as the loader. */ +export type ResolvePackagePathMatchesLoader = Assert< + Eq +>; + +describe('ConnectorProviderContext.resolvePackagePath — the host-provided package anchor', () => { + it('is optional: a context without it still conforms, and a factory can tell it is absent', async () => { + expectTypeOf().toEqualTypeOf< + ((relativePath: string) => Promise) | undefined + >(); + + // The shape every host built before this member existed. This literal + // failing to compile is the regression. + const withoutAnchor: ConnectorProviderContext = { name: 'tools', label: 'Tools', type: 'api', providerConfig: {} }; + expect(withoutAnchor.resolvePackagePath).toBeUndefined(); + + const withAnchor: ConnectorProviderContext = { + ...withoutAnchor, + resolvePackagePath: async (relativePath) => `/srv/app/${relativePath}`, + }; + await expect(withAnchor.resolvePackagePath?.('scripts')).resolves.toBe('/srv/app/scripts'); + }); +}); diff --git a/packages/spec/src/integration/connector-provider.ts b/packages/spec/src/integration/connector-provider.ts index dd3effa79ac..d36e0852921 100644 --- a/packages/spec/src/integration/connector-provider.ts +++ b/packages/spec/src/integration/connector-provider.ts @@ -115,6 +115,27 @@ export interface ConnectorProviderContext { * factory needing a file must then fail with a clear message. */ readonly loadPackageFile?: (relativePath: string) => Promise; + /** + * Host-injected package path resolver (ADR-0097 follow-up), the sibling of + * {@link loadPackageFile} for a factory that needs a **location** rather than + * file contents — e.g. a working directory for a process the provider + * launches, so the declaring app's relative paths resolve against the app's + * root and not against wherever the server happened to be started. + * + * Resolves a **relative** path against the root of the stack/package that + * declared the entry and returns the absolute path. `'.'` returns the root + * itself. It follows the same confinement rule as `loadPackageFile`, and the + * host enforces both with one check: an empty path, an absolute path, and a + * path that escapes the root after resolution (`../x`, `a/../../x`) are + * rejected (throws). The check is on the normalized path, not on symlink + * targets. It does not check that the path exists — what to do with a + * missing location is the factory's call. + * + * `undefined` on hosts that cannot provide it (edge/browser kernels, or a + * host that predates this member) — a factory then keeps the behaviour it + * had without it, or fails with a clear message if it cannot. + */ + readonly resolvePackagePath?: (relativePath: string) => Promise; } /**