Skip to content

Upgrade yarn packages with CVEs #221

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 16, 2025
Merged

Upgrade yarn packages with CVEs #221

merged 7 commits into from
Jul 16, 2025

Conversation

cristianoventura
Copy link
Contributor

@cristianoventura cristianoventura commented Jul 15, 2025 •
edited
Loading

What?

This PR upgrades packages that contain CVEs as reported in #214.

Most of the packages are related to transitive dependencies from uplot and lerna. Upgrading uplot to a patched version required some code changes, as the newest one contains changes on how the legends in the charts are rendered.

Screen.Recording.2025-07-15.at.2.12.26.PM.mov

Before this change:

$ cd ./dashboard/assets
$ yarn audit

80 vulnerabilities found - Packages audited: 720
Severity: 25 Low | 30 Moderate | 23 High | 2 Critical
✨  Done in 1.82s.

After this change:

$ cd ./dashboard/assets
$ yarn audit

0 vulnerabilities found - Packages audited: 657
✨  Done in 0.68s.

Why?

Direct and indirect dependencies have CVEs.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (mage lint) and all checks pass.
  • I have run tests locally (mage test) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

#214

@cristianoventura cristianoventura self-assigned this Jul 15, 2025
@CLAassistant
Copy link

CLAassistant commented Jul 15, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

cristianoventura
cristianoventura commented Jul 15, 2025
View reviewed changes
dashboard/assets/packages/report/src/components/Chart/Chart.utils.ts
@@ -70,7 +70,7 @@ export const createOptions = ({ plot, width }: CreateOptionsProps): Options => {
width: width,
height: 250,
cursor: { sync: { key: sync.key } },
legend: { live: false },
legend: { live: true },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's been a change in the newest version related to how the legend is handled. In order to have it visible on hover, we need this option set to true.

leeoniya/uPlot@1.6.30...1.6.31#diff-6f581b44485db1123ab1cc1b28b05d08b22ddb46d7ad1d19252acb9f15debf3dR2627-R2632

dashboard/assets/packages/ui/src/App/App.css.ts
Comment on lines +18 to +20
globalStyle('.u-legend', {
height: '55px',
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the legends are now dynamic on hover, it's possible that it breaks into tw lines, so having more height here enables support for smaller windows without pushing the content on the page.

Screen.Recording.2025-07-15.at.2.21.05.PM.mov

dashboard/assets/packages/ui/src/components/Chart/Chart.hooks.ts
Comment on lines +44 to +52
const handleRemoveTimeLegend = (uplot: uPlot) => {
const legendTable = uplot.root.querySelector('.u-legend')
if (legendTable) {
const firstRow = legendTable.querySelector('tr')
if (firstRow) {
firstRow.style.display = 'none'
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, the newest version of uplot includes a time legend. I've decided to remove it via the ready hook to keep the UI as close as possible to the previous version.

@cristianoventura cristianoventura requested a review from 2Steaks July 15, 2025 18:25
@cristianoventura cristianoventura marked this pull request as ready for review July 15, 2025 18:25
@cristianoventura cristianoventura requested a review from a team as a code owner July 15, 2025 18:25
@cristianoventura cristianoventura requested review from szkiba and removed request for a team July 15, 2025 18:25
cristianoventura
cristianoventura commented Jul 15, 2025
View reviewed changes
dashboard/assets/packages/report/package.json
@@ -36,7 +36,7 @@
"eslint-plugin-prettier": "^5.0.1",
"rollup-plugin-visualizer": "^5.9.2",
"sass": "^1.65.1",
"vite": "^4.4.11",
"vite": "^4.5.14",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vite also contains multiple CVEs as reported in:

GHSA-vg6x-rcgg-rjx6
GHSA-92r3-m2mg-pj97
GHSA-356w-63v5-8wf4
GHSA-859w-5945-r5v3
GHSA-8jhw-289h-jh2g
GHSA-9cwx-2883-4wfx
GHSA-x574-m823-4x7w
GHSA-4r4m-qw57-chr8
GHSA-xcj6-pq6g-qj4x

@cristianoventura cristianoventura mentioned this pull request Jul 15, 2025
Closed
5 tasks
2Steaks
2Steaks approved these changes Jul 16, 2025
View reviewed changes
Copy link
Collaborator

@2Steaks 2Steaks left a comment

Choose a reason for hiding this comment

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

Awesome stuff! 🙌

szkiba
szkiba requested changes Jul 16, 2025
View reviewed changes
Copy link
Contributor

@szkiba szkiba left a comment

Choose a reason for hiding this comment

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

Thanks, this is fantastic!

It seems that the new version does not take into account the desktop default dark mode setting. That is, if the user's preferred mode is dark mode, it will still start in light mode. You can then switch. (I tried with several browsers)

Is there a way to make it take into account the default dark mode setting like before?

@cristianoventura
Copy link
Contributor Author

Thanks, this is fantastic!

It seems that the new version does not take into account the desktop default dark mode setting. That is, if the user's preferred mode is dark mode, it will still start in light mode. You can then switch. (I tried with several browsers)

Is there a way to make it take into account the default dark mode setting like before?

@szkiba Good catch! I noticed this behaviour in the older version as well. However, I've added a fix to address that problem going forward 5bf7d79

  • The page will respect the OS preference until the theme is manually changed by the user
  • Once the theme is manually changed by the user, the page theme will give precedence to that choice instead
Screen.Recording.2025-07-16.at.2.08.04.PM.mov

@szkiba szkiba self-requested a review July 16, 2025 18:36
szkiba
szkiba approved these changes Jul 16, 2025
View reviewed changes
Copy link
Contributor

@szkiba szkiba left a comment

Choose a reason for hiding this comment

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

LGTM

@szkiba szkiba merged commit 44ee7ab into master Jul 16, 2025
31 checks passed
@szkiba szkiba deleted the task/upgrade-dashboard-cves branch July 16, 2025 18:46
@szkiba szkiba mentioned this pull request Jul 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@2Steaks 2Steaks 2Steaks approved these changes

@szkiba szkiba szkiba approved these changes

Assignees

@cristianoventura cristianoventura

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cristianoventura @CLAassistant @2Steaks @szkiba