Skip to content

fix(security): swapping out jsonpath-plus for jsonpath in v20#942

Merged
erunion merged 1 commit intooas-v20from
fix/v20-vulnerabilities
Feb 24, 2025
Merged

fix(security): swapping out jsonpath-plus for jsonpath in v20#942
erunion merged 1 commit intooas-v20from
fix/v20-vulnerabilities

Conversation

@erunion
Copy link
Member

@erunion erunion commented Feb 23, 2025

🚥 Towards readmeio/api#969

🐳 Context

The v20 series of this library, which v6 of api notably uses, has a vulnerability in its dependency on jsonpath-plus.

We have already upgraded jsonpath-plus in later releases of oas to resolve this problem but because those newer versions require Node 18+ and v20 of oas is still on Node 14+ we can't bump it. We have also resolved this problem in the still-in-beta release of api in the v7 release but due to a remaining list of issues to resolve and time constraints we can't pull that out of its next channel.

Instead I'm reaching for the last option here and it's to fix the problem in our v20 series directly.

🧰 Changes

Because I can't upgrade jsonpath-plus within v20, as that would be a breaking change, I'm instead opting for the shittier solution of swapping it out for jsonpath. jsonpath has no dependencies, has not been updated in 4 years, but it does not have any open vulnerabilities and can mostly handle the our analyzer queries.

With the move here to jsonpath we lose the ability to do backreference parent and property queries, which unfortunately means that the query results will be a little different but the queries still generally point to the same place.

I don't really like this solution because jsonpath is severely limited when compared to jsonpath-plus but given that oas@20 is still in use in api@6 and rdme@81 this should at least stem the tide of us receiving reports about it showing up in npm audit.

🧬 QA & Testing

Check out the test assertion changes for the query result differences. They're still mostly the same, just slightly different.

Footnotes

  1. rdme@8 has long been deprecated but according to NPM it's still received 47k downloads in the last 7 days. 🙃

@erunion erunion added the security Pull requests that address a security vulnerability label Feb 23, 2025
@erunion erunion requested a review from kanadgupta February 23, 2025 02:40
"$..requestBody..['text/xml']",
"$..requestBody..['text/xml-external-parsed-entity']",
'$..requestBody.content[?(@property.match(/\\+xml$/i))]',
// '$..requestBody.content[?(@property.match(/\\+xml$/i))]',
Copy link
Member Author

Choose a reason for hiding this comment

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

jsonpath doesn't support @property.match queries.

Copy link
Contributor

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

happy with this! only feedback: can you rename the v20 branch to oas-v20 or something so it's explicitly clear which subpackage it's for?

rdme@8 has long been deprecated but according to NPM it's still received 47k downloads in the last 7 days. 🙃

regarding the above, rdme@9 has only been out since december and isn't actually deprecated on npm so i'm not too worried. it makes sense why we're doing this for api@6, but rdme@9 was designed to be a seamless upgrade for nearly all rdme@8 users. i'm pretty opposed to backchanneling any changes for rdme@8, that's why we maintain rdme@9 in the first place. writing a good deprecation message for rdme@8 and earlier on npm should help get those numbers up.

@erunion erunion changed the base branch from v20 to oas-v20 February 24, 2025 17:53
@erunion erunion merged commit fd202ab into oas-v20 Feb 24, 2025
5 checks passed
@erunion erunion deleted the fix/v20-vulnerabilities branch February 24, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security Pull requests that address a security vulnerability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments