fix: guard identifier lookups against object prototype collisions - #1519
fix: guard identifier lookups against object prototype collisions#1519mah1104ahm wants to merge 4 commits into
Conversation
22ca652 to
e975cd2
Compare
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Thank you for the contribution @mah1104ahm but I don't love these changes.
I'm broadly in favour of using more maps and fewer objects for indexes - although it's tricky for us because maps don't serialize and sometimes they can be a pain.
But I'm not convinced that the underlying issue is ever going to cause users a problem in production. I can run steps called constructor and __proto__ without any problems.
Maybe you can give me and @elias-ba (the original reporter) a bit of time to discuss the original issue, and we'll come back to you
| "start": "docker run worker-integration-tests", | ||
| "test": "pnpm clean && npx ava -s --timeout 2m", | ||
| "test:cache": "npx ava -s --timeout 2m" | ||
| "test": "pnpm clean && npx ava -s --timeout 4m", |
There was a problem hiding this comment.
I'm not sure why this change was added here? We do have a few flaky tests but I'm not aware that timeout is the issue. Please revert.
| import { AutoinstallError } from '../errors'; | ||
| import ExecutionContext from '../classes/ExecutionContext'; | ||
|
|
||
| const hasOwn = (target: object, key: PropertyKey) => |
There was a problem hiding this comment.
This function is called just twice in this file. I see no reason to alias it - please just do eg context.versions.hasOwnProperty(name)
| if (!context.versions[name]) { | ||
| context.versions[name] = []; | ||
| if (!hasOwn(context.versions, name)) { | ||
| Object.defineProperty(context.versions, name, { |
There was a problem hiding this comment.
But name here is always an adaptor name. It should always be of the form @openfn/language-x. This will never conflict.
| }); | ||
| }); | ||
|
|
||
| test('autoinstall supports an adaptor named constructor', async (t) => { |
There was a problem hiding this comment.
good test - this fails on main. It's a bit of a strech of a use-case because I can't ever see is trying to use a key like this as an adaptor name - but it validates the fix and proves that using a map rather than object is a better approach here
Then again, wouldn't hasOwnProperty here be a neater fix than using the map?
// autoinstall.ts
if (!context.versions[name]) {
context.versions[name] = [];
}
| job.linker ??= {}; | ||
| // @ts-ignore | ||
| job.linker[name] = paths[adaptor!]; | ||
| Object.defineProperty(job.linker, name, { |
There was a problem hiding this comment.
But constructor is perfeclyl safe to write to an object? Not sold we need this complication
| export const conditions: Record<string, string> = { | ||
| on_job_success: 'Boolean(!state?.errors?.[upstreamStepId] ?? true)', | ||
| on_job_failure: 'Boolean(state?.errors && state.errors[upstreamStepId])', | ||
| on_job_success: |
There was a problem hiding this comment.
I've just run tests on __proto__ and constructor as step names and the conditions seem to evaluate fine. This diff makes the compiled significantly harder to read (which is a problem when we ever get to debugging).
Are we sure this fix is needed?
| }, | ||
| }); | ||
|
|
||
| t.deepEqual(Object.keys(workflow.steps), ['__proto__']); |
There was a problem hiding this comment.
hmm this only fails on the object.keys call, which is a bit artificial. Runs actually seem to work perfectly well with __proto__ as a step name (and tbh I don't have a lot of sympathy for users wishing to do this!)
|
Thank you for the detailed review. I reverted both worker integration
timeout changes in 0bff71f and verified that the scripts again use the
original two-minute limit.
I understand your concern about the production relevance and
complexity of the broader changes. I will leave that scope on hold
while you and the original reporter discuss the issue. I have not
treated the existing green CI as agreement on the design or as
evidence that all the proposed changes are necessary.
M Ahmed
|
|
To help scope the follow-up when you and the original reporter are
ready, here are the specific decisions I need. No immediate reply is
expected; the broader changes remain on hold.
1. Production case: is there a supported input that produces incorrect
execution, lookup, or serialization? If so, please share a minimal
workflow/adaptor example, expected result, actual result, and kit
version. The synthetic key-enumeration test alone does not establish
production impact.
2. Identifier contract: must adaptor names always be
@openfn/language-*, and should reserved object-property names be
accepted as step IDs or rejected at validation?
3. Preferred scope: would you prefer a small own-property lookup fix
with regression coverage, validation at the input boundary, or no
change until a production reproducer exists? Should the Map and
compiled-output changes be removed to preserve serialization and
readable generated code?
A short reply in this form would be sufficient:
- Supported failing case / none established:
- Expected identifier rules:
- Preferred scope and files to retain/remove:
- Acceptance test or evidence required:
I can implement the agreed scope after that decision. The unrelated
timeout changes have already been reverted.
M Ahmed
|
2 participants
Short Description
Closes #1513.
Prevents workflow step, edge, adaptor, and linker identifiers such as
constructorand__proto__from colliding with inherited JavaScript object properties.Implementation Details
Mapinternally while preserving the existing object return type;QA Notes
One ws-worker Sentry timing assertion failed on the first broad run, passed in isolation, and the complete ws-worker rerun passed. It is outside the changed paths.
AI Usage
AI-assisted development tools supported issue analysis, implementation, regression testing, and verification. The submitted changes were validated through the test and build evidence above.