Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow DELETE HTTP method for journalist interface #4023

Merged
merged 4 commits into from
Mar 29, 2019
Merged

Allow DELETE HTTP method for journalist interface #4023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3d5934e
Allow DELETE HTTP method for journalist interface
emkll Jan 8, 2019
a802dde
Remove Apache ErrorDocument directives on Journalist Interface
emkll Jan 10, 2019
12977cc
tests: Add unit test verifying error handlers are in place
redshiftzero Jan 10, 2019
febee2b
Allow ETag headers for Journalist Interface
emkll Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions install_files/ansible-base/roles/app/templates/sites-available/journalist.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,10 @@ 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

#Redirect error pages to ensure headers are sent
ErrorDocument 400 /notfound
ErrorDocument 401 /notfound
ErrorDocument 403 /notfound
ErrorDocument 404 /notfound
ErrorDocument 500 /error

<Directory />
Options None
AllowOverride None
Expand All @@ -53,11 +45,11 @@ ErrorDocument 500 /error
<Directory /var/www/>
Options {{ apache_dir_options | default('None') }}
AllowOverride None
<Limit GET POST HEAD>
<Limit GET POST HEAD DELETE>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove the ErrorDocument directives above as part of this ticket. I'm advocating this because they override the API error handlers we have implemented in the application code. Recall these Apache directives in the journalist config are why we saw redirects in #3977 instead of nice JSON responses. Let me know if you see a reason to keep them in the Apache config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After taking a look at the removal of the ErrorDocument directives, the default Apache pages for errors do not return the security headers that are configured for the journalist interface (i.e.: x-xss-protection referrer-policy ...) While these error pages are static, I think we should still provide the headers for those pages. However, this meas we will need to provide ErrorDocuments for each error (400, 401, 403, 404 and 500)

Order allow,deny
allow from {{ securedrop_app_apache_allow_from }}
</Limit>
<LimitExcept GET POST HEAD>
<LimitExcept GET POST HEAD DELETE>
Order deny,allow
Deny from all
</LimitExcept>
Expand All @@ -66,11 +58,11 @@ ErrorDocument 500 /error
<Directory /var/www/securedrop>
Options {{ apache_dir_options | default('None') }}
AllowOverride None
<Limit GET POST HEAD>
<Limit GET POST HEAD DELETE>
Order allow,deny
allow from {{ securedrop_app_apache_allow_from }}
</Limit>
<LimitExcept GET POST HEAD>
<LimitExcept GET POST HEAD DELETE>
Order deny,allow
Deny from all
</LimitExcept>
Expand Down
9 changes: 4 additions & 5 deletions molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
]


Expand Down Expand Up @@ -51,11 +50,11 @@ def test_apache_headers_journalist_interface(host, header):
<Directory /var/www/>
Options None
AllowOverride None
<Limit GET POST HEAD>
<Limit GET POST HEAD DELETE>
Order allow,deny
allow from {apache_allow_from}
</Limit>
<LimitExcept GET POST HEAD>
<LimitExcept GET POST HEAD DELETE>
Order deny,allow
Deny from all
</LimitExcept>
Expand All @@ -64,11 +63,11 @@ def test_apache_headers_journalist_interface(host, header):
<Directory {securedrop_code}>
Options None
AllowOverride None
<Limit GET POST HEAD>
<Limit GET POST HEAD DELETE>
Order allow,deny
allow from {apache_allow_from}
</Limit>
<LimitExcept GET POST HEAD>
<LimitExcept GET POST HEAD DELETE>
Order deny,allow
Deny from all
</LimitExcept>
Expand Down
4 changes: 2 additions & 2 deletions securedrop/journalist_app/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
6 changes: 6 additions & 0 deletions securedrop/tests/test_journalist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
11 changes: 10 additions & 1 deletion securedrop/tests/test_journalist_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down