Summary: | Add context to the console message generated by Document::printNavigationErrorMessage. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||
Component: | WebCore Misc. | Assignee: | Mike West <mkwst> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ojan.autocc, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 105819 | ||||||||||
Bug Blocks: | 101964 | ||||||||||
Attachments: |
|
Description
Mike West
2012-12-26 14:34:18 PST
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. |