Skip to content

Conversation

@jdleesmiller
Copy link
Contributor

🚥 Resolves #849

🧰 Changes

Use of lodash per method packages is now discouraged (docs), and they appear to be unmaintained and have security vulnerabilities. As explained in the linked docs, depending on lodash is preferred.

🧬 QA & Testing

The tests on the api package passed locally.

@jdleesmiller jdleesmiller changed the title Use lodash instead of per method packages chore: Use lodash instead of per method packages Feb 23, 2024
@kanadgupta kanadgupta changed the title chore: Use lodash instead of per method packages chore(deps): use lodash instead of per method packages Mar 1, 2024
@TBonnin TBonnin mentioned this pull request Mar 5, 2024
3 tasks
TBonnin added a commit to NangoHQ/nango that referenced this pull request Mar 5, 2024
jobs depends on the api package to make request to render api This
package suffers from a vulnerability of one of its dependency,
specifically the lodash.setWith package which is actually deprecated.
There is a PR open for api to use full lodash instead of per method
packages (which are deprecated) but it has not been merged yet.
readmeio/api#859

This commit replaces the api package used to generate a render sdk from
their openapi spec by a home-made RenderAPI class (40 lines of code)
TBonnin added a commit to NangoHQ/nango that referenced this pull request Mar 6, 2024
jobs depends on the `api` package to make request to render api This
package suffers from a vulnerability of one of its dependency,
specifically the lodash.setWith package which is actually deprecated.
There is a PR open for `api` to use full lodash instead of per method
packages (which are deprecated) but it has not been merged yet.
readmeio/api#859

This commit replaces the `api` package used to generate a render sdk
from their openapi spec by a home-made RenderAPI class (40 lines of
code)

## Issue ticket number and link

https://linear.app/nango/issue/NAN-453/[credal]-fix-jobs-vulnerability

## Checklist before requesting a review (skip if just adding/editing
APIs & templates)
- [ ] I added tests, otherwise the reason is: 
- [ ] I added observability, otherwise the reason is:
- [ ] I added analytics, otherwise the reason is:
@jdleesmiller
Copy link
Contributor Author

Thanks for approving the CI runs. It looks like a generic CI failure... which I am not sure what to do with. It is possible that I have broken something in lerna by not using lerna to remove the package?

@erunion erunion requested review from erunion and kanadgupta March 11, 2024 16:30
@kanadgupta
Copy link
Contributor

kanadgupta commented Mar 11, 2024

Hi! See lodash/lodash#5107, specifically this part:

This doesn’t work with modules (.mjs files) in NodeJS. Instead, lodash needs to be imported as a default import.

The api subpackage (e.g., ./packages/api) is ESM-only. I tried swapping this out for lodash-es and it appears to work as expected.

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.

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NPM Audit Report Vulnerability due to lodash.setwith

2 participants