1321299 - toString() broken on cross-origin objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jessilyn Davis]

Mozilla uses Salesforce Marketing Cloud to manage our email programs. It is one of the leading email marketing platforms worldwide - and it's currently broken in Beta, Nightly & Dev.

In filing a bug with the Salesforce team, I got this reply:


Mozilla plans to implement ES6 Symbol.toStringTag property in Firefox version 51. (https://bugzilla.mozilla.org/show_bug.cgi?id=1114580) 

In ES6, Object.prototype.toString does not use as Class meta property anymore, so they are adding Symbol.toStringTag to all DOM prototype objects. This will cause issues in our script where we access an object that is not in the current domain. The error message is "Permission denied to access property Symbol.toStringTag".

If Mozilla release the version as it currently is, there is a potential to see multiple issues with Firefox users. There are multiple apps that would not work.
Looking at Mozilla bug page, it appears that they are aware of this issue as it is a child bug of the Symbol.toStringTag implementation, but according to the site, no work is currently being done. https://bugzilla.mozilla.org/showdependencytree.cgi?id=1114580&hide_resolved=1

Applications that could have issue and possibly not work in Firefox but not limited to are as follow:
Email Studio
MobileConnect 2.0
Web and Mobile Analytics
Approvals Service
Content Builder

According the release schedule, this version is scheduled to go out on January 24, 2017.


Is there a way to ensure that this fix happens before Jan 24th's release? 


Risk: we may loose Firefox users who use Salesforce (more than just Mozilla)

Comment 1

[Jessilyn Davis]

Attachment: JS_error.png

Comment 2

[Dennis Schubert [:denschub][OOO, back Jan 12]]

Thanks for filing, Jessilyn. Moving this into Tech Evangelism so we have a chance to have a look at it and do cross-checks with other browsers before figuring out actions to take.

Comment 3

[Mike Taylor [:miketaylr]]

Tom, any thoughts about this?

Comment 4

[Mike Taylor [:miketaylr]]

Dennis can add STR (I believe he's looking into this), but I want to make sure we track this ASAP, however late in the release cycle. :/

Comment 5

[Dennis Schubert [:denschub][OOO, back Jan 12]]

Attachment: Simple testcase

I've attached a simple test case and it's hosted on http://stuff.schub.io/mozilla/testcases/1321299/. Click the button to see an exception in Firefox, but "[object Object]" in Chrome.

Comment 6

[Dennis Schubert [:denschub][OOO, back Jan 12]]

This is a regression from bug 1114580, where we implemented Symbol.toStringTag. Renaming the title to be more easily understandable and more accurate.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Much simpler testcase:

  data:text/html,<iframe src="http://example.com"></iframe><script>onload = function() { try { alert({}.toString.call(frames[0])) } catch (e) { alert(e); } }</script>

This should work per spec.  See https://html.spec.whatwg.org/multipage/browsers.html#crossorigingetownpropertyhelper-(-o,-p-) step 1.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8816294 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Review of attachment 8816294 [details] [diff] [review]:
-----------------------------------------------------------------

So, is this basically what I proposed in bug 1114580#c24? Did that just get dropped on the floor or something?

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +155,5 @@
>      return CrossOriginOpaque;
>  }
>  
>  bool
> +IdIsCrossOriginFilteredSymbol(JSContext* cx, JS::HandleId id)

Let's call this IsCrossOriginWhitelistedSymbol, and add a comment that the symbol value itself is nerfed.

@@ +195,5 @@
> +               IdIsCrossOriginFilteredSymbol(cx, id)) {
> +        // We always allow access to @@toStringTag, @@hasInstance, and
> +        // @@isConcatSpreadable.  But then we nerf them to be a value descriptor
> +        // with value undefined; this happens in FilteringWrapper's
> +        // FilterPropertyDescriptor.

This comment will need to change.

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +61,5 @@
> +        // of whether our target has them.
> +        //
> +        // XXXbz Should we do this for the cases when we have explicit
> +        // exposedProps exposing these symbols?  It's not clear how to do that,
> +        // exactly...

As much as I really don't care about ExposedProps, this comment does point to the fact that this check doesn't really belong here, and should move to CrossOriginXrayWrapper::getPropertyDescriptor.

Moreover, the whole bit about our target having them or not having them seems to miss the point - this is an XrayWrapper with no waiver or expando object, so what we see from the underlying wrapper is entirely within our own control. And it's not clear to me what code is responsible for making these symbol-valued properties show up. For a normal DOM Xray we'd just inherit it off the prototype, but we don't have a prototype here.

So we need to explicitly add the property in getPropertyDescriptor, but then we also need to add it in getOwnPropertyKeys or whatever it's called these days. I'd think that the underlying wrapper implementation shouldn't supply these properties, so we should be able to strongly assert that they don't exist before stubbing them in.

I'm pretty sure a test for this will fail, and the only reason the tests pass right now is that there is no call to getOwnPropertySymbols. Can you add that please? And maybe a test using Object.getOwnPropertyDescriptors, which IIUC handles both?

::: testing/web-platform/tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.html
@@ +71,5 @@
>   */
>  
> +var whitelistedWindowPropNames = ['location', 'postMessage', 'window', 'frames', 'self', 'top', 'parent',
> +                                  'opener', 'closed', 'close', 'blur', 'focus',
> +                                  'length'];

Nit: Is there a reason to wrap this last item?

@@ +180,5 @@
>    assert_equals(desc.enumerable, false, "property descriptor for " + propName + " should be non-enumerable");
>    assert_equals(desc.configurable, true, "property descriptor for " + propName + " should be configurable");
> +  if (isSymbol) {
> +    assert_true("value" in desc,
> +                "property descriptor for " + propName + " should be a value descriptor");

Shouldn't we also check that it's undefined here?

@@ +270,5 @@
> +addTest(function() {
> +  assert_array_equals(Object.getOwnPropertySymbols(C), [],
> +                      "Object.getOwnPropertySymbols() should not find any symbol-named properties for cross-origin Window");
> +  assert_array_equals(Object.getOwnPropertySymbols(C.location), [],
> +                      "Object.getOwnPropertySymbols() should not find any symbol-named properties for cross-origin Window");

Shouldn't this say "Location"?

::: testing/web-platform/tests/html/browsers/origin/cross-origin-objects/win-documentdomain.sub.html
@@ +1,4 @@
>  <!DOCTYPE html>
>  <html>
>  <head>
> +  <script src="/common/get-host-info.sub.js"></script>

Hm, why is this needed?

Comment 10

[Bobby Holley (:bholley)]

Oh, and of course you _did_ add a test for getOwnPropertySymbols, and that's what the whole spec issue was about. Sorry about that, I was looking at the current version of the file rather than your patch.

I think the spec is basically an oversight, so we should just change it right now and write this patch (and the test) properly, rather than having to come back and do it later.

Comment 11

[Gerry Chang [:gchang]]

Track 51+ as this impacts real users who use Salesforce and other applications.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Comment 13

[Boris Zbarsky [:bzbarsky]]

> So, is this basically what I proposed in bug 1114580#c24?

Pretty similar, yes, independently derived.  ;) 

> Did that just get dropped on the floor or something?

Apparently, yes.  :(

> Let's call this IsCrossOriginWhitelistedSymbol, and add a comment that the symbol value itself is nerfed.

Done.  Comment says:

  // Check whether the given jsid is a symbol whose value can be gotten
  // cross-origin.  Cross-origin gets always return undefined as the value.

> As much as I really don't care about ExposedProps, this comment does
> point to the fact that this check doesn't really belong here, and should
> move to CrossOriginXrayWrapper::getPropertyDescriptor.

Done.

> Nit: Is there a reason to wrap this last item?

Well, my editor did that because 80 chars. ;)  Undone.

> Shouldn't we also check that it's undefined here?

Er... yes!  I meant to, and thought I had, but clearly not.  Good catch.  Fixed.

> Shouldn't this say "Location"?

Yes.  Fixed.

> Hm, why is this needed?

It's not, strictly speaking.   I just ran all the tests in the cross-origin-objects dir (to see what test coverage we needed), found that one of them was marked as timing out _without_ this patch, and fixed it to not do that: it's using a get_port function and without the script that defines that function it ends up with exceptions and times out.

> so we should just change it right now and write this patch (and the test) properly

OK.  Done.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from previous patch, for reference

Comment 15

[Boris Zbarsky [:bzbarsky]]

Note that if the final spec ends up defining the ordering expected out of [[OwnPropertyKeys]], then we may need to adjust our code accordingly and can just change the spec to using array_equals.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I meant change the _test_ to using _assert_array_equals_.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8816374 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Review of attachment 8816374 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those fixes.

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +248,5 @@
> +        return false;
> +    }
> +
> +    props.infallibleAppend(
> +      SYMBOL_TO_JSID(JS::GetWellKnownSymbol(cx, JS::SymbolCode::toStringTag)));

Worried about duplicating this list with the one in IsCrossOriginWhitelistedSymbol. Can you make a static array of the SymbolCodes and then iterate over that in both places?

::: testing/web-platform/tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.html
@@ +267,5 @@
>                        "Object.getOwnPropertyNames() gives the right answer for cross-origin Location");
>  }, "[[OwnPropertyKeys]] should return all properties from cross-origin objects");
>  
> +// Compare two arrays that need to have the same elements but may be in
> +// different orders.

Nit: I think it would be a bit more idiomatic to define a symbol comparator, and sort both the reference and test arrays.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f494ff3b83b
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined.  r=bholley

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Can you make a static array of the SymbolCodes and then iterate over that in both places?

Done.

> I think it would be a bit more idiomatic to define a symbol comparator

Yes, but I haven't found any way to define one that makes any sense, unless I assume the symbols are all built-in ones.  I'll add a comment.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8816550 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1114580
[User impact if declined]: Some sites won't work.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Verifying that this
   fixes the SalesForce site would be good.  I don't have steps to reproduce,
   but maybe Jessilyn does.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Slightly.
[Why is the change risky/not risky?]: Changes web-visible behavior.  That said,
   the new behavior is one that other browsers (e.g. Chrome) already ship and
   is the one we had decided on a while ago to avoid breaking sites.  The real
   question is whether this is more or less risky than backing out bug 1114580.
   I suspect it's less risky.
[String changes made/needed]: None.

Comment 22

[Jessilyn Davis]

Dennis Schubert has login access to Salesforce to help troubleshoot this bug. Please let him know what version of Firefox to use and he can verify if the fix works.  Thanks!

Comment 23

[Boris Zbarsky [:bzbarsky]]

I guess tomorrow's nightly might have the changes, depending on when sheriffs merge stuff.

If Dennis wants to test a tinderbox build and tells me what OS I can link him to a relevant binary.

Comment 24

[Dennis Schubert [:denschub][OOO, back Jan 12]]

I have confirmed the patch using tinderbox build 1480710284. SalesForce is working, and so are all the testcases.

Everyone, thank you for reacting this fast!

Comment 25

[Jessilyn Davis]

\o/ Yes! Thank you all!!

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5f494ff3b83b

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8816550 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

web compat fix, take in aurora52

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b563a3069c60

Comment 29

[Nathan Froyd [:froydnj]]

bz, it looks like beta is also affected too (bug 1114580 says it was fixed in 51 and 51 is marked affected here).  Do you think we can get this in before we release next week?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yes, I know it's affected; that's why I asked for beta approval 6 days ago.  As soon as someone gets around to approving this for beta, I'll land it...

Comment 31

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #30)
> Yes, I know it's affected; that's why I asked for beta approval 6 days ago. 
> As soon as someone gets around to approving this for beta, I'll land it...

Sorry!  I missed the beta? flag somehow...

Comment 32

[Gerry Chang [:gchang]]

Comment on attachment 8816550 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Fix a web compatibility issue. Beta51+. Should be in 51 beta 7.

Comment 33

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ddcf602cb70753ecbd965a67ad50fcd41b55639e

Comment 34

[Andrei Vaida [:avaida]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #21)
> [Needs manual test from QE? If yes, steps to reproduce]: Verifying that this
>    fixes the SalesForce site would be good.  I don't have steps to reproduce,
>    but maybe Jessilyn does.

Jessilyn, is there anything manual QA can do here to help?

Comment 35

[Dennis Schubert [:denschub][OOO, back Jan 12]]

As Jessilyn wrote in comment 22, you'd need to have access to a Salesforce system in order to see the original issue. There is, however, a testcase in comment 7. If you run it before the fix, you'll see an exception. After the fix, it'll show "[object Object]".

I am not aware of any other "real world issues" with this so far, so I don't think there's an easy way to verify the fix besides the tests mentioned above.