Created attachment 180763 [details] Patch

Comment on attachment 180763 [details] Patch I need to generate new expectations for non-chromium ports, so this patch isn't for landing as-is, but the general direction should be correct. WDYT?

Attachment 180763 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/dom/Document.cpp:398: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 180763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180763&action=review > Source/WebCore/dom/Document.cpp:2720 > + printNavigationErrorMessage(targetFrame, url(), "The frame attempting navigation of the top-level window is sandboxed, and the 'allow-top-navigation' flag is not set."); I assume none of the inspector messages are localized? > LayoutTests/fast/frames/sandboxed-iframe-navigation-targetlink-expected.txt:3 > +CONSOLE MESSAGE: Unsafe JavaScript attempt to initiate a navigation change for frame with URL about:blank from frame with URL sandboxed-iframe-navigation-targetlink.html.The frame attempting navigation of the top-level window is sandboxed, and the 'allow-top-navigation' flag is not set. Spaces?

Comment on attachment 180763 [details] Patch Attachment 180763 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15540469 New failing tests: http/tests/security/frameNavigation/xss-DENIED-plugin-navigation.html http/tests/security/no-popup-from-sandbox-top.html

Created attachment 180787 [details] Patch

Comment on attachment 180763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180763&action=review Thanks! I didn't expect anyone to be around until January. :) >> Source/WebCore/dom/Document.cpp:2720 >> + printNavigationErrorMessage(targetFrame, url(), "The frame attempting navigation of the top-level window is sandboxed, and the 'allow-top-navigation' flag is not set."); > > I assume none of the inspector messages are localized? They are not, which is unfortunate. That's on my list of things to change, but I haven't started on it yet. >> LayoutTests/fast/frames/sandboxed-iframe-navigation-targetlink-expected.txt:3 >> +CONSOLE MESSAGE: Unsafe JavaScript attempt to initiate a navigation change for frame with URL about:blank from frame with URL sandboxed-iframe-navigation-targetlink.html.The frame attempting navigation of the top-level window is sandboxed, and the 'allow-top-navigation' flag is not set. > > Spaces? Indeed! The next patch fixes that, and slightly changes the message (s/initiate a navigation change/initiate navigation/g, and wrapping URLs in quotes).

Comment on attachment 180787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180787&action=review > Source/WebCore/dom/Document.cpp:395 > -static void printNavigationErrorMessage(Frame* frame, const KURL& activeURL) > +static void printNavigationErrorMessage(Frame* frame, const KURL& activeURL, const String& reason) The reason string should be a const char* rather than a const String&. It is not helpful to create a String object and then destroy it just to carry the reason string from the call site into this function. > Source/WebCore/dom/Document.cpp:2721 > - printNavigationErrorMessage(targetFrame, url()); > + if (isSandboxed(SandboxTopNavigation) && targetFrame == m_frame->tree()->top()) > + printNavigationErrorMessage(targetFrame, url(), "The frame attempting navigation of the top-level window is sandboxed, but the 'allow-top-navigation' flag is not set."); > + else > + printNavigationErrorMessage(targetFrame, url(), "The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors."); It seems inelegant to repeat the printNavigationErrorMessage call. Instead I suggest either a local variable of type const char* or a helper function that returns the reason string.

Created attachment 180829 [details] Patch for landing

Thanks, Darin. (In reply to comment #8) > (From update of attachment 180787 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180787&action=review > > > Source/WebCore/dom/Document.cpp:395 > > -static void printNavigationErrorMessage(Frame* frame, const KURL& activeURL) > > +static void printNavigationErrorMessage(Frame* frame, const KURL& activeURL, const String& reason) > > The reason string should be a const char* rather than a const String&. It is not helpful to create a String object and then destroy it just to carry the reason string from the call site into this function. Good point. Done. > > Source/WebCore/dom/Document.cpp:2721 > > - printNavigationErrorMessage(targetFrame, url()); > > + if (isSandboxed(SandboxTopNavigation) && targetFrame == m_frame->tree()->top()) > > + printNavigationErrorMessage(targetFrame, url(), "The frame attempting navigation of the top-level window is sandboxed, but the 'allow-top-navigation' flag is not set."); > > + else > > + printNavigationErrorMessage(targetFrame, url(), "The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors."); > > It seems inelegant to repeat the printNavigationErrorMessage call. Instead I suggest either a local variable of type const char* or a helper function that returns the reason string. I went with the former. Moving the minimal logic out to another helper function seems like overkill.

Comment on attachment 180829 [details] Patch for landing Clearing flags on attachment: 180829 Committed r138517: <http://trac.webkit.org/changeset/138517>

All reviewed patches have been landed. Closing bug.