Make Location's toString() actually work

[annevk]

I mistakenly copied all boilerplate from IDL’s unforgeable interfaces concept (which was only ever used for Location) and thereby turned the fallback behavior into the actual behavior (or an exception arguably, as [[Configurable]] is false).

Fixes #2284.

[annevk]

I wrote tests: web-platform-tests/wpt#4623. Also for the non-IDL bits of Location.

https://bugzilla.mozilla.org/show_bug.cgi?id=1333045 is the Gecko bug as per the issue. Will file bugs on other browsers once there's some review.

[domenic]

LGTM, merge at will. Can you remind me why we have a special toJSON/valueOf/toPrimitive? Maybe adding a note to the spec?

[annevk]

I think the whole unforgeable setup is to avoid scripts getting confused assuming they found a way to get a correct handle on this object. And those scripts include plugins. So mostly a legacy thing? @bzbarsky would be able to confirm and if so, I suppose I could add a note.

[bzbarsky]

The original restrictions a very long time ago were prompted by Flash using things like "location.href" to determine the origin of the page for use in its own security checks.

But at this point people seem to have a general expectation of being able to use the moral equivalent of "String(location)" for some sort of security purposes, as far as I can tell. This requires the valueOf/@@toPrimitive/toString bits.

The toJSON bit was perhaps added because people expected similar things from JSON.stringify(location); worth checking the blame on that one...

[annevk]

It was added in whatwg/webidl@e1ead07, but there's no pointers... Due to the date I was able to find https://www.w3.org/Bugs/Public/show_bug.cgi?id=20008 which leads to https://lists.w3.org/Archives/Public/public-script-coord/2012JulSep/thread.html#msg144. None of those emails mention toJSON even once. Yay?

[domenic]

Should we check what browsers implement here for Location specifically, and maybe back out toJSON if it's not?

[bzbarsky]

@heycam, do you recall what the story was on toJSON on unforgeable interfaces?

As for browsers, looks like neither Safari nor Chrome nor WebKit nightly nor Edge implement the spec bits for toJSON. Firefox does implement them.

So I guess if we don't figure out specifically what this was defending against we could just remove it from Firefox and the spec.

[annevk]

@cdumez you want to look at this issue since you implemented this relatively recently in Safari. Any reason you didn't implement location.toJSON()?

[domenic]

/cc @zetafunction as I believe he's working on this area in Blink

[cdumez]

@annevk I don't remember working on this recently in WebKit. As far as I can see, Location's idl does not have a serializer so why do we expect it to have a toJSON()?

[annevk]

I'm going to tentatively assume @heycam added it because he created a generic unforgeable interfaces design while we only needed one for Location. (The same reason the design included toString() handling which is redundant with Location.)

[annevk]

Updated the test PR too.

[heycam]

I guess I added it from some kind of completeness point of view (perhaps a plugin might try to JSONify an [Unforgeable] object, but the interface doesn't have a stringifer, and the page has defined Object.prototype.toJSON to do something). But there wasn't anything concrete that this was defending against.

[domenic]

That attack still seems doable; having a stringifier doesn't save us. http://jsbin.com/xonacebobu/edit?html,console

If we want to give that strong "any stringification of location will not be hijackable" guarantee, then we should probably keep toJSON.

@cdumez: you asked in web-platform-tests/wpt#4623 (comment) why we expect valueOf to be on the instance. The reason for that, and the rest of the weird stuff going on here, is exactly that "any stringification of location will not be hijackable" guarantee. By adding own properties we ensure that when various parts of JS query for them, they get what we control, not what the page author could control by monkey-patching Object.prototype.

[cdumez]

@domenic My question was about what part of the spec mandates this. But never mind, I finally found it (https://html.spec.whatwg.org/multipage/browsers.html#the-location-interface -> To create a Location object... section).

[domenic]

Yeah, we should probably add a comment in the IDL block too...

[annevk]

So we keep toJSON()? Seems okay to me, but I'd like a bit more of a decision before I do more work here.

[domenic]

We basically should decide whether we think this "not hijackable" guarantee is a legacy thing for old flash plugins, or something we want all web code to be able to depend on. If it's legacy then we just go for shortest path to interop, which I guess means removing toJSON. If it's still something we believe in then we should keep toJSON.

I don't have a strong opinion here. Does anyone? I've personally never heard of a web developer depending on location being un-hijackable, but absence of evidence is not evidence of absence.

Regardless, we should:

• Add a comment in the IDL block linking to this section for the non-IDL stuff
• Add a note explaining why things are unforgeable and why we define all these own properties (whether that be because of a non-hijackable guarantee or because of a legacy situation)

[annevk]

@erights to what extent do you rely on location being "unforgeable"? Would you prefer it to have an immutable toJSON() or should we just let that go? (You're the one person I can think of that might care about this one way or another.)

[annevk]

I cannot seem to reach @erights. I suggest we go with the majority and drop special toJSON() treatment. I will go ahead with that plan next week to give everyone a couple more days.

[erights]

@annevk Sorry.

I think it is fine to drop special handling for toJSON.

[annevk]

Proposed commit message:

Make Location's toString() work and drop its toJSON()

I mistakenly copied all boilerplate from IDL’s unforgeable interfaces concept (which was only ever used for Location) and thereby turned the toString() fallback behavior into the actual behavior (or an exception arguably, as [[Configurable]] is false).

We also determined that only Firefox supports an unforgeable toJSON() and nobody really has a use for it, so that is dropped.

Tests: https://github.com/w3c/web-platform-tests/pull/4623.

Fixes #2284.

[domenic]

@annevk your commits look good, but I wanted to throw in some additional info. I've added a commit doing so. Let me know what you think!

[annevk]

Bugs filed for href/toString:

• https://bugs.chromium.org/p/chromium/issues/detail?id=691952
• https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10952687/

For Symbol.toPrimitive:

• https://bugs.webkit.org/show_bug.cgi?id=168295
• https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10952708/

For toJSON:

• For Edge I added a comment to the bug above.
• https://bugzilla.mozilla.org/show_bug.cgi?id=1333045 (I think this can be reused)

For valueOf:

• Reused aforementioned bugs.