Closed Bug 1599256 Opened 2 years ago Closed 2 years ago

Cannot log into thestar.com

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 + fixed
firefox73 --- fixed

People

(Reporter: wlach, Assigned: ckerschb)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [domsecurity-active])

Attachments

(2 files)

Attached image image.png

How to reproduce

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

Flags: needinfo?(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.

Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Flags: needinfo?(ckerschb)
Priority: -- → P1
Whiteboard: [domsecurity-active]
See Also: → 1593832

(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.

(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

Flags: needinfo?(wlachance)

(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.

(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.

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

Flags: needinfo?(wlachance)

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?

Flags: needinfo?(jkt)
Flags: needinfo?(dveditz)
Flags: needinfo?(bzbarsky)

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.

Flags: needinfo?(jkt)

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

Flags: needinfo?(bzbarsky)
See Also: → 1418975
See Also: → 1552504

Er, I meant to needinfo Anne.

Flags: needinfo?(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).

Flags: needinfo?(annevk)

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.

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

Flags: needinfo?(dveditz) → needinfo?(bugs)

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.

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?

Flags: needinfo?(bugs)

(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.

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

Blocks: 1601887

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.

Pushed by opoprus@mozilla.com:
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

(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!

Flags: needinfo?(ckerschb)
Pushed by nbeleuzu@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

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
Attachment #9114097 - Flags: approval-mozilla-beta?

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

Attachment #9114097 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.