code.django

code_django.py

Hi Tim


action 1 : https://code.djangoproject.com/github/login


https://github.com/trac-hacks/trac-github/blob/master/tracext/github.py#L64&L69

    def _do_login(self, req):
        oauth = self._oauth_session(req)
        authorization_url, state = oauth.authorization_url(
            'https://github.com/login/oauth/authorize')
        req.session['oauth_state'] = state ## store state parameter on session
        req.redirect(authorization_url)


from action 1 to action 2 : https://github.com/login/oauth/authorize


  https://github.com/trac-hacks/trac-github/blob/master/tracext/github.py#L71&L93
  
    def _do_oauth(self, req):
         ## you see any check valid state 
        
        if req.session['oauth_state'] == req.args.get('state'): # i think in  req object there is state parameter, 
          
                ## Now ....
         
        oauth = self._oauth_session(req)
        authorization_response = req.abs_href(req.path_info) + '?' + req.query_string
        client_secret = self._client_config('secret')
        import oauthlib
        try:
            oauth.fetch_token(
                'https://github.com/login/oauth/access_token',
                authorization_response=authorization_response,
                client_secret=client_secret)
        except oauthlib.oauth2.MissingTokenError, e:
            self.log.warn(e)
            add_warning(req, _("Invalid request. Please try to login again."))
            self._redirect_back(req)                                            ## _redirect_back but req.session['oauth_state'] is valid

        user = oauth.get('https://api.github.com/user').json()
        # Small hack to pass the username to _do_login.
        req.environ['REMOTE_USER'] = user['login']
        # Save other available values in the session.
        req.session.setdefault('name', user.get('name') or '')
        req.session.setdefault('email', user.get('email') or '')

        return super(GitHubLoginModule, self)._do_login(req)



action 3 :  https://code.djangoproject.com/github/logout

you see not delete req.session['oauth_state'] after logout, this means always req.session['oauth_state'] is True and my request 
https://github.com/login/oauth/authorize?response_type=code&client_id=c001736dc7ada934dcd7&redirect_uri=https://code.djangoproject.com%2Fgithub%2Foauth&scope=&state=

redirect to root page code.djangoproject.com, without check session state parameter,


also i can not find this method on github.py `_do_logout`

     elif req.path_info.startswith('/github/logout'):
            self._do_logout(req)
##################################################################################
################################## Code By Tim Graham, ########################### 
##################################################################################
diff --git a/README.md b/README.md
index dc30905..23af0b8 100644
--- a/README.md
+++ b/README.md
@@ -339,6 +339,10 @@ for git repositories. If you have an idea to fix it, please submit a patch!
 Changelog
 ---------
 
+### 2.1.6
+
+* CSRF security fix: add verification of OAuth state parameter.
+
 ### 2.1.5
 
 * Support reading the GitHub OAuth secret from a file.
diff --git a/runtests.py b/runtests.py
index 4c8d361..6e00a31 100755
--- a/runtests.py
+++ b/runtests.py
@@ -271,6 +271,23 @@ class GitHubBrowserTests(TracGitHubTests):
         self.assertFalse('msg 6' in html)
 
 
+class GitHubLoginModuleTests(TracGitHubTests):
+
+    def testOauthInvalidState(self):
+        with self.assertRaises(urllib2.HTTPError) as exc:
+            urllib2.urlopen(URL + 'github/oauth?state=abc').read()
+
+        self.assertEqual(exc.exception.code, 302)
+        self.assertEqual(exc.exception.headers['Location'], URL[:-1])
+
+    def testOauthLogout(self):
+        with self.assertRaises(urllib2.HTTPError) as exc:
+            urllib2.urlopen(URL + 'github/logout').read()
+
+        self.assertEqual(exc.exception.code, 302)
+        self.assertEqual(exc.exception.headers['Location'], URL[:-1])
+
+
 class GitHubPostCommitHookTests(TracGitHubTests):
 
     def testDefaultRepository(self):
diff --git a/tracext/github.py b/tracext/github.py
index 4270bed..b541298 100644
--- a/tracext/github.py
+++ b/tracext/github.py
@@ -69,6 +69,11 @@ class GitHubLoginModule(LoginModule):
         req.redirect(authorization_url)
 
     def _do_oauth(self, req):
+        session_oauth_state = req.session.get('oauth_state')
+        get_state = req.args.get('state')
+        if session_oauth_state != get_state:
+            self._reject(req, 'oauth state mismatch %s != %s' % (session_oauth_state, get_state))
+
         oauth = self._oauth_session(req)
         authorization_response = req.abs_href(req.path_info) + '?' + req.query_string
         client_secret = self._client_config('secret')
@@ -79,9 +84,7 @@ class GitHubLoginModule(LoginModule):
                 authorization_response=authorization_response,
                 client_secret=client_secret)
         except oauthlib.oauth2.MissingTokenError, e:
-            self.log.warn(e)
-            add_warning(req, _("Invalid request. Please try to login again."))
-            self._redirect_back(req)
+            self._reject(req, e)
 
         user = oauth.get('https://api.github.com/user').json()
         # Small hack to pass the username to _do_login.
@@ -92,6 +95,15 @@ class GitHubLoginModule(LoginModule):
 
         return super(GitHubLoginModule, self)._do_login(req)
 
+    def _do_logout(self, req):
+        req.session.pop('oauth_state', None)
+        super(GitHubLoginModule, self)._do_logout(req)
+
+    def _reject(self, req, e):
+        self.log.warn(e)
+        add_warning(req, _("Invalid request. Please try to login again."))
+        self._redirect_back(req)
+
     def _oauth_session(self, req):
         client_id = self._client_config('id')
         redirect_uri = req.abs_href.github('oauth')