[webkit-reviews] review denied: [Bug 158875] Add flag allow-popups-to-escape-sandbox to iframe sandbox : [Attachment 312289] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 158875: Add flag allow-popups-to-escape-sandbox to iframe sandbox
https://bugs.webkit.org/show_bug.cgi?id=158875

Attachment 312289: Patch

https://bugs.webkit.org/attachment.cgi?id=312289&action=review




--- Comment #25 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 312289
  --> https://bugs.webkit.org/attachment.cgi?id=312289
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312289&action=review

> Source/WebCore/dom/Document.cpp:3041
> +	   const char* failureReason = nullptr;

I am trying to map this implementation to:
https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate

But this is not straightforward.

> Source/WebCore/dom/Document.cpp:3044
> +	   else if (targetFrame != &m_frame->tree().top() &&
isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) &&
(isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame))

How do we know targetFrame is a popup here? The check says that targetFrame is
not m_frame's top frame.

What if targetFrame is one of m_frame's subframes?

> Source/WebCore/dom/Document.cpp:3047
> +	       if (!isSandboxed(SandboxTopNavigation))

I don't think this is possible. See this code earlier in the function:
if (!isSandboxed(SandboxTopNavigation) && targetFrame ==
&m_frame->tree().top())
    return true;