Bug 105774 – Add context to the console message generated by Document::printNavigationErrorMessage.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

There's a FIXME noting that the console message generated when navigation is blocked (due to sandboxing, for instance) should contain some context that would allow a developer to understand what's going on. We should fix it.

[Mike West]

Created attachment 180763 [details]
Patch

[Mike West]

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?

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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?

[Build Bot]

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

[Mike West]

Created attachment 180787 [details]
Patch

[Mike West]

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).

[Darin Adler]

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.

[Mike West]

Created attachment 180829 [details]
Patch for landing

[Mike West]

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.

[WebKit Review Bot]

Comment on attachment 180829 [details]
Patch for landing

Clearing flags on attachment: 180829

Committed r138517: <http://trac.webkit.org/changeset/138517>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.