Fix this arg for URLSearchParams prototype call

[JosephusPaye]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• New feature
• Other, please explain:

What changes did you make? (provide an overview)

Changed the this arg passed to the URLSearchParams prototype call in the Headers proxy handlers for append(), set(), delete(), has(), getAll(). Currently it's set to receiver, but it should be target, as it is with the keys() handler.

Which issue (if any) does this pull request address?

#917

Is there anything you'd like reviewers to know?

Ran the tests in Node (v14.15.0) and everything continues to pass. Manually tested in Electron (v11.1.1, with Node v12.18.3 and Chrome v87.0.4280.88), and it fixes the issue described in #917.

[tekwiz]

The fact that tests were already passing is concerning. Please add a test case

[JosephusPaye]

The error is not reproducible in Node, which is why the tests are passing. It occurs in Electron though, which suggests it could be a difference in the implementation of URLSearchParams in Node vs Chromium, or something different about how that class interacts with a Proxy. I don't have any evidence of a difference though, other than the fact that the library definitely fails in Electron with a proxy error originating from the lines changed in the PR, as documented in #917.

From reading the current code, as well as the Proxy docs, it seems the current usage only works incidentally. From what I can tell, the object being bound to this is meant to be the original object being proxied (target), not receiver (which is itself a proxy). This will match the correct usage in the keys() handler.

I'm happy to add a test if you can point me to a way to run the Mocha tests in Electron. I've tried this library, but it doesn't work (I get ES module errors originating from within Mocha's code about using require() instead of import).

[TimothyGu]

The cause for this bug is inconsistent brand checks between Node.js' implementation of URLSearchParams compared to browser's. Consider the following:

const u = new URLSearchParams();
u.get.call(Object.create(u), '');

In the browser (and hence Electron), the second line results in a thrown exception, usually with a message like "Invalid invocation" or something similar. However, Node.js uses a different style of brand checks that checks if the object has a special symbol property. Since property accesses fall back to the prototype object in JavaScript, Node.js is fine with this code, even though Object.create(u) is not technically a URLSearchParams object. (The blame for this incompatibility could fall on me, as I was the person who implemented much of URLSearchParams in Node.js.)

The v3 node-fetch way of implementing Headers depends on the Node.js behavior. The current code does:

return URLSearchParams.prototype[p].call(
	receiver,
	String(name).toLowerCase(),
	String(value)
);

where receiver is the Proxy object returned by Headers.constructor. The Node.js URLSearchParams implementation will happily accept this code, since property accesses to receiver eventually go to target, which is a URLSearchParams object. However, the browser will not, since receiver is the Proxy object and not the URLSearchParams object. (This is why replacing receiver with target works.

As far as this PR goes, I think it is absolutely the right fix. In fact, I encourage people to merge this as soon as possible, since having a popular package like node-fetch depend on the incorrect behavior of Node.js itself prevents Node.js from fixing this bug. If you want to actually create a test for this PR, the best bet is to monkey-patch the Node.js URLSearchParams so that it acts more like the browser:

// Monkey-patch URLSearchParams to do brand checks more accurately.
// This makes the implementation more similar to browsers (or Electron),
// and works around a Node.js bug.

const prevCtor = URLSearchParams;
const uspSet = new WeakSet();
URLSearchParams = function(...args) {
  const obj = Reflect.construct(prevCtor, args, new.target);
  uspSet.add(obj);
  return obj;
};
Object.setPrototypeOf(URLSearchParams, prevCtor);
URLSearchParams.prototype = prevCtor.prototype;
URLSearchParams.prototype.constructor = URLSearchParams;

const prevHas = prevCtor.prototype.has;
URLSearchParams.prototype.has = function (...args) {
  if (!uspSet.has(this)) {
    throw new TypeError('has called on an object that is not a URLSearchParams');
  }
  return Reflect.apply(prevHas, this, args);
};

/////////////////////////////////////////////

const { Headers } = require('node-fetch');
const h = new Headers();
console.log(h.has('abc'));

This code fragment throws an error under the current definition of Headers, but passes after this PR.

---

That said, I think it's absolutely crazy that Headers is a subclass of URLSearchParams, and that canonical Headers objects are proxies. I have filed #1119 to consider reverting this new implementation.

[TimothyGu]

As I mentioned in my comment, IMO testing this change is more trouble than it's worth. LGTM as it is.

[JosephusPaye]

Thanks for the details @TimothyGu, very illuminating.

[bitinn]

Look like we could have used some expertise in this PR, personally I am convinced by Timothy, but would be good to hear some feedbacks from node.js team members :)

Same regarding #1119 too.

Stale