search

GoogleChrome / lighthouse-ci

Automate running Lighthouse for every commit, viewing the changes, and preventing regressions
Apache License 2.0
6.41k stars 644 forks source link

chore(server,security): upgrade sequelize and umzug #878

Closed jorisdugue closed 9 months ago

jorisdugue commented 1 year ago

Hi 👋

close #874 #907

jorisdugue commented 1 year ago

HI @paulirish,

The last error is due to chrome missing path If you have any idea about it 👀 (I didn't change this part of the code)

paulirish commented 1 year ago

merged in main and resolved the conflicts. we'll see how this goes....

paulirish commented 1 year ago

@jorisdugue thanks for trying to get this in a good state. it's clearly not a walk in the park. :) appreciate it!

rdmulford commented 1 year ago

hi, wondering if there are any updates on this?

jorisdugue commented 1 year ago

hi, wondering if there are any updates on this?

@rdmulford hey, the code is functional but the unit tests do not work because of an error related to the WS

From what I could see having done quite a bit of testing. Even if I took the main branch for the tests, these will not pass because currently only the job cache allows the tests to work...

I looked into the problem currently the problem is related to puppeter and storyshot (related to JEST)

I tried to integrate the storybook in 7.X this one works but I also have the error related to the WS so it is not very reassuring in itself.

Now that puppeter has also evolved, we get warnings in all corners also because of the healess which has changed quite a bit (an integration is also available in the PRs #907 )

jorisdugue commented 1 year ago

Hey,

Ok, after a few months of research to understand where the problem came from. I finally managed to figure out where the problem with the ws came from.

To do this, you just had to activate the debug of the "remote-pipe".

I also took advantage of updating the v6 storybook to v7 after fixing a lot of security issue. I also upgrade lighthouse to 10.4.0

I also grabbed the https://github.com/GoogleChrome/lighthouse-ci/pull/907 branch to reduce message issues. On the other hand, some changes were missing for the headless, but this is now fixed.

@paulirish

This issue can be close after 😄 #874

jorisdugue commented 1 year ago

Is there any chance this will be reviewed and pushed? @paulirish

HofmannZ commented 12 months ago

Security should be high priority. I don't understand why this is open for months 😕

MrCrunchwrap commented 11 months ago

@paulirish - Any chance of this being merged? This dep is pretty far out of date and is being flagged by security scans.

jmccann commented 9 months ago

Just curious what is holding up this PR. It seemed to be of interest earlier by a maintainer but since the issues have been worked through there has been no response from a maintainer.

cc @paulirish and @adamraine as I've seen you both are most recent people to have merged some PRs.

Is there any direction to being able to get this merged? I'm not very familiar with the scope required to upgrade sequelize.

Personally, looking at this PR I'm wondering if there was scope creep. e.g., where all the other updates required in order to update sequelize? @jorisdugue 

Upgrade Umzug 2.2 to 3.2 for same reason of Sequelize
Upgrade js-yaml 3.13.1 to 4.1
Upgrade puppeteer
Upgrade the storybook v6 to v7 (for reduce the vuln again)
Upgrade lighthouse to 10.4
Upgrade TS 4.X to TS 5.X

Or could we have more smaller PRs for this work to make it easier to review? I know personally I wouldn't want to review this large of a PR even though I'd really like to see sequelize updated. 😅

jorisdugue commented 9 months ago

@jmccann The basic update was to reduce the critical vulnerabilities of the project for information we are talking about more than 100 critical vulnerabilities flaws. And other updates during this time I had to maintain the branch several times in relation to the basic work but in any case the next updates of puppeteer are in UMD therefore not compatible in the state of the project x)

connorjclark commented 9 months ago

FWIW, I looked through the sequelize security reports and it seems we don't use any of the features that were reported to be broken.

I'll go through some of these same upgrades as separate PRs today. Closing this PR, because the scope of these changes is too large and we need to be more piecemeal about how we land things. Regardless, thank you @jorisdugue, especially for the Sequelize major upgrades, I'm sure that will come in handy as reference when I do the upgrade in a new branch.