fix: preserve percent-encoding in presigned URLs to prevent signature invalidation (#662)

yarl.URL() normalizes %XX sequences (e.g. %40 → @), which breaks
S3/CloudFront presigned URL signatures that are computed over the
encoded form. This adds `_make_url(encoded=True)` to preserve encoding,
manual redirect following to avoid aiohttp re-encoding Location headers,
and an original_url passthrough so already-signed HTTP URLs bypass
OpenDAL's presign (which strips query params).

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

Author: bennyz

python/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py

@@ -64,9 +64,19 @@ async def OpendalAdapter(
     path: str,  # file path in storage backend relative to the storage root
     mode: Literal["rb", "wb"] = "rb",  # binary read or binary write mode
     compression: Compression | None = None,  # compression algorithm
+    original_url: str | None = None,  # original HTTP URL, bypasses OpenDAL presign to avoid path re-encoding
 ):
+    if original_url is not None and compression is not None:
+        raise ValueError("compression is not supported with direct HTTP URLs (original_url)")
+    if mode == "wb" and original_url is not None:
+        raise ValueError("original_url is not supported in write mode")
+    if mode == "rb" and compression is None and original_url is not None:
+        yield PresignedRequestResource(
+            headers={}, url=original_url, method="GET"
+        ).model_dump(mode="json")
+        return
     # if the access mode is binary read, and the storage backend supports presigned read requests
-    if mode == "rb" and operator.capability().presign_read and compression is None:
+    elif mode == "rb" and operator.capability().presign_read and compression is None:
         # create presigned url for the specified file with a 60 second expiration
         presigned = await operator.to_async_operator().presign_read(path, expire_second=60)
         yield PresignedRequestResource(

python/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py

@@ -51,7 +51,7 @@ def operator_for_path(path: PathBuf) -> tuple[PathBuf, Operator, str]:
         - the operator for the given path
         - the scheme of the operator.
     """
-    if type(path) is str and path.startswith(("http://", "https://")):
+    if isinstance(path, str) and path.startswith(("http://", "https://")):
         parsed_url = urlparse(path)
         operator = Operator("http", root="/", endpoint=f"{parsed_url.scheme}://{parsed_url.netloc}")
         return Path(parsed_url.path), operator, "http"
@@ -79,10 +79,13 @@ def write_from_path(self, path: PathBuf, operator: Operator | None = None):
         """
         Write into remote file with content from local file
         """
+        original_url = None
         if operator is None:
+            if isinstance(path, str) and path.startswith(("http://", "https://")):
+                original_url = path
             path, operator, _ = operator_for_path(path)
 
-        with OpendalAdapter(client=self.client, operator=operator, path=path) as handle:
+        with OpendalAdapter(client=self.client, operator=operator, path=path, original_url=original_url) as handle:
             return self.__write(handle)
 
     @validate_call(validate_return=True, config=ConfigDict(arbitrary_types_allowed=True))
@@ -630,10 +633,15 @@ def _flash_single(
         compression: Compression | None,
     ):
         """Flash image to DUT"""
+        original_url = None
         if operator is None:
+            if isinstance(image, str) and image.startswith(("http://", "https://")):
+                original_url = image
             image, operator, _ = operator_for_path(image)
 
-        with OpendalAdapter(client=self, operator=operator, path=image, mode="rb", compression=compression) as handle:
+        with OpendalAdapter(
+            client=self, operator=operator, path=image, mode="rb", compression=compression, original_url=original_url
+        ) as handle:
             return self.call("flash", handle, target)
 
     def flash(
@@ -763,10 +771,15 @@ def flash(
 
         self.host()
 
+        original_url = None
         if operator is None:
+            if isinstance(path, str) and path.startswith(("http://", "https://")):
+                original_url = path
             path, operator, _ = operator_for_path(path)
 
-        with OpendalAdapter(client=self, operator=operator, path=path, mode="rb", compression=compression) as handle:
+        with OpendalAdapter(
+            client=self, operator=operator, path=path, mode="rb", compression=compression, original_url=original_url
+        ) as handle:
             try:
                 return self.write(handle)
             finally:

python/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py

@@ -12,6 +12,7 @@
 
 from .common import PresignedRequest
 from .driver import MockFlasher, MockStorageMux, MockStorageMuxFlasher, Opendal
+from jumpstarter.client.core import DriverError
 from jumpstarter.common.utils import serve
 
 
@@ -322,3 +323,290 @@ def test_copy_and_rename_tracking(tmp_path):
     assert "copied_dir" in created_paths
     assert "renamed_dir" in created_paths
     assert len(created_paths) == 4
+
+
+def test_flash_http_url_preserves_percent_encoding():
+    """Flashing from HTTP URL with percent-encoded path must preserve encoding.
+
+    Without _make_url(encoded=True), yarl.URL decodes %40 to @ in the path,
+    so the HTTP request hits a different URL than intended. Presigned URL
+    signatures (S3, CloudFront) are computed over the encoded form and would
+    be rejected.
+    """
+    received_paths = []
+
+    class EncodingCheckHandler(BaseHTTPRequestHandler):
+        def do_HEAD(self):
+            self.send_response(200)
+            self.send_header("content-length", "4")
+            self.end_headers()
+
+        def do_GET(self):
+            received_paths.append(self.path)
+            self.send_response(200)
+            self.send_header("content-length", "4")
+            self.end_headers()
+            self.wfile.write(b"test")
+
+        def log_message(self, format, *args):
+            pass
+
+    with serve(MockFlasher()) as flasher:
+        server = HTTPServer(("127.0.0.1", 0), EncodingCheckHandler)
+        port = server.server_address[1]
+        server_thread = Thread(target=server.serve_forever)
+        server_thread.daemon = True
+        server_thread.start()
+
+        try:
+            flasher.flash(f"http://127.0.0.1:{port}/path%40encoded/file.bin")
+
+            assert len(received_paths) >= 1
+            assert "%40" in received_paths[-1], (
+                f"Server received decoded path {received_paths[-1]!r} — "
+                f"_make_url is not preserving percent-encoding"
+            )
+        finally:
+            server.shutdown()
+            server.server_close()
+            server_thread.join(timeout=2)
+
+
+def test_flash_http_redirect_preserves_percent_encoding():
+    """When a presigned URL returns a redirect, encoding in Location must be preserved.
+
+    aiohttp's automatic redirect following re-parses Location through yarl.URL(),
+    decoding %40 to @. The fix disables auto-redirect and manually follows with
+    _make_url(encoded=True).
+    """
+    received_paths = []
+
+    class RedirectHandler(BaseHTTPRequestHandler):
+        def do_HEAD(self):
+            self.send_response(200)
+            self.send_header("content-length", "4")
+            self.end_headers()
+
+        def do_GET(self):
+            received_paths.append(self.path)
+            if self.path.startswith("/start"):
+                port = self.server.server_address[1]
+                self.send_response(302)
+                self.send_header(
+                    "Location",
+                    f"http://127.0.0.1:{port}/redirect%40target/file.bin",
+                )
+                self.end_headers()
+            else:
+                self.send_response(200)
+                self.send_header("content-length", "4")
+                self.end_headers()
+                self.wfile.write(b"test")
+
+        def log_message(self, format, *args):
+            pass
+
+    with serve(MockFlasher()) as flasher:
+        server = HTTPServer(("127.0.0.1", 0), RedirectHandler)
+        port = server.server_address[1]
+        server_thread = Thread(target=server.serve_forever)
+        server_thread.daemon = True
+        server_thread.start()
+
+        try:
+            flasher.flash(f"http://127.0.0.1:{port}/start")
+
+            assert len(received_paths) == 2, (
+                f"Expected 2 requests (initial + redirect), got {len(received_paths)}"
+            )
+            assert received_paths[0] == "/start"
+            assert "%40" in received_paths[1], (
+                f"Redirect target received decoded path {received_paths[1]!r} — "
+                f"redirect following is not preserving percent-encoding"
+            )
+        finally:
+            server.shutdown()
+            server.server_close()
+            server_thread.join(timeout=2)
+
+
+def test_flash_http_chained_redirects_preserve_percent_encoding():
+    """Chained redirects (A->B->C) must all preserve percent-encoding."""
+    received_paths = []
+
+    class ChainedRedirectHandler(BaseHTTPRequestHandler):
+        def do_HEAD(self):
+            self.send_response(200)
+            self.send_header("content-length", "4")
+            self.end_headers()
+
+        def do_GET(self):
+            received_paths.append(self.path)
+            port = self.server.server_address[1]
+            if self.path.startswith("/hop1"):
+                self.send_response(302)
+                self.send_header(
+                    "Location",
+                    f"http://127.0.0.1:{port}/hop2%40middle/file.bin",
+                )
+                self.end_headers()
+            elif self.path.startswith("/hop2"):
+                self.send_response(302)
+                self.send_header(
+                    "Location",
+                    f"http://127.0.0.1:{port}/hop3%40final/file.bin",
+                )
+                self.end_headers()
+            else:
+                self.send_response(200)
+                self.send_header("content-length", "4")
+                self.end_headers()
+                self.wfile.write(b"test")
+
+        def log_message(self, format, *args):
+            pass
+
+    with serve(MockFlasher()) as flasher:
+        server = HTTPServer(("127.0.0.1", 0), ChainedRedirectHandler)
+        port = server.server_address[1]
+        server_thread = Thread(target=server.serve_forever)
+        server_thread.daemon = True
+        server_thread.start()
+
+        try:
+            flasher.flash(f"http://127.0.0.1:{port}/hop1")
+
+            assert len(received_paths) == 3, (
+                f"Expected 3 requests (hop1 + hop2 + hop3), got {len(received_paths)}"
+            )
+            assert received_paths[0] == "/hop1"
+            assert "%40" in received_paths[1], (
+                f"Hop 2 received decoded path {received_paths[1]!r}"
+            )
+            assert "%40" in received_paths[2], (
+                f"Hop 3 received decoded path {received_paths[2]!r}"
+            )
+        finally:
+            server.shutdown()
+            server.server_close()
+            server_thread.join(timeout=2)
+
+
+@pytest.mark.parametrize("status_code", [301, 302, 303, 307, 308])
+def test_flash_http_redirect_all_status_codes(status_code):
+    """All redirect status codes (301, 302, 303, 307, 308) must be followed with encoding preserved."""
+    received_paths = []
+
+    class StatusCodeRedirectHandler(BaseHTTPRequestHandler):
+        def do_HEAD(self):
+            self.send_response(200)
+            self.send_header("content-length", "4")
+            self.end_headers()
+
+        def do_GET(self):
+            received_paths.append(self.path)
+            if self.path.startswith("/start"):
+                port = self.server.server_address[1]
+                self.send_response(status_code)
+                self.send_header(
+                    "Location",
+                    f"http://127.0.0.1:{port}/target%40encoded/file.bin",
+                )
+                self.end_headers()
+            else:
+                self.send_response(200)
+                self.send_header("content-length", "4")
+                self.end_headers()
+                self.wfile.write(b"test")
+
+        def log_message(self, format, *args):
+            pass
+
+    with serve(MockFlasher()) as flasher:
+        server = HTTPServer(("127.0.0.1", 0), StatusCodeRedirectHandler)
+        port = server.server_address[1]
+        server_thread = Thread(target=server.serve_forever)
+        server_thread.daemon = True
+        server_thread.start()
+
+        try:
+            flasher.flash(f"http://127.0.0.1:{port}/start")
+
+            assert len(received_paths) == 2
+            assert received_paths[0] == "/start"
+            assert "%40" in received_paths[1], (
+                f"Status {status_code}: redirect target received decoded path {received_paths[1]!r}"
+            )
+        finally:
+            server.shutdown()
+            server.server_close()
+            server_thread.join(timeout=2)
+
+
+def test_flash_http_redirect_loop_raises():
+    """Redirect loop must raise RuntimeError after max_redirects."""
+
+    class LoopRedirectHandler(BaseHTTPRequestHandler):
+        def do_HEAD(self):
+            self.send_response(200)
+            self.send_header("content-length", "4")
+            self.end_headers()
+
+        def do_GET(self):
+            port = self.server.server_address[1]
+            self.send_response(302)
+            self.send_header(
+                "Location",
+                f"http://127.0.0.1:{port}/loop",
+            )
+            self.end_headers()
+
+        def log_message(self, format, *args):
+            pass
+
+    with serve(MockFlasher()) as flasher:
+        server = HTTPServer(("127.0.0.1", 0), LoopRedirectHandler)
+        port = server.server_address[1]
+        server_thread = Thread(target=server.serve_forever)
+        server_thread.daemon = True
+        server_thread.start()
+
+        try:
+            with pytest.raises(DriverError, match="Too many redirects"):
+                flasher.flash(f"http://127.0.0.1:{port}/loop")
+        finally:
+            server.shutdown()
+            server.server_close()
+            server_thread.join(timeout=2)
+
+
+def test_flash_http_redirect_missing_location_raises():
+    """302 without Location header must raise RuntimeError."""
+
+    class NoLocationHandler(BaseHTTPRequestHandler):
+        def do_HEAD(self):
+            self.send_response(200)
+            self.send_header("content-length", "4")
+            self.end_headers()
+
+        def do_GET(self):
+            self.send_response(302)
+            self.end_headers()
+
+        def log_message(self, format, *args):
+            pass
+
+    with serve(MockFlasher()) as flasher:
+        server = HTTPServer(("127.0.0.1", 0), NoLocationHandler)
+        port = server.server_address[1]
+        server_thread = Thread(target=server.serve_forever)
+        server_thread.daemon = True
+        server_thread.start()
+
+        try:
+            with pytest.raises(DriverError, match="missing Location header"):
+                flasher.flash(f"http://127.0.0.1:{port}/start")
+        finally:
+            server.shutdown()
+            server.server_close()
+            server_thread.join(timeout=2)

python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py

@@ -47,7 +47,7 @@ def _upload_file_if_needed(self, file_path: str, operator: Operator | None = Non
                 self.logger.info(f"Uploading {file_path} to storage as {filename}")
 
             try:
-                self.storage.write_from_path(filename, path_buf, operator=operator)
+                self.storage.write_from_path(filename, file_path, operator=operator)
             except Exception as e:
                 raise RideSXFlashError(
                     f"Failed to transfer '{file_path}' to exporter storage as '{filename}' (scheme={operator_scheme})"