Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

Fix Travis lint errors and regressions on wildcards, follow up #19479 #19531

Closed
wants to merge 2 commits into from
Closed

Fix Travis lint errors and regressions on wildcards, follow up #19479 #19531

wants to merge 2 commits into from

Conversation

cschanaj
Copy link
Collaborator

@cschanaj cschanaj commented Aug 29, 2020

This PR fixes a regression from #19479 on wildcard support for disabled site (added since #18619) and some Travis lint errors.

@zoracon is the UX redesign work being blocked by the pandemic? It would be much nicer to have a consistent CSS styles/ framework to work with when adding new UI elements. IMHO, it would be better if we could provide visual clues to our users that wildcards are supported.

try {
new URL(`http://${host}/`);
return true;
} catch {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible to fix the lint error without removing this code completely, just replace this line with:

    } catch (_) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that the URL constructor throw TypeError only. And it will not throw error for some common invalid URLs.

> try { new URL('https://*...,example.com/'); console.log('ok'); } catch(error) { console.log(error); }
ok

> try { new URL(123); console.log('ok'); } catch(error) { console.log(error); }
TypeError [ERR_INVALID_URL]: Invalid URL: 123
    at onParseError (internal/url.js:256:9)
    at new URL (internal/url.js:332:5)
    at repl:1:7
    at Script.runInThisContext (vm.js:120:18)
    at REPLServer.defaultEval (repl.js:433:29)
    at bound (domain.js:427:14)
    at REPLServer.runBound [as eval] (domain.js:440:12)
    at REPLServer.onLine (repl.js:760:10)
    at REPLServer.emit (events.js:327:22)
    at REPLServer.EventEmitter.emit (domain.js:483:12) {
  input: '123',
  code: 'ERR_INVALID_URL'
}
undefined

The disable_on_site will validate the hostname and returns an error by default. IMHO we should rely on a single-source-of-truth implementation for validations.

disable_on_site: () => {
const host = util.getNormalisedHostname(message.object);
// always validate hostname before adding it to the disabled list
if (util.isValidHostname(host)) {
disabledList.add(host);
return storeDisabledList('disable');
}
return sendResponse(false);
},

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a good idea indeed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants