[🐛 Bug]: install_addon() is removing the first letter of all the files of an extension when the path to an extension dir ends with a '/'

[jkbzh]

What happened?

The firefox webdriver provides an install_addon() call that lets you install a browser extension. You can call this extension by giving the path to either the extensions xpi file or the path to where the source code for the extension is found.

For the second case, install_addon() will go walk the directory where the extension is and create an xpi file. However, the function doesn't control that the path that is given ends with a '/'. The function works correctly when the path doesn't end with a '/'. But when you do give it a path that does end with a '/', install_addon() is over-compensating the trailing slash of the patch, which results in that all the files that it is adding to the zip file are missing their first letter.

https://github.com/SeleniumHQ/selenium/blob/83083c63593f245ed82cfd9e43a17654d5f73a43/py/selenium/webdriver/firefox/webdriver.py#L126C1-L134C68

L126 always return true if parameter "path" is a path, independently of it having a trailing end '/'
if os.path.isdir(path):

L128 is already compensating for the trailing slash that may be added by os_walk_path() in line 30... but note that no extra slash is added when the path itself ends with a slash.

        path_root = len(path) + 1  # account for trailing slash

L133 compensates again for the trailing slash as it doesn't take into account that the index for the first char in a string starts from 0, not 1
zipped.write(filename, filename[path_root:])

One solution would be to strip any trailing '/' that may in the parameter value:

      path_root = len(path.rstrip(os.sep)) + 1  # account for trailing slash

I let you adjust this to what is more pythonic and portable to you.

How can we reproduce the issue?

You can clone from https://github.com/mdn/webextensions-examples/ and call
with webdriver.Firefox(options=options) as driver:
    # install and init the extension
    addon_id = driver.install_addon(extension_path, temporary=True)

with extension_path pointing to the dir where the source code (and manifest for the extension exists).  For example: 

/webextensions-examples/tree/main/borderify/

If you pass the extension_path without a trailing '/', it works.

If you pass the extension_path with a trailing '/', the first character of each file will be missing in the generated xpi file.

Relevant log output

File "my_home/scripts/selenium_xpi_request.py", line 88, in <module>
    addon_id = driver.install_addon(extension_path, temporary=True)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "my_home/.virtualenvs/w3cpy3/lib/python3.11/site-packages/selenium/webdriver/firefox/webdriver.py", line 140, in install_addon
    return self.execute("INSTALL_ADDON", payload)["value"]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "my_home/.virtualenvs/w3cpy3/lib/python3.11/site-packages/selenium/webdriver/remote/webdriver.py", line 347, in execute
    self.error_handler.check_response(response)
  File "my_home/.virtualenvs/w3cpy3/lib/python3.11/site-packages/selenium/webdriver/remote/errorhandler.py", line 229, in check_response
    raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.WebDriverException: Message: Could not install add-on: /tmp/addon-9c494ff3-9c7e-4420-ac77-feb5ddb7e349.xpi: File /tmp/addon-9c494ff3-9c7e-4420-ac77-feb5ddb7e349.xpi does not contain a valid manifest
Stacktrace:
RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8
WebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:191:5
UnknownError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:800:5
install@chrome://remote/content/marionette/addon.sys.mjs:88:13

Operating System

Debian 12.5

Selenium version

Python 3.11.2

What are the browser(s) and version(s) where you see this issue?

Mozilla Firefox 123.0.1

What are the browser driver(s) and version(s) where you see this issue?

geckodriver 0.34.0 (c44f0d09630a 2024-01-02 15:36 +0000)

Are you using Selenium Grid?

No

[github-actions]

@jkbzh, thank you for creating this issue. We will troubleshoot it as soon as we can.

---

Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

[jkbzh]

Forgot to show how the generated .xpi file looks like. This is only an extract:

unzip -l /tmp/addon-9c494ff3-9c7e-4420-ac77-feb5ddb7e349.xpi
4080 2024-03-13 18:12 ackground.js
964 2024-03-13 18:05 anifest.json
778 2024-03-06 22:57 EADME.md
179 2024-03-06 22:57 cons/LICENSE
454 2024-03-13 18:06 ptions/options.html
986 2024-03-13 18:13 ptions/options.js

[diemol]

We have a test for this passing on our examples CI: https://github.com/SeleniumHQ/seleniumhq.github.io/blob/trunk/examples/python/tests/browsers/test_firefox.py#L91C40-L91C50, and https://github.com/SeleniumHQ/seleniumhq.github.io/blob/trunk/examples/python/tests/conftest.py#L97.

Please compare and let us know.

[jkbzh]

Hi @diemol . Thanks for your prompt answer.

We have a test for this passing on our examples CI: https://github.com/SeleniumHQ/seleniumhq.github.io/blob/trunk/examples/python/tests/browsers/test_firefox.py#L91C40-L91C50, and https://github.com/SeleniumHQ/seleniumhq.github.io/blob/trunk/examples/python/tests/conftest.py#L97.

I'll try to elaborate.

The "path" argument in selenium.webdriver.install_addon(self, path, temporary=False) can point to either an xpi file or to the directory where the source code for that extension is found (manifest.json, background.js, ...). If "path" points to an xpi file, install_addon() will read that file into memory; if it is a directory,install_addon()will create an in-memory xpi file containing the directory's content. This is taken care in the following block:

selenium/py/selenium/webdriver/firefox/webdriver.py

Lines 126 to 137 in 83083c6

	if os.path.isdir(path):
	fp = BytesIO()
	path_root = len(path) + 1 # account for trailing slash
	with zipfile.ZipFile(fp, "w", zipfile.ZIP_DEFLATED) as zipped:
	for base, _, files in os.walk(path):
	for fyle in files:
	filename = os.path.join(base, fyle)
	zipped.write(filename, filename[path_root:])
	addon = base64.b64encode(fp.getvalue()).decode("UTF-8")
	else:
	with open(path, "rb") as file:
	addon = base64.b64encode(file.read()).decode("UTF-8")

Those two tests you point out test that install_addon() works when the "path" argument points to an xpi file (test/webextensions-selenium-example.xpi). However, you are missing tests for the second case, when "path" points to the directory of the source code of an extension.

Assuming that the code for webextensions-selenium-example is available under tests/extensions/webextensions-selenium-example/, you would need to add these two tests to conftest.py, where "path" points to the extension source code directory. One test when path has a trailing slash, and another one where there's no trailing slash. The first test is the one that reveals the issue I reported. The second test works.

(code here below is just a test proposal, but hasn't been run and may need to be adapted to your test suite)

@pytest.fixture(scope='function')
def addon_path_dir_with_slash():
    if os.path.abspath("").endswith("tests"):
        path = os.path.abspath("extensions/webextensions-selenium-example/")
    else:
        path = os.path.abspath("tests/extensions/webextensions-selenium-example/")


    yield path

@pytest.fixture(scope='function')
def addon_path_dir_without_slash():
    if os.path.abspath("").endswith("tests"):
        path = os.path.abspath("extensions/webextensions-selenium-example")
    else:
        path = os.path.abspath("tests/extensions/webextensions-selenium-example")


    yield path

Hope this explains the issue and the two missing tests.

[AutomatedTester]

Do you want to raise a PR and maybe fix the issue? You’ve done so much work already you should get some kudos for it

…

On 14 Mar 2024 at 12:53 +0000, jkbzh ***@***.***>, wrote: Hi @diemol . Thanks for your prompt answer. > We have a test for this passing on our examples CI: https://github.com/SeleniumHQ/seleniumhq.github.io/blob/trunk/examples/python/tests/browsers/test_firefox.py#L91C40-L91C50, and https://github.com/SeleniumHQ/seleniumhq.github.io/blob/trunk/examples/python/tests/conftest.py#L97. I'll try to elaborate. The "path" argument in selenium.webdriver.install_addon(self, path, temporary=False) can point to either an xpi file or to the directory where the source code for that extension is found (manifest.json, background.js, ...). If "path" points to an xpi file, install_addon() will read that file into memory; if it is a directory,install_addon()will create an in-memory xpi file containing the directory's content. This is taken care in the following block: https://github.com/SeleniumHQ/selenium/blob/83083c63593f245ed82cfd9e43a17654d5f73a43/py/selenium/webdriver/firefox/webdriver.py#L126-L137 Those two tests you point out test that install_addon() works when the "path" argument points to an xpi file (test/webextensions-selenium-example.xpi). However, you are missing tests for the second case, when "path" points to the directory of the source code of an extension. Assuming that the code for webextensions-selenium-example is available under tests/extensions/webextensions-selenium-example/, you would need to add these two tests to conftest.py, where "path" points to the extension source code directory. One test when path has a trailing slash, and another one where there's no trailing slash. The first test is the one that reveals the issue I reported. The second test works. (code here below is just a test proposal, but hasn't been run) @pytest.fixture(scope='function') def addon_path(): if os.path.abspath("").endswith("tests"): path = os.path.abspath("extensions/webextensions-selenium-example/") else: path = os.path.abspath("tests/extensions/webextensions-selenium-example/") yield path @pytest.fixture(scope='function') def addon_path(): if os.path.abspath("").endswith("tests"): path = os.path.abspath("extensions/webextensions-selenium-example") else: path = os.path.abspath("tests/extensions/webextensions-selenium-example") yield path Hope this explains the issue and the two missing tests. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[jkbzh]

Do you want to raise a PR and maybe fix the issue? You’ve done so much work already you should get some kudos for it
@AutomatedTester, sure. I hope I sent the PR correctly. I need some guidance to fix the related test and add a second one.

[jkbzh]

We have a test for this passing on our examples CI: https://github.com/SeleniumHQ/seleniumhq.github.io/blob/trunk/examples/python/tests/browsers/test_firefox.py#L91C40-L91C50, and https://github.com/SeleniumHQ/seleniumhq.github.io/blob/trunk/examples/python/tests/conftest.py#L97.

Please compare and let us know.

Hi! I read again through the tests and found out that one was broken (wrong fixture) and that a test was missing for the issue I am reporting in this ticket. I sent a PR with suggestions on how to fix them but can't run the test suite myself. Hope it helps.