From 0df7ad2d965da058b0927f6e2066ccc5fd42f337 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Fri, 31 Jul 2026 22:26:52 +0200 Subject: [PATCH] feat(types): Support Unicode in object metadata headers --- Cargo.lock | 3 +- .../python/src/objectstore_client/client.py | 12 +- .../python/src/objectstore_client/metadata.py | 8 +- .../python/src/objectstore_client/utils.py | 33 ++- clients/python/tests/test_e2e.py | 36 +++ clients/python/tests/test_utils.py | 40 +++ clients/rust/Cargo.toml | 1 - clients/rust/src/many.rs | 18 +- clients/rust/tests/e2e.rs | 48 ++++ objectstore-server/Cargo.toml | 1 - objectstore-server/src/batch.rs | 7 +- objectstore-server/src/endpoints/batch.rs | 7 +- objectstore-server/src/endpoints/objects.rs | 38 ++- objectstore-server/src/extractors/batch.rs | 25 +- objectstore-server/tests/objects.rs | 94 ++++++ objectstore-service/src/backend/gcs.rs | 105 +++++-- .../src/backend/s3_compatible.rs | 25 ++ objectstore-types/Cargo.toml | 1 + objectstore-types/src/headers.rs | 270 ++++++++++++++++++ objectstore-types/src/lib.rs | 1 + objectstore-types/src/metadata.rs | 72 ++++- 21 files changed, 753 insertions(+), 92 deletions(-) create mode 100644 objectstore-types/src/headers.rs diff --git a/Cargo.lock b/Cargo.lock index 65e00e3b..a821a8c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2755,7 +2755,6 @@ dependencies = [ "multer", "objectstore-test", "objectstore-types", - "percent-encoding", "reqwest 0.13.4", "sentry-core", "serde", @@ -2832,7 +2831,6 @@ dependencies = [ "objectstore-test", "objectstore-types", "papaya", - "percent-encoding", "pin-project-lite", "rand 0.10.2", "reqwest 0.13.4", @@ -2930,6 +2928,7 @@ dependencies = [ "humantime-serde", "insta", "mediatype", + "percent-encoding", "serde", "serde_json", "thiserror", diff --git a/clients/python/src/objectstore_client/client.py b/clients/python/src/objectstore_client/client.py index 6efff4c5..40267935 100644 --- a/clients/python/src/objectstore_client/client.py +++ b/clients/python/src/objectstore_client/client.py @@ -354,14 +354,14 @@ def put( headers[HEADER_EXPIRATION] = format_expiration(expiration_policy) if origin: - headers[HEADER_ORIGIN] = origin + headers[HEADER_ORIGIN] = utils.encode_header_value(origin) if filename is not None: - headers[HEADER_FILENAME] = filename + headers[HEADER_FILENAME] = utils.encode_header_value(filename) if metadata: for k, v in metadata.items(): - headers[f"{HEADER_META_PREFIX}{k}"] = v + headers[f"{HEADER_META_PREFIX}{k}"] = utils.encode_header_value(v) if key == "": key = None @@ -619,14 +619,14 @@ def initiate_multipart_upload( headers[HEADER_EXPIRATION] = format_expiration(expiration_policy) if origin: - headers[HEADER_ORIGIN] = origin + headers[HEADER_ORIGIN] = utils.encode_header_value(origin) if filename is not None: - headers[HEADER_FILENAME] = filename + headers[HEADER_FILENAME] = utils.encode_header_value(filename) if metadata: for k, v in metadata.items(): - headers[f"{HEADER_META_PREFIX}{k}"] = v + headers[f"{HEADER_META_PREFIX}{k}"] = utils.encode_header_value(v) if key == "": key = None diff --git a/clients/python/src/objectstore_client/metadata.py b/clients/python/src/objectstore_client/metadata.py index ce4f50b3..a1c9d093 100644 --- a/clients/python/src/objectstore_client/metadata.py +++ b/clients/python/src/objectstore_client/metadata.py @@ -7,6 +7,8 @@ from datetime import datetime, timedelta from typing import Literal, TypeVar, cast +from objectstore_client.utils import decode_header_value + Compression = Literal["zstd"] | Literal["none"] HEADER_EXPIRATION = "x-sn-expiration" @@ -108,13 +110,13 @@ def from_headers(cls, headers: Mapping[str, str]) -> Metadata: elif k == HEADER_TIME_EXPIRES: time_expires = datetime.fromisoformat(v) elif k == HEADER_ORIGIN: - origin = v + origin = decode_header_value(v) elif k == HEADER_FILENAME: - filename = v + filename = decode_header_value(v) elif k == HEADER_SIZE: size = int(v) elif k.startswith(HEADER_META_PREFIX): - custom_metadata[k[len(HEADER_META_PREFIX) :]] = v + custom_metadata[k[len(HEADER_META_PREFIX) :]] = decode_header_value(v) return Metadata( content_type=content_type, diff --git a/clients/python/src/objectstore_client/utils.py b/clients/python/src/objectstore_client/utils.py index 8e7996f0..32d72941 100644 --- a/clients/python/src/objectstore_client/utils.py +++ b/clients/python/src/objectstore_client/utils.py @@ -2,7 +2,7 @@ import io from typing import IO, Any -from urllib.parse import quote +from urllib.parse import quote, unquote import filetype # type: ignore[import-untyped] from zstandard import ZstdCompressionReader @@ -22,6 +22,16 @@ _PATH_SAFE = "/:@!$&'()*+,;=" _QUERY_SAFE = _PATH_SAFE + "?" +# Characters left unescaped in a metadata header value: every visible ASCII +# character except "%". Non-ASCII, the C0 controls, and DEL are escaped because a +# header value cannot carry them; "%" is escaped so that a literal percent sign in +# a value can never be confused with an escape sequence when decoding. +# +# This is byte-for-byte identical to the Rust `HEADER_ESCAPE` set (see +# `objectstore-types/src/headers.rs`), so both clients and the server agree on the +# wire form. Values that are already plain ASCII are left untouched. +_HEADER_VALUE_SAFE = "".join(chr(b) for b in range(0x20, 0x7F) if b != ord("%")) + def encode_path(path: str) -> str: """Percent-encodes a request path as it will appear on the wire. @@ -42,6 +52,27 @@ def encode_query(query: str) -> str: return quote(query, safe=_QUERY_SAFE) +def encode_header_value(value: str) -> str: + """Percent-encodes a free-form metadata string for an HTTP header value. + + HTTP header values carry only visible ASCII, so anything outside that range has + to be escaped to survive the transport (see :data:`_HEADER_VALUE_SAFE`). This is + the inverse of :func:`decode_header_value`; only the transport representation is + escaped, never the logical string held in :class:`Metadata`. + """ + return quote(value, safe=_HEADER_VALUE_SAFE) + + +def decode_header_value(value: str) -> str: + """Decodes a percent-encoded HTTP header value into its logical string. + + Inverse of :func:`encode_header_value`. Values without escape sequences are + returned unchanged, so headers written before the encoding existed still read + back as-is. + """ + return unquote(value, errors="strict") + + def parse_accept_encoding(header: str) -> list[str]: """Parse an Accept-Encoding header value for use in objectstore GET requests. diff --git a/clients/python/tests/test_e2e.py b/clients/python/tests/test_e2e.py index 49b13309..1ee70cdb 100644 --- a/clients/python/tests/test_e2e.py +++ b/clients/python/tests/test_e2e.py @@ -222,6 +222,42 @@ def test_full_cycle_with_origin(server_url: str) -> None: assert retrieved.metadata.filename == "report.pdf" +def test_full_cycle_with_unicode_metadata(server_url: str) -> None: + client = Client( + server_url, + token=TestSecretKey.get(), + ) + test_usecase = Usecase( + "test-usecase", + expiration_policy=TimeToLive(timedelta(days=1)), + ) + + session = client.session(test_usecase, org=42, project=1337) + + object_key = session.put( + b"test data", + origin="Ünknown-源", + filename="réport-📄.pdf", + metadata={"release": "vérsion-1.0-🚀", "note": "100% done"}, + ) + assert object_key is not None + + retrieved = session.get(object_key) + assert retrieved is not None + assert retrieved.payload.read() == b"test data" + assert retrieved.metadata.origin == "Ünknown-源" + assert retrieved.metadata.filename == "réport-📄.pdf" + assert retrieved.metadata.custom == { + "release": "vérsion-1.0-🚀", + "note": "100% done", + } + + head = session.head(object_key) + assert head is not None + assert head.filename == "réport-📄.pdf" + assert head.custom["release"] == "vérsion-1.0-🚀" + + def test_full_cycle_uncompressed(server_url: str) -> None: client = Client( server_url, diff --git a/clients/python/tests/test_utils.py b/clients/python/tests/test_utils.py index f3c44ae6..ccff8c95 100644 --- a/clients/python/tests/test_utils.py +++ b/clients/python/tests/test_utils.py @@ -4,6 +4,8 @@ import zstandard from objectstore_client.utils import ( _ZstdCompressionReaderWrapper, + decode_header_value, + encode_header_value, parse_accept_encoding, ) @@ -81,3 +83,41 @@ def test_parse_accept_encoding_empty() -> None: def test_parse_accept_encoding_q_spacing() -> None: assert parse_accept_encoding("gzip ; q=0") == [] assert parse_accept_encoding("gzip ; q=1") == ["gzip"] + + +def test_encode_header_value_escapes_unicode() -> None: + assert encode_header_value("réport-📄.pdf") == "r%C3%A9port-%F0%9F%93%84.pdf" + + +def test_encode_header_value_escapes_percent() -> None: + assert encode_header_value("100% done") == "100%25 done" + + +def test_encode_header_value_leaves_visible_ascii_alone() -> None: + # Must stay byte-identical so values that predate the encoding are unaffected. + value = "has\"quote path/to.txt!$&'()*+,;=:@?<>[]{}|^`~#" + assert encode_header_value(value) == value + + +def test_encode_header_value_escapes_control_characters() -> None: + assert encode_header_value("a\r\nb\tc\x7f") == "a%0D%0Ab%09c%7F" + + +@pytest.mark.parametrize( + "value", + [ + "réport-📄.pdf", + "100% done", + "50%.pdf", + "plain.txt", + "a\r\nb", + "", + ], +) +def test_header_value_roundtrip(value: str) -> None: + assert decode_header_value(encode_header_value(value)) == value + + +def test_decode_header_value_rejects_invalid_utf8() -> None: + with pytest.raises(UnicodeDecodeError): + decode_header_value("%FF.pdf") diff --git a/clients/rust/Cargo.toml b/clients/rust/Cargo.toml index 15438436..09faa312 100644 --- a/clients/rust/Cargo.toml +++ b/clients/rust/Cargo.toml @@ -19,7 +19,6 @@ infer = { workspace = true } jsonwebtoken = { workspace = true } multer = { workspace = true } objectstore-types = { workspace = true } -percent-encoding = { workspace = true } # Pinned below workspace version for MSRV compatibility reqwest = { version = "0.13.1", default-features = false, features = ["charset", "http2", "system-proxy", "json", "stream", "multipart"] } sentry-core = { version = ">=0.41", default-features = false, features = ["client"] } diff --git a/clients/rust/src/many.rs b/clients/rust/src/many.rs index 2ffc6bab..632d96c1 100644 --- a/clients/rust/src/many.rs +++ b/clients/rust/src/many.rs @@ -7,8 +7,8 @@ use std::task::{Context, Poll}; use futures_util::{Stream, StreamExt as _}; use multer::Field; +use objectstore_types::headers; use objectstore_types::metadata::{Compression, Metadata}; -use percent_encoding::NON_ALPHANUMERIC; use reqwest::header::{CONTENT_TYPE, HeaderMap, HeaderName, HeaderValue}; use reqwest::multipart::Part; @@ -180,12 +180,9 @@ fn operation_headers(operation: &str, key: Option<&str>) -> HeaderMap { HeaderValue::from_str(operation).expect("operation kind is always a valid header value"), ); if let Some(key) = key { - let encoded = - percent_encoding::percent_encode(key.as_bytes(), NON_ALPHANUMERIC).to_string(); headers.insert( HeaderName::from_static(HEADER_BATCH_OPERATION_KEY), - HeaderValue::try_from(encoded) - .expect("percent-encoded string is always a valid header value"), + headers::encode_header_value(key), ); } headers @@ -338,16 +335,7 @@ impl OperationResult { // Prioritize the server-provided key, fall back to the one from context. let key = headers .remove(HEADER_BATCH_OPERATION_KEY) - .and_then(|v| { - v.to_str() - .ok() - .and_then(|encoded| { - percent_encoding::percent_decode_str(encoded) - .decode_utf8() - .ok() - }) - .map(|s| s.into_owned()) - }) + .and_then(|v| headers::decode_header_value(&v).ok()) .or_else(|| ctx.key().map(str::to_owned)); let body = field.bytes().await?; diff --git a/clients/rust/tests/e2e.rs b/clients/rust/tests/e2e.rs index 826b1d3b..97be9519 100644 --- a/clients/rust/tests/e2e.rs +++ b/clients/rust/tests/e2e.rs @@ -149,6 +149,54 @@ async fn stores_under_given_key() { assert_eq!(stored_id, "test-key123!!"); } +#[tokio::test] +async fn roundtrips_unicode_metadata() { + let server = test_server().await; + + let client = Client::builder(server.url("/")) + .token(test_token_generator()) + .build() + .unwrap(); + let usecase = Usecase::new("usecase"); + let session = client.session(usecase.for_project(12345, 1337)).unwrap(); + + let filename = "réport-📄.pdf"; + let release = "vérsion-1.0-🚀"; + + let stored_id = session + .put("oh hai!") + .filename(filename) + .origin("Ünknown-源") + .append_metadata("release", release) + .append_metadata("note", "100% done") + .key("unicode-metadata") + .send() + .await + .unwrap() + .key; + + let response = session.get(&stored_id).send().await.unwrap().unwrap(); + let metadata = response.metadata.clone(); + assert_eq!(metadata.filename.as_deref(), Some(filename)); + assert_eq!(metadata.origin.as_deref(), Some("Ünknown-源")); + assert_eq!( + metadata.custom.get("release").map(String::as_str), + Some(release) + ); + assert_eq!( + metadata.custom.get("note").map(String::as_str), + Some("100% done"), + ); + assert_eq!(response.payload().await.unwrap(), "oh hai!"); + + let metadata = session.head(&stored_id).send().await.unwrap().unwrap(); + assert_eq!(metadata.filename.as_deref(), Some(filename)); + assert_eq!( + metadata.custom.get("release").map(String::as_str), + Some(release) + ); +} + #[tokio::test] async fn stores_structured_keys() { let server = test_server().await; diff --git a/objectstore-server/Cargo.toml b/objectstore-server/Cargo.toml index 9c68baa0..3e3d446b 100644 --- a/objectstore-server/Cargo.toml +++ b/objectstore-server/Cargo.toml @@ -36,7 +36,6 @@ objectstore-service = { workspace = true } objectstore-types = { workspace = true } papaya = { workspace = true } pin-project-lite = { workspace = true } -percent-encoding = { workspace = true } rand = { workspace = true } reqwest = { workspace = true, features = ["charset", "http2", "system-proxy", "native-tls-no-alpn"] } rustls = { workspace = true } diff --git a/objectstore-server/src/batch.rs b/objectstore-server/src/batch.rs index 4da38b46..5517d846 100644 --- a/objectstore-server/src/batch.rs +++ b/objectstore-server/src/batch.rs @@ -11,10 +11,11 @@ /// Clients use this to match each response part back to its corresponding request operation. pub const HEADER_BATCH_OPERATION_INDEX: &str = "x-sn-batch-operation-index"; -/// Base64-encoded object key for this batch operation, required on request parts. +/// Object key for this batch operation, required on request parts. /// -/// The key is base64-encoded to allow arbitrary byte sequences in object keys without conflicting -/// with HTTP header encoding restrictions. +/// The key is escaped with [`encode_header_value`](objectstore_types::headers::encode_header_value) +/// so that keys containing characters a header value cannot carry — non-ASCII in particular — still +/// survive the transport. pub const HEADER_BATCH_OPERATION_KEY: &str = "x-sn-batch-operation-key"; /// Operation kind for this batch part: `"get"`, `"insert"`, or `"delete"`. diff --git a/objectstore-server/src/endpoints/batch.rs b/objectstore-server/src/endpoints/batch.rs index b7aa57c5..20174881 100644 --- a/objectstore-server/src/endpoints/batch.rs +++ b/objectstore-server/src/endpoints/batch.rs @@ -11,7 +11,7 @@ use http::header::CONTENT_TYPE; use http::{HeaderMap, HeaderValue, StatusCode}; use objectstore_service::id::{ObjectContext, ObjectKey}; use objectstore_service::streaming::{OpResponse, Operation}; -use percent_encoding::NON_ALPHANUMERIC; +use objectstore_types::headers; use crate::auth::AuthAwareService; use crate::batch::{ @@ -262,12 +262,9 @@ fn insert_index_header(headers: &mut HeaderMap, idx: usize) { } fn insert_key_header(headers: &mut HeaderMap, key: &ObjectKey) { - let encoded = percent_encoding::percent_encode(key.as_bytes(), NON_ALPHANUMERIC).to_string(); headers.insert( HEADER_BATCH_OPERATION_KEY, - encoded - .parse() - .expect("percent-encoded string is always a valid header value"), + headers::encode_header_value(key), ); } diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index fa93d462..1aafcbac 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -1,3 +1,5 @@ +use std::fmt::Write as _; + use axum::body::Body; use axum::extract::State; use axum::http::{HeaderMap, StatusCode}; @@ -6,6 +8,7 @@ use axum::routing; use axum::{Json, Router}; use objectstore_service::error::Error as ServiceError; use objectstore_service::id::{ObjectContext, ObjectId}; +use objectstore_types::headers::ExtValue; use objectstore_types::metadata::Metadata; use objectstore_types::range::ContentRange; use serde::Serialize; @@ -146,25 +149,24 @@ fn insert_content_length(headers: &mut HeaderMap, metadata: &Metadata) { } fn insert_content_disposition(response: &mut Response, metadata: &Metadata) { - if let Some(val) = metadata - .filename - .as_deref() - .and_then(format_content_disposition) - { - response - .headers_mut() - .insert(http::header::CONTENT_DISPOSITION, val); + if let Some(filename) = metadata.filename.as_deref() { + response.headers_mut().insert( + http::header::CONTENT_DISPOSITION, + format_content_disposition(filename), + ); } } /// Formats a `Content-Disposition: attachment; filename="..."` header value. /// -/// The filename is sanitized (`/` and `\` become `-`, dots-only names become all -/// dashes) and then escaped for RFC 6266 quoted-string (`"` is backslash-escaped). +/// The filename is sanitized (`/` and `\` become `-`, dots-only names become all dashes, non-ASCII +/// and control characters become `_`) and then escaped for the RFC 6266 quoted-string (`"` is +/// backslash-escaped). /// -/// Returns `None` if the resulting value is not a valid HTTP header value (e.g. -/// the filename contains control characters). -fn format_content_disposition(filename: &str) -> Option { +/// A filename that is not pure ASCII cannot be represented in that quoted-string, so it +/// additionally gets an RFC 8187 `filename*` parameter carrying the full UTF-8 value. The +/// quoted-string then serves as the ASCII fallback for clients that ignore `filename*`. +fn format_content_disposition(filename: &str) -> http::HeaderValue { let all_dots = filename.chars().all(|c| c == '.'); let mut result = String::from("attachment; filename=\""); @@ -176,13 +178,21 @@ fn format_content_disposition(filename: &str) -> Option { result.push('\\'); '"' } + c if !c.is_ascii() || c.is_control() => '_', c => c, }; result.push(c); } result.push('"'); - http::HeaderValue::from_str(&result).ok() + if !filename.is_ascii() { + write!(result, "; filename*={}", ExtValue(filename)) + .expect("writing to a string cannot fail"); + } + + // INVARIANT: every character written above is visible ASCII — the quoted-string replaces + // non-ASCII and control characters with `_`, and the `ext-value` is percent-encoded. + http::HeaderValue::from_str(&result).expect("content disposition is a valid header value") } async fn object_put( diff --git a/objectstore-server/src/extractors/batch.rs b/objectstore-server/src/extractors/batch.rs index 5b75996c..d15fd125 100644 --- a/objectstore-server/src/extractors/batch.rs +++ b/objectstore-server/src/extractors/batch.rs @@ -12,6 +12,7 @@ use axum::extract::{ use bytes::BytesMut; use futures::{StreamExt, stream::BoxStream}; use objectstore_service::streaming::{Delete, Get, Head, Insert, Operation}; +use objectstore_types::headers; use objectstore_types::metadata::Metadata; use thiserror::Error; @@ -71,19 +72,11 @@ async fn try_operation_from_field(mut field: Field<'_>) -> Result TestServer { TestServer::with_config(Config { @@ -81,6 +82,99 @@ async fn filename_with_quotes_is_escaped() -> Result<()> { Ok(()) } +#[tokio::test] +async fn filename_with_unicode_roundtrips() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + + let filename = "réport-📄.pdf"; + + // Non-ASCII travels percent-encoded; this is the raw wire form our clients send. + let resp = client + .put(server.url("/v1/objects/test/org=1/cd-unicode")) + .header("x-sn-filename", "r%C3%A9port-%F0%9F%93%84.pdf") + .body("data") + .send() + .await?; + assert_eq!(resp.status(), reqwest::StatusCode::OK); + + let resp = client + .get(server.url("/v1/objects/test/org=1/cd-unicode")) + .send() + .await?; + assert_eq!(resp.status(), reqwest::StatusCode::OK); + + // Read the filename back through the metadata parser rather than off the raw + // header, so the assertion holds regardless of how the value is encoded on the wire. + let metadata = Metadata::from_headers(resp.headers(), "")?; + assert_eq!(metadata.filename.as_deref(), Some(filename)); + + // The wire value is escaped, so it survives as visible ASCII. + assert_eq!( + resp.headers().get("x-sn-filename").unwrap(), + "r%C3%A9port-%F0%9F%93%84.pdf", + ); + + // Non-ASCII needs the RFC 8187 form, with the quoted-string as the ASCII fallback. + assert_eq!( + resp.headers().get("content-disposition").unwrap(), + "attachment; filename=\"r_port-_.pdf\"; \ + filename*=UTF-8''r%C3%A9port-%F0%9F%93%84.pdf", + ); + + Ok(()) +} + +#[tokio::test] +async fn custom_metadata_with_unicode_roundtrips() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + + let release = "vérsion-1.0-🚀"; + + // Non-ASCII travels percent-encoded, as does a literal percent sign. + let resp = client + .put(server.url("/v1/objects/test/org=1/meta-unicode")) + .header("x-snme-release", "v%C3%A9rsion-1.0-%F0%9F%9A%80") + .header("x-snme-note", "100%25 done") + .body("data") + .send() + .await?; + assert_eq!(resp.status(), reqwest::StatusCode::OK); + + for resp in [ + client + .get(server.url("/v1/objects/test/org=1/meta-unicode")) + .send() + .await?, + client + .head(server.url("/v1/objects/test/org=1/meta-unicode")) + .send() + .await?, + ] { + assert_eq!(resp.status(), reqwest::StatusCode::OK); + + let metadata = Metadata::from_headers(resp.headers(), "")?; + assert_eq!( + metadata.custom.get("release").map(String::as_str), + Some(release) + ); + assert_eq!( + metadata.custom.get("note").map(String::as_str), + Some("100% done"), + ); + + // Escaped on the wire, including the literal percent sign. + assert_eq!( + resp.headers().get("x-snme-release").unwrap(), + "v%C3%A9rsion-1.0-%F0%9F%9A%80", + ); + assert_eq!(resp.headers().get("x-snme-note").unwrap(), "100%25 done"); + } + + Ok(()) +} + #[tokio::test] async fn filename_with_slashes_is_sanitized() -> Result<()> { let server = test_server().await; diff --git a/objectstore-service/src/backend/gcs.rs b/objectstore-service/src/backend/gcs.rs index 3bc48b41..82a0b454 100644 --- a/objectstore-service/src/backend/gcs.rs +++ b/objectstore-service/src/backend/gcs.rs @@ -9,6 +9,7 @@ use std::{fmt, io}; use anyhow::Context; use futures_util::{StreamExt, TryStreamExt}; use gcp_auth::TokenProvider; +use objectstore_types::headers; use objectstore_types::metadata::{ExpirationPolicy, Metadata}; use objectstore_types::range::{ByteRange, ContentRange}; use reqwest::header::HeaderName; @@ -157,22 +158,27 @@ impl GcsObject { ); } + // Free-form strings are stored escaped, even though this JSON representation could carry + // them verbatim. See `insert_gcs_meta_header` for why, and why both writers must agree. if let Some(origin) = &metadata.origin { - gcs_object - .metadata - .insert(GcsMetaKey::Origin, origin.clone()); + gcs_object.metadata.insert( + GcsMetaKey::Origin, + headers::encode_header_str(origin).into(), + ); } if let Some(filename) = &metadata.filename { - gcs_object - .metadata - .insert(GcsMetaKey::Filename, filename.clone()); + gcs_object.metadata.insert( + GcsMetaKey::Filename, + headers::encode_header_str(filename).into(), + ); } for (key, value) in &metadata.custom { - gcs_object - .metadata - .insert(GcsMetaKey::Custom(key.clone()), value.clone()); + gcs_object.metadata.insert( + GcsMetaKey::Custom(key.clone()), + headers::encode_header_str(value).into(), + ); } gcs_object @@ -190,8 +196,16 @@ impl GcsObject { .transpose()? .unwrap_or_default(); - let origin = self.metadata.remove(&GcsMetaKey::Origin); - let filename = self.metadata.remove(&GcsMetaKey::Filename); + let origin = self + .metadata + .remove(&GcsMetaKey::Origin) + .map(|value| decode_gcs_meta_value(&value)) + .transpose()?; + let filename = self + .metadata + .remove(&GcsMetaKey::Filename) + .map(|value| decode_gcs_meta_value(&value)) + .transpose()?; let content_type = self.content_type; let compression = self.content_encoding.map(|s| s.parse()).transpose()?; @@ -209,7 +223,7 @@ impl GcsObject { let mut custom = BTreeMap::new(); for (key, value) in self.metadata { if let GcsMetaKey::Custom(custom_key) = key { - custom.insert(custom_key, value); + custom.insert(custom_key, decode_gcs_meta_value(&value)?); } else { return Err(Error::Generic { context: format!( @@ -352,6 +366,24 @@ fn metadata_to_gcs_headers(metadata: &Metadata) -> Result { Ok(headers) } +/// Decodes a stored GCS metadata value into its logical string. +fn decode_gcs_meta_value(value: &str) -> Result { + headers::decode_header_str(value).map_err(|cause| Error::Generic { + context: "GCS: invalid percent-encoded UTF-8 in object metadata".to_owned(), + cause: Some(Box::new(cause)), + }) +} + +/// Inserts a single `x-goog-meta-*` header, escaping the value for transport. +/// +/// Google: "you should generally avoid non-ascii characters, because they are not permitted in +/// HTTP headers, which the XML API uses" ([docs]). Real GCS does preserve raw UTF-8 here, but that +/// is undocumented, and it still drops leading whitespace and turns invalid UTF-8 into `U+FFFD`. +/// +/// [`GcsObject::from_metadata`] escapes the same values on the JSON path: reads always come back +/// through the JSON API and cannot tell which writer produced an object, so both must agree. +/// +/// [docs]: https://docs.cloud.google.com/storage/docs/metadata fn insert_gcs_meta_header( headers: &mut header::HeaderMap, key: &GcsMetaKey, @@ -363,10 +395,7 @@ fn insert_gcs_meta_header( context: format!("GCS: invalid header name: {header_name}"), cause: Some(Box::new(e)), })?, - value.parse().map_err(|e| Error::Generic { - context: format!("GCS: invalid header value for {header_name}"), - cause: Some(Box::new(e)), - })?, + headers::encode_header_value(value), ); Ok(()) } @@ -1189,6 +1218,50 @@ mod tests { Ok(()) } + /// Metadata with a non-ASCII filename and custom metadata value. + fn unicode_metadata() -> Metadata { + Metadata { + filename: Some("réport-📄.pdf".into()), + custom: BTreeMap::from_iter([("release".into(), "vérsion-1.0-🚀".into())]), + ..Default::default() + } + } + + /// Both GCS write paths must agree on how a logical string is stored, because every read goes + /// through the JSON API: `put_object` writes metadata as JSON, `initiate_multipart` writes it + /// as `x-goog-meta-*` request headers. + #[tokio::test] + async fn test_unicode_metadata_roundtrip_json_upload() -> Result<()> { + let backend = create_test_backend().await?; + let id = make_id(); + let metadata = unicode_metadata(); + + backend + .put_object(&id, &metadata, stream::single("hello, world")) + .await?; + + let meta = backend.get_metadata(&id).await?.unwrap(); + assert_eq!(meta.filename, metadata.filename); + assert_eq!(meta.custom, metadata.custom); + + Ok(()) + } + + #[tokio::test] + async fn test_unicode_metadata_roundtrip_multipart_upload() -> Result<()> { + let backend = create_test_backend().await?; + let id = make_id(); + let metadata = unicode_metadata(); + + multipart_put(&backend, &id, &metadata, "hello, world").await?; + + let meta = backend.get_metadata(&id).await?.unwrap(); + assert_eq!(meta.filename, metadata.filename); + assert_eq!(meta.custom, metadata.custom); + + Ok(()) + } + #[test] fn from_metadata_uses_provided_time_expires() { let expires = SystemTime::now() + Duration::from_hours(1); diff --git a/objectstore-service/src/backend/s3_compatible.rs b/objectstore-service/src/backend/s3_compatible.rs index 8644a0c7..f34beec0 100644 --- a/objectstore-service/src/backend/s3_compatible.rs +++ b/objectstore-service/src/backend/s3_compatible.rs @@ -392,6 +392,7 @@ impl Backend for S3CompatibleBackend { #[cfg(test)] mod tests { + use std::collections::BTreeMap; use std::time::Duration; use anyhow::Result; @@ -452,6 +453,30 @@ mod tests { assert_eq!(custom_time, expected); } + #[test] + fn metadata_to_gcs_headers_escapes_unicode() { + let metadata = Metadata { + filename: Some("réport-📄.pdf".into()), + custom: BTreeMap::from_iter([("release".into(), "vérsion-1.0-🚀".into())]), + ..Default::default() + }; + + let headers = metadata_to_gcs_headers(&metadata, GCS_CUSTOM_PREFIX).unwrap(); + assert_eq!( + headers.get("x-goog-meta-x-sn-filename").unwrap(), + "r%C3%A9port-%F0%9F%93%84.pdf", + ); + assert_eq!( + headers.get("x-goog-meta-x-snme-release").unwrap(), + "v%C3%A9rsion-1.0-%F0%9F%9A%80", + ); + + // The prefixed headers this backend writes are the ones it reads back. + let roundtripped = Metadata::from_headers(&headers, GCS_CUSTOM_PREFIX).unwrap(); + assert_eq!(roundtripped.filename, metadata.filename); + assert_eq!(roundtripped.custom, metadata.custom); + } + #[tokio::test] async fn test_get_metadata_nonexistent() -> Result<()> { let backend = create_test_backend(); diff --git a/objectstore-types/Cargo.toml b/objectstore-types/Cargo.toml index af2bb749..07596f94 100644 --- a/objectstore-types/Cargo.toml +++ b/objectstore-types/Cargo.toml @@ -16,6 +16,7 @@ http = { workspace = true } humantime = { workspace = true } humantime-serde = { workspace = true } mediatype = { workspace = true } +percent-encoding = { workspace = true } serde = { workspace = true } thiserror = { workspace = true } diff --git a/objectstore-types/src/headers.rs b/objectstore-types/src/headers.rs new file mode 100644 index 00000000..afd00bd3 --- /dev/null +++ b/objectstore-types/src/headers.rs @@ -0,0 +1,270 @@ +//! Escaping for free-form values carried in HTTP headers. +//! +//! HTTP header values carry only visible ASCII, so any value that may contain arbitrary Unicode — +//! an object key, a filename, a custom metadata value — has to be escaped to survive the +//! transport. This module owns that escaping for the whole workspace: it is the only place that +//! depends on [`percent_encoding`], so callers escape by name rather than by assembling a +//! character set of their own. +//! +//! Encoding is a property of the *transport*, never of the value: everything in memory holds the +//! logical string, and [`encode_header_value`] is applied only when writing a header. + +use std::borrow::Cow; +use std::fmt; +use std::str::Utf8Error; + +use http::HeaderValue; +use http::header::ToStrError; +use percent_encoding::{ + AsciiSet, CONTROLS, NON_ALPHANUMERIC, percent_decode_str, utf8_percent_encode, +}; + +/// The characters escaped when a free-form value is written into a header value. +/// +/// Non-ASCII bytes are escaped by the encoder itself; this set adds the C0 controls and `DEL`, +/// which a header value cannot carry, plus `%` so that a literal percent sign can never be +/// confused with an escape sequence when decoding. +/// +/// Every other visible ASCII character is left alone, which keeps values that are already plain +/// ASCII byte-identical to their logical form on the wire. +const HEADER_ESCAPE: &AsciiSet = &CONTROLS.add(b'%'); + +/// The characters escaped in an [RFC 8187] `ext-value`. +/// +/// This is the complement of the spec's `attr-char` production: alphanumerics plus a handful of +/// symbols survive, everything else — including the non-ASCII bytes this exists for — is escaped. +/// +/// [RFC 8187]: https://www.rfc-editor.org/rfc/rfc8187 +const EXT_VALUE_ESCAPE: &AsciiSet = &NON_ALPHANUMERIC + .remove(b'!') + .remove(b'#') + .remove(b'$') + .remove(b'&') + .remove(b'+') + .remove(b'-') + .remove(b'.') + .remove(b'^') + .remove(b'_') + .remove(b'`') + .remove(b'|') + .remove(b'~'); + +/// Escapes a logical string into a header value. +/// +/// This is the inverse of [`decode_header_value`]. Escaping cannot fail — the result is always +/// visible ASCII — so this returns the [`HeaderValue`] directly rather than a string a caller has +/// to parse and handle the impossible error of. +/// +/// # Examples +/// +/// ``` +/// use objectstore_types::headers::encode_header_value; +/// +/// assert_eq!(encode_header_value("report.pdf"), "report.pdf"); +/// assert_eq!(encode_header_value("réport.pdf"), "r%C3%A9port.pdf"); +/// assert_eq!(encode_header_value("100% done"), "100%25 done"); +/// ``` +pub fn encode_header_value(value: &str) -> HeaderValue { + // INVARIANT: `HEADER_ESCAPE` escapes every byte a header value cannot carry — the controls, + // `DEL`, and everything non-ASCII — so what is left is always visible ASCII. + HeaderValue::from_str(&encode_header_str(value)) + .expect("escaped value is always a valid header value") +} + +/// Escapes a logical string for a transport that is not an HTTP header. +/// +/// Use this where the escaped form is needed as a string rather than a header — notably GCS object +/// metadata, which is written as JSON but has to match what the `x-goog-meta-*` headers carry. +/// Where the target *is* a header, prefer [`encode_header_value`]. +/// +/// Values that are already plain ASCII are returned borrowed and unchanged. +/// +/// # Examples +/// +/// ``` +/// use objectstore_types::headers::encode_header_str; +/// +/// assert_eq!(encode_header_str("report.pdf"), "report.pdf"); +/// assert_eq!(encode_header_str("réport.pdf"), "r%C3%A9port.pdf"); +/// ``` +pub fn encode_header_str(value: &str) -> Cow<'_, str> { + utf8_percent_encode(value, HEADER_ESCAPE).into() +} + +/// The reasons a header value can fail to decode into a logical string. +#[derive(Debug, thiserror::Error)] +pub enum DecodeError { + /// The raw header value contained bytes outside visible ASCII. + /// + /// A conforming writer escapes those, so this means the value was not written by one. + #[error("header value is not visible ASCII")] + NotAscii(#[from] ToStrError), + + /// The escape sequences did not decode to valid UTF-8. + #[error("header value is not valid percent-encoded UTF-8")] + InvalidUtf8(#[from] Utf8Error), +} + +/// Decodes a header value into the logical string it carries. +/// +/// This is the inverse of [`encode_header_value`], and the counterpart most callers want: it +/// covers both ways a raw header can fail to be a logical string, so there is no separate +/// [`HeaderValue::to_str`] step to handle. Decoding does not depend on how aggressively the writer +/// escaped, so values written by older peers — or with a different escape set — read back +/// unchanged. +/// +/// Callers are expected to wrap the error in one of their own that names the header at fault. +/// +/// # Examples +/// +/// ``` +/// use http::HeaderValue; +/// use objectstore_types::headers::decode_header_value; +/// +/// let value = HeaderValue::from_static("r%C3%A9port.pdf"); +/// assert_eq!(decode_header_value(&value)?, "réport.pdf"); +/// # Ok::<(), objectstore_types::headers::DecodeError>(()) +/// ``` +pub fn decode_header_value(value: &HeaderValue) -> Result { + Ok(decode_header_str(value.to_str()?)?) +} + +/// Decodes an escaped string back into its logical form. +/// +/// Use this for escaped values that do not arrive in an actual header — notably GCS object +/// metadata, which is read back as JSON. Where the value *is* a header, prefer +/// [`decode_header_value`], which also rejects raw bytes outside visible ASCII. +/// +/// # Examples +/// +/// ``` +/// use objectstore_types::headers::decode_header_str; +/// +/// assert_eq!(decode_header_str("r%C3%A9port.pdf")?, "réport.pdf"); +/// assert_eq!(decode_header_str("100%25 done")?, "100% done"); +/// assert!(decode_header_str("%FF.pdf").is_err()); +/// # Ok::<(), std::str::Utf8Error>(()) +/// ``` +pub fn decode_header_str(value: &str) -> Result { + Ok(percent_decode_str(value).decode_utf8()?.into_owned()) +} + +/// A logical string wrapped for use as an [RFC 8187] `ext-value`, in header parameters like +/// `Content-Disposition`'s `filename*`. +/// +/// The string is escaped lazily as this is displayed, so a caller can write it straight into a +/// header it is already building instead of allocating an intermediate string. The output includes +/// the charset prefix, so it goes directly after the `=` of a header parameter. +/// +/// Unlike [`encode_header_value`], this escapes everything outside a narrow `attr-char` set, +/// because an `ext-value` sits inside a header parameter rather than spanning a whole value. +/// +/// [RFC 8187]: https://www.rfc-editor.org/rfc/rfc8187 +/// +/// # Examples +/// +/// ``` +/// use objectstore_types::headers::ExtValue; +/// +/// assert_eq!(ExtValue("réport.pdf").to_string(), "UTF-8''r%C3%A9port.pdf"); +/// ``` +#[derive(Debug)] +pub struct ExtValue<'a>(pub &'a str); + +impl fmt::Display for ExtValue<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("UTF-8''")?; + fmt::Display::fmt(&utf8_percent_encode(self.0, EXT_VALUE_ESCAPE), f) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn encodes_non_ascii() { + assert_eq!( + encode_header_value("réport-📄.pdf"), + "r%C3%A9port-%F0%9F%93%84.pdf", + ); + } + + #[test] + fn encodes_percent() { + assert_eq!(encode_header_value("100% done"), "100%25 done"); + } + + #[test] + fn encodes_control_characters() { + assert_eq!(encode_header_value("a\r\nb\tc\x7f"), "a%0D%0Ab%09c%7F"); + } + + #[test] + fn encodes_to_a_borrowed_str_when_unchanged() { + std::assert_matches!(encode_header_str("report.pdf"), Cow::Borrowed(_)); + std::assert_matches!(encode_header_str("réport.pdf"), Cow::Owned(_)); + } + + #[test] + fn leaves_visible_ascii_alone() { + // Must stay byte-identical so values written before this encoding existed are unaffected. + let value = r#"has"quote path/to.txt!$&'()*+,;=:@?<>[]{}|^`~#"#; + assert_eq!(encode_header_value(value), value); + } + + #[test] + fn roundtrips() { + for value in [ + "réport-📄.pdf", + "100% done", + "50%.pdf", + "plain.txt", + "a\r\nb", + "", + ] { + let encoded = encode_header_str(value); + assert!(encoded.is_ascii(), "{encoded} is not ascii"); + assert_eq!(decode_header_str(&encoded).unwrap(), value); + + assert_eq!( + decode_header_value(&encode_header_value(value)).unwrap(), + value, + ); + } + } + + #[test] + fn decodes_aggressively_escaped_values() { + // Decoding is independent of the writer's escape set, which is what makes it safe to + // change how much we escape without breaking peers. + assert_eq!(decode_header_str("%6B%65%79%2D%31").unwrap(), "key-1"); + } + + #[test] + fn decode_rejects_invalid_utf8() { + let header = HeaderValue::from_static("%FF.pdf"); + std::assert_matches!( + decode_header_value(&header), + Err(DecodeError::InvalidUtf8(_)), + ); + assert!(decode_header_str("%FF.pdf").is_err()); + } + + #[test] + fn decode_rejects_raw_non_ascii() { + // A conforming writer escapes these, so an unescaped byte means the value is malformed + // rather than merely unencoded. + let header = HeaderValue::from_bytes("réport.pdf".as_bytes()).unwrap(); + std::assert_matches!(decode_header_value(&header), Err(DecodeError::NotAscii(_)),); + } + + #[test] + fn ext_value_escapes_reserved_characters() { + assert_eq!( + ExtValue("réport 📄.pdf").to_string(), + "UTF-8''r%C3%A9port%20%F0%9F%93%84.pdf", + ); + assert_eq!(ExtValue("a\"b;c").to_string(), "UTF-8''a%22b%3Bc"); + } +} diff --git a/objectstore-types/src/lib.rs b/objectstore-types/src/lib.rs index b68ee553..910f293e 100644 --- a/objectstore-types/src/lib.rs +++ b/objectstore-types/src/lib.rs @@ -6,6 +6,7 @@ #![warn(missing_debug_implementations)] pub mod auth; +pub mod headers; pub mod metadata; pub mod multipart; pub mod presign; diff --git a/objectstore-types/src/metadata.rs b/objectstore-types/src/metadata.rs index b25436ae..1049f982 100644 --- a/objectstore-types/src/metadata.rs +++ b/objectstore-types/src/metadata.rs @@ -29,6 +29,21 @@ //! prefix on top, so `x-sn-expiration` becomes `x-goog-meta-x-sn-expiration`. //! The [`Metadata::from_headers`] and [`Metadata::to_headers`] methods accept //! a `prefix` parameter for this purpose. +//! +//! # Escaping free-form values +//! +//! [`Metadata`] always holds logical strings: [`filename`](Metadata::filename), +//! [`origin`](Metadata::origin), and [`custom`](Metadata::custom) values may +//! contain arbitrary Unicode. +//! +//! Over the wire, metadata travels in HTTP headers, which have no charset. In +//! practice anything outside visible ASCII is either rejected outright or +//! silently reinterpreted. +//! +//! The fields are therefore percent-encoded in headers, via +//! [`headers::encode_header_value`] and [`headers::decode_header_value`]. +//! Encoding is a property of the *transport*, never of the stored value: +//! anything reading [`Metadata`] sees the logical string. use std::borrow::Cow; use std::collections::BTreeMap; @@ -41,6 +56,8 @@ use http::header::{self, HeaderMap, HeaderName}; use humantime::{format_duration, format_rfc3339_micros, parse_duration, parse_rfc3339}; use serde::{Deserialize, Serialize}; +use crate::headers; + /// The custom HTTP header that contains the serialized [`ExpirationPolicy`]. pub const HEADER_EXPIRATION: &str = "x-sn-expiration"; /// The custom HTTP header that contains the object creation time. @@ -88,6 +105,9 @@ pub enum Error { /// The object size is not a valid byte count. #[error("invalid object size")] Size(#[from] ParseIntError), + /// A free-form header value did not decode into a logical string. + #[error("invalid metadata header value")] + Encoding(#[from] crate::headers::DecodeError), /// An internal consistency invariant on the metadata was violated. #[error("invariant violation: {0}")] Invariant(&'static str), @@ -307,7 +327,10 @@ pub struct Metadata { /// /// When present, the server includes a `Content-Disposition: attachment; filename=""` /// header in GET responses, prompting browsers and download tools to save the file - /// under this name. + /// under this name. Non-ASCII filenames additionally get an RFC 8187 `filename*` parameter. + /// + /// This is a logical string and may contain arbitrary Unicode; it is escaped only on the + /// wire (see [the module docs](self#escaping-free-form-values)). #[serde(skip_serializing_if = "Option::is_none")] pub filename: Option, @@ -423,10 +446,10 @@ impl Metadata { metadata.time_expires = Some(time); } HEADER_ORIGIN => { - metadata.origin = Some(value.to_str()?.to_owned()); + metadata.origin = Some(headers::decode_header_value(value)?); } HEADER_FILENAME => { - metadata.filename = Some(value.to_str()?.to_owned()); + metadata.filename = Some(headers::decode_header_value(value)?); } HEADER_SIZE if !skip_read_only => { let size = value.to_str()?; @@ -435,8 +458,8 @@ impl Metadata { _ => { // customer-provided metadata if let Some(name) = name.strip_prefix(HEADER_META_PREFIX) { - let value = value.to_str()?; - metadata.custom.insert(name.into(), value.into()); + let value = headers::decode_header_value(value)?; + metadata.custom.insert(name.into(), value); } } } @@ -489,11 +512,11 @@ impl Metadata { } if let Some(origin) = origin { let name = HeaderName::try_from(format!("{prefix}{HEADER_ORIGIN}"))?; - headers.append(name, origin.parse()?); + headers.append(name, headers::encode_header_value(origin)); } if let Some(filename) = filename { let name = HeaderName::try_from(format!("{prefix}{HEADER_FILENAME}"))?; - headers.append(name, filename.parse()?); + headers.append(name, headers::encode_header_value(filename)); } if let Some(size) = size { let name = HeaderName::try_from(format!("{prefix}{HEADER_SIZE}"))?; @@ -503,7 +526,7 @@ impl Metadata { // customer-provided metadata for (key, value) in custom { let name = HeaderName::try_from(format!("{prefix}{HEADER_META_PREFIX}{key}"))?; - headers.append(name, value.parse()?); + headers.append(name, headers::encode_header_value(value)); } Ok(headers) @@ -633,6 +656,39 @@ mod tests { assert_eq!(roundtripped.filename, metadata.filename); } + /// Every free-form field is escaped on the way out and decoded on the way back in. + /// + /// The escaping itself is covered in [`crate::headers`]; this only pins down that each of the + /// three fields that needs it actually goes through it, in both directions. + #[test] + fn free_form_values_are_escaped_on_the_wire() { + let metadata = Metadata { + origin: Some("Ünknown-源".into()), + filename: Some("réport-📄.pdf".into()), + custom: BTreeMap::from([("release".to_owned(), "100% vérsion-🚀".to_owned())]), + ..Default::default() + }; + + let headers = metadata.to_headers("").unwrap(); + assert_eq!( + headers.get(HEADER_ORIGIN).unwrap(), + "%C3%9Cnknown-%E6%BA%90" + ); + assert_eq!( + headers.get(HEADER_FILENAME).unwrap(), + "r%C3%A9port-%F0%9F%93%84.pdf", + ); + assert_eq!( + headers.get(format!("{HEADER_META_PREFIX}release")).unwrap(), + "100%25 v%C3%A9rsion-%F0%9F%9A%80", + ); + + let roundtripped = Metadata::from_headers(&headers, "").unwrap(); + assert_eq!(roundtripped.origin, metadata.origin); + assert_eq!(roundtripped.filename, metadata.filename); + assert_eq!(roundtripped.custom, metadata.custom); + } + #[test] fn from_headers_content_type_and_encoding() { let mut headers = HeaderMap::new();