Skip to content

fix: Fixed Chrome Not Opening Extensions Pages provided in URL #2321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kodergeek wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: Fixed Chrome Not Opening Extensions Pages provided in URL #2321

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a163a3c
bug-1979 partial fix
kodergeek Sep 22, 2021
2f86d4a
fix: Fixed Chrome Not Opening Extensions Pages provided in URL
kodergeek Sep 28, 2021
d485a71
Remove unused ws message handling from bg.js
kodergeek Sep 28, 2021
8ca4550
Fixed PR issues and added unit test for special urls
kodergeek Oct 3, 2021
7b56d10
Remove Code duplication issue and fix unit tests
kodergeek Oct 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions src/extension-runners/chromium.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,28 @@ export class ChromiumExtensionRunner {
});
});

const chromeFlags = [...DEFAULT_CHROME_FLAGS];
let startingUrl;
var specialStartingUrls = '';
if (this.params.startUrl) {
const startingUrls = Array.isArray(this.params.startUrl) ?
this.params.startUrl : [this.params.startUrl];

// Remove URLs starting with chrome:// from startingUrls and let bg.js open them instead
for (let i = 0; i < startingUrls.length; i++) {
if (startingUrls[i].toLowerCase().startsWith('chrome://')) {
specialStartingUrls += startingUrls[i] + " "

@Rob--W Rob--W Sep 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The URLs should be appended to an array, not to a string.

@kodergeek kodergeek Oct 3, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I had a hard time with template literals and arrays that's why I used string with spaces, but JSON.stringify() solves both this problem and potential code injection issue you mentioned nicely!

startingUrls.splice(i, 1)

@Rob--W Rob--W Sep 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By removing the element during the iteration, you'll end up skipping URLs.

For example:

[
  "chrome://extensions",
  "chrome://version"
]

At i=0, "chrome://extensions" would be appended to the list of start URLs.
But due to the startingUrls.splice(i, 1) call, the array will be ["chrome://version"].
At the next iteration, i=1, which would result in "chrome://version" to be skipped.

To fix this, you could iterate over the URLs in reverse order and prepend the URLs to the specialStartingUrls list. Then .splice(i, 1) wouldn't be an issue any more.

}
}

startingUrl = startingUrls.shift();

chromeFlags.push(...startingUrls);
}

// Create the extension that will manage the addon reloads
this.reloadManagerExtension = await this.createReloadManagerExtension();
this.reloadManagerExtension = await this.createReloadManagerExtension(specialStartingUrls);

// Start chrome pointing it to a given profile dir
const extensions = [this.reloadManagerExtension].concat(
Expand All @@ -166,8 +186,6 @@ export class ChromiumExtensionRunner {
log.debug(`(chromiumBinary: ${chromiumBinary})`);
}

const chromeFlags = [...DEFAULT_CHROME_FLAGS];

chromeFlags.push(`--load-extension=${extensions}`);

if (this.params.args) {
Expand Down Expand Up @@ -213,13 +231,6 @@ export class ChromiumExtensionRunner {
chromeFlags.push(`--profile-directory=${profileDirName}`);
}

let startingUrl;
if (this.params.startUrl) {
const startingUrls = Array.isArray(this.params.startUrl) ?
this.params.startUrl : [this.params.startUrl];
startingUrl = startingUrls.shift();
chromeFlags.push(...startingUrls);
}

this.chromiumInstance = await this.chromiumLaunch({
enableExtensions: true,
Expand Down Expand Up @@ -280,7 +291,7 @@ export class ChromiumExtensionRunner {
});
}

async createReloadManagerExtension(): Promise<string> {
async createReloadManagerExtension(specialStartingUrls: string): Promise<string> {
const tmpDir = new TempDir();
await tmpDir.create();
this.registerCleanup(() => tmpDir.remove());
Expand Down Expand Up @@ -334,6 +345,19 @@ export class ChromiumExtensionRunner {
const ws = new window.WebSocket(
"ws://${wssInfo.address}:${wssInfo.port}");


if (${specialStartingUrls.length} > 0)
{
const chromeTabList = "${specialStartingUrls}".trim().split(" ")

@Rob--W Rob--W Sep 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serialize the list of URLs with ${JSON.stringify(specialStartingUrls)} instead of splitting by strings. Otherwise, if the URL contains a ", this could be abused to execute code in the context of the helper extension. That's problematic.

chrome.runtime.onInstalled.addListener(details => {
if (details.reason === chrome.runtime.OnInstalledReason.INSTALL ) {
chromeTabList.forEach(url => {
chrome.tabs.create({ url });
});
}

@Rob--W Rob--W Sep 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace is off, please fix this.

});
}

ws.onmessage = async (evt) => {
const msg = JSON.parse(evt.data);
if (msg.type === 'webExtReloadAllExtensions') {
Expand Down