🐛 Fix: file uploads due to bad path encoding (#6287)

Author: pcrespov

services/web/server/src/simcore_service_webserver/storage/_handlers.py

@@ -37,30 +37,45 @@
 
 def _get_base_storage_url(app: web.Application) -> URL:
     settings: StorageSettings = get_plugin_settings(app)
-
-    # storage service API endpoint
-    return URL(settings.base_url)
+    return URL(settings.base_url, encoded=True)
 
 
-def _get_storage_prefix(app: web.Application) -> str:
+def _get_storage_vtag(app: web.Application) -> str:
     settings: StorageSettings = get_plugin_settings(app)
     storage_prefix: str = settings.STORAGE_VTAG
     return storage_prefix
 
 
-def _resolve_storage_url(request: web.Request) -> URL:
-    """Composes a new url against storage API"""
+def _to_storage_url(request: web.Request) -> URL:
+    """Converts web-api url to storage-api url"""
     userid = request[RQT_USERID_KEY]
 
     # storage service API endpoint
-    endpoint = _get_base_storage_url(request.app)
+    url = _get_base_storage_url(request.app)
 
     basepath_index = 3
     # strip basepath from webserver API path (i.e. webserver api version)
     # >>> URL('http://storage:1234/v5/storage/asdf/').raw_parts[3:]
     suffix = "/".join(request.url.raw_parts[basepath_index:])
 
-    return (endpoint / suffix).with_query(request.query).update_query(user_id=userid)
+    return (
+        url.joinpath(suffix, encoded=True)
+        .with_query(request.query)
+        .update_query(user_id=userid)
+    )
+
+
+def _from_storage_url(request: web.Request, storage_url: AnyUrl) -> AnyUrl:
+    """Converts storage-api url to web-api url"""
+    assert storage_url.path  # nosec
+
+    prefix = f"/{_get_storage_vtag(request.app)}"
+    converted_url = request.url.with_path(
+        f"/v0/storage{storage_url.path.removeprefix(prefix)}", encoded=True
+    ).with_scheme(request.headers.get(X_FORWARDED_PROTO, request.url.scheme))
+
+    webserver_url: AnyUrl = parse_obj_as(AnyUrl, f"{converted_url}")
+    return webserver_url
 
 
 class _ResponseTuple(NamedTuple):
@@ -71,7 +86,7 @@ class _ResponseTuple(NamedTuple):
 async def _forward_request_to_storage(
     request: web.Request, method: str, body: dict[str, Any] | None = None, **kwargs
 ) -> _ResponseTuple:
-    url = _resolve_storage_url(request)
+    url = _to_storage_url(request)
     session = get_client_session(request.app)
 
     async with session.request(
@@ -81,16 +96,6 @@ async def _forward_request_to_storage(
         return _ResponseTuple(payload=payload, status_code=resp.status)
 
 
-def _unresolve_storage_url(request: web.Request, storage_url: AnyUrl) -> AnyUrl:
-    assert storage_url.path  # nosec
-    prefix = f"/{_get_storage_prefix(request.app)}"
-    converted_url = request.url.with_path(
-        f"/v0/storage{storage_url.path.removeprefix(prefix)}"
-    ).with_scheme(request.headers.get(X_FORWARDED_PROTO, request.url.scheme))
-    converted_url_: AnyUrl = parse_obj_as(AnyUrl, f"{converted_url}")
-    return converted_url_
-
-
 # ---------------------------------------------------------------------
 
 routes = web.RouteTableDef()
@@ -233,10 +238,10 @@ class _QueryParams(BaseModel):
     payload, status = await _forward_request_to_storage(request, "PUT", body=None)
     data, _ = unwrap_envelope(payload)
     file_upload_schema = FileUploadSchema.parse_obj(data)
-    file_upload_schema.links.complete_upload = _unresolve_storage_url(
+    file_upload_schema.links.complete_upload = _from_storage_url(
         request, file_upload_schema.links.complete_upload
     )
-    file_upload_schema.links.abort_upload = _unresolve_storage_url(
+    file_upload_schema.links.abort_upload = _from_storage_url(
         request, file_upload_schema.links.abort_upload
     )
     return create_data_response(jsonable_encoder(file_upload_schema), status=status)
@@ -261,7 +266,7 @@ class _PathParams(BaseModel):
     )
     data, _ = unwrap_envelope(payload)
     file_upload_complete = FileUploadCompleteResponse.parse_obj(data)
-    file_upload_complete.links.state = _unresolve_storage_url(
+    file_upload_complete.links.state = _from_storage_url(
         request, file_upload_complete.links.state
     )
     return create_data_response(jsonable_encoder(file_upload_complete), status=status)

services/web/server/tests/unit/isolated/test_utils.py

@@ -18,25 +18,33 @@
 
 
 def test_yarl_new_url_generation():
-    api_endpoint = URL("http://director:8001/v0")
+    base_url_with_autoencode = URL("http://director:8001/v0", encoded=False)
     service_key = "simcore/services/dynamic/smash"
     service_version = "1.0.3"
 
+    # NOTE: careful, first we need to encode the "/" in this file path.
+    # For that we need safe="" option
+    assert urllib.parse.quote("/") == "/"
+    assert urllib.parse.quote("/", safe="") == "%2F"
+    assert urllib.parse.quote("%2F", safe="") == "%252F"
+
     quoted_service_key = urllib.parse.quote(service_key, safe="")
 
     # Since 1.6.x composition using '/' creates URLs with auto-encoding enabled by default
     assert (
-        (str(api_endpoint / "services" / quoted_service_key / service_version))
+        str(
+            base_url_with_autoencode / "services" / quoted_service_key / service_version
+        )
         == "http://director:8001/v0/services/simcore%252Fservices%252Fdynamic%252Fsmash/1.0.3"
     )
 
     # Passing encoded=True parameter prevents URL auto-encoding, user is responsible about URL correctness
-    url = URL(
+    url_without_autoencode = URL(
         f"http://director:8001/v0/services/{quoted_service_key}/1.0.3", encoded=True
     )
 
     assert (
-        (str(url))
+        str(url_without_autoencode)
         == "http://director:8001/v0/services/simcore%2Fservices%2Fdynamic%2Fsmash/1.0.3"
     )
 

services/web/server/tests/unit/with_dbs/03/test_storage_handlers.py

@@ -5,10 +5,13 @@
 
 
 import json
+import urllib.parse
 from typing import Any
 
 import pytest
-from aiohttp.test_utils import TestClient
+from aiohttp import web
+from aiohttp.test_utils import TestClient, make_mocked_request
+from faker import Faker
 from models_library.api_schemas_storage import (
     FileUploadCompleteResponse,
     FileUploadLinks,
@@ -20,16 +23,26 @@
 from pytest_simcore.helpers.typing_env import EnvVarsDict
 from pytest_simcore.helpers.webserver_login import UserInfoDict
 from servicelib.aiohttp.rest_responses import wrap_as_envelope
+from servicelib.request_keys import RQT_USERID_KEY
 from simcore_postgres_database.models.users import UserRole
+from simcore_service_webserver.application_settings import setup_settings
+from simcore_service_webserver.storage._handlers import (
+    _from_storage_url,
+    _to_storage_url,
+)
+from yarl import URL
 
 
 @pytest.fixture
-def app_environment(app_environment: EnvVarsDict, monkeypatch: pytest.MonkeyPatch):
+def app_environment(
+    app_environment: EnvVarsDict, monkeypatch: pytest.MonkeyPatch
+) -> EnvVarsDict:
     return app_environment | setenvs_from_dict(
         monkeypatch,
         {
             "WEBSERVER_DB_LISTENER": "0",
             "WEBSERVER_GARBAGE_COLLECTOR": "null",
+            "STORAGE_HOST": "fake-storage",
         },
     )
 
@@ -49,7 +62,7 @@ def _resolve(*args, **kwargs) -> AnyUrl:
         return parse_obj_as(AnyUrl, "http://private-url")
 
     mocker.patch(
-        "simcore_service_webserver.storage._handlers._unresolve_storage_url",
+        "simcore_service_webserver.storage._handlers._from_storage_url",
         autospec=True,
         side_effect=_resolve,
     )
@@ -88,7 +101,7 @@ def _resolve(*args, **kwargs) -> AnyUrl:
 
 @pytest.mark.parametrize("user_role", [UserRole.USER])
 @pytest.mark.parametrize(
-    "file_id", [DOUBLE_ENCODE_SLASH_IN_FILE_ID, SINGLE_ENCODE_SLASH_IN_FILE_ID]
+    "file_id", [SINGLE_ENCODE_SLASH_IN_FILE_ID, DOUBLE_ENCODE_SLASH_IN_FILE_ID]
 )
 @pytest.mark.parametrize(
     "method, path, body, expected_response",
@@ -159,3 +172,48 @@ async def test_openapi_regression_test(
     decoded_response = await response.json()
     assert decoded_response["error"] is None
     assert decoded_response["data"] is not None
+
+
+def test_url_storage_resolver_helpers(faker: Faker, app_environment: EnvVarsDict):
+
+    app = web.Application()
+    setup_settings(app)
+
+    # NOTE: careful, first we need to encode the "/" in this file path.
+    # For that we need safe="" option
+    assert urllib.parse.quote("/") == "/"
+    assert urllib.parse.quote("/", safe="") == "%2F"
+    assert urllib.parse.quote("%2F", safe="") == "%252F"
+
+    file_id = urllib.parse.quote(f"{faker.uuid4()}/{faker.uuid4()}/file.py", safe="")
+    assert "%2F" in file_id
+    assert "%252F" not in file_id
+
+    url = URL(f"/v0/storage/locations/0/files/{file_id}:complete", encoded=True)
+    assert url.raw_parts[-1] == f"{file_id}:complete"
+
+    web_request = make_mocked_request("GET", str(url), app=app)
+    web_request[RQT_USERID_KEY] = faker.pyint()
+
+    # web -> storage
+    storage_url = _to_storage_url(web_request)
+    # Something like
+    # http://storage:123/v5/locations/0/files/e3e70...c07cd%2Ff7...55%2Ffile.py:complete?user_id=8376
+
+    assert storage_url.raw_parts[-1] == web_request.url.raw_parts[-1]
+
+    assert storage_url.host == app_environment["STORAGE_HOST"]
+    assert storage_url.port == int(app_environment["STORAGE_PORT"])
+    assert storage_url.query["user_id"] == str(web_request[RQT_USERID_KEY])
+
+    # storage -> web
+    web_url: AnyUrl = _from_storage_url(
+        web_request, parse_obj_as(AnyUrl, str(storage_url))
+    )
+
+    assert storage_url.host != web_url.host
+    assert storage_url.port != web_url.port
+
+    assert isinstance(storage_url, URL)  # this is a bit inconvenient
+    assert isinstance(web_url, AnyUrl)
+    assert str(web_url) == str(web_request.url)