From 20064fcca66f0a9c4282022247b7cb1f4d91207f Mon Sep 17 00:00:00 2001 From: John Hensley Date: Mon, 16 Dec 2019 19:29:36 -0500 Subject: [PATCH 1/4] Improve error handling, tests Try to ensure the proxy always returns a valid JSON response. Add specific handling of upstream error responses to proxy.py, and add tests and fixtures for that. Replace the Proxy._on_done static method with an instance method. In main.py, bail out upon receiving invalid input JSON, instead of printing an error response and then continuing to try to use it, resulting in printing another error response. Have main.py use the callbacks on the proxy as given, instead of always overwriting them with the defaults from callbacks.py. Rework entrypoint.py so that it should always print a JSON response, and add tests for it. --- fixtures/main_error_response.yaml | 30 +++++ fixtures/main_input_body.yaml | 59 +++++++++ fixtures/main_json_response.yaml | 92 ++++++++++++++ fixtures/main_non_json_response.yaml | 145 ++++++++++++++++++++++ fixtures/proxy_bad_request.yaml | 20 +++ fixtures/proxy_callbacks.yaml | 145 ++++++++++++++++++++++ fixtures/proxy_cannot_connect.yaml | 22 ++++ fixtures/proxy_internal_error.yaml | 22 ++++ fixtures/proxy_internal_server_error.yaml | 20 +++ fixtures/proxy_unofficial_status.yaml | 20 +++ securedrop_proxy/entrypoint.py | 76 ++++++------ securedrop_proxy/main.py | 5 +- securedrop_proxy/proxy.py | 61 ++++++--- tests/files/dev-config.yaml | 5 + tests/test_callbacks.py | 51 ++++++++ tests/test_config.py | 4 + tests/test_entrypoint.py | 128 +++++++++++++++++++ tests/test_main.py | 83 +++++++++++-- tests/test_proxy.py | 132 +++++++++++++++++++- 19 files changed, 1044 insertions(+), 76 deletions(-) create mode 100644 fixtures/main_error_response.yaml create mode 100644 fixtures/main_input_body.yaml create mode 100644 fixtures/main_json_response.yaml create mode 100644 fixtures/main_non_json_response.yaml create mode 100644 fixtures/proxy_bad_request.yaml create mode 100644 fixtures/proxy_callbacks.yaml create mode 100644 fixtures/proxy_cannot_connect.yaml create mode 100644 fixtures/proxy_internal_error.yaml create mode 100644 fixtures/proxy_internal_server_error.yaml create mode 100644 fixtures/proxy_unofficial_status.yaml create mode 100644 tests/files/dev-config.yaml create mode 100644 tests/test_entrypoint.py diff --git a/fixtures/main_error_response.yaml b/fixtures/main_error_response.yaml new file mode 100644 index 0000000..30d6fd7 --- /dev/null +++ b/fixtures/main_error_response.yaml @@ -0,0 +1,30 @@ +interactions: +- request: + body: null + headers: + Content-Length: + - '0' + method: '' + uri: https://jsonplaceholder.typicode.com/ + response: + body: + string: "\r\n400 Bad Request\r\n\r\n\ +

400 Bad Request

\r\n
cloudflare
\r\ + \n\r\n\r\n" + headers: + CF-RAY: + - '-' + Connection: + - close + Content-Length: + - '155' + Content-Type: + - text/html + Date: + - Mon, 16 Dec 2019 22:11:39 GMT + Server: + - cloudflare + status: + code: 400 + message: Bad Request +version: 1 diff --git a/fixtures/main_input_body.yaml b/fixtures/main_input_body.yaml new file mode 100644 index 0000000..0ea82b1 --- /dev/null +++ b/fixtures/main_input_body.yaml @@ -0,0 +1,59 @@ +interactions: +- request: + body: id=42&title=test + headers: + Content-Length: + - '16' + Content-Type: + - application/x-www-form-urlencoded + method: POST + uri: https://jsonplaceholder.typicode.com/posts + response: + body: + string: "{\n \"id\": 101,\n \"title\": \"test\"\n}" + headers: + Access-Control-Allow-Credentials: + - 'true' + Access-Control-Expose-Headers: + - Location + CF-Cache-Status: + - DYNAMIC + CF-RAY: + - 546ab6213f81f11a-IAD + Cache-Control: + - no-cache + Connection: + - keep-alive + Content-Length: + - '34' + Content-Type: + - application/json; charset=utf-8 + Date: + - Tue, 17 Dec 2019 17:45:33 GMT + Etag: + - W/"22-i04alCk7PdGrJ2UKCUwBOO0LB3w" + Expect-CT: + - max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct" + Expires: + - '-1' + Location: + - http://jsonplaceholder.typicode.com/posts/101 + Pragma: + - no-cache + Server: + - cloudflare + Set-Cookie: + - __cfduid=d1fa880178dc2db917f26b8ce4e0d56e41576604733; expires=Thu, 16-Jan-20 + 17:45:33 GMT; path=/; domain=.typicode.com; HttpOnly; SameSite=Lax + Vary: + - Origin, X-HTTP-Method-Override, Accept-Encoding + Via: + - 1.1 vegur + X-Content-Type-Options: + - nosniff + X-Powered-By: + - Express + status: + code: 201 + message: Created +version: 1 diff --git a/fixtures/main_json_response.yaml b/fixtures/main_json_response.yaml new file mode 100644 index 0000000..782bee3 --- /dev/null +++ b/fixtures/main_json_response.yaml @@ -0,0 +1,92 @@ +interactions: +- request: + body: null + headers: {} + method: GET + uri: https://jsonplaceholder.typicode.com/posts?userId=1 + response: + body: + string: "[\n {\n \"userId\": 1,\n \"id\": 1,\n \"title\": \"sunt aut\ + \ facere repellat provident occaecati excepturi optio reprehenderit\",\n \ + \ \"body\": \"quia et suscipit\\nsuscipit recusandae consequuntur expedita\ + \ et cum\\nreprehenderit molestiae ut ut quas totam\\nnostrum rerum est autem\ + \ sunt rem eveniet architecto\"\n },\n {\n \"userId\": 1,\n \"id\"\ + : 2,\n \"title\": \"qui est esse\",\n \"body\": \"est rerum tempore\ + \ vitae\\nsequi sint nihil reprehenderit dolor beatae ea dolores neque\\nfugiat\ + \ blanditiis voluptate porro vel nihil molestiae ut reiciendis\\nqui aperiam\ + \ non debitis possimus qui neque nisi nulla\"\n },\n {\n \"userId\":\ + \ 1,\n \"id\": 3,\n \"title\": \"ea molestias quasi exercitationem repellat\ + \ qui ipsa sit aut\",\n \"body\": \"et iusto sed quo iure\\nvoluptatem\ + \ occaecati omnis eligendi aut ad\\nvoluptatem doloribus vel accusantium quis\ + \ pariatur\\nmolestiae porro eius odio et labore et velit aut\"\n },\n {\n\ + \ \"userId\": 1,\n \"id\": 4,\n \"title\": \"eum et est occaecati\"\ + ,\n \"body\": \"ullam et saepe reiciendis voluptatem adipisci\\nsit amet\ + \ autem assumenda provident rerum culpa\\nquis hic commodi nesciunt rem tenetur\ + \ doloremque ipsam iure\\nquis sunt voluptatem rerum illo velit\"\n },\n\ + \ {\n \"userId\": 1,\n \"id\": 5,\n \"title\": \"nesciunt quas odio\"\ + ,\n \"body\": \"repudiandae veniam quaerat sunt sed\\nalias aut fugiat\ + \ sit autem sed est\\nvoluptatem omnis possimus esse voluptatibus quis\\nest\ + \ aut tenetur dolor neque\"\n },\n {\n \"userId\": 1,\n \"id\": 6,\n\ + \ \"title\": \"dolorem eum magni eos aperiam quia\",\n \"body\": \"\ + ut aspernatur corporis harum nihil quis provident sequi\\nmollitia nobis aliquid\ + \ molestiae\\nperspiciatis et ea nemo ab reprehenderit accusantium quas\\\ + nvoluptate dolores velit et doloremque molestiae\"\n },\n {\n \"userId\"\ + : 1,\n \"id\": 7,\n \"title\": \"magnam facilis autem\",\n \"body\"\ + : \"dolore placeat quibusdam ea quo vitae\\nmagni quis enim qui quis quo nemo\ + \ aut saepe\\nquidem repellat excepturi ut quia\\nsunt ut sequi eos ea sed\ + \ quas\"\n },\n {\n \"userId\": 1,\n \"id\": 8,\n \"title\": \"\ + dolorem dolore est ipsam\",\n \"body\": \"dignissimos aperiam dolorem qui\ + \ eum\\nfacilis quibusdam animi sint suscipit qui sint possimus cum\\nquaerat\ + \ magni maiores excepturi\\nipsam ut commodi dolor voluptatum modi aut vitae\"\ + \n },\n {\n \"userId\": 1,\n \"id\": 9,\n \"title\": \"nesciunt\ + \ iure omnis dolorem tempora et accusantium\",\n \"body\": \"consectetur\ + \ animi nesciunt iure dolore\\nenim quia ad\\nveniam autem ut quam aut nobis\\\ + net est aut quod aut provident voluptas autem voluptas\"\n },\n {\n \"\ + userId\": 1,\n \"id\": 10,\n \"title\": \"optio molestias id quia eum\"\ + ,\n \"body\": \"quo et expedita modi cum officia vel magni\\ndoloribus\ + \ qui repudiandae\\nvero nisi sit\\nquos veniam quod sed accusamus veritatis\ + \ error\"\n }\n]" + headers: + Access-Control-Allow-Credentials: + - 'true' + Age: + - '4646' + CF-Cache-Status: + - HIT + CF-RAY: + - 54640259fa6fe0d2-IAD + Cache-Control: + - max-age=14400 + Connection: + - keep-alive + Content-Type: + - application/json; charset=utf-8 + Date: + - Mon, 16 Dec 2019 22:14:15 GMT + Etag: + - W/"aa6-j2NSH739l9uq40OywFMn7Y0C/iY" + Expect-CT: + - max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct" + Expires: + - '-1' + Pragma: + - no-cache + Server: + - cloudflare + Set-Cookie: + - __cfduid=d8ac1a396d31b9e9c1f816924e5bf186d1576534455; expires=Wed, 15-Jan-20 + 22:14:15 GMT; path=/; domain=.typicode.com; HttpOnly + Transfer-Encoding: + - chunked + Vary: + - Origin, Accept-Encoding + Via: + - 1.1 vegur + X-Content-Type-Options: + - nosniff + X-Powered-By: + - Express + status: + code: 200 + message: OK +version: 1 diff --git a/fixtures/main_non_json_response.yaml b/fixtures/main_non_json_response.yaml new file mode 100644 index 0000000..e5e02a9 --- /dev/null +++ b/fixtures/main_non_json_response.yaml @@ -0,0 +1,145 @@ +interactions: +- request: + body: null + headers: {} + method: GET + uri: https://jsonplaceholder.typicode.com/ + response: + body: + string: "\n\n\n\n\n\n\n\n\n\n\nJSONPlaceholder - Fake online REST API for developers\n\ + \n\n
\n\n\ + \n Announcement: You can now support\n\ + JSONPlaceholder on GitHub Sponsors!\n\n
\n
\n\n
\n
\n

\nJSONPlaceholder\n\ +

\n

\nFake Online REST API for Testing and Prototyping\n\ +
Serving ~350M requests per month\n
Powered by\nJSON Server\n+\nLowDB\n

\n\n\n\n\n\n
\n
\n\n
\n

Gold\ + \ Sponsors

\n

\n\n\n\n

\n

\nYour Company Logo Here\n

\n
\n\n
\n
\n\ + \n

Intro

\n

\nJSONPlaceholder is a free online REST API that you\ + \ can use whenever you need some fake data.\n
It's great for tutorials,\ + \ testing new libraries, sharing code examples, ...\n

\n\n

Example

\n\ +

\nRun this code in a console or from any site:\n

\n
fetch('https://jsonplaceholder.typicode.com/todos/1')\n\
+        \  .then(response => response.json())\n  .then(json => console.log(json))\n\
+
\n

\n\n

\n
\nCongrats\ + \ you've made your first call to JSONPlaceholder! \U0001F603 \U0001F389\n\ +

\nTip: you can use\n\nhttp://\n or\n\nhttps://\n\ + when making requests to JSONPlaceholder.\n

\n
\n\n\n

Resources

\n

\nJSONPlaceholder comes with a\ + \ set of 6 common resources:\n

\n\n\n\n\n\n\n\ + \n\n\ + \n\n\n\n\ + \n\n\n\n\ + \n\n\n\n\ + \n\n\n\n\ + \n
\n\ + \ /posts\n100 posts
\n/comments\n500 comments
\n/albums\n100 albums
\n/photos\n5000 photos
\n/todos\n200 todos
\n/users\n10 users
\n

\nNote: resources have relations. For\ + \ example:\nposts have many\ncomments,\nalbums have many\n\ + photos, ... see below for routes examples.\n

\n\n

Routes

\n\ +

\nAll HTTP methods are supported.\n

\n\n\n\n\ + \n\n\n\n\n\n\n\n\n\n\n\ + \n\n\n\n\n\n\n\n\n\n\n\n\n\ + \n\n\n\n\n\n\ + \n\n\n\n\n
GET\n/posts\n
GET\n\ + /posts/1\n
GET\n\ + /posts/1/comments\n
GET\n/comments?postId=1\n\ +
GET\n/posts?userId=1\n\ +
POST/posts
PUT/posts/1
PATCH/posts/1
DELETE/posts/1
\n

\nNote:\ + \ you can view detailed examples\nhere.\n

\n\ + \n

Use your own data

\n\n

\nWith My JSON Server online service and a simple GitHub repo, you can have\ + \ your own online fake REST server in seconds.\n

\n
\n
\n
\n\ + \n\n\n\n\n\ + \n\n\n" + headers: + Access-Control-Allow-Credentials: + - 'true' + Age: + - '5668' + CF-Cache-Status: + - HIT + CF-RAY: + - 5463fe6ccb0ecf04-IAD + Cache-Control: + - public, max-age=14400 + Connection: + - keep-alive + Content-Type: + - text/html; charset=UTF-8 + Date: + - Mon, 16 Dec 2019 22:11:34 GMT + Expect-CT: + - max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct" + Last-Modified: + - Mon, 05 Aug 2019 03:07:14 GMT + Server: + - cloudflare + Set-Cookie: + - __cfduid=d13b55a7f6e786f74d5f9f1f084a183a31576534294; expires=Wed, 15-Jan-20 + 22:11:34 GMT; path=/; domain=.typicode.com; HttpOnly + Transfer-Encoding: + - chunked + Vary: + - Origin, Accept-Encoding + Via: + - 1.1 vegur + X-Powered-By: + - Express + status: + code: 200 + message: OK +version: 1 diff --git a/fixtures/proxy_bad_request.yaml b/fixtures/proxy_bad_request.yaml new file mode 100644 index 0000000..9d71f3d --- /dev/null +++ b/fixtures/proxy_bad_request.yaml @@ -0,0 +1,20 @@ +interactions: +- request: + body: null + headers: + Accept: + - application/json + method: GET + uri: http://localhost:8000/bad + response: + body: + string: '' + headers: + Date: + - Mon, 16 Dec 2019 23:44:53 GMT + Server: + - BaseHTTP/0.6 Python/3.7.3 + status: + code: 400 + message: Bad Request +version: 1 diff --git a/fixtures/proxy_callbacks.yaml b/fixtures/proxy_callbacks.yaml new file mode 100644 index 0000000..7224c8c --- /dev/null +++ b/fixtures/proxy_callbacks.yaml @@ -0,0 +1,145 @@ +interactions: +- request: + body: null + headers: {} + method: GET + uri: https://jsonplaceholder.typicode.com/ + response: + body: + string: "\n\n\n\n\n\n\n\n\n\n\nJSONPlaceholder - Fake online REST API for developers\n\ + \n\n
\n\n\ + \n Announcement: You can now support\n\ + JSONPlaceholder on GitHub Sponsors!\n\n
\n
\n\n
\n
\n

\nJSONPlaceholder\n\ +

\n

\nFake Online REST API for Testing and Prototyping\n\ +
Serving ~350M requests per month\n
Powered by\nJSON Server\n+\nLowDB\n

\n\n\n\n\n\n
\n
\n\n
\n

Gold\ + \ Sponsors

\n

\n\n\n\n

\n

\nYour Company Logo Here\n

\n
\n\n
\n
\n\ + \n

Intro

\n

\nJSONPlaceholder is a free online REST API that you\ + \ can use whenever you need some fake data.\n
It's great for tutorials,\ + \ testing new libraries, sharing code examples, ...\n

\n\n

Example

\n\ +

\nRun this code in a console or from any site:\n

\n
fetch('https://jsonplaceholder.typicode.com/todos/1')\n\
+        \  .then(response => response.json())\n  .then(json => console.log(json))\n\
+
\n

\n\n

\n
\nCongrats\ + \ you've made your first call to JSONPlaceholder! \U0001F603 \U0001F389\n\ +

\nTip: you can use\n\nhttp://\n or\n\nhttps://\n\ + when making requests to JSONPlaceholder.\n

\n
\n\n\n

Resources

\n

\nJSONPlaceholder comes with a\ + \ set of 6 common resources:\n

\n\n\n\n\n\n\n\ + \n\n\ + \n\n\n\n\ + \n\n\n\n\ + \n\n\n\n\ + \n\n\n\n\ + \n
\n\ + \ /posts\n100 posts
\n/comments\n500 comments
\n/albums\n100 albums
\n/photos\n5000 photos
\n/todos\n200 todos
\n/users\n10 users
\n

\nNote: resources have relations. For\ + \ example:\nposts have many\ncomments,\nalbums have many\n\ + photos, ... see below for routes examples.\n

\n\n

Routes

\n\ +

\nAll HTTP methods are supported.\n

\n\n\n\n\ + \n\n\n\n\n\n\n\n\n\n\n\ + \n\n\n\n\n\n\n\n\n\n\n\n\n\ + \n\n\n\n\n\n\ + \n\n\n\n\n
GET\n/posts\n
GET\n\ + /posts/1\n
GET\n\ + /posts/1/comments\n
GET\n/comments?postId=1\n\ +
GET\n/posts?userId=1\n\ +
POST/posts
PUT/posts/1
PATCH/posts/1
DELETE/posts/1
\n

\nNote:\ + \ you can view detailed examples\nhere.\n

\n\ + \n

Use your own data

\n\n

\nWith My JSON Server online service and a simple GitHub repo, you can have\ + \ your own online fake REST server in seconds.\n

\n
\n
\n
\n\ + \n\n\n\n\n\ + \n\n\n" + headers: + Access-Control-Allow-Credentials: + - 'true' + Age: + - '4432' + CF-Cache-Status: + - HIT + CF-RAY: + - 5469de47682ecf00-IAD + Cache-Control: + - public, max-age=14400 + Connection: + - keep-alive + Content-Type: + - text/html; charset=UTF-8 + Date: + - Tue, 17 Dec 2019 15:18:12 GMT + Expect-CT: + - max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct" + Last-Modified: + - Mon, 05 Aug 2019 03:07:14 GMT + Server: + - cloudflare + Set-Cookie: + - __cfduid=dc33ebab29f22c648b958d03fed5596b51576595892; expires=Thu, 16-Jan-20 + 15:18:12 GMT; path=/; domain=.typicode.com; HttpOnly; SameSite=Lax + Transfer-Encoding: + - chunked + Vary: + - Origin, Accept-Encoding + Via: + - 1.1 vegur + X-Powered-By: + - Express + status: + code: 200 + message: OK +version: 1 diff --git a/fixtures/proxy_cannot_connect.yaml b/fixtures/proxy_cannot_connect.yaml new file mode 100644 index 0000000..9788ab4 --- /dev/null +++ b/fixtures/proxy_cannot_connect.yaml @@ -0,0 +1,22 @@ +interactions: +- request: + body: null + headers: + Accept: + - application/json + method: GET + uri: http://localhost:8000/ + response: + body: + string: 'hello + + ' + headers: + Date: + - Tue, 17 Dec 2019 00:06:22 GMT + Server: + - BaseHTTP/0.6 Python/3.7.3 + status: + code: 200 + message: OK +version: 1 diff --git a/fixtures/proxy_internal_error.yaml b/fixtures/proxy_internal_error.yaml new file mode 100644 index 0000000..7295de6 --- /dev/null +++ b/fixtures/proxy_internal_error.yaml @@ -0,0 +1,22 @@ +interactions: +- request: + body: null + headers: + Accept: + - application/json + method: GET + uri: http://localhost:8000/ + response: + body: + string: 'hello + + ' + headers: + Date: + - Tue, 17 Dec 2019 00:00:13 GMT + Server: + - BaseHTTP/0.6 Python/3.7.3 + status: + code: 200 + message: OK +version: 1 diff --git a/fixtures/proxy_internal_server_error.yaml b/fixtures/proxy_internal_server_error.yaml new file mode 100644 index 0000000..603e624 --- /dev/null +++ b/fixtures/proxy_internal_server_error.yaml @@ -0,0 +1,20 @@ +interactions: +- request: + body: null + headers: + Accept: + - application/json + method: GET + uri: http://localhost:8000/crash + response: + body: + string: '' + headers: + Date: + - Mon, 16 Dec 2019 23:58:10 GMT + Server: + - BaseHTTP/0.6 Python/3.7.3 + status: + code: 500 + message: Internal Server Error +version: 1 diff --git a/fixtures/proxy_unofficial_status.yaml b/fixtures/proxy_unofficial_status.yaml new file mode 100644 index 0000000..fa62ef9 --- /dev/null +++ b/fixtures/proxy_unofficial_status.yaml @@ -0,0 +1,20 @@ +interactions: +- request: + body: null + headers: + Accept: + - application/json + method: GET + uri: http://localhost:8000/teapot + response: + body: + string: '' + headers: + Date: + - Mon, 16 Dec 2019 23:59:52 GMT + Server: + - BaseHTTP/0.6 Python/3.7.3 + status: + code: 418 + message: '' +version: 1 diff --git a/securedrop_proxy/entrypoint.py b/securedrop_proxy/entrypoint.py index 70f21a3..fb0a8c1 100755 --- a/securedrop_proxy/entrypoint.py +++ b/securedrop_proxy/entrypoint.py @@ -6,6 +6,8 @@ # called with exactly one argument: the path to its config file. See # the README for configuration options. +import http +import json import logging import os import sys @@ -30,60 +32,55 @@ def start(): ''' try: configure_logging() - except Exception as e: - print(e) - return - - logging.debug('Starting SecureDrop Proxy {}'.format(version)) - # a fresh, new proxy object - p = proxy.Proxy() + logging.debug('Starting SecureDrop Proxy {}'.format(version)) - # set up an error handler early, so we can use it during - # configuration, etc - p.on_done = callbacks.err_on_done + # a fresh, new proxy object + p = proxy.Proxy() - # path to config file must be at argv[1] - if len(sys.argv) != 2: - p.simple_error( - 500, "sd-proxy script not called with path to configuration file" - ) - p.on_done(p.res) + # set up an error handler early, so we can use it during + # configuration, etc + original_on_done = p.on_done + p.on_done = callbacks.err_on_done - # read config. `read_conf` will call `p.on_done` if there is a config - # problem, and will return a Conf object on success. - conf_path = sys.argv[1] - p.conf = config.read_conf(conf_path, p) + # path to config file must be at argv[1] + if len(sys.argv) != 2: + raise ValueError("sd-proxy script not called with path to configuration file") - # read user request from STDIN - incoming = [] - for line in sys.stdin: - incoming.append(line) - incoming = "\n".join(incoming) + # read config. `read_conf` will call `p.on_done` if there is a config + # problem, and will return a Conf object on success. + conf_path = sys.argv[1] + p.conf = config.read_conf(conf_path, p) - main.__main__(incoming, p) + # read user request from STDIN + incoming = [] + for line in sys.stdin: + incoming.append(line) + incoming = "\n".join(incoming) - -def excepthook(*exc_args): - ''' - This function is called in the event of a catastrophic failure. - Log exception and exit cleanly. - ''' - logging.error('Unrecoverable error', exc_info=(exc_args)) - sys.__excepthook__(*exc_args) - print('') # force terminal prompt on to a new line - sys.exit(1) + p.on_done = original_on_done + main.__main__(incoming, p) + except Exception as e: + response = { + "status": http.HTTPStatus.INTERNAL_SERVER_ERROR, + "body": json.dumps({ + "error": str(e), + }) + } + print(json.dumps(response)) + sys.exit(1) def configure_logging() -> None: ''' All logging related settings are set up by this function. ''' - log_folder = os.path.join(DEFAULT_HOME, 'logs') + home = os.getenv("SECUREDROP_HOME", DEFAULT_HOME) + log_folder = os.path.join(home, 'logs') if not os.path.exists(log_folder): os.makedirs(log_folder) - log_file = os.path.join(DEFAULT_HOME, 'logs', 'proxy.log') + log_file = os.path.join(home, 'logs', 'proxy.log') # set logging format log_fmt = ('%(asctime)s - %(name)s:%(lineno)d(%(funcName)s) %(levelname)s: %(message)s') @@ -98,6 +95,3 @@ def configure_logging() -> None: log = logging.getLogger() log.setLevel(LOGLEVEL) log.addHandler(handler) - - # override excepthook to capture a log of catastrophic failures. - sys.excepthook = excepthook diff --git a/securedrop_proxy/main.py b/securedrop_proxy/main.py index 6fadafa..8986449 100644 --- a/securedrop_proxy/main.py +++ b/securedrop_proxy/main.py @@ -21,6 +21,7 @@ def __main__(incoming, p): logging.error(e) p.simple_error(400, 'Invalid JSON in request') p.on_done(p.res) + return req = proxy.Req() try: @@ -38,6 +39,6 @@ def __main__(incoming, p): req.body = client_req['body'] p.req = req - p.on_save = callbacks.on_save - p.on_done = callbacks.on_done + if p.on_save is None: + p.on_save = callbacks.on_save p.proxy() diff --git a/securedrop_proxy/proxy.py b/securedrop_proxy/proxy.py index cc30b32..59a9a3a 100644 --- a/securedrop_proxy/proxy.py +++ b/securedrop_proxy/proxy.py @@ -1,4 +1,5 @@ import furl +import http import json import logging import requests @@ -6,6 +7,7 @@ import werkzeug import securedrop_proxy.version as version +from securedrop_proxy import callbacks logger = logging.getLogger(__name__) @@ -28,22 +30,21 @@ def __init__(self, status): class Proxy: - @staticmethod - def _on_done(res): - print(json.dumps(res.__dict__)) - - def __init__(self, conf=None, req=Req(), on_save=None, on_done=None): + def __init__(self, conf=None, req=Req(), on_save=None, on_done=None, timeout: float = None): self.conf = conf self.req = req self.res = None self.on_save = on_save if on_done is not None: self.on_done = on_done - else: - self.on_done = self._on_done + + self.timeout = float(timeout) if timeout else 10 self._prepared_request = None + def on_done(self, res): + callbacks.on_done(res) + @staticmethod def valid_path(path): u = furl.furl(path) @@ -75,7 +76,7 @@ def prep_request(self): try: url = furl.furl("{}://{}:{}/{}".format(scheme, host, port, path)) except Exception as e: - logging.error(e) + logger.error(e) self.simple_error(500, "Proxy error while generating URL to request") raise ValueError("Error generating URL from provided values") @@ -118,7 +119,7 @@ def handle_non_json_response(self): self.res = res def handle_response(self): - logging.debug('Handling response') + logger.debug("Handling response") ctype = werkzeug.http.parse_options_header(self._presp.headers["content-type"]) @@ -135,23 +136,45 @@ def proxy(self): try: if self.on_save is None: - self.simple_error(400, "Request callback is not set.") - raise ValueError("Request callback is not set.") + self.simple_error( + http.HTTPStatus.BAD_REQUEST, "Request on_save callback is not set." + ) + raise ValueError("Request on_save callback is not set.") self.prep_request() - logging.debug('Sending request') + logger.debug("Sending request") s = requests.Session() - self._presp = s.send(self._prepared_request) + self._presp = s.send(self._prepared_request, timeout=self.timeout) + self._presp.raise_for_status() self.handle_response() - except ValueError as e: - logging.error(e) + logger.error(e) # effectively a 4xx error # we have set self.response to indicate an error pass - - # catch server errors here, handle maybe-differently from - # ValueErrors... - + except requests.exceptions.Timeout as e: + # Timeout covers both ConnectTimeout and ReadTimeout + logger.error(e) + self.simple_error(http.HTTPStatus.GATEWAY_TIMEOUT, "request timed out") + except ( + requests.exceptions.ConnectionError, # covers ProxyError, SSLError + requests.exceptions.TooManyRedirects, + ) as e: + logger.error(e) + self.simple_error(http.HTTPStatus.BAD_GATEWAY, "could not connect to server") + except requests.exceptions.HTTPError as e: + logger.error(e) + try: + self.simple_error( + e.response.status_code, + http.HTTPStatus(e.response.status_code).phrase.lower() + ) + except ValueError: + # Return a generic error message when the response + # status code is not found in http.HTTPStatus. + self.simple_error(e.response.status_code, "unspecified server error") + except Exception as e: + logger.error(e) + self.simple_error(http.HTTPStatus.INTERNAL_SERVER_ERROR, "internal proxy error") self.on_done(self.res) diff --git a/tests/files/dev-config.yaml b/tests/files/dev-config.yaml new file mode 100644 index 0000000..8e72283 --- /dev/null +++ b/tests/files/dev-config.yaml @@ -0,0 +1,5 @@ +host: jsonplaceholder.typicode.com +scheme: https +port: 443 +target_vm: compost +dev: True diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index d0993e4..83c861b 100644 --- a/tests/test_callbacks.py +++ b/tests/test_callbacks.py @@ -5,6 +5,8 @@ import unittest from unittest.mock import patch +import vcr + from securedrop_proxy import callbacks from securedrop_proxy import config from securedrop_proxy import proxy @@ -76,3 +78,52 @@ def test_on_save_200_success(self): 'application/json') self.assertEqual(self.res.status, 200) self.assertIn('filename', self.res.body) + + @vcr.use_cassette("fixtures/proxy_callbacks.yaml") + def test_custom_callbacks(self): + """ + Test the handlers in a real proxy request. + """ + conf = config.Conf() + conf.host = 'jsonplaceholder.typicode.com' + conf.scheme = 'https' + conf.port = 443 + + req = proxy.Req() + req.method = "GET" + + on_save_addition = "added by the on_save callback\n" + on_done_addition = "added by the on_done callback\n" + + def on_save(fh, res, conf): + res.headers['Content-Type'] = 'text/plain' + res.body = on_save_addition + + def on_done(res): + res.headers['Content-Type'] = 'text/plain' + res.body += on_done_addition + + p = proxy.Proxy(self.conf, req, on_save=on_save, on_done=on_done) + p.proxy() + + self.assertEqual( + p.res.body, + "{}{}".format(on_save_addition, on_done_addition) + ) + + @vcr.use_cassette("fixtures/proxy_callbacks.yaml") + def test_production_on_save(self): + """ + Test on_save's production file handling. + """ + conf = config.Conf() + conf.host = 'jsonplaceholder.typicode.com' + conf.scheme = 'https' + conf.port = 443 + conf.dev = False + conf.target_vm = "sd-svs-dispvm" + + with patch("subprocess.run") as patched_run: + fh = tempfile.NamedTemporaryFile() + callbacks.on_save(fh, self.res, conf) + self.assertEqual(patched_run.call_args[0][0][0], "qvm-move-to-vm") diff --git a/tests/test_config.py b/tests/test_config.py index 916ad67..0ae93ea 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -90,3 +90,7 @@ def err_on_done(res): with self.assertRaises(SystemExit): config.read_conf('tests/files/missing-target-vm.yaml', self.p) + + def test_dev_config(self): + c = config.read_conf('tests/files/dev-config.yaml', self.p) + self.assertTrue(c.dev) diff --git a/tests/test_entrypoint.py b/tests/test_entrypoint.py new file mode 100644 index 0000000..13b1d04 --- /dev/null +++ b/tests/test_entrypoint.py @@ -0,0 +1,128 @@ +import contextlib +import http +import io +import json +import os +import sys +import tempfile +import unittest.mock + +import vcr +from securedrop_proxy import entrypoint + + +@contextlib.contextmanager +def sdhome(*args, **kwds): + with tempfile.TemporaryDirectory() as tmphome: + os.environ["SECUREDROP_HOME"] = tmphome + try: + yield tmphome + finally: + del os.environ["SECUREDROP_HOME"] + + +class TestEntrypoint(unittest.TestCase): + """ + Test the entrypoint used in production. + """ + + def test_missing_config(self): + config_path = "/tmp/nonexistent-config.yaml" + self.assertFalse(os.path.exists(config_path)) + + output = None + with unittest.mock.patch( + "sys.argv", new_callable=lambda: ["sd-proxy", config_path] + ) as mock_argv, unittest.mock.patch("sys.stdout", new_callable=io.StringIO) as mock_stdout: + with self.assertRaises(SystemExit), sdhome(): + entrypoint.start() + output = mock_stdout.getvalue() + + response = json.loads(output) + self.assertEqual(response["status"], http.HTTPStatus.INTERNAL_SERVER_ERROR) + body = json.loads(response["body"]) + self.assertEqual( + body["error"], "Configuration file does not exist at {}".format(config_path) + ) + + def test_unwritable_log_folder(self): + """ + Tests a permission problem in `configure_logging`. + """ + output = None + with sdhome() as home: + os.chmod(home, 0o0444) + with unittest.mock.patch("sys.stdout", new_callable=io.StringIO) as mock_stdout: + with self.assertRaises(SystemExit): + entrypoint.start() + output = mock_stdout.getvalue() + os.chmod(home, 0o0700) + + response = json.loads(output) + self.assertEqual(response["status"], http.HTTPStatus.INTERNAL_SERVER_ERROR) + body = json.loads(response["body"]) + self.assertIn("Permission denied: ", body["error"]) + + def test_wrong_number_of_arguments(self): + with sdhome() as home: + with unittest.mock.patch( + "sys.argv", new_callable=lambda: ["sd-proxy"] + ) as mock_argv, unittest.mock.patch( + "sys.stdout", new_callable=io.StringIO + ) as mock_stdout: + with self.assertRaises(SystemExit): + entrypoint.start() + output = mock_stdout.getvalue() + + response = json.loads(output) + self.assertEqual(response["status"], http.HTTPStatus.INTERNAL_SERVER_ERROR) + body = json.loads(response["body"]) + self.assertEqual( + body["error"], "sd-proxy script not called with path to configuration file" + ) + + def test_empty_input(self): + config_path = "tests/files/valid-config.yaml" + self.assertTrue(os.path.exists(config_path)) + + with sdhome() as home: + with unittest.mock.patch( + "sys.stdin", new_callable=lambda: io.StringIO("") + ) as mock_stdin, unittest.mock.patch( + "sys.stdout", new_callable=io.StringIO + ) as mock_stdout, unittest.mock.patch( + "sys.argv", new_callable=lambda: ["sd-proxy", config_path] + ) as mock_argv: + entrypoint.start() + output = mock_stdout.getvalue() + + response = json.loads(output) + self.assertEqual(response["status"], http.HTTPStatus.BAD_REQUEST) + body = json.loads(response["body"]) + self.assertEqual( + body["error"], "Invalid JSON in request" + ) + + @vcr.use_cassette("fixtures/main_json_response.yaml") + def test_json_response(self): + config_path = "tests/files/valid-config.yaml" + self.assertTrue(os.path.exists(config_path)) + + test_input = { + "method": "GET", + "path_query": "/posts?userId=1", + } + + output = None + with sdhome() as home, unittest.mock.patch( + "sys.stdin", new_callable=lambda: io.StringIO(json.dumps(test_input)) + ) as mock_stding, unittest.mock.patch( + "sys.stdout", new_callable=io.StringIO + ) as mock_stdout, unittest.mock.patch( + "sys.argv", new_callable=lambda: ["sd-proxy", config_path] + ) as mock_argv: + entrypoint.start() + output = mock_stdout.getvalue() + + response = json.loads(output) + self.assertEqual(response["status"], http.HTTPStatus.OK) diff --git a/tests/test_main.py b/tests/test_main.py index 8a4a28e..1c49461 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,3 +1,4 @@ +import http from io import StringIO import json import subprocess @@ -5,6 +6,8 @@ import unittest import uuid +import vcr + from securedrop_proxy import config from securedrop_proxy import main from securedrop_proxy import proxy @@ -18,6 +21,7 @@ def setUp(self): self.conf.port = 443 self.conf.dev = True + @vcr.use_cassette('fixtures/main_json_response.yaml') def test_json_response(self): test_input_json = """{ "method": "GET", "path_query": "/posts?userId=1" }""" @@ -32,12 +36,10 @@ def on_save(res, fh, conf): pass def on_done(res): - res = res.__dict__ - self.assertEqual(res['status'], 200) + self.assertEqual(res.status, http.HTTPStatus.OK) + print(json.dumps(res.__dict__)) - self.p = proxy.Proxy(self.conf, req, on_save) - self.p.on_done = on_done - self.p.proxy() + self.p = proxy.Proxy(self.conf, req, on_save, on_done) saved_stdout = sys.stdout try: @@ -52,6 +54,7 @@ def on_done(res): for item in json.loads(response['body']): self.assertEqual(item['userId'], 1) + @vcr.use_cassette('fixtures/main_non_json_response.yaml') def test_non_json_response(self): test_input_json = """{ "method": "GET", "path_query": "" }""" @@ -66,7 +69,6 @@ def on_save(fh, res, conf): res.body = json.dumps({'filename': self.fn}) self.p = proxy.Proxy(self.conf, proxy.Req(), on_save) - self.p.proxy() saved_stdout = sys.stdout try: @@ -88,9 +90,9 @@ def on_save(fh, res, conf): saved_file = f.read() # We expect HTML content in the file from the test data - self.assertIn("", saved_file) + self.assertIn("", saved_file) - def test_error_response(self): + def test_input_invalid_json(self): test_input_json = """"foo": "bar", "baz": "bliff" }""" def on_save(fh, res, conf): @@ -101,9 +103,66 @@ def on_done(res): self.assertEqual(res['status'], 400) sys.exit(1) - self.p = proxy.Proxy(self.conf, proxy.Req(), on_save) - self.p.on_done = on_done + p = proxy.Proxy(self.conf, proxy.Req(), on_save, on_done) with self.assertRaises(SystemExit): - self.p.proxy() - main.__main__(test_input_json, self.p) + main.__main__(test_input_json, p) + + def test_input_missing_keys(self): + test_input_json = """{ "foo": "bar", "baz": "bliff" }""" + + def on_save(fh, res, conf): + pass + + def on_done(res): + res = res.__dict__ + self.assertEqual(res['status'], 400) + self.assertEqual(res['body'], '{"error": "Missing keys in request"}') + sys.exit(1) + + p = proxy.Proxy(self.conf, proxy.Req(), on_save, on_done) + with self.assertRaises(SystemExit): + main.__main__(test_input_json, p) + + @vcr.use_cassette('fixtures/main_json_response.yaml') + def test_input_headers(self): + test_input = { + "method": "GET", + "path_query": "/posts?userId=1", + "headers": { "X-Test-Header": "th" } + } + + def on_save(fh, res, conf): + pass + + p = proxy.Proxy(self.conf, proxy.Req(), on_save) + main.__main__(json.dumps(test_input), p) + self.assertEqual(p.req.headers, test_input["headers"]) + + @vcr.use_cassette('fixtures/main_input_body.yaml') + def test_input_body(self): + test_input = { + "method": "POST", + "path_query": "/posts", + "body": { "id": 42, "title": "test" } + } + + def on_save(fh, res, conf): + pass + + p = proxy.Proxy(self.conf, proxy.Req(), on_save) + main.__main__(json.dumps(test_input), p) + self.assertEqual(p.req.body, test_input["body"]) + + @vcr.use_cassette('fixtures/main_non_json_response.yaml') + def test_default_callbacks(self): + test_input = { + "method": "GET", + "path_query": "", + } + + p = proxy.Proxy(self.conf, proxy.Req()) + with unittest.mock.patch("securedrop_proxy.callbacks.on_done") as on_done, unittest.mock.patch("securedrop_proxy.callbacks.on_save") as on_save: + main.__main__(json.dumps(test_input), p) + self.assertEqual(on_save.call_count, 1) + self.assertEqual(on_done.call_count, 1) diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 6278dce..bc73b05 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -1,8 +1,12 @@ +import http import json -import vcr import unittest import uuid +import requests +import vcr + +from securedrop_proxy import callbacks from securedrop_proxy import proxy from securedrop_proxy import config from securedrop_proxy import version @@ -136,7 +140,7 @@ def test_proxy_400_no_handler(self): self.assertEqual(p.res.status, 400) self.assertEqual(p.res.headers['Content-Type'], 'application/json') - self.assertIn('Request callback is not set', + self.assertIn('Request on_save callback is not set', p.res.body) @@ -167,3 +171,127 @@ def test_proxy_500_misconfiguration(self): self.assertEqual(p.res.headers['Content-Type'], 'application/json') self.assertIn('Proxy error while generating URL to request', p.res.body) + + +class TestServerErrorHandling(unittest.TestCase): + def setUp(self): + self.conf = config.Conf() + self.conf.host = "localhost" + self.conf.scheme = "http" + self.conf.port = 8000 + + def make_request(self, method="GET", path_query="/", headers=None): + req = proxy.Req() + req.method = method if method else "GET" + req.path_query = path_query if path_query else "/" + req.headers = headers if headers else {"Accept": "application/json"} + return req + + def test_cannot_connect(self): + """ + Test for "502 Bad Gateway" when the server can't be reached. + """ + req = self.make_request() + + conf = config.Conf() + conf.host = "sdproxytest.local" + conf.scheme = "https" + conf.port = 8000 + + p = proxy.Proxy(conf, req, on_save=callbacks.on_save) + p.proxy() + + self.assertEqual(p.res.status, http.HTTPStatus.BAD_GATEWAY) + self.assertIn("application/json", p.res.headers["Content-Type"]) + body = json.loads(p.res.body) + self.assertEqual(body["error"], "could not connect to server") + + def test_server_timeout(self): + """ + Test for "504 Gateway Timeout" when the server times out. + """ + class TimeoutProxy(proxy.Proxy): + """ + Mocks a slow upstream server. + + VCR cassettes cannot represent a request that takes too + long. This Proxy subclass raises the exception that would + cause. + """ + def prep_request(self): + raise requests.exceptions.Timeout('test timeout') + + req = self.make_request(path_query="/tarpit") + p = TimeoutProxy(self.conf, req, on_save=callbacks.on_save, timeout=0.00001) + p.proxy() + + self.assertEqual(p.res.status, http.HTTPStatus.GATEWAY_TIMEOUT) + self.assertIn("application/json", p.res.headers["Content-Type"]) + body = json.loads(p.res.body) + self.assertEqual(body["error"], "request timed out") + + @vcr.use_cassette("fixtures/proxy_bad_request.yaml") + def test_bad_request(self): + """ + Test handling of "400 Bad Request" from the server. + """ + req = self.make_request(path_query="/bad") + p = proxy.Proxy(self.conf, req, on_save=callbacks.on_save) + p.proxy() + + self.assertEqual(p.res.status, http.HTTPStatus.BAD_REQUEST) + self.assertIn("application/json", p.res.headers["Content-Type"]) + body = json.loads(p.res.body) + self.assertEqual(body["error"], http.HTTPStatus.BAD_REQUEST.phrase.lower()) + + @vcr.use_cassette("fixtures/proxy_unofficial_status.yaml") + def test_unofficial_status(self): + """ + Make sure we handle unofficial status codes from the server. + + Should the server ever need to return a status code not in + Python's http.HTTPStatus, the proxy should still return a + proper JSON error response with a generic error message. + """ + req = self.make_request(path_query="/teapot") + p = proxy.Proxy(self.conf, req, on_save=callbacks.on_save) + p.proxy() + + self.assertEqual(p.res.status, 418) + self.assertIn("application/json", p.res.headers["Content-Type"]) + body = json.loads(p.res.body) + self.assertEqual(body["error"], "unspecified server error") + + @vcr.use_cassette("fixtures/proxy_internal_server_error.yaml") + def test_internal_server_error(self): + """ + Test handling of "500 Internal Server Error" from the server. + """ + req = self.make_request(path_query="/crash") + p = proxy.Proxy(self.conf, req, on_save=callbacks.on_save) + p.proxy() + + self.assertEqual(p.res.status, http.HTTPStatus.INTERNAL_SERVER_ERROR) + self.assertIn("application/json", p.res.headers["Content-Type"]) + body = json.loads(p.res.body) + self.assertEqual( + body["error"], + http.HTTPStatus.INTERNAL_SERVER_ERROR.phrase.lower() + ) + + @vcr.use_cassette("fixtures/proxy_internal_error.yaml") + def test_internal_error(self): + """ + Ensure that the proxy returns JSON despite internal errors. + """ + def bad_on_save(self, fh, res, conf): + raise Exception("test internal proxy error") + + req = self.make_request() + p = proxy.Proxy(self.conf, req, on_save=bad_on_save) + p.proxy() + + self.assertEqual(p.res.status, http.HTTPStatus.INTERNAL_SERVER_ERROR) + self.assertIn("application/json", p.res.headers["Content-Type"]) + body = json.loads(p.res.body) + self.assertEqual(body["error"], "internal proxy error") From e2e9bf3ebed4a4803d45d984ad1466cf47419a07 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Wed, 18 Dec 2019 10:45:04 -0500 Subject: [PATCH 2/4] Address review Give test_main.test_input_headers its own fixture. Use "if not callback" instead of "if callback is None". --- fixtures/main_input_headers.yaml | 94 ++++++++++++++++++++++++++++++++ securedrop_proxy/main.py | 2 +- securedrop_proxy/proxy.py | 2 +- tests/test_main.py | 2 +- 4 files changed, 97 insertions(+), 3 deletions(-) create mode 100644 fixtures/main_input_headers.yaml diff --git a/fixtures/main_input_headers.yaml b/fixtures/main_input_headers.yaml new file mode 100644 index 0000000..606bfa7 --- /dev/null +++ b/fixtures/main_input_headers.yaml @@ -0,0 +1,94 @@ +interactions: +- request: + body: null + headers: + X-Test-Header: + - th + method: GET + uri: https://jsonplaceholder.typicode.com/posts?userId=1 + response: + body: + string: "[\n {\n \"userId\": 1,\n \"id\": 1,\n \"title\": \"sunt aut\ + \ facere repellat provident occaecati excepturi optio reprehenderit\",\n \ + \ \"body\": \"quia et suscipit\\nsuscipit recusandae consequuntur expedita\ + \ et cum\\nreprehenderit molestiae ut ut quas totam\\nnostrum rerum est autem\ + \ sunt rem eveniet architecto\"\n },\n {\n \"userId\": 1,\n \"id\"\ + : 2,\n \"title\": \"qui est esse\",\n \"body\": \"est rerum tempore\ + \ vitae\\nsequi sint nihil reprehenderit dolor beatae ea dolores neque\\nfugiat\ + \ blanditiis voluptate porro vel nihil molestiae ut reiciendis\\nqui aperiam\ + \ non debitis possimus qui neque nisi nulla\"\n },\n {\n \"userId\":\ + \ 1,\n \"id\": 3,\n \"title\": \"ea molestias quasi exercitationem repellat\ + \ qui ipsa sit aut\",\n \"body\": \"et iusto sed quo iure\\nvoluptatem\ + \ occaecati omnis eligendi aut ad\\nvoluptatem doloribus vel accusantium quis\ + \ pariatur\\nmolestiae porro eius odio et labore et velit aut\"\n },\n {\n\ + \ \"userId\": 1,\n \"id\": 4,\n \"title\": \"eum et est occaecati\"\ + ,\n \"body\": \"ullam et saepe reiciendis voluptatem adipisci\\nsit amet\ + \ autem assumenda provident rerum culpa\\nquis hic commodi nesciunt rem tenetur\ + \ doloremque ipsam iure\\nquis sunt voluptatem rerum illo velit\"\n },\n\ + \ {\n \"userId\": 1,\n \"id\": 5,\n \"title\": \"nesciunt quas odio\"\ + ,\n \"body\": \"repudiandae veniam quaerat sunt sed\\nalias aut fugiat\ + \ sit autem sed est\\nvoluptatem omnis possimus esse voluptatibus quis\\nest\ + \ aut tenetur dolor neque\"\n },\n {\n \"userId\": 1,\n \"id\": 6,\n\ + \ \"title\": \"dolorem eum magni eos aperiam quia\",\n \"body\": \"\ + ut aspernatur corporis harum nihil quis provident sequi\\nmollitia nobis aliquid\ + \ molestiae\\nperspiciatis et ea nemo ab reprehenderit accusantium quas\\\ + nvoluptate dolores velit et doloremque molestiae\"\n },\n {\n \"userId\"\ + : 1,\n \"id\": 7,\n \"title\": \"magnam facilis autem\",\n \"body\"\ + : \"dolore placeat quibusdam ea quo vitae\\nmagni quis enim qui quis quo nemo\ + \ aut saepe\\nquidem repellat excepturi ut quia\\nsunt ut sequi eos ea sed\ + \ quas\"\n },\n {\n \"userId\": 1,\n \"id\": 8,\n \"title\": \"\ + dolorem dolore est ipsam\",\n \"body\": \"dignissimos aperiam dolorem qui\ + \ eum\\nfacilis quibusdam animi sint suscipit qui sint possimus cum\\nquaerat\ + \ magni maiores excepturi\\nipsam ut commodi dolor voluptatum modi aut vitae\"\ + \n },\n {\n \"userId\": 1,\n \"id\": 9,\n \"title\": \"nesciunt\ + \ iure omnis dolorem tempora et accusantium\",\n \"body\": \"consectetur\ + \ animi nesciunt iure dolore\\nenim quia ad\\nveniam autem ut quam aut nobis\\\ + net est aut quod aut provident voluptas autem voluptas\"\n },\n {\n \"\ + userId\": 1,\n \"id\": 10,\n \"title\": \"optio molestias id quia eum\"\ + ,\n \"body\": \"quo et expedita modi cum officia vel magni\\ndoloribus\ + \ qui repudiandae\\nvero nisi sit\\nquos veniam quod sed accusamus veritatis\ + \ error\"\n }\n]" + headers: + Access-Control-Allow-Credentials: + - 'true' + Age: + - '1789' + CF-Cache-Status: + - HIT + CF-RAY: + - 54722d954de5e0ea-IAD + Cache-Control: + - max-age=14400 + Connection: + - keep-alive + Content-Type: + - application/json; charset=utf-8 + Date: + - Wed, 18 Dec 2019 15:30:26 GMT + Etag: + - W/"aa6-j2NSH739l9uq40OywFMn7Y0C/iY" + Expect-CT: + - max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct" + Expires: + - '-1' + Pragma: + - no-cache + Server: + - cloudflare + Set-Cookie: + - __cfduid=d21e159ad4987bc5ba9d781aed2f9db5c1576683026; expires=Fri, 17-Jan-20 + 15:30:26 GMT; path=/; domain=.typicode.com; HttpOnly; SameSite=Lax + Transfer-Encoding: + - chunked + Vary: + - Origin, Accept-Encoding + Via: + - 1.1 vegur + X-Content-Type-Options: + - nosniff + X-Powered-By: + - Express + status: + code: 200 + message: OK +version: 1 diff --git a/securedrop_proxy/main.py b/securedrop_proxy/main.py index 8986449..e67f158 100644 --- a/securedrop_proxy/main.py +++ b/securedrop_proxy/main.py @@ -39,6 +39,6 @@ def __main__(incoming, p): req.body = client_req['body'] p.req = req - if p.on_save is None: + if not p.on_save: p.on_save = callbacks.on_save p.proxy() diff --git a/securedrop_proxy/proxy.py b/securedrop_proxy/proxy.py index 59a9a3a..dcdcbc0 100644 --- a/securedrop_proxy/proxy.py +++ b/securedrop_proxy/proxy.py @@ -135,7 +135,7 @@ def handle_response(self): def proxy(self): try: - if self.on_save is None: + if not self.on_save: self.simple_error( http.HTTPStatus.BAD_REQUEST, "Request on_save callback is not set." ) diff --git a/tests/test_main.py b/tests/test_main.py index 1c49461..0559518 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -124,7 +124,7 @@ def on_done(res): with self.assertRaises(SystemExit): main.__main__(test_input_json, p) - @vcr.use_cassette('fixtures/main_json_response.yaml') + @vcr.use_cassette('fixtures/main_input_headers.yaml') def test_input_headers(self): test_input = { "method": "GET", From 8cb672b3af7f8f92e654e5f665c6216c3955bebf Mon Sep 17 00:00:00 2001 From: Kushal Das Date: Fri, 3 Jan 2020 14:34:33 +0530 Subject: [PATCH 3/4] Fixes lint issues in test files --- tests/test_entrypoint.py | 29 ++++++++++++----------- tests/test_main.py | 50 ++++++++++++++++++++++------------------ 2 files changed, 42 insertions(+), 37 deletions(-) diff --git a/tests/test_entrypoint.py b/tests/test_entrypoint.py index 13b1d04..8a5b0ea 100644 --- a/tests/test_entrypoint.py +++ b/tests/test_entrypoint.py @@ -3,7 +3,6 @@ import io import json import os -import sys import tempfile import unittest.mock @@ -33,7 +32,9 @@ def test_missing_config(self): output = None with unittest.mock.patch( "sys.argv", new_callable=lambda: ["sd-proxy", config_path] - ) as mock_argv, unittest.mock.patch("sys.stdout", new_callable=io.StringIO) as mock_stdout: + ) as mock_argv, unittest.mock.patch( # noqa: F841 + "sys.stdout", new_callable=io.StringIO + ) as mock_stdout: with self.assertRaises(SystemExit), sdhome(): entrypoint.start() output = mock_stdout.getvalue() @@ -52,7 +53,9 @@ def test_unwritable_log_folder(self): output = None with sdhome() as home: os.chmod(home, 0o0444) - with unittest.mock.patch("sys.stdout", new_callable=io.StringIO) as mock_stdout: + with unittest.mock.patch( + "sys.stdout", new_callable=io.StringIO + ) as mock_stdout: with self.assertRaises(SystemExit): entrypoint.start() output = mock_stdout.getvalue() @@ -64,10 +67,10 @@ def test_unwritable_log_folder(self): self.assertIn("Permission denied: ", body["error"]) def test_wrong_number_of_arguments(self): - with sdhome() as home: + with sdhome() as home: # noqa: F841 with unittest.mock.patch( "sys.argv", new_callable=lambda: ["sd-proxy"] - ) as mock_argv, unittest.mock.patch( + ) as mock_argv, unittest.mock.patch( # noqa: F841 "sys.stdout", new_callable=io.StringIO ) as mock_stdout: with self.assertRaises(SystemExit): @@ -85,23 +88,21 @@ def test_empty_input(self): config_path = "tests/files/valid-config.yaml" self.assertTrue(os.path.exists(config_path)) - with sdhome() as home: + with sdhome() as home: # noqa: F841 with unittest.mock.patch( "sys.stdin", new_callable=lambda: io.StringIO("") - ) as mock_stdin, unittest.mock.patch( + ) as mock_stdin, unittest.mock.patch( # noqa: F841 "sys.stdout", new_callable=io.StringIO ) as mock_stdout, unittest.mock.patch( "sys.argv", new_callable=lambda: ["sd-proxy", config_path] - ) as mock_argv: + ) as mock_argv: # noqa: F841 entrypoint.start() output = mock_stdout.getvalue() response = json.loads(output) self.assertEqual(response["status"], http.HTTPStatus.BAD_REQUEST) body = json.loads(response["body"]) - self.assertEqual( - body["error"], "Invalid JSON in request" - ) + self.assertEqual(body["error"], "Invalid JSON in request") @vcr.use_cassette("fixtures/main_json_response.yaml") def test_json_response(self): @@ -114,13 +115,13 @@ def test_json_response(self): } output = None - with sdhome() as home, unittest.mock.patch( + with sdhome() as home, unittest.mock.patch( # noqa: F841 "sys.stdin", new_callable=lambda: io.StringIO(json.dumps(test_input)) - ) as mock_stding, unittest.mock.patch( + ) as mock_stding, unittest.mock.patch( # noqa: F841 "sys.stdout", new_callable=io.StringIO ) as mock_stdout, unittest.mock.patch( "sys.argv", new_callable=lambda: ["sd-proxy", config_path] - ) as mock_argv: + ) as mock_argv: # noqa: F841 entrypoint.start() output = mock_stdout.getvalue() diff --git a/tests/test_main.py b/tests/test_main.py index 0559518..c4e10e2 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -16,20 +16,20 @@ class TestMain(unittest.TestCase): def setUp(self): self.conf = config.Conf() - self.conf.host = 'jsonplaceholder.typicode.com' - self.conf.scheme = 'https' + self.conf.host = "jsonplaceholder.typicode.com" + self.conf.scheme = "https" self.conf.port = 443 self.conf.dev = True - @vcr.use_cassette('fixtures/main_json_response.yaml') + @vcr.use_cassette("fixtures/main_json_response.yaml") def test_json_response(self): test_input_json = """{ "method": "GET", "path_query": "/posts?userId=1" }""" req = proxy.Req() - req.method = 'GET' - req.path_query = '' - req.headers = {'Accept': 'application/json'} + req.method = "GET" + req.path_query = "" + req.headers = {"Accept": "application/json"} # Use custom callbacks def on_save(res, fh, conf): @@ -51,10 +51,10 @@ def on_done(res): sys.stdout = saved_stdout response = json.loads(output) - for item in json.loads(response['body']): - self.assertEqual(item['userId'], 1) + for item in json.loads(response["body"]): + self.assertEqual(item["userId"], 1) - @vcr.use_cassette('fixtures/main_non_json_response.yaml') + @vcr.use_cassette("fixtures/main_non_json_response.yaml") def test_non_json_response(self): test_input_json = """{ "method": "GET", "path_query": "" }""" @@ -64,9 +64,9 @@ def on_save(fh, res, conf): subprocess.run(["cp", fh.name, "/tmp/{}".format(self.fn)]) - res.headers['X-Origin-Content-Type'] = res.headers['Content-Type'] - res.headers['Content-Type'] = 'application/json' - res.body = json.dumps({'filename': self.fn}) + res.headers["X-Origin-Content-Type"] = res.headers["Content-Type"] + res.headers["Content-Type"] = "application/json" + res.body = json.dumps({"filename": self.fn}) self.p = proxy.Proxy(self.conf, proxy.Req(), on_save) @@ -80,10 +80,10 @@ def on_save(fh, res, conf): sys.stdout = saved_stdout response = json.loads(output) - self.assertEqual(response['status'], 200) + self.assertEqual(response["status"], 200) # The proxy should have created a filename in the response body - self.assertIn('filename', response['body']) + self.assertIn("filename", response["body"]) # The file should not be empty with open("/tmp/{}".format(self.fn)) as f: @@ -100,7 +100,7 @@ def on_save(fh, res, conf): def on_done(res): res = res.__dict__ - self.assertEqual(res['status'], 400) + self.assertEqual(res["status"], 400) sys.exit(1) p = proxy.Proxy(self.conf, proxy.Req(), on_save, on_done) @@ -116,20 +116,20 @@ def on_save(fh, res, conf): def on_done(res): res = res.__dict__ - self.assertEqual(res['status'], 400) - self.assertEqual(res['body'], '{"error": "Missing keys in request"}') + self.assertEqual(res["status"], 400) + self.assertEqual(res["body"], '{"error": "Missing keys in request"}') sys.exit(1) p = proxy.Proxy(self.conf, proxy.Req(), on_save, on_done) with self.assertRaises(SystemExit): main.__main__(test_input_json, p) - @vcr.use_cassette('fixtures/main_input_headers.yaml') + @vcr.use_cassette("fixtures/main_input_headers.yaml") def test_input_headers(self): test_input = { "method": "GET", "path_query": "/posts?userId=1", - "headers": { "X-Test-Header": "th" } + "headers": {"X-Test-Header": "th"}, } def on_save(fh, res, conf): @@ -139,12 +139,12 @@ def on_save(fh, res, conf): main.__main__(json.dumps(test_input), p) self.assertEqual(p.req.headers, test_input["headers"]) - @vcr.use_cassette('fixtures/main_input_body.yaml') + @vcr.use_cassette("fixtures/main_input_body.yaml") def test_input_body(self): test_input = { "method": "POST", "path_query": "/posts", - "body": { "id": 42, "title": "test" } + "body": {"id": 42, "title": "test"}, } def on_save(fh, res, conf): @@ -154,7 +154,7 @@ def on_save(fh, res, conf): main.__main__(json.dumps(test_input), p) self.assertEqual(p.req.body, test_input["body"]) - @vcr.use_cassette('fixtures/main_non_json_response.yaml') + @vcr.use_cassette("fixtures/main_non_json_response.yaml") def test_default_callbacks(self): test_input = { "method": "GET", @@ -162,7 +162,11 @@ def test_default_callbacks(self): } p = proxy.Proxy(self.conf, proxy.Req()) - with unittest.mock.patch("securedrop_proxy.callbacks.on_done") as on_done, unittest.mock.patch("securedrop_proxy.callbacks.on_save") as on_save: + with unittest.mock.patch( + "securedrop_proxy.callbacks.on_done" + ) as on_done, unittest.mock.patch( + "securedrop_proxy.callbacks.on_save" + ) as on_save: main.__main__(json.dumps(test_input), p) self.assertEqual(on_save.call_count, 1) self.assertEqual(on_done.call_count, 1) From 97eed4b5341c27e37e395885c06c3f9380239efa Mon Sep 17 00:00:00 2001 From: Kushal Das Date: Fri, 3 Jan 2020 14:43:00 +0530 Subject: [PATCH 4/4] Fixes mypy error on redefining attribute --- securedrop_proxy/proxy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/securedrop_proxy/proxy.py b/securedrop_proxy/proxy.py index dcdcbc0..733d2ed 100644 --- a/securedrop_proxy/proxy.py +++ b/securedrop_proxy/proxy.py @@ -35,14 +35,14 @@ def __init__(self, conf=None, req=Req(), on_save=None, on_done=None, timeout: fl self.req = req self.res = None self.on_save = on_save - if on_done is not None: + if on_done: self.on_done = on_done self.timeout = float(timeout) if timeout else 10 self._prepared_request = None - def on_done(self, res): + def on_done(self, res): # type: ignore callbacks.on_done(res) @staticmethod