fix(deps): upgrade swagger-ui-react v5.17.3, react-dom v18.3.1, and react v18.3.1. Fixes CVEs #13069
Conversation
Signed-off-by: Yuan Tang <[email protected]>
Signed-off-by: Yuan Tang <[email protected]>
terrytangyuan
changed the title
| "react": "^18.3.1", | ||
| "react-chartjs-2": "^2.11.2", | ||
| "react-datepicker": "^4.25.0", | ||
| "react-dom": "^16.14.0", | ||
| "react-dom": "^18.3.1", |
There was a problem hiding this comment.
Followed https://react.dev/blog/2022/03/08/react-18-upgrade-guide
yarn add react react-dom
Signed-off-by: Yuan Tang <[email protected]>
| import {createRoot} from 'react-dom/client'; | ||
|
|
||
| ReactDOM.render(<App />, document.getElementById('app')); | ||
| const container = document.getElementById('app'); | ||
| const root = createRoot(container!); | ||
| root.render(<App />); |
There was a problem hiding this comment.
Signed-off-by: Yuan Tang <[email protected]>
There was a problem hiding this comment.
dep overrides aka resolutions
So I mentioned on Slack that this needs a few overrides; upgrading React by two major versions is not what I meant 😕
Notably, I did not update swagger-ui-react past v5.10.x before in #12540 very intentionally due to its compatibility issues.
Since then, the issue I linked there, swagger-api/swagger-ui#9495, was updated with a docs page https://github.com/swagger-api/swagger-ui/wiki/Using-older-version-of-React that we could follow for compatibility. That doc lists what dep overrides are necessary, and that was what I meant.
I mentioned that I hadn't fully tested this locally yet in Slack either -- I was getting this error swagger-api/swagger-ui#9929 at the time, which may require another override.
patch isolation, breaking major updates
As a best practice, we should try and isolate patches, especially security patches, as much as possible, in general.
Bumping another dep by two major versions, especially the foundational UI framework, React, has little isolation to it as it would affect the entire UI either directly or indirectly.
I would suggest that such a foundational, breaking upgrade really get a thorough review by a UI SME as well, which this did not.
The "Verification" section, as is required in the PR template, which is in general important for the UI (where we lack many tests and visual nuances are relevant), especially for such a bump, is missing here as well.
We had not upgraded before due to several incompatibilities. We already had been using deprecated functionality -- off the top of my head, two notable ones I can think of include:
- refactor(ui): convert
WorkflowsList+WorkflowsFilterto functional components #11891 and a few prior refactors of mine removedBasePage, which used the legacy, undocumented context API, which was only promised to work with React 16 and would be removed in future breaking changes. - Argo UI still has many class components. In particular, several were using unsafe lifecycle methods which I only recently updated to use the non-deprecated
UNSAFE_prefix in refactor(deps): migrate from archivedtslinttoeslintargo-ui#509. The deprecated, unprefixed variants were similarly only promised to work with React 16 and were removed in React 17.- I also only recently updated
argo-uito a version with those changes in build(ui): improve performance withesbuild-loader#12516. That was merged less than a week before this PR was.
- I also only recently updated
So if upgrading React by two major versions works, it is almost entirely coincidence due to refactoring work I had been doing many months prior to make such an upgrade possible.
Notably, such coincidences mean that there is no way to backport this cleanly.
You also wanted to backport this to Argo 3.4 in #11851 (comment); outside of the issues above, Argo 3.4 is itself only on swagger-ui-react v4 as #12540 was never backported.
EDIT: Actually, Argo 3.5 is also on swagger-ui-react v4 so backporting this to 3.5 per #11997 (comment) is difficult for that reason as well
I hope all the careful refactoring and months-long changes I had been making to prepare for such an upgrade, as well as all the incompatibilities that may result from attempting to backport this PR, illustrate why such breaking upgrades to foundational frameworks need significant diligence & review around them.
CVE has minimal impact, would prefer to remove swagger-ui-react entirely tbh
swagger-ui-react is only used on a single page in the UI, to load the API docs. As of #12061, it isn't even loaded until a user navigates to that page.
As such, a CVE in swagger-ui-react is of minimal effect to Argo users in general.
These particular CVEs are in deps of deps of deps (and so on) of swagger-ui-react and are not really exploitable as such (maybe with a malicious swagger doc, which would require a few other exploits to be able to do, and even then is only a maybe).
I'm also pretty sure they are in devDeps (patch-package is a devDep) and so are false positives too per swagger-api/swagger-ui#8288 (comment) and swagger-api/swagger-ui#9909
Since swagger-ui-react is such a big dep with complex deps of its own (that have also had CVEs more than once, e.g. #12470, #12058) and has various efficiency issues per #6645 et al, I've actually been thinking of entirely removing the API Docs page to reduce our complexity & deps as well as improve the speed of installation and build.
Users can still use the API reference in the docs (which is aided now by versioned docs #11390) and if they want the specific version of their Server, could download it from the API and load it locally in their own editor etc.
IMO, removing the dep/page entirely would be a more stable change to backport than bumping React by two majors.
But we could also ignore the CVE as not exploitable and/or false positive.
| const container = document.getElementById('app'); | ||
| const root = createRoot(container!); |
There was a problem hiding this comment.
| const container = document.getElementById('app'); | |
| const root = createRoot(container!); | |
| const root = createRoot(document.getElementById('app')!); |
we can probably simplify this, the container var is otherwise unused
agilgur5
added
type/security
|
The React upgrade also produced a bunch of new install warnings too: I'm not sure if all of those are safely ignoreable. Some of them are certainly fixable though ( EDIT: also lots of browser console warnings: |
The React 18 upgrade in argoproj#13069 introduced a bug on the details page where the "Submit" button is disabled immediately on page load. Specifically, I believe this is happening due to batching changes that affect the following two `useEffect()` calls, which are common to all the details pages modified in this PR: ``` useEffect(() => { (async () => { try { const newEventSource = await services.eventSource.get(name, namespace); setEventSource(newEventSource); setEdited(false); // set back to false setError(null); } catch (err) { setError(err); } })(); }, [name, namespace]); useEffect(() => setEdited(true), [eventSource]); ``` The first runs immediately and invokes `setEventSource(newEventSource)`, which causes the second `useEffect()` call to be fired, since the `eventSource` dependency has changed. Since both are invoking `setEdited()`, this is basically a race condition, since the state of `edited` is going to depend on whether these calls are batched together are fired separately. This PR fixes that by removing the second `useEffect()` call, which eliminates the race condition. Instead, we only call `setEdited(true)` when the editor is modified. Signed-off-by: Mason Malone <[email protected]>
The React 18 upgrade in argoproj#13069 introduced a bug on the details page where the "Submit" button is disabled immediately on page load. Specifically, I believe this is happening due to batching changes that affect the following two `useEffect()` calls, which are common to all the details pages modified in this PR: ``` useEffect(() => { (async () => { try { const newEventSource = await services.eventSource.get(name, namespace); setEventSource(newEventSource); setEdited(false); // set back to false setError(null); } catch (err) { setError(err); } })(); }, [name, namespace]); useEffect(() => setEdited(true), [eventSource]); ``` The first runs immediately and invokes `setEventSource(newEventSource)`, which causes the second `useEffect()` call to be fired, since the `eventSource` dependency has changed. Since both are invoking `setEdited()`, this is basically a race condition, since the state of `edited` is going to depend on whether these calls are batched together are fired separately. This PR fixes that by removing the second `useEffect()` call, which eliminates the race condition. Instead, we only call `setEdited(true)` when the editor is modified. Signed-off-by: Mason Malone <[email protected]>
The React 18 upgrade in argoproj#13069 introduced a bug on the details page where the "Submit" button is disabled immediately on page load. Specifically, I believe this is happening due to batching changes that affect the following two `useEffect()` calls, which are common to all the details pages modified in this PR: ``` useEffect(() => { (async () => { try { const newEventSource = await services.eventSource.get(name, namespace); setEventSource(newEventSource); setEdited(false); // set back to false setError(null); } catch (err) { setError(err); } })(); }, [name, namespace]); useEffect(() => setEdited(true), [eventSource]); ``` The first runs immediately and invokes `setEventSource(newEventSource)`, which causes the second `useEffect()` call to be fired, since the `eventSource` dependency has changed. Since both are invoking `setEdited()`, this is basically a race condition, since the state of `edited` is going to depend on whether these calls are batched together are fired separately. This PR fixes that by removing the second `useEffect()` call, which eliminates the race condition. Instead, we only call `setEdited(true)` when the editor is modified. Signed-off-by: Mason Malone <[email protected]>
|
Follow-ups on Slack and in #11997 (comment) |
This fixes CVEs: https://security.snyk.io/vuln/SNYK-JS-MICROMATCH-6838728, https://security.snyk.io/vuln/SNYK-JS-BRACES-6838727