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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for GitHub Enterprise instances #26

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

Conversation

outadoc
Copy link

@outadoc outadoc commented Mar 5, 2021

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.

Screen Shot 2021-03-05 at 14 23 23

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.

@hediet
Copy link
Owner

hediet commented Mar 5, 2021

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 ;)

@outadoc
Copy link
Author

outadoc commented Mar 5, 2021

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 馃檪

@hediet
Copy link
Owner

hediet commented Mar 20, 2021

Sorry for the delay, I had to learn for exams.
Are you sure this is not required to make it work?

It is very unfortunate that I cannot test this feature, as I don't have access to a github enterprise instance.

@outadoc
Copy link
Author

outadoc commented Mar 20, 2021

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 馃憤

@hediet hediet force-pushed the master branch 9 times, most recently from 518ff83 to d742493 Compare March 20, 2021 19:36
@hediet
Copy link
Owner

hediet commented Apr 1, 2021

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!

@hediet hediet closed this Apr 1, 2021
@fregante
Copy link

fregante commented Mar 9, 2023

when this extension requested for more permissions in the last update

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

@fregante fregante mentioned this pull request Mar 9, 2023
@hediet hediet reopened this Mar 13, 2023
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.

None yet

3 participants