Make <embed> and <object> behave more like <iframe>
Categories
(Core :: DOM: Navigation, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox105 | --- | fixed |
People
(Reporter: annevk, Assigned: sefeng)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
(Whiteboard: [orb:m1])
Attachments
(2 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
Bug 1595491 - Part 2: Adapt test files to <embed> and <object> behaving more like <iframe>. r=smaug!
48 bytes,
text/x-phabricator-request
|
Details | Review |
Chrome and Safari appear to create a browsing context more often than Firefox for the embed
and object
elements. Firefox already resizes for image/svg+xml
, so that logic could be reused for image documents (though see bug 1594986).
Plugins might have to remain a special case, but Firefox could start honoring X-Frame-Options and CSP's frame-ancestors for all responses that go through embed
and object
elements. If honoring those headers seems agreeable, perhaps we should split that out as it's easier to do and it'll make it harder for folks to make mistakes with Fetch Metadata (bug 1508292).
See also https://github.com/whatwg/fetch/pull/948 for context.
Comment 1•4 years ago
|
||
I have no opinions on the header bits; I see no reasons to not honor them, honestly, apart from possible compat issues.
As far as creating a browsing context for the image case, we could do that, but we'd need to do a bit more work than we do for SVG. Specifically, we'd need to:
- Get rid of things like margins and padding around the image in the image document (at least when loaded via object/embed).
- Possibly make it possible to have browsing contexts that don't show up in the parent's
frames[n]
and don't show up by name. This depends on whether we'd want to make the existence of the browsing context visible or not. - Fix UI code that shows image context menu options on
<object data="some-image">
, assuming we show those, to work with the new setup.
Obviously doing this plus dropping plug-ins would let us significantly simplify nsObjectLoadingContent, to the point where it would almost go away as a concept.
Updated•4 years ago
|
Reporter | ||
Comment 2•3 years ago
|
||
As per bug 1686040 comment 5 our current setup for these elements is problematic as too much data can enter the attacker process. It seems that in Chrome and Safari these elements always end up instantiating documents (e.g., https://software.hixie.ch/utilities/js/live-dom-viewer/saved/9009) and we should consider doing the same. Getting there incrementally is acceptable though. E.g., allowing sniffed images to the attacker process and nothing else could be a reasonable middle ground, if that would be easier to do.
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Updated•3 years ago
|
Reporter | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 3•2 years ago
|
||
Latest try is here, and it's starting to come together (if I haven't done anything drastically wrong in the latest update): https://treeherder.mozilla.org/#/jobs?repo=try&revision=81e119a921f15b4e55f02cc31b4b67d29e3b51cd
One thing that's failing though is 315920-18d.html, and I talked briefly with Emilio about this, and he graciously offered to take a quick look!
I'll look at it as well though, but after that, it's review time!
Comment 4•2 years ago
|
||
Turns out there is another bug that is happening due to me not handling nsImageLoadingContent
correctly (this is probably the same, it also has got to do with nsImageLoadingContent
).
But that's it. And in the end I wouldn't be surprised if 315920-18d.html also is an nsImageLoadingContent
thing. It feels like it has got to do with -moz-broken
which is handled through ImageState
in nsImageLoadingContent
.
Comment 5•2 years ago
|
||
It turned out to not be exactly that, but it does have with -moz-broken
to do. The thing that's missing is a final call to nsObjectLoadingContent::NotifyStateChanged
when loading is complete, to trigger a restyle due to nsObjectLoadingContent
state changing. Easy enough to change.
Comment 6•2 years ago
|
||
By making image loading in <embed> and <object> behave more like when
an <iframe> loads an image, we can make sure that the synthetic
document generated is process switched if the image is cross
origin. This is done by making image loading in nsObjectLoadingContent
follow the document loading path.
We also make sure that we pass the image size back to the embedder
element to not get stuck with the intrinsic size.
To avoid named targeting being able to target these synthetic
documents, as well as showing up in Window.frames
and being counted
in Window.length
, we keep a filtered list of non-synthetic browsing
contexts for that use-case.
This feature is controlled by two prefs:
-
browser.opaqueResponseBlocking.syntheticBrowsingContext
This triggers the creation of synthetic documents for images loaded
in <object> or embed. -
browser.opaqueResponseBlocking.syntheticBrowsingContext.filter
This turns on the filtering of synthetic browsing contexts in named
targeting,Window.length
andWindow.frames
.
Comment 7•2 years ago
|
||
Depends on D148117
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 8•2 years ago
|
||
I haven't had a chance to try your patches yet, but I wanted to early-flag a possible a11y concern. I think this might result in images embedded with <embed>
or <object>
being exposed in the a11y tree as being inside a frame, where this isn't the case for <img>
and isn't currently the case for <embed>
or <object>
either. This does have some consequences for screen reader users: some additional verbosity; and VoiceOver treats frames as additional containers that users have to "interact with" (explicitly move inside) to see what's inside.
I'm not super familiar with how embed and object work. Why might an author use embed/object to embed an image instead of img? Given that this is about cross-origin process switches, is that currently an issue for img or is it not relevant there for some reason?
You'll be able to see whether there's a difference by looking in the Firefox Accessibility Inspector. I'll also try to play with this when I get a chance.
We can probably work around this in the a11y engine by exposing a generic role for OuterDocAccessible in this case which screen readers should ignore.
Having said all of this, Chrome doesn't seem to handle images embedded with embed/object very well with regard to the a11y tree at all, so this certainly isn't going to be a major web compat issue or anything like that.
Comment 9•2 years ago
|
||
I tried the patches and confirmed my suspicion here. It's a bit worse than I thought because setting the title attribute on the embed to provide alternative text doesn't expose the alternative text on the image inside the frame, which means that if you embed an image like this, there's no way to provide alternative text for it. I'll think on possible solutions.
Comment 10•2 years ago
|
||
Thanks for the comments James!
So the most common reason to embed images instead of using <img>
is(/was) for when you had an image format only available in plugins. Which is why there are still images in <object>
and <embed>
out there. The reason for why we want to load cross origin images loaded in <embed>
or <object>
is that we want to block an attack vector for spectre et al. By not having to consider <object>
and <embed>
we can simplify the ORB implementation significantly.
As for providing alternative text, it would be possible to set the alt
attribute on the image in the image document somehow I'm sure. But I wonder how this is done for <iframe>
today. If you do <iframe src="image.png" title="A title></iframe>
, what do you get then? I checked, you get the behavior you describe in comment 8. So this change won't at least make it worse than it already is for <iframe>
. And the upside is that if we fix it in ImageDocument.cpp
we'll fix it for all cases.
The plan is to land this on nightly only to begin with. Would it be ok with you to have a slightly broken nightly for a11y of embed/object? If so I'll submit a follow-up for fixing this before it rides to beta.
Comment 11•2 years ago
|
||
(In reply to Andreas Farre [:farre] from comment #10)
The reason for why we want to load cross origin images loaded in
<embed>
or<object>
is that we want to block an attack vector for spectre et al.
Why is this not an issue for <img>
? Is it because the data supplied and allowed in that case is limited, whereas embed/object (by virtue of supporting so many types) have a much greater potential for spectre, etc. attacks?
As for providing alternative text, it would be possible to set the
alt
attribute on the image in the image document somehow I'm sure.
I guess that would currently involve passing data over IPDL, but yeah, it seems possible. With the new a11y caching architecture we're developing, this would be easier; we could probably handle this entirely in the parent process.
But I wonder how this is done for
<iframe>
today. If you do<iframe src="image.png" title="A title></iframe>
, what do you get then? I checked, you get the behavior you describe in comment 8. So this change won't at least make it worse than it already is for<iframe>
.
Ah. Somehow, I didn't realise you could embed images with iframe. Yuck... but you're right, you get the same problem there.
The plan is to land this on nightly only to begin with. Would it be ok with you to have a slightly broken nightly for a11y of embed/object? If so I'll submit a follow-up for fixing this before it rides to beta.
That seems totally reasonable, especially as i doubt this will be a "real" problem in the wild. If an author is actually thinking about a11y, it's not unreasonable that they'd switch to using img instead of embed or object for semantic reasons if nothing else.
Comment 12•2 years ago
|
||
(In reply to James Teh [:Jamie] from comment #11)
Why is this not an issue for
<img>
? Is it because the data supplied and allowed in that case is limited, whereas embed/object (by virtue of supporting so many types) have a much greater potential for spectre, etc. attacks?
For <embed>
and <object>
it's mainly because they're already container elements, and can have browsing context, like they do whey documents are embedded. And it removes a whole bunch of special casing, and even the actual need to block any responses. For <img>
(and other media elements and <script>
) we need to look at the responses and block them if needed, we can't isolate them in any other way. If you're interested there's more info here: https://github.com/annevk/orb
That seems totally reasonable, especially as i doubt this will be a "real" problem in the wild. If an author is actually thinking about a11y, it's not unreasonable that they'd switch to using img instead of embed or object for semantic reasons if nothing else.
This is wonderful. I'll file a bug about it.
Updated•2 years ago
|
Updated•2 years ago
|
Comment 13•2 years ago
|
||
Pushed by sefeng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/91d87de145e3 Part 1: Make <embed> and <object> behave more like <iframe>. r=smaug,emilio https://hg.mozilla.org/integration/autoland/rev/3687b7153c44 Part 2: Adapt test files to <embed> and <object> behaving more like <iframe>. r=smaug
Comment 14•2 years ago
•
|
||
Backed out 2 changesets (bug 1595491) for causing build bustages in docshell/base/BrowsingContext.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/eecfa46043c20f261e0f6e825609fcd658d49ea3
/builds/worker/checkouts/gecko/docshell/base/BrowsingContext.cpp:742:26: error: unused variable 'context' [-Werror=unused-variable]
Assignee | ||
Updated•2 years ago
|
Comment 15•2 years ago
|
||
Also R2 failure Log <--> R2
Comment 16•2 years ago
|
||
Pushed by sefeng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c07b32cdfb47 Part 1: Make <embed> and <object> behave more like <iframe>. r=smaug,emilio https://hg.mozilla.org/integration/autoland/rev/3c894371eace Part 2: Adapt test files to <embed> and <object> behaving more like <iframe>. r=smaug
Comment 17•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c07b32cdfb47
https://hg.mozilla.org/mozilla-central/rev/3c894371eace
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Description
•