Running JS against a chrome:// page risks sensitive information. Add a safeguard against malicious bookmarklets and the like to WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp
Created attachment 104751 [details] Patch
Comment on attachment 104751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104751&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:2291 > + // Protect privileged pages against bookmarklets and other javascript > + // manipulations. > + if (m_frame->document()->url().protocolIs("chrome") > + || m_frame->document()->url().protocolIs("chrome-extension")) > + return; We shouldn't have these string literals in this code. Isn't there something in SchemeRegistry we could use?
Didn't see an appropriate shouldTreatScheme... method in SchemeRegistry.h. Also didn't see the literal "chrome" anywhere under webkit :( ... would this be added to a SchemeRegistry category outside of webkit by the browser itself? eg, $ find . -name \*.h -exec grep '"chrome' {} /dev/null \; ./Source/WebKit/chromium/base/logging.h:// "chromeos" directory. $ find . -name \*.cpp -exec grep '"chrome' {} /dev/null \; ./Source/WebKit/chromium/src/BackForwardListChromium.cpp:const char backForwardNavigationScheme[] = "chrome-back-forward"; ./Source/WebKit/chromium/src/WebFrameImpl.cpp: if (m_frame->document()->url().protocolIs("chrome") ./Source/WebKit/chromium/src/WebFrameImpl.cpp: || m_frame->document()->url().protocolIs("chrome-extension")) ./Source/WebKit/chromium/tests/WebFrameTest.cpp: chromeURL("chrome://"
You can add a new type to SchemeRegistry.h. Chrome registers a bunch of schemes at with SchemeRegistry when it initializes WebKit.
Created attachment 104770 [details] Patch
Ok, but feels kind of wrong since only the chromium code path checks this particular registry.
Probably needs to get wrapped in a WebSecurityPolicy method ???
> Ok, but feels kind of wrong since only the chromium code path checks this particular registry. Yeah, you should probably check this in ScriptController::executeIfJavaScriptURL also. > Probably needs to get wrapped in a WebSecurityPolicy method ??? Yep!
Created attachment 104857 [details] Patch
would ScriptController::executeIfJavaScriptURL affect <a href="javascript:"> cases, or only navigation from the browser itself? Current patch only applies this test as before.
Created attachment 104860 [details] Patch
Patch with two ChangeLogs. For review. Thanks.
Comment on attachment 104860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104860&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) This line will prevent this change from landing. You should replace it with a line that says you wrote a Chromium WebKit API unit test. > Source/WebCore/ChangeLog:9 > + It would be nice to put some more explanation in the ChangeLog as to why we're making this change and whether we think this registry is useful for WebCore's JavaScript URL implementation. > Source/WebCore/platform/SchemeRegistry.cpp:107 > +static URLSchemesMap& notAllowingJavascriptURLsSchemes() This name seems slightly tortured, but I can see how it fits the pattern.
> would ScriptController::executeIfJavaScriptURL affect <a href="javascript:"> cases, or only navigation from the browser itself? Current patch only applies this test as before. Only the <a href="javascript:"> case. I think it's fine to handle that case in a separate patch, but we should say that in the ChangeLog entry.
+fishd for WebKit API review.
Created attachment 105013 [details] Patch
Reworked comments in change log in latest patch.
Comment on attachment 105013 [details] Patch Rejecting attachment 105013 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2 Last 500 characters of output: at 63 (offset 3 lines). Hunk #4 succeeded at 98 (offset 3 lines). Hunk #5 succeeded at 119 (offset 3 lines). Hunk #6 succeeded at 145 (offset 3 lines). Hunk #7 succeeded at 191 with fuzz 1 (offset 24 lines). 1 out of 7 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/WebFrameTest.cpp.rej patching file Source/WebKit/chromium/tests/data/history.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/9485272
Created attachment 105033 [details] Patch
Comment on attachment 105033 [details] Patch Merge conflict resolved.
Comment on attachment 105033 [details] Patch Forwarding fishd's r+.
Comment on attachment 105033 [details] Patch Clearing flags on attachment: 105033 Committed r93734: <http://trac.webkit.org/changeset/93734>
All reviewed patches have been landed. Closing bug.