Skip to content

New whitelist/block impl based on prefs #25

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.

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

Merged
merged 4 commits into from
Jun 25, 2014
Merged

New whitelist/block impl based on prefs #25

merged 4 commits into from
Jun 25, 2014

Conversation

mkaply
Copy link
Member

@mkaply mkaply commented Jun 17, 2014

Kai:

Here's the new file for you to checkout.

It's a complete rewrite of the fileblock service that I did that tries harder to block things that the user shouldn't load (certain chrome files, etc.).

It also blocks things like about:config and others by default.

The whitelist/blacklist is in a pref

@mkaply
Copy link
Member Author

mkaply commented Jun 17, 2014

I'll be more than happy to walk you through what this file does if you want. It even confuses me sometimes :)

@kaihendry
Copy link
Member

Ok, so lets assume we have:

whitelist=*.webconverger.com, *.webconverger.org
blacklist=*

(this doesn't conflict with the deprecated old way of black/white-listing, using the hosts= API)

That via https://github.com/Webconverger/webc/blob/master/etc/webc/live-config.sh will map to

pref("extensions.webconverger.whitelist", "*.webconverger.com, *.webconverger.org");
pref("extensions.webconverger.blacklist", "*");

So I am hoping that works when I try it. I'm also wondering what happens if we don't have blacklist=* at all.

I'm also wondering how the globbing works. Lets say there is a strange domain like, www.config.webconverger.com. Will that be permitted?

@mkaply
Copy link
Member Author

mkaply commented Jun 18, 2014

If you have a whitelist, but no blacklist, everything else is blacklisted.
As far as globbing, it takes the strings a a true regex, so you'd want to make sure it matched exactly what you wanted.

@kaihendry
Copy link
Member

So in the www.config.webconverger.com case, are you saying I need something like *.*.webconverger.com to make it work as intended?

@mkaply
Copy link
Member Author

mkaply commented Jun 18, 2014

So in the www.config.webconverger.com case, are you saying I need something like ..webconverger.com to make it work as intended?

Honestly, I don't know. I'm horrible at regex. I only did regex because it was requested. Personally, I think domain based whitelist/blacklist is much better, but we need the regex for file based stuff.

I'm not sure why you would block www.config.webconverger.com in your scenario though, because it's a subdomain of webconverger.com (which you allow).

@kaihendry
Copy link
Member

Ok, I'll do some of my own testing. Thanks for this Mike.

@kaihendry
Copy link
Member

Whitelist trumps blacklist

So yes, with a configuration like:

pref("extensions.webconverger.whitelist", "www.theguardian.com");
pref("extensions.webconverger.blacklist", "bbc.co.uk");

The blacklist definition is overridden. i.e. accessing google.com does not work. I guess this just needs to be documented.

Fails silently

Ideally when accessing a prohibited URL, it has a message saying this side is blocked.

The current experience feels like there is something wrong with Webconverger I fear.

Dependencies of page are blocked

With a preference like pref("extensions.webconverger.whitelist", "www.theguardian.com"); the experience is http://s.natalian.org/2014-06-19/1403172253_1364x748.png. I.e. CSS and images are black listed since they are on different URLs, like static.guim.co.uk.

Is there a way of allowing the dependencies of the page through, but only filter the URL on the address bar?

@kaihendry
Copy link
Member

I think blacklisting can be ignored. The use case here is that people want to simply list their trusted domains for their deployment. So the API imo should look like:

 homepage=http://www.bl.uk/
 whitelist=bl.uk

So this would block anyone going to Google.com. However dependencies of http://www.bl.uk/ (and any subdomain) which include twitter et al... would work, but only in that page.

As for the experience when navigating away to a blacklisted site from the URL bar. I think the extension could simply say: "Sorry, this Webconverger kiosk has been configured to only allow: bl.uk".

@mkaply
Copy link
Member Author

mkaply commented Jun 19, 2014

Yeah, you're probably right. blacklist isn't really necessary here.

I think we do need multiple sites on the whitelist, though.

As far as a message when navigating away, I think I can add that in. Unfortunately, that will be our first ever message in Webconverger, won't it? So we'll need to translate it...

@kaihendry
Copy link
Member

Hey Mike. When you said "we do need multiple sites on the whitelist", means that the extension can't permit dependencies of a page? If so, that kinda sucks... :/

@mkaply
Copy link
Member Author

mkaply commented Jun 19, 2014

When you said "we do need multiple sites on the whitelist",

No, I just meant that in your example, you're showing one site and having a message associated with one site. In reality, the whitelist can be anything.

@mkaply
Copy link
Member Author

mkaply commented Jun 20, 2014

You are brilliant. This is a much better implementation. When a site is whitelist, all content on that site displays no matter what. I can't believe I never thought of that.

@kaihendry
Copy link
Member

Thanks Mike. This is a great start and I've now managed to do some testing this morning and I've noticed the comma separated domains aren't being trimmed right, despite me seeing your code right there. I'm not sure how to log to stderr from components/fileBlockService.js, so I wasn't able to debug myself!

For example pref("extensions.webconverger.whitelist", "bbc.co.uk, example.com"); example.com will not be on the whitelist due to the leading space character (I assume). When I remove the leading space, it whitelists correctly.

BETTER MESSAGE

My preference is to show the user what the white list is exactly, so there is no ambiguity. So a alert like:

Not allowed, whitelist only permits: "bbc.co.uk,webconverger.com,example.com,google.com"

Gets my vote. Lets worry about i18n later.

DDG search breaking

If you just type a term in and enter, Firefox redirects you to the default search engine, DDG in our case and we get an error (if it's not whitelisted). It's a terrible UX, though at this point I'll just document it.

Back/forward UX

pref("extensions.webconverger.whitelist", "bbc.co.uk,google.com");

I noticed when going back and forth using the back/forward buttons between whitelisted domains, news.bbc.co.uk & google.com, I got an error. I'm not sure why, but that's a concern. I suspect it's because google.com redirects to google.co.uk here ...

@mkaply
Copy link
Member Author

mkaply commented Jun 25, 2014

Trim issue was silly. You can see it in my patch.

I fixed the message as well.

Should we just add DDG to the default whitelist? Or is that too heavy handed?

Or maybe we should be turning off that behavior in the kiosk (going to google for searches).

As far as the bbc/google problem goes, I don't see that in the US...

@kaihendry
Copy link
Member

Great, your fixes work!

Not sure if it's easy to disable 'URL bar search' altogether. Probably not worth worrying about.

I'll be rolling this into the release I'm planning to make tomorrow. Awesome work. :)

kaihendry added a commit that referenced this pull request Jun 25, 2014
New whitelist preference, extensions.webconverger.whitelist
@kaihendry kaihendry merged commit e1665b1 into Webconverger:master Jun 25, 2014
@mkaply
Copy link
Member Author

mkaply commented Jun 25, 2014

Not sure if it's easy to disable 'URL bar search' altogether. Probably not worth worrying about.

I think it's really easy. Just a pref if I remember correctly.

@mkaply
Copy link
Member Author

mkaply commented Jun 25, 2014

Set keyword.enabled to false.

@kaihendry
Copy link
Member

Can that be applied via the prefs= API?

That makes me think, extensions.webconverger.whitelist could be too (surely). But
the prefs.js isn't cached like the Webconverger config is.

@mkaply
Copy link
Member Author

mkaply commented Jun 25, 2014

Can that be applied via the prefs= API?

Yes.

That makes me think, extensions.webconverger.whitelist could be too (surely). But the prefs.js isn't cached like the Webconverger config is.

Yes, it can as well. It's just a preference. I assume you have code somewhere that is converting the whitelist= stuff to the preference,

@kaihendry
Copy link
Member

Yes, just about to implement that whitelist= API nonetheless. Thanks for the workaround. Hopefully we have all the bases covered here nicely.

kaihendry added a commit to Webconverger/webc that referenced this pull request Jun 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants