Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Moddle): replicate set/get API for createAny elements #54

Merged
merged 2 commits into from May 4, 2023

Conversation

marstamm
Copy link
Contributor

@marstamm marstamm commented May 3, 2023

@marstamm marstamm requested a review from a team May 3, 2023 17:36
@marstamm marstamm self-assigned this May 3, 2023
@marstamm marstamm requested review from philippfromme and barmac and removed request for a team May 3, 2023 17:36
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label May 3, 2023
Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment below. Let's also test for prototype pollution.

lib/moddle.js Outdated
return this[name];
},
set: function(name, value) {
this[name] = value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leads to prototype pollution:

~/workspace/bpmn-io/moddle replicate-api-on-createAny 7s
❯ node
Welcome to Node.js v18.12.1.
Type ".help" for more information.
> const Moddle = require('.')
undefined
> Moddle
{
  Moddle: [Function: Moddle],
  coerceType: [Function: coerceType],
  isBuiltInType: [Function: isBuiltIn],
  isSimpleType: [Function: isSimple],
  parseNameNS: [Function: parseName]
}
> const model = new Moddle.Moddle([])
undefined
> const a = model.createAny('test', 'http://example.com')
undefined
> a.set('__proto__', { hacked() { console.log('hacked') } })
undefined
> a.hacked()
hacked
undefined

Cf. min-dash#set for mitigation: https://github.com/bpmn-io/min-dash/blob/master/lib/object.js#L40
We could also reuse min-dash method if we make sure name is string and not array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use min-dash#set here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Addressed with 118b5fd by using min-dash

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels May 4, 2023
set: function(name, value) {
this[name] = value;
set: function(key, value) {
set(this, [ key ], value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@marstamm marstamm requested a review from barmac May 4, 2023 07:39
@fake-join fake-join bot merged commit 80fe511 into master May 4, 2023
3 checks passed
@fake-join fake-join bot deleted the replicate-api-on-createAny branch May 4, 2023 08:03
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label May 4, 2023
@marstamm
Copy link
Contributor Author

marstamm commented May 4, 2023

Thanks for the review, I'll take care of the patch release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants