From f0c00fcbe41dcde78790dddbcac599699faa5d9c Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 2 Feb 2022 11:04:16 -0500
Subject: [PATCH 01/25] News file.

---
 newsfragments/3860.minor | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 newsfragments/3860.minor

diff --git a/newsfragments/3860.minor b/newsfragments/3860.minor
new file mode 100644
index 000000000..e69de29bb

From bceed6e19984418745c077d0f04993fb994b296c Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 2 Feb 2022 11:52:31 -0500
Subject: [PATCH 02/25] More bucket allocation logic.

---
 src/allmydata/storage/http_server.py    | 72 +++++++++++++------------
 src/allmydata/test/test_storage_http.py | 50 +++++++++++++++++
 2 files changed, 89 insertions(+), 33 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index f885baa22..50aa6ae9e 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -128,10 +128,15 @@ class StorageIndexUploads(object):
     """
 
     # Map share number to BucketWriter
-    shares = attr.ib()  # type: Dict[int,BucketWriter]
+    shares = attr.ib(factory=dict)  # type: Dict[int,BucketWriter]
 
-    # The upload key.
-    upload_secret = attr.ib()  # type: bytes
+    # Mape share number to the upload secret (different shares might have
+    # different upload secrets).
+    upload_secrets = attr.ib(factory=dict)  # type: Dict[int,bytes]
+
+    def add_upload(self, share_number, upload_secret, bucket):
+        self.shares[share_number] = bucket
+        self.upload_secrets[share_number] = upload_secret
 
 
 class HTTPServer(object):
@@ -179,39 +184,40 @@ class HTTPServer(object):
     def allocate_buckets(self, request, authorization, storage_index):
         """Allocate buckets."""
         storage_index = si_a2b(storage_index.encode("ascii"))
-        info = loads(request.content.read())
         upload_secret = authorization[Secrets.UPLOAD]
+        info = loads(request.content.read())
 
         if storage_index in self._uploads:
-            # Pre-existing upload.
-            in_progress = self._uploads[storage_index]
-            if timing_safe_compare(in_progress.upload_secret, upload_secret):
-                # Same session.
-                # TODO add BucketWriters only for new shares that don't already have buckets; see the HTTP spec for details.
-                # The backend code may already implement this logic.
-                pass
-            else:
-                # TODO Fail, since the secret doesnt match.
-                pass
-        else:
-            # New upload.
-            already_got, sharenum_to_bucket = self._storage_server.allocate_buckets(
-                storage_index,
-                renew_secret=authorization[Secrets.LEASE_RENEW],
-                cancel_secret=authorization[Secrets.LEASE_CANCEL],
-                sharenums=info["share-numbers"],
-                allocated_size=info["allocated-size"],
-            )
-            self._uploads[storage_index] = StorageIndexUploads(
-                shares=sharenum_to_bucket, upload_secret=authorization[Secrets.UPLOAD]
-            )
-            return self._cbor(
-                request,
-                {
-                    "already-have": set(already_got),
-                    "allocated": set(sharenum_to_bucket),
-                },
-            )
+            for share_number in info["share-numbers"]:
+                in_progress = self._uploads[storage_index]
+                # For pre-existing upload, make sure password matches.
+                if (
+                    share_number in in_progress.upload_secrets
+                    and not timing_safe_compare(
+                        in_progress.upload_secrets[share_number], upload_secret
+                    )
+                ):
+                    request.setResponseCode(http.UNAUTHORIZED)
+                    return b""
+
+        already_got, sharenum_to_bucket = self._storage_server.allocate_buckets(
+            storage_index,
+            renew_secret=authorization[Secrets.LEASE_RENEW],
+            cancel_secret=authorization[Secrets.LEASE_CANCEL],
+            sharenums=info["share-numbers"],
+            allocated_size=info["allocated-size"],
+        )
+        uploads = self._uploads.setdefault(storage_index, StorageIndexUploads())
+        for share_number, bucket in sharenum_to_bucket.items():
+            uploads.add_upload(share_number, upload_secret, bucket)
+
+        return self._cbor(
+            request,
+            {
+                "already-have": set(already_got),
+                "allocated": set(sharenum_to_bucket),
+            },
+        )
 
     @_authorized_route(
         _app,
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 982e22859..39f07a54a 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -24,6 +24,7 @@ from klein import Klein
 from hyperlink import DecodedURL
 from collections_extended import RangeMap
 from twisted.internet.task import Clock
+from twisted.web import http
 
 from .common import SyncTestCase
 from ..storage.server import StorageServer
@@ -386,6 +387,55 @@ class ImmutableHTTPAPITests(SyncTestCase):
             )
             self.assertEqual(downloaded, expected_data[offset : offset + length])
 
+    def test_allocate_buckets_second_time_wrong_upload_key(self):
+        """
+        If allocate buckets endpoint is called second time with wrong upload
+        key on the same shares, the result is an error.
+        """
+        im_client = StorageClientImmutables(self.http.client)
+
+        # Create a upload:
+        upload_secret = urandom(32)
+        lease_secret = urandom(32)
+        storage_index = b"".join(bytes([i]) for i in range(16))
+        result_of(
+            im_client.create(
+                storage_index, {1, 2, 3}, 100, upload_secret, lease_secret, lease_secret
+            )
+        )
+        with self.assertRaises(ClientException) as e:
+            result_of(
+                im_client.create(
+                    storage_index, {2, 3}, 100, b"x" * 32, lease_secret, lease_secret
+                )
+            )
+        self.assertEqual(e.exception.args[0], http.UNAUTHORIZED)
+
+    def test_allocate_buckets_second_time_different_shares(self):
+        """
+        If allocate buckets endpoint is called second time with different
+        upload key on different shares, that creates the buckets.
+        """
+        im_client = StorageClientImmutables(self.http.client)
+
+        # Create a upload:
+        upload_secret = urandom(32)
+        lease_secret = urandom(32)
+        storage_index = b"".join(bytes([i]) for i in range(16))
+        result_of(
+            im_client.create(
+                storage_index, {1, 2, 3}, 100, upload_secret, lease_secret, lease_secret
+            )
+        )
+
+        # Add same shares:
+        created2 = result_of(
+            im_client.create(
+                storage_index, {4, 6}, 100, b"x" * 2, lease_secret, lease_secret
+            )
+        )
+        self.assertEqual(created2.allocated, {4, 6})
+
     def test_list_shares(self):
         """
         Once a share is finished uploading, it's possible to list it.

From 39fe48b1746d5d854cdd87129112430332265bba Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 2 Feb 2022 12:55:41 -0500
Subject: [PATCH 03/25] More passing IStorageServer tests.

---
 src/allmydata/storage_client.py           | 10 ++++++----
 src/allmydata/test/test_istorageserver.py |  3 ---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py
index e7dbb27a8..665cfd513 100644
--- a/src/allmydata/storage_client.py
+++ b/src/allmydata/storage_client.py
@@ -1094,15 +1094,18 @@ class _HTTPBucketReader(object):
 class _HTTPStorageServer(object):
     """
     Talk to remote storage server over HTTP.
+
+    The same upload key is used for all communication.
     """
     _http_client = attr.ib(type=StorageClient)
+    _upload_secret = attr.ib(type=bytes)
 
     @staticmethod
     def from_http_client(http_client):  # type: (StorageClient) -> _HTTPStorageServer
         """
         Create an ``IStorageServer`` from a HTTP ``StorageClient``.
         """
-        return _HTTPStorageServer(http_client=http_client)
+        return _HTTPStorageServer(http_client=http_client, upload_secret=urandom(20))
 
     def get_version(self):
         return self._http_client.get_version()
@@ -1117,10 +1120,9 @@ class _HTTPStorageServer(object):
             allocated_size,
             canary,
     ):
-        upload_secret = urandom(20)
         immutable_client = StorageClientImmutables(self._http_client)
         result = immutable_client.create(
-            storage_index, sharenums, allocated_size, upload_secret, renew_secret,
+            storage_index, sharenums, allocated_size, self._upload_secret, renew_secret,
             cancel_secret
         )
         result = yield result
@@ -1130,7 +1132,7 @@ class _HTTPStorageServer(object):
                      client=immutable_client,
                      storage_index=storage_index,
                      share_number=share_num,
-                     upload_secret=upload_secret
+                     upload_secret=self._upload_secret
                  ))
                  for share_num in result.allocated
             })
diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py
index cf1d977d8..679b0f964 100644
--- a/src/allmydata/test/test_istorageserver.py
+++ b/src/allmydata/test/test_istorageserver.py
@@ -1148,13 +1148,10 @@ class HTTPImmutableAPIsTests(
         "test_add_lease_renewal",
         "test_add_new_lease",
         "test_advise_corrupt_share",
-        "test_allocate_buckets_repeat",
         "test_bucket_advise_corrupt_share",
         "test_disconnection",
-        "test_get_buckets_skips_unfinished_buckets",
         "test_matching_overlapping_writes",
         "test_non_matching_overlapping_writes",
-        "test_written_shares_are_allocated",
     }
 
 

From ce2468cdffb56705c7f9edde4581b7b675716117 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Fri, 4 Feb 2022 09:53:39 -0500
Subject: [PATCH 04/25] Validate inputs automatically as part of parsing.

---
 src/allmydata/storage/http_server.py    | 27 +++++++++------
 src/allmydata/test/test_storage_http.py | 45 +++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 50aa6ae9e..cf8284e3e 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -23,6 +23,7 @@ from klein import Klein
 from twisted.web import http
 import attr
 from werkzeug.http import parse_range_header, parse_content_range_header
+from werkzeug.routing import BaseConverter
 
 # TODO Make sure to use pure Python versions?
 from cbor2 import dumps, loads
@@ -32,6 +33,7 @@ from .http_common import swissnum_auth_header, Secrets
 from .common import si_a2b
 from .immutable import BucketWriter
 from ..util.hashutil import timing_safe_compare
+from ..util.base32 import rfc3548_alphabet
 
 
 class ClientSecretsException(Exception):
@@ -139,12 +141,22 @@ class StorageIndexUploads(object):
         self.upload_secrets[share_number] = upload_secret
 
 
+class StorageIndexConverter(BaseConverter):
+    """Parser/validator for storage index URL path segments."""
+
+    regex = "[" + str(rfc3548_alphabet, "ascii") + "]{26}"
+
+    def to_python(self, value):
+        return si_a2b(value.encode("ascii"))
+
+
 class HTTPServer(object):
     """
     A HTTP interface to the storage server.
     """
 
     _app = Klein()
+    _app.url_map.converters["storage_index"] = StorageIndexConverter
 
     def __init__(
         self, storage_server, swissnum
@@ -178,12 +190,11 @@ class HTTPServer(object):
     @_authorized_route(
         _app,
         {Secrets.LEASE_RENEW, Secrets.LEASE_CANCEL, Secrets.UPLOAD},
-        "/v1/immutable/<string:storage_index>",
+        "/v1/immutable/<storage_index:storage_index>",
         methods=["POST"],
     )
     def allocate_buckets(self, request, authorization, storage_index):
         """Allocate buckets."""
-        storage_index = si_a2b(storage_index.encode("ascii"))
         upload_secret = authorization[Secrets.UPLOAD]
         info = loads(request.content.read())
 
@@ -222,12 +233,11 @@ class HTTPServer(object):
     @_authorized_route(
         _app,
         {Secrets.UPLOAD},
-        "/v1/immutable/<string:storage_index>/<int:share_number>",
+        "/v1/immutable/<storage_index:storage_index>/<int(signed=False):share_number>",
         methods=["PATCH"],
     )
     def write_share_data(self, request, authorization, storage_index, share_number):
         """Write data to an in-progress immutable upload."""
-        storage_index = si_a2b(storage_index.encode("ascii"))
         content_range = parse_content_range_header(request.getHeader("content-range"))
         # TODO in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
         # 1. Malformed header should result in error 416
@@ -236,7 +246,7 @@ class HTTPServer(object):
         # 4. Impossible range should resul tin error 416
         offset = content_range.start
 
-        # TODO basic checks on validity of start, offset, and content-range in general. also of share_number.
+        # TODO basic checks on validity of start, offset, and content-range in general.
         # TODO basic check that body isn't infinite. require content-length? or maybe we should require content-range (it's optional now)? if so, needs to be rflected in protocol spec.
 
         data = request.content.read()
@@ -264,34 +274,31 @@ class HTTPServer(object):
     @_authorized_route(
         _app,
         set(),
-        "/v1/immutable/<string:storage_index>/shares",
+        "/v1/immutable/<storage_index:storage_index>/shares",
         methods=["GET"],
     )
     def list_shares(self, request, authorization, storage_index):
         """
         List shares for the given storage index.
         """
-        storage_index = si_a2b(storage_index.encode("ascii"))
         share_numbers = list(self._storage_server.get_buckets(storage_index).keys())
         return self._cbor(request, share_numbers)
 
     @_authorized_route(
         _app,
         set(),
-        "/v1/immutable/<string:storage_index>/<int:share_number>",
+        "/v1/immutable/<storage_index:storage_index>/<int(signed=False):share_number>",
         methods=["GET"],
     )
     def read_share_chunk(self, request, authorization, storage_index, share_number):
         """Read a chunk for an already uploaded immutable."""
         # TODO in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
-        # 1. basic checks on validity on storage index, share number
         # 2. missing range header should have response code 200 and return whole thing
         # 3. malformed range header should result in error? or return everything?
         # 4. non-bytes range results in error
         # 5. ranges make sense semantically (positive, etc.)
         # 6. multiple ranges fails with error
         # 7. missing end of range means "to the end of share"
-        storage_index = si_a2b(storage_index.encode("ascii"))
         range_header = parse_range_header(request.getHeader("range"))
         offset, end = range_header.ranges[0]
         assert end != None  # TODO support this case
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 39f07a54a..f78d83b27 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -25,6 +25,8 @@ from hyperlink import DecodedURL
 from collections_extended import RangeMap
 from twisted.internet.task import Clock
 from twisted.web import http
+from werkzeug import routing
+from werkzeug.exceptions import NotFound as WNotFound
 
 from .common import SyncTestCase
 from ..storage.server import StorageServer
@@ -34,6 +36,7 @@ from ..storage.http_server import (
     Secrets,
     ClientSecretsException,
     _authorized_route,
+    StorageIndexConverter,
 )
 from ..storage.http_client import (
     StorageClient,
@@ -42,6 +45,7 @@ from ..storage.http_client import (
     ImmutableCreateResult,
     UploadProgress,
 )
+from ..storage.common import si_b2a
 
 
 def _post_process(params):
@@ -148,6 +152,47 @@ class ExtractSecretsTests(SyncTestCase):
             _extract_secrets(["lease-cancel-secret eA=="], {Secrets.LEASE_RENEW})
 
 
+class RouteConverterTests(SyncTestCase):
+    """Tests for custom werkzeug path segment converters."""
+
+    adapter = routing.Map(
+        [
+            routing.Rule(
+                "/<storage_index:storage_index>/", endpoint="si", methods=["GET"]
+            )
+        ],
+        converters={"storage_index": StorageIndexConverter},
+    ).bind("example.com", "/")
+
+    @given(storage_index=st.binary(min_size=16, max_size=16))
+    def test_good_storage_index_is_parsed(self, storage_index):
+        """
+        A valid storage index is accepted and parsed back out by
+        StorageIndexConverter.
+        """
+        self.assertEqual(
+            self.adapter.match(
+                "/{}/".format(str(si_b2a(storage_index), "ascii")), method="GET"
+            ),
+            ("si", {"storage_index": storage_index}),
+        )
+
+    def test_long_storage_index_is_not_parsed(self):
+        """An overly long storage_index string is not parsed."""
+        with self.assertRaises(WNotFound):
+            self.adapter.match("/{}/".format("a" * 27), method="GET")
+
+    def test_short_storage_index_is_not_parsed(self):
+        """An overly short storage_index string is not parsed."""
+        with self.assertRaises(WNotFound):
+            self.adapter.match("/{}/".format("a" * 25), method="GET")
+
+    def test_bad_characters_storage_index_is_not_parsed(self):
+        """An overly short storage_index string is not parsed."""
+        with self.assertRaises(WNotFound):
+            self.adapter.match("/{}_/".format("a" * 25), method="GET")
+
+
 # TODO should be actual swissnum
 SWISSNUM_FOR_TEST = b"abcd"
 

From 7107a85fba4bb7c8e6998d48442a1caac987279a Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Tue, 8 Feb 2022 10:19:37 -0500
Subject: [PATCH 05/25] Refactor client, separating low-level and high-level
 concerns.

---
 src/allmydata/storage/http_client.py    | 35 ++++++++++++++++---------
 src/allmydata/test/test_storage_http.py | 18 ++++++++-----
 2 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py
index 2dc133b52..3b1dbfb30 100644
--- a/src/allmydata/storage/http_client.py
+++ b/src/allmydata/storage/http_client.py
@@ -68,7 +68,7 @@ class ImmutableCreateResult(object):
 
 class StorageClient(object):
     """
-    HTTP client that talks to the HTTP storage server.
+    Low-level HTTP client that talks to the HTTP storage server.
     """
 
     def __init__(
@@ -78,7 +78,7 @@ class StorageClient(object):
         self._swissnum = swissnum
         self._treq = treq
 
-    def _url(self, path):
+    def relative_url(self, path):
         """Get a URL relative to the base URL."""
         return self._base_url.click(path)
 
@@ -92,7 +92,7 @@ class StorageClient(object):
         )
         return headers
 
-    def _request(
+    def request(
         self,
         method,
         url,
@@ -120,13 +120,22 @@ class StorageClient(object):
             )
         return self._treq.request(method, url, headers=headers, **kwargs)
 
+
+class StorageClientGeneral(object):
+    """
+    High-level HTTP APIs that aren't immutable- or mutable-specific.
+    """
+
+    def __init__(self, client):  # type: (StorageClient) -> None
+        self._client = client
+
     @inlineCallbacks
     def get_version(self):
         """
         Return the version metadata for the server.
         """
-        url = self._url("/v1/version")
-        response = yield self._request("GET", url)
+        url = self._client.relative_url("/v1/version")
+        response = yield self._client.request("GET", url)
         decoded_response = yield _decode_cbor(response)
         returnValue(decoded_response)
 
@@ -174,11 +183,11 @@ class StorageClientImmutables(object):
         Result fires when creating the storage index succeeded, if creating the
         storage index failed the result will fire with an exception.
         """
-        url = self._client._url("/v1/immutable/" + _encode_si(storage_index))
+        url = self._client.relative_url("/v1/immutable/" + _encode_si(storage_index))
         message = dumps(
             {"share-numbers": share_numbers, "allocated-size": allocated_size}
         )
-        response = yield self._client._request(
+        response = yield self._client.request(
             "POST",
             url,
             lease_renew_secret=lease_renew_secret,
@@ -211,10 +220,10 @@ class StorageClientImmutables(object):
         whether the _complete_ share (i.e. all chunks, not just this one) has
         been uploaded.
         """
-        url = self._client._url(
+        url = self._client.relative_url(
             "/v1/immutable/{}/{}".format(_encode_si(storage_index), share_number)
         )
-        response = yield self._client._request(
+        response = yield self._client.request(
             "PATCH",
             url,
             upload_secret=upload_secret,
@@ -262,10 +271,10 @@ class StorageClientImmutables(object):
         the HTTP protocol will be simplified, see
         https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3777
         """
-        url = self._client._url(
+        url = self._client.relative_url(
             "/v1/immutable/{}/{}".format(_encode_si(storage_index), share_number)
         )
-        response = yield self._client._request(
+        response = yield self._client.request(
             "GET",
             url,
             headers=Headers(
@@ -285,10 +294,10 @@ class StorageClientImmutables(object):
         """
         Return the set of shares for a given storage index.
         """
-        url = self._client._url(
+        url = self._client.relative_url(
             "/v1/immutable/{}/shares".format(_encode_si(storage_index))
         )
-        response = yield self._client._request(
+        response = yield self._client.request(
             "GET",
             url,
         )
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index f78d83b27..41391845f 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -44,6 +44,7 @@ from ..storage.http_client import (
     StorageClientImmutables,
     ImmutableCreateResult,
     UploadProgress,
+    StorageClientGeneral,
 )
 from ..storage.common import si_b2a
 
@@ -253,7 +254,7 @@ class RoutingTests(SyncTestCase):
         """
         # Without secret, get a 400 error.
         response = result_of(
-            self.client._request(
+            self.client.request(
                 "GET",
                 "http://127.0.0.1/upload_secret",
             )
@@ -262,7 +263,7 @@ class RoutingTests(SyncTestCase):
 
         # With secret, we're good.
         response = result_of(
-            self.client._request(
+            self.client.request(
                 "GET", "http://127.0.0.1/upload_secret", upload_secret=b"MAGIC"
             )
         )
@@ -307,10 +308,12 @@ class GenericHTTPAPITests(SyncTestCase):
         If the wrong swissnum is used, an ``Unauthorized`` response code is
         returned.
         """
-        client = StorageClient(
-            DecodedURL.from_text("http://127.0.0.1"),
-            b"something wrong",
-            treq=StubTreq(self.http.http_server.get_resource()),
+        client = StorageClientGeneral(
+            StorageClient(
+                DecodedURL.from_text("http://127.0.0.1"),
+                b"something wrong",
+                treq=StubTreq(self.http.http_server.get_resource()),
+            )
         )
         with self.assertRaises(ClientException) as e:
             result_of(client.get_version())
@@ -323,7 +326,8 @@ class GenericHTTPAPITests(SyncTestCase):
         We ignore available disk space and max immutable share size, since that
         might change across calls.
         """
-        version = result_of(self.http.client.get_version())
+        client = StorageClientGeneral(self.http.client)
+        version = result_of(client.get_version())
         version[b"http://allmydata.org/tahoe/protocols/storage/v1"].pop(
             b"available-space"
         )

From d38183335eb7ca8bbcb3d9b1a7cd57a898061ae6 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Tue, 8 Feb 2022 10:46:55 -0500
Subject: [PATCH 06/25] Handle bad Content-Range headers.

---
 src/allmydata/storage/http_client.py    |  6 ++-
 src/allmydata/storage/http_server.py    | 12 +++---
 src/allmydata/test/test_storage_http.py | 57 +++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 7 deletions(-)

diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py
index 3b1dbfb30..475aa2330 100644
--- a/src/allmydata/storage/http_client.py
+++ b/src/allmydata/storage/http_client.py
@@ -48,7 +48,11 @@ def _encode_si(si):  # type: (bytes) -> str
 
 
 class ClientException(Exception):
-    """An unexpected error."""
+    """An unexpected response code from the server."""
+
+    def __init__(self, code, *additional_args):
+        Exception.__init__(self, code, *additional_args)
+        self.code = code
 
 
 def _decode_cbor(response):
diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index cf8284e3e..ad1b8b2ac 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -239,14 +239,14 @@ class HTTPServer(object):
     def write_share_data(self, request, authorization, storage_index, share_number):
         """Write data to an in-progress immutable upload."""
         content_range = parse_content_range_header(request.getHeader("content-range"))
-        # TODO in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
-        # 1. Malformed header should result in error 416
-        # 2. Non-bytes unit should result in error 416
-        # 3. Missing header means full upload in one request
-        # 4. Impossible range should resul tin error 416
+        if content_range is None or content_range.units != "bytes":
+            # TODO Missing header means full upload in one request
+            request.setResponseCode(http.REQUESTED_RANGE_NOT_SATISFIABLE)
+            return b""
+        
         offset = content_range.start
 
-        # TODO basic checks on validity of start, offset, and content-range in general.
+        
         # TODO basic check that body isn't infinite. require content-length? or maybe we should require content-range (it's optional now)? if so, needs to be rflected in protocol spec.
 
         data = request.content.read()
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 41391845f..7e459ee2a 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -25,6 +25,7 @@ from hyperlink import DecodedURL
 from collections_extended import RangeMap
 from twisted.internet.task import Clock
 from twisted.web import http
+from twisted.web.http_headers import Headers
 from werkzeug import routing
 from werkzeug.exceptions import NotFound as WNotFound
 
@@ -291,6 +292,24 @@ class HttpTestFixture(Fixture):
         )
 
 
+class StorageClientWithHeadersOverride(object):
+    """Wrap ``StorageClient`` and override sent headers."""
+
+    def __init__(self, storage_client, add_headers):
+        self.storage_client = storage_client
+        self.add_headers = add_headers
+
+    def __getattr__(self, attr):
+        return getattr(self.storage_client, attr)
+
+    def request(self, *args, headers=None, **kwargs):
+        if headers is None:
+            headers = Headers()
+        for key, value in self.add_headers.items():
+            headers.setRawHeaders(key, [value])
+        return self.storage_client.request(*args, headers=headers, **kwargs)
+
+
 class GenericHTTPAPITests(SyncTestCase):
     """
     Tests of HTTP client talking to the HTTP server, for generic HTTP API
@@ -518,6 +537,44 @@ class ImmutableHTTPAPITests(SyncTestCase):
         # Now shares 1 and 3 exist:
         self.assertEqual(result_of(im_client.list_shares(storage_index)), {1, 3})
 
+    def test_upload_bad_content_range(self):
+        """
+        Malformed or invalid Content-Range headers to the immutable upload
+        endpoint result in a 416 error.
+        """
+        im_client = StorageClientImmutables(self.http.client)
+        upload_secret = urandom(32)
+        lease_secret = urandom(32)
+        storage_index = b"0" * 16
+        result_of(
+            im_client.create(
+                storage_index, {1}, 10, upload_secret, lease_secret, lease_secret
+            )
+        )
+
+        def check_invalid(bad_content_range_value):
+            client = StorageClientImmutables(
+                StorageClientWithHeadersOverride(
+                    self.http.client, {"content-range": bad_content_range_value}
+                )
+            )
+            with self.assertRaises(ClientException) as e:
+                result_of(
+                    client.write_share_chunk(
+                        storage_index,
+                        1,
+                        upload_secret,
+                        0,
+                        b"0123456789",
+                    )
+                )
+            self.assertEqual(e.exception.code, http.REQUESTED_RANGE_NOT_SATISFIABLE)
+
+        check_invalid("not a valid content-range header at all")
+        check_invalid("bytes -1-9/10")
+        check_invalid("bytes 0--9/10")
+        check_invalid("teapots 0-9/10")
+
     def test_list_shares_unknown_storage_index(self):
         """
         Listing unknown storage index's shares results in empty list of shares.

From ecb1a3c5a0bd63a6c36be5251b4881771ab65586 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Feb 2022 12:25:47 -0500
Subject: [PATCH 07/25] Just require content-range for simplicity.

---
 docs/proposed/http-storage-node-protocol.rst | 2 +-
 src/allmydata/storage/http_server.py         | 9 +++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst
index b00b327e3..4d8d60560 100644
--- a/docs/proposed/http-storage-node-protocol.rst
+++ b/docs/proposed/http-storage-node-protocol.rst
@@ -540,7 +540,7 @@ Rejected designs for upload secrets:
 Write data for the indicated share.
 The share number must belong to the storage index.
 The request body is the raw share data (i.e., ``application/octet-stream``).
-*Content-Range* requests are encouraged for large transfers to allow partially complete uploads to be resumed.
+*Content-Range* requests are required; for large transfers this allows partially complete uploads to be resumed.
 For example,
 a 1MiB share can be divided in to eight separate 128KiB chunks.
 Each chunk can be uploaded in a separate request.
diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index ad1b8b2ac..708b99380 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -240,16 +240,13 @@ class HTTPServer(object):
         """Write data to an in-progress immutable upload."""
         content_range = parse_content_range_header(request.getHeader("content-range"))
         if content_range is None or content_range.units != "bytes":
-            # TODO Missing header means full upload in one request
             request.setResponseCode(http.REQUESTED_RANGE_NOT_SATISFIABLE)
             return b""
-        
+
         offset = content_range.start
 
-        
-        # TODO basic check that body isn't infinite. require content-length? or maybe we should require content-range (it's optional now)? if so, needs to be rflected in protocol spec.
-
-        data = request.content.read()
+        # TODO limit memory usage
+        data = request.content.read(content_range.stop - content_range.start + 1)
         try:
             bucket = self._uploads[storage_index].shares[share_number]
         except (KeyError, IndexError):

From 72bac785ee3b21d4d7bd9cb8f71df76ae3fd35a5 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Feb 2022 12:27:08 -0500
Subject: [PATCH 08/25] Done elsewhere.

---
 src/allmydata/test/test_storage_http.py | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 7e459ee2a..ae316708c 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -615,13 +615,6 @@ class ImmutableHTTPAPITests(SyncTestCase):
         TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
         """
 
-    def test_upload_offset_cannot_be_negative(self):
-        """
-        A negative upload offset will be rejected.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
-        """
-
     def test_mismatching_upload_fails(self):
         """
         If an uploaded chunk conflicts with an already uploaded chunk, a

From 95d7548629e6a96a2a2e6f57dbc8dac2015caeaf Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Feb 2022 12:30:38 -0500
Subject: [PATCH 09/25] Upload to non-existent place.

---
 src/allmydata/storage/http_server.py    |  4 +--
 src/allmydata/test/test_storage_http.py | 33 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 708b99380..6f9033380 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -250,8 +250,8 @@ class HTTPServer(object):
         try:
             bucket = self._uploads[storage_index].shares[share_number]
         except (KeyError, IndexError):
-            # TODO return 404
-            raise
+            request.setResponseCode(http.NOT_FOUND)
+            return b""
 
         finished = bucket.write(offset, data)
 
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index ae316708c..7a5c24905 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -583,6 +583,39 @@ class ImmutableHTTPAPITests(SyncTestCase):
         storage_index = b"".join(bytes([i]) for i in range(16))
         self.assertEqual(result_of(im_client.list_shares(storage_index)), set())
 
+    def test_upload_non_existent_storage_index(self):
+        """
+        Uploading to a non-existent storage index or share number results in
+        404.
+        """
+        im_client = StorageClientImmutables(self.http.client)
+        upload_secret = urandom(32)
+        lease_secret = urandom(32)
+        storage_index = b"".join(bytes([i]) for i in range(16))
+        result_of(
+            im_client.create(
+                storage_index, {1}, 10, upload_secret, lease_secret, lease_secret
+            )
+        )
+
+        def unknown_check(storage_index, share_number):
+            with self.assertRaises(ClientException) as e:
+                result_of(
+                    im_client.write_share_chunk(
+                        storage_index,
+                        share_number,
+                        upload_secret,
+                        0,
+                        b"0123456789",
+                    )
+                )
+            self.assertEqual(e.exception.code, http.NOT_FOUND)
+
+        # Wrong share number:
+        unknown_check(storage_index, 7)
+        # Wrong storage index:
+        unknown_check(b"X" * 16, 7)
+
     def test_multiple_shares_uploaded_to_different_place(self):
         """
         If a storage index has multiple shares, uploads to different shares are

From 8c739343f3f21d3b8ad112a87eca582e03638967 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Feb 2022 12:37:49 -0500
Subject: [PATCH 10/25] Reduce duplication.

---
 src/allmydata/test/test_storage_http.py | 104 +++++++++---------------
 1 file changed, 39 insertions(+), 65 deletions(-)

diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 7a5c24905..2bad160e4 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -373,6 +373,28 @@ class ImmutableHTTPAPITests(SyncTestCase):
             self.skipTest("Not going to bother supporting Python 2")
         super(ImmutableHTTPAPITests, self).setUp()
         self.http = self.useFixture(HttpTestFixture())
+        self.im_client = StorageClientImmutables(self.http.client)
+
+    def create_upload(self, share_numbers, length):
+        """
+        Create a write bucket on server, return:
+
+            (upload_secret, lease_secret, storage_index, result)
+        """
+        upload_secret = urandom(32)
+        lease_secret = urandom(32)
+        storage_index = urandom(16)
+        created = result_of(
+            self.im_client.create(
+                storage_index,
+                share_numbers,
+                length,
+                upload_secret,
+                lease_secret,
+                lease_secret,
+            )
+        )
+        return (upload_secret, lease_secret, storage_index, created)
 
     def test_upload_can_be_downloaded(self):
         """
@@ -386,17 +408,8 @@ class ImmutableHTTPAPITests(SyncTestCase):
         length = 100
         expected_data = b"".join(bytes([i]) for i in range(100))
 
-        im_client = StorageClientImmutables(self.http.client)
-
         # Create a upload:
-        upload_secret = urandom(32)
-        lease_secret = urandom(32)
-        storage_index = b"".join(bytes([i]) for i in range(16))
-        created = result_of(
-            im_client.create(
-                storage_index, {1}, 100, upload_secret, lease_secret, lease_secret
-            )
-        )
+        (upload_secret, _, storage_index, created) = self.create_upload({1}, 100)
         self.assertEqual(
             created, ImmutableCreateResult(already_have=set(), allocated={1})
         )
@@ -407,7 +420,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         # Three writes: 10-19, 30-39, 50-59. This allows for a bunch of holes.
         def write(offset, length):
             remaining.empty(offset, offset + length)
-            return im_client.write_share_chunk(
+            return self.im_client.write_share_chunk(
                 storage_index,
                 1,
                 upload_secret,
@@ -451,7 +464,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         # We can now read:
         for offset, length in [(0, 100), (10, 19), (99, 1), (49, 200)]:
             downloaded = result_of(
-                im_client.read_share_chunk(storage_index, 1, offset, length)
+                self.im_client.read_share_chunk(storage_index, 1, offset, length)
             )
             self.assertEqual(downloaded, expected_data[offset : offset + length])
 
@@ -460,20 +473,13 @@ class ImmutableHTTPAPITests(SyncTestCase):
         If allocate buckets endpoint is called second time with wrong upload
         key on the same shares, the result is an error.
         """
-        im_client = StorageClientImmutables(self.http.client)
-
         # Create a upload:
-        upload_secret = urandom(32)
-        lease_secret = urandom(32)
-        storage_index = b"".join(bytes([i]) for i in range(16))
-        result_of(
-            im_client.create(
-                storage_index, {1, 2, 3}, 100, upload_secret, lease_secret, lease_secret
-            )
+        (upload_secret, lease_secret, storage_index, _) = self.create_upload(
+            {1, 2, 3}, 100
         )
         with self.assertRaises(ClientException) as e:
             result_of(
-                im_client.create(
+                self.im_client.create(
                     storage_index, {2, 3}, 100, b"x" * 32, lease_secret, lease_secret
                 )
             )
@@ -484,21 +490,14 @@ class ImmutableHTTPAPITests(SyncTestCase):
         If allocate buckets endpoint is called second time with different
         upload key on different shares, that creates the buckets.
         """
-        im_client = StorageClientImmutables(self.http.client)
-
         # Create a upload:
-        upload_secret = urandom(32)
-        lease_secret = urandom(32)
-        storage_index = b"".join(bytes([i]) for i in range(16))
-        result_of(
-            im_client.create(
-                storage_index, {1, 2, 3}, 100, upload_secret, lease_secret, lease_secret
-            )
+        (upload_secret, lease_secret, storage_index, created) = self.create_upload(
+            {1, 2, 3}, 100
         )
 
         # Add same shares:
         created2 = result_of(
-            im_client.create(
+            self.im_client.create(
                 storage_index, {4, 6}, 100, b"x" * 2, lease_secret, lease_secret
             )
         )
@@ -508,23 +507,15 @@ class ImmutableHTTPAPITests(SyncTestCase):
         """
         Once a share is finished uploading, it's possible to list it.
         """
-        im_client = StorageClientImmutables(self.http.client)
-        upload_secret = urandom(32)
-        lease_secret = urandom(32)
-        storage_index = b"".join(bytes([i]) for i in range(16))
-        result_of(
-            im_client.create(
-                storage_index, {1, 2, 3}, 10, upload_secret, lease_secret, lease_secret
-            )
-        )
+        (upload_secret, _, storage_index, created) = self.create_upload({1, 2, 3}, 10)
 
         # Initially there are no shares:
-        self.assertEqual(result_of(im_client.list_shares(storage_index)), set())
+        self.assertEqual(result_of(self.im_client.list_shares(storage_index)), set())
 
         # Upload shares 1 and 3:
         for share_number in [1, 3]:
             progress = result_of(
-                im_client.write_share_chunk(
+                self.im_client.write_share_chunk(
                     storage_index,
                     share_number,
                     upload_secret,
@@ -535,22 +526,14 @@ class ImmutableHTTPAPITests(SyncTestCase):
             self.assertTrue(progress.finished)
 
         # Now shares 1 and 3 exist:
-        self.assertEqual(result_of(im_client.list_shares(storage_index)), {1, 3})
+        self.assertEqual(result_of(self.im_client.list_shares(storage_index)), {1, 3})
 
     def test_upload_bad_content_range(self):
         """
         Malformed or invalid Content-Range headers to the immutable upload
         endpoint result in a 416 error.
         """
-        im_client = StorageClientImmutables(self.http.client)
-        upload_secret = urandom(32)
-        lease_secret = urandom(32)
-        storage_index = b"0" * 16
-        result_of(
-            im_client.create(
-                storage_index, {1}, 10, upload_secret, lease_secret, lease_secret
-            )
-        )
+        (upload_secret, _, storage_index, created) = self.create_upload({1}, 10)
 
         def check_invalid(bad_content_range_value):
             client = StorageClientImmutables(
@@ -579,29 +562,20 @@ class ImmutableHTTPAPITests(SyncTestCase):
         """
         Listing unknown storage index's shares results in empty list of shares.
         """
-        im_client = StorageClientImmutables(self.http.client)
         storage_index = b"".join(bytes([i]) for i in range(16))
-        self.assertEqual(result_of(im_client.list_shares(storage_index)), set())
+        self.assertEqual(result_of(self.im_client.list_shares(storage_index)), set())
 
     def test_upload_non_existent_storage_index(self):
         """
         Uploading to a non-existent storage index or share number results in
         404.
         """
-        im_client = StorageClientImmutables(self.http.client)
-        upload_secret = urandom(32)
-        lease_secret = urandom(32)
-        storage_index = b"".join(bytes([i]) for i in range(16))
-        result_of(
-            im_client.create(
-                storage_index, {1}, 10, upload_secret, lease_secret, lease_secret
-            )
-        )
+        (upload_secret, _, storage_index, _) = self.create_upload({1}, 10)
 
         def unknown_check(storage_index, share_number):
             with self.assertRaises(ClientException) as e:
                 result_of(
-                    im_client.write_share_chunk(
+                    self.im_client.write_share_chunk(
                         storage_index,
                         share_number,
                         upload_secret,

From faacde4e32a68543d6e7e92ea80a705e44208afa Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Feb 2022 12:41:32 -0500
Subject: [PATCH 11/25] Conflicting writes.

---
 src/allmydata/storage/http_server.py    | 10 +++++----
 src/allmydata/test/test_storage_http.py | 27 +++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 6f9033380..4613a73c3 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -31,7 +31,7 @@ from cbor2 import dumps, loads
 from .server import StorageServer
 from .http_common import swissnum_auth_header, Secrets
 from .common import si_a2b
-from .immutable import BucketWriter
+from .immutable import BucketWriter, ConflictingWriteError
 from ..util.hashutil import timing_safe_compare
 from ..util.base32 import rfc3548_alphabet
 
@@ -253,9 +253,11 @@ class HTTPServer(object):
             request.setResponseCode(http.NOT_FOUND)
             return b""
 
-        finished = bucket.write(offset, data)
-
-        # TODO if raises ConflictingWriteError, return HTTP CONFLICT code.
+        try:
+            finished = bucket.write(offset, data)
+        except ConflictingWriteError:
+            request.setResponseCode(http.CONFLICT)
+            return b""
 
         if finished:
             bucket.close()
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 2bad160e4..225784cb6 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -626,9 +626,32 @@ class ImmutableHTTPAPITests(SyncTestCase):
         """
         If an uploaded chunk conflicts with an already uploaded chunk, a
         CONFLICT error is returned.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
         """
+        (upload_secret, _, storage_index, created) = self.create_upload({1}, 100)
+
+        # Write:
+        result_of(
+            self.im_client.write_share_chunk(
+                storage_index,
+                1,
+                upload_secret,
+                0,
+                b"0" * 10,
+            )
+        )
+
+        # Conflicting write:
+        with self.assertRaises(ClientException) as e:
+            result_of(
+                self.im_client.write_share_chunk(
+                    storage_index,
+                    1,
+                    upload_secret,
+                    0,
+                    b"0123456789",
+                )
+            )
+            self.assertEqual(e.exception.code, http.NOT_FOUND)
 
     def test_read_of_wrong_storage_index_fails(self):
         """

From bae5d58ab97468e6fdefc8570a1f448ff3c30631 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Feb 2022 13:07:34 -0500
Subject: [PATCH 12/25] Another test.

---
 src/allmydata/test/test_storage_http.py | 29 +++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 225784cb6..e61a023bd 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -594,9 +594,34 @@ class ImmutableHTTPAPITests(SyncTestCase):
         """
         If a storage index has multiple shares, uploads to different shares are
         stored separately and can be downloaded separately.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
         """
+        (upload_secret, _, storage_index, _) = self.create_upload({1, 2}, 10)
+        result_of(
+            self.im_client.write_share_chunk(
+                storage_index,
+                1,
+                upload_secret,
+                0,
+                b"1" * 10,
+            )
+        )
+        result_of(
+            self.im_client.write_share_chunk(
+                storage_index,
+                2,
+                upload_secret,
+                0,
+                b"2" * 10,
+            )
+        )
+        self.assertEqual(
+            result_of(self.im_client.read_share_chunk(storage_index, 1, 0, 10)),
+            b"1" * 10,
+        )
+        self.assertEqual(
+            result_of(self.im_client.read_share_chunk(storage_index, 2, 0, 10)),
+            b"2" * 10,
+        )
 
     def test_bucket_allocated_with_new_shares(self):
         """

From 45ee5e33460fa06f3c557c17b9697c27f09c4aa4 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Feb 2022 13:08:34 -0500
Subject: [PATCH 13/25] Done elsewhere.

---
 src/allmydata/test/test_storage_http.py | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index e61a023bd..ac52303f6 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -623,30 +623,6 @@ class ImmutableHTTPAPITests(SyncTestCase):
             b"2" * 10,
         )
 
-    def test_bucket_allocated_with_new_shares(self):
-        """
-        If some shares already exist, allocating shares indicates only the new
-        ones were created.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
-        """
-
-    def test_bucket_allocation_new_upload_secret(self):
-        """
-        If a bucket was allocated with one upload secret, and a different upload
-        key is used to allocate the bucket again, the second allocation fails.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
-        """
-
-    def test_upload_with_wrong_upload_secret_fails(self):
-        """
-        Uploading with a key that doesn't match the one used to allocate the
-        bucket will fail.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
-        """
-
     def test_mismatching_upload_fails(self):
         """
         If an uploaded chunk conflicts with an already uploaded chunk, a

From 5d9e0c9bca14ace590b842250efa69bb092d0b48 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Feb 2022 13:14:27 -0500
Subject: [PATCH 14/25] Not found tests and implementation.

---
 src/allmydata/storage/http_server.py    |  9 ++++--
 src/allmydata/test/test_storage_http.py | 41 ++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 4613a73c3..e7aa12f55 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -302,8 +302,13 @@ class HTTPServer(object):
         offset, end = range_header.ranges[0]
         assert end != None  # TODO support this case
 
-        # TODO if not found, 404
-        bucket = self._storage_server.get_buckets(storage_index)[share_number]
+        try:
+            bucket = self._storage_server.get_buckets(storage_index)[share_number]
+        except KeyError:
+            request.setResponseCode(http.NOT_FOUND)
+            return b""
+
+        # TODO limit memory usage
         data = bucket.read(offset, end - offset)
         request.setResponseCode(http.PARTIAL_CONTENT)
         # TODO set content-range on response. We we need to expand the
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index ac52303f6..b96eebb96 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -654,19 +654,52 @@ class ImmutableHTTPAPITests(SyncTestCase):
             )
             self.assertEqual(e.exception.code, http.NOT_FOUND)
 
+    def upload(self, share_number):
+        """
+        Create a share, return (storage_index).
+        """
+        (upload_secret, _, storage_index, _) = self.create_upload({share_number}, 26)
+        result_of(
+            self.im_client.write_share_chunk(
+                storage_index,
+                share_number,
+                upload_secret,
+                0,
+                b"abcdefghijklmnopqrstuvwxyz",
+            )
+        )
+        return storage_index
+
     def test_read_of_wrong_storage_index_fails(self):
         """
         Reading from unknown storage index results in 404.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
         """
+        with self.assertRaises(ClientException) as e:
+            result_of(
+                self.im_client.read_share_chunk(
+                    b"1" * 16,
+                    1,
+                    0,
+                    10,
+                )
+            )
+        self.assertEqual(e.exception.code, http.NOT_FOUND)
 
     def test_read_of_wrong_share_number_fails(self):
         """
         Reading from unknown storage index results in 404.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
         """
+        storage_index = self.upload(1)
+        with self.assertRaises(ClientException) as e:
+            result_of(
+                self.im_client.read_share_chunk(
+                    storage_index,
+                    7,  # different share number
+                    0,
+                    10,
+                )
+            )
+        self.assertEqual(e.exception.code, http.NOT_FOUND)
 
     def test_read_with_negative_offset_fails(self):
         """

From 7db1ddd8750d429d5a77ef3e3c423f99fe89fbad Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Thu, 10 Feb 2022 13:12:17 -0500
Subject: [PATCH 15/25] Implement Range header validation.

---
 docs/proposed/http-storage-node-protocol.rst |  2 +-
 src/allmydata/storage/http_server.py         | 15 +++++---
 src/allmydata/test/test_storage_http.py      | 40 ++++++++++++++++----
 3 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst
index 4d8d60560..315546b8a 100644
--- a/docs/proposed/http-storage-node-protocol.rst
+++ b/docs/proposed/http-storage-node-protocol.rst
@@ -644,7 +644,7 @@ Read a contiguous sequence of bytes from one share in one bucket.
 The response body is the raw share data (i.e., ``application/octet-stream``).
 The ``Range`` header may be used to request exactly one ``bytes`` range, in which case the response code will be 206 (partial content).
 Interpretation and response behavior is as specified in RFC 7233 § 4.1.
-Multiple ranges in a single request are *not* supported.
+Multiple ranges in a single request are *not* supported; open-ended ranges are also not supported.
 
 Discussion
 ``````````
diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index e7aa12f55..89c9ca6fa 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -291,16 +291,19 @@ class HTTPServer(object):
     )
     def read_share_chunk(self, request, authorization, storage_index, share_number):
         """Read a chunk for an already uploaded immutable."""
-        # TODO in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
         # 2. missing range header should have response code 200 and return whole thing
-        # 3. malformed range header should result in error? or return everything?
-        # 4. non-bytes range results in error
-        # 5. ranges make sense semantically (positive, etc.)
-        # 6. multiple ranges fails with error
         # 7. missing end of range means "to the end of share"
         range_header = parse_range_header(request.getHeader("range"))
+        if (
+            range_header is None
+            or range_header.units != "bytes"
+            or len(range_header.ranges) > 1  # more than one range
+            or range_header.ranges[0][1] is None  # range without end
+        ):
+            request.setResponseCode(http.REQUESTED_RANGE_NOT_SATISFIABLE)
+            return b""
+
         offset, end = range_header.ranges[0]
-        assert end != None  # TODO support this case
 
         try:
             bucket = self._storage_server.get_buckets(storage_index)[share_number]
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index b96eebb96..f115d632c 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -703,14 +703,38 @@ class ImmutableHTTPAPITests(SyncTestCase):
 
     def test_read_with_negative_offset_fails(self):
         """
-        The offset for reads cannot be negative.
-
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
+        Malformed or unsupported Range headers result in 416 (requested range
+        not satisfiable) error.
         """
+        storage_index = self.upload(1)
 
-    def test_read_with_negative_length_fails(self):
-        """
-        The length for reads cannot be negative.
+        def check_bad_range(bad_range_value):
+            client = StorageClientImmutables(
+                StorageClientWithHeadersOverride(
+                    self.http.client, {"range": bad_range_value}
+                )
+            )
+
+            with self.assertRaises(ClientException) as e:
+                result_of(
+                    client.read_share_chunk(
+                        storage_index,
+                        1,
+                        0,
+                        10,
+                    )
+                )
+            self.assertEqual(e.exception.code, http.REQUESTED_RANGE_NOT_SATISFIABLE)
+
+        check_bad_range("molluscs=0-9")
+        check_bad_range("bytes=-2-9")
+        check_bad_range("bytes=0--10")
+        check_bad_range("bytes=-300-")
+        check_bad_range("bytes=")
+        # Multiple ranges are currently unsupported, even if they're
+        # semantically valid under HTTP:
+        check_bad_range("bytes=0-5, 6-7")
+        # Ranges without an end are currently unsupported, even if they're
+        # semantically valid under HTTP.
+        check_bad_range("bytes=0-")
 
-        TBD in https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860
-        """

From 416af7328cd0a89930c719882ab36ce4f3e74839 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Thu, 10 Feb 2022 13:31:09 -0500
Subject: [PATCH 16/25] Support lack of Range header.

---
 src/allmydata/storage/http_server.py    | 26 ++++++++++++------
 src/allmydata/test/test_storage_http.py | 36 ++++++++++++++++++++-----
 2 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 89c9ca6fa..88de18f12 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -291,8 +291,24 @@ class HTTPServer(object):
     )
     def read_share_chunk(self, request, authorization, storage_index, share_number):
         """Read a chunk for an already uploaded immutable."""
-        # 2. missing range header should have response code 200 and return whole thing
-        # 7. missing end of range means "to the end of share"
+        try:
+            bucket = self._storage_server.get_buckets(storage_index)[share_number]
+        except KeyError:
+            request.setResponseCode(http.NOT_FOUND)
+            return b""
+
+        if request.getHeader("range") is None:
+            # Return the whole thing.
+            start = 0
+            while True:
+                # TODO should probably yield to event loop occasionally...
+                data = bucket.read(start, start + 65536)
+                if not data:
+                    request.finish()
+                    return
+                request.write(data)
+                start += len(data)
+
         range_header = parse_range_header(request.getHeader("range"))
         if (
             range_header is None
@@ -305,12 +321,6 @@ class HTTPServer(object):
 
         offset, end = range_header.ranges[0]
 
-        try:
-            bucket = self._storage_server.get_buckets(storage_index)[share_number]
-        except KeyError:
-            request.setResponseCode(http.NOT_FOUND)
-            return b""
-
         # TODO limit memory usage
         data = bucket.read(offset, end - offset)
         request.setResponseCode(http.PARTIAL_CONTENT)
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index f115d632c..bbda00bf6 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -46,6 +46,7 @@ from ..storage.http_client import (
     ImmutableCreateResult,
     UploadProgress,
     StorageClientGeneral,
+    _encode_si,
 )
 from ..storage.common import si_b2a
 
@@ -654,21 +655,26 @@ class ImmutableHTTPAPITests(SyncTestCase):
             )
             self.assertEqual(e.exception.code, http.NOT_FOUND)
 
-    def upload(self, share_number):
+    def upload(self, share_number, data_length=26):
         """
-        Create a share, return (storage_index).
+        Create a share, return (storage_index, uploaded_data).
         """
-        (upload_secret, _, storage_index, _) = self.create_upload({share_number}, 26)
+        uploaded_data = (b"abcdefghijklmnopqrstuvwxyz" * ((data_length // 26) + 1))[
+            :data_length
+        ]
+        (upload_secret, _, storage_index, _) = self.create_upload(
+            {share_number}, data_length
+        )
         result_of(
             self.im_client.write_share_chunk(
                 storage_index,
                 share_number,
                 upload_secret,
                 0,
-                b"abcdefghijklmnopqrstuvwxyz",
+                uploaded_data,
             )
         )
-        return storage_index
+        return storage_index, uploaded_data
 
     def test_read_of_wrong_storage_index_fails(self):
         """
@@ -689,7 +695,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         """
         Reading from unknown storage index results in 404.
         """
-        storage_index = self.upload(1)
+        storage_index, _ = self.upload(1)
         with self.assertRaises(ClientException) as e:
             result_of(
                 self.im_client.read_share_chunk(
@@ -706,7 +712,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         Malformed or unsupported Range headers result in 416 (requested range
         not satisfiable) error.
         """
-        storage_index = self.upload(1)
+        storage_index, _ = self.upload(1)
 
         def check_bad_range(bad_range_value):
             client = StorageClientImmutables(
@@ -738,3 +744,19 @@ class ImmutableHTTPAPITests(SyncTestCase):
         # semantically valid under HTTP.
         check_bad_range("bytes=0-")
 
+    @given(data_length=st.integers(min_value=1, max_value=300000))
+    def test_read_with_no_range(self, data_length):
+        """
+        A read with no range returns the whole immutable.
+        """
+        storage_index, uploaded_data = self.upload(1, data_length)
+        response = result_of(
+            self.http.client.request(
+                "GET",
+                self.http.client.relative_url(
+                    "/v1/immutable/{}/1".format(_encode_si(storage_index))
+                ),
+            )
+        )
+        self.assertEqual(response.code, http.OK)
+        self.assertEqual(result_of(response.content()), uploaded_data)

From aa68be645f80dde5a52dc5a32d74304921e97288 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Thu, 10 Feb 2022 13:47:56 -0500
Subject: [PATCH 17/25] Return Content-Range in responses.

---
 src/allmydata/storage/http_server.py    | 12 +++++------
 src/allmydata/test/test_storage_http.py | 27 +++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 88de18f12..bb4ef6c00 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -24,6 +24,7 @@ from twisted.web import http
 import attr
 from werkzeug.http import parse_range_header, parse_content_range_header
 from werkzeug.routing import BaseConverter
+from werkzeug.datastructures import ContentRange
 
 # TODO Make sure to use pure Python versions?
 from cbor2 import dumps, loads
@@ -323,11 +324,10 @@ class HTTPServer(object):
 
         # TODO limit memory usage
         data = bucket.read(offset, end - offset)
+
         request.setResponseCode(http.PARTIAL_CONTENT)
-        # TODO set content-range on response. We we need to expand the
-        # BucketReader interface to return share's length.
-        #
-        # request.setHeader(
-        #    "content-range", range_header.make_content_range(share_length).to_header()
-        # )
+        request.setHeader(
+            "content-range",
+            ContentRange("bytes", offset, offset + len(data)).to_header(),
+        )
         return data
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index bbda00bf6..c864f923c 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -760,3 +760,30 @@ class ImmutableHTTPAPITests(SyncTestCase):
         )
         self.assertEqual(response.code, http.OK)
         self.assertEqual(result_of(response.content()), uploaded_data)
+
+    def test_validate_content_range_response_to_read(self):
+        """
+        The server responds to ranged reads with an appropriate Content-Range
+        header.
+        """
+        storage_index, _ = self.upload(1, 26)
+
+        def check_range(requested_range, expected_response):
+            headers = Headers()
+            headers.setRawHeaders("range", [requested_range])
+            response = result_of(
+                self.http.client.request(
+                    "GET",
+                    self.http.client.relative_url(
+                        "/v1/immutable/{}/1".format(_encode_si(storage_index))
+                    ),
+                    headers=headers,
+                )
+            )
+            self.assertEqual(
+                response.headers.getRawHeaders("content-range"), [expected_response]
+            )
+
+        check_range("bytes=0-10", "bytes 0-10/*")
+        # Can't go beyond the end of the immutable!
+        check_range("bytes=10-100", "bytes 10-25/*")

From fa2f142bc9ecd507312614ab134e53a74f7c8ce4 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Thu, 10 Feb 2022 13:50:09 -0500
Subject: [PATCH 18/25] Another ticket.

---
 src/allmydata/storage/http_server.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index bb4ef6c00..491fcd39b 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -247,6 +247,7 @@ class HTTPServer(object):
         offset = content_range.start
 
         # TODO limit memory usage
+        # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3872
         data = request.content.read(content_range.stop - content_range.start + 1)
         try:
             bucket = self._uploads[storage_index].shares[share_number]
@@ -303,6 +304,7 @@ class HTTPServer(object):
             start = 0
             while True:
                 # TODO should probably yield to event loop occasionally...
+                # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3872
                 data = bucket.read(start, start + 65536)
                 if not data:
                     request.finish()
@@ -323,6 +325,7 @@ class HTTPServer(object):
         offset, end = range_header.ranges[0]
 
         # TODO limit memory usage
+        # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3872
         data = bucket.read(offset, end - offset)
 
         request.setResponseCode(http.PARTIAL_CONTENT)

From b049d4a792efa69c8f82ef0800380694b41cd5f3 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Thu, 10 Feb 2022 13:52:47 -0500
Subject: [PATCH 19/25] Fix get_version with new API.

---
 src/allmydata/storage_client.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py
index abced41b3..528711539 100644
--- a/src/allmydata/storage_client.py
+++ b/src/allmydata/storage_client.py
@@ -75,7 +75,9 @@ from allmydata.util.observer import ObserverList
 from allmydata.util.rrefutil import add_version_to_remote_reference
 from allmydata.util.hashutil import permute_server_hash
 from allmydata.util.dictutil import BytesKeyDict, UnicodeKeyDict
-from allmydata.storage.http_client import StorageClient, StorageClientImmutables
+from allmydata.storage.http_client import (
+    StorageClient, StorageClientImmutables, StorageClientGeneral,
+)
 
 
 # who is responsible for de-duplication?
@@ -1108,7 +1110,7 @@ class _HTTPStorageServer(object):
         return _HTTPStorageServer(http_client=http_client, upload_secret=urandom(20))
 
     def get_version(self):
-        return self._http_client.get_version()
+        return StorageClientGeneral(self._http_client).get_version()
 
     @defer.inlineCallbacks
     def allocate_buckets(

From 7466ee25a8d94017735a0380fc232fd8ec472421 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Thu, 10 Feb 2022 13:57:57 -0500
Subject: [PATCH 20/25] Don't send header if it makes no sense to do so.

---
 src/allmydata/storage/http_server.py | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 491fcd39b..f89a156a3 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -329,8 +329,11 @@ class HTTPServer(object):
         data = bucket.read(offset, end - offset)
 
         request.setResponseCode(http.PARTIAL_CONTENT)
-        request.setHeader(
-            "content-range",
-            ContentRange("bytes", offset, offset + len(data)).to_header(),
-        )
+        if len(data):
+            # For empty bodies the content-range header makes no sense since
+            # the end of the range is inclusive.
+            request.setHeader(
+                "content-range",
+                ContentRange("bytes", offset, offset + len(data)).to_header(),
+            )
         return data

From abf3048ab347ae737344db9bc4701b1452f83ad1 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Thu, 10 Feb 2022 17:07:21 -0500
Subject: [PATCH 21/25] More passing HTTP IStorageServer tests.

---
 src/allmydata/storage_client.py           | 10 ++++++++--
 src/allmydata/test/test_istorageserver.py |  2 --
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py
index 528711539..17a6f79a5 100644
--- a/src/allmydata/storage_client.py
+++ b/src/allmydata/storage_client.py
@@ -58,7 +58,7 @@ from twisted.plugin import (
 from eliot import (
     log_call,
 )
-from foolscap.api import eventually
+from foolscap.api import eventually, RemoteException
 from foolscap.reconnector import (
     ReconnectionInfo,
 )
@@ -77,6 +77,7 @@ from allmydata.util.hashutil import permute_server_hash
 from allmydata.util.dictutil import BytesKeyDict, UnicodeKeyDict
 from allmydata.storage.http_client import (
     StorageClient, StorageClientImmutables, StorageClientGeneral,
+    ClientException as HTTPClientException,
 )
 
 
@@ -1037,8 +1038,13 @@ class _FakeRemoteReference(object):
     """
     local_object = attr.ib(type=object)
 
+    @defer.inlineCallbacks
     def callRemote(self, action, *args, **kwargs):
-        return getattr(self.local_object, action)(*args, **kwargs)
+        try:
+            result = yield getattr(self.local_object, action)(*args, **kwargs)
+            return result
+        except HTTPClientException as e:
+            raise RemoteException(e.args)
 
 
 @attr.s
diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py
index f3083a4bd..95261ddb2 100644
--- a/src/allmydata/test/test_istorageserver.py
+++ b/src/allmydata/test/test_istorageserver.py
@@ -1162,8 +1162,6 @@ class HTTPImmutableAPIsTests(
         "test_advise_corrupt_share",
         "test_bucket_advise_corrupt_share",
         "test_disconnection",
-        "test_matching_overlapping_writes",
-        "test_non_matching_overlapping_writes",
     }
 
 

From 5aa00abc3da0624541fe9e74aa61355cd59af8c0 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Fri, 11 Feb 2022 15:02:14 -0500
Subject: [PATCH 22/25] Use the correct API (since direct returns break Python
 2 imports)

---
 src/allmydata/storage_client.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py
index 17a6f79a5..c2e26b4b6 100644
--- a/src/allmydata/storage_client.py
+++ b/src/allmydata/storage_client.py
@@ -1042,7 +1042,7 @@ class _FakeRemoteReference(object):
     def callRemote(self, action, *args, **kwargs):
         try:
             result = yield getattr(self.local_object, action)(*args, **kwargs)
-            return result
+            defer.returnValue(result)
         except HTTPClientException as e:
             raise RemoteException(e.args)
 

From 60bcd5fe9f41b811cb2630424935977c6e1db674 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Mon, 7 Mar 2022 08:25:12 -0500
Subject: [PATCH 23/25] Address review comments.

---
 src/allmydata/test/test_storage_http.py | 47 +++++++++++++------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index c864f923c..c15749fc8 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -374,7 +374,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
             self.skipTest("Not going to bother supporting Python 2")
         super(ImmutableHTTPAPITests, self).setUp()
         self.http = self.useFixture(HttpTestFixture())
-        self.im_client = StorageClientImmutables(self.http.client)
+        self.imm_client = StorageClientImmutables(self.http.client)
 
     def create_upload(self, share_numbers, length):
         """
@@ -386,7 +386,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         lease_secret = urandom(32)
         storage_index = urandom(16)
         created = result_of(
-            self.im_client.create(
+            self.imm_client.create(
                 storage_index,
                 share_numbers,
                 length,
@@ -407,7 +407,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         that's already done in test_storage.py.
         """
         length = 100
-        expected_data = b"".join(bytes([i]) for i in range(100))
+        expected_data = bytes(range(100))
 
         # Create a upload:
         (upload_secret, _, storage_index, created) = self.create_upload({1}, 100)
@@ -421,7 +421,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         # Three writes: 10-19, 30-39, 50-59. This allows for a bunch of holes.
         def write(offset, length):
             remaining.empty(offset, offset + length)
-            return self.im_client.write_share_chunk(
+            return self.imm_client.write_share_chunk(
                 storage_index,
                 1,
                 upload_secret,
@@ -465,7 +465,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         # We can now read:
         for offset, length in [(0, 100), (10, 19), (99, 1), (49, 200)]:
             downloaded = result_of(
-                self.im_client.read_share_chunk(storage_index, 1, offset, length)
+                self.imm_client.read_share_chunk(storage_index, 1, offset, length)
             )
             self.assertEqual(downloaded, expected_data[offset : offset + length])
 
@@ -480,7 +480,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         )
         with self.assertRaises(ClientException) as e:
             result_of(
-                self.im_client.create(
+                self.imm_client.create(
                     storage_index, {2, 3}, 100, b"x" * 32, lease_secret, lease_secret
                 )
             )
@@ -498,7 +498,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
 
         # Add same shares:
         created2 = result_of(
-            self.im_client.create(
+            self.imm_client.create(
                 storage_index, {4, 6}, 100, b"x" * 2, lease_secret, lease_secret
             )
         )
@@ -511,12 +511,12 @@ class ImmutableHTTPAPITests(SyncTestCase):
         (upload_secret, _, storage_index, created) = self.create_upload({1, 2, 3}, 10)
 
         # Initially there are no shares:
-        self.assertEqual(result_of(self.im_client.list_shares(storage_index)), set())
+        self.assertEqual(result_of(self.imm_client.list_shares(storage_index)), set())
 
         # Upload shares 1 and 3:
         for share_number in [1, 3]:
             progress = result_of(
-                self.im_client.write_share_chunk(
+                self.imm_client.write_share_chunk(
                     storage_index,
                     share_number,
                     upload_secret,
@@ -527,7 +527,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
             self.assertTrue(progress.finished)
 
         # Now shares 1 and 3 exist:
-        self.assertEqual(result_of(self.im_client.list_shares(storage_index)), {1, 3})
+        self.assertEqual(result_of(self.imm_client.list_shares(storage_index)), {1, 3})
 
     def test_upload_bad_content_range(self):
         """
@@ -563,8 +563,8 @@ class ImmutableHTTPAPITests(SyncTestCase):
         """
         Listing unknown storage index's shares results in empty list of shares.
         """
-        storage_index = b"".join(bytes([i]) for i in range(16))
-        self.assertEqual(result_of(self.im_client.list_shares(storage_index)), set())
+        storage_index = bytes(range(16))
+        self.assertEqual(result_of(self.imm_client.list_shares(storage_index)), set())
 
     def test_upload_non_existent_storage_index(self):
         """
@@ -576,7 +576,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         def unknown_check(storage_index, share_number):
             with self.assertRaises(ClientException) as e:
                 result_of(
-                    self.im_client.write_share_chunk(
+                    self.imm_client.write_share_chunk(
                         storage_index,
                         share_number,
                         upload_secret,
@@ -598,7 +598,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         """
         (upload_secret, _, storage_index, _) = self.create_upload({1, 2}, 10)
         result_of(
-            self.im_client.write_share_chunk(
+            self.imm_client.write_share_chunk(
                 storage_index,
                 1,
                 upload_secret,
@@ -607,7 +607,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
             )
         )
         result_of(
-            self.im_client.write_share_chunk(
+            self.imm_client.write_share_chunk(
                 storage_index,
                 2,
                 upload_secret,
@@ -616,11 +616,11 @@ class ImmutableHTTPAPITests(SyncTestCase):
             )
         )
         self.assertEqual(
-            result_of(self.im_client.read_share_chunk(storage_index, 1, 0, 10)),
+            result_of(self.imm_client.read_share_chunk(storage_index, 1, 0, 10)),
             b"1" * 10,
         )
         self.assertEqual(
-            result_of(self.im_client.read_share_chunk(storage_index, 2, 0, 10)),
+            result_of(self.imm_client.read_share_chunk(storage_index, 2, 0, 10)),
             b"2" * 10,
         )
 
@@ -633,7 +633,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
 
         # Write:
         result_of(
-            self.im_client.write_share_chunk(
+            self.imm_client.write_share_chunk(
                 storage_index,
                 1,
                 upload_secret,
@@ -645,7 +645,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         # Conflicting write:
         with self.assertRaises(ClientException) as e:
             result_of(
-                self.im_client.write_share_chunk(
+                self.imm_client.write_share_chunk(
                     storage_index,
                     1,
                     upload_secret,
@@ -666,7 +666,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
             {share_number}, data_length
         )
         result_of(
-            self.im_client.write_share_chunk(
+            self.imm_client.write_share_chunk(
                 storage_index,
                 share_number,
                 upload_secret,
@@ -682,7 +682,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         """
         with self.assertRaises(ClientException) as e:
             result_of(
-                self.im_client.read_share_chunk(
+                self.imm_client.read_share_chunk(
                     b"1" * 16,
                     1,
                     0,
@@ -698,7 +698,7 @@ class ImmutableHTTPAPITests(SyncTestCase):
         storage_index, _ = self.upload(1)
         with self.assertRaises(ClientException) as e:
             result_of(
-                self.im_client.read_share_chunk(
+                self.imm_client.read_share_chunk(
                     storage_index,
                     7,  # different share number
                     0,
@@ -732,9 +732,12 @@ class ImmutableHTTPAPITests(SyncTestCase):
                 )
             self.assertEqual(e.exception.code, http.REQUESTED_RANGE_NOT_SATISFIABLE)
 
+        # Bad unit
         check_bad_range("molluscs=0-9")
+        # Negative offsets
         check_bad_range("bytes=-2-9")
         check_bad_range("bytes=0--10")
+        # Negative offset no endpoint
         check_bad_range("bytes=-300-")
         check_bad_range("bytes=")
         # Multiple ranges are currently unsupported, even if they're

From 4efa65d3dbfdd844e14537b6dd18683c8ed89092 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Mon, 7 Mar 2022 08:29:26 -0500
Subject: [PATCH 24/25] Typo.

---
 src/allmydata/storage/http_server.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index f89a156a3..9048359b1 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -133,7 +133,7 @@ class StorageIndexUploads(object):
     # Map share number to BucketWriter
     shares = attr.ib(factory=dict)  # type: Dict[int,BucketWriter]
 
-    # Mape share number to the upload secret (different shares might have
+    # Map share number to the upload secret (different shares might have
     # different upload secrets).
     upload_secrets = attr.ib(factory=dict)  # type: Dict[int,bytes]
 

From 87ab56426ad1634ef97cd2ca3805cc07038cb2a4 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Mon, 7 Mar 2022 08:38:31 -0500
Subject: [PATCH 25/25] Validate another edge case of bad storage index.

---
 src/allmydata/storage/http_server.py    | 8 ++++++--
 src/allmydata/test/test_storage_http.py | 7 ++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 9048359b1..0e1969593 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -18,12 +18,13 @@ else:
 
 from functools import wraps
 from base64 import b64decode
+import binascii
 
 from klein import Klein
 from twisted.web import http
 import attr
 from werkzeug.http import parse_range_header, parse_content_range_header
-from werkzeug.routing import BaseConverter
+from werkzeug.routing import BaseConverter, ValidationError
 from werkzeug.datastructures import ContentRange
 
 # TODO Make sure to use pure Python versions?
@@ -148,7 +149,10 @@ class StorageIndexConverter(BaseConverter):
     regex = "[" + str(rfc3548_alphabet, "ascii") + "]{26}"
 
     def to_python(self, value):
-        return si_a2b(value.encode("ascii"))
+        try:
+            return si_a2b(value.encode("ascii"))
+        except (AssertionError, binascii.Error, ValueError):
+            raise ValidationError("Invalid storage index")
 
 
 class HTTPServer(object):
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index c15749fc8..14f4437b6 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -191,10 +191,15 @@ class RouteConverterTests(SyncTestCase):
             self.adapter.match("/{}/".format("a" * 25), method="GET")
 
     def test_bad_characters_storage_index_is_not_parsed(self):
-        """An overly short storage_index string is not parsed."""
+        """A storage_index string with bad characters is not parsed."""
         with self.assertRaises(WNotFound):
             self.adapter.match("/{}_/".format("a" * 25), method="GET")
 
+    def test_invalid_storage_index_is_not_parsed(self):
+        """An invalid storage_index string is not parsed."""
+        with self.assertRaises(WNotFound):
+            self.adapter.match("/nomd2a65ylxjbqzsw7gcfh4ivr/", method="GET")
+
 
 # TODO should be actual swissnum
 SWISSNUM_FOR_TEST = b"abcd"