Skip to content
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.

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

Manifest V3 migration #47

Closed
wants to merge 2 commits into from

Conversation

gantoine
Copy link

@gantoine gantoine commented Jul 29, 2023

QA

If you're interested in testing these changes but don't want to pull and build locally, you can use these builds:

  • chrome-2.0.0.zip: download and extract, then "Load unpacked" in chrome://extensions/
  • firefox-2.0.0.zip
    : download the zip file, then "Load temporary add-on" in about:debugging#/runtime/this-firefox

Manifest V3

With the switch to manifest V3, the spec for Chrome and Firefox splits; those differences need to be taken into consideration when building a manifest.json. Therefore, this PR takes the approach of splitting the manifest into 3 files: manifest.common.json, manifest.firefox.json and manifest.chrome.json. During the build process, the common file and one the browser files are merged using jq, and the output is saved as manifest.json (which is no longer tracked in git).

  • background is now split as Chrome uses a service worker to run the background script
  • Renamed browser_action to action
  • Shortcut text removed from default_title
  • browser_style set to false since now deprecated
  • browser_specific_settings.id required for Firefox (set to random UUIDv4)
  • Add storage permission as localStorage is deprecated in extension contexts

And one more thing: the build script now outputs both packaged and unpackaged builds to /artifacts/chrome and /artifacts/firefox.

What changed?

Granular permissions

By marking ["http://*/*", "https://*/*"] as optional_host_permissions, we can request only the permission(s) we need to connect to the Linkding instance. When first configuring the extension, you'll be prompted to grant permission to the page that hosts Linkding.

This has been reverted.

Firefox Chrome
Screenshot 2023-07-27 at 5 46 09 PM Screenshot 2023-07-27 at 6 29 21 PM

A note on Firefox: A green dot now appears under the badge on any website where access hasn't been explicitly granted. This seems to be due to the combination of the tabs permission and having https://*/* as an optional permission.

This entire refactor was started from a desire to lock down this extension's permissions, as seeing "Read your entire browsing history" on install is not great. That being said, if we feel that the green dot is distracting, I can always revert the permissions to their original version.

> "After installing an extension, if you navigate to a website that requires permissions for the extension to work, the extensions button will display a notification dot. If the extension is already pinned in the toolbar, the notification dot will appear below the extension icon."

This has been reverted.

https://support.mozilla.org/en-US/kb/extensions-button~

Green dot
Screenshot 2023-07-29 at 9 27 57 AM

localStorage.setItem(key, value);
return Promise.resolve();
}
return browserAPI.storage.local.set({ [key]: value });
Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Kind of a breaking change for FF users, as their options will be gone after the update. But looks like I did the same for Chrome users when switching to Manifest V3...

I guess it's alright, I'll try to mention it in the update notes.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we can do a 1-time upgrade for those FF users? Something like reading from localStorage and writing those values to storage, then clearing local storage.

build.sh Outdated
cp -r build icons options popup styles artifacts/firefox

# Build manifest file for Firefox
jq -s '.[0] * .[1]' manifests/manifest.COMMON.json manifests/manifest.FIREFOX.json > artifacts/firefox/manifest.json
Copy link
Owner

Choose a reason for hiding this comment

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

I have not used jq so far, others might not either. Rather than adding a new dependency, we could just add a small script to the manifests folder handle this.

/**
 * Merges the given manifests into a single manifest.
 */
const fs = require('fs');

const manifestFile1 = process.argv[2];
const manifestFile2 = process.argv[3];
const outputFile = process.argv[4];

const manifest1 = JSON.parse(fs.readFileSync(manifestFile1, 'utf8'));
const manifest2 = JSON.parse(fs.readFileSync(manifestFile2, 'utf8'));

const mergedManifest = {
  ...manifest1,
  ...manifest2,
}

fs.writeFileSync(outputFile, JSON.stringify(mergedManifest, null, 2));

Copy link
Author

Choose a reason for hiding this comment

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

I understand the concern, but jq allows for manipulation and merging of complex JSON objects, including matching nested fields. I'll switch to using node-jq so other contributors won't need to install jq globally, and we can add it as a dev dependency.

Comment on lines 3 to 4
"name": "Linkding",
"version": "2.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

No need to change the name or version at this point.

"default_icon": {
"19": "icons/button_19x19.png",
"32": "icons/button_32x32.png",
"38": "icons/button_38x38.png"
},
"default_title": "Add bookmark (Alt+Shift+L)",
"default_title": "Add bookmark",
Copy link
Owner

Choose a reason for hiding this comment

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

This was intentionally added together with the shortcut, and seems to be the only way the shortcut is surfaced in the extension. I'd prefer to keep this unless there is a very good reason.

Copy link
Author

Choose a reason for hiding this comment

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

I only removed it since none of the other extensions I use that have shortcuts, add the shortcut to their title. It's not a trend I'm familiar with when it comes to browser extensions. All the same, I agree that the shortcut isn't surfaced anywhere else at the moment, so I'll let you make the call on whether it should stay or go, and we can always revisit this later.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "linkding-extension",
"version": "1.7.0",
"version": "2.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Let's revert the version for now.

package.json Outdated
@@ -12,21 +12,22 @@
"url": "https://github.com/sissbruecker/linkding-extension/issues"
},
"homepage": "https://github.com/sissbruecker/linkding-extension#readme",
"type": "module",
Copy link
Owner

Choose a reason for hiding this comment

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

There isn't really a need to add this, the only code that is ever run is the rollup bundle, which doesn't contain imports. Let's remove it.

Copy link
Owner

Choose a reason for hiding this comment

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

Not having a manifest file in the project root is kind of inconvenient when developing / debugging the extension. My process during development so far was to just load the project root folder as temporary / local extension and it would just work.

Maybe the script suggested in the other comment could be used to generate a manifest in the project root before running rollup in the npm dev script. That would mean there would be two dev scripts:

  "dev-chrome": "node ./manifests/merge.js ./manifests/manifest.COMMON.json ./manifests/manifest.CHROME.json ./manifest.json && rollup -c -w",
  "dev-firefox": "node ./manifests/merge.js ./manifests/manifest.COMMON.json ./manifests/manifest.FIREFOX.json ./manifest.json && rollup -c -w",

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I totally agree, and I like your approach. I'll include something like this in the upcoming commit!

"strict_min_version": "109.0"
}
},
"optional_permissions": [
Copy link
Owner

Choose a reason for hiding this comment

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

While I appreciate the general notion to reduce the permissions, the way this currently works has a lot of potential for confusion:

  • The dot indicates to me that some action is needed
  • The first thing I would do is click on the extension icon. In the extension's popup there is no related information to the dot.
  • The dot disappears after you click the icon.
  • After right-clicking the icon, and then carefully processing the menu items there, I might understand that permissions were requested, and that clicking the extension icon gave those permissions, thus removing the dot.
  • Then having the dot show up again on every single page I visit sounds annoying

I would suggest to revert the permissions for now, and then maybe at some later point investigate further if this can be improved somehow.

Copy link
Author

Choose a reason for hiding this comment

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

Now that I've had time to review the code, I'm (personally) much more comfortable giving full permissions to the extension. Given the annoyance of the green dot I'll revert the permission changes. 👍🏼

Copy link
Owner

Choose a reason for hiding this comment

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

Testing this in Firefox, it still seems to treat the host_permissions as optional, which means the connection test request fails, and the dot still shows up. The docs kind of elude to this:

Most browsers treat host_permission as optional. If you request permissions using this key, users may get prompted to grant those permissions during installation. As of June 2023, Safari, Firefox, and some Chromium-based browsers don't prompt the user during installation.

What a weird way to go about it rather than just asking during installation. Also what is the point of optional host permissions?

Anyway, It seems you can request host permissions for the same wildcard URLs that are specified in the manifest, which then grants the extension to access all URLs:

    // Request host permissions
    const granted = await browserAPI.permissions.request({
      origins: [`https://*/*`, `http://*/*`],
    });

However I'm not sure if that is intentional or a bug, also haven't checked how Chrome handles this.

At this point I'm not sure if I want to continue migrating FF to v3, there really isn't a benefit regarding permissions for users so far, and it would require this weird approach above with the delayed permission grant.

Copy link
Author

@gantoine gantoine Jul 30, 2023

Choose a reason for hiding this comment

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

Oh the joys of self-hosting 😄

I'm having some issues building the extension for FF on master, it keeps complaining about the package being corrupted. Unfortunately I don't have much more time right now to dig into this, so I'll close this PR as well, and hopefully someone can pick up the work I've done in #46 and split it into their own PRs.

localStorage.setItem(key, value);
return Promise.resolve();
}
return browserAPI.storage.local.set({ [key]: value });
Copy link
Owner

Choose a reason for hiding this comment

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

Kind of a breaking change for FF users, as their options will be gone after the update. But looks like I did the same for Chrome users when switching to Manifest V3...

I guess it's alright, I'll try to mention it in the update notes.

package.json Outdated
"build": "rollup -c",
"dev": "rollup -c -w"
"generate": "./build.sh"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"generate": "./build.sh"
"package": "./build.sh"

Might be more indicative of what it does. Can also rename the shell script if we agree on this.

Copy link
Author

Choose a reason for hiding this comment

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

I like it, will rename both. 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants