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.

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

[DevTools] upgrade electron to latest version & security improvements #26337

Merged
merged 3 commits into from Mar 8, 2023
Merged

[DevTools] upgrade electron to latest version & security improvements #26337

merged 3 commits into from Mar 8, 2023

Conversation

mondaychen
Copy link
Contributor

@mondaychen mondaychen commented Mar 7, 2023 •
edited

Summary

resolves #25667
This PR also resolves several security issues in the standalone app

How did you test this change?

Tested locally yarn start in react-devtools package. Everything works normal

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 7, 2023
@mondaychen mondaychen requested a review from tyao1 March 7, 2023 18:25
@react-sizebot
Copy link

react-sizebot commented Mar 7, 2023 •
edited

Comparing: 703c675...b4fe0ad

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 156.70 kB 156.70 kB = 49.58 kB 49.58 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 158.70 kB 158.70 kB = 50.25 kB 50.25 kB
facebook-www/ReactDOM-prod.classic.js = 538.92 kB 538.92 kB = 95.86 kB 95.86 kB
facebook-www/ReactDOM-prod.modern.js = 522.78 kB 522.78 kB = 93.55 kB 93.55 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against b4fe0ad

@mondaychen mondaychen merged commit aef9303 into facebook:main Mar 8, 2023
@micsco
Copy link

micsco commented Mar 24, 2023

@mondaychen thanks for doing this upgrade. I've noted though that the preload.js file is missing from the package (found on npm).

This seems to be because the file wasn't added to the package.json file under the files property. I'm not able to make a contribution right now to resolve this but wanted to bring it to your attention in case you can fix it before I can.

@mondaychen
Copy link
Contributor Author

Ah thanks @micsco. I'll work on a fix

@mondaychen mondaychen mentioned this pull request Mar 24, 2023
Merged
mondaychen added a commit that referenced this pull request Mar 24, 2023
@mondaychen
[DevTools] missing file name in package.json (#26469)
bde974a 
resolves
#26337 (comment)
@mondaychen
Copy link
Contributor Author

@micsco this should have been fixed in 4.27.4. Thank you again for reporting

@micsco
Copy link

micsco commented Mar 24, 2023

@mondaychen amazing, thank you for looking at this so quickly. It's not yet on npm but I'll keep an eye out for it and confirm when so.

Thanks again

@sammy-SC sammy-SC mentioned this pull request Apr 5, 2023
Closed
@Willie-Boy Willie-Boy mentioned this pull request Apr 5, 2023
Merged
mondaychen pushed a commit that referenced this pull request Apr 6, 2023
@Willie-Boy
[DevTools] Replace deprecated new-window with `webContents.setWindo…
60cfeee 
…wOpenHandler()` (#26559)

## Summary

The electron package was recently upgraded from ^11.1.0 to ^23.1.2
(#26337). However, the WebContents `new-window` event – that is used in
the react-devtools project – was deprecated in
[v12.0.0](https://releases.electronjs.org/release/v12.0.0) and removed
in [v22.2.0](https://releases.electronjs.org/release/v22.2.0). The event
was replaced by `webContents.setWindowOpenHandler()`. This PR replaces
the `new-window` event with `webContents.setWindowOpenHandler()`.

## How did you test this change?

I created a simple electron application with similar functionality:

```
const { app, BrowserWindow, shell } = require('electron')

const createWindow = () => {
  const mainWindow = new BrowserWindow({
    width: 800,
    height: 600
  })

  mainWindow.webContents.setWindowOpenHandler(({ url }) => {
    shell.openExternal(url)
    return { action: 'deny' }
  })

  mainWindow.loadFile('index.html')
}

app.whenReady().then(() => {
  createWindow()
})
```

---------

Co-authored-by: root <root@DESKTOP-KCGHLB8.localdomain>
kassens pushed a commit to kassens/react that referenced this pull request Apr 17, 2023
@Willie-Boy @kassens
[DevTools] Replace deprecated new-window with `webContents.setWindo…
8c68740 
…wOpenHandler()` (facebook#26559)

## Summary

The electron package was recently upgraded from ^11.1.0 to ^23.1.2
(facebook#26337). However, the WebContents `new-window` event – that is used in
the react-devtools project – was deprecated in
[v12.0.0](https://releases.electronjs.org/release/v12.0.0) and removed
in [v22.2.0](https://releases.electronjs.org/release/v22.2.0). The event
was replaced by `webContents.setWindowOpenHandler()`. This PR replaces
the `new-window` event with `webContents.setWindowOpenHandler()`.

## How did you test this change?

I created a simple electron application with similar functionality:

```
const { app, BrowserWindow, shell } = require('electron')

const createWindow = () => {
  const mainWindow = new BrowserWindow({
    width: 800,
    height: 600
  })

  mainWindow.webContents.setWindowOpenHandler(({ url }) => {
    shell.openExternal(url)
    return { action: 'deny' }
  })

  mainWindow.loadFile('index.html')
}

app.whenReady().then(() => {
  createWindow()
})
```

---------

Co-authored-by: root <root@DESKTOP-KCGHLB8.localdomain>
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@Willie-Boy @AndyPengc12
[DevTools] Replace deprecated new-window with `webContents.setWindo…
f4466e6 
…wOpenHandler()` (facebook#26559)

## Summary

The electron package was recently upgraded from ^11.1.0 to ^23.1.2
(facebook#26337). However, the WebContents `new-window` event – that is used in
the react-devtools project – was deprecated in
[v12.0.0](https://releases.electronjs.org/release/v12.0.0) and removed
in [v22.2.0](https://releases.electronjs.org/release/v22.2.0). The event
was replaced by `webContents.setWindowOpenHandler()`. This PR replaces
the `new-window` event with `webContents.setWindowOpenHandler()`.

## How did you test this change?

I created a simple electron application with similar functionality:

```
const { app, BrowserWindow, shell } = require('electron')

const createWindow = () => {
  const mainWindow = new BrowserWindow({
    width: 800,
    height: 600
  })

  mainWindow.webContents.setWindowOpenHandler(({ url }) => {
    shell.openExternal(url)
    return { action: 'deny' }
  })

  mainWindow.loadFile('index.html')
}

app.whenReady().then(() => {
  createWindow()
})
```

---------

Co-authored-by: root <root@DESKTOP-KCGHLB8.localdomain>
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
@mondaychen
[DevTools] Replace deprecated new-window with `webContents.setWindo…
6a14212 
…wOpenHandler()` (#26559)

## Summary

The electron package was recently upgraded from ^11.1.0 to ^23.1.2
(#26337). However, the WebContents `new-window` event – that is used in
the react-devtools project – was deprecated in
[v12.0.0](https://releases.electronjs.org/release/v12.0.0) and removed
in [v22.2.0](https://releases.electronjs.org/release/v22.2.0). The event
was replaced by `webContents.setWindowOpenHandler()`. This PR replaces
the `new-window` event with `webContents.setWindowOpenHandler()`.

## How did you test this change?

I created a simple electron application with similar functionality:

```
const { app, BrowserWindow, shell } = require('electron')

const createWindow = () => {
  const mainWindow = new BrowserWindow({
    width: 800,
    height: 600
  })

  mainWindow.webContents.setWindowOpenHandler(({ url }) => {
    shell.openExternal(url)
    return { action: 'deny' }
  })

  mainWindow.loadFile('index.html')
}

app.whenReady().then(() => {
  createWindow()
})
```

---------

Co-authored-by: root <root@DESKTOP-KCGHLB8.localdomain>

DiffTrain build for commit 60cfeee.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyao1 tyao1 tyao1 approved these changes

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DevTools Bug]: react-devtools depends on vulnerable version of electron
5 participants
@mondaychen @react-sizebot @micsco @tyao1 @facebook-github-bot