Closed Bug 1397512 Opened 7 years ago Closed 7 years ago

Azure Active Directory using Single Sign On causes the back button on the browser to fail due to creating a history entry despite the presence of a no-store header

Categories

(Core :: DOM: Navigation, defect, P2)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: makers, Assigned: freesamael)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Bug 1397512 - Part 1: Use LOAD_NORMAL_REPLACE in OnLinkClickSync if it's running inside an onload handler.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1397512 - Part 2: Add wpt test of form submission inside an onload handler.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36 Edge/15.15063

Steps to reproduce:

1) Go to Office.com or Outlook Web access (or any other site that requires AAD login), then sign in with 2FA
2) Go to a random external site
3) Go to http://microsoft.sharepoint.com/sites/msw/ (or another site that requires AAD login)
4) click back



Actual results:

Result: Can’t navigate back, the back button does not work at all


Expected results:

Back button should work.

We’ve determined this to be a Firefox bug and this behavior has existed for a long time now. Other browsers, upon seeing the no-store header in the response, do not create a history entry for the intermediate page. Firefox does create the history entry, and hence clicking back goes to a non-existent page and the back button fails to work.
Summary: Azure Active Directory using Single Sign On causes the back button on the browser to fail due to creating a history entry in a no-store header → Azure Active Directory using Single Sign On causes the back button on the browser to fail due to creating a history entry despite the presence of a no-store header
Component: Untriaged → Document Navigation
Product: Firefox → Core
Flags: needinfo?(sawang)
I can reproduce on Nightly with office.com -> mozilla.com -> outlook.com sequence.
Assignee: nobody → sawang
Flags: needinfo?(sawang)
Humm... I'm a bit confused. With these steps:

1. Goto office.com & sign-in my personal microsoft account.
2. Goto mozilla.com.
3. Goto outlook.com, noticing it's already signed-in.
4. Press back.
=> I got Firefox 57, Chrome 63, Edge 40 stuck here. Only Safari 36 went back to mozilla.com.

5. Press forward.
6. Press back.
=> Now I got Safari stuck here, too.

It doesn't really seem to work with major browsers. Am I missing something?

(In reply to Matt from comment #0)
> Other browsers, upon seeing the no-store header in the response, 
> do not create a history entry for the intermediate page.

I don't quite understand this part, either. If a redirection is triggered by http 301/302 
or within the onload handler, the "intermediate" URL in this case wouldn't be added to 
session history; If you make a redirection after onload handler then the current URL has 
already been added to history before location change.

"no-store" tells browsers not to cache the page at all, which means on history navigation, 
browsers would re-fetch the HTML resource from the server, but I don't see how it's relevant 
to history entry.

Do you have some minimal test cases to explain the problem you encountered?
Flags: needinfo?(makers)
Priority: -- → P2
"no-store" is a red herring. The real issue is the history entry. Here's a set of repro steps for the history entry issue:

1. Goto outlook.com & sign-in with your personal microsoft account.
2. Goto mozilla.com
3. Goto account.microsoft.com
4. Press back twice (the account.microsoft.com page adds an additional history entry which is not an actual navigation)
=> You momentarily get redirected to login.live.com and immediately arrive back at account.microsoft.com

I can repro this with Firefox 57. This does not repro with Edge 40 or Chrome 62. Both Edge and Chrome did not create a history entry for the "Continue" page that was served out of login.live.com. Also, if you examine the response of this "Continue" page, it consists of a 200 with an auto-post form that gets submitted as part of the body's onload event handler.
(In reply to gambedotti from comment #3)
> "no-store" is a red herring. The real issue is the history entry. Here's a
> set of repro steps for the history entry issue:
> 
> 1. Goto outlook.com & sign-in with your personal microsoft account.
> 2. Goto mozilla.com
> 3. Goto account.microsoft.com
> 4. Press back twice (the account.microsoft.com page adds an additional
> history entry which is not an actual navigation)
> => You momentarily get redirected to login.live.com and immediately arrive
> back at account.microsoft.com
> 

Thanks for the info. I'll take a look at this part.

> I can repro this with Firefox 57. This does not repro with Edge 40 or Chrome
> 62. Both Edge and Chrome did not create a history entry for the "Continue"
> page that was served out of login.live.com. Also, if you examine the
> response of this "Continue" page, it consists of a 200 with an auto-post
> form that gets submitted as part of the body's onload event handler.

After got back to outlook.com with step 4 on Chrome or Edge, pressing back again makes no effect. That was what confuses me.
Flags: needinfo?(makers)
The DoSubmit in web content calls HTMLFormElement::Submit which seems to using nsDocShell::OnLinkClickSync directly
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/dom/html/HTMLFormElement.cpp#804-810

The loadType in OnLinkClickSync is always LOAD_LINK, since it was for link click...
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/docshell/base/nsDocShell.cpp#14477

Despite it was in onload handler, the incorrect loadType causes history entries being added.

I could add a check of mIsExecutingOnLoadHandler in OnLinkClickSync, but it feels confusing that "link click can happen in onload handler". Let me think about this...
(In reply to Samael Wang [:freesamael] from comment #5) 
> it feels confusing that "link click can happen in onload handler"

Hmm...probably it's not so strange since one can also call button.click() in an onload handler.
I think this is worth being a WPT test, but haven't written WPT test before so not sure if this is the right way to add a test.

It seems the behavior of how history should behave when navigation happens in an onload handler is not well defined in the spec. :annevk hinted that I can use .tentative file names until it's defined in spec. 

There's at least a related test for iframe.src assignment in an onloader handler:
https://github.com/w3c/web-platform-tests/blob/master/html/browsers/history/the-location-interface/assign_before_load.html
Comment on attachment 8922267 [details]
Bug 1397512 - Part 2: Add wpt test of form submission inside an onload handler.

https://reviewboard.mozilla.org/r/193316/#review198544

This is largely rs+. Testing itself looks reasonable, but WPT's API is so hard to follow.

::: testing/web-platform/tests/html/browsers/history/the-session-history-of-browsing-contexts/navigation_in_onload_form-submission-dynamic-iframe.tentative.html:14
(Diff revision 1)
> +      function test() {
> +        let testFrame = document.createElement("iframe");
> +        testFrame.src = "navigation_in_onload_form-submission-1.tentative.html";
> +        document.body.appendChild(testFrame);
> +        fetch_tests_from_window(testFrame.contentWindow);
> +        test_wrapper.done();

Why you need async_test in this file?
Attachment #8922267 - Flags: review?(bugs) → review+
Comment on attachment 8922266 [details]
Bug 1397512 - Part 1: Use LOAD_NORMAL_REPLACE in OnLinkClickSync if it's running inside an onload handler.

https://reviewboard.mozilla.org/r/193314/#review198546

As always with session history changes, scary, but looks reasonable given the normal load event handling.
Attachment #8922266 - Flags: review?(bugs) → review+
I think it's actually easier to read without using fetch_tests_from_window, besides the original test case times out on Edge, so I removed fetch_tests_from_window from the test case.
The filenames confused wpt-manifest-update. Renamed files so the support files would be marked as "support" in manifest by wpt-manifest-update.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/77126e9996d1
Part 1: Use LOAD_NORMAL_REPLACE in OnLinkClickSync if it's running inside an onload handler. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6c0ed784da16
Part 2: Add wpt test of form submission inside an onload handler. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/77126e9996d1
https://hg.mozilla.org/mozilla-central/rev/6c0ed784da16
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Thanks so much for addressing this issue!
Samael, is there a spec issue raised for this?  

https://html.spec.whatwg.org/multipage/history.html#location-object-setter-navigate describes the conditions under which navigation from location.* sets does replacement (and they are broader than just "is in onload").  But https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm has nothing of the sort (except in cases when the form is targeted to a new window).  

The only other place where we do the "load inside onload means replace" thing is for toplevel docshells, as part of general LoadURI() bits.  But the link click changes here are not restricted to root docshells, right?

What do other browsers actually do here in the non-root-docshell case?  In the root docshell case?
Flags: needinfo?(sawang)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)
> Samael, is there a spec issue raised for this?  

There wasn't. I just submitted https://github.com/whatwg/html/issues/3247

> The only other place where we do the "load inside onload means replace"
> thing is for toplevel docshells, as part of general LoadURI() bits.  But the
> link click changes here are not restricted to root docshells, right?
>
> What do other browsers actually do here in the non-root-docshell case?  In
> the root docshell case?

The tentative test case was verified on Chrome and Edge first before landing, so If I didn't get it wrong, Chrome and Edge don't generate new entries for "form submission inside onload handler" in both top-level documents and in iframes, and Firefox aligns with these 2 browsers now.

Safari doesn't pass the test. It generates new entries in both top-level documents and iframes.
Flags: needinfo?(sawang)
> I just submitted https://github.com/whatwg/html/issues/3247

Thank you!

> Safari doesn't pass the test. It generates new entries in both top-level documents and iframes.

Does it fail on the original site this bug was reported on too?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #21)
> > I just submitted https://github.com/whatwg/html/issues/3247
> 
> Thank you!
> 
> > Safari doesn't pass the test. It generates new entries in both top-level documents and iframes.
> 
> Does it fail on the original site this bug was reported on too?

Oh, wait, it doesn't!

Now I recall Safari has a very special behavior on history navigation - if a page `window.open` a new tab, Safari will report `history.length=2` and UI back button will be available. Pressing back immediately will close the newly opened tab and take user "back" to the original tab. 

This special "back to previous tab" feature is available until navigation happens in the newly created tab, presumably to prevent user from accidentally closing the new tab on history navigation, but the trick makes it fail in the test. It in fact doesn't add entries for form submission.

I should probably make the test case record original history length on window.open. Will file a follow-up bug.
Depends on: 1419692
Depends on: 1418119
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: