Issue 7294 - Pass necessary functions to strip-fetch-query-parameter

We forgot to pass `toRegExp` and `regexEscape` to the injector.

[Manish Jethani, 2019-02-21 08:15:43]

Patch Set 1

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
File lib/content/snippets.js (right):

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
lib/content/snippets.js:864: if (typeof fetch_ != "function")
Being defensive, just in case fetch does't exist (for whatever reason). It
does't hurt.

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
lib/content/snippets.js:879: return (() => {}).apply.call(fetch_, this, args);
This is because you can have a function with a null prototype. Again, just being
defensive. We don't want our snippets to break any web pages.

[a.giammarchi, 2019-02-21 08:27:35]

One change is OK, but the other one is a bit useless and doesn't really solve
much, IMO

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
File lib/content/snippets.js (right):

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
lib/content/snippets.js:879: return (() => {}).apply.call(fetch_, this, args);
On 2019/02/21 08:15:43, Manish Jethani wrote:
> This is because you can have a function with a null prototype. Again, just
being
> defensive. We don't want our snippets to break any web pages.

IMO this is super ugly and it doesn't prevent possible issues via
Function.prototype.apply overrides.

I believe we should stick with the standard and use `return fetch_(...args);`
because `fetch`, like any other global context, doesn't need a context to
execute so it'd be silly to expect one.

Moreover, the fetch *we* trap, doesn't want to pass through any context 'cause
it's supposed to be the original global one which already acts like that.

If you disagree, at least please consider using `fetch.apply.call(fetch, this,
args)` so that no runtime arrow is needed each time, but again, any
Function.prototype.apply override would easily catch that, no matter if it's an
Arrow or not.

Alternatively, we could secure execution through call/apply traps
http://webreflection.blogspot.com/2011/10/bind-apply-and-call-trap.html
but that requires some shared utility which means bigger refactory.

[a.giammarchi, 2019-02-21 08:28:18]

like any other global function *

[Manish Jethani, 2019-02-21 08:57:16]

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
File lib/content/snippets.js (right):

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
lib/content/snippets.js:879: return (() => {}).apply.call(fetch_, this, args);
On 2019/02/21 08:27:35, a.giammarchi wrote:
> On 2019/02/21 08:15:43, Manish Jethani wrote:
> > This is because you can have a function with a null prototype. Again, just
> being
> > defensive. We don't want our snippets to break any web pages.
> 
> IMO this is super ugly and it doesn't prevent possible issues via
> Function.prototype.apply overrides.

This is not about overrides but rather about the possibility that the prototype
can be null.

  function fetch() {
    // Custom implementation of fetch
  }
  fetch.__proto__ = null; // For some reason the developer decided to null this
out

Our code would bomb here, unless we use the original version of apply.

> I believe we should stick with the standard and use `return fetch_(...args);`
> because `fetch`, like any other global context, doesn't need a context to
> execute so it'd be silly to expect one.
>
> Moreover, the fetch *we* trap, doesn't want to pass through any context 'cause
> it's supposed to be the original global one which already acts like that.

What if the fetch on the page is a custom implementation? How do we know that
it's the standard one? Who are we to decide how the fetch function should work?
Our wrapper should simply pass on the this, even if it's null or undefined. It
should be transparent.

> If you disagree [...]

I do disagree but I also think this part of the change is unrelated. I can
revert this part. Let me do that.

[Manish Jethani, 2019-02-21 09:00:37]

Patch Set 2

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
File lib/content/snippets.js (right):

https://codereview.adblockplus.org/30013555/diff/30013556/lib/content/snippet...
lib/content/snippets.js:879: return (() => {}).apply.call(fetch_, this, args);
On 2019/02/21 08:57:16, Manish Jethani wrote:
> I do disagree but I also think this part of the change is unrelated. I can
> revert this part. Let me do that.

Done.

[a.giammarchi, 2019-02-21 10:17:39]

On 2019/02/21 08:57:16, Manish Jethani wrote:
> What if the fetch on the page is a custom implementation?

Well, any script (including 3rd parts) would invoke it as `fetch(...)` and if
it's a custom implementation it will get the default global context
automatically, so there's literally less chances this break invoking directly
the function, than using that `apply.call(...)` indirection.

As example, if you write in console `fetch.call(null, 'index.html')` it works
regardless, and you write `fetch.call(null, 'fail')` it shows `Failed to execute
'fetch' on 'Window'`, because fetch cannot possibly ever accept, or use, any
other context.

[a.giammarchi, 2019-02-21 10:26:58]

last hint before giving the ok

https://codereview.adblockplus.org/30013555/diff/30013561/lib/content/snippet...
File lib/content/snippets.js (left):

https://codereview.adblockplus.org/30013555/diff/30013561/lib/content/snippet...
lib/content/snippets.js:875: return fetch_.apply(this, args);
cannot we just `return fetch_(...args)` after latest discussion?
alternatively patch `apply` via bind right away and `return apply(fetch_, this,
args)`?

example
```
let fetch_ = window.fetch;
// ...
function fetch(...args) {
  // ....
  return apply(fetch_, this, args);
}
let apply = fetch.call.bind(fetch.bind)(fetch.call, fetch.apply);
window.fetch = fetch;
```
that ensure, unless they have modified Function.prototype.apply/bind/call that
no matter how they patched fetch, it's invoked.

It suffers same "maybe they wrapped apply/call/bind" but in case it's executed
before they do that, and if there is only one param to removed, it'll be secured
"forever".

What do you think?

[Manish Jethani, 2019-02-21 10:35:04]

https://codereview.adblockplus.org/30013555/diff/30013561/lib/content/snippet...
File lib/content/snippets.js (left):

https://codereview.adblockplus.org/30013555/diff/30013561/lib/content/snippet...
lib/content/snippets.js:875: return fetch_.apply(this, args);
On 2019/02/21 10:26:58, a.giammarchi wrote:
> cannot we just `return fetch_(...args)` after latest discussion?
> alternatively patch `apply` via bind right away and `return apply(fetch_,
this,
> args)`?
> [...]

I don't see why we should assume anything about the fetch function. Maybe it is
completely different than the fetch API that we expect. Maybe there's a custom
implementation that requires `this`. How do we know? We should just pass on the
`this` that we get instead of trying to "correct" it; that is the decent thing
to do. It is not the snippet's business to decide whether the fetch function in
the document should get a `this` -- if the caller has passed a `this`, we should
pass it on.

This applies in general to all API wrappers. We need to interfere as little as
possible with the  code.

Anyway, since we decided not to make any more changes in this patch, let's take
this discussion elsewhere.

[a.giammarchi, 2019-02-21 10:38:19]

On 2019/02/21 10:35:04, Manish Jethani wrote:
> Anyway, since we decided not to make any more changes in this patch, let's
take
> this discussion elsewhere.

I understnd your point, which is why In the rest 90% of the comment I've
suggested to secure apply and pass the context.

Any reason not to do that instead of a runtime O(n) `(()=>{}).apply.call` ?

[a.giammarchi, 2019-02-21 10:42:38]

On 2019/02/21 10:38:19, a.giammarchi wrote:
> Any reason not to do that instead of a runtime O(n) `(()=>{}).apply.call` ?

alternatively: wouldn't be at least cleaner and more efficient to do instead:

```
let {apply} = Function; // pass through the prototype anyway
return apply.call(fetch_, this, args);
```

if you don't like the secured apply outside, I think I would prefer at least
that.
Maybe engines can be smart about `(()=>{}).apply` but the lookup to the
prototype is done anyway, and if we can avoid useless GC pressure maybe we
should?

[Manish Jethani, 2019-02-21 11:47:00]

On 2019/02/21 10:42:38, a.giammarchi wrote:
> On 2019/02/21 10:38:19, a.giammarchi wrote:
> > Any reason not to do that instead of a runtime O(n) `(()=>{}).apply.call` ?
> 
> alternatively: wouldn't be at least cleaner and more efficient to do instead:
> 
> ```
> let {apply} = Function; // pass through the prototype anyway
> return apply.call(fetch_, this, args);
> ```
> [...]

We could explore this, but let's do it outside this patch. The goal here was
really to fix the bug.

[a.giammarchi, 2019-02-21 11:59:35]

On 2019/02/21 11:47:00, Manish Jethani wrote:
> On 2019/02/21 10:42:38, a.giammarchi wrote:
> > On 2019/02/21 10:38:19, a.giammarchi wrote:
> > > Any reason not to do that instead of a runtime O(n) `(()=>{}).apply.call`
?
> > 
> > alternatively: wouldn't be at least cleaner and more efficient to do
instead:
> > 
> > ```
> > let {apply} = Function; // pass through the prototype anyway
> > return apply.call(fetch_, this, args);
> > ```
> > [...]
> 
> We could explore this, but let's do it outside this patch. The goal here was
> really to fix the bug.

LGTM then.