Issue 7269 - Restrict URL rewrites to GETs only

There are a few HTTP methods:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods

URL rewriting was only intended for GET requests. We should stop doing it for other requests.

[Manish Jethani, 2019-02-08 12:03:55]

Patch Set 1

[a.giammarchi, 2019-02-08 12:10:27]

I have tested this and it works as expected.

Beside the minor comment that shouldn't be a blocker, this LGTM.

https://codereview.adblockplus.org/30002586/diff/30002587/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/30002586/diff/30002587/lib/requestBlocker....
lib/requestBlocker.js:207: result = {redirectUrl: rewrittenUrl};
Minor aesthetic improvement: how about we rename `rewrittenUrl` as `redirectUrl`
so that `result = {redirectUrl}` since it's `redirectUrl` that we want to set at
the end, and never `reqrittenUrl` ?

[kzar, 2019-02-08 12:21:18]

On 2019/02/08 12:01:22, Manish Jethani wrote:

Please can you file an issue for this first?

[hub, 2019-02-08 17:05:26]

https://codereview.adblockplus.org/30002586/diff/30002587/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/30002586/diff/30002587/lib/requestBlocker....
lib/requestBlocker.js:207: result = {redirectUrl: rewrittenUrl};
On 2019/02/08 12:10:27, a.giammarchi wrote:
> Minor aesthetic improvement: how about we rename `rewrittenUrl` as
`redirectUrl`
> so that `result = {redirectUrl}` since it's `redirectUrl` that we want to set
at
> the end, and never `reqrittenUrl` ?

it is named this way because it is the $rewrite option.

[a.giammarchi, 2019-02-08 17:07:32]

On 2019/02/08 17:05:26, hub wrote:
> it is named this way because it is the $rewrite option.

That locally scoped variable has no meaning though as it is, which is why I've
suggested a rename.

Anyway, not a big deal, surely not a blocker.

[Sebastian Noack, 2019-02-08 22:07:59]

Any particular reason to limit this to GET requests? I guess, there is nothing
that prevents websites to send POST requests  causing ads to show?

https://codereview.adblockplus.org/30002586/diff/30002587/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/30002586/diff/30002587/lib/requestBlocker....
lib/requestBlocker.js:201: if (details.method == "GET")
Nit: Any reason for introducing another block? Can't you just combine it with
the above check?

  if (typeof filter.rewrite == "string" && details.method == "GET")
  {
    ...
  }

[a.giammarchi, 2019-02-08 22:59:27]

On 2019/02/08 22:07:59, Sebastian Noack wrote:
> Any particular reason to limit this to GET requests? I guess, there is nothing
> that prevents websites to send POST requests  causing ads to show?

We had troubles handling OPTIONS requests, in a site where POST requests are
common too.

So I'd ask the question the other way around: have we consider all kind of
requests when we shipped the `$request` feature?

[Sebastian Noack, 2019-02-08 23:14:14]

On 2019/02/08 22:59:27, a.giammarchi wrote:
> We had troubles handling OPTIONS requests, in a site where POST requests are
> common too.
> 
> So I'd ask the question the other way around: have we consider all kind of
> requests when we shipped the `$request` feature?

Of course not, but in the end we just provided a new filter capability here, and
what it is used for is up to the filter lists. If we remove all filter
capabilities that could break a website if used careless by filter lists, there
won't be anything left.

I'm not convinced yet what exactly we should do here, but a single filter
breaking a web site doesn't seem to make for a strong case. On the other hand, I
can see scenarios where it could be helpful to redirect POST requests, in
particular considering that as soon as we limit the $rewrite filter option to
GET requests, websites might specifically start using other request methods for
circumvention.

[a.giammarchi, 2019-02-08 23:50:26]

On 2019/02/08 23:14:14, Sebastian Noack wrote:
> > So I'd ask the question the other way around: have we consider all kind of
> > requests when we shipped the `$request` feature?
> 
> Of course not, but in the end we just provided a new filter capability here

A filter capability for which scenario? 'cause it easily breaks on OPTIONS
requests.

> If we remove all filter capabilities that could break a website if used
careless by filter lists, there
> won't be anything left.

Between the most common request ever (GET) and "nothing" there is an abyss toi
me, but I understand what you are saying.

> I'm not convinced yet what exactly we should do here, but a single filter
> breaking a web site doesn't seem to make for a strong case.

I think it does 'cause we haven't tested OPTIONS cases, so would a check for
everything but OPTIONS convince you more?

'cause we don't have any argument convincing us every method is OK, since we
already have an example of a failing one.

>  On the other hand, I can see scenarios where it could be helpful to redirect
POST requests, in
> particular considering that as soon as we limit the $rewrite filter option to
> GET requests, websites might specifically start using other request methods
for
> circumvention.

They are kinda doing it already, if they realize as soon as an OPTION request
fails they circumvented us.

So let's pick our poison for the time being ?

[Sebastian Noack, 2019-02-09 00:07:02]

Can you show me an example, how a $rewrite filter works as expected (preventing
ads from showing up without side effects) except when applied to OPTION
requests?

[a.giammarchi, 2019-02-09 06:53:43]

On 2019/02/09 00:07:02, Sebastian Noack wrote:
> Can you show me an example, how a $rewrite filter works as expected
(preventing
> ads from showing up without side effects) except when applied to OPTION
> requests?

The following breaks the site if this patch is not applied:
http://hub.eyeo.com/issues/19423#note-43

[Manish Jethani, 2019-02-09 11:28:23]

On 2019/02/09 06:53:43, a.giammarchi wrote:
> On 2019/02/09 00:07:02, Sebastian Noack wrote:
> > Can you show me an example, how a $rewrite filter works as expected
> (preventing
> > ads from showing up without side effects) except when applied to OPTION
> > requests?
> 
> The following breaks the site if this patch is not applied:
> http://hub.eyeo.com/issues/19423#note-43

Let's discuss on https://issues.adblockplus.org/ticket/7269

I have modified the issue a bit to reflect the real problem we're trying to
solve here. If there's agreement, I'll update the patch.

[a.giammarchi, 2019-02-09 13:27:20]

I agree on what's described in the issue so I'd change the logic to early return
in case it's a preflight request and keep current functionality for everything
else that is not OPTIONS.

This seems the best compromise of them all.

https://codereview.adblockplus.org/30002586/diff/30002587/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/30002586/diff/30002587/lib/requestBlocker....
lib/requestBlocker.js:201: if (details.method == "GET")
I think if we go for `if (details.method == "OPTIONS") return;` we can move all
together this check to the top most part of the function.

[Manish Jethani, 2019-02-10 12:18:44]

Removing everyone here for now. Let's take the discussion to the issue tracker.