Update extstore s3 driver key percent encoding to be consistent with other SDKs and S3's safe character guidelines. (#1637)

Author: cconstable

temporalio/contrib/aws/s3driver/README.md

@@ -58,9 +58,23 @@ driver = S3StorageDriver(client=MyS3Client(), bucket="my-temporal-payloads")
 
 ### Key structure
 
-Payloads are stored under content-addressable keys derived from a SHA-256 hash of the serialized payload bytes, segmented by namespace and workflow/activity identifiers when serialization context is available, e.g.:
+Payloads are stored under content-addressable keys derived from a SHA-256 hash of the serialized payload bytes, segmented by namespace and workflow/activity identifiers when serialization context is available.
 
-    v0/ns/my-namespace/wfi/my-workflow-id/d/sha256/<hash>
+Workflow key:
+
+    v0/ns/{namespace}/wt/{workflow-type}/wi/{workflow-id}/ri/{run-id}/d/{hash-algorithm}/{hex-digest}
+
+Activity key:
+
+    v0/ns/{namespace}/at/{activity-type}/ai/{activity-id}/ri/{run-id}/d/{hash-algorithm}/{hex-digest}
+
+Fallback key (used when no namespace, workflow, or activity information is available):
+
+    v0/d/{hash-algorithm}/{hex-digest}
+
+- Missing values (including a missing run ID) are encoded as the literal `null`.
+- `hex-digest` is the lower-case SHA-256 hex digest (64 characters).
+- Dynamic path segments are percent-encoded: any byte outside S3's [safe character set](https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html) (`0-9`, `a-z`, `A-Z`, and `! - _ . * ' ( )`) is escaped as `%XX` over its UTF-8 bytes.
 
 ### Notes
 

temporalio/contrib/aws/s3driver/_driver.py

@@ -8,7 +8,7 @@
 
 import asyncio
 import hashlib
-import urllib.parse
+import string
 from collections.abc import Callable, Coroutine, Sequence
 from typing import Any, TypeVar
 
@@ -25,6 +25,26 @@
 
 _T = TypeVar("_T")
 
+# S3's safe character set for object key names. See
+# https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html
+_S3_SAFE_CHARS = frozenset(string.ascii_letters + string.digits + "!-_.*'()")
+
+
+def _percent_encode(val: str | None) -> str | None:
+    """Percent-encode a path segment per the S3 key spec.
+
+    Encodes every byte outside :data:`_S3_SAFE_CHARS` as ``%XX`` with
+    upper-case hex digits, operating on the UTF-8 bytes so non-ASCII
+    characters are escaped byte by byte. Returns ``None`` for empty or
+    missing values so callers can substitute ``"null"``.
+    """
+    if not val:
+        return None
+    return "".join(
+        chr(byte) if chr(byte) in _S3_SAFE_CHARS else f"%{byte:02X}"
+        for byte in val.encode("utf-8")
+    )
+
 
 def _format_client_context(client: S3StorageDriverClient) -> str:
     """Format the client's ``describe()`` output as ", k=v, k=v" for error
@@ -127,24 +147,20 @@ async def store(
         (e.g. proto binary). The returned list is the same length as
         ``payloads``.
         """
-
-        def _quote(val: str | None) -> str | None:
-            return urllib.parse.quote(val, safe="") if val else None
-
         # Build context segments from the target identity.
         context_segments = ""
         target = context.target
-        namespace = _quote(target.namespace) if target is not None else None
+        namespace = _percent_encode(target.namespace) if target is not None else None
         namespace_segment = f"/ns/{namespace}" if namespace else ""
         if isinstance(target, StorageDriverWorkflowInfo):
-            wf_type = _quote(target.type) or "null"
-            wf_id = _quote(target.id) or "null"
-            wf_run_id = _quote(target.run_id) or "null"
+            wf_type = _percent_encode(target.type) or "null"
+            wf_id = _percent_encode(target.id) or "null"
+            wf_run_id = _percent_encode(target.run_id) or "null"
             context_segments = f"/wt/{wf_type}/wi/{wf_id}/ri/{wf_run_id}"
         elif isinstance(target, StorageDriverActivityInfo):
-            act_type = _quote(target.type) or "null"
-            act_id = _quote(target.id) or "null"
-            act_run_id = _quote(target.run_id) or "null"
+            act_type = _percent_encode(target.type) or "null"
+            act_id = _percent_encode(target.id) or "null"
+            act_run_id = _percent_encode(target.run_id) or "null"
             context_segments = f"/at/{act_type}/ai/{act_id}/ri/{act_run_id}"
 
         async def _upload(payload: Payload) -> StorageDriverClaim:

tests/contrib/aws/s3driver/test_s3driver.py

@@ -336,6 +336,88 @@ async def test_key_urlencodes_namespace(
             == f"v0/ns/my%2Fns%231/wt/null/wi/wf1/ri/null/d/sha256/{expected_hash}"
         )
 
+    async def test_key_preserves_s3_safe_special_chars(
+        self, driver_client: S3StorageDriverClient
+    ) -> None:
+        """S3's safe special characters are left unescaped per the key spec."""
+        driver = S3StorageDriver(client=driver_client, bucket=BUCKET)
+        payload = make_payload()
+        # Every special character S3 lists as safe: ! - _ . * ' ( )
+        ctx = make_workflow_context(namespace="ns1", workflow_id="!-_.*'()")
+        [claim] = await driver.store(ctx, [payload])
+        expected_hash = hashlib.sha256(payload.SerializeToString()).hexdigest()
+        assert (
+            claim.claim_data["key"]
+            == f"v0/ns/ns1/wt/null/wi/!-_.*'()/ri/null/d/sha256/{expected_hash}"
+        )
+
+    async def test_key_escapes_tilde(
+        self, driver_client: S3StorageDriverClient
+    ) -> None:
+        """Tilde is not in S3's safe set and must be percent-encoded (%7E)."""
+        driver = S3StorageDriver(client=driver_client, bucket=BUCKET)
+        payload = make_payload()
+        ctx = make_workflow_context(namespace="ns1", workflow_id="wf~1")
+        [claim] = await driver.store(ctx, [payload])
+        expected_hash = hashlib.sha256(payload.SerializeToString()).hexdigest()
+        assert (
+            claim.claim_data["key"]
+            == f"v0/ns/ns1/wt/null/wi/wf%7E1/ri/null/d/sha256/{expected_hash}"
+        )
+
+    async def test_key_escapes_non_ascii_as_utf8_bytes(
+        self, driver_client: S3StorageDriverClient
+    ) -> None:
+        """Non-ASCII characters are percent-encoded byte by byte from UTF-8."""
+        driver = S3StorageDriver(client=driver_client, bucket=BUCKET)
+        payload = make_payload()
+        # "é" is U+00E9, which is 0xC3 0xA9 in UTF-8.
+        ctx = make_workflow_context(namespace="ns1", workflow_id="café")
+        [claim] = await driver.store(ctx, [payload])
+        expected_hash = hashlib.sha256(payload.SerializeToString()).hexdigest()
+        assert (
+            claim.claim_data["key"]
+            == f"v0/ns/ns1/wt/null/wi/caf%C3%A9/ri/null/d/sha256/{expected_hash}"
+        )
+
+    async def test_key_matches_spec_workflow_example(
+        self, driver_client: S3StorageDriverClient
+    ) -> None:
+        """Reproduces the workflow key example from the S3 key spec."""
+        driver = S3StorageDriver(client=driver_client, bucket=BUCKET)
+        payload = make_payload()
+        ctx = make_workflow_context(
+            namespace="payments prod",
+            workflow_type="ChargeWorkflow",
+            workflow_id="order+123=abc",
+            run_id="3f1d6c7a-8b2e-4f7a-9d0a-87a6f95e4d31",
+        )
+        [claim] = await driver.store(ctx, [payload])
+        expected_hash = hashlib.sha256(payload.SerializeToString()).hexdigest()
+        assert claim.claim_data["key"] == (
+            "v0/ns/payments%20prod/wt/ChargeWorkflow/wi/order%2B123%3Dabc"
+            f"/ri/3f1d6c7a-8b2e-4f7a-9d0a-87a6f95e4d31/d/sha256/{expected_hash}"
+        )
+
+    async def test_key_matches_spec_activity_example(
+        self, driver_client: S3StorageDriverClient
+    ) -> None:
+        """Reproduces the activity key example from the S3 key spec."""
+        driver = S3StorageDriver(client=driver_client, bucket=BUCKET)
+        payload = make_payload()
+        ctx = make_activity_context(
+            namespace="payments prod",
+            activity_type="Capture/Charge",
+            activity_id="activity id+42",
+            run_id="9e1d1fd9-2f8a-4c40-93e2-731f31b9268b",
+        )
+        [claim] = await driver.store(ctx, [payload])
+        expected_hash = hashlib.sha256(payload.SerializeToString()).hexdigest()
+        assert claim.claim_data["key"] == (
+            "v0/ns/payments%20prod/at/Capture%2FCharge/ai/activity%20id%2B42"
+            f"/ri/9e1d1fd9-2f8a-4c40-93e2-731f31b9268b/d/sha256/{expected_hash}"
+        )
+
     async def test_key_urlencoded_roundtrip(
         self, driver_client: S3StorageDriverClient
     ) -> None: