Skip to content

fix(deps): update dependency jsonpath-plus to 10.0.0 due to vulnerability #379

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

Closed
wants to merge 1 commit into from

Conversation

knowacki23
Copy link

This is a simple dependency bump of the jsonpath-plus dependency.

This dependency is vulnerable according to Snyk: https://security.snyk.io/vuln/SNYK-JS-JSONPATHPLUS-7945884
As this is a non-major upgrade and I have not seen the vulnerability being mentioned anywhere in the repository I thought I'd open a PR for it.

…lity

Signed-off-by: Nowacki, Kacper <kacper.nowacki@dynatrace.com>
@mehtaruchir
Copy link

Any plans to release a new version w.r.t this PR.

@Fraraven
Copy link

We would also appreciate a new release
Thanks a lot

@chris-pardy
Copy link
Collaborator

chris-pardy commented Oct 15, 2024

@knowacki23 there's a pretty risky change releasing this without doing a major version bump for json-rules-engine.
The risk is that with we're currently on version 7 of this library but so we'd pick up breaking changes from version 8, 9, and 10. Some things of note:

  • We don't specify an engine currently in the package.json but we test on 14, 16, and 18. Going to version 10 would drop support for node version below 18.
  • When moving to a "safe eval" in browsers (version 9) and node (version 10) only a subset of javascript is supported for the selector functions. That means that some json path expressions may break (unless we explicitly mark that we want to keep using native eval)

Overall I'm not opposed to doing this with a new major version of JSON-Rules engine. However I think a better approach would be to codify a proposal for functions and move the JSON path implementation into a function. That would let us move JSON Path to a peer dependency and not worry as much about breaking changes like this in the future.

That could look something like this:

{
    "fn": "jsonpath",
    "params": {
          "path": "$.my.path",
          "value": { "fact": "factAsObject" } 
    }
}

@fdiazcobos
Copy link

We would also appreciate a new release, due the vulnerability

@knowacki23
Copy link
Author

Hey @chris-pardy,
Thank you for looking into that and for the explanation.
Should I then create an issue ticket for that?

@AkshayKulkarni03
Copy link

jsonpath-plus does mention that version 10.0.0

Require Node 18+
cause of that node 14 might be failing
also node 14 is quite old now , may be it can be looked differently

@chris-pardy chris-pardy added the next-major issue best resolved in next major breaking change label Oct 17, 2024
@chris-pardy chris-pardy added this to the version 7 milestone Oct 17, 2024
@chris-pardy
Copy link
Collaborator

@knowacki23 I'm going to flag this for version 7. In the meantime you should be able to use overrides (https://docs.npmjs.com/cli/v9/configuring-npm/package-json#overrides) in your package manager of choice to make that upgrade locally. Since the API isn't changing in version 10 it should be fine to do otherwise.

@brianphillips
Copy link
Contributor

I'm going to flag this for version 7.

Personally, I think bundling a change like this with what looks to be somewhat ambitious changes (at least looking at the v7 milestone) isn't ideal. As you state, the package is API-compatible with older versions so this shouldn't require any changes on users of json-rules-engine except that (as you also state) this new version would not be usable to those still on older versions of node.

Also, this might be controversial but I would recommend moving the supported/tested node versions up to include more recent versions as even v18 has only 6 months of security updates remaining before it also reaches the EOL date (v14 and v16 are already well beyond their EOL dates).

Thanks for your work on this package!

@chris-pardy
Copy link
Collaborator

If this was literally just the Node version thing I would agree with you except it's not. It's the node version and the fact that the new version of the json path plus library supports only a subset of the filters. We can't know the full-extent of the impact that will have without rolling it out, and it's not safe to do so.

The easiest thing to do would be not to push this into a large "version 7 with a bunch of other stuff" release but rather just do the major version change, update the library and call it a day.

I'm going to flag this for version 7.

Personally, I think bundling a change like this with what looks to be somewhat ambitious changes (at least looking at the v7 milestone) isn't ideal. As you state, the package is API-compatible with older versions so this shouldn't require any changes on users of json-rules-engine except that (as you also state) this new version would not be usable to those still on older versions of node.

Also, this might be controversial but I would recommend moving the supported/tested node versions up to include more recent versions as even v18 has only 6 months of security updates remaining before it also reaches the EOL date (v14 and v16 are already well beyond their EOL dates).

Thanks for your work on this package!

@CacheControl
Copy link
Owner

just do the major version change, update the library and call it a day.

@chris-pardy I agree and looks like this is a priority for a lot of folks. Happy to do this myself but sounds like you have a plan in your head for the next release(s) and I don't want to muddle things - do you mind?

@chris-pardy
Copy link
Collaborator

just do the major version change, update the library and call it a day.

@chris-pardy I agree and looks like this is a priority for a lot of folks. Happy to do this myself but sounds like you have a plan in your head for the next release(s) and I don't want to muddle things - do you mind?

The biggest thing I had hoped to get out with version 7 is a real type-script port and some other typing fixes to make it easier to add new features in the future. Honestly the current state of the world there is that while I still want to do it I'll push this into v7 #385 and then turn that into a real release and build a v8 branch for the other breaking changes.

@chris-pardy
Copy link
Collaborator

Moved to #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-major issue best resolved in next major breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants