Bug 40500 - Web Inspector: Should not expose window.console._inspectorCommandLineAPI to the web
Summary: Web Inspector: Should not expose window.console._inspectorCommandLineAPI to t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Pavel Feldman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-06-11 15:03 PDT by Sam Weinig
Modified: 2010-06-15 04:38 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed fix. (13.57 KB, patch)
2010-06-12 00:44 PDT, Pavel Feldman
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-06-11 15:03:20 PDT
The WebInspector should not be exposing window.console._inspectorCommandLineAPI to the web.  We only want to expose APIs we want people to use.
Comment 1 Sam Weinig 2010-06-11 15:03:49 PDT
<rdar://problem/8085287>
Comment 2 Pavel Feldman 2010-06-12 00:44:40 PDT
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;
Comment 3 Joseph Pecoraro 2010-06-14 13:44:35 PDT
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!
Comment 4 Pavel Feldman 2010-06-15 04:37:46 PDT
> 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 :)
Comment 5 Pavel Feldman 2010-06-15 04:38:30 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/InjectedScript.js
Committed r61181