Skip to content

Commit

Permalink
Ensure inspectedWindow.reload reloads the correct page
Browse files Browse the repository at this point in the history 
This prevents unintended reloads when racing with navigations.

Drive-by: Check extension allowlist when loading page resources.

Bug: 338248595
Change-Id: Ibbac5bbd45b1db0d05e32fe8e384740933ee4639
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5546062
Reviewed-by: Danil Somsikov <[email protected]>
Commit-Queue: Philip Pfaffe <[email protected]>
  • Loading branch information
@pfaffe
pfaffe authored and Devtools-frontend LUCI CQ committed May 17, 2024
1 parent bba0abf commit 33a09fb
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 23 deletions.
28 changes: 23 additions & 5 deletions front_end/core/sdk/ResourceTreeModel.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,22 @@ import {
describeWithMockConnection,
setMockConnectionResponseHandler,
} from '../../testing/MockConnection.js';
import {addChildFrame, getMainFrame, MAIN_FRAME_ID, navigate} from '../../testing/ResourceTreeHelpers.js';
import {
addChildFrame,
getInitializedResourceTreeModel,
getMainFrame,
LOADER_ID,
MAIN_FRAME_ID,
navigate,
} from '../../testing/ResourceTreeHelpers.js';

import * as SDK from './sdk.js';

describeWithMockConnection('ResourceTreeModel', () => {
it('calls clearRequests on reloadPage', () => {
const target = createTarget();
const resourceTreeModel = target.model(SDK.ResourceTreeModel.ResourceTreeModel);
it('calls clearRequests on reloadPage', async () => {
const resourceTreeModel = await getInitializedResourceTreeModel(createTarget());
const clearRequests = sinon.stub(SDK.NetworkManager.NetworkManager.prototype, 'clearRequests');
resourceTreeModel!.reloadPage();
resourceTreeModel.reloadPage();
assert.isTrue(clearRequests.calledOnce, 'Not called just once');
});

Expand Down Expand Up @@ -102,6 +108,18 @@ describeWithMockConnection('ResourceTreeModel', () => {
assert.isTrue(reloadSubframePage.notCalled);
});

it('tags reloads with the targets loaderId', async () => {
const target = createTarget();
const resourceTreeModel = await getInitializedResourceTreeModel(target);

const reload = sinon.spy(target.pageAgent(), 'invoke_reload');
assert.isNotNull(resourceTreeModel.mainFrame);
resourceTreeModel.reloadPage();
assert.isTrue(reload.calledOnce);
assert.deepStrictEqual(
reload.args[0], [{ignoreCache: undefined, loaderId: LOADER_ID, scriptToEvaluateOnLoad: undefined}]);
});

it('identifies not top frame', async () => {
const tabTarget = createTarget({type: SDK.Target.Type.Tab});
const mainFrameTarget = createTarget({parentTarget: tabTarget});
Expand Down
12 changes: 8 additions & 4 deletions front_end/core/sdk/ResourceTreeModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,10 @@ export class ResourceTreeModel extends SDKModel<EventTypes> {
}

reloadPage(ignoreCache?: boolean, scriptToEvaluateOnLoad?: string): void {
const loaderId = this.mainFrame?.loaderId;
if (!loaderId) {
return;
}
// Only dispatch PageReloadRequested upon first reload request to simplify client logic.
if (!this.#pendingReloadOptions) {
this.dispatchEventToListeners(Events.PageReloadRequested, this);
Expand All @@ -431,7 +435,7 @@ export class ResourceTreeModel extends SDKModel<EventTypes> {
networkManager.clearRequests();
}
this.dispatchEventToListeners(Events.WillReloadPage);
void this.agent.invoke_reload({ignoreCache, scriptToEvaluateOnLoad});
void this.agent.invoke_reload({ignoreCache, scriptToEvaluateOnLoad, loaderId});
}

navigate(url: Platform.DevToolsPath.UrlString): Promise<Protocol.Page.NavigateResponse> {
Expand Down Expand Up @@ -659,7 +663,7 @@ export class ResourceTreeFrame {
#sameTargetParentFrameInternal: ResourceTreeFrame|null;
readonly #idInternal: Protocol.Page.FrameId;
crossTargetParentFrameId: string|null;
#loaderIdInternal: string;
#loaderIdInternal: Protocol.Network.LoaderId;
#nameInternal: string|null|undefined;
#urlInternal: Platform.DevToolsPath.UrlString;
#domainAndRegistryInternal: string;
Expand Down Expand Up @@ -692,7 +696,7 @@ export class ResourceTreeFrame {
this.#idInternal = frameId;
this.crossTargetParentFrameId = null;

this.#loaderIdInternal = payload?.loaderId || '';
this.#loaderIdInternal = payload?.loaderId ?? '' as Protocol.Network.LoaderId;
this.#nameInternal = payload && payload.name;
this.#urlInternal =
payload && payload.url as Platform.DevToolsPath.UrlString || Platform.DevToolsPath.EmptyUrlString;
Expand Down Expand Up @@ -812,7 +816,7 @@ export class ResourceTreeFrame {
return this.#unreachableUrlInternal;
}

get loaderId(): string {
get loaderId(): Protocol.Network.LoaderId {
return this.#loaderIdInternal;
}

Expand Down
40 changes: 40 additions & 0 deletions front_end/models/extensions/ExtensionServer.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,28 @@ describeWithDevtoolsExtension('Extensions', {}, context => {
assert.isTrue(addExtensionSpy.calledOnce, 'addExtension called once');
assert.isTrue(addExtensionSpy.returned(undefined), 'addExtension returned undefined');
});

it('only returns page resources for allowed targets', async () => {
const urls = ['http://example.com', 'chrome://version'] as Platform.DevToolsPath.UrlString[];
const targets = urls.map(async url => {
const target = createTarget({url});
const resourceTreeModel = target.model(SDK.ResourceTreeModel.ResourceTreeModel);
assert.isNotNull(resourceTreeModel);
await resourceTreeModel.once(SDK.ResourceTreeModel.Events.CachedResourcesLoaded);
target.setInspectedURL(url);
resourceTreeModel.mainFrame?.addResource(new SDK.Resource.Resource(
resourceTreeModel, null, url, url, null, null, Common.ResourceType.resourceTypes.Document, 'application/text',
null, null));
return target;
});

await Promise.all(targets);

const resources =
await new Promise<Chrome.DevTools.Resource[]>(r => context.chrome.devtools!.inspectedWindow.getResources(r));

assert.deepStrictEqual(resources.map(r => r.url), ['https://example.com/', 'http://example.com']);
});
});

describeWithDevtoolsExtension('Extensions', {}, context => {
Expand Down Expand Up @@ -245,6 +267,24 @@ describeWithDevtoolsExtension('Extensions', {}, context => {

await context.chrome.devtools?.recorder.unregisterRecorderExtensionPlugin(extensionPlugin);
});

it('reload only the main toplevel frame', async () => {
const target = SDK.TargetManager.TargetManager.instance().primaryPageTarget();
assert.isNotNull(target);
const secondTarget = createTarget();

const secondResourceTreeModel = secondTarget.model(SDK.ResourceTreeModel.ResourceTreeModel);
assert.isNotNull(secondResourceTreeModel);
const secondReloadStub = sinon.stub(secondResourceTreeModel, 'reloadPage');
const resourceTreeModel = target.model(SDK.ResourceTreeModel.ResourceTreeModel);
assert.isNotNull(resourceTreeModel);
const reloadStub = sinon.stub(resourceTreeModel, 'reloadPage');
const reloadPromise = new Promise(resolve => reloadStub.callsFake(resolve));
context.chrome.devtools!.inspectedWindow.reload();
await reloadPromise;
assert.isTrue(reloadStub.calledOnce);
assert.isTrue(secondReloadStub.notCalled);
});
});

const allowedUrl = FRAME_URL;
Expand Down
41 changes: 27 additions & 14 deletions front_end/models/extensions/ExtensionServer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,17 @@ export class ExtensionServer extends Common.ObjectWrapper.ObjectWrapper<EventTyp
{command: 'open-resource', resource: this.makeResource(contentProvider), lineNumber: lineNumber + 1});
}

private onReload(message: PrivateAPI.ExtensionServerRequestMessage): Record {
private extensionAllowedOnURL(url: Platform.DevToolsPath.UrlString, port: MessagePort): boolean {
const origin = extensionOrigins.get(port);
const extension = origin && this.registeredExtensions.get(origin);
return Boolean(extension?.isAllowedOnTarget(url));
}

private extensionAllowedOnTarget(target: SDK.Target.Target, port: MessagePort): boolean {
return this.extensionAllowedOnURL(target.inspectedURL(), port);
}

private onReload(message: PrivateAPI.ExtensionServerRequestMessage, port: MessagePort): Record {
if (message.command !== PrivateAPI.Commands.Reload) {
return this.status.E_BADARG('command', `expected ${PrivateAPI.Commands.Reload}`);
}
Expand All @@ -821,7 +831,15 @@ export class ExtensionServer extends Common.ObjectWrapper.ObjectWrapper<EventTyp
if (options.injectedScript) {
injectedScript = '(function(){' + options.injectedScript + '})()';
}
SDK.ResourceTreeModel.ResourceTreeModel.reloadAllPages(Boolean(options.ignoreCache), injectedScript);
const target = SDK.TargetManager.TargetManager.instance().primaryPageTarget();
if (!target) {
return this.status.OK();
}
const resourceTreeModel = target.model(SDK.ResourceTreeModel.ResourceTreeModel);
if (!this.extensionAllowedOnTarget(target, port)) {
return this.status.E_FAILED('Permission denied');
}
resourceTreeModel?.reloadPage(Boolean(options.ignoreCache), injectedScript);
return this.status.OK();
}

Expand Down Expand Up @@ -854,10 +872,8 @@ export class ExtensionServer extends Common.ObjectWrapper.ObjectWrapper<EventTyp
if (message.command !== PrivateAPI.Commands.GetHAR) {
return this.status.E_BADARG('command', `expected ${PrivateAPI.Commands.GetHAR}`);
}
const origin = extensionOrigins.get(port);
const extension = origin && this.registeredExtensions.get(origin);
const requests =
Logs.NetworkLog.NetworkLog.instance().requests().filter(r => extension?.isAllowedOnTarget(r.url()));
Logs.NetworkLog.NetworkLog.instance().requests().filter(r => this.extensionAllowedOnURL(r.url(), port));
const harLog = await HAR.Log.Log.build(requests);
for (let i = 0; i < harLog.entries.length; ++i) {
// @ts-ignore
Expand All @@ -870,7 +886,7 @@ export class ExtensionServer extends Common.ObjectWrapper.ObjectWrapper<EventTyp
return {url: contentProvider.contentURL(), type: contentProvider.contentType().name()};
}

private onGetPageResources(): {url: string, type: string}[] {
private onGetPageResources(_message: unknown, port: MessagePort): {url: string, type: string}[] {
const resources = new Map<unknown, {
url: string,
type: string,
Expand All @@ -889,7 +905,9 @@ export class ExtensionServer extends Common.ObjectWrapper.ObjectWrapper<EventTyp
uiSourceCodes.forEach(pushResourceData.bind(this));
for (const resourceTreeModel of SDK.TargetManager.TargetManager.instance().models(
SDK.ResourceTreeModel.ResourceTreeModel)) {
resourceTreeModel.forAllResources(pushResourceData.bind(this));
if (this.extensionAllowedOnTarget(resourceTreeModel.target(), port)) {
resourceTreeModel.forAllResources(pushResourceData.bind(this));
}
}

return [...resources.values()];
Expand All @@ -898,10 +916,7 @@ export class ExtensionServer extends Common.ObjectWrapper.ObjectWrapper<EventTyp
private async getResourceContent(
contentProvider: TextUtils.ContentProvider.ContentProvider, message: PrivateAPI.ExtensionServerRequestMessage,
port: MessagePort): Promise<void> {
const url = contentProvider.contentURL();
const origin = extensionOrigins.get(port);
const extension = origin && this.registeredExtensions.get(origin);
if (!extension?.isAllowedOnTarget(url)) {
if (!this.extensionAllowedOnURL(contentProvider.contentURL(), port)) {
this.dispatchCallback(message.requestId, port, this.status.E_FAILED('Permission denied'));
return undefined;
}
Expand Down Expand Up @@ -945,9 +960,7 @@ export class ExtensionServer extends Common.ObjectWrapper.ObjectWrapper<EventTyp
const response = error ? this.status.E_FAILED(error) : this.status.OK();
this.dispatchCallback(requestId, port, response);
}
const origin = extensionOrigins.get(port);
const extension = origin && this.registeredExtensions.get(origin);
if (!extension?.isAllowedOnTarget(url as Platform.DevToolsPath.UrlString)) {
if (!this.extensionAllowedOnURL(url as Platform.DevToolsPath.UrlString, port)) {
return this.status.E_FAILED('Permission denied');
}

Expand Down

0 comments on commit 33a09fb

Please sign in to comment.