diff --git a/.changeset/clean-cats-guard.md b/.changeset/clean-cats-guard.md new file mode 100644 index 000000000..0a00080ab --- /dev/null +++ b/.changeset/clean-cats-guard.md @@ -0,0 +1,7 @@ +--- +'@openfn/runtime': patch +'@openfn/engine-multi': patch +'@openfn/ws-worker': patch +--- + +Handle step and adaptor identifiers that match built-in object properties. diff --git a/packages/engine-multi/src/api/autoinstall.ts b/packages/engine-multi/src/api/autoinstall.ts index 6d351d9f2..1db3eabc8 100644 --- a/packages/engine-multi/src/api/autoinstall.ts +++ b/packages/engine-multi/src/api/autoinstall.ts @@ -14,6 +14,9 @@ import { AUTOINSTALL_COMPLETE, AUTOINSTALL_ERROR } from '../events'; import { AutoinstallError } from '../errors'; import ExecutionContext from '../classes/ExecutionContext'; +const hasOwn = (target: object, key: PropertyKey) => + Object.prototype.hasOwnProperty.call(target, key); + // none of these options should be on the plan actually export type AutoinstallOptions = { skipRepoValidation?: boolean; @@ -122,7 +125,7 @@ const autoinstall = async (context: ExecutionContext): Promise => { } const adaptors = Array.from(identifyAdaptors(plan)); - const paths: ModulePaths = {}; + const paths = new Map(); const adaptorsToLoad = []; for (const a of adaptors) { @@ -148,8 +151,13 @@ const autoinstall = async (context: ExecutionContext): Promise => { const alias = getAliasedName(resolvedAdaptorName); // Write the adaptor version to the context for reporting later - if (!context.versions[name]) { - context.versions[name] = []; + if (!hasOwn(context.versions, name)) { + Object.defineProperty(context.versions, name, { + value: [], + configurable: true, + enumerable: true, + writable: true, + }); } if (!context.versions[name].includes(v)) { (context.versions[name] as string[]).push(v); @@ -161,10 +169,10 @@ const autoinstall = async (context: ExecutionContext): Promise => { } // important: write back to paths with the RAW specifier - paths[a] = { + paths.set(a, { path: `${repoDir}/node_modules/${alias}`, version: v, - }; + }); if (!(await isInstalledFn(resolvedAdaptorName, repoDir, logger))) { adaptorsToLoad.push(resolvedAdaptorName); @@ -175,11 +183,16 @@ const autoinstall = async (context: ExecutionContext): Promise => { for (const step of plan.workflow.steps) { const job = step as unknown as Job; for (const adaptor of job.adaptors ?? []) { - if (paths[adaptor!]) { + const modulePath = paths.get(adaptor!); + if (modulePath) { const { name } = getNameAndVersion(adaptor!); job.linker ??= {}; - // @ts-ignore - job.linker[name] = paths[adaptor!]; + Object.defineProperty(job.linker, name, { + value: modulePath, + configurable: true, + enumerable: true, + writable: true, + }); } } } @@ -196,11 +209,11 @@ const autoinstall = async (context: ExecutionContext): Promise => { if (err) { throw err; } - return paths; + return Object.fromEntries(paths); }); } - return paths; + return Object.fromEntries(paths); }; export default autoinstall; @@ -232,7 +245,7 @@ const isInstalled = async ( const pkg = await loadRepoPkg(repoDir); if (pkg) { const { dependencies } = pkg; - return dependencies.hasOwnProperty(alias); + return hasOwn(dependencies, alias); } }; diff --git a/packages/engine-multi/test/api/autoinstall.test.ts b/packages/engine-multi/test/api/autoinstall.test.ts index 36d550164..0a780e0dc 100644 --- a/packages/engine-multi/test/api/autoinstall.test.ts +++ b/packages/engine-multi/test/api/autoinstall.test.ts @@ -92,6 +92,27 @@ test('Autoinstall basically works', async (t) => { }); }); +test('autoinstall supports an adaptor named constructor', async (t) => { + const context = createContext( + { + handleInstall: mockHandleInstall, + handleIsInstalled: async () => false, + }, + [{ adaptors: ['constructor@1.0.0'] }], + [/constructor/] + ); + + const paths = await autoinstall(context); + + t.deepEqual(paths, { + 'constructor@1.0.0': { + path: 'tmp/repo/node_modules/constructor_1.0.0', + version: '1.0.0', + }, + }); + t.deepEqual(context.versions.constructor, ['1.0.0']); +}); + test('mock is installed: should be installed', async (t) => { const isInstalled = mockIsInstalled({ name: 'repo', diff --git a/packages/runtime/src/execute/compile-plan.ts b/packages/runtime/src/execute/compile-plan.ts index ea27b9835..0b96c4cb9 100644 --- a/packages/runtime/src/execute/compile-plan.ts +++ b/packages/runtime/src/execute/compile-plan.ts @@ -15,14 +15,29 @@ import { getNameAndVersion } from '../modules/repo'; // map special condition strings to JS expressions // The special strings are generated by lightning, and are useful convenience for local dev export const conditions: Record = { - on_job_success: 'Boolean(!state?.errors?.[upstreamStepId] ?? true)', - on_job_failure: 'Boolean(state?.errors && state.errors[upstreamStepId])', + on_job_success: + 'Boolean(!state?.errors || !Object.prototype.hasOwnProperty.call(state.errors, upstreamStepId) || !state.errors[upstreamStepId])', + on_job_failure: + 'Boolean(state?.errors && Object.prototype.hasOwnProperty.call(state.errors, upstreamStepId) && state.errors[upstreamStepId])', always: 'true', }; // create a couple of aliases for future reference conditions.on_upstream_success = conditions.on_job_success; conditions.on_upstream_fail = conditions.on_job_failure; +// Assignment to __proto__ invokes a setter on ordinary objects. Define lookup +// entries as own data properties so external ids cannot collide with built-ins. +const defineOwn = (target: object, key: string, value: unknown) => + Object.defineProperty(target, key, { + value, + configurable: true, + enumerable: true, + writable: true, + }); + +const hasOwn = (target: object, key: PropertyKey) => + Object.prototype.hasOwnProperty.call(target, key); + const compileEdges = ( from: string, edges: string | Record, @@ -41,13 +56,13 @@ const compileEdges = ( try { const edge = edges[edgeId]; if (typeof edge === 'boolean') { - result[edgeId] = edge; + defineOwn(result, edgeId, edge); } else if (typeof edge === 'string') { - result[edgeId] = { + defineOwn(result, edgeId, { condition: compileFunction(mapCondition(edge), context, [ 'upstreamStepId', ]), - }; + }); } else { const newEdge = { ...edge, @@ -59,7 +74,7 @@ const compileEdges = ( ['upstreamStepId'] ); } - result[edgeId] = newEdge as CompiledEdge; + defineOwn(result, edgeId, newEdge as CompiledEdge); } } catch (e: any) { errs.push( @@ -87,7 +102,7 @@ const findUpstream = (workflow: Workflow, id: string) => { if (job.next === id) { return job.id; } - } else if (job.next[id]) { + } else if (hasOwn(job.next, id) && job.next[id]) { return job.id; } } @@ -141,7 +156,7 @@ export default (plan: ExecutionPlan) => { const maybeAssign = (a: any, b: any, keys: Array) => { keys.forEach((key) => { - if (a.hasOwnProperty(key)) { + if (hasOwn(a, key)) { b[key] = a[key]; } }); @@ -169,7 +184,7 @@ export default (plan: ExecutionPlan) => { newStep.linker ??= {}; for (const adaptor of job.adaptors!) { const { name, version } = getNameAndVersion(adaptor); - newStep.linker[name] = { version: version! }; + defineOwn(newStep.linker, name, { version: version! }); } } @@ -179,7 +194,7 @@ export default (plan: ExecutionPlan) => { }); } newStep.previous = findUpstream(workflow, stepId); - newPlan.workflow.steps[stepId] = newStep; + defineOwn(newPlan.workflow.steps, stepId, newStep); } if (errs.length) { diff --git a/packages/runtime/test/execute/compile-plan.test.ts b/packages/runtime/test/execute/compile-plan.test.ts index ddebddada..e4ac8062b 100644 --- a/packages/runtime/test/execute/compile-plan.test.ts +++ b/packages/runtime/test/execute/compile-plan.test.ts @@ -144,6 +144,18 @@ test('should convert steps to an object', (t) => { t.is(workflow.steps.b.expression, 'y'); }); +test('should preserve a step whose id is __proto__', (t) => { + const { workflow } = compilePlan({ + workflow: { + steps: [{ id: '__proto__', expression: 'x' }], + }, + }); + + t.deepEqual(Object.keys(workflow.steps), ['__proto__']); + t.is(workflow.steps.__proto__.id, '__proto__'); + t.is(workflow.steps.__proto__.expression, 'x'); +}); + test('should set previous job with 2 steps', (t) => { const plan: ExecutionPlan = { workflow: { diff --git a/packages/ws-worker/src/api/reasons.ts b/packages/ws-worker/src/api/reasons.ts index ab0c20c63..c2f184f41 100644 --- a/packages/ws-worker/src/api/reasons.ts +++ b/packages/ws-worker/src/api/reasons.ts @@ -3,6 +3,9 @@ import { Step } from '@openfn/runtime'; import { ExitReason, ExitReasonStrings } from '@openfn/lexicon/lightning'; import type { RunState } from '../types'; +const hasOwn = (target: object, key: PropertyKey) => + Object.prototype.hasOwnProperty.call(target, key); + // This takes the result state and error from the job const calculateJobExitReason = ( jobId: string, @@ -18,7 +21,11 @@ const calculateJobExitReason = ( reason = error.severity ?? 'crash'; error_message = error.message; error_type = error.subtype || error.type || error.name; - } else if (state.errors?.[jobId]) { + } else if ( + state.errors && + hasOwn(state.errors, jobId) && + state.errors[jobId] + ) { reason = 'fail'; ({ message: error_message, name: error_type } = state.errors[jobId]); } diff --git a/packages/ws-worker/test/api/reasons.test.ts b/packages/ws-worker/test/api/reasons.test.ts index 1945e7068..95e053131 100644 --- a/packages/ws-worker/test/api/reasons.test.ts +++ b/packages/ws-worker/test/api/reasons.test.ts @@ -32,6 +32,14 @@ test('still success if a prior job has errors', (t) => { t.is(r.error_message, null); }); +test('built-in object properties are not treated as job errors', (t) => { + const r = calculateJobExitReason('constructor', { errors: {} } as any); + + t.is(r.reason, 'success'); + t.is(r.error_type, null); + t.is(r.error_message, null); +}); + test('fail', (t) => { const jobId = 'a'; const state: any = {