Automattic / notifications-panel

Core notifications panel for WordPress.com notifications
0 stars 1 forks source link

Dependencies: Upgrade i18n-calypso and wpcom-proxy-request #282

Closed codebykat closed 6 years ago

codebykat commented 6 years ago

as per


┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ debug                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ i18n-calypso [dev]                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ i18n-calypso > debug                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/534
gwwar commented 6 years ago

🤔 Calypso is also on 1.8.4, which doesn't have this issue. I wonder if it's because we're missing the "dependencies" prop on package.json that we're seeing odd status messages from github otherwise we could target the moderate warnings first.

codebykat commented 6 years ago

Investigation results for the remaining issues:

Of the 13 vulnerabilities reported by npm audit, 9 are related to node-sass:

The remaining 4 are dependencies of our other packages:

codebykat commented 6 years ago

I wonder if it's because we're missing the "dependencies" prop on package.json that we're seeing odd status messages from github otherwise we could target the moderate warnings first.

Hmmmm so when I upgraded wpcom-proxy-request it told me to also upgrade webpack and ajv; somewhere in there it did add the dependencies prop, but with only ajv in it.

Should we pull in all the existing dependencies from package.lock? I glanced at the prop in Calypso and there's quite a lot in there. Is there a way to generate it or something?

codebykat commented 6 years ago

@dmsnell if you have a second maybe you can re-review / answer the above? :)

dmsnell commented 6 years ago

Should we pull in all the existing dependencies from package.lock? I glanced at the prop in Calypso and there's quite a lot in there. Is there a way to generate it or something?

@codebykat we should be able to delete that file and the node_modules directory and then run npm install - it should regenerate it and that's a common way to resolve issues like this.

dmsnell commented 6 years ago

on the further point I think we should be fine doing whatever updates we need if they don't involve too much work. depending on what webpack update it wants that could mean easy or hard.

within the next two weeks I'd like to see if I can't merge this codebase into Calypso and then in that case we'd end up blowing away all this duplicate maintenance. point being: don't spend too much time on updating packages or config files; if it's an easy quick win then 💯 go for it, but if not then I'll see if we can't just eliminate the effort entirely 👍

codebykat commented 6 years ago

Updated with a new package-lock.json. Guess I'll merge and see what happens :)