cli(run): prevent file:// from running, return error

[justinribeiro]

Per #5924, this prevents file:// urls from running in LH.

A few things:

1. The regex doesn't actually look for file, rather it checks to verify if the URL is a valid form. To see what the regex matches, I put up https://regexr.com/3um70 for easy glance (which is pretty much just an extended version of Mathias' list from https://mathiasbynens.be/demo/url-regex)
2. The regex does make an exception for chrome:// urls
3. I placed this in run as opposed to running checks in cli-flags mostly because it seemed the cleaner route. Re-uses handleError in this case (though the error message placement probably needs to move)
4. I added a specific test that utilizes jest.spyOn and a mock for the process.exit() call that handleError makes when failing on file:/// (though the console.error will still appear when running tests):
   

Resolves #5924.

[brendankenny]

I think we should move this check into core, rather than the cli, so that all the clients get it. I don't believe extensions are allowed to debug file:// tabs anymore, but I believe devtools can and anyone calling lighthouse as a node module definitely can.

Preferably the check can be early on, so maybe here in the module index? We can simplify a bit, something like

const URL = require('./lib/url-shim.js');
const LHError = require('./lib/lh-error.js');
const ALLOWED_PROTOCOLS = /^(chrome|https?):$/;

// ...

if (url && !ALLOWED_PROTOCOLS.test(new URL(url).protocol)) {
  throw new LHError(LHError.errors.INVALID_URL);
}

Then add the INVALID_URL error you've defined over in LHError. new URL() will check for a valid URL, leaving only the protocol check to us.

Testing should also be simpler, too (just asserting it throws).

WDYT? Biggest downside seems like it'll be that we're doing work in the CLI (e.g. starting Chrome, etc) that will be rendered useless by the time it gets here, but we have other checks that end up like that, and with any luck it's a something a user will only run into once (or once every typo :).

[brendankenny]

we might want to allow about:blank, too? It doesn't actually work in Lighthouse, but it's useful to get the NO_DOCUMENT_REQUEST error. Maybe that's dumb, though.

@paulirish

[brendankenny]

@paulirish confirms that is dumb, so nevermind about the about:blank part :)

[justinribeiro]

@brendankenny Sounds good! My initial thought was that this could run earlier, but I was unsure if firing up Chrome and then failing out if the URL was invalid was a okay. Since that sounds alright, I'll shuffle this to check earlier and get this PR updated.

[justinribeiro]

@brendankenny The one difference in the above implementation from your guidance is that instead of running a regex check on allowed protocols, I broke that out into URL.isProtocolAllowed and added the allowed protocols similar to the TLD defs. I felt like this was easier to maintain and test, but if you want that removed and just with a regex check within core/index, I can swap it out.

Other than that, operates as you noted above.

[brendankenny]

sorry for the delay, this LGTM!

🚫 📁 🏃