From 3d5934e27bdf07ac7d7214f2cf4208751d5940df Mon Sep 17 00:00:00 2001 From: mickael e Date: Tue, 8 Jan 2019 14:36:15 -0500 Subject: [PATCH 1/4] Allow DELETE HTTP method for journalist interface Certain Journalist API operations require DELETE (e.g. delete or unstar a source). This will ensure that Apache allows the DELETE operation for new installs (or existing installs after an Ansible run). --- .../roles/app/templates/sites-available/journalist.conf | 8 ++++---- .../app/apache/test_apache_journalist_interface.py | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf b/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf index 4fd947c3e0..a4719c4fbd 100644 --- a/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf +++ b/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf @@ -53,11 +53,11 @@ ErrorDocument 500 /error Options {{ apache_dir_options | default('None') }} AllowOverride None - + Order allow,deny allow from {{ securedrop_app_apache_allow_from }} - + Order deny,allow Deny from all @@ -66,11 +66,11 @@ ErrorDocument 500 /error Options {{ apache_dir_options | default('None') }} AllowOverride None - + Order allow,deny allow from {{ securedrop_app_apache_allow_from }} - + Order deny,allow Deny from all diff --git a/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py b/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py index 8c4609fedf..71f0b22efb 100644 --- a/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py +++ b/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py @@ -51,11 +51,11 @@ def test_apache_headers_journalist_interface(host, header): Options None AllowOverride None - + Order allow,deny allow from {apache_allow_from} - + Order deny,allow Deny from all @@ -64,11 +64,11 @@ def test_apache_headers_journalist_interface(host, header): Options None AllowOverride None - + Order allow,deny allow from {apache_allow_from} - + Order deny,allow Deny from all From a802dde7fdc128ce5444504ca1b054af3219c1b1 Mon Sep 17 00:00:00 2001 From: mickael e Date: Thu, 10 Jan 2019 13:34:57 -0500 Subject: [PATCH 2/4] Remove Apache ErrorDocument directives on Journalist Interface These directives were returning incorrect values for the Journalist API. Errors should be handled in the application to ensure headers are sent. The API should return appropirate error codes, and the web interface should always redirect to the login page. --- .../roles/app/templates/sites-available/journalist.conf | 7 ------- 1 file changed, 7 deletions(-) diff --git a/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf b/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf index a4719c4fbd..6dec14bec4 100644 --- a/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf +++ b/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf @@ -36,13 +36,6 @@ Header unset Etag # Limit the max submitted size of requests. LimitRequestBody 524288000 -#Redirect error pages to ensure headers are sent -ErrorDocument 400 /notfound -ErrorDocument 401 /notfound -ErrorDocument 403 /notfound -ErrorDocument 404 /notfound -ErrorDocument 500 /error - Options None AllowOverride None From 12977cc4707fad62595e56cea9de81ccaa8e3470 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Thu, 10 Jan 2019 15:16:38 -0800 Subject: [PATCH 3/4] tests: Add unit test verifying error handlers are in place The status codes in here are to ensure that the cases the ErrorDocument directives previously in the Apache journalist configuration are handled in the application code. --- securedrop/journalist_app/api.py | 4 ++-- securedrop/tests/test_journalist.py | 6 ++++++ securedrop/tests/test_journalist_api.py | 11 ++++++++++- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/securedrop/journalist_app/api.py b/securedrop/journalist_app/api.py index 01230b9d1b..0d2de87604 100644 --- a/securedrop/journalist_app/api.py +++ b/securedrop/journalist_app/api.py @@ -309,7 +309,7 @@ def get_current_user(): user = get_user_object(request) return jsonify(user.to_json()), 200 - def _handle_http_exception(error): + def _handle_api_http_exception(error): # Workaround for no blueprint-level 404/5 error handlers, see: # https://github.com/pallets/flask/issues/503#issuecomment-71383286 response = jsonify({'error': error.name, @@ -318,6 +318,6 @@ def _handle_http_exception(error): return response, error.code for code in default_exceptions: - api.errorhandler(code)(_handle_http_exception) + api.errorhandler(code)(_handle_api_http_exception) return api diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 8100a0b748..dd81df6e95 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -2024,3 +2024,9 @@ def test_does_set_cookie_headers(journalist_app, test_journo): observed_headers = response.headers assert 'Set-Cookie' in observed_headers.keys() assert 'Cookie' in observed_headers['Vary'] + + +def test_app_error_handlers_defined(journalist_app): + for status_code in [400, 401, 403, 404, 500]: + # This will raise KeyError if an app-wide error handler is not defined + assert journalist_app.error_handler_spec[None][status_code] diff --git a/securedrop/tests/test_journalist_api.py b/securedrop/tests/test_journalist_api.py index fc51e3982e..f99a277ea2 100644 --- a/securedrop/tests/test_journalist_api.py +++ b/securedrop/tests/test_journalist_api.py @@ -244,7 +244,16 @@ def test_user_without_token_cannot_post_protected_endpoints(journalist_app, assert response.status_code == 403 -def test_api_404(journalist_app, journalist_api_token): +def test_api_error_handlers_defined(journalist_app): + """Ensure the expected error handler is defined in the API blueprint""" + for status_code in [400, 401, 403, 404, 500]: + result = journalist_app.error_handler_spec['api'][status_code] + + expected_error_handler = '_handle_api_http_exception' + assert result.values()[0].__name__ == expected_error_handler + + +def test_api_error_handler_404(journalist_app, journalist_api_token): with journalist_app.test_client() as app: response = app.get('/api/v1/invalidendpoint', headers=get_api_headers(journalist_api_token)) From febee2bcf687c8a104616917dec24534f1a1c1cc Mon Sep 17 00:00:00 2001 From: Mickael E Date: Mon, 14 Jan 2019 10:34:16 -0500 Subject: [PATCH 4/4] Allow ETag headers for Journalist Interface ETags are useful to the journalist API to ensure file integrity, and in the future would allow file download resumption. Disabling ETags is a defense against information leakage https://nvd.nist.gov/vuln/detail/CVE-2003-1418. Since inodes are no longer included by Apache by default, since 2.3.14, that Apache only exposes filesize and MTime by default (information readily available on the Journalist interface, that the Journalist Interface is behind ATHS, it should be safe to re-enable ETags on the journalist interface only. --- .../roles/app/templates/sites-available/journalist.conf | 1 - .../staging/app/apache/test_apache_journalist_interface.py | 1 - 2 files changed, 2 deletions(-) diff --git a/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf b/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf index 6dec14bec4..3beeec1418 100644 --- a/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf +++ b/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf @@ -31,7 +31,6 @@ Header set X-Content-Type-Options: nosniff Header set X-Download-Options: noopen Header set X-Content-Security-Policy: "default-src 'self'" Header set Content-Security-Policy: "default-src 'self'" -Header unset Etag # Limit the max submitted size of requests. LimitRequestBody 524288000 diff --git a/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py b/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py index 71f0b22efb..cc1a5341a8 100644 --- a/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py +++ b/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py @@ -16,7 +16,6 @@ "Header set X-Content-Security-Policy: \"default-src 'self'\"", "Header set Content-Security-Policy: \"default-src 'self'\"", 'Header set Referrer-Policy "no-referrer"', - 'Header unset Etag', ]