Bug 40500 – Web Inspector: Should not expose window.console._inspectorCommandLineAPI to the web

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Sam Weinig]

The WebInspector should not be exposing window.console._inspectorCommandLineAPI to the web.  We only want to expose APIs we want people to use.

[Sam Weinig]

<rdar://problem/8085287>

[Pavel Feldman]

Created attachment 58544 [details]
[PATCH] Proposed fix.

- I see no solution other than adding a reference to the API for the time of evaluation;
- also refactored code in a way that console API is defined using prototype, not the legacy string literal;

[Joseph Pecoraro]

Comment on attachment 58544 [details]
[PATCH] Proposed fix.

> WebCore/inspector/front-end/InjectedScript.js
>  InjectedScript._evaluateOn = function(evalFunction, object, expression, dontUseCommandLineAPI)

I know you are just leaving this alone, but I think this would
read much better if the last argument was just "useCommandLineAPI".
Since you are making changes to this part specifically, if you
agree it would be nice to update it! I try to avoid double
negatives "! dontDoSomething".



> +    if (!dontUseCommandLineAPI)
> +        delete inspectedWindow.console._commandLineAPI;

We discussed this. Its not strong enough to plug the hole,
but it removes its face from generic web scripts.


> +            while (node = results.iterateNext()) nodes.push(node);

NIT: Style, move nodes.push to the next line.


> +        inspectedWindow.console.log(object);

Can this just be console.log(object)?


> +        if (InjectedScript._type(object) === "node") {
> +            InjectedScriptHost.pushNodePathToFrontend(object, false, true);
> +        } else {

NIT: Style, braces.


> +    get $0()
> +    get $1()
> +    get $2()
> +    get $3()
> +    get $4()

I think these are much clearer when they are on oneline,
like they used to be. Sometimes style rules are meant
to be broken!

[Pavel Feldman]

> I know you are just leaving this alone, but I think this would
> read much better if the last argument was just "useCommandLineAPI".
> Since you are making changes to this part specifically, if you
> agree it would be nice to update it! I try to avoid double
> negatives "! dontDoSomething".

That would change the default behavior (when parameter is not specified).

> > +            while (node = results.iterateNext()) nodes.push(node);
> 
> NIT: Style, move nodes.push to the next line.

Done.

> > +        inspectedWindow.console.log(object);
> 
> Can this just be console.log(object)?
>

We should access window using the variable given in closure so that the current global object was not 'detected'. It would probably work in this case, but I left it for consistency.

> > +        if (InjectedScript._type(object) === "node") {
> > +            InjectedScriptHost.pushNodePathToFrontend(object, false, true);
> > +        } else {
> 
> NIT: Style, braces.

Done.

> > +    get $0()
> > +    get $1()
> > +    get $2()
> > +    get $3()
> > +    get $4()
> 
> I think these are much clearer when they are on oneline,
> like they used to be. Sometimes style rules are meant
> to be broken!

I've been respecting the style in places that were suffering from it much more than this one, so I'd stick to it. Again, violating the style would give you an option of commenting on it in the review. Obviously not an argument in this case since you anyway commented :)

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/InjectedScript.js
Committed r61181