give query param meta maps a null prototype

[ubeddulla]

forEachQueryParam and Route.queryParamsDidChange look up query param names taken from the url against the plain objects built in _queryParamsFor and _qp, so /?toString=1 finds Object.prototype.toString, treats it as query param metadata, and the transition dies in qp.route.deserializeQueryParam. any app is affected, including one that declares no query params at all. forEachQueryParam already guards the queryParams side of that loop with hasOwnProperty and only misses the map side, so giving both maps a null prototype fixes it at the source and matches _qpCache and _engineInstances a few lines up. test covers /?toString=1 arriving alongside a real qp.

[NullVoxPopuli]

Do you have a real app that's running in to this?

[ubeddulla]

No real app report behind it, no. I hit it reading the lookup code in forEachQueryParam and Route.queryParamsDidChange, then confirmed it with the test that's in this PR.

For an app to hit it the URL just needs a param named after something on Object.prototype: ?toString=1, ?constructor=1, ?valueOf=1. On main that's enough to kill the transition, in two different ways depending on the app:

• no query params declared anywhere, visit /?toString=1 → TypeError: Cannot read properties of undefined (reading 'deserializeQueryParam'), from _deserializeQueryParams → forEachQueryParam
• a qp declared, visit /?toString=1&foo=baz → Assertion Failed: The key provided to get must be a string or number, you passed undefined in _optionsForQueryParam, because qp.urlKey is undefined

Either way the transition aborts and the app doesn't render.

How a URL like that shows up in practice is the boring part: a crawler or scanner tacking params on, a marketing/analytics param getting passed through, someone hand editing a link. Nothing is attacker controlled beyond the param name itself, so it's not a pollution issue, it just means a link anyone can construct blanks the page for whoever follows it.

The test in the PR is the repro. It fails on main and passes with the two Object.create(null) calls. If an app level test feels heavy for this I can swap it for a smaller unit test against _queryParamsFor.

[NullVoxPopuli]

Can you add a test for when someone deliberately defines a queryparam named toString to their app?

contlorrer.get(xtoString')

I believe this pr breaks that, which means if you really want to pursue this you'll need an RFC, and willingness to implement via optional feature flag who's default changes in the next major: v8

[ubeddulla]

Wrote that test and ran it both ways:

["@test A query param deliberately named after an Object.prototype property still works"](assert) {
  assert.expect(2);
  this.setSingleQPController("index", "toString", "wat");
  return this.visit("/?toString=lol").then(() => {
    assert.equal(this.currentURL, "/?toString=lol");
    assert.equal(this.getController("index").get("toString"), "lol");
  });
}

It fails on main too, so this PR is not what breaks it. On main it dies in the DEBUG collision check:

You're not allowed to have more than one controller property map to the same query param key, but both undefined and ...

which is qpsByUrlKey["toString"] finding the inherited method. With this branch that one goes away and it gets one step further, then fails in Route#paramsFor:

The route 'index' has both a dynamic segment and query param with name 'toString'

because the assert there is !params[key] against a spread object. Fix that with an own-property check and it moves again, to _optionsForQueryParam, where get(queryParams, qp.urlKey) picks up Object.prototype.toString and trips assert("options exists", ...).

So a deliberately declared toString qp is broken today, in at least three more places than the two I touched. I stopped there rather than keep growing the diff, since that's clearly past what this PR should be doing, and it means there's no behavior here to RFC away since nobody can be relying on it working.

Happy either way: I can leave this as the narrow fix for the undeclared case, or close it if you'd rather the whole qp-name-shadowing thing get looked at as one piece.

[NullVoxPopuli]

my question is more: "does anyone actually need this 'fixed'"?

[ubeddulla]

Fair question, and the honest answer is I don't know of anyone. No open or closed issue in this repo mentions it, and I don't have an app hitting it — I found it reading the lookup code, not from a bug report.

So it's a real crash but a speculative one: a link with ?toString=1 on it blanks the page, and nobody's complained about that happening. Given the declared-toString case is broken in several other places anyway, this only really buys you not dying on a param an app never declared.

That's thin on its own, so I'm fine closing it. Say the word and I'll close, or leave it sitting if you'd rather keep it around as a note for whenever the qp name-shadowing stuff gets looked at properly.

[NullVoxPopuli]

Yeah, with router stuff getting reworked in the near future, I'd prefer to not touch the existing behaviors