search

dwyl / elixir-auth-github

:octocat: Minimalist GitHub OAuth Authentication for Elixir Apps. Tested, Documented & Maintained. Setup in 5 mins. 🚀
GNU General Public License v2.0
38 stars 4 forks source link

Valid Scopes #36

Closed nelsonic closed 4 years ago

nelsonic commented 4 years ago

At present we have a lot of items in the list of @valid_scopes: https://github.com/dwyl/elixir-auth-github/blob/44021233add025348f261f5bf0b9b32fe6c5711a/lib/elixir_auth_github.ex#L51-L58

This feels like it was copy-pasted from https://developer.github.com/apps/building-oauth-apps/understanding-scopes-for-oauth-apps/#available-scopes without much thought given to why we would want to allow all these scopes in our App(s). (Note: this is not intended as criticism, this package was built to ship as fast as possible. Most devs don't have a security-first mindset, we need to change that. Hence: https://github.com/dwyl/learn-security)

I'm especially concerned with "delete_repo", "write:public_key", "admin:public_key", "write:gpg_key" and "admin:gpg_key"

The ability to write:public_key would allow a malicious attacker to overwrite someone's RSA key for their GitHub account which would allow them to inflict serious damage on their account. (think re-writing commit history to introduce security vulnerabilities into master of popular published packages...) I certainly could not afford for this to happen to me given that I maintain auth packages used by thousands of Apps.

We are only using two scopes in the application that uses this package: "user:email" and "read:org" https://github.com/dwyl/library/blob/221152bfd4185e5c83747f591b1c66d6a75ce183/lib/library_web/controllers/login_controller.ex#L17 These scopes are appropriate for the Library App as we want to know if the person is a member of the @dwyl org. ✅

I do not foresee us ever using the Admin scopes in any @dwyl App. Therefore I think we should remove them from our auth package.

Proposed Amendment

In the interest of avoiding anyone using this package for any nefarious ends, I propose that we remove these "admin" and security scopes until someone specifically requests them providing a really good use case.

Remove:

Note: I know that removing these scopes is a "Breaking Change" that could potentially break someone's App if they were to update their dependency on this package. 💭 It's difficult to determine from download stats https://hex.pm/packages/elixir_auth_github how many people/projects outside of @dwyl are using the package: image 319 downloads for the latest version certainly would imply that people outside of @dwyl have at least attempted to use the package. 🎉 However given that all the Stargazers are members of @dwyl image and only 3 people (Zooey, Finn and me) have ever opened an issue on or contributed to this repo, I feel it's quite "safe" to make this change.

nelsonic commented 4 years ago

On line 99 we are merely joining the scopes supplied to login_url_with_scope/1 with a "%20": https://github.com/dwyl/elixir-auth-github/blob/44021233add025348f261f5bf0b9b32fe6c5711a/lib/elixir_auth_github.ex#L92-L104

This means that the people consuming this package can still easily add their own scopes (_even new ones that are not listed in @valid_scopes because they were by GitHub added recently_) by just appending them to the end of the url returned by login_url/1

url <> "&scope=#{Enum.join(["admin:org", "delete_repo", "etc"], "%20")}"

Yes, this is an extra step. But it means that people are intentional about the scopes they are adding and: a) we won't need to update the list of @valid_scopes each time GitHub releases a new feature. e.g workflow actions b) we (@dwyl) are not arbitrarily restricting anyone from building Apps with legit "admin" use case.

nelsonic commented 4 years ago

@valid_scopes list removed as part of removing the filter_valid_scopes/1 #37