Playbooks support for React 18

[jgheithcock]

Summary

This is the master version of React18 fixes for Playbooks.

As @JulienTant pointed out, this isn't really connected to the conditional work and we will want to get this in independent of conditional playbooks.

The big change is to use the mattermost repo of our fork of react-bootstrap. Note that the forked version still uses this ReactDOM.unstable_renderSubtreeIntoContainer method and so generates a number of warnings.. (@hmhealey fyi - please let me know if I've got this wrong)

UPDATE: @calebroseland points out that this is likely due to our continued use of react-bootstrap version of Tooltip and OverlayTrigger). We are too close to release however to swap those out. (Reviewers please comment if you disagree).

Ticket Link

N/A

Checklist

• Gated by experimental feature flag
• Unit tests updated

[dryrunsecurity]

This pull request introduces code that relies on a globally exposed third-party library (ReactBootstrap) fetched from window/global instead of as a bundled npm dependency, creating a client-side supply chain risk where a malicious script could replace the global and compromise components. While not marked as blocking, this dependency on a global increases attack surface (e.g., via XSS or compromised third-party scripts) and should be replaced with a direct, versioned dependency or otherwise hardened.

Reliance on globally exposed third-party library in webapp/src/externals/react-bootstrap.ts

Vulnerability

Reliance on globally exposed third-party library

Description

The application retrieves ReactBootstrap from the global window or global object rather than bundling it as a direct npm dependency. This creates a dependency on an external script that must be loaded into the global scope. If this global object can be manipulated by another script (e.g., through an XSS vulnerability or a compromised third-party script loaded on the same page), an attacker could replace the legitimate ReactBootstrap components with malicious ones, leading to a client-side supply chain attack.

mattermost-plugin-playbooks/webapp/src/externals/react-bootstrap.ts

Lines 10 to 12 in ab7159e

	if (typeof window !== 'undefined' && (window as any).ReactBootstrap) {
	return (window as any).ReactBootstrap;
	}

---

All finding details can be found in the DryRun Security Dashboard.

[hmhealey]

I'm not so sure about the way that we're doing this since we're faking React Bootstrap during development and then manually getting the library from the window instead of letting Webpack handle that for us. What's the reason for switching away from defining it as a Webpack external?

Sharing dependencies between the web app and plugins is definitely a tricky subject since we've not been able to invest in a better system for doing that (we still have some code around to enable module federation from an attempt to do that a few years ago), so I'd like to understand if there's something making it difficult to use the existing Webpack-based system for that.

Edit: On a whim, I tried adding the forked version of React Bootstrap to this, and I found out that there's an error because of how I was patching packages in that repo. I guess that's a sign that we really shouldn't be patching packages in a library we're intending to use elsewhere since having one dependency mess with other dependencies in node_modules seems like a disaster. I'll submit a fix for that for the time being, and we'll have to look at sorting that out in the future 😅

Second edit: I submitted mattermost/react-bootstrap#6 to fix that issue, but even without that, I'm not actually able to run npm install on this branch because of a dependency conflict because other dependencies like @testing-library/react-hooks, react-custom-scrollbars, and react-beautiful-dnd seem to be conflicting

[hmhealey]

Note that this means that we'll be losing the ability to use React Bootstrap in unit tests (not that I think Playbooks has any), and we're similarly losing the type checking for it since we've removed @types/react-boostrap above as well. I would've expected that we'd leave the React Bootstrap dependency here but switch it to the 0.34 branch of our fork (https://github.com/mattermost/react-bootstrap/tree/0.34) which the web app uses. Calls also similarly relies on the 0.34 branch, although it's a slightly older version.

Doing that would let us revert the changes to how React Bootstrap is imported, and it should give us back the type definitions since I don't think there's any type checking for RB components' props at the moment.

If you did want to make those match the web app, the web app uses react-bootstrap@github:mattermost/react-bootstrap#05559f4c61c5a314783c390d2d82906ee8c7e558 (which uses our fork instead of the package from NPM) and @types/react-bootstrap@0.32.35

[jgheithcock]

The issue was that this caused that version of react-bootstrap to be bundled in. And so, we ran into all the React 18 issues from that. I tried to get the Jest tests working without mocking them but in the end, I either needed to bundle them (which, we could do, and just stay in sync) or mock the components for the tests.

[hmhealey]

That's odd that it was bundled in. Webpack shouldn't bundle React Bootstrap as long as it's imported from a module with the name react-bootstrap or whatever is listed here in the Webpack config. The same goes for dependencies like React, React Redux, and everything else listed in the externals field in the Webpack config.

I'm fine accepting this PR as-is since it should still work for Playbooks even if we're having to jump through some hoops to make this work. I'd still like to figure out what the heck is going on here still since other plugins are sure to run into those same issues as well when they upgrade, but that doesn't need to block this PR. I'll also add this to the long list of reasons why our plugin dependency management needs an overhaul

[hmhealey]

Regarding my other comment, if we keep importing React Bootstrap as import from 'react-bootstrap', Webpack actually handles looking at window.ReactBootstrap for us because of how we've included it in the externals field in the Webpack config.

https://github.com/mattermost/mattermost-plugin-playbooks/blob/master/webapp/webpack.config.js#L121

Outside of Webpack (like in unit tests and type checks as mentioned), the version in node_modules would be used

[jgheithcock]

Then maybe we can just close this down. This all came up due to @JulienTant seeing hundreds, if not thousands of warnings running on the Zen browser. I see these warnings as well but they didn't cause an issue.

Sadly, even when using the forked version, we still get warnings - so maybe this isn't worthwhile and we should investigate the work needed to go to the newer version of react-bootstrap that does support 18 (with the breaking changes it brings). @hmhealey what is your opinion?

[hmhealey]

I'd definitely expect to see some warnings, but not that many. Do you happen to have any of those warnings still around so we can see what exactly they are? I'm curious to know what exactly is causing those. Perhaps Playbooks uses some part of React Bootstrap that the web app isn't.

As for upgrading React Bootstrap, that's a whole can of worms because it requires upgrading from vanilla Bootstrap 3 to 4 which has a ton of breaking changes in the CSS. We're unfortunately stuck between a rock and a hard place here because of the past decision we made to not make that upgrade before we had plugins making that infinitely more complex 😞

[hmhealey]

The big change is to use the mattermost repo of our fork of react-bootstrap. Note that the forked version still uses this ReactDOM.unstable_renderSubtreeIntoContainer method and so generates a number of warnings.. (@hmhealey fyi - please let me know if I've got this wrong)

UPDATE: @calebroseland points out that this is likely due to our continued use of react-bootstrap version of Tooltip and OverlayTrigger). We are too close to release however to swap those out. (Reviewers please comment if you disagree).

Caleb's got it right. That's fine and more or less expected. The usage of unstable_renderSubtreeIntoContainer comes from a dependency of React Bootstrap, so I wasn't able to easily replace it when working on the fork. It just means that code inside of those tooltips won't be able to use React 18's "concurrent React" features, but the rest of the web app can't use that either for similar reasons. We'll look at enabling concurrent React at some point later this year, and since Playbooks is getting React Bootstrap from the web app, it'll automatically get that at the same time the rest of the web app does

[calebroseland]

I think it would be good to keep the dependency import natural/as-is (relying on webpack externals to pull in react-bootstrap from webapp), but also change it to point to the same mattermost/react-bootstrap dependency that mattermost webapp points to. This would keep both environments using the same version—"runtime" pulling from window.ReactBootstrap and jest/testing pulling from node_modules.

I think the thing that will suppress the warnings is when we either remove playbooks usage of OverlayTrigger/Tooltip, or possibly a fix in mattermost/react-bootstrap.

"react-bootstrap": "react-bootstrap@github:mattermost/react-bootstrap#05559f4c61c5a314783c390d2d82906ee8c7e558" ,

[jgheithcock]

Closing this as it was pointed out we can add the react-bootstrap in package.json but as long as it is in externals, it won't be bundled in. I will open a different PR with just the React 18 changes.