Most of these checks are there, so perhaps the CSP one should be there, too? Notably, isViewSource check seems to be duplicated in these locations... But one looks in Frame, and another in Document!
> Most of these checks are there, so perhaps the CSP one should be there, too? In CSP, you can have JavaScript URLs turned off but have script (more generally) turned on. CSP treats JavaScript URLs like inline script (e.g., <script>...</script>), the idea being that JavaScript URLs can be injected by attackers (e.g., <a href="$userSuppliedString">...</a>). The view-source bits do look dodgy though.
Thank you Adam, reusing the bug to track the second issue then.
I'll take a look at the isViewSource checks.
Can JavaScript URLs ever execute if JavaScript is disabled? That is, could I simply replace some of the logic in executeIfJavaScriptURL with a call to canExecuteScripts?
(In reply to comment #4) > Can JavaScript URLs ever execute if JavaScript is disabled? That is, could I simply replace some of the logic in executeIfJavaScriptURL with a call to canExecuteScripts? For instance, executeIfJavaScriptURL doesn't check settings, nor does it check sandbox status. It seems like it should do both.
> Can JavaScript URLs ever execute if JavaScript is disabled? Nope. > That is, could I simply replace some of the logic in executeIfJavaScriptURL with a call to canExecuteScripts? Yes, but we'll need the extra CSP check for 'unsafe-inline'.
(In reply to comment #6) > > That is, could I simply replace some of the logic in executeIfJavaScriptURL with a call to canExecuteScripts? > > Yes, but we'll need the extra CSP check for 'unsafe-inline'. Right. I've put a patch together that simply replaces the viewsource check with a call to canExecuteScripts. That works correctly, so far as I can tell, but I'm having trouble identifying a testable change in behavior. The two missing tests (settings and sandbox) seem both to be taken care of somewhere else in the code; I haven't had any luck constructing a test that failed without the patch. Do you see anything relevant?
I don't expect there will be any change in behavior.
Created attachment 173563 [details] Patch
Comment on attachment 173563 [details] Patch I'm looking at the call site in FrameLoader::urlSelected. Is there some way that was checking that JavaScript was disabled before? I don't see where that check would have been. data:text/html,<iframe sandbox srcdoc="<a href=javascript:alert(1)>click me</a>"><iframe> That doesn't seem to execute when clicked....
I see, the check is in ScriptController::executeScript, which is called by executeIfJavaScriptURL
Comment on attachment 173563 [details] Patch So, this does the wrong thing for view-source documents. It's too bad we don't have a test for that. (We should add one.) Notice that ScriptController::canExecuteScripts returns true when isViewSource, but executeIfJavaScriptURL used to abort execution if inViewSourceMode
As for the relation between isViewSource and inViewSourceMode, it looks like the Document freezes the inViewSourceMode from the Frame on creation (i.e., in DocumentWriter::createDocument). The inViewSourceMode is at the mercy of the frame's attributes (i.e., in HTMLFrameElementBase::parseAttribute). That means this teh call site in executeIfJavaScriptURL should use isViewSource from the Document rather than inViewSourceMode from the frame. The way to write a test is to create a frame with a javascript URL in it, after the frame loads, set the viewsource attribute on the frame, then try to execute the javascript URL. Currently, the javascript URL will be blocked (the Frame is in view source mode but the document is not), but there isn't any reason that it should be blocked.
(In reply to comment #13) > As for the relation between isViewSource and inViewSourceMode, it looks like the Document freezes the inViewSourceMode from the Frame on creation (i.e., in DocumentWriter::createDocument). The inViewSourceMode is at the mercy of the frame's attributes (i.e., in HTMLFrameElementBase::parseAttribute). > > That means this teh call site in executeIfJavaScriptURL should use isViewSource from the Document rather than inViewSourceMode from the frame. The way to write a test is to create a frame with a javascript URL in it, after the frame loads, set the viewsource attribute on the frame, then try to execute the javascript URL. Currently, the javascript URL will be blocked (the Frame is in view source mode but the document is not), but there isn't any reason that it should be blocked. Sorry, I missed this response way back in November. Given that executeIfJavaScriptURL ends up calling canExecuteScripts (via executeScript), I think we can safely drop the isViewSource check from the former entirely. I'll attach a patch in a moment that does just that, and adds a small test to verify that the document's viewsource bit is checked.
Created attachment 184709 [details] Patch
Comment on attachment 184709 [details] Patch Ok
Comment on attachment 184709 [details] Patch Clearing flags on attachment: 184709 Committed r140839: <http://trac.webkit.org/changeset/140839>
All reviewed patches have been landed. Closing bug.