-
Notifications
You must be signed in to change notification settings - Fork 492
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
Conversation
…lity Signed-off-by: Nowacki, Kacper <kacper.nowacki@dynatrace.com>
Any plans to release a new version w.r.t this PR. |
We would also appreciate a new release |
@knowacki23 there's a pretty risky change releasing this without doing a major version bump for json-rules-engine.
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" }
}
} |
We would also appreciate a new release, due the vulnerability |
Hey @chris-pardy, |
jsonpath-plus does mention that version 10.0.0 Require Node 18+ |
@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. |
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 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! |
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.
|
@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. |
Moved to #385 |
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.