[webkit-reviews] review granted: [Bug 53191] showModalDialog does not correctly return the defined returnValue in case domain relaxing is used : [Attachment 104769] proposed fix

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Alexey Proskuryakov
<ap at webkit.org>'s request for review:
Bug 53191: showModalDialog does not correctly return the defined returnValue in
case domain relaxing is used
https://bugs.webkit.org/show_bug.cgi?id=53191

Attachment 104769: proposed fix
https://bugs.webkit.org/attachment.cgi?id=104769&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104769&action=review


r=me with those changes

> Source/WebCore/ChangeLog:19
> +	   dismissed. A dialog can navigate itself, and it also creates a new
JSDOMWindow on firt load

s/firt/first/

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:678
> +    JSDOMWindow* globalObject = toJSDOMWindow(m_frame.get(),
normalWorld(m_exec->globalData()));
> +    if (!globalObject)
> +	   return jsUndefined();
> +    if
(!asJSDOMWindow(m_exec->lexicalGlobalObject())->allowsAccessFrom(globalObject->
globalExec()))
>	   return jsUndefined();
>      Identifier identifier(m_exec, "returnValue");
>      PropertySlot slot;
> -    if (!m_globalObject->JSGlobalObject::getOwnPropertySlot(m_exec,
identifier, slot))
> +    if (!globalObject->JSGlobalObject::getOwnPropertySlot(m_exec,
identifier, slot))

There's no need to do this allowsAccessFrom check ourselves; it's built into
the window object. Just call the normal getOwnPropertySlot, instead of skipping
window checks by calling JSGlobalObject::getOwnPropertySlot directly.

By the way, make sure to call out this additional security check in your
ChangeLog, and explain why you added it (to match Firefox).