Noissue - Always add HTTP(S) prefix to URL unless anchored

Noissue - Always add HTTP(S) prefix to URL unless anchored

[Manish Jethani, 2017-05-30 06:37:49]

Patch Set 1

The filter "xttp" generates "^https?://.*xttp" whereas the filter "http" only
generates "http". It should be consistent, "http" should also only match HTTP(S)
URLs.

After this change "http" will generate "^https?://.*http".

[Sebastian Noack, 2017-05-30 07:06:16]

On 2017/05/30 06:37:49, Manish Jethani wrote:
> Patch Set 1
> 
> The filter "xttp" generates "^https?://.*xttp" whereas the filter "http" only
> generates "http". It should be consistent, "http" should also only match
HTTP(S)
> URLs.
> 
> After this change "http" will generate "^https?://.*http".

Is there any further background of this change? The current behavior seems
correct to me. With (non Content Blocker) filter matching, the filter's pattern
can match everywhere in the URL including in the protocol part.

[kzar, 2017-05-30 07:30:52]

On 2017/05/30 07:06:16, Sebastian Noack wrote:
> On 2017/05/30 06:37:49, Manish Jethani wrote:
> > After this change "http" will generate "^https?://.*http".
> 
> Is there any further background of this change?

If so could you put it into an issue as well? For reference the default is to
file an issue for changes unless they are trivial. A change like this could have
a large effect that people like Mario, Arthur and our QA team - let alone the
people reviewing the change - might need to understand, even though the code
change itself is trivial.

[Manish Jethani, 2017-05-30 08:23:11]

On 2017/05/30 07:30:52, kzar wrote:
> On 2017/05/30 07:06:16, Sebastian Noack wrote:
> > On 2017/05/30 06:37:49, Manish Jethani wrote:
> > > After this change "http" will generate "^https?://.*http".
> > 
> > Is there any further background of this change?
> 
> If so could you put it into an issue as well? For reference the default is to
> file an issue for changes unless they are trivial. A change like this could
have
> a large effect that people like Mario, Arthur and our QA team - let alone the
> people reviewing the change - might need to understand, even though the code
> change itself is trivial.

OK, I'll file an issue for this.

Sebastian: "http" should generate "^https?://.*http". Right now it doesn't
because the string "http" is being treated as a special case for no reason. Does
Adblock Plus treat "http" occurring at the beginning of the URL pattern as a
special case too? I don't think so. I've looked at EasyList+AA and "http://" is
always preceded by a "|" indicating the beginning of the URL, as it should be.

This is going to matter more when we get more URL schemes like "wss://" into the
content blocker rules.

[Sebastian Noack, 2017-05-30 09:57:50]

On 2017/05/30 08:23:11, Manish Jethani wrote:
> On 2017/05/30 07:30:52, kzar wrote:
> > On 2017/05/30 07:06:16, Sebastian Noack wrote:
> > > On 2017/05/30 06:37:49, Manish Jethani wrote:
> > > > After this change "http" will generate "^https?://.*http".
> > > 
> > > Is there any further background of this change?
> > 
> > If so could you put it into an issue as well? For reference the default is
to
> > file an issue for changes unless they are trivial. A change like this could
> have
> > a large effect that people like Mario, Arthur and our QA team - let alone
the
> > people reviewing the change - might need to understand, even though the code
> > change itself is trivial.
> 
> OK, I'll file an issue for this.
> 
> Sebastian: "http" should generate "^https?://.*http". Right now it doesn't
> because the string "http" is being treated as a special case for no reason.
Does
> Adblock Plus treat "http" occurring at the beginning of the URL pattern as a
> special case too? I don't think so. I've looked at EasyList+AA and "http://"
is
> always preceded by a "|" indicating the beginning of the URL, as it should be.
> 
> This is going to matter more when we get more URL schemes like "wss://" into
the
> content blocker rules.

In Adblock Plus (without Content Blockers) "http" would match
"http://example.com", as the filter is generally agnostic of the structure of
URLs. It's recommended to add | to the beginning of the filter if you want to
match the protocol, and EasyList might comply, but that is not
required/guaranteed in particular for third-party filter lists.

The reason I added "^https?://" to the beginning of the patterns, except it
would have been redundant, was to exclude protocols like file:// and ftp://
which Adblock Plus is used to ignore. Of course this will also cause ws:// and
wss:// to be ignored, which will be a problem now.

However, I now wonder whether we even have to bother about file:// and ftp://
URLs, or whether they aren't even go through the Content Blocker? Then we
wouldn't have to prefix these patterns with a protocol part, in the first place.

[Manish Jethani, 2017-05-30 11:09:43]

On 2017/05/30 09:57:50, Sebastian Noack wrote:
> On 2017/05/30 08:23:11, Manish Jethani wrote:
> > On 2017/05/30 07:30:52, kzar wrote:
> > > On 2017/05/30 07:06:16, Sebastian Noack wrote:
> > > > On 2017/05/30 06:37:49, Manish Jethani wrote:
> > > > > After this change "http" will generate "^https?://.*http".
> > > > 
> > > > Is there any further background of this change?
> > > 
> > > If so could you put it into an issue as well? For reference the default is
> to
> > > file an issue for changes unless they are trivial. A change like this
could
> > have
> > > a large effect that people like Mario, Arthur and our QA team - let alone
> the
> > > people reviewing the change - might need to understand, even though the
code
> > > change itself is trivial.
> > 
> > OK, I'll file an issue for this.
> > 
> > Sebastian: "http" should generate "^https?://.*http". Right now it doesn't
> > because the string "http" is being treated as a special case for no reason.
> Does
> > Adblock Plus treat "http" occurring at the beginning of the URL pattern as a
> > special case too? I don't think so. I've looked at EasyList+AA and "http://"
> is
> > always preceded by a "|" indicating the beginning of the URL, as it should
be.
> > 
> > This is going to matter more when we get more URL schemes like "wss://" into
> the
> > content blocker rules.
> 
> In Adblock Plus (without Content Blockers) "http" would match
> "http://example.com", as the filter is generally agnostic of the structure of

It wouldn't match "ftp://http-everywhere.com" though, which the generated
content blocker rule would.

> URLs. It's recommended to add | to the beginning of the filter if you want to
> match the protocol, and EasyList might comply, but that is not
> required/guaranteed in particular for third-party filter lists.
>
> The reason I added "^https?://" to the beginning of the patterns, except it
> would have been redundant, was to exclude protocols like file:// and ftp://
> which Adblock Plus is used to ignore. Of course this will also cause ws:// and
> wss:// to be ignored, which will be a problem now.
> 
> However, I now wonder whether we even have to bother about file:// and ftp://
> URLs, or whether they aren't even go through the Content Blocker? Then we
> wouldn't have to prefix these patterns with a protocol part, in the first
place.

OK, I'm working on adding support for WebSocket and WebRTC, I'll open a new code
review request when I'm done and I think we should take the discussion there.

[Manish Jethani, 2017-05-30 11:11:01]

Closed as this is redundant now since I'm already doing this as part of the
WebSocket and WebRTC changes.