From e90358e1f454bfdcc4e2ba6871251005c3a85209 Mon Sep 17 00:00:00 2001 From: Elliott Sales de Andrade Date: Wed, 9 Mar 2022 16:31:10 -0500 Subject: [PATCH 1/2] Verify git checkout path is in site directory This is unlikely, as we verify inputs come from GitHub using signatures, but it's best to be safe about this. --- test_webhook.py | 8 ++++++++ webhook.py | 7 ++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/test_webhook.py b/test_webhook.py index 177b85a..87647ef 100644 --- a/test_webhook.py +++ b/test_webhook.py @@ -121,6 +121,14 @@ async def test_github_webhook_errors(aiohttp_client, monkeypatch): assert resp.status == 400 assert 'incorrect repository' in await resp.text() + # Fields provided, but invalid. + resp = await client.post( + '/gh/non-existent-repo', headers=valid_headers, + data='{"sender": {"login": "QuLogic"}, "repository":' + ' {"name": "..", "owner": {"login": "matplotlib"}}}') + assert resp.status == 400 + assert 'incorrect repository' in await resp.text() + # Problem on our side. resp = await client.post( '/gh/non-existent', diff --git a/webhook.py b/webhook.py index bc46d68..2d49bfa 100644 --- a/webhook.py +++ b/webhook.py @@ -156,7 +156,12 @@ async def github_webhook(request: web.Request): delivery, ref, expected_branch) return web.Response(status=200) - checkout = Path(os.environ.get('SITE_DIR', 'sites'), repository) + site_dir = Path(os.environ.get('SITE_DIR', 'sites')).resolve() + checkout = (site_dir / repository).resolve() + if not checkout.is_relative_to(site_dir): + raise web.HTTPBadRequest( + reason=(f'{delivery}: Checkout for {organization}/{repository} ' + 'does not exist')) if not (checkout / '.git').is_dir(): raise web.HTTPInternalServerError( reason=(f'{delivery}: Checkout for {organization}/{repository} ' From a52631a1e6cdb118e81a7315330d13f3aa2db904 Mon Sep 17 00:00:00 2001 From: Elliott Sales de Andrade Date: Wed, 9 Mar 2022 20:12:40 -0500 Subject: [PATCH 2/2] Move site directory init check to app creation --- test_webhook.py | 5 +++-- webhook.py | 11 ++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/test_webhook.py b/test_webhook.py index 87647ef..dbe0e13 100644 --- a/test_webhook.py +++ b/test_webhook.py @@ -65,8 +65,9 @@ async def test_update_repo(tmp_path_factory): assert dest_commit == src_commit -async def test_github_webhook_errors(aiohttp_client, monkeypatch): +async def test_github_webhook_errors(aiohttp_client, monkeypatch, tmp_path): """Test invalid inputs to webhook.""" + monkeypatch.setenv('SITE_DIR', str(tmp_path)) client = await aiohttp_client(create_app()) # Only /gh/ exists. @@ -142,12 +143,12 @@ async def test_github_webhook_errors(aiohttp_client, monkeypatch): async def test_github_webhook_valid(aiohttp_client, monkeypatch, tmp_path): """Test valid input to webhook.""" + monkeypatch.setenv('SITE_DIR', str(tmp_path)) client = await aiohttp_client(create_app()) # Do no actual work, since that's tested above. monkeypatch.setattr(webhook, 'verify_signature', mock.Mock(verify_signature, return_value=True)) - monkeypatch.setenv('SITE_DIR', str(tmp_path)) ur_mock = mock.Mock(update_repo, return_value=None) monkeypatch.setattr(webhook, 'update_repo', ur_mock) diff --git a/webhook.py b/webhook.py index 2d49bfa..7d4e4f2 100644 --- a/webhook.py +++ b/webhook.py @@ -156,9 +156,8 @@ async def github_webhook(request: web.Request): delivery, ref, expected_branch) return web.Response(status=200) - site_dir = Path(os.environ.get('SITE_DIR', 'sites')).resolve() - checkout = (site_dir / repository).resolve() - if not checkout.is_relative_to(site_dir): + checkout = (request.app['site_dir'] / repository).resolve() + if not checkout.is_relative_to(request.app['site_dir']): raise web.HTTPBadRequest( reason=(f'{delivery}: Checkout for {organization}/{repository} ' 'does not exist')) @@ -176,7 +175,11 @@ async def github_webhook(request: web.Request): def create_app(): """Create the aiohttp app and setup routes.""" + site_dir = Path(os.environ.get('SITE_DIR', 'sites')).resolve() + assert site_dir.is_dir() + app = web.Application() + app['site_dir'] = site_dir app.add_routes([ web.post('/gh/{repo}', github_webhook), ]) @@ -184,8 +187,6 @@ def create_app(): if __name__ == '__main__': - assert Path(os.environ.get('SITE_DIR', 'sites')).is_dir() - if len(sys.argv) > 1: from urllib.parse import urlparse url = sys.argv[1]