Skip to content
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

Chore: npm audit fix #8480

Closed
wants to merge 3 commits into from

Conversation

Julien-Marcou
Copy link
Member

@Julien-Marcou Julien-Marcou commented Jul 10, 2022

Description of change

Update the package-lock.json using npm audit fix to fix auto-fixable security issues.

Pre-Merge Checklist
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@Julien-Marcou
Copy link
Member Author

Julien-Marcou commented Jul 10, 2022

Looks like npm run webdoc is broken after this, I'm looking into it

All the errors were already there before the npm audit fix.

Here is an excerpt of the errors:

Parameter node couldn't be parsed
[DocParser]: Failed to parse doc for (@Unnamed)
Catharsis failed to parse: Object<string, string | ((mode: string) => void) | CSSStyleDeclaration>
Catharsis failed to parse: [number, number] | null
"scaleMode" is not a parameter & cannot come after the last parameter "height"
"resolution" is not a parameter & cannot come after the last parameter "scaleMode"

So, looks like everything is correct.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Webdoc is failing

Need some insight @ShukantPal

(node:4199) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'map' of undefined
    at mangled (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/model/lib/doc.js:296:50)
    at discoverMembers (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/parser/lib/transformer/mod-discover-members.js:53:45)
    at ensureDiscovered (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/parser/lib/transformer/mod-discover-members.js:21:3)
    at discoverMembers (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/parser/lib/transformer/mod-discover-members.js:81:5)
    at discover (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/parser/lib/transformer/mod-discover-members.js:116:3)
    at Object.discover [as mod] (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/parser/lib/transformer/mod-discover-members.js:120:7)
    at mod (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/parser/lib/transformer/document-tree-modifiers.js:66:22)
    at parse (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/parser/lib/parse.js:82:38)
    at async main (/home/runner/work/pixijs/pixijs/node_modules/@webdoc/cli/lib/index.js:136:5)

Related, I wish that webdoc threw an exit value of > 0, this crash does not fail the process, which it should IMO.

@Julien-Marcou
Copy link
Member Author

Ha, I missed that one, indeed that's a new error that wasn't there before.

Downgrading to @webdoc/cli 1.5.5 (the previously used version) doesn't fix the problem.

I wish that webdoc threw an exit value of > 0, this crash does not fail the process, which it should IMO.

I agree

@Julien-Marcou
Copy link
Member Author

Julien-Marcou commented Jul 10, 2022

Here are the lines that @webdoc/cli tries to parse when it crashes:

/**
* DisplayObject default updateTransform, does not update children of container.
* Will crash if there's no parent element.
* @memberof PIXI.DisplayObject#
* @method displayObjectUpdateTransform
*/
DisplayObject.prototype.displayObjectUpdateTransform = DisplayObject.prototype.updateTransform;

It's expecting the methods to have parameters when it mangles it, but the method has no parameter

@Julien-Marcou
Copy link
Member Author

Julien-Marcou commented Jul 10, 2022

We can fix the problem by downgrading to @webdoc/cli: 1.5.5 AND @webdoc/parser: 1.5.5.

Because @webdoc/parser: 1.6.0+ is a valid dependency for @webdoc/cli: 1.5.5, we have to pin down the version of both packages if we don't want the @webdoc/parser to be accidentally upgraded.

That being said, an issue has probably been introduced between @webdoc/parser: 1.5.5 and @webdoc/parser: 1.6.0 which needs to be fixed (I opened an issue: webdoc-labs/webdoc#182).

@bigtimebuddy
Copy link
Member

Preference would be downgrade while webdoc works on the issue.

@Julien-Marcou
Copy link
Member Author

I downgraded all webdoc packages to the v1.5.5, but I didn't pinned down the version of @webdoc/model to the v1.5.5, so another npm audit fix, npm update or npm update @webdoc/cli will update it again.

As it would only be a temporary solution to pin down the version, and that we can't add comments in the package.json, I didn't wanted to do it. What do you think?

@bigtimebuddy
Copy link
Member

Pinning in the package.json is okay to ~1.5.5, I'm not sure how long it will be fixed so make sure that's reflected in the dependencies is more helpful than just having it be in the lock.

@Julien-Marcou
Copy link
Member Author

With npm, we can't directly pin a sub-dependency.

With:

{
  "devDependencies": {
    "@webdoc/cli": "~1.5.5",
    "@webdoc/parser": "~1.5.5",
    "@webdoc/model": "~1.5.5",
  }
}

Running npm audit fix or npm update @webdoc/cli will still install @webdoc/model v1.6.6 in /node_modules/@webdoc/cli/node_modules.

So I'll have to add:

{
  "overrides": {
    "@webdoc/parser": "~1.5.5",
    "@webdoc/model": "~1.5.5"
  }
}

Which only works since NPM v8.3.0

@bigtimebuddy
Copy link
Member

There's a slight problem with this approach and requiring npm >= 8.3.0 See this version bumping script: https://github.com/pixijs/pixijs/blob/dev/package.json#L34

We use a specific version of npm@7 to do the version bump on the lock file (see #7396) to workaround a bug in Lerna, which looks like it may have been fixed (lerna/lerna#3091).

We probably need to update Lerna before we change this.

@Julien-Marcou
Copy link
Member Author

Yes, that's problematic, when a minor update on sub-dependency has a blocking issue, we had to add a lot of stuff to fix it on previous version of NPM.
That's why I didn't want to pin the version down at first (yes that doesn't fix the issue with webdoc so we would have to avoid npm update and npm audit fix until the fix is released).

The issue is already present on the repo, any npm update or npm audit fix will fetch the broken version of webdoc.

I opened this PR not only to fix some security issues, but also to "clean" the package-lock.json in order to upgrade Lerna next and have less changes in the package-lock.json.

Lerna v4 has a lot of deprecated dependencies and a security issue that can only be fixed by manually upgrading to Lerna v5.

But we can also do it the other way arround (upgrade lerna first and then run npm audit fix) and/or wait for the webdoc fix.

@ShukantPal
Copy link
Member

@ShukantPal
Copy link
Member

webdoc@2.0.0 also has no deprecated [transitive] dependencies too

@ghiscoding
Copy link

There's a slight problem with this approach and requiring npm >= 8.3.0 See this version bumping script: https://github.com/pixijs/pixijs/blob/dev/package.json#L34

We use a specific version of npm@7 to do the version bump on the lock file (see #7396) to workaround a bug in Lerna, which looks like it may have been fixed (lerna/lerna#3091).

We probably need to update Lerna before we change this.

Hello, I came across your issue by looking at the recent PR in Lerna, I could suggest an alternative which would be to use Lerna-Lite, it's a smaller and more modular fork of Lerna, I created the fork when Lerna was nearly dead and that was before Nrwl came over. The commands are the same and I keep PRs in sync with Lerna but I also added extra features which might be useful in your use case, I have this option lerna version --sync-workspace-lock which will update the lock by internally calling specific command for each client (for example npm > 8.5 would call npm install --package-lock-only but < 8.5 it would call npm shrinkwrap --package-lock-only and then do a file rename).

Also note that the recent version of Lerna, v5.2.0 and higher, will also install Nx (want it or not) because they made Nx as a dependency, however in Lerna-Lite Nx remains totally optional.

If this doesn't help with your use case then just please disregard my comment
Cheers and happy coding

@bigtimebuddy
Copy link
Member

I'm going to close this. I think we are ready for a new audit of dev since we've moved our v7 development there. Likely, we'll have a few more prereleases to iron-out any issues.

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.

None yet

4 participants