[Tech] Enable sandboxing for ipcRenderer Processes

[BrettCleary]

This PR refactors all ipcRenderer calls into preload.ts as mentioned and begins to resolve #1772.

This allows contextIsolation to be set as true, which limits some of the api's that the renderer process can call and is an important security measure (https://www.electronjs.org/docs/latest/tutorial/context-isolation).

This refactor should also improve frontend developer productivity by clearly documenting the types and arguments when calling ipcRenderer.send, ipcRenderer.invoke, and ipcRenderer.on (now called via window.api.METHOD and defined in preload.ts). It will also clearly define the ipcRenderer calls/handlers in one place (preload.ts) whereas before these were duplicated across several frontend files in some instances and were pretty difficult to determine the types that could/should be passed.

There is also now the option to share types between frontend and backend ipc calls/handlers. Here is how it could work (documented in src/common/types.ts):

// here is a way to type the callback function in ipcMain.on or ipcMain.handle
// does not prevent callbacks with fewer parameters from being passed though
// the microsoft team is very opposed to enabling the above constraint https://github.com/microsoft/TypeScript/issues/17868
// ipcMain.handle('updateGame', async (e, appName, runner) => {}) 
// could be converted to:
// ipcMain.handle('updateGame', typedCallback<WrapApiFunction<typeof updateGame>>() => {})
// this has the benefit of type checking for the arguments typed in the preload api
// but may be overly complex for a small benefit

Here are the functions that type the callback from the preload.ts method

export function typedCallback<T>(arg: T) {
  return arg
}

export type WrapApiFunction<
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  TFunction extends (...args: any) => any
> = (
  e: Electron.IpcMainInvokeEvent,
  ...args: [...Parameters<TFunction>]
) => ReturnType<TFunction>

I decided to not include this in the current PR as it is quite complex and may only have marginal benefits since the types are already documented in preload.ts.

Also, the preload.ts file is built separately with es-build now (same build tool used by vite) and not bundled. This is necessary as the preload option for BrowserWindow requires an absolute path to the preload.js file.

This is an important first step towards enabling sandboxing in the renderer processes. Since it's a pretty big refactor already, allows contextIsolation to be enabled, and has other productivity benefits, I figured it would be better to go ahead and PR now.

FURTHER WORK
The next step towards enabling sandboxing will be to set nodeIntegration: false. The first issue encountered with this was that electron-store methods inside of preload.ts are synchronous but ipcRenderer calls are all async. As it is necessary to refactor these methods into the main process after setting nodeIntegration: false, this will propagate changes to a lot of frontend react components that are expecting these store method calls to be synchronous. This will be another large change, so I think it's best to PR now before beginning experimentation with that.

---

Use the following Checklist if you have changed something on the Backend or Frontend:

• Tested the feature and it's working on a current and clean install.
• Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
• Created / Updated Tests (If necessary)
• Created / Updated documentation (If necessary)

[CommandMC]

Thanks for this, was looking into doing something similar for a while now.

Looking into this a bit, it seems like it's also quite easy to add return type definitions to IPC functions now, which is (IMO) also something we need. But that can come gradually as people work on the system, no need to include everything here.

One very small thing, I'm not sure if just api is the best name for the folder & property on the window object. But then again, I can't really think of a better name right now either :^)

You've described a method of converting the electron-store calls so nodeIntegration can be turned off. I assume you've refrained from actually implementing that here since the PR's only about contextIsolation? Nevermind, should've taken care to read everything first before asking questions

[flavioislima]

Those are pretty interesting changes and codewise it looks pretty good.
Although we need to change the whole app now on all platforms since these changes touch all functionalities.

can you point out the features and platforms you have already tested?
It is also important to test on both a current Heroic installation and a new one, by removing/renaming the config folders for both legendary and heroic.

I can help test but as more people test the better. :)

[flavioislima]

Testing here on Linux with my current install and what is not working so far:

• [Linux] Game Settings > Wine > WineCFG and Winetricks won't open
• process is coming undefined on the Library. I wonder if this was introduced on this PR or on another one, but it is working fine in main
• On Install/Uninstall the game page is not updating correctly for GOG games (Epic works fine). Might be that the refresh method is not being called thereafter the action is finished.
• [Linux] On Wine Manager the progress is not working. The wine version installs correctly but no progress while downloading.

[flavioislima]

On a clean install the app doesn't open. Somehow it is trying to find Legendary folder but since it doesn't exist it fails:

(node:487388) UnhandledPromiseRejectionWarning: Error: ENOENT: no such file or directory, scandir '/home/flavio/.config/legendary/metadata'
    at Object.readdirSync (node:fs:1405:3)
    at e.readdirSync (node:electron/js2c/asar_bundle:5:10830)
    at LegendaryLibrary.loadGamesInAccount (/home/flavio/Develop/Electron/HeroicGamesLauncher/build/electron/main.js:1850:16)
    at new LegendaryLibrary (/home/flavio/Develop/Electron/HeroicGamesLauncher/build/electron/main.js:1841:10)
    at LegendaryLibrary.get (/home/flavio/Develop/Electron/HeroicGamesLauncher/build/electron/main.js:1845:41)
    at createWindow (/home/flavio/Develop/Electron/HeroicGamesLauncher/build/electron/main.js:10647:20)
    at /home/flavio/Develop/Electron/HeroicGamesLauncher/build/electron/main.js:10819:11

[CommandMC]

On a clean install the app doesn't open. Somehow it is trying to find Legendary folder but since it doesn't exist it fails:

This is my fault and not related to this PR:

HeroicGamesLauncher/src/backend/legendary/library.ts

Line 76 in 2ba6604

readdirSync(legendaryMetadata).forEach((filename) => {

This assumes legendaryMetadata exists. A quick if (existsSync(legendaryMetadata)) in front there would solve that issue

[BrettCleary]

Those are pretty interesting changes and codewise it looks pretty good. Although we need to change the whole app now on all platforms since these changes touch all functionalities.

can you point out the features and platforms you have already tested? It is also important to test on both a current Heroic installation and a new one, by removing/renaming the config folders for both legendary and heroic.

I can help test but as more people test the better. :)

Awesome! And noted about the legendary/heroic config folders.

I've just been testing on Windows. I've only run the unit tests and the app in dev mode. I am planning to test the builds and features more comprehensively today on Windows, mac, linux. Appreciate the help testing too!

[BrettCleary]

Thanks for this, was looking into doing something similar for a while now.

Looking into this a bit, it seems like it's also quite easy to add return type definitions to IPC functions now, which is (IMO) also something we need. But that can come gradually as people work on the system, no need to include everything here.

One very small thing, I'm not sure if just api is the best name for the folder & property on the window object. But then again, I can't really think of a better name right now either :^)

You've described a method of converting the electron-store calls so nodeIntegration can be turned off. I assume you've refrained from actually implementing that here since the PR's only about contextIsolation? Nevermind, should've taken care to read everything first before asking questions

Yes I agree. It'd be nice to have the return types too. Agreed that they can be added gradually moving forward.

I'm open to naming the folder something different. I named it that because I saw some other projects using a similar name, I'm pretty sure it doesn't conflict with window's existing properties, and it's quite simple to remember.

[flavioislima]

Yes I agree. It'd be nice to have the return types too. Agreed that they can be added gradually moving forward.

I'm open to naming the folder something different. I named it that because I saw some other projects using a similar name, I'm pretty sure it doesn't conflict with window's existing properties, and it's quite simple to remember.

Imo api sounds good as the name for the folder. It is pretty clear since it will be used to call the methods from frontend to backend.

[flavioislima]

Login with Epic is not working but that is because the beta branch is not updated with the main branch so we need to updated it to get the fixes from it.

[flavioislima]

ok, just updated the beta branch, now just update this branch against beta to get the fixes.

[BrettCleary]

On a clean install the app doesn't open. Somehow it is trying to find Legendary folder but since it doesn't exist it fails:

This is my fault and not related to this PR:

HeroicGamesLauncher/src/backend/legendary/library.ts

Line 76 in 2ba6604

readdirSync(legendaryMetadata).forEach((filename) => {

This assumes legendaryMetadata exists. A quick if (existsSync(legendaryMetadata)) in front there would solve that issue

Shouldn't it be mkdirSync(legendaryMetadata, {recursive: true}) or is the directory being created somewhere else?

[flavioislima]

Shouldn't it be mkdirSync(legendaryMetadata, {recursive: true}) or is the directory being created somewhere else?

This folder is created after login with epic, so legendary creates it.

[BrettCleary]

Looks like the windows build check is throwing the same error as the last two commits to the beta branch.

FetchError: request to https://artifacts.electronjs.org/headers/dist/v20.0.2/node-v20.0.2-headers.tar.gz failed, reason: connect ETIMEDOUT 13.107.246.69:443

Maybe the remote server was down during these tests. Doesn't seem to be related to the changes we've made unless I'm missing something. Can we re run the checks? Or should we just ignore for now

[flavioislima]

Just ignore the builds. Some days github is more unstable than others.

[BrettCleary]

Those are pretty interesting changes and codewise it looks pretty good. Although we need to change the whole app now on all platforms since these changes touch all functionalities.

can you point out the features and platforms you have already tested? It is also important to test on both a current Heroic installation and a new one, by removing/renaming the config folders for both legendary and heroic.

I can help test but as more people test the better. :)
@flavioislima

I've been testing today on Windows and Mac. I'm having a hard time in some cases trying to determine what the expected behavior should be though. I'm also still familiarizing myself with the frontend/repo in general so it's taking me a lot of time to go through every error message as well.

On Windows I received the following error messages:

Can't download from epic cdn (I think this is being discussed in the discord and is unrelated to the current PR)

forbidden access to epic store for fortnite download
[Core] INFO: Trying to re-use existing login session...
[cli] INFO: Saving install tags for "Fortnite" to config: ,chunk0,chunk10
[cli] INFO: Preparing download for "Fortnite" (Fortnite)...
Traceback (most recent call last):
  File "legendary\cli.py", line 3054, in <module>
  File "legendary\cli.py", line 2971, in main
  File "legendary\cli.py", line 931, in install_game
  File "legendary\core.py", line 1296, in prepare_download
  File "legendary\core.py", line 1221, in get_cdn_manifest
  File "requests\models.py", line 1021, in raise_for_status
requests.exceptions.HTTPError: 403 Client Error: Forbidden for url: https://download.epicgames.com/Builds/Fortnite/CloudDir/

HGL showed a "Can't get game info" error when trying to expand the game in the library while downloading it through Epic Games Launcher.

React devtools is also throwing the following error when checking/unchecking some of the settings components for the first time.

Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.

The following error is sometimes thrown as well

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
    at NewLogin (http://localhost:5173/src/frontend/screens/Login/index.tsx:33:7)

But I'm able to navigate the Epic games store, install through EGL, and then refresh the library to see my game. The game launches fine through HGL. When I download a game from the GOG store. I don't see it show up in HGL. I'm not sure how to troubleshoot this as it seems there is only a sync option for the EGL.

I've tested on Mac too and I've gotten the following error

Error: Error invoking remote method 'checkGameUpdates': Error: ENOENT: no such file or directory, open '/Users/brett/.config/legendary/installed.json'.

I'm not sure when legendary is supposed to be installed on MacOS, so I'm having trouble troubleshooting this too.

On Mac I'm able to see games from Epic Games Launcher, but the game that I've downloaded through EGL still shows up as "Not Installed". Clicking into the game gives

... legendary info a74 ...
SyntaxError: Unexpected end of JSON input
Cannot get game info
SyntaxError: Unexpected end of JSON input

and I get a "Cannot get game info" message on the screen

[Nocccer]

I tested our beta channel before. Everything worked fine. On your branch i got some errors:

This happens as soon as i press game settings

simplescreenrecorder-2022-09-09_01.51.17.mp4

ProgressDialog should be in frontend/components

[Nocccer]

Gets pretty close to perfect.
I found something i don't know if you introduced it, but i could not reproduce it on beta.

If i uninstall a game it is still shown as installed on the library page till i refresh via the refresh button.

[Nocccer]

We getting close :)
Left some comments

[flavioislima]

@BrettCleary the wine downloader still have issues but I found a solution.
On WineItem/index..tsx you need to add a useEffect like this:

  useEffect(() => {
    if (version) {
      window.api.handleProgressOfWineManager(
        version,
        (
          e: Electron.IpcRendererEvent,
          progress: {
            state: State
            progress: ProgressInfo
          }
        ) => {
          setProgress(progress)
        }
      )
    }
  }, [])

Also change the api call to remove the event:

export const handleProgressOfWineManager = (version: string, callback: any) => {
  ipcRenderer.on('progressOfWineManager' + version, callback)
  return () => {
    ipcRenderer.removeListener('progressOfWineManager' + version, callback)
  }
}

And finally on wine manager ipc_handlers use the window to send the command:

  const window = BrowserWindow.getAllWindows()[0]
  const onProgress = (state: State, progress?: ProgressInfo) => {
    window.webContents.send('progressOfWineManager' + release.version, {
      state,
      progress
    })
  }

I think the only mandatory change is the first one but the others are for consistency and best practices.
This way we will be following the same pattern that we are using for the game install progress. :)

[Nocccer]

window.webContents.send('progressOfWineManager' + release.version, {
state,
progress
})

Why window.webContents.send and not taking the event.sender.send ? Is there any benefit ?

[Nocccer]

Beside the one thing that flavio suggested, i would love to get rid of all
// eslint-disable-next-line @typescript-eslint/no-explicit-any. I added some lines where i know the type.

[flavioislima]

window.webContents.send('progressOfWineManager' + release.version, {
state,
progress
})

Why window.webContents.send and not taking the event.sender.send ? Is there any benefit ?

In practice, it doesn't change anything. Was just about consistency because for the game progress we are using one way and for wine progress another way. Thats confusing because they are doing the same thing.

[BrettCleary]

window.webContents.send('progressOfWineManager' + release.version, {
state,
progress
})

Why window.webContents.send and not taking the event.sender.send ? Is there any benefit ?

The difference would be that event.sender.send would send the wine download progress back to the window that invoked installWineVersion whereas using the window.webContents.send approach would send to the first BrowserWindow. Since we're only using one browser window, it's interchangeable.

[BrettCleary]

@BrettCleary the wine downloader still have issues but I found a solution. On WineItem/index..tsx you need to add a useEffect like this:

  useEffect(() => {
    if (version) {
      window.api.handleProgressOfWineManager(
        version,
        (
          e: Electron.IpcRendererEvent,
          progress: {
            state: State
            progress: ProgressInfo
          }
        ) => {
          setProgress(progress)
        }
      )
    }
  }, [])

Also change the api call to remove the event:

export const handleProgressOfWineManager = (version: string, callback: any) => {
  ipcRenderer.on('progressOfWineManager' + version, callback)
  return () => {
    ipcRenderer.removeListener('progressOfWineManager' + version, callback)
  }
}

And finally on wine manager ipc_handlers use the window to send the command:

  const window = BrowserWindow.getAllWindows()[0]
  const onProgress = (state: State, progress?: ProgressInfo) => {
    window.webContents.send('progressOfWineManager' + release.version, {
      state,
      progress
    })
  }

I think the only mandatory change is the first one but the others are for consistency and best practices. This way we will be following the same pattern that we are using for the game install progress. :)

Noted. latest commit addresses this. Also the useEffect will have to return the callback to remove the listener on each render and since TS throws error because not all code paths return a value, I added the ()=>void callback at the end.

useEffect(() => {
    if (version) {
      const removeWineManagerDownloadListener =
        window.api.handleProgressOfWineManager(
          version,
          (
            e: Electron.IpcRendererEvent,
            progress: {
              state: State
              progress: ProgressInfo
            }
          ) => {
            setProgress(progress)
          }
        )
      return removeWineManagerDownloadListener
    }
    /* eslint-disable @typescript-eslint/no-empty-function */
    return () => {}
  }, [])

[BrettCleary]

Beside the one thing that flavio suggested, i would love to get rid of all // eslint-disable-next-line @typescript-eslint/no-explicit-any. I added some lines where i know the type.

Agreed. I would prefer to do this incrementally though especially since before this PR, there were no types in ipcRenderer/main calls

[flavioislima]

Looks good now, all bugs I could found were fixed, left a small comment but you should be able to merge whenever you like now :D

Thanks for that by the way, was a pretty good refactor ⚔️

[flavioislima]

@Nocccer approve it when you find some time since you asked for changes. ;)

[Nocccer]

LGTM. I left some comment we need to discuss and maybe fix after the merge.