989584 - Can't print boarding pass at delta.com on Firefox 30+, due to the site using "var opener = { stuff }"

Status: RESOLVED FIXED

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

Description

[(not currently active) Ted Mielczarek]

Trying to print a boarding pass from delta.com doesn't work currently. I tried with Nightly and with some other version, although I've forgotten precisely which at this point. It worked in Chrome on Windows and in Safari on Mac.

I get to the page with buttons labeled "Send to Mobile", "Print" and "Done" (the URL is https://www.delta.com/PCCOciWeb/DisplayTripSummaryNext.action), but clicking on any button does nothing. The error console upon loading the page contains:
NS_ERROR_UNEXPECTED:  ocibundle.js:35
TypeError: s is undefined obpGetBoardingPassScript.js:36
TypeError: s is undefined DisplayTripSummaryNext.action:634


https://content.delta.com/content/dam/delta-applications/pcc/oci_140201/1/shared/jawr/js/ocibundle.js?ver=14020133
https://content.delta.com/content/dam/delta-applications/pcc/oci_140201/1/bundles/boardingpass/js/obpGetBoardingPassScript.js?ver=14020133

Comment 1

[Adam Stevenson [:adamopenweb]]

This is going to be difficult to test. Moving to desktop component for now.

Comment 2

[Kohei Yoshino [:kohei]]

Attachment: testcase

A minimum testcase is here. Defining |opener| as a global variable doesn't work due to Bug 932322.

Comment 3

[Hallvord R. M. Steen [:hallvors]]

(For the context: "testing" here is about monitoring the site to see if it still has the problem/fixes it/regresses the fix later on)
Well, some bugs are indeed untestable. This one, we're lucky - consider:

var ver;for(let el,i=0;el=document.scripts[i];i++){
if(el.src.indexOf('?ver=')>-1)ver = el.src.substring(el.src.indexOf('?ver=')+5, el.src.length);
}

location = 'https://content.delta.com/content/dam/delta-applications/pcc/oci_'+ver.substr(0,6)+'/1/shared/jawr/js/ocibundle.js?ver='+ver;

and then

document.body.textContent.indexOf('\nvar opener={open:') === -1

Hacky fun :-]

Naturally we have no guarantees of the lifetime of that test, but I'll still ship it.. https://github.com/hallvors/sitecomptester-extension/commit/16bc03523f6e58e6f99b0d2b698d843e9a4088c3

Comment 4

[Kohei Yoshino [:kohei]]

And the URL provided also works without sign in.

Comment 5

[Boris Zbarsky [:bzbarsky]]

How does this work in other UAs?  Seems like we might need to get the spec changed here if "var opener = junk" works across the board...

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is complicated by the fact that "opener" is one of the properties on window that you can read cross-origin, as far as I can tell.  But in chrome it can only be read _sometimes_?  That's pretty insane.  Bobby, thoughts?

Comment 7

[Kohei Yoshino [:kohei]]

var opener = '';
var opener = 1;
var opener = true;
// TypeError: Value being assigned to Window.opener is not an object.

var opener = {};
var opener = [];
var opener = new Map();
// NS_ERROR_UNEXPECTED

var opener = null;
// No error

Comment 8

[Boris Zbarsky [:bzbarsky]]

Kohei, in what browser?  Comment 7 is the current nightly Firefox behavior, which is what's breaking this site, no?

Comment 9

[Kohei Yoshino [:kohei]]

Beta (30), Aurora (31), Nightly (32) return the same result as my comment 7, while Release (29) and Chrome (34) output nothing and window.opener is overrode. The problematic code on delta.com is:

> var opener = {
>   open: function (url, windowname, width, height, resizable, scrollbars) {
>     try {
>       // snip
>       return window.open(go, windowname, params);
>     } catch (e) {
>       alert(e.description);
>     }
>   }
> };
> 
> var newwin = opener.open(url, windowname, width, height);

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Beta (30), Aurora (31), Nightly (32) return the same result as my comment 7,

Yes, I know.

> while Release (29) and Chrome (34) output nothing and window.opener is overrode.

My question is what this code:

  var opener = {};

as well as this code:

  opener = {};

does in all browsers for both {} and at least the following values: null, window, "", 5, undefined.  That means examining how it affects the value the page that did the set sees, how it affects the value same-origin pages see, and how it affects the value that cross-origin pages see...

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> This is complicated by the fact that "opener" is one of the properties on
> window that you can read cross-origin, as far as I can tell.

Yes, per spec and per implementations last I checked (see https://bug839867.bugzilla.mozilla.org/attachment.cgi?id=712247 ).

> But in chrome it can only be read _sometimes_?

Are you referring to Blink? If so, I have no idea. We should ask them.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Are you referring to Blink?

Yes.  

> If so, I have no idea. We should ask them.

A simple testcase shows that in Blink that it can be read cross-origin until the page sets it, and then it can no longer be read or something.

Comment 13

[Hallvord R. M. Steen [:hallvors]]

I used to have some tests for this, but it seems Opera hasn't released them.. :-/

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Boris Zbarsky [:bz] from comment #12)
> > Are you referring to Blink?
> 
> Yes.  
> 
> > If so, I have no idea. We should ask them.
> 
> A simple testcase shows that in Blink that it can be read cross-origin until
> the page sets it, and then it can no longer be read or something.

Does that still mean this is an evangelism bug?  Tracking for now, but we're late in FF30 beta and might not be able to get anything done in time unless this is an issue on our side.

Comment 15

[Hallvord R. M. Steen [:hallvors]]

(In reply to Lukas Blakk [:lsblakk] from comment #14)

> Does that still mean this is an evangelism bug?

IMO no, it isn't. Of course it can be fixed by the page - but we're asking for trouble if we keep this incompatibility in the engine. We're likely to break more sites, so we should align with Blink.

Comment 16

[Boris Zbarsky [:bzbarsky]]

For Firefox 30, we will deal with this in bug 976920.

For 31 and later we need to figure out what the plan is.  "Align with Blink" is certainly not possible unless we know what Blink's behavior is, and depending on that behavior might not be technologically possible at all in our architecture.  So the first step is for someone to write some testcases for the stuff from comment 10 so we can figure out what Blink actually does here.  Oh, and those should also test what happens for both the object case and null and an actual window object across navigations (because iirc some but not all changes to window.opener persist across navigations in Blink)...  Hallvord, do you have time to write up some tests?

Comment 17

[Hallvord R. M. Steen [:hallvors]]

Tests coming up here: https://github.com/w3c/web-platform-tests/pull/999

Boris, does that look OK? I note that Presto-Opera, Blink-Opera and Chrome pass this test. Firefox fails because it throws more exceptions that the rest (they don't throw) and IE11 fails because .. well, I haven't been able to figure out why, something about postMessage() not behaving the way I expect..?

Comment 18

[Hallvord R. M. Steen [:hallvors]]

(Forgot to test for the "across navigation" part

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yeah, need to figure out what IE and Safari are doing, as well as what happens across navigations....

Comment 20

[Adam Stevenson [:adamopenweb]]

Hallvord as requested from Safari with pop-ups allowed.

Result: Fail
Test Name: overwriting window.opener
Message: assert_equals: opener set to object with var - reading throws? expected true but got false(stack: assert@http://w3c-test.org/resources/testharness.js:1738:42 assert_equals@http://w3c-test.org/resources/testharness.js:335:15 http://w3c-test.org/submissions/999/html/browsers/the-window-object/security-window/overwriting-window-opener.html:108:22 step@http://w3c-test.org/resources/testharness.js:798:30 http://w3c-test.org/resources/testharness.js:827:40)

Comment 21

[Hallvord R. M. Steen [:hallvors]]

Huh? That's an interesting result - so it allows setting opener from within the popup (as in "does not throw and appears to suceed") yet allows reading it from another origin later on? .-o Maybe it does some magic "opener is not the same from inside the window as from outside the window" thingy??

Comment 22

[Boris Zbarsky [:bzbarsky]]

Can we change the test to include some information about the read value in the case when we expect reading to fail?  In general, what I'm looking for here are exploratory "what is the exact behavior?" tests, not conformance tests, since we have no idea what the "right" behavior is so far...

Comment 23

[Hallvord R. M. Steen [:hallvors]]

https://github.com/w3c/web-platform-tests/pull/999 now also has tests for persistence across navigation (one same-origin navigation and one same-to-other-origin navigation).

(In reply to Boris Zbarsky [:bz] from comment #22)
> Can we change the test to include some information about the read value in
> the case when we expect reading to fail?  

I had more "what's going on here?" code, but I removed it again and wrote a conformance test since it seems the behaviour is pretty consistent across the browsers that do behave consistently. 

If Safari really doesn't throw on reading a redefined window.opener it seems like a bug. (Even though you from a developer point of view may well find it weird that a property is sometimes cross-origin readable and sometimes not we presumably have the same issue for every exposed property - for example if a site does 

function postMessage(){ /*code that does important stuff*/ }
 
we presumably have to make sure other sites can't do unexpected cross-origin window.postMessage() calls.)

Of course I can work with Adam to figure out what Safari is really up to if you think it's worth exploring :). I've added some more info to the message if that test fails (but that code doesn't seem to be on w3c-test.org yet).

PS: great if somebody could do the review https://critic.hoppipolla.co.uk/r/1607

Comment 24

[Boris Zbarsky [:bzbarsky]]

> since it seems the behaviour is pretty consistent across the browsers that do behave
> consistently. 

Which browsers did you actually test?  What is the behavior in those browsers?  How are you defining "consistently"?

I'm basically looking for a behavior matrix here, not a "pass/fail" indicator or value judgements on what behavior is consistent...

> If Safari really doesn't throw on reading a redefined window.opener it seems like a bug. 

I don't see why, depending on what gets read.

> we presumably have to make sure other sites can't do unexpected cross-origin
> window.postMessage() calls

Per spec and in Gecko other sites see the canonical postMessage function instead of the redefined one.  They don't throw when you try to postMessage to that window!

> Of course I can work with Adam to figure out what Safari is really up to if you think
> it's worth exploring

I would like to understand the exact observed behavior of Safari, Chrome, and recent IE (and Gecko, but I already know the Gecko behavior) in all combinations of the following:

1)  Possible initial states: no opener (e.g. subframe), set by UA (window.open)
2)  Set states: Not set by page, set by page to null, some window, undefined, {}, 5
3)  Access sources: same-origin, cross-origin
4)  Access times, for cases when a page sets it: before the page that did the set unloads,
      after it unloads.

If you're willing to write a test that basically spits out those 44 pieces of information in whatever browser runs it, I'd really appreciate it.  If you're not, please just let me know and I'll do it myself.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Also, for the spec history here, see https://www.w3.org/Bugs/Public/show_bug.cgi?id=11839 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=21946

It's possible that we just need to change from "Window" to "any" here and otherwise leave the spec as-is (complete with its existing text about how cross-origin property access on Window works).  That doesn't match Chrome, certainly, but it's probably the simplest and sanest behavior.

The observable behavior then would be as follows:

1)  A window has an internal opener value, set based on whether and by whom it was opened.
2)  Doing "window.opener = null" writes null to this internal opener value.
3)  Doing "window.opener = foo" where foo !== null redefines the "opener" property on the
    window object to be a value property with the value foo.
4)  Cross-origin access always gets the current internal opener value.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Untracking and marking unaffected on 30 to reflect the backout in bug 976920.

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK.  I think we need to change the spec (filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=25917 ) and change our code accordingly.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow sites to set window.opener to any value

Comment 29

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #16)
> "Align with Blink"
> is certainly not possible unless we know what Blink's behavior is

Here's the custom setter: https://chromium.googlesource.com/chromium/blink.git/+/master/Source/bindings/v8/custom/V8WindowCustom.cpp#196 It special-cases null "to behave like Firefox", but I'm not sure what it does exactly. It always seems to delete the existing property and sets a value property instead.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Sure.  That's what we do with our patch too.  The bigger question was what Blink does around same-origin access and the like, and the answer is it does weird things that we don't want to do and that don't follow the spec.  ;)

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/026bd94e5bd0

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8431082 [details] [diff] [review]
Allow sites to set window.opener to any value

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 932322 
User impact if declined: Need to either check in bug 976920 on Aurora 31, or
   Delta's website is broken.
Testing completed (on m-c, etc.): Passes tests.
Risk to taking this patch (and alternatives if risky): I think this is pretty low
   risk, personally.
String or IDL/UUID changes made by this patch: Changes the IID of nsIDOMWindow.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/065972d5ac21