DEV Community

Daniel Pertu
Daniel Pertu

Posted on

Our window decides a link is ours with a string prefix, and the string that beats it has an @ in it

#electron #security #javascript #node

Notifio is a desktop app that watches rental searches and tells you the moment a new listing appears. Its window is not loading a file from disk. There is an Express server running inside the Electron main process, bound to 127.0.0.1, and the window loads a URL from it:

let serverPort = Number(process.env.PORT) || 3000;
// ...
mainWindow.loadURL(`http://localhost:${serverPort}`);
Enter fullscreen mode Exit fullscreen mode

I have written about why the UI is reached over HTTP at all: the renderer has no preload script and talks to 28 endpoints on 127.0.0.1. That arrangement buys a lot. It also creates one question every window in the app has to answer about every URL it is asked to open: is this our own UI, or is it somebody else's website?

Our answer is a string prefix.

The rule, in two handlers 

// Open all external links in the system browser
// "external" = anything that isn't the embedded Express server on localhost:3000
const internalOrigin = `http://localhost:${serverPort}`;

mainWindow.webContents.setWindowOpenHandler(({ url }) => {
  if (!url.startsWith(internalOrigin)) {
    shell.openExternal(url);
  }
  return { action: 'deny' };
});

mainWindow.webContents.on('will-navigate', (event, url) => {
  if (!url.startsWith(internalOrigin)) {
    event.preventDefault();
    shell.openExternal(url);
  }
});
Enter fullscreen mode Exit fullscreen mode

Two handlers rather than one, because there are two ways a click can ask for a new document. An anchor with target="_blank" goes through setWindowOpenHandler. A plain anchor navigating the current document goes through will-navigate. The app has exactly one of each, which is how I know both paths are live:

  • the finds list renders each listing as <a href={find.url} target="_blank" rel="noreferrer">
  • the activation screen renders one <a href={pricingLink}> with no target, pointing at our own website

The intent is right. A desktop window we control has no address bar, so a page loaded inside it cannot be checked by the person looking at it, and the same reasoning is why our in-app purchase deliberately leaves the app and opens Stripe in the user's real browser instead. Nothing that is not ours should ever paint inside our window.

The implementation does not enforce that. startsWith compares bytes, and an origin is not a prefix.

Four URLs, run through both tests 

for (const u of [
  'http://localhost:3000@evil.example/x',
  'http://localhost:30001/',
  'http://127.0.0.1:3000/',
  'http://localhost:3000.evil.example/',
]) {
  try {
    console.log(u, '->', new URL(u).origin, '| prefix pass:', u.startsWith('http://localhost:3000'));
  } catch {
    console.log(u, '-> INVALID', '| prefix pass:', u.startsWith('http://localhost:3000'));
  }
}
Enter fullscreen mode Exit fullscreen mode 
http://localhost:3000@evil.example/x -> http://evil.example      | prefix pass: true
http://localhost:30001/              -> http://localhost:30001   | prefix pass: true
http://127.0.0.1:3000/               -> http://127.0.0.1:3000    | prefix pass: false
http://localhost:3000.evil.example/  -> INVALID                  | prefix pass: true
Enter fullscreen mode Exit fullscreen mode

The first line is the interesting one. Everything before an @ in the authority is userinfo, so localhost:3000 there is a username and a password, and the host is evil.example. The URL is perfectly valid, its origin is nothing to do with us, and it passes our test.

The fourth line is the example everybody reaches for first, and it is a dud: once a colon has started the port, 3000.evil.example is not a port, so the URL does not parse at all. Worth knowing, because if you only test that one you will conclude the prefix check is fine.

The second line is the mundane version of the same bug. If a user ever sets PORT=3000 and anything else on their machine is on 30001, a link to it reads as internal.

The third line is the bug running the other way. http://127.0.0.1:3000 is our own server, on the interface it is actually bound to, and the prefix test says it is a stranger. If any part of the UI ever linked to the loopback address by number instead of by name, the app would hand its own interface to Chrome.

Too loose and too tight, from one comparison.

What it costs today

Less than it sounds, and the reason is an accident rather than a design.

The only href in the UI whose value I did not write is the listing link in the finds list, and that one carries target="_blank". So it goes through setWindowOpenHandler, which returns { action: 'deny' } on every path. A URL that wrongly reads as internal is therefore denied and never opened anywhere: the click does nothing at all. A dead link in the list of things the app just found you is a bad bug, and it is still three steps better than the alternative.

The alternative is the will-navigate path, where a URL that wrongly reads as internal is simply allowed. The window would leave our UI and render a stranger's page, in a window with no address bar, in an app the user has typed a licence token into. Nothing routes third-party URLs down that path today. "Today" is doing a lot of work in that sentence, and the next person to add an anchor to that list will not know it.

There is a second half to this, visible in the same two handlers: shell.openExternal(url) is called on anything that is not our prefix, with no check on the scheme. openExternal hands a string to the operating system's URL opener. For https: that is a browser. For other schemes it is whatever the OS has registered, which is not a decision our app should be making on behalf of a URL that arrived from a third party. The desktop notification has the same shape:

notification.on('click', () => {
  bits.shell.openExternal(first.url).catch(() => {});
});
Enter fullscreen mode Exit fullscreen mode

That is three call sites that pass an externally sourced string straight to the platform opener.

The six lines it should be 

const INTERNAL_ORIGINS = new Set([
  `http://localhost:${serverPort}`,
  `http://127.0.0.1:${serverPort}`,
]);

function isInternal(url: string): boolean {
  try {
    return INTERNAL_ORIGINS.has(new URL(url).origin);
  } catch {
    return false;
  }
}

function openOutside(url: string): void {
  try {
    const { protocol } = new URL(url);
    if (protocol === 'http:' || protocol === 'https:') void shell.openExternal(url);
  } catch {
    /* not a URL: nothing to open */
  }
}
Enter fullscreen mode Exit fullscreen mode

new URL() does the parsing that startsWith was pretending to do, the Set fixes the loopback-by-number case, the try is there because will-navigate can hand you mailto:, blob: and strings that are not URLs at all, and the scheme allowlist means the OS only ever gets asked to open the two schemes a web link is allowed to be.

Six lines, and the thing I keep turning over is that the app already had this lesson written down somewhere else. The monitor has a guard whose entire job is to refuse a URL before anything follows it, with a public-suffix parse and a host comparison, because automation following an arbitrary link is obviously dangerous. The window had the same problem and got a prefix, because at the time it looked like a routing nicety rather than a boundary. It is a boundary. Every place your app decides "this is mine" is.

You can see the window this is about by downloading the app, which is a one-time licence rather than a subscription, with the terms on the pricing page and the setup walkthrough in the help pages. The sites it can watch are listed on the supported sites page, each with its own page for how that site's own alerts behave.

Top comments (0)