From 0c225b00cc132072981903d16db1c0fb788a8b86 Mon Sep 17 00:00:00 2001 From: junkafarian Date: Mon, 2 Dec 2013 17:42:05 +0000 Subject: [PATCH 1/6] added support for authenticating updates from github --- cinch/github.py | 8 ++++---- setup_env.sample.sh | 4 ++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/cinch/github.py b/cinch/github.py index 51dfbda..d54f607 100644 --- a/cinch/github.py +++ b/cinch/github.py @@ -2,7 +2,7 @@ import json import logging -from flask import request +from flask import abort, request from github import Github, UnknownObjectException from cinch import app, models @@ -55,8 +55,6 @@ def _handle_pull_request(self, pull_request_data): pull.head_commit = commit.sha - - models.db.session.commit() def _handle_master_update(self): @@ -163,7 +161,9 @@ def update_pull_data(self, pull, data): def accept_github_update(): """ View for github web hooks to handle updates """ - # TODO: verify request is from github + update_secret = app.config.get('GITHUB_UPDATE_SECRET') + if update_secret and request.args.get('secret') != update_secret: + abort(401) github_token = app.config.get('GITHUB_TOKEN') gh = Github(github_token) diff --git a/setup_env.sample.sh b/setup_env.sample.sh index 64c1da3..de03a42 100644 --- a/setup_env.sample.sh +++ b/setup_env.sample.sh @@ -20,3 +20,7 @@ export CINCH_SENTRY_DSN= # A sqlalchemy parseable database identifier export CINCH_DB_URI= + +# For authenticating updates sent from github. +# This must be included in the callback url under the param `secret` +export CINCH_GITHUB_UPDATE_SECRET= From a4a69e4b25746ecbe1364d22e6a65fcec77bf694 Mon Sep 17 00:00:00 2001 From: junkafarian Date: Mon, 2 Dec 2013 18:51:49 +0000 Subject: [PATCH 2/6] updated token comparison to use werkzeug.security.safe_str_cmp --- cinch/github.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cinch/github.py b/cinch/github.py index d54f607..fbad498 100644 --- a/cinch/github.py +++ b/cinch/github.py @@ -4,6 +4,7 @@ import logging from flask import abort, request from github import Github, UnknownObjectException +from werkzeug.security import safe_str_cmp from cinch import app, models from cinch.check import check, CheckStatus @@ -162,7 +163,10 @@ def accept_github_update(): """ View for github web hooks to handle updates """ update_secret = app.config.get('GITHUB_UPDATE_SECRET') - if update_secret and request.args.get('secret') != update_secret: + if ( + update_secret and + not safe_str_cmp(request.args.get('secret', ''), update_secret) + ): abort(401) github_token = app.config.get('GITHUB_TOKEN') From 760ddde24e87129247ec9fc49bd82dbbac87019e Mon Sep 17 00:00:00 2001 From: junkafarian Date: Mon, 17 Mar 2014 17:20:12 +0000 Subject: [PATCH 3/6] Added tests for expected github update auth behaviour On branch update-secret Your branch is up-to-date with 'origin/update-secret'. Changes to be committed: (use "git reset HEAD ..." to unstage) modified: tests/conftest.py new file: tests/test_github.py --- tests/conftest.py | 15 +++++++++++++-- tests/test_github.py | 23 +++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 tests/test_github.py diff --git a/tests/conftest.py b/tests/conftest.py index ea9001c..867c38f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,5 @@ import os -from flask import Flask import pytest @@ -13,6 +12,10 @@ def pytest_configure(config): if db_uri: os.environ['DB_URI'] = db_uri + # disable error catching for better traceback + from cinch import app + app.testing = True + @pytest.fixture def session(): @@ -30,6 +33,14 @@ def drop_and_recreate_db(): @pytest.yield_fixture def app_context(): - app = Flask(__name__) + from cinch import app with app.test_request_context(): yield + + +@pytest.yield_fixture(autouse=True) +def temp_config(): + from cinch import app + original_config = app.config + yield + app.config = original_config diff --git a/tests/test_github.py b/tests/test_github.py new file mode 100644 index 0000000..14ad56c --- /dev/null +++ b/tests/test_github.py @@ -0,0 +1,23 @@ +from mock import patch +import pytest + +from cinch import app + + +@patch('cinch.github.GithubUpdateHandler') +@pytest.mark.parametrize(('args', 'status_code'), [ + ({}, 401), + ({'secret': 'incorrect secret'}, 401), + ({'secret': 'secret'}, 200), +]) +def test_updates_require_secret(handler, args, status_code): + app.config['GITHUB_TOKEN'] = 'abc' + app.config['GITHUB_UPDATE_SECRET'] = 'secret' + + with app.test_client() as client: + res = client.post('/api/github/update', query_string=args, data={ + 'payload': '{}', + }) + + assert res.status_code == status_code + assert handler.called == (status_code == 200) From 874ab0de752e8a506cab821d120c8cd4cbc128ff Mon Sep 17 00:00:00 2001 From: junkafarian Date: Mon, 31 Mar 2014 15:00:50 +0100 Subject: [PATCH 4/6] Try not to override the original config Fergus! --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 867c38f..30b3481 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -41,6 +41,6 @@ def app_context(): @pytest.yield_fixture(autouse=True) def temp_config(): from cinch import app - original_config = app.config + original_config = app.config.copy() yield app.config = original_config From 4289d2bed621b2b59f50b9834b6d715e639b3934 Mon Sep 17 00:00:00 2001 From: junkafarian Date: Mon, 31 Mar 2014 15:03:26 +0100 Subject: [PATCH 5/6] less obscure test conditions --- tests/test_github.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/test_github.py b/tests/test_github.py index 14ad56c..f453844 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -5,12 +5,12 @@ @patch('cinch.github.GithubUpdateHandler') -@pytest.mark.parametrize(('args', 'status_code'), [ - ({}, 401), - ({'secret': 'incorrect secret'}, 401), - ({'secret': 'secret'}, 200), +@pytest.mark.parametrize(('args', 'status_code', 'valid_update'), [ + ({}, 401, False), + ({'secret': 'incorrect secret'}, 401, False), + ({'secret': 'secret'}, 200, True), ]) -def test_updates_require_secret(handler, args, status_code): +def test_updates_require_secret(handler, args, status_code, valid_update): app.config['GITHUB_TOKEN'] = 'abc' app.config['GITHUB_UPDATE_SECRET'] = 'secret' @@ -20,4 +20,4 @@ def test_updates_require_secret(handler, args, status_code): }) assert res.status_code == status_code - assert handler.called == (status_code == 200) + assert handler.called == valid_update From 96328c8dbba87a4a65f2be4ad129c335d42de267 Mon Sep 17 00:00:00 2001 From: junkafarian Date: Mon, 9 Jun 2014 18:23:13 +0100 Subject: [PATCH 6/6] simplify assertions --- tests/test_github.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/test_github.py b/tests/test_github.py index 7c0782b..5f7e0b4 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -11,12 +11,12 @@ URL = '/api/github/update' -@pytest.mark.parametrize(('args', 'status_code', 'valid_update'), [ - ({}, 401, False), - ({'secret': 'incorrect secret'}, 401, False), - ({'secret': 'secret'}, 200, True), +@pytest.mark.parametrize(('args', 'status_code'), [ + ({}, 401), + ({'secret': 'incorrect secret'}, 401), + ({'secret': 'secret'}, 200), ]) -def test_updates_require_secret(args, status_code, valid_update): +def test_updates_require_secret(args, status_code): app.config['GITHUB_TOKEN'] = 'abc' app.config['GITHUB_UPDATE_SECRET'] = 'secret' @@ -27,7 +27,6 @@ def test_updates_require_secret(args, status_code, valid_update): ) assert res.status_code == status_code - assert valid_update == (res.data == 'pong') @pytest.fixture(autouse=True)