1222906 - Images (and possibly other elements) with ids override other properties/expandos (maybe only createTreeWalker?) on document object created with createHTMLDocument

Status: RESOLVED INVALID

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

Description

[David Ross]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

I have some code that does essentially the following:

srcDoc = document.implementation.createHTMLDocument();
var iSpan = srcDoc.createElement("span");
iSpan.innerHTML = "<img id=createTreeWalker sanitizer-throws-in=firefox />";
srcDoc.body.appendChild(iSpan);
var tw = srcDoc.createTreeWalker(srcDoc.body, NodeFilter.SHOW_ALL, /* nodeFilter */ null, false);

In Chrome and IE this successfully calls createTreeWalker on the document created by createHTMLDocument().  On Firefox 42 it results in:
TypeError: srcDoc.createTreeWalker is not a function

This is because the element with ID "createTreeWalker" clobbers the createTreeWalker method at the root of the document.

I'm observing this behavior in the "jSanity" client-side HTML filter:
https://github.com/microsoft/jsanity
To reproduce:
1.  Go here: http://jsanity.azurewebsites.net/jsanity-demo-pretty.htm
2.  Replace the contents of the first textarea with:
<img id=createTreeWalker sanitizer-throws-in=firefox />
3.  Click "Sanitize!"
4.  Observe the script error.
Related jSanity bug report: https://github.com/Microsoft/JSanity/issues/5
I plan to make changes to jSanity to address this as necessary, but this also seems like a good opportunity to fix what appears to be a browser bug.

I'd say that in this case:
1)  The FF DOM is more clobber-able than other browsers.
2)  It enables the jSanity sanitizer to be bypassed.  Of course it's up to jSanity to uphold its own security guarantees, but...
3)  The behavior observed in FF violates the "principle of least surprise" that suggests if you try to access a method on a freshly created & populated HTML document, you'll get the legit DOM method.

If it's not possible to address this in FF with a code change, I'd like to ask for the official guidance on how to correctly / properly / safely obtain a legit DOM method.  Krzysztof Kotowicz suggested:
Object.getOwnPropertyDescriptor(Expected.prototype, 'property-name').get.apply(potentiallyClobbered)
...but this seems like a hack that shouldn't be necessary.

Another option (if the current behavior is set in stone) would be to provide a "noclobber" mode to achieve something more consistent with the behavior observed in other browsers.


Actual results:

Per above, the createTreeWalker method is clobbered.


Expected results:

createTreeWalker not clobbered.

Comment 1

[Andrew McCreight [:mccr8]]

Boris, what is the right behavior here?

createTreeWalker isn't specified as readonly, but maybe readonly is only for attributes.

Comment 2

[Daniel Veditz [:dveditz]]

> On Firefox 42 it results in: TypeError: srcDoc.createTreeWalker is not a function

Does this imply that it's a change in Firefox 42 from an earlier version? Or just clarifying which version you tested?

Comment 3

[Peter Van der Beken [:peterv]]

Document is marked OverrideBuiltins (per https://html.spec.whatwg.org/#document), so this is the behaviour as speced (see http://heycam.github.io/webidl/#OverrideBuiltins).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yes, Document is marked as [OverrideBuiltins], and I _think_ that's required for web compat (though I'm not sure exactly what IE does here; worth checking, as well as looking into the history of why it got marked [OverrideBuiltins]).  The set of named properties is specified as:

 * applet, exposed embed, form, iframe, img, or exposed object elements that have a name
   content attribute whose value is name, or
 * applet or exposed object elements that have an id content attribute whose value is name,
   or
 * img elements that have an id content attribute whose value is name, and that have a
   non-empty name content attribute present also.

See https://html.spec.whatwg.org/multipage/dom.html#dom-document-nameditem-filter

Note that <img id="createTreeTreeWalker> doesn't match the criteria there.  But this testcase:

  <img id="createTreeWalker" name="x"><script>alert(document.createTreeWalker)</script>

shows "[Object HTMLImageElement]" in Chrome, as expected per the above spec text.

So the only Firefox issue here per spec is that we're not enforcing the "and also has a non-empty name content attribute" condition on <img id="whatever">.  That's easy enough to fix; just need to adjust nsGenericHTMLElement::ShouldExposeIdAsHTMLDocumentProperty to do that.  That will address issue (1) from comment 0 by making our behavior match Chrome (and the spec), but of course do absolutely nothing for issues (2) and (3).  I filed bug 1223523 for that part.

>...but this seems like a hack that shouldn't be necessary.

It's necessary if you deal with [OverrideBuiltins] named getters.

Now you don't have to do the whole getOwnPropertyDescriptor bit for methods.  Just:

  Document.prototype.createTreeWalker.call( stuff )

will do the right thing.  For an attribute, you'd need to do the more complicated thing, though.

Where all that leaves this bug report, I have no idea.  ;)

Comment 5

[David Ross]

Thank you!  That should give me what I need for the sanitizer.  It also demonstrates that there's nothing much FF-specific here.  

I still think there's a need for a sort of "noclobber" flag somewhere that can be enabled to provide a more intuitive behavior for a given DOM, given the security implications of clobbering.  But it sounds like that's something to investigate on the standardization side of things.

Comment 6

[Daniel Veditz [:dveditz]]

Peter: can we unhide this bug (which appears to be a spec compliance issue) or is there really a security risk here?

Comment 7

[Peter Van der Beken [:peterv]]

Yeah, I don't think there's a new security risk here, beyond the fact that script needs to deal with [OverrideBuiltins] named getters. But that's orthogonal to the bug (and not something we can change anyway I think).

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm going to mark this invalid, but please reopen if the spec gets changed.