App: Automatically consider known login pages as internal (fix #706) (PR #1124)

Co-authored-by: Ronan Jouchet <ronan@jouchet.fr>

app/src/components/mainWindow.ts

@@ -225,7 +225,14 @@ export function createMainWindow(
   };
 
   const onWillNavigate = (event: Event, urlToGo: string): void => {
-    if (!linkIsInternal(options.targetUrl, urlToGo, options.internalUrls)) {
+    if (
+      !linkIsInternal(
+        options.targetUrl,
+        urlToGo,
+        options.internalLoginPages,
+        options.internalUrls,
+      )
+    ) {
       event.preventDefault();
       if (options.blockExternalUrls) {
         onBlockedExternalUrl(urlToGo);
@@ -294,6 +301,7 @@ export function createMainWindow(
       urlToGo,
       disposition,
       options.targetUrl,
+      options.internalLoginPages,
       options.internalUrls,
       preventDefault,
       shell.openExternal.bind(this),

app/src/components/mainWindowHelpers.test.ts

@@ -21,6 +21,7 @@ test('internal urls should not be handled', () => {
     internalUrl,
     undefined,
     originalUrl,
+    true,
     undefined,
     preventDefault,
     openExternal,
@@ -49,6 +50,7 @@ test('external urls should be opened externally', () => {
     externalUrl,
     undefined,
     originalUrl,
+    true,
     undefined,
     preventDefault,
     openExternal,
@@ -78,6 +80,7 @@ test('external urls should be ignored if blockExternal is true', () => {
     externalUrl,
     undefined,
     originalUrl,
+    true,
     undefined,
     preventDefault,
     openExternal,
@@ -106,6 +109,7 @@ test('tab disposition should be ignored if tabs are not enabled', () => {
     internalUrl,
     foregroundDisposition,
     originalUrl,
+    true,
     undefined,
     preventDefault,
     openExternal,
@@ -134,6 +138,7 @@ test('tab disposition should be ignored if url is external', () => {
     externalUrl,
     foregroundDisposition,
     originalUrl,
+    true,
     undefined,
     preventDefault,
     openExternal,
@@ -162,6 +167,7 @@ test('foreground tabs with internal urls should be opened in the foreground', ()
     internalUrl,
     foregroundDisposition,
     originalUrl,
+    true,
     undefined,
     preventDefault,
     openExternal,
@@ -191,6 +197,7 @@ test('background tabs with internal urls should be opened in background tabs', (
     internalUrl,
     backgroundDisposition,
     originalUrl,
+    true,
     undefined,
     preventDefault,
     openExternal,
@@ -220,6 +227,7 @@ test('about:blank urls should be handled', () => {
     'about:blank',
     undefined,
     originalUrl,
+    true,
     undefined,
     preventDefault,
     openExternal,

app/src/components/mainWindowHelpers.ts

@@ -2,9 +2,10 @@ import { linkIsInternal } from '../helpers/helpers';
 
 export function onNewWindowHelper(
   urlToGo: string,
-  disposition,
+  disposition: string,
   targetUrl: string,
-  internalUrls,
+  internalLoginPages: boolean,
+  internalUrls: string | RegExp,
   preventDefault,
   openExternal,
   createAboutBlankWindow,
@@ -13,7 +14,7 @@ export function onNewWindowHelper(
   blockExternal: boolean,
   onBlockedExternalUrl: (url: string) => void,
 ): void {
-  if (!linkIsInternal(targetUrl, urlToGo, internalUrls)) {
+  if (!linkIsInternal(targetUrl, urlToGo, internalLoginPages, internalUrls)) {
     preventDefault();
     if (blockExternal) {
       onBlockedExternalUrl(urlToGo);

app/src/helpers/helpers.test.ts

@@ -10,49 +10,111 @@ const externalUrl = 'https://www.wikipedia.org/wiki/Electron';
 const wildcardRegex = /.*/;
 
 test('the original url should be internal', () => {
-  expect(linkIsInternal(internalUrl, internalUrl, undefined)).toEqual(true);
+  expect(linkIsInternal(internalUrl, internalUrl, false, undefined)).toEqual(
+    true,
+  );
 });
 
 test('sub-paths of the original url should be internal', () => {
   expect(
-    linkIsInternal(internalUrl, internalUrl + internalUrlSubPath, undefined),
+    linkIsInternal(
+      internalUrl,
+      internalUrl + internalUrlSubPath,
+      false,
+      undefined,
+    ),
   ).toEqual(true);
 });
 
 test("'about:blank' should be internal", () => {
-  expect(linkIsInternal(internalUrl, 'about:blank', undefined)).toEqual(true);
+  expect(linkIsInternal(internalUrl, 'about:blank', false, undefined)).toEqual(
+    true,
+  );
 });
 
 test('urls from different sites should not be internal', () => {
-  expect(linkIsInternal(internalUrl, externalUrl, undefined)).toEqual(false);
+  expect(linkIsInternal(internalUrl, externalUrl, false, undefined)).toEqual(
+    false,
+  );
 });
 
 test('all urls should be internal with wildcard regex', () => {
-  expect(linkIsInternal(internalUrl, externalUrl, wildcardRegex)).toEqual(true);
+  expect(
+    linkIsInternal(internalUrl, externalUrl, false, wildcardRegex),
+  ).toEqual(true);
 });
 
 test('a "www." of a domain should be considered internal', () => {
-  expect(linkIsInternal(internalUrl, internalUrlWww, undefined)).toEqual(true);
+  expect(linkIsInternal(internalUrl, internalUrlWww, false, undefined)).toEqual(
+    true,
+  );
 });
 
 test('urls on the same "base domain" should be considered internal', () => {
-  expect(linkIsInternal(internalUrl, sameBaseDomainUrl, undefined)).toEqual(
-    true,
-  );
+  expect(
+    linkIsInternal(internalUrl, sameBaseDomainUrl, false, undefined),
+  ).toEqual(true);
 });
 
 test('urls on the same "base domain" should be considered internal, even with a www', () => {
-  expect(linkIsInternal(internalUrlWww, sameBaseDomainUrl, undefined)).toEqual(
-    true,
-  );
+  expect(
+    linkIsInternal(internalUrlWww, sameBaseDomainUrl, false, undefined),
+  ).toEqual(true);
 });
 
 test('urls on the same "base domain" should be considered internal, long SLD', () => {
   expect(
-    linkIsInternal(internalUrlCoUk, sameBaseDomainUrlCoUk, undefined),
+    linkIsInternal(internalUrlCoUk, sameBaseDomainUrlCoUk, false, undefined),
   ).toEqual(true);
 });
 
+const testLoginPages = [
+  'https://amazon.co.uk/signin',
+  'https://amazon.com/signin',
+  'https://amazon.de/signin',
+  'https://amazon.com/ap/signin',
+  'https://facebook.co.uk/login',
+  'https://facebook.com/login',
+  'https://facebook.de/login',
+  'https://github.co.uk/login',
+  'https://github.com/login',
+  'https://github.de/login',
+  'https://accounts.google.co.uk',
+  'https://accounts.google.com',
+  'https://accounts.google.de',
+  'https://linkedin.co.uk/uas/login',
+  'https://linkedin.com/uas/login',
+  'https://linkedin.de/uas/login',
+  'https://login.live.co.uk',
+  'https://login.live.com',
+  'https://login.live.de',
+  'https://okta.co.uk',
+  'https://okta.com',
+  'https://subdomain.okta.com',
+  'https://okta.de',
+  'https://twitter.co.uk/oauth/authenticate',
+  'https://twitter.com/oauth/authenticate',
+  'https://twitter.de/oauth/authenticate',
+];
+
+test.each(testLoginPages)(
+  '%s login page should be internal if internalLoginPages is enabled',
+  (loginUrl: string) => {
+    expect(linkIsInternal(internalUrl, loginUrl, true, undefined)).toEqual(
+      true,
+    );
+  },
+);
+
+test.each(testLoginPages)(
+  '%s login page should not be internal if internalLoginPages is disabled',
+  (loginUrl: string) => {
+    expect(linkIsInternal(internalUrl, loginUrl, false, undefined)).toEqual(
+      false,
+    );
+  },
+);
+
 const smallCounterTitle = 'Inbox (11) - nobody@example.com - Gmail';
 const largeCounterTitle = 'Inbox (8,756) - nobody@example.com - Gmail';
 const noCounterTitle = 'Inbox - nobody@example.com - Gmail';

app/src/helpers/helpers.ts

@@ -18,15 +18,35 @@ export function isWindows(): boolean {
   return os.platform() === 'win32';
 }
 
+function isInternalLoginPage(url: string): boolean {
+  const internalLoginPagesArray = [
+    'amazon\\.[a-zA-Z\\.]*/[a-zA-Z\\/]*signin', // Amazon
+    `facebook\\.[a-zA-Z\\.]*\\/login`, // Facebook
+    'github\\.[a-zA-Z\\.]*\\/login', // GitHub
+    'accounts\\.google\\.[a-zA-Z\\.]*', // Google
+    'linkedin\\.[a-zA-Z\\.]*/uas/login', // LinkedIn
+    'login\\.live\\.[a-zA-Z\\.]*', // Microsoft
+    'okta\\.[a-zA-Z\\.]*', // Okta
+    'twitter\\.[a-zA-Z\\.]*/oauth/authenticate', // Twitter
+  ];
+  const regex = RegExp(internalLoginPagesArray.join('|'));
+  return regex.test(url);
+}
+
 export function linkIsInternal(
   currentUrl: string,
   newUrl: string,
+  internalLoginPages: boolean,
   internalUrlRegex: string | RegExp,
 ): boolean {
   if (newUrl === 'about:blank') {
     return true;
   }
 
+  if (internalLoginPages && isInternalLoginPage(newUrl)) {
+    return true;
+  }
+
   if (internalUrlRegex) {
     const regex = RegExp(internalUrlRegex);
     return regex.test(newUrl);
@@ -50,6 +70,7 @@ export function linkIsInternal(
       currentUrl,
       'To:',
       newUrl,
+      err,
     );
     return false;
   }

docs/api.md

@@ -43,6 +43,7 @@
   - [[ignore-gpu-blacklist]](#ignore-gpu-blacklist)
   - [[enable-es3-apis]](#enable-es3-apis)
   - [[insecure]](#insecure)
+  - [[internal-login-pages]](#internal-login-pages)
   - [[internal-urls]](#internal-urls)
   - [[block-external-urls]](#block-external-urls)
   - [[proxy-rules]](#proxy-rules)
@@ -394,6 +395,30 @@ Passes the enable-es3-apis flag to the Chrome engine, to force the activation of
 
 Forces the packaged app to ignore web security errors, such as [Mixed Content](https://developer.mozilla.org/en-US/docs/Security/Mixed_content) errors when receiving HTTP content on a HTTPS site.
 
+#### [internal-login-pages]
+
+```
+--internal-login-pages <boolean>
+```
+
+Whether to consider known login providers to be internal, so as to avoid login urls being opened in a browser without being able to login to the application. Default: true
+
+Known login providers:
+- Amazon
+- Facebook
+- GitHub
+- Google
+- LinkedIn
+- Microsoft
+- Okta
+- Twitter
+
+Example:
+
+```bash
+nativefier https://google.com --internal-login-pages false
+```
+
 #### [internal-urls]
 
 ```
@@ -408,13 +433,23 @@ once stripped of `www.`. For example, by default,
 - URLs from/to `foo.com`, `app.foo.com`, `www.foo.com` are considered internal.
 - URLs from/to `abc.com` and `xyz.com` are considered external.
 
-Example:
+*[Breaking change in Nativefier 43.0.0]* Finally, URLs for known login pages
+(e.g. `accounts.google.com` or `login.live.com`) are considered internal.
+This does not replace `internal-urls`, it complements it, and happens *before*
+your `internal-urls` rule is applied. So, if you already set the flag to let such
+auth pages open internally, you don't need to change it but it might be unnecessary.
+
+We think this is desirable behavior and are so far unaware of cases where users
+might not want this. If you disagree, please chime in at
+[PR #1124: App: Automatically consider known login pages as internal](https://github.com/nativefier/nativefier/pull/1124)
+
+Example of `--internal-urls` causing all links to Google to be considered internal:
 
 ```bash
 nativefier https://google.com --internal-urls ".*?\.google\.*?"
 ```
 
-Or, if you want to allow all domains for example for external auths,
+Or, if you never expect Nativefier to open an "external" page in your OS browser,
 
 ```bash
 nativefier https://google.com --internal-urls ".*?"

src/build/prepareElectronApp.ts

@@ -44,6 +44,7 @@ function pickElectronAppArgs(options: AppOptions): any {
     ignoreCertificate: options.nativefier.ignoreCertificate,
     ignoreGpuBlacklist: options.nativefier.ignoreGpuBlacklist,
     insecure: options.nativefier.insecure,
+    internalLoginPages: options.nativefier.internalLoginPages,
     internalUrls: options.nativefier.internalUrls,
     blockExternalUrls: options.nativefier.blockExternalUrls,
     maxHeight: options.nativefier.maxHeight,

src/cli.ts

@@ -6,9 +6,14 @@ import * as dns from 'dns';
 import * as commander from 'commander';
 import * as log from 'loglevel';
 
-import { isArgFormatInvalid, isWindows } from './helpers/helpers';
+import { isArgFormatInvalid } from './helpers/helpers';
 import { supportedArchs, supportedPlatforms } from './infer/inferOs';
 import { buildNativefierApp } from './main';
+import {
+  parseBoolean,
+  parseBooleanOrString,
+  parseJson,
+} from './utils/parseUtils';
 
 // package.json is `require`d to let tsc strip the `src` folder by determining
 // baseUrl=src. A static import would prevent that and cause an ugly extra "src" folder
@@ -19,37 +24,6 @@ function collect(val: any, memo: any[]): any[] {
   return memo;
 }
 
-function parseBooleanOrString(val: string): boolean | string {
-  switch (val) {
-    case 'true':
-      return true;
-    case 'false':
-      return false;
-    default:
-      return val;
-  }
-}
-
-function parseJson(val: string): any {
-  if (!val) return {};
-  try {
-    return JSON.parse(val);
-  } catch (err) {
-    const windowsShellHint = isWindows()
-      ? `\n   In particular, Windows cmd doesn't have single quotes, so you have to use only double-quotes plus escaping: "{\\"someKey\\": \\"someValue\\"}"`
-      : '';
-
-    log.error(
-      `Unable to parse JSON value: ${val}\n` +
-        `JSON should look like {"someString": "someValue", "someBoolean": true, "someArray": [1,2,3]}.\n` +
-        ` - Only double quotes are allowed, single quotes are not.\n` +
-        ` - Learn how your shell behaves and escapes characters.${windowsShellHint}\n` +
-        ` - If unsure, validate your JSON using an online service.`,
-    );
-    throw err;
-  }
-}
-
 function getProcessEnvs(val: string): any {
   if (!val) {
     return {};
@@ -238,6 +212,11 @@ if (require.main === module) {
       'default zoom factor to use when the app is opened; defaults to 1.0',
       parseFloat,
     )
+    .option(
+      '--internal-login-pages',
+      "Force known login pages (Amazon, Facebook, GitHub, Google, LinkedIn, Microsoft, Okta, Twitter) to be internal urls so they don't have to be individually specified. Default: true",
+      (value) => parseBoolean(value, true),
+    )
     .option(
       '--internal-urls <value>',
       'regex of URLs to consider "internal"; all other URLs will be opened in an external browser. Default: URLs on same second-level domain as app',

src/integration-test.ts

@@ -36,13 +36,21 @@ function checkApp(appRoot: string, inputOptions: any): void {
   const iconPath = path.join(appPath, iconFile);
   expect(fs.existsSync(iconPath)).toBe(true);
   expect(fs.statSync(iconPath).size).toBeGreaterThan(1000);
+
+  // Internal Login Pages enabled by default
+  expect(nativefierConfig.internalLoginPages).toBe(
+    inputOptions.internalLoginPages === undefined
+      ? true
+      : inputOptions.internalLoginPages,
+  );
 }
 
 describe('Nativefier', () => {
   jest.setTimeout(300000);
 
-  test('builds a Nativefier app for several platforms', async () => {
-    for (const platform of ['darwin', 'linux']) {
+  test.each(['darwin', 'linux'])(
+    'builds a Nativefier app for platform %s',
+    async (platform) => {
       const tempDirectory = getTempDir('integtest');
       const options = {
         targetUrl: 'https://google.com/',
@@ -52,6 +60,19 @@ describe('Nativefier', () => {
       };
       const appPath = await buildNativefierApp(options);
       checkApp(appPath, options);
-    }
+    },
+  );
+
+  test('can disable internalLoginPages', async () => {
+    const tempDirectory = getTempDir('integtest');
+    const options = {
+      targetUrl: 'https://google.com/',
+      out: tempDirectory,
+      overwrite: true,
+      platform: 'linux',
+      internalLoginPages: false,
+    };
+    const appPath = await buildNativefierApp(options);
+    checkApp(appPath, options);
   });
 });

src/options/model.ts

@@ -34,6 +34,7 @@ export interface AppOptions {
     ignoreGpuBlacklist: boolean;
     inject: string[];
     insecure: boolean;
+    internalLoginPages: boolean;
     internalUrls: string;
     blockExternalUrls: boolean;
     maximize: boolean;

src/options/optionsMain.ts

@@ -74,6 +74,11 @@ export async function getOptions(rawOptions: any): Promise<AppOptions> {
       ignoreGpuBlacklist: rawOptions.ignoreGpuBlacklist || false,
       inject: rawOptions.inject || [],
       insecure: rawOptions.insecure || false,
+      internalLoginPages:
+        rawOptions.internalLoginPages === undefined ||
+        typeof rawOptions.internalLoginPages !== 'boolean'
+          ? true
+          : rawOptions.internalLoginPages,
       internalUrls: rawOptions.internalUrls || null,
       blockExternalUrls: rawOptions.blockExternalUrls || false,
       maximize: rawOptions.maximize || false,

src/utils/parseUtils.test.ts

@@ -0,0 +1,21 @@
+import { parseBoolean } from './parseUtils';
+
+test.each([
+  ['true', true, true],
+  ['1', true, true],
+  ['yes', true, true],
+  [1, true, true],
+  [true, true, true],
+  ['false', false, true],
+  ['0', false, true],
+  ['no', false, true],
+  [0, false, true],
+  [false, false, true],
+  [undefined, true, true],
+  [undefined, false, false],
+])(
+  'parseBoolean("%s") === %s',
+  (testString: string, expectedResult: boolean, _default: boolean) => {
+    expect(parseBoolean(testString, _default)).toBe(expectedResult);
+  },
+);

src/utils/parseUtils.ts

@@ -0,0 +1,60 @@
+import * as log from 'loglevel';
+
+import { isWindows } from '../helpers/helpers';
+
+export function parseBoolean(
+  val: boolean | string | number,
+  _default: boolean,
+): boolean {
+  try {
+    if (typeof val === 'boolean') {
+      return val;
+    }
+    val = String(val);
+    switch (val.toLocaleLowerCase()) {
+      case 'true':
+      case '1':
+      case 'yes':
+        return true;
+      case 'false':
+      case '0':
+      case 'no':
+        return false;
+      default:
+        return _default;
+    }
+  } catch (_) {
+    return _default;
+  }
+}
+
+export function parseBooleanOrString(val: string): boolean | string {
+  switch (val) {
+    case 'true':
+      return true;
+    case 'false':
+      return false;
+    default:
+      return val;
+  }
+}
+
+export function parseJson(val: string): any {
+  if (!val) return {};
+  try {
+    return JSON.parse(val);
+  } catch (err) {
+    const windowsShellHint = isWindows()
+      ? `\n   In particular, Windows cmd doesn't have single quotes, so you have to use only double-quotes plus escaping: "{\\"someKey\\": \\"someValue\\"}"`
+      : '';
+
+    log.error(
+      `Unable to parse JSON value: ${val}\n` +
+        `JSON should look like {"someString": "someValue", "someBoolean": true, "someArray": [1,2,3]}.\n` +
+        ` - Only double quotes are allowed, single quotes are not.\n` +
+        ` - Learn how your shell behaves and escapes characters.${windowsShellHint}\n` +
+        ` - If unsure, validate your JSON using an online service.`,
+    );
+    throw err;
+  }
+}