-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: add support for GitHub Enterprise instances #26
base: master
Are you sure you want to change the base?
Conversation
982fbb5
to
5aad5c3
Compare
Thanks a lot, I will have a closer look soon! Looks good in general. However, I think it might be a good idea to lock the version of the two new dependencies. I doubt they ever have to be updated automatically. Also, can you (and maybe also someone else) have a quick glimpse at the source code of these new dependencies that npm installed as well as their dependencies? I would like to avoid any security holes under any circumstances ;) |
Thanks! I've locked the versions and taken a look at the dependencies' code. They seem legit and I didn't notice anything fishy, but I'm not fluent in JS/TS, so take that with a grain of salt 🙂 |
Sorry for the delay, I had to learn for exams. It is very unfortunate that I cannot test this feature, as I don't have access to a github enterprise instance. |
Hi, no problem. 🙂 I believe it is; by default, when you programmatically whitelist a new allowed domain, your content scripts won't be run when you visit it. You need to explicitly register them for the new domain, which is what this package does (while providing a polyfill for Chrome). If you need me to test a change, I can probably find some time to do so 👍 |
518ff83
to
d742493
Compare
Given that a significant amount of users (~15%) uninstalled this extension when this extension requested for more permissions in the last update, I think it is better to create a separate extension that enables GitHub enterprise support. Thanks for you effort! |
Note that this setup does not require further permissions; "contextMenus" "activeTab" do not trigger a popup. You can follow the guide at https://github.com/fregante/webext-dynamic-content-scripts/blob/main/how-to-add-github-enterprise-support-to-web-extensions.md |
This PR aims to add GHE support to this cool extension 🎉
It turns out other extensions like Refined Github use very cool packages to do this: https://github.com/fregante/webext-domain-permission-toggle and https://github.com/fregante/webext-dynamic-content-scripts.
They add a browser action that the user can right-click on to enable running content scripts on a specific domain, and automatically request the right permissions for it.
This is otherwise easily doable in Firefox, but Chrome lacks support for the required contentScripts.register API, so this polyfill is especially nice.
I've tested it a bit on the latest versions of Firefox and Chrome with no visible issues, feel free to test some more.