Skip to content

Commit 79b7fae

Browse files
committed
fix: detect original_url before operator guard to fix encoding with explicit operators
When callers pass an explicit operator (e.g. after resolving via operator_for_path()), the original_url detection was skipped because it was nested inside `if operator is None`. This caused HTTP URLs to go through OpenDAL presign_read, mangling percent-encoded paths. Move the URL check before the guard in all three methods. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
1 parent 72e0ed8 commit 79b7fae

2 files changed

Lines changed: 74 additions & 6 deletions

File tree

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,9 @@ def write_from_path(self, path: PathBuf, operator: Operator | None = None):
8080
Write into remote file with content from local file
8181
"""
8282
original_url = None
83+
if isinstance(path, str) and path.startswith(("http://", "https://")):
84+
original_url = path
8385
if operator is None:
84-
if isinstance(path, str) and path.startswith(("http://", "https://")):
85-
original_url = path
8686
path, operator, _ = operator_for_path(path)
8787

8888
with OpendalAdapter(client=self.client, operator=operator, path=path, original_url=original_url) as handle:
@@ -634,9 +634,9 @@ def _flash_single(
634634
):
635635
"""Flash image to DUT"""
636636
original_url = None
637+
if isinstance(image, str) and image.startswith(("http://", "https://")):
638+
original_url = image
637639
if operator is None:
638-
if isinstance(image, str) and image.startswith(("http://", "https://")):
639-
original_url = image
640640
image, operator, _ = operator_for_path(image)
641641

642642
with OpendalAdapter(
@@ -772,9 +772,9 @@ def flash(
772772
self.host()
773773

774774
original_url = None
775+
if isinstance(path, str) and path.startswith(("http://", "https://")):
776+
original_url = path
775777
if operator is None:
776-
if isinstance(path, str) and path.startswith(("http://", "https://")):
777-
original_url = path
778778
path, operator, _ = operator_for_path(path)
779779

780780
with OpendalAdapter(

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import hashlib
22
import os
3+
from contextlib import contextmanager
34
from http.server import BaseHTTPRequestHandler, HTTPServer
45
from pathlib import Path
56
from random import randbytes
@@ -325,6 +326,73 @@ def test_copy_and_rename_tracking(tmp_path):
325326
assert len(created_paths) == 4
326327

327328

329+
@contextmanager
330+
def _http_path_recording_server():
331+
"""Start an HTTP server that records request paths and serves minimal responses."""
332+
received_paths = []
333+
334+
class Handler(BaseHTTPRequestHandler):
335+
def do_HEAD(self):
336+
self.send_response(200)
337+
self.send_header("content-length", "4")
338+
self.end_headers()
339+
340+
def do_GET(self):
341+
received_paths.append(self.path)
342+
self.send_response(200)
343+
self.send_header("content-length", "4")
344+
self.end_headers()
345+
self.wfile.write(b"data")
346+
347+
def log_message(self, format, *args):
348+
pass
349+
350+
server = HTTPServer(("127.0.0.1", 0), Handler)
351+
port = server.server_address[1]
352+
thread = Thread(target=server.serve_forever, daemon=True)
353+
thread.start()
354+
try:
355+
yield port, received_paths
356+
finally:
357+
server.shutdown()
358+
server.server_close()
359+
thread.join(timeout=2)
360+
361+
362+
def _assert_encoding_preserved(received_paths):
363+
assert len(received_paths) >= 1
364+
assert "%40" in received_paths[-1], (
365+
f"Server received decoded path {received_paths[-1]!r} — "
366+
f"original_url bypass did not activate with explicit operator"
367+
)
368+
369+
370+
def test_write_from_path_http_with_explicit_operator(tmp_path):
371+
"""write_from_path must use original_url bypass even when operator is passed explicitly.
372+
373+
Callers like RideSX resolve the operator themselves via operator_for_path() and
374+
pass it in. The original_url detection must happen before the `if operator is None`
375+
guard, otherwise the HTTP URL goes through OpenDAL presign_read which mangles it
376+
into a double-host path like endpoint/https%3A/host/path.
377+
"""
378+
with serve(Opendal(scheme="fs", kwargs={"root": str(tmp_path)})) as client:
379+
with _http_path_recording_server() as (port, received_paths):
380+
url = f"http://127.0.0.1:{port}/path%40encoded/file.bin"
381+
explicit_operator = Operator("http", endpoint=f"http://127.0.0.1:{port}")
382+
client.write_from_path("dest.bin", url, operator=explicit_operator)
383+
_assert_encoding_preserved(received_paths)
384+
385+
386+
def test_flash_http_with_explicit_operator():
387+
"""FlasherClient.flash must use original_url bypass even when operator is passed explicitly."""
388+
with serve(MockFlasher()) as flasher:
389+
with _http_path_recording_server() as (port, received_paths):
390+
url = f"http://127.0.0.1:{port}/path%40encoded/file.bin"
391+
explicit_operator = Operator("http", endpoint=f"http://127.0.0.1:{port}")
392+
flasher.flash(url, operator=explicit_operator)
393+
_assert_encoding_preserved(received_paths)
394+
395+
328396
def test_flash_http_url_preserves_percent_encoding():
329397
"""Flashing from HTTP URL with percent-encoded path must preserve encoding.
330398

0 commit comments

Comments
 (0)