[Security] Approved hosts mechanism (GH-227 follow up) #237

Closed deansheather closed 5 years ago

Now that GH-227 has been merged, we need to implement some way of allowing the open in sail button to be injected into third-party GitHub enterprise or GitLab self-hosted instances.

Kyle and I had a meeting to discuss this, and the way we decided on was allowing the button to be injected into any site that appears to be GitHub or GitLab like we used to. When the user clicks on the button, the content script will send the "open in sail" request to the background script which will check the approved hosts list (stored in extension storage as approved_hosts). If the host is in the list, the request will be accepted, otherwise a popup will appear asking the user if they want to allow this site to access sail.

This solves the security issue, as any malicious clicks on a sail link would cause a popup to appear (unless the user has trusted that domain already).

The approved hosts list would allow for some sort of wildcard matching (.example.com would match *.example.com and example.com, for example). A configuration page for the extension would be required to remove hosts from the list or manually add them.

We considered storing the approved hosts list in the sail config file (which already exists), but it would be complex for sail to automatically update the approved hosts list if we wanted to add a popup. The extension method seems like a better way (in our opinion) from a UX perspective, and from an ease-of-implementation perspective.

CC: @lucacasonato, @roberthmiller, @kylecarbs

Interested on hearing any thoughts or concerns about this from the people who were originally involved in the discovery/fix before I begin working on it tomorrow.

I like this idea. Having the user add hosts via the popup seems like a good idea.

The idea seems sound. I am not sure that storing the hosts within the extension is fine, but if not, we can cross that bridge when we come to it.

The extension is an isolated environment so storing confidential data there is fine. That is why browser password managers work.

On Wed, Jul 17, 2019 at 5:47 PM Robert M notifications@github.com wrote:

The idea seems sound. I am not sure that storing the hosts within the extension is fine, but if not, we can cross that bridge when we come to it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cdr/sail/issues/237?email_source=notifications&email_token=AB3XNVLOZVZMLVUHZFDSD5TP745JLA5CNFSM4IERQFB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2E2WZA#issuecomment-512338788, or mute the thread https://github.com/notifications/unsubscribe-auth/AB3XNVK7ZQWGO4LBHBNTHULP745JLANCNFSM4IERQFBQ .

Had a feeling, but I never have worked in a chrome/browser environment.

coder / sail

[Security] Approved hosts mechanism (GH-227 follow up) #237