The following works in other engines, but doesn't work in WebKit: var ifr = document.createElement("iframe"); ifr.setAttribute("src", "javascript:'PASS'"); document.body.appendChild(ifr); This regressed in <http://trac.webkit.org/changeset/24994>.
Created attachment 38501 [details] proposed fix
Comment on attachment 38501 [details] proposed fix There are a few more case this probably needs to patch. See JSHTMLIFrameElementCustom.cpp, JSHTMLFrameElementCustom.cpp and JSAttrCustom.cpp. There may be more I am missing, so it is probably best to go back to the initial checkin and see what we did.
Why isn't checkNodeSecurity passing? If it's a blank document, it should inherit its SecurityOrigin from its parent... Maybe the issue is that it isn't attached yet? We should test document.writing into the detached blank frame as well.
Created attachment 38505 [details] fix more copy/paste checks
Created attachment 38507 [details] remove unrelated changes
Comment on attachment 38507 [details] remove unrelated changes > - if (!checkNodeSecurity(exec, static_cast<HTMLFrameElementBase*>(ownerElement)->contentDocument())) > + HTMLFrameElementBase* frameElement = static_cast<HTMLFrameElementBase*>(ownerElement); > + if (frameElement->contentDocument() && !checkNodeSecurity(exec, frameElement->contentDocument())) > return; I think it would be better to put the contentDocument into a local variable instead of the frame. > HTMLFrameElementBase* frame = static_cast<HTMLFrameElementBase*>(element); > - if (!checkNodeSecurity(exec, frame->contentDocument())) > + if (frame->contentDocument() && !checkNodeSecurity(exec, frame->contentDocument())) > return false; Ditto. > - if (!checkNodeSecurity(exec, imp->contentDocument())) > + if (imp->contentDocument() && !checkNodeSecurity(exec, imp->contentDocument())) > return false; I think this might read slightly better if the contentDocument was in a local variable. I had trouble seeing the relationship between the two. > - if (!checkNodeSecurity(exec, imp->contentDocument())) > + if (imp->contentDocument() && !checkNodeSecurity(exec, imp->contentDocument())) > return; Ditto. It would be better if there was some shared code rather than this null check having to be everywhere. Maybe a checkDocumentSecurity function that takes a HTMLFrameElementBase* or Document*? r=me
(In reply to comment #3) > Why isn't checkNodeSecurity passing? If it's a blank document, it should > inherit its SecurityOrigin from its parent... There is no document in this case yet (hence the added null checks). Sam actually wonders if "node &&" in checkNodeSecurity() is doing what it's supposed to do. > We should test document.writing into the detached blank frame as well. This sounds like something that's at best tangentially related to this bug to me (I'm also not sure what to document.write).
Comment on attachment 38507 [details] remove unrelated changes This seems fine, but we really should come back to this at some point to address the horribly named checkNodeSecurity (My first attempt at a better name is allowsAccessForNode, which kind of still sucks). It is unclear why the current implementation returns false for null nodes, but I am concerned of the ramifications of changing that without more thought. r=me, but please look into Adam's concern about document.write() if you are able to.
I also want to note that the test probably doesn't need to be in http
(In reply to comment #7) > There is no document in this case yet (hence the added null checks). Sam > actually wonders if "node &&" in checkNodeSecurity() is doing what it's > supposed to do. I see. I think you're right that this is secure, but reasoning that out is quite complicated. I need to spend some more time thinking about the security model of detached frames. For example, do they have window objects? Can their window objects become visible to other origins? I don't think we need to answer those questions before moving forward with this patch though. > This sounds like something that's at best tangentially related to this bug to > me (I'm also not sure what to document.write). I think it's fine to look into this in another bug.
Committed revision 47734. > I think it would be better to put the contentDocument into a local variable > instead of the frame. Fixed. > I also want to note that the test probably doesn't need to be in http The rest of javaScriptURL tests (as landed in r24994) are in http/security, I just wanted to have this next to them. > I need to spend some more time thinking about the security > model of detached frames. For example, do they have window objects? I think I see what you're saying now. My understanding is that they have both window and document objects, so this patch shouldn't have the behavior for them. My reason to make this patch was to fix layout test in bug 28374, so I want to be done with it quickly. It indeed seems that more thought is required to verify whether checkNodeSecurity is doing the right thing - maybe a better fix would be to correct that instead.