freedomofpress / securedrop-client

a Qt-based GUI for SecureDrop journalists 📰🗞️
GNU Affero General Public License v3.0
40 stars 39 forks source link

`API._streaming_download()` does not handle HTTP `416` `Range Not Satisfiable` errors #2232

Open cfm opened 1 day ago

cfm commented 1 day ago

Description

API._streaming_download() chokes on HTTP 416 Range Not Satisfiable errors, because the JSON error object may be received in the first chunk of any request, including any retry, not only at the beginning of the accumulated response.

Steps to Reproduce

--- a/client/securedrop_client/sdk/__init__.py
+++ b/client/securedrop_client/sdk/__init__.py
@@ -153,7 +153,7 @@ class API:
             try:
                 # Update the range request if we're retrying
                 if retry > 0:
-                    data["headers"]["Range"] = f"bytes={bytes_written}-"
+                    data["headers"]["Range"] = f"bytes={bytes_written}-{bytes_written-1}"
                     logger.debug(f"Retry {retry}, range: {bytes_written}-")

                 # Serialize the data to send

Expected Behavior

The SDK handles the error gracefully.

Actual Behavior

        filepath = os.path.join(path, submission.filename)
>       Path(filepath).write_bytes(response.contents)
E       AttributeError: 'NoneType' object has no attribute 'contents'

securedrop_client/sdk/__init__.py:699: AttributeError
legoktm commented 1 day ago

Nice catch. Is this just applicable to 416 or could this be literally any error if e.g. the server returns a 504?

Ideally this would've been a type error caught by mypy...

cfm commented 1 day ago

Yes, you're right: any retry-level error fails this way, and we don't handle HTTP 416 anywhere.

I was going to ask you about mocking Popen.stdin for the purpose of mangling a Range header to get a real HTTP 416 cassette, but I think I'll give pytest-subprocess a try first.