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

kodergeek · 2021-09-22T17:27:39Z

No description provided.

Rob--W · 2021-09-23T11:03:07Z

Do you need anything else from me to proceed? I posted a very brief comment in reply to your question at #1979 (comment). It does answer your question, but I can imagine if some more detail is needed to understand my reply.

kodergeek · 2021-09-23T12:15:10Z

Yes, I appreciate if you can point in code where I should add the call so that bg.js extension can successfully open the extensions tab as you described in your comment. I couldn't figure it out and I tried in a few different places in code to no avail.

Rob--W · 2021-09-23T14:16:44Z

If your goal is to use the WebSocket to transmit the message, then you'll have to send a message once the manager extension has connected.

Here is an example of a snippet that connects: https://github.com/mozilla/web-ext/blob/6.4.0/src/extension-runners/chromium.js#L147

You would have to send the message once (not on every connect), to prevent the extension from opening tabs again when the WebSocket connection is temporarily interrupted for some reason.

For the current requirements, a simpler alternative is to hard-code the startup URL in the source of the manager extension. After all, the URLs are known when the extension is generated, and they need to be opened only once. To implement this approach, you'd need to pass the list of URLs to https://github.com/mozilla/web-ext/blob/6.4.0/src/extension-runners/chromium.js#L154 and write it to the generated bg.js

To make sure that the tabs are opened only once, use chrome.runtime.onInstalled and open the tabs if details.reason === "install".

In your current patch you're also filtering on chrome://extensions specificially. I suggest to just do this for any chrome: URL. E.g. chrome://version would have the same issue.

kodergeek · 2021-09-28T17:58:28Z

I have now updated the PR with the second approach as suggested by you, eg hard-coding the startup URL in the source of the manager extension.

I have also tested it with some different URL combinations and it seems to be working. Please check it out and let me know if there's anything that should be improved.

Also, there are 4 failing checks for my PR which I have no idea what they relate to. So if you think the fix is good to be merged I'd appreciate if you could advise me how to fix the failing checks :)

Rob--W · 2021-09-30T13:11:59Z

The test failures are linting issues. Run eslint to see the exact errors.

This is the output of circleci:

/home/circleci/web-ext/src/extension-runners/chromium.js
  163:34  error  Unexpected string concatenation                      prefer-template
  163:52  error  Strings must use singlequote                         quotes
  163:55  error  Missing semicolon                                    semi
  164:36  error  Missing semicolon                                    semi
  174:1   error  This line has a length of 95. Maximum allowed is 80  max-len
  294:1   error  This line has a length of 84. Maximum allowed is 80  max-len

✖ 6 problems (6 errors, 0 warnings)
  4 errors and 0 warnings potentially fixable with the `--fix` option.

Rob--W

Please address the implementation issues below (and also check that eslint passes), and add a unit test that verifies the expected parameters and opened tabs in https://github.com/mozilla/web-ext/blob/master/tests/unit/test-extension-runners/test.chromium.js.

You'd have to verify that the expected args are present, and that the special URLs are in the generated helper extension.

Rob--W · 2021-09-30T13:13:33Z

src/extension-runners/chromium.js

+      // 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] + " "


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

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!

Rob--W · 2021-09-30T13:18:32Z

src/extension-runners/chromium.js

+      for (let i = 0; i < startingUrls.length; i++) {
+        if (startingUrls[i].toLowerCase().startsWith('chrome://')) {
+          specialStartingUrls += startingUrls[i] + " "
+          startingUrls.splice(i, 1)


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.

Rob--W · 2021-09-30T13:20:08Z

src/extension-runners/chromium.js

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


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.

Rob--W · 2021-09-30T13:20:56Z

src/extension-runners/chromium.js

+            chromeTabList.forEach(url => {
+              chrome.tabs.create({ url });
+            });
+            }           


Whitespace is off, please fix this.

codecov · 2021-10-03T10:00:21Z

Codecov Report

Merging #2321 (8ca4550) into master (1a0c926) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 8ca4550 differs from pull request most recent head 7b56d10. Consider uploading reports for the commit 7b56d10 to get more accurate results

@@           Coverage Diff           @@
##           master    #2321   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files          32       32           
  Lines        1699     1707    +8     
=======================================
+ Hits         1697     1705    +8     
  Misses          2        2

Impacted Files	Coverage Δ
src/extension-runners/chromium.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a0c926...7b56d10. Read the comment docs.

kodergeek · 2021-10-03T10:14:18Z

Thanks for your feedback @Rob--W. I have now resolved the issues you mentioned and have also added a unit test to both check for presence of special URLs in generated script, and also lack of them in the args list sent to chrome.

One issue I encountered during running existing unit tests was with the ordering of arguments being pushed to chromeFlags array. I had to move some code up to be able to check starting URLs and send them to createReloadManagerExtension because createReloadManagerExtension is now expecting the special arguments list, and it resulted in reordering of chromeFlags array because startingUrls was pushed first in chromeFlags array, and it resulted in 2 unit tests failing. So I have rearranged the code slightly and it has caused a slight code duplication at here and here. But no unit tests are failing as a result.

Rob--W · 2021-10-05T11:25:00Z

The code duplication is really unfortunate. Can you think of a way to implement the functionality without duplication?

kodergeek · 2021-10-09T14:58:29Z

Well yes the easiest way to remove code duplication and keeping the functionality is to make a small change to 2 unit tests by shifting around the list of arguments that are being asserted. I have updated the PR with the suggested fix. Is that something that is acceptable or I should not change the existing unit tests at all?

kodergeek · 2021-11-02T06:32:02Z

Hi @Rob--W Just wondering if you've had time to review this?

Rob--W · 2021-11-11T15:24:31Z

Hi @Rob--W Just wondering if you've had time to review this?

Unfortunately I did not. I'll try to get back to you next week.

dleetr · 2023-06-06T13:05:19Z

This PR seems to be abandoned. I need the functionality, so I have continued it in the link below, preserving the original work here and adding a small enhancement to it.
#2774

bug-1979 partial fix

a163a3c

kodergeek mentioned this pull request Sep 22, 2021

Unable to open the Extensions page with --start-url when running in Chrome #1979

Open

fix: Fixed Chrome Not Opening Extensions Pages provided in URL

2f86d4a

Remove unused ws message handling from bg.js

d485a71

Rob--W requested changes Sep 30, 2021

View reviewed changes

Fixed PR issues and added unit test for special urls

8ca4550

Remove Code duplication issue and fix unit tests

7b56d10

dleetr mentioned this pull request Jun 3, 2023

fix: Chrome not opening special chrome URLs or extension specific pages #2774

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kodergeek commented Sep 22, 2021

Rob--W commented Sep 23, 2021

kodergeek commented Sep 23, 2021

Rob--W commented Sep 23, 2021

kodergeek commented Sep 28, 2021

Rob--W commented Sep 30, 2021

Rob--W left a comment

Rob--W Sep 30, 2021

kodergeek Oct 3, 2021

Rob--W Sep 30, 2021

Rob--W Sep 30, 2021

Rob--W Sep 30, 2021

codecov bot commented Oct 3, 2021 •

edited

Loading

kodergeek commented Oct 3, 2021

Rob--W commented Oct 5, 2021

kodergeek commented Oct 9, 2021

kodergeek commented Nov 2, 2021

Rob--W commented Nov 11, 2021

dleetr commented Jun 6, 2023

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

Are you sure you want to change the base?

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

Conversation

kodergeek commented Sep 22, 2021

Rob--W commented Sep 23, 2021

kodergeek commented Sep 23, 2021

Rob--W commented Sep 23, 2021

kodergeek commented Sep 28, 2021

Rob--W commented Sep 30, 2021

Rob--W left a comment

Choose a reason for hiding this comment

Rob--W Sep 30, 2021

Choose a reason for hiding this comment

kodergeek Oct 3, 2021

Choose a reason for hiding this comment

Rob--W Sep 30, 2021

Choose a reason for hiding this comment

Rob--W Sep 30, 2021

Choose a reason for hiding this comment

Rob--W Sep 30, 2021

Choose a reason for hiding this comment

codecov bot commented Oct 3, 2021 • edited Loading

Codecov Report

kodergeek commented Oct 3, 2021

Rob--W commented Oct 5, 2021

kodergeek commented Oct 9, 2021

kodergeek commented Nov 2, 2021

Rob--W commented Nov 11, 2021

dleetr commented Jun 6, 2023

codecov bot commented Oct 3, 2021 •

edited

Loading