Bug 101683 – Merge isViewSource checks in ScriptController::executeIfJavaScriptURL and ScriptController::canExecuteScripts.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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!

[Adam Barth]

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

[Alexey Proskuryakov]

Thank you Adam, reusing the bug to track the second issue then.

[Mike West]

I'll take a look at the isViewSource checks.

[Mike West]

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?

[Mike West]

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

[Adam Barth]

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

[Mike West]

(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?

[Adam Barth]

I don't expect there will be any change in behavior.

[Mike West]

Created attachment 173563 [details]
Patch

[Adam Barth]

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

[Adam Barth]

I see, the check is in ScriptController::executeScript, which is called by executeIfJavaScriptURL

[Adam Barth]

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

[Adam Barth]

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.

[Mike West]

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

[Mike West]

Created attachment 184709 [details]
Patch

[Adam Barth]

Comment on attachment 184709 [details]
Patch

Ok

[WebKit Review Bot]

Comment on attachment 184709 [details]
Patch

Clearing flags on attachment: 184709

Committed r140839: <http://trac.webkit.org/changeset/140839>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.