1599256 - Cannot log into thestar.com

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[William Lachance (:wlach)]

Attachment: image.png

How to reproduce

• Go to: https://www.thestar.com/sign-in
• Click on "create account"
• Enter a set of valid credentials (doesn't matter what)
• Click on "Create Account"

I also filed a webcompat issue for this, before realizing this might just be a bug: https://webcompat.com/issues/45399

Running mozregression, it appears this commit is the culprit: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cd45360fb1ff5110acd8d54329ab45fce7395ba0&tochange=d5bee3db1b4f0ce5ff876f08230336022ae7d8ab

... which points to bug 1584998

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Thanks for reporting - most likely this will be fixed by Bug 1593832. Assigning to myself in the meantime to make sure this gets fixed.

Comment 2

[William Lachance (:wlach)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)

Thanks for reporting - most likely this will be fixed by Bug 1593832. Assigning to myself in the meantime to make sure this gets fixed.

Unfortunately this does not seem to be fixed in November 27th's nightly, I can still reproduce the problem.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #2)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)

Thanks for reporting - most likely this will be fixed by Bug 1593832. Assigning to myself in the meantime to make sure this gets fixed.

Unfortunately this does not seem to be fixed in November 27th's nightly, I can still reproduce the problem.

The patches from Bug 1593832 just hit autoland (https://bugzilla.mozilla.org/show_bug.cgi?id=1593832#c5). Once merged into central, can you please help me verify it works again correctly? Thanks

Comment 4

[William Lachance (:wlach)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)

The patches from Bug 1593832 just hit autoland (https://bugzilla.mozilla.org/show_bug.cgi?id=1593832#c5). Once merged into central, can you please help me verify it works again correctly? Thanks

Ah sure thing. Sorry, I assumed (incorrectly) that it had already landed. I'll check again later this week, can leave needinfo open for now.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #4)

Ah sure thing. Sorry, I assumed (incorrectly) that it had already landed. I'll check again later this week, can leave needinfo open for now.

Yeah, sure. Please note that we can not close that bug even after verification that it works correctly again, because there remains a problem for Fission, hence making it block the fission master bug for dom:security.

Comment 6

[William Lachance (:wlach)]

Still not working in today's nightly I'm afraid.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

So, it took me a while to figure out what the problem is here. FWIW, Chrome also blocks the iframe and logs the following message to the console:

Refused to display 'https://torstar.us.janraincapture.com/widget/traditional_register.jsonp' in a frame because it set 'X-Frame-Options' to 'sameorigin'.

In other words, Firefox as well as Chrome is blocking the iframe. The difference is that Chrome is firing the .onload() event even when a frame is blocked by XFO. Before Bug 1584998 we loaded about:blank when XFO blocked the frame and fired the .onload() event. Within Bug 1584998 we added that new error page:
https://bug1584998.bmoattachments.org/attachment.cgi?id=9104192
instead of loading about:blank (see https://phabricator.services.mozilla.com/D50588#change-50MVHzgy2Nf3) which causes us not fire the .onload() event anymore.

FWIW, we have Bug 1552504 on file which discusses whether we should fire any event when a frame is blocked by XFO. So either this bug becomes a webcompat issue, or we go back to fire the .onload() event.

The XFO spec doesn't mention anything regarding firing an event in the block case. I don't think we should fire the .onerror() event because I think we had that discussion that the .onerror() could be observed and hence leaks information.

Boris, Dan, Jonathan, what's your take? Should we fire the .onload() event in case the framing is blocked by XFO?

Comment 8

[Jonathan Kingston [:jkt] he/him]

Without checking the standard, I'm interested how the absence of onerror doesn't leak information given that the parent frame can observe the lack of events fired and infer that it was blocked due to framing issues. Is the issue related to the information exposed inside the onError error instance; like the redirected URL?

Either way not firing an event seems a little weird, we should probably behave the same as Chrome or agree on the change with them and standardise it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Also related is bug 1418975.

In general, the spec is not very clear about what should happen for the load event in this situation, last I looked. Anne, am I just missing something there?

Also note https://github.com/whatwg/html/issues/125

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er, I meant to needinfo Anne.

Comment 11

[Anne (:annevk)]

The X-Frame-Options not being defined issue is https://github.com/whatwg/html/issues/1230.

Given Mike's recent comment there treating this as a network error seems good. It appears that Chrome might still dispatch a load event for such errors though. Aligning with that seems reasonable. I don't think that creates a new communication channel, but it will nix the idea for an error event (issue 125 mentioned in comment 9).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, I am pretty sure Chrome fires a load event on the iframe in the "network error" case, though we should check that carefully. I guess we could align with that, if that's what they do.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Smaug, in case x-frame-options blocks an iframe we used to display about:blank which fired the onload() event for the iframe. Within Bug 1584998 we added a new error 'NS_ERROR_XFO_VIOLATION' (see [1]) and also a new error page which now does not fire the onload() event anymore.

Do you have some pointers for me where I would have to change things within docshell.cpp and/or document.cpp so the new error page could fire the onload() event again?

[1] https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3785

Comment 14

[Anne (:annevk)]

As I mentioned in bug 1552504, if we're going to dispatch load events we really ought to do so for all network errors and completely align with Chrome.

Comment 15

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Yeah, that. If we want to start to dispatch load events for error loads, let's do that consistently.

For 72, could we keep loading about:blank?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #15)

Yeah, that. If we want to start to dispatch load events for error loads, let's do that consistently.

For 72, could we keep loading about:blank?

Yeah, agreed - I'll get that ready for us.

Comment 17

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on affected Nightly73 and Beta72, updating flags.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1599256: Fix web compatibility issues by reverting changes and going back to loading about:blank and firing the onload event in case XFO blocks an iframe. r=smaug,jkt

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

FWIW, reverting changes and have the iframe onload event fire when a frame is blocked by XFO fixes the problem described in Comment 0. I just set the checkin-needed flag and we should also uplift that changeset and ship it within FF72.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8f22dc26ee7
Fix web compatibility issues by reverting changes and going back to loading about:blank and firing the onload event in case XFO blocks an iframe. r=jkt,smaug

Comment 21

[Andrei Ciure[:aciure]]

Backed out changeset b8f22dc26ee7 (bug 1599256) for causing test_ignore_xfo.html to permafail

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=b8f22dc26ee7bd48691c647ced93ab530e87035d

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=c68fb518e696f153903156f8d65913e2f77ac2fa&selectedJob=280874904&searchStr=Linux%2Cx64%2Cdebug%2CMochitests%2Ctest-linux64%2Fdebug-mochitest-e10s-7%2CM%287%29

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=c68fb518e696f153903156f8d65913e2f77ac2fa&selectedJob=280874121&searchStr=Linux%2Cx64%2Casan%2CMochitests%2Ctest-linux64-asan%2Fopt-mochitest-e10s-5%2CM%285%29

backout: https://hg.mozilla.org/integration/autoland/rev/68b0102b619ea3fa5eb5373a152f3a924e85576f

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrei Ciure[:andrei_ciure] from comment #21)

Backed out changeset b8f22dc26ee7 (bug 1599256) for causing test_ignore_xfo.html to permafail

I'll take a look - thanks!

Comment 23

[Andrei Ciure[:aciure]]

Also this failure:
https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=c68fb518e696f153903156f8d65913e2f77ac2fa&searchStr=Android%2C7.0%2Cx86-64%2Copt%2CMochitests%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-mochitest-e10s-2%2CM%282%29&selectedJob=280877866

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Slightly modified the test, should work now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4a3e80d6e4085592ef2901afcf8dcfe97ca810a

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b4749d24a1
Fix web compatibility issues by reverting changes and going back to loading about:blank and firing the onload event in case XFO blocks an iframe. r=jkt,smaug

Comment 26

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f1b4749d24a1

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9114097 [details]
Bug 1599256: Fix web compatibility issues by reverting changes and going back to loading about:blank and firing the onload event in case XFO blocks an iframe. r=smaug,jkt

Beta/Release Uplift Approval Request

• User impact if declined: Web-Compatibility issue: If an iframe is blocked by x-frame-options then the 'onload' event on the iframe will not be fired if we do not uplift that patch. Apparently some web-pages rely on that even to be fired, hence we should uplift the change. FWIW, also creates parity with Chrome.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's not risky, because we basically just revert code behavior which we changed within Bug 1584998.
• String changes made/needed: no

Comment 28

[Julien Cristau [:jcristau]]

Comment on attachment 9114097 [details]
Bug 1599256: Fix web compatibility issues by reverting changes and going back to loading about:blank and firing the onload event in case XFO blocks an iframe. r=smaug,jkt

webcompat regression fix, approved for 72.0b8

Comment 29

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ae47c01826b