Bug 73959 – Web Inspector: allow cross-domain requests for inspector; use XHR, not custom InspectorFrontendHost methods to fetch things

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

[Andrey Kosyakov]

We used to rely on InspectorFrontendHost::loadResourceSynchronously to fetch third-party resources into inspector front-end (currently only used by source maps). This patch changes to use XHRs instead and grants cross-domain access to inspector front-end.

[Andrey Kosyakov]

Created attachment 118127 [details]
Patch

[Timothy Hatcher]

Comment on attachment 118127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118127&action=review

> Source/WebCore/inspector/InspectorFrontendHost.cpp:125
> +    "file"

Is "file" really needed? That seems like a security risk. (Though the inspector is loaded as a file URL in Safari already, so I guess this isn't any worse.)

[WebKit Review Bot]

Comment on attachment 118127 [details]
Patch

Attachment 118127 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10741168

New failing tests:
http/tests/inspector/compiler-source-mapping.html
http/tests/inspector/compiler-source-mapping-debug.html

[Andrey Kosyakov]

(In reply to comment #2)
> (From update of attachment 118127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118127&action=review
> 
> > Source/WebCore/inspector/InspectorFrontendHost.cpp:125
> > +    "file"
> 
> Is "file" really needed? That seems like a security risk. (Though the inspector is loaded as a file URL in Safari already, so I guess this isn't any worse.)

We currently use this only to fetch source maps. I assume for someone doing development locally, it might be useful to load source maps from files -- although, perhaps, not something they won't survive without.

The loadResourceSynchronously method I was replacing seems not to prevent loading resources from files, so at least we keep the existing behavior.

If anyone feels strongly about allowing file access in the inspector, I don't mind removing it -- yet, as you pointed out, Safari won't be more secure, as it both hosts inspector front-end in a file: scheme and considers different files to be in a same security origin. Fro Chrome, apparently, there also is an escalation path from compromising front-end to local file access, since it allows file: schema access from extensions.

[Andrey Kosyakov]

Created attachment 118158 [details]
Patch

[Andrey Kosyakov]

Fixed another occurrence of loadResourceSynchronously() that I missed.

[Pavel Podivilov]

Comment on attachment 118158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118158&action=review

> Source/WebCore/inspector/InspectorFrontendHost.cpp:137
> +    for (size_t i = 0; i < sizeof schemasToAllowRequestsFor / sizeof schemasToAllowRequestsFor[0]; ++i)

Please use WTF_ARRAY_LENGTH.

> Source/WebCore/inspector/front-end/CompilerSourceMapping.js:109
> +        var mapping = this._loadSynchronously(this._sourceMappingURL);

Why this moved out of try/catch block?

> Source/WebCore/inspector/front-end/CompilerSourceMapping.js:157
> +        return this._loadSynchronously(sourceURL);

Ditto.

[Pavel Podivilov]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 118127 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118127&action=review
> > 
> > > Source/WebCore/inspector/InspectorFrontendHost.cpp:125
> > > +    "file"
> > 
> > Is "file" really needed? That seems like a security risk. (Though the inspector is loaded as a file URL in Safari already, so I guess this isn't any worse.)
> 
> We currently use this only to fetch source maps. I assume for someone doing development locally, it might be useful to load source maps from files -- although, perhaps, not something they won't survive without.
> 
> The loadResourceSynchronously method I was replacing seems not to prevent loading resources from files, so at least we keep the existing behavior.
> 

I think it prevents loading resources from files.

[Adam Barth]

Comment on attachment 118127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118127&action=review

> Source/WebCore/inspector/InspectorFrontendHost.cpp:136
> +    KURL frontendURL = frontendPage->mainFrame()->loader()->originalRequest().url();
> +    m_frontendOrigin = SecurityOrigin::create(frontendURL);

Why not just frontendPage->mainFrame()->document()->securityOrigin() ?

[Andrey Kosyakov]

(In reply to comment #7)
> > +    for (size_t i = 0; i < sizeof schemasToAllowRequestsFor / sizeof schemasToAllowRequestsFor[0]; ++i)
> Please use WTF_ARRAY_LENGTH.

Fixed.

> Why this moved out of try/catch block?

Oops. Fixed as well.

> I think it prevents loading resources from files.

It does not.

[Andrey Kosyakov]

(In reply to comment #9)

> Why not just frontendPage->mainFrame()->document()->securityOrigin() ?

Thanks, fixed.

[Andrey Kosyakov]

Created attachment 118414 [details]
Patch

[Pavel Feldman]

Comment on attachment 118414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118414&action=review

> Source/WebCore/inspector/InspectorFrontendHost.cpp:137
> +        SecurityPolicy::addOriginAccessWhitelistEntry(*m_frontendOrigin, schemasToAllowRequestsFor[i], "", true);

We should not grant additional privileges to the front-end origin since we can compromise the embedder. It is up to the embedder to choose the origin for the front-end and granting additional privileges there will be unexpected.