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

Bolster isBrowser check for electron-link bundled applications #437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bolster isBrowser check for electron-link bundled applications #437

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion api/WebApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,14 @@ import os = require('os');
import url = require('url');
import path = require('path');

const isBrowser: boolean = typeof window !== 'undefined';
// https://www.electronjs.org/docs/api/process
interface ElectronProcess {
type: 'browser' | 'renderer' | 'worker'
};
const isElectron: boolean = process && process.versions && process.versions.hasOwnProperty('electron');
const isElectronWebpage: boolean = isElectron && (<ElectronProcess><unknown>process).type === 'renderer';
const isBrowser: boolean = typeof window !== 'undefined' && (!isElectron || isElectronWebpage);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it make sense to add null check for windows.navigator property instead of introducing electron specific logic?

Copy link

Choose a reason for hiding this comment

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

@anatolybolshakov it is not enough. Please refer to this discussion: electron/electron#2288. Although they are recommending to check userAgent....

Copy link

Choose a reason for hiding this comment

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

I believe that the correct way for doing this is to just keep the first check
const isElectron: boolean = process && process.versions && process.versions.hasOwnProperty('electron');
but remove the latter one
const isElectronWebpage: boolean = isElectron && (<ElectronProcess><unknown>process).type === 'renderer';

because if it is the renderer navigator.userAgent will be defined.

Copy link

@Mr-Wallet Mr-Wallet Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a way we can get this PR un-stuck? We'd rather not maintain our own fork of this package indefinitely. 😅

A userAgent check for a specific value wouldn't work for us, as we override the userAgent on the default session so that it doesn't have electron in it. (Long story.) However it is truthy, so if it's simply a check for the existence of navigator and of the userAgent thereof, that would work.

However that doesn't seem to be what's discussed in the electron issue that was linked; that issue was more about trying to determine if a browser-like process is electron or not; and vaguely to me it sounds like, "if context isolation is enabled/access to Node is disabled, and the userAgent isn't roughly default, there's no way to tell." But frankly at that point, we don't actually need to tell; if it's browser-like, isBrowser === true will work regardless of whether or not it's Electron.

The main issue of this PR is making sure it doesn't break when it's not like a normal browser (i.e. there is no window.navigator). For both normal browsers and Electron renderers (regardless of how isolated or not they are), the isBrowser path in this file is definitely safe. We just need to know when it's not safe so that we can avoid it, and unfortunately window (the existing check) is defined in Electron 'main' processes, but there's also no window.navigator (which is the actual dependency in the isBrowser code path).

For us it seemed sufficient to do what's in this PR; if a renderer is isolated it might false-negative and think it's not Electron when it is; but in that case we wanted isBrowser to be true anyway, so we get the right answer regardless. Context isolation (which can hide the fact that it's Electron) is impossible for the 'main' process, which is the case we're trying to solve for. For our purposes, this PR gets the job done, and with regards to 'renderer' processes (which are the focus of electron/electron#2288), I don't think this can break any use case (isolation or no). Isolated 'worker' processes are unsolved but this package wouldn't work in such a process anyway; and we haven't tested it but un-isolated workers would probably be fixed under this change as well.

This PR seems strictly the same or better than the existing code for all use cases, but we're not picky as long as we can get something merged. Please let me know your thoughts.

(P.S. unrelated but please bump typed-rest-client to 1.8.8 or higher)


/**
* Methods to return handler objects (see handlers folder)
*/
Expand Down