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.
Created attachment 118127 [details] Patch
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.)
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
(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.
Created attachment 118158 [details] Patch
Fixed another occurrence of loadResourceSynchronously() that I missed.
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.
(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.
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() ?
(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.
(In reply to comment #9) > Why not just frontendPage->mainFrame()->document()->securityOrigin() ? Thanks, fixed.
Created attachment 118414 [details] Patch
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.