There are a number of calls to the javascript eval() function inside InjectedScriptSource.js. This has recently become problematic with the introduction of Content-Security-Policy (CSP) which limits the contexts in which code generated from strings can execute. When firing up the inspector against a page with CSP enabled, I am seeing an error (chromium example): ASSERTION FAILED: !hadException third_party/WebKit/Source/WebCore/inspector/InjectedScript.cpp(110) : WebCore::Node* WebCore::InjectedScript::nodeForObjectId(const WTF::String&) [1873:1873:3971452761588:ERROR:CONSOLE(119)] "Uncaught Error: Code generation from strings disallowed for this context" Tracking this down, I see that this operates by cobbling up a string of the form '{"injectedScriptId":1,"id":1}' and passing this into the JS engine as a string via ScriptFunctionCall::call() to the JS function named "nodeToObjectId". This is found in WebCore/Inspector/InjectedScriptSource.js, which tries to parse the string by the function _parseObjectId(objectId), in the same file: _parseObjectId: function(objectId) { return eval("(" + objectId + ")"); }, I was able to circumvent this particular issue by changing the eval to a JSON.parse(objectId). For the cases where this will work, this is a cleaner approach than the wide-open eval() calls, even though the contents of the string may be completely controlled by the inspector itself. A layout test running the inspector against CSP-enabled pages would be required here.
@tsepez: Would you be willing to post a patch replacing eval with JSON.parse in a bug blocking this one? That way we can use this bug as a meta bug to track all these issues.
Replacing eval with JSON.parse is a no go since inspector would depend on prototype's JSON.parse for the pages like cnn.com. Instead, we might need to add eval into the InjectedScriptHost binding and grant appropriate permissions to the operation.
Can we give InjectedScriptHost its own JSON object from a fresh v8::Context? CSP just disables eval on a per-context basis.
(In reply to comment #3) > Can we give InjectedScriptHost its own JSON object from a fresh v8::Context? CSP just disables eval on a per-context basis. We can do that if it does not turn into some kind of context nightmare as the one we had with the utility context. There are two use cases for eval usage in injected script: 1) JSON parsing. We can work around it by means of native JSON parsing that we use for the inspector protocol. It is just that we were lazy and used eval 2) window.eval for console expression evaluation. I am not quite sure what you are suggesting to do with this one.
> 1) JSON parsing. We can work around it by means of native JSON parsing that we use for the inspector protocol. It is just that we were lazy and used eval Ok. That sounds like it's under control. > 2) window.eval for console expression evaluation. I am not quite sure what you are suggesting to do with this one. I've lost track of how this all fits together, but can we add a binding to InjectedScriptHost to call ScriptController::evaluate ? It's just the JavaScript eval function/operator that's blocked by CSP.
> > 2) window.eval for console expression evaluation. I am not quite sure what you are suggesting to do with this one. > > I've lost track of how this all fits together, but can we add a binding to InjectedScriptHost to call ScriptController::evaluate ? It's just the JavaScript eval function/operator that's blocked by CSP. Sure, I don't see why not.
Related Chromium issue: http://code.google.com/p/chromium/issues/detail?id=82212
(In reply to comment #5) > > 2) window.eval for console expression evaluation. I am not quite sure what you are suggesting to do with this one. > > I've lost track of how this all fits together, but can we add a binding to InjectedScriptHost to call ScriptController::evaluate ? It's just the JavaScript eval function/operator that's blocked by CSP. Providing a binding to InjectedScriptHost to call ScriptController::evaluate wouldn't help here if ScriptController::disableEval has already been called for the context. We need a way to enable evaluations only for expressions provided by inspector. One possible approach would be for the native part of the inspector to temporarily allow 'eval' using e.g. callback set with V8::SetAllowCodeGenerationFromStringsCallback that allows overriding eval permissions. It would work for synchronous evals but not for things like 'setTimeout("eval('var x = 10;')",0)'. Another way of dealing with CSP-enabled Pages would be to ask user whether he would like to disable CSP when he tries to evaluate something in console/performs other inspector actions that require JS evaluation.
> Providing a binding to InjectedScriptHost to call ScriptController::evaluate wouldn't help here if ScriptController::disableEval has already been called for the context. We need a way to enable evaluations only for expressions provided by inspector. Why is that? disableEval shouldn't affect ScriptController::evaluate.
(In reply to comment #9) > > Providing a binding to InjectedScriptHost to call ScriptController::evaluate wouldn't help here if ScriptController::disableEval has already been called for the context. We need a way to enable evaluations only for expressions provided by inspector. > > Why is that? disableEval shouldn't affect ScriptController::evaluate. Oh, I see it uses Script::Compile/Run so it should work fine. But what about asynchronous evaluations when user types something like this in inspector console: setTimeout("eval('var x = 10;')",0) It is probably OK to fail in this case with a reasonable error message.
> It is probably OK to fail in this case with a reasonable error message. Yeah, that will be blocked, just like it would be if run by the page itself.
(In reply to comment #5) > > 1) JSON parsing. We can work around it by means of native JSON parsing that we use for the inspector protocol. It is just that we were lazy and used eval > > Ok. That sounds like it's under control. I've got this almost working. There doesn't seem to be a way to convert from an InspectorValue back to a native JavaScript value. Looks like I need to add that.
Created attachment 93826 [details] work-in-progress
Created attachment 93845 [details] Patch
Comment on attachment 93845 [details] Patch Attachment 93845 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8707356
Comment on attachment 93845 [details] Patch Attachment 93845 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8708347
Created attachment 93848 [details] Patch
Comment on attachment 93848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93848&action=review > Source/WebCore/bindings/v8/V8InspectorValue.cpp:40 > +v8::Handle<v8::Value> toV8(InspectorValue* inspectorValue) InspectorValue is used for constructing JSON objects in native parts of the inspector. It should not be exposed to the JavaScript code. > Source/WebCore/inspector/InjectedScriptHost.idl:47 > + // FIXME: parseJSON actually returns an arbitray value. Should its return type be DOMObject then? > Source/WebCore/inspector/InjectedScriptHost.idl:48 > + [Custom] void parseJSON(in DOMString json); This solves only part of the problem. We still need to support evaluation in the inspected page for inspector console to work. We need to implement InjectedScriptHost.evaluate instead. This would cover both JSON.parse and console evaluations. Additionally it wouldn't require bindings for InspectorValues.
I'm working on a change that introduces InjectedScriptHost.evaluate it should work for JSON parsing as well.
(In reply to comment #19) > I'm working on a change that introduces InjectedScriptHost.evaluate it should work for JSON parsing as well. That approach will also work. Thanks for working on this bug.
Created attachment 93919 [details] Patch
Comment on attachment 93919 [details] Patch Attachment 93919 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8705705
(In reply to comment #21) > Created an attachment (id=93919) [details] > Patch I don't use ScriptController::evaluate because we need access to exceptions and return values. The fix works fine for V8 but it makes inspector tests hang in case of JSC. Also in debug mode they crash and it seems that the problem is that JSMainThreadExecState::evaluate assumes that JS stack is empty when it's called. I'd tried alternative approach with creating EvalExecutable but since evals are disabled by a property on JSGlobalObject this didn't work without adding an option for EvalExecutable to bypass JSGlobalObject::isEvalEnabled() check. Any ideas how to implement this in the JSC binding?
Comment on attachment 93919 [details] Patch Attachment 93919 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8705712 New failing tests: inspector/debugger/event-listener-breakpoints.html
Created attachment 93926 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
> Any ideas how to implement this in the JSC binding? I'll take a look.
The problem appears to be that JSC::evaluate only works with the global scope, but the inspector appears to need to evaluate things in a non-global scope (at least in these tests).
Created attachment 93943 [details] Works in JSC
Comment on attachment 93943 [details] Works in JSC Attachment 93943 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8705771 New failing tests: inspector/debugger/event-listener-breakpoints.html
Created attachment 93956 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 94061 [details] Patch with Adam's implementation of JSC binding Adam, thanks for you help with JSC!
Committed r86837: <http://trac.webkit.org/changeset/86837>