From 0f0b54ba17958aa014b53ce16a083d4b47e7ea85 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 20 Oct 2026 19:12:19 +0200 Subject: [PATCH 1/6] feat: Support download response options --- clients/CHANGELOG.md | 6 + clients/python/README.md | 34 ++++ .../python/src/objectstore_client/client.py | 66 ++++++- clients/python/tests/test_e2e.py | 77 ++++++++ clients/python/tests/test_presign.py | 104 +++++++++- objectstore-server/docs/architecture.md | 4 +- objectstore-server/src/endpoints/mod.rs | 5 + objectstore-server/src/endpoints/objects.rs | 62 +----- objectstore-server/src/extractors/mod.rs | 1 + .../src/extractors/response_options.rs | 73 ++++++++ objectstore-server/src/lib.rs | 1 + objectstore-server/src/response_options.rs | 177 ++++++++++++++++++ objectstore-server/tests/presigned.rs | 58 ++++++ objectstore-server/tests/response_options.rs | 174 +++++++++++++++++ objectstore-types/src/metadata.rs | 7 +- objectstore-types/src/presign.rs | 18 ++ 16 files changed, 798 insertions(+), 69 deletions(-) create mode 100644 objectstore-server/src/extractors/response_options.rs create mode 100644 objectstore-server/src/response_options.rs create mode 100644 objectstore-server/tests/response_options.rs diff --git a/clients/CHANGELOG.md b/clients/CHANGELOG.md index 8f0c80094..8ad066c38 100644 --- a/clients/CHANGELOG.md +++ b/clients/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### New Features ✨ + +- (py-client) Add disposition and cache-control options to object URLs and pre-signed URLs (FS-570). + ## 0.2.10 ### New Features ✨ diff --git a/clients/python/README.md b/clients/python/README.md index 924eb82a9..75f78066b 100644 --- a/clients/python/README.md +++ b/clients/python/README.md @@ -254,6 +254,40 @@ with urllib.request.urlopen(url) as resp: content = resp.read() ``` +### Download response options + +Both `Session.object_url` and `Session.presigned_object_url` accept keyword-only +`response_content_disposition` and `response_cache_control` options: + +```python +url = session.presigned_object_url( + "GET", + "my-key", + response_content_disposition="inline", + response_cache_control="private, max-age=300", +) +``` + +Disposition accepts `inline` or `attachment`, defaulting to `attachment` when omitted. +The stored filename is retained. Cache control is passed through as an HTTP header value; +it is not interpreted or limited by authorization expiry. A cached response can remain +usable after the URL expires. Omitting cache control preserves the server's existing behavior. + +These options apply only to successful GET/HEAD responses, including ranged downloads. +404 and other error responses retain their existing headers and caching behavior. +Successful downloads include `X-Content-Type-Options: nosniff`; inline responses also include +`Content-Security-Policy: sandbox` without relaxation tokens. Inline MIME types are not +restricted by an allowlist. Sandboxing is a browser safeguard, not content sanitization, +and does not prevent all external resource loads. + +The object's declared content type and compression encoding cannot be overridden. +Content-language and expires overrides are not supported. + +Pre-signed URLs bind both options into the signature. JWT-bearing `object_url` URLs +only authenticate the JWT's claims, so recipients can edit the response options. +Existing URLs and signatures are unchanged when the options are omitted. +Deploy server support before callers rely on these options; older servers ignore them. + ## Configuration In production, store the `Client` and `Usecase` at module level and reuse them. diff --git a/clients/python/src/objectstore_client/client.py b/clients/python/src/objectstore_client/client.py index d4f82b3f3..f9c67d26a 100644 --- a/clients/python/src/objectstore_client/client.py +++ b/clients/python/src/objectstore_client/client.py @@ -10,7 +10,7 @@ from io import BytesIO from os import PathLike from typing import IO, Any, Literal, NamedTuple, cast -from urllib.parse import urlparse +from urllib.parse import quote, urlencode, urlparse import sentry_sdk import urllib3 @@ -52,6 +52,23 @@ USER_AGENT = f"objectstore-client/{version('objectstore-client')}" +def _response_options_query( + disposition: Literal["inline", "attachment"] | None, + cache_control: str | None, +) -> str: + """Encode individual response options without changing legacy URLs.""" + pairs: list[tuple[str, str]] = [] + if disposition is not None: + if disposition not in ("inline", "attachment"): + raise ValueError( + f"unsupported response content disposition {disposition!r}" + ) + pairs.append(("response-content-disposition", disposition)) + if cache_control is not None: + pairs.append(("response-cache-control", cache_control)) + return urlencode(pairs, quote_via=quote) + + class GetResponse(NamedTuple): metadata: Metadata payload: IO[bytes] @@ -94,7 +111,7 @@ def __init__( "usecase name must contain only ASCII letters, digits, '_' or '-'" ) self._name = name - + if resumable_threshold_bytes != "none" and ( type(resumable_threshold_bytes) is not int or resumable_threshold_bytes < 0 ): @@ -709,7 +726,14 @@ def get( span.set_attribute("objectstore.decompressed", decompressed) return GetResponse(metadata, stream) - def object_url(self, key: str, token_validity: timedelta | None = None) -> str: + def object_url( + self, + key: str, + token_validity: timedelta | None = None, + *, + response_content_disposition: Literal["inline", "attachment"] | None = None, + response_cache_control: str | None = None, + ) -> str: """ Generates a GET url to the object with the given `key`. @@ -721,10 +745,23 @@ def object_url(self, key: str, token_validity: timedelta | None = None) -> str: information is embedded in the returned URL's query string, valid for the given duration. + Response options apply only to successful GET/HEAD responses. Disposition + defaults to attachment; inline responses include browser sandboxing without + restricting MIME types. Cache control is passed through independently of + authorization expiry, so cached responses can outlive the URL's validity. + + These options are not bound by the JWT and can be changed by recipients. + Use :meth:`presigned_object_url` to bind the options to a signature. + Raises ``ValueError`` if ``token_validity`` is provided but no - ``SecretKey`` is configured on this session. + ``SecretKey`` is configured on this session, or if disposition is invalid. """ url = self._make_url(key, full=True) + query = _response_options_query( + response_content_disposition, response_cache_control + ) + if query: + url = f"{url}?{query}" if token_validity is None: return url @@ -734,13 +771,17 @@ def object_url(self, key: str, token_validity: timedelta | None = None) -> str: ) # A JWT's compact serialization is base64url, whose alphabet is URL-safe, # so it can go in the query string as-is with no further encoding. - return f"{url}?{PARAM_AUTH}={token}" + separator = "&" if query else "?" + return f"{url}{separator}{PARAM_AUTH}={token}" def presigned_object_url( self, method: Literal["GET", "HEAD"], key: str, duration: timedelta = timedelta(hours=1), + *, + response_content_disposition: Literal["inline", "attachment"] | None = None, + response_cache_control: str | None = None, ) -> str: """ Generates a pre-signed URL authorizing a single ``method`` request on the @@ -752,12 +793,19 @@ def presigned_object_url( Raises ``ValueError`` if no ``SecretKey`` is configured on this session's client, if ``method`` is not supported, or if ``duration`` - is above the one-week maximum. + is above the one-week maximum, or if disposition is invalid. The returned URL carries a signature that allows the recipient to perform a request without an auth token. It can be handed to any HTTP client; the URL is already percent-encoded, and must be transmitted verbatim. + Response options apply only to successful GET/HEAD responses. Disposition + defaults to attachment; inline responses include browser sandboxing without + restricting MIME types. Cache control is passed through independently of + authorization expiry, so cached responses can outlive the URL's validity. + + Both response options are included in the signature when supplied. + Note that `HEAD` and `GET` are considered equivalent from the point of view of signatures, meaning that a signature for `GET` can also be used for `HEAD`, and viceversa. @@ -795,6 +843,12 @@ def presigned_object_url( f"&{presign.PARAM_DURATION}={duration_secs}" ) + response_query = _response_options_query( + response_content_disposition, response_cache_control + ) + if response_query: + encoded_query = f"{encoded_query}&{response_query}" + canonical = presign.build_canonical_form( method, encoded_path, encoded_query ) diff --git a/clients/python/tests/test_e2e.py b/clients/python/tests/test_e2e.py index 1f08b3737..2aff24961 100644 --- a/clients/python/tests/test_e2e.py +++ b/clients/python/tests/test_e2e.py @@ -1094,6 +1094,83 @@ def test_presigned_get_encoding_corner_cases(server_url: str, key: str) -> None: assert body == payload +@pytest.mark.parametrize("method", ["GET", "HEAD"]) +def test_presigned_response_options(server_url: str, method: str) -> None: + session = _presign_session(server_url) + key = "inline-café/looks%20encoded&+" + session.put(b"example", key=key, content_type="text/html") + cache_control = 'private, max-age=3600, x="a&b=+%?#"' + url = session.presigned_object_url( + "GET", + key, + response_content_disposition="inline", + response_cache_control=cache_control, + ) + with urllib.request.urlopen(urllib.request.Request(url, method=method)) as response: + assert response.status == 200 + assert response.headers["content-disposition"] == "inline" + assert response.headers["cache-control"] == cache_control + assert response.headers["content-type"] == "text/html" + assert response.headers["content-security-policy"] == "sandbox" + assert response.headers["x-content-type-options"] == "nosniff" + assert response.read() == (b"" if method == "HEAD" else b"example") + + changed_case = url.replace( + "response-content-disposition", "RESPONSE-CONTENT-DISPOSITION" + ) + status, _ = _fetch(changed_case, method=method) + assert status == 200 + for tampered in [ + url.replace("=inline", "=attachment"), + url.replace("max-age%3D3600", "max-age%3D60"), + url.replace("response-content-disposition=inline&", ""), + ]: + status, _ = _fetch(tampered, method=method) + assert status == 401 + + +def test_jwt_response_options_remain_editable(server_url: str) -> None: + session = _presign_session(server_url) + session.put(b"example", key="jwt-options") + url = session.object_url( + "jwt-options", + token_validity=timedelta(minutes=5), + response_content_disposition="inline", + response_cache_control="no-store", + ) + for target, expected in [ + (url, "inline"), + (url.replace("=inline", "=attachment"), "attachment"), + ]: + with urllib.request.urlopen(target) as response: + assert response.status == 200 + assert response.headers["content-disposition"] == expected + assert response.headers["cache-control"] == "no-store" + + +def test_inline_preserves_stored_mime_type_and_compression(server_url: str) -> None: + session = Client(server_url, token=TestSecretKey.get()).session( + Usecase("test-usecase", compression="zstd"), org=42, project=1337 + ) + payload = b'' + session.put(payload, key="svg-options", content_type="image/svg+xml") + url = session.object_url( + "svg-options", + token_validity=timedelta(minutes=5), + response_content_disposition="inline", + ) + # Unsupported overrides cannot change the stored metadata. + url += "&response-content-type=text/html&response-content-encoding=gzip" + with urllib.request.urlopen(url) as response: + assert response.headers["content-type"] == "image/svg+xml" + assert response.headers["content-encoding"] == "zstd" + assert response.headers["content-security-policy"] == "sandbox" + with zstandard.ZstdDecompressor().stream_reader( + BytesIO(response.read()) + ) as reader: + assert reader.read() == payload + + def test_object_url_read_only_token_succeeds(server_url: str) -> None: session = _presign_session(server_url) session.put(b"read only hello", key="read-only-url") diff --git a/clients/python/tests/test_presign.py b/clients/python/tests/test_presign.py index 8a2fd20e3..288a578b4 100644 --- a/clients/python/tests/test_presign.py +++ b/clients/python/tests/test_presign.py @@ -2,10 +2,17 @@ import base64 import os +from datetime import UTC, datetime, timedelta +from unittest.mock import patch +from urllib.parse import parse_qs, urlsplit -from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PublicKey +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import ( + Ed25519PrivateKey, + Ed25519PublicKey, +) from cryptography.hazmat.primitives.serialization import load_pem_public_key -from objectstore_client import presign, utils +from objectstore_client import Client, Usecase, presign, utils from objectstore_client.auth import SecretKey TEST_EDDSA_PRIVKEY_PATH = os.path.join(os.path.dirname(__file__), "ed25519.private.pem") @@ -73,3 +80,96 @@ def test_sign_canonical_form_roundtrips_with_public_key() -> None: signature = base64.urlsafe_b64decode(signature_b64 + padding) assert len(signature) == 64 public_key.verify(signature, canonical.encode()) # raises on mismatch + + +@pytest.mark.parametrize("key", ["café/looks%20encoded&+", "nested/space key"]) +def test_response_options_are_encoded_and_signed(key: str) -> None: + with open(TEST_EDDSA_PRIVKEY_PATH) as f: + secret = SecretKey("test_kid", f.read()) + session = Client("https://example.com/prefix", token=secret).session( + Usecase("testing"), org=1 + ) + cache_control = 'private, max-age=3600, x="a&b=+%?#"' + with patch("objectstore_client.client.datetime") as clock: + clock.now.return_value = datetime(2026, 4, 20, 13, 37, tzinfo=UTC) + legacy = session.presigned_object_url("GET", key) + explicit_none = session.presigned_object_url( + "GET", key, response_content_disposition=None, response_cache_control=None + ) + url = session.presigned_object_url( + "GET", + key, + response_content_disposition="inline", + response_cache_control=cache_control, + ) + assert legacy == explicit_none + legacy_path = utils.encode_path(f"/prefix/v1/objects/testing/org=1/{key}") + legacy_query = "os_kid=test_kid&os_timestamp=2026-04-20T13:37:00Z&os_duration=3600" + legacy_signature = secret.signature_for_canonical_form( + presign.build_canonical_form("GET", legacy_path, legacy_query) + ) + assert ( + legacy + == f"https://example.com:443{legacy_path}?{legacy_query}&os_sig={legacy_signature}" + ) + parsed = urlsplit(url) + assert parsed.path == urlsplit(legacy).path + query = parse_qs(parsed.query) + assert query["response-content-disposition"] == ["inline"] + assert query["response-cache-control"] == [cache_control] + assert len(query) == 6 + assert "%26" in parsed.query and "%2B" in parsed.query + canonical = presign.build_canonical_form("GET", parsed.path, parsed.query) + assert secret.signature_for_canonical_form(canonical) == query["os_sig"][0] + altered = parsed.query.replace("=inline", "=attachment") + assert ( + secret.signature_for_canonical_form( + presign.build_canonical_form("GET", parsed.path, altered) + ) + != query["os_sig"][0] + ) + + +def test_legacy_canonical_vector() -> None: + path = "/prefix/v1/objects/testing/org=1;project=2/caf%C3%A9/looks%2520encoded&+" + query = ( + "unknown=b&os_sig=ignored&dup=b&&OS_KID=test&dup=a&flag" + "&encoded=a%26b%3D%2B&unknown=a" + ) + expected = ( + f"GET\n{path}\n" + "dup=a&dup=b&encoded=a%26b%3D%2B&flag&os_kid=test&unknown=a&unknown=b" + ) + assert presign.build_canonical_form("HEAD", path, query) == expected + signature = Ed25519PrivateKey.from_private_bytes(bytes([7]) * 32).sign( + expected.encode() + ) + assert base64.urlsafe_b64encode(signature).decode().rstrip("=") == ( + "2_8Qv-7PKMDMd-ku1kF33Xh8aPXjLj3QaIE_nLJuaDUGxtWUDQzCiU6Uz0Mb1zgH" + "tfsCTB6CcQoZitooeJgmBw" + ) + + +def test_jwt_url_options_roundtrip_without_binding_token() -> None: + with open(TEST_EDDSA_PRIVKEY_PATH) as f: + secret = SecretKey("test_kid", f.read()) + session = Client("https://example.com/prefix", token=secret).session( + Usecase("testing") + ) + with patch.object(session, "mint_token", return_value="same.jwt.token"): + legacy = session.object_url("key", token_validity=timedelta(minutes=5)) + url = session.object_url( + "key", + token_validity=timedelta(minutes=5), + response_content_disposition="inline", + response_cache_control='private, x="&=+"', + ) + assert parse_qs(urlsplit(url).query) == { + "os_auth": ["same.jwt.token"], + "response-content-disposition": ["inline"], + "response-cache-control": ['private, x="&=+"'], + } + assert legacy.endswith("?os_auth=same.jwt.token") + assert session.object_url( + "key", response_content_disposition=None + ) == session.object_url("key") diff --git a/objectstore-server/docs/architecture.md b/objectstore-server/docs/architecture.md index 1af031fe5..61114f23b 100644 --- a/objectstore-server/docs/architecture.md +++ b/objectstore-server/docs/architecture.md @@ -41,7 +41,9 @@ A request flows through several layers before reaching the storage service: operation. 6. **Response**: metadata is mapped to HTTP headers (see [`objectstore-types` docs](objectstore_types) for the header mapping) and - the payload is streamed back. + the payload is streamed back. Successful individual-object downloads apply typed + [response-option headers](crate::response_options) for disposition, caching, and browser + safeguards; error responses retain their existing behavior. ## Authentication & Authorization diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index a501bb467..0be4332dd 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -23,6 +23,11 @@ //! Object metadata travels in request and response headers; see //! [`objectstore_types::metadata`] for the mapping and the separate JSON update contract. //! +//! GET and HEAD accept `response-content-disposition=inline|attachment` (default attachment) +//! and `response-cache-control=
`. These options apply only to successful object +//! responses, including ranged GETs. See [`crate::response_options`] for validation, browser +//! safeguards, and cache-lifetime behavior. +//! //! # Resumable Upload Endpoints //! //! A resumable upload transfers a single object across several requests. diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index cdc3f28a6..accd02e9f 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -25,8 +25,6 @@ //! strings are rejected by JSON deserialization with 422. Requests exceeding the //! usecase's `expiration.max`, measured from request time, return 400. -use std::fmt::Write as _; - use axum::body::Body; use axum::extract::{Request, State}; use axum::handler::Handler; @@ -37,7 +35,6 @@ use axum::{Json, Router}; use objectstore_service::backend::common::{ExpiryTarget, ExpiryUpdate, SetExpiryResponse}; use objectstore_service::error::ErrorKind; use objectstore_service::id::{ObjectContext, ObjectId}; -use objectstore_types::headers::ExtValue; use objectstore_types::metadata::{ExpiryAnchor, ExpiryExtension, Metadata, MetadataUpdate}; use objectstore_types::range::ContentRange; use serde::Serialize; @@ -48,6 +45,7 @@ use crate::endpoints::resumable; use crate::extractors::byte_range::OptionalByteRange; use crate::extractors::request_time::RequestTime; use crate::extractors::{Xt, body::MeteredBody}; +use crate::response_options::ResponseOptions; use crate::resumable::ResumableTarget; use crate::state::ServiceState; @@ -176,7 +174,7 @@ async fn object_get( service: AuthAwareService, State(state): State, Xt(id): Xt, - _headers: HeaderMap, + options: ResponseOptions, RequestTime(access_time): RequestTime, OptionalByteRange(byte_range): OptionalByteRange, ) -> ApiResult { @@ -230,14 +228,14 @@ async fn object_get( } }; - insert_content_disposition(&mut response, &metadata); insert_accept_ranges(&mut response); - Ok(response) + Ok((options.into_headers(metadata.filename.as_deref()), response).into_response()) } async fn object_head( service: AuthAwareService, + options: ResponseOptions, Xt(id): Xt, RequestTime(access_time): RequestTime, ) -> ApiResult { @@ -251,9 +249,8 @@ async fn object_head( insert_content_length(&mut headers, &metadata); let mut response = (StatusCode::OK, headers).into_response(); - insert_content_disposition(&mut response, &metadata); insert_accept_ranges(&mut response); - Ok(response) + Ok((options.into_headers(metadata.filename.as_deref()), response).into_response()) } /// Inserts a `Content-Length` header covering the complete object. @@ -270,55 +267,6 @@ fn insert_content_length(headers: &mut HeaderMap, metadata: &Metadata) { } } -/// Serves every object as an attachment, named after its filename when it has one. -fn insert_content_disposition(response: &mut Response, metadata: &Metadata) { - let value = match metadata.filename.as_deref() { - Some(filename) => format_content_disposition(filename), - None => http::HeaderValue::from_static("attachment"), - }; - response - .headers_mut() - .insert(http::header::CONTENT_DISPOSITION, value); -} - -/// Formats a `Content-Disposition: attachment; filename="..."` header value. -/// -/// 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). -/// -/// 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=\""); - for c in filename.chars() { - let c = match c { - '/' | '\\' => '-', - '.' if all_dots => '-', - '"' => { - result.push('\\'); - '"' - } - c if !c.is_ascii() || c.is_control() => '_', - c => c, - }; - result.push(c); - } - result.push('"'); - - 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 insert_object( service: AuthAwareService, State(state): State, diff --git a/objectstore-server/src/extractors/mod.rs b/objectstore-server/src/extractors/mod.rs index 8a2de1187..f2379c6c8 100644 --- a/objectstore-server/src/extractors/mod.rs +++ b/objectstore-server/src/extractors/mod.rs @@ -6,6 +6,7 @@ pub mod byte_range; pub mod downstream_service; mod id; pub mod request_time; +mod response_options; mod service; /// An extractor for a remote type. diff --git a/objectstore-server/src/extractors/response_options.rs b/objectstore-server/src/extractors/response_options.rs new file mode 100644 index 000000000..310c0f86d --- /dev/null +++ b/objectstore-server/src/extractors/response_options.rs @@ -0,0 +1,73 @@ +//! Extracts validated response overrides while preserving the original URI for authentication. + +use axum::extract::{FromRequestParts, Query}; +use http::{HeaderValue, request::Parts}; + +use crate::endpoints::common::ApiError; +use crate::response_options::{ContentDisposition, ResponseOptions}; + +impl FromRequestParts for ResponseOptions { + type Rejection = ApiError; + + async fn from_request_parts(parts: &mut Parts, _state: &S) -> Result { + let Query(pairs) = Query::>::try_from_uri(&parts.uri) + .map_err(|error| ApiError::map_client("invalid response options", error))?; + let mut disposition = None; + let mut cache_control = None; + for (name, value) in pairs { + if name.eq_ignore_ascii_case("response-content-disposition") { + if disposition.is_some() { + return Err(ApiError::client("duplicate response-content-disposition")); + } + disposition = Some(match value.as_str() { + "inline" => ContentDisposition::Inline, + "attachment" => ContentDisposition::Attachment, + _ => return Err(ApiError::client("invalid response-content-disposition")), + }); + } else if name.eq_ignore_ascii_case("response-cache-control") { + if cache_control.is_some() { + return Err(ApiError::client("duplicate response-cache-control")); + } + cache_control = Some(HeaderValue::from_str(&value).map_err(|error| { + ApiError::map_client("invalid response-cache-control", error) + })?); + } + } + Ok(Self { + disposition: disposition.unwrap_or_default(), + cache_control, + }) + } +} + +#[cfg(test)] +mod tests { + use axum::extract::FromRequestParts; + use http::Request; + + use crate::response_options::{ContentDisposition, ResponseOptions}; + + #[tokio::test] + async fn decodes_options_without_changing_signed_uri() { + let uri = "/object?unknown=b&unknown=a&%72esponse-content-disposition=inline&RESPONSE-CACHE-CONTROL=private%2C%20x%3D%22a%26b%3D%2B%25%3F%23%22"; + let (mut parts, ()) = Request::builder().uri(uri).body(()).unwrap().into_parts(); + let options = ResponseOptions::from_request_parts(&mut parts, &()) + .await + .unwrap(); + assert_eq!(options.disposition, ContentDisposition::Inline); + assert_eq!(options.cache_control.unwrap(), "private, x=\"a&b=+%?#\""); + assert_eq!(parts.uri.to_string(), uri); + } + + #[tokio::test] + async fn defaults_preserve_unrelated_parameters() { + for uri in ["/object", "/object?unknown=b&unknown=a&flag&&"] { + let (mut parts, ()) = Request::builder().uri(uri).body(()).unwrap().into_parts(); + let options = ResponseOptions::from_request_parts(&mut parts, &()) + .await + .unwrap(); + assert_eq!(options.disposition, ContentDisposition::Attachment); + assert!(options.cache_control.is_none()); + } + } +} diff --git a/objectstore-server/src/lib.rs b/objectstore-server/src/lib.rs index e505bf073..f1ab51b32 100644 --- a/objectstore-server/src/lib.rs +++ b/objectstore-server/src/lib.rs @@ -13,6 +13,7 @@ pub mod killswitches; pub mod multipart; pub mod observability; pub mod rate_limits; +pub mod response_options; pub mod resumable; pub mod state; pub mod usecases; diff --git a/objectstore-server/src/response_options.rs b/objectstore-server/src/response_options.rs new file mode 100644 index 000000000..4adfa7840 --- /dev/null +++ b/objectstore-server/src/response_options.rs @@ -0,0 +1,177 @@ +//! Presentation and caching options for successful individual-object downloads. +//! +//! The request extractor validates these options without modifying the signed URI. Handlers +//! attach their typed headers only to successful GET/HEAD responses, including partial content. +//! Error responses retain their existing headers and cache behavior. +//! +//! # Query options +//! +//! - `response-content-disposition`: exactly `inline` or `attachment` (the default). The stored +//! filename is retained and formatted using the existing download filename rules. +//! - `response-cache-control`: an HTTP-valid header value, passed through after URL decoding. +//! When omitted, no cache policy is added. Cache lifetimes are independent of authorization +//! expiry: cached responses can remain usable after a signed URL expires. +//! +//! Parameter names are case-insensitive, matching pre-signed URL canonicalization. Duplicate +//! supported options and invalid values are rejected with 400. Unrelated parameters are ignored +//! by this extractor and remain covered by the existing signature scheme. +//! +//! Content type and content encoding always come from stored metadata. Content-language and +//! expires overrides are not supported. No MIME allowlist is applied to inline responses. + +use std::convert::Infallible; +use std::fmt::Write as _; + +use axum::response::{IntoResponseParts, ResponseParts}; +use http::{HeaderValue, header}; +use objectstore_types::headers::ExtValue; + +/// Whether a browser should download an object or display it inline. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ContentDisposition { + /// Download the object (the default). + #[default] + Attachment, + /// Display the object with browser sandboxing enabled. + Inline, +} + +impl ContentDisposition { + fn as_str(self) -> &'static str { + match self { + Self::Attachment => "attachment", + Self::Inline => "inline", + } + } +} + +/// Validated `response-content-disposition` and `response-cache-control` query options. +/// +/// Disposition defaults to attachment. Cache control is passed through without interpreting its +/// directives or coordinating its lifetime with URL expiry. Stored MIME type and encoding are +/// never overridden, and inline content is not restricted by a MIME allowlist. +#[derive(Debug, Default)] +pub(crate) struct ResponseOptions { + /// Selected browser disposition, defaulting to attachment. + pub(crate) disposition: ContentDisposition, + /// Optional cache policy, validated only for HTTP header safety. + pub(crate) cache_control: Option, +} + +impl ResponseOptions { + /// Builds success-only headers using the stored filename, when present. + pub(crate) fn into_headers(self, filename: Option<&str>) -> ResponseOptionHeaders { + let disposition = match filename { + Some(filename) => format_content_disposition(self.disposition, filename), + None => HeaderValue::from_static(self.disposition.as_str()), + }; + ResponseOptionHeaders { + disposition, + cache_control: self.cache_control, + inline: self.disposition == ContentDisposition::Inline, + } + } +} + +/// Object presentation and caching headers, including mandatory browser safeguards. +/// +/// All successful downloads get `nosniff`; inline responses also get a CSP sandbox without +/// relaxation tokens. Sandboxing is a browser safeguard, not content sanitization, and does not +/// prevent all external resource loads. Apply these parts only to successful object responses. +#[derive(Debug)] +pub(crate) struct ResponseOptionHeaders { + disposition: HeaderValue, + cache_control: Option, + inline: bool, +} + +impl IntoResponseParts for ResponseOptionHeaders { + type Error = Infallible; + + fn into_response_parts( + self, + mut response: ResponseParts, + ) -> Result { + let headers = response.headers_mut(); + headers.insert(header::CONTENT_DISPOSITION, self.disposition); + headers.insert( + header::X_CONTENT_TYPE_OPTIONS, + HeaderValue::from_static("nosniff"), + ); + if let Some(cache_control) = self.cache_control { + headers.insert(header::CACHE_CONTROL, cache_control); + } + if self.inline { + headers.insert( + header::CONTENT_SECURITY_POLICY, + HeaderValue::from_static("sandbox"), + ); + } + Ok(response) + } +} + +/// Formats the selected disposition with a stored filename. +/// +/// 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). +/// +/// 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(disposition: ContentDisposition, filename: &str) -> HeaderValue { + let all_dots = filename.chars().all(|c| c == '.'); + + let mut result = format!("{}; filename=\"", disposition.as_str()); + for c in filename.chars() { + let c = match c { + '/' | '\\' => '-', + '.' if all_dots => '-', + '"' => { + result.push('\\'); + '"' + } + c if !c.is_ascii() || c.is_control() => '_', + c => c, + }; + result.push(c); + } + result.push('"'); + + 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. + HeaderValue::from_str(&result).expect("content disposition is a valid header value") +} + +#[cfg(test)] +mod tests { + use axum::response::IntoResponse; + + use crate::response_options::{ContentDisposition, ResponseOptions}; + + #[test] + fn inline_filename_retains_escaping_and_unicode_encoding() { + let response = ( + ResponseOptions { + disposition: ContentDisposition::Inline, + cache_control: None, + } + .into_headers(Some("réport.pdf")), + (), + ) + .into_response(); + assert_eq!( + response.headers()["content-disposition"], + "inline; filename=\"r_port.pdf\"; filename*=UTF-8''r%C3%A9port.pdf" + ); + assert_eq!(response.headers()["content-security-policy"], "sandbox"); + assert_eq!(response.headers()["x-content-type-options"], "nosniff"); + assert!(!response.headers().contains_key("cache-control")); + } +} diff --git a/objectstore-server/tests/presigned.rs b/objectstore-server/tests/presigned.rs index 81e920f67..4ba7584ae 100644 --- a/objectstore-server/tests/presigned.rs +++ b/objectstore-server/tests/presigned.rs @@ -148,6 +148,64 @@ async fn presigned_head_succeeds() -> Result<()> { Ok(()) } +#[tokio::test] +async fn response_options_are_signed_and_case_insensitive() -> Result<()> { + let server = test_server().await; + seed_object(&server, "hello").await?; + let legacy_query = presign_query(&Method::GET, OBJECT_PATH, ONE_HOUR); + let (base, _) = legacy_query.rsplit_once("&os_sig=").unwrap(); + let query = format!( + "{base}&response-content-disposition=inline&response-cache-control=private%2C%20max-age%3D3600&unknown=b&unknown=a" + ); + let signature = CanonicalRequest::new(&Method::GET, OBJECT_PATH, Some(&query)) + .sign(signing_key().as_bytes()); + let signed = format!("{query}&os_sig={signature}"); + let client = reqwest::Client::new(); + for method in [Method::GET, Method::HEAD] { + for query in [ + signed.clone(), + signed + .replace( + "response-content-disposition", + "RESPONSE-CONTENT-DISPOSITION", + ) + .replace("response-cache-control", "RESPONSE-CACHE-CONTROL"), + ] { + let response = client + .request(method.clone(), presigned_url(&server, OBJECT_PATH, &query)) + .send() + .await?; + assert_eq!(response.status(), reqwest::StatusCode::OK); + assert_eq!(response.headers()["content-disposition"], "inline"); + assert_eq!(response.headers()["cache-control"], "private, max-age=3600"); + assert_eq!(response.headers()["content-security-policy"], "sandbox"); + } + } + for query in [ + signed.replace("=inline", "=attachment"), + signed.replace("&response-content-disposition=inline", ""), + signed.replace("max-age%3D3600", "max-age%3D60"), + signed.replace("&response-cache-control=private%2C%20max-age%3D3600", ""), + format!("{legacy_query}&response-content-disposition=inline"), + format!("{legacy_query}&response-cache-control=public"), + ] { + let response = client + .get(presigned_url(&server, OBJECT_PATH, &query)) + .send() + .await?; + assert_eq!(response.status(), reqwest::StatusCode::UNAUTHORIZED); + for name in [ + "content-disposition", + "cache-control", + "x-content-type-options", + "content-security-policy", + ] { + assert!(!response.headers().contains_key(name)); + } + } + Ok(()) +} + #[tokio::test] async fn presigned_delete_is_rejected() -> Result<()> { let server = test_server().await; diff --git a/objectstore-server/tests/response_options.rs b/objectstore-server/tests/response_options.rs new file mode 100644 index 000000000..5f61f4c05 --- /dev/null +++ b/objectstore-server/tests/response_options.rs @@ -0,0 +1,174 @@ +//! Response overrides on successful downloads, with error behavior left unchanged. + +use anyhow::Result; +use http::{Method, StatusCode}; +use objectstore_server::config::{AuthZ, Config}; +use objectstore_test::server::TestServer; +use reqwest::Url; + +async fn test_server() -> TestServer { + TestServer::with_config(Config { + auth: AuthZ { + enforce: false, + ..Default::default() + }, + ..Default::default() + }) + .await +} + +#[tokio::test] +async fn successful_downloads_apply_options_and_preserve_metadata() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + let path = "/v1/objects/test/org=1/options"; + client + .put(server.url(path)) + .header("content-type", "text/html") + .header("x-sn-filename", "report.html") + .body("example") + .send() + .await? + .error_for_status()?; + + for method in [Method::GET, Method::HEAD] { + for disposition in [None, Some("attachment"), Some("inline")] { + let mut url = Url::parse(&server.url(path))?; + url.query_pairs_mut().append_pair( + "response-cache-control", + "private, max-age=3600, x=\"a&b=+%?#\"", + ); + if let Some(disposition) = disposition { + url.query_pairs_mut() + .append_pair("response-content-disposition", disposition); + } + let response = client.request(method.clone(), url).send().await?; + assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.headers()["content-type"], "text/html"); + assert_eq!(response.headers()["x-sn-filename"], "report.html"); + assert_eq!(response.headers()["content-length"], "20"); + assert_eq!( + response.headers()["content-disposition"], + format!( + "{}; filename=\"report.html\"", + disposition.unwrap_or("attachment") + ) + ); + assert_eq!( + response.headers()["cache-control"], + "private, max-age=3600, x=\"a&b=+%?#\"" + ); + assert_eq!(response.headers()["x-content-type-options"], "nosniff"); + assert_eq!( + response + .headers() + .get("content-security-policy") + .map(|v| v.to_str().unwrap()), + (disposition == Some("inline")).then_some("sandbox") + ); + let body = response.text().await?; + assert_eq!( + body, + if method == Method::HEAD { + "" + } else { + "example" + } + ); + } + } + let response = client.get(server.url(path)).send().await?; + assert!(!response.headers().contains_key("cache-control")); + Ok(()) +} + +#[tokio::test] +async fn ranged_and_error_responses() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + let path = "/v1/objects/test/org=1/range-options"; + client + .put(server.url(path)) + .body("abcdef") + .send() + .await? + .error_for_status()?; + let mut url = Url::parse(&server.url(path))?; + url.set_query(Some( + "response-content-disposition=inline&response-cache-control=public%2C%20max-age%3D3600", + )); + let response = client + .get(url.clone()) + .header("range", "bytes=1-3") + .send() + .await?; + assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); + assert_eq!(response.headers()["content-range"], "bytes 1-3/6"); + assert_eq!(response.headers()["content-length"], "3"); + assert_eq!(response.headers()["content-disposition"], "inline"); + assert_eq!(response.headers()["content-security-policy"], "sandbox"); + assert_eq!(response.text().await?, "bcd"); + + for (target, range, expected) in [ + (url.clone(), "bytes=99-", StatusCode::RANGE_NOT_SATISFIABLE), + ( + { + let mut missing = url.clone(); + missing.set_path("/v1/objects/test/org=1/missing-options"); + missing + }, + "bytes=0-", + StatusCode::NOT_FOUND, + ), + ] { + let response = client.get(target).header("range", range).send().await?; + assert_eq!(response.status(), expected); + for name in [ + "content-disposition", + "cache-control", + "x-content-type-options", + "content-security-policy", + ] { + assert!( + !response.headers().contains_key(name), + "unexpected {name} on {expected}" + ); + } + } + url.set_path("/v1/objects/test/org=1/missing-options"); + let response = client.head(url).send().await?; + assert_eq!(response.status(), StatusCode::NOT_FOUND); + for name in [ + "content-disposition", + "cache-control", + "x-content-type-options", + "content-security-policy", + ] { + assert!(!response.headers().contains_key(name)); + } + + Ok(()) +} + +#[tokio::test] +async fn invalid_and_duplicate_options_are_rejected() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + for query in [ + "response-content-disposition=other", + "response-content-disposition=inline%3Bfilename%3Dx", + "response-cache-control=public%0D%0AX-Evil%3Ayes", + "response-cache-control=%00", + "response-content-disposition=inline&RESPONSE-CONTENT-DISPOSITION=attachment", + "response-cache-control=public&%72esponse-cache-control=private", + ] { + let mut url = Url::parse(&server.url("/v1/objects/test/org=1/invalid-options"))?; + url.set_query(Some(query)); + assert_eq!( + client.get(url).send().await?.status(), + StatusCode::BAD_REQUEST, + "{query}" + ); + } + Ok(()) +} diff --git a/objectstore-types/src/metadata.rs b/objectstore-types/src/metadata.rs index 8d1c6160a..1c748c41f 100644 --- a/objectstore-types/src/metadata.rs +++ b/objectstore-types/src/metadata.rs @@ -375,9 +375,10 @@ pub struct Metadata { /// An optional filename associated with this object (header: `x-sn-filename`). /// - /// 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. Non-ASCII filenames additionally get an RFC 8187 `filename*` parameter. + /// Successful GET/HEAD responses include this name in `Content-Disposition`. + /// Disposition defaults to `attachment`; the `response-content-disposition` query option + /// can select `inline` while retaining 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)). diff --git a/objectstore-types/src/presign.rs b/objectstore-types/src/presign.rs index d815723db..1e434dd57 100644 --- a/objectstore-types/src/presign.rs +++ b/objectstore-types/src/presign.rs @@ -221,6 +221,24 @@ mod tests { ); } + #[test] + fn legacy_signature_vector_preserves_exotic_paths_and_queries() { + let path = "/prefix/v1/objects/testing/org=1;project=2/caf%C3%A9/looks%2520encoded&+"; + let query = + "unknown=b&os_sig=ignored&dup=b&&OS_KID=test&dup=a&flag&encoded=a%26b%3D%2B&unknown=a"; + let canonical = CanonicalRequest::new(&Method::HEAD, path, Some(query)); + assert_eq!( + canonical.0, + format!( + "GET\n{path}\ndup=a&dup=b&encoded=a%26b%3D%2B&flag&os_kid=test&unknown=a&unknown=b" + ) + ); + assert_eq!( + canonical.sign(&[7; 32]), + "2_8Qv-7PKMDMd-ku1kF33Xh8aPXjLj3QaIE_nLJuaDUGxtWUDQzCiU6Uz0Mb1zgHtfsCTB6CcQoZitooeJgmBw" + ); + } + #[test] fn sign_and_verify_roundtrip() { let sk = test_signing_key(); From f1337b16b9207fb3781f686e43427ac6b8c1f8e6 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 21 Oct 2026 11:58:56 +0200 Subject: [PATCH 2/6] ref: Remove download cache-control overrides --- clients/CHANGELOG.md | 2 +- clients/python/README.md | 13 ++++------ .../python/src/objectstore_client/client.py | 21 ++++------------ clients/python/tests/test_e2e.py | 6 ----- clients/python/tests/test_presign.py | 10 ++------ objectstore-server/docs/architecture.md | 2 +- objectstore-server/src/endpoints/mod.rs | 7 +++--- .../src/extractors/response_options.rs | 15 ++--------- objectstore-server/src/response_options.rs | 25 +++++-------------- objectstore-server/tests/presigned.rs | 20 +++++---------- objectstore-server/tests/response_options.rs | 19 +------------- 11 files changed, 32 insertions(+), 108 deletions(-) diff --git a/clients/CHANGELOG.md b/clients/CHANGELOG.md index 8ad066c38..0f8b57fe7 100644 --- a/clients/CHANGELOG.md +++ b/clients/CHANGELOG.md @@ -4,7 +4,7 @@ ### New Features ✨ -- (py-client) Add disposition and cache-control options to object URLs and pre-signed URLs (FS-570). +- (py-client) Add a content disposition option to object URLs and pre-signed URLs (FS-570). ## 0.2.10 diff --git a/clients/python/README.md b/clients/python/README.md index 75f78066b..e3802162f 100644 --- a/clients/python/README.md +++ b/clients/python/README.md @@ -256,25 +256,22 @@ with urllib.request.urlopen(url) as resp: ### Download response options -Both `Session.object_url` and `Session.presigned_object_url` accept keyword-only -`response_content_disposition` and `response_cache_control` options: +Both `Session.object_url` and `Session.presigned_object_url` accept a keyword-only +`response_content_disposition` option: ```python url = session.presigned_object_url( "GET", "my-key", response_content_disposition="inline", - response_cache_control="private, max-age=300", ) ``` Disposition accepts `inline` or `attachment`, defaulting to `attachment` when omitted. -The stored filename is retained. Cache control is passed through as an HTTP header value; -it is not interpreted or limited by authorization expiry. A cached response can remain -usable after the URL expires. Omitting cache control preserves the server's existing behavior. +The stored filename is retained. These options apply only to successful GET/HEAD responses, including ranged downloads. -404 and other error responses retain their existing headers and caching behavior. +404 and other error responses retain their existing headers. Successful downloads include `X-Content-Type-Options: nosniff`; inline responses also include `Content-Security-Policy: sandbox` without relaxation tokens. Inline MIME types are not restricted by an allowlist. Sandboxing is a browser safeguard, not content sanitization, @@ -283,7 +280,7 @@ and does not prevent all external resource loads. The object's declared content type and compression encoding cannot be overridden. Content-language and expires overrides are not supported. -Pre-signed URLs bind both options into the signature. JWT-bearing `object_url` URLs +Pre-signed URLs bind the disposition option into the signature. JWT-bearing `object_url` URLs only authenticate the JWT's claims, so recipients can edit the response options. Existing URLs and signatures are unchanged when the options are omitted. Deploy server support before callers rely on these options; older servers ignore them. diff --git a/clients/python/src/objectstore_client/client.py b/clients/python/src/objectstore_client/client.py index f9c67d26a..174d24144 100644 --- a/clients/python/src/objectstore_client/client.py +++ b/clients/python/src/objectstore_client/client.py @@ -54,7 +54,6 @@ def _response_options_query( disposition: Literal["inline", "attachment"] | None, - cache_control: str | None, ) -> str: """Encode individual response options without changing legacy URLs.""" pairs: list[tuple[str, str]] = [] @@ -64,8 +63,6 @@ def _response_options_query( f"unsupported response content disposition {disposition!r}" ) pairs.append(("response-content-disposition", disposition)) - if cache_control is not None: - pairs.append(("response-cache-control", cache_control)) return urlencode(pairs, quote_via=quote) @@ -732,7 +729,6 @@ def object_url( token_validity: timedelta | None = None, *, response_content_disposition: Literal["inline", "attachment"] | None = None, - response_cache_control: str | None = None, ) -> str: """ Generates a GET url to the object with the given `key`. @@ -747,8 +743,7 @@ def object_url( Response options apply only to successful GET/HEAD responses. Disposition defaults to attachment; inline responses include browser sandboxing without - restricting MIME types. Cache control is passed through independently of - authorization expiry, so cached responses can outlive the URL's validity. + restricting MIME types. These options are not bound by the JWT and can be changed by recipients. Use :meth:`presigned_object_url` to bind the options to a signature. @@ -757,9 +752,7 @@ def object_url( ``SecretKey`` is configured on this session, or if disposition is invalid. """ url = self._make_url(key, full=True) - query = _response_options_query( - response_content_disposition, response_cache_control - ) + query = _response_options_query(response_content_disposition) if query: url = f"{url}?{query}" if token_validity is None: @@ -781,7 +774,6 @@ def presigned_object_url( duration: timedelta = timedelta(hours=1), *, response_content_disposition: Literal["inline", "attachment"] | None = None, - response_cache_control: str | None = None, ) -> str: """ Generates a pre-signed URL authorizing a single ``method`` request on the @@ -801,10 +793,9 @@ def presigned_object_url( Response options apply only to successful GET/HEAD responses. Disposition defaults to attachment; inline responses include browser sandboxing without - restricting MIME types. Cache control is passed through independently of - authorization expiry, so cached responses can outlive the URL's validity. + restricting MIME types. - Both response options are included in the signature when supplied. + Response options are included in the signature when supplied. Note that `HEAD` and `GET` are considered equivalent from the point of view of signatures, meaning that a signature for `GET` can also be used for @@ -843,9 +834,7 @@ def presigned_object_url( f"&{presign.PARAM_DURATION}={duration_secs}" ) - response_query = _response_options_query( - response_content_disposition, response_cache_control - ) + response_query = _response_options_query(response_content_disposition) if response_query: encoded_query = f"{encoded_query}&{response_query}" diff --git a/clients/python/tests/test_e2e.py b/clients/python/tests/test_e2e.py index 2aff24961..1f96c01c6 100644 --- a/clients/python/tests/test_e2e.py +++ b/clients/python/tests/test_e2e.py @@ -1099,17 +1099,14 @@ def test_presigned_response_options(server_url: str, method: str) -> None: session = _presign_session(server_url) key = "inline-café/looks%20encoded&+" session.put(b"example", key=key, content_type="text/html") - cache_control = 'private, max-age=3600, x="a&b=+%?#"' url = session.presigned_object_url( "GET", key, response_content_disposition="inline", - response_cache_control=cache_control, ) with urllib.request.urlopen(urllib.request.Request(url, method=method)) as response: assert response.status == 200 assert response.headers["content-disposition"] == "inline" - assert response.headers["cache-control"] == cache_control assert response.headers["content-type"] == "text/html" assert response.headers["content-security-policy"] == "sandbox" assert response.headers["x-content-type-options"] == "nosniff" @@ -1122,7 +1119,6 @@ def test_presigned_response_options(server_url: str, method: str) -> None: assert status == 200 for tampered in [ url.replace("=inline", "=attachment"), - url.replace("max-age%3D3600", "max-age%3D60"), url.replace("response-content-disposition=inline&", ""), ]: status, _ = _fetch(tampered, method=method) @@ -1136,7 +1132,6 @@ def test_jwt_response_options_remain_editable(server_url: str) -> None: "jwt-options", token_validity=timedelta(minutes=5), response_content_disposition="inline", - response_cache_control="no-store", ) for target, expected in [ (url, "inline"), @@ -1145,7 +1140,6 @@ def test_jwt_response_options_remain_editable(server_url: str) -> None: with urllib.request.urlopen(target) as response: assert response.status == 200 assert response.headers["content-disposition"] == expected - assert response.headers["cache-control"] == "no-store" def test_inline_preserves_stored_mime_type_and_compression(server_url: str) -> None: diff --git a/clients/python/tests/test_presign.py b/clients/python/tests/test_presign.py index 288a578b4..7fa79374c 100644 --- a/clients/python/tests/test_presign.py +++ b/clients/python/tests/test_presign.py @@ -89,18 +89,16 @@ def test_response_options_are_encoded_and_signed(key: str) -> None: session = Client("https://example.com/prefix", token=secret).session( Usecase("testing"), org=1 ) - cache_control = 'private, max-age=3600, x="a&b=+%?#"' with patch("objectstore_client.client.datetime") as clock: clock.now.return_value = datetime(2026, 4, 20, 13, 37, tzinfo=UTC) legacy = session.presigned_object_url("GET", key) explicit_none = session.presigned_object_url( - "GET", key, response_content_disposition=None, response_cache_control=None + "GET", key, response_content_disposition=None ) url = session.presigned_object_url( "GET", key, response_content_disposition="inline", - response_cache_control=cache_control, ) assert legacy == explicit_none legacy_path = utils.encode_path(f"/prefix/v1/objects/testing/org=1/{key}") @@ -116,9 +114,7 @@ def test_response_options_are_encoded_and_signed(key: str) -> None: assert parsed.path == urlsplit(legacy).path query = parse_qs(parsed.query) assert query["response-content-disposition"] == ["inline"] - assert query["response-cache-control"] == [cache_control] - assert len(query) == 6 - assert "%26" in parsed.query and "%2B" in parsed.query + assert len(query) == 5 canonical = presign.build_canonical_form("GET", parsed.path, parsed.query) assert secret.signature_for_canonical_form(canonical) == query["os_sig"][0] altered = parsed.query.replace("=inline", "=attachment") @@ -162,12 +158,10 @@ def test_jwt_url_options_roundtrip_without_binding_token() -> None: "key", token_validity=timedelta(minutes=5), response_content_disposition="inline", - response_cache_control='private, x="&=+"', ) assert parse_qs(urlsplit(url).query) == { "os_auth": ["same.jwt.token"], "response-content-disposition": ["inline"], - "response-cache-control": ['private, x="&=+"'], } assert legacy.endswith("?os_auth=same.jwt.token") assert session.object_url( diff --git a/objectstore-server/docs/architecture.md b/objectstore-server/docs/architecture.md index 61114f23b..22625a0f7 100644 --- a/objectstore-server/docs/architecture.md +++ b/objectstore-server/docs/architecture.md @@ -42,7 +42,7 @@ A request flows through several layers before reaching the storage service: 6. **Response**: metadata is mapped to HTTP headers (see [`objectstore-types` docs](objectstore_types) for the header mapping) and the payload is streamed back. Successful individual-object downloads apply typed - [response-option headers](crate::response_options) for disposition, caching, and browser + [response-option headers](crate::response_options) for disposition and browser safeguards; error responses retain their existing behavior. ## Authentication & Authorization diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index 0be4332dd..b7fd0f3cf 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -23,10 +23,9 @@ //! Object metadata travels in request and response headers; see //! [`objectstore_types::metadata`] for the mapping and the separate JSON update contract. //! -//! GET and HEAD accept `response-content-disposition=inline|attachment` (default attachment) -//! and `response-cache-control=
`. These options apply only to successful object -//! responses, including ranged GETs. See [`crate::response_options`] for validation, browser -//! safeguards, and cache-lifetime behavior. +//! GET and HEAD accept `response-content-disposition=inline|attachment` (default attachment). +//! Response options apply only to successful object responses, including ranged GETs. See +//! [`crate::response_options`] for validation, browser safeguards, and filename formatting. //! //! # Resumable Upload Endpoints //! diff --git a/objectstore-server/src/extractors/response_options.rs b/objectstore-server/src/extractors/response_options.rs index 310c0f86d..75fc64a0a 100644 --- a/objectstore-server/src/extractors/response_options.rs +++ b/objectstore-server/src/extractors/response_options.rs @@ -1,7 +1,7 @@ //! Extracts validated response overrides while preserving the original URI for authentication. use axum::extract::{FromRequestParts, Query}; -use http::{HeaderValue, request::Parts}; +use http::request::Parts; use crate::endpoints::common::ApiError; use crate::response_options::{ContentDisposition, ResponseOptions}; @@ -13,7 +13,6 @@ impl FromRequestParts for ResponseOptions { let Query(pairs) = Query::>::try_from_uri(&parts.uri) .map_err(|error| ApiError::map_client("invalid response options", error))?; let mut disposition = None; - let mut cache_control = None; for (name, value) in pairs { if name.eq_ignore_ascii_case("response-content-disposition") { if disposition.is_some() { @@ -24,18 +23,10 @@ impl FromRequestParts for ResponseOptions { "attachment" => ContentDisposition::Attachment, _ => return Err(ApiError::client("invalid response-content-disposition")), }); - } else if name.eq_ignore_ascii_case("response-cache-control") { - if cache_control.is_some() { - return Err(ApiError::client("duplicate response-cache-control")); - } - cache_control = Some(HeaderValue::from_str(&value).map_err(|error| { - ApiError::map_client("invalid response-cache-control", error) - })?); } } Ok(Self { disposition: disposition.unwrap_or_default(), - cache_control, }) } } @@ -49,13 +40,12 @@ mod tests { #[tokio::test] async fn decodes_options_without_changing_signed_uri() { - let uri = "/object?unknown=b&unknown=a&%72esponse-content-disposition=inline&RESPONSE-CACHE-CONTROL=private%2C%20x%3D%22a%26b%3D%2B%25%3F%23%22"; + let uri = "/object?unknown=b&unknown=a&%72esponse-content-disposition=inline"; let (mut parts, ()) = Request::builder().uri(uri).body(()).unwrap().into_parts(); let options = ResponseOptions::from_request_parts(&mut parts, &()) .await .unwrap(); assert_eq!(options.disposition, ContentDisposition::Inline); - assert_eq!(options.cache_control.unwrap(), "private, x=\"a&b=+%?#\""); assert_eq!(parts.uri.to_string(), uri); } @@ -67,7 +57,6 @@ mod tests { .await .unwrap(); assert_eq!(options.disposition, ContentDisposition::Attachment); - assert!(options.cache_control.is_none()); } } } diff --git a/objectstore-server/src/response_options.rs b/objectstore-server/src/response_options.rs index 4adfa7840..7074abd09 100644 --- a/objectstore-server/src/response_options.rs +++ b/objectstore-server/src/response_options.rs @@ -1,16 +1,13 @@ -//! Presentation and caching options for successful individual-object downloads. +//! Presentation options for successful individual-object downloads. //! //! The request extractor validates these options without modifying the signed URI. Handlers //! attach their typed headers only to successful GET/HEAD responses, including partial content. -//! Error responses retain their existing headers and cache behavior. +//! Error responses retain their existing headers. //! //! # Query options //! //! - `response-content-disposition`: exactly `inline` or `attachment` (the default). The stored //! filename is retained and formatted using the existing download filename rules. -//! - `response-cache-control`: an HTTP-valid header value, passed through after URL decoding. -//! When omitted, no cache policy is added. Cache lifetimes are independent of authorization -//! expiry: cached responses can remain usable after a signed URL expires. //! //! Parameter names are case-insensitive, matching pre-signed URL canonicalization. Duplicate //! supported options and invalid values are rejected with 400. Unrelated parameters are ignored @@ -45,17 +42,14 @@ impl ContentDisposition { } } -/// Validated `response-content-disposition` and `response-cache-control` query options. +/// Validated presentation options for successful object downloads. /// -/// Disposition defaults to attachment. Cache control is passed through without interpreting its -/// directives or coordinating its lifetime with URL expiry. Stored MIME type and encoding are -/// never overridden, and inline content is not restricted by a MIME allowlist. +/// Disposition defaults to attachment. Stored MIME type and encoding are never overridden, +/// and inline content is not restricted by a MIME allowlist. #[derive(Debug, Default)] pub(crate) struct ResponseOptions { /// Selected browser disposition, defaulting to attachment. pub(crate) disposition: ContentDisposition, - /// Optional cache policy, validated only for HTTP header safety. - pub(crate) cache_control: Option, } impl ResponseOptions { @@ -67,13 +61,12 @@ impl ResponseOptions { }; ResponseOptionHeaders { disposition, - cache_control: self.cache_control, inline: self.disposition == ContentDisposition::Inline, } } } -/// Object presentation and caching headers, including mandatory browser safeguards. +/// Object presentation headers, including mandatory browser safeguards. /// /// All successful downloads get `nosniff`; inline responses also get a CSP sandbox without /// relaxation tokens. Sandboxing is a browser safeguard, not content sanitization, and does not @@ -81,7 +74,6 @@ impl ResponseOptions { #[derive(Debug)] pub(crate) struct ResponseOptionHeaders { disposition: HeaderValue, - cache_control: Option, inline: bool, } @@ -98,9 +90,6 @@ impl IntoResponseParts for ResponseOptionHeaders { header::X_CONTENT_TYPE_OPTIONS, HeaderValue::from_static("nosniff"), ); - if let Some(cache_control) = self.cache_control { - headers.insert(header::CACHE_CONTROL, cache_control); - } if self.inline { headers.insert( header::CONTENT_SECURITY_POLICY, @@ -160,7 +149,6 @@ mod tests { let response = ( ResponseOptions { disposition: ContentDisposition::Inline, - cache_control: None, } .into_headers(Some("réport.pdf")), (), @@ -172,6 +160,5 @@ mod tests { ); assert_eq!(response.headers()["content-security-policy"], "sandbox"); assert_eq!(response.headers()["x-content-type-options"], "nosniff"); - assert!(!response.headers().contains_key("cache-control")); } } diff --git a/objectstore-server/tests/presigned.rs b/objectstore-server/tests/presigned.rs index 4ba7584ae..fc01edc6c 100644 --- a/objectstore-server/tests/presigned.rs +++ b/objectstore-server/tests/presigned.rs @@ -154,9 +154,7 @@ async fn response_options_are_signed_and_case_insensitive() -> Result<()> { seed_object(&server, "hello").await?; let legacy_query = presign_query(&Method::GET, OBJECT_PATH, ONE_HOUR); let (base, _) = legacy_query.rsplit_once("&os_sig=").unwrap(); - let query = format!( - "{base}&response-content-disposition=inline&response-cache-control=private%2C%20max-age%3D3600&unknown=b&unknown=a" - ); + let query = format!("{base}&response-content-disposition=inline&unknown=b&unknown=a"); let signature = CanonicalRequest::new(&Method::GET, OBJECT_PATH, Some(&query)) .sign(signing_key().as_bytes()); let signed = format!("{query}&os_sig={signature}"); @@ -164,12 +162,10 @@ async fn response_options_are_signed_and_case_insensitive() -> Result<()> { for method in [Method::GET, Method::HEAD] { for query in [ signed.clone(), - signed - .replace( - "response-content-disposition", - "RESPONSE-CONTENT-DISPOSITION", - ) - .replace("response-cache-control", "RESPONSE-CACHE-CONTROL"), + signed.replace( + "response-content-disposition", + "RESPONSE-CONTENT-DISPOSITION", + ), ] { let response = client .request(method.clone(), presigned_url(&server, OBJECT_PATH, &query)) @@ -177,17 +173,14 @@ async fn response_options_are_signed_and_case_insensitive() -> Result<()> { .await?; assert_eq!(response.status(), reqwest::StatusCode::OK); assert_eq!(response.headers()["content-disposition"], "inline"); - assert_eq!(response.headers()["cache-control"], "private, max-age=3600"); assert_eq!(response.headers()["content-security-policy"], "sandbox"); } } for query in [ signed.replace("=inline", "=attachment"), signed.replace("&response-content-disposition=inline", ""), - signed.replace("max-age%3D3600", "max-age%3D60"), - signed.replace("&response-cache-control=private%2C%20max-age%3D3600", ""), + signed.replace("&unknown=b", ""), format!("{legacy_query}&response-content-disposition=inline"), - format!("{legacy_query}&response-cache-control=public"), ] { let response = client .get(presigned_url(&server, OBJECT_PATH, &query)) @@ -196,7 +189,6 @@ async fn response_options_are_signed_and_case_insensitive() -> Result<()> { assert_eq!(response.status(), reqwest::StatusCode::UNAUTHORIZED); for name in [ "content-disposition", - "cache-control", "x-content-type-options", "content-security-policy", ] { diff --git a/objectstore-server/tests/response_options.rs b/objectstore-server/tests/response_options.rs index 5f61f4c05..68e228938 100644 --- a/objectstore-server/tests/response_options.rs +++ b/objectstore-server/tests/response_options.rs @@ -34,10 +34,6 @@ async fn successful_downloads_apply_options_and_preserve_metadata() -> Result<() for method in [Method::GET, Method::HEAD] { for disposition in [None, Some("attachment"), Some("inline")] { let mut url = Url::parse(&server.url(path))?; - url.query_pairs_mut().append_pair( - "response-cache-control", - "private, max-age=3600, x=\"a&b=+%?#\"", - ); if let Some(disposition) = disposition { url.query_pairs_mut() .append_pair("response-content-disposition", disposition); @@ -54,10 +50,6 @@ async fn successful_downloads_apply_options_and_preserve_metadata() -> Result<() disposition.unwrap_or("attachment") ) ); - assert_eq!( - response.headers()["cache-control"], - "private, max-age=3600, x=\"a&b=+%?#\"" - ); assert_eq!(response.headers()["x-content-type-options"], "nosniff"); assert_eq!( response @@ -77,8 +69,6 @@ async fn successful_downloads_apply_options_and_preserve_metadata() -> Result<() ); } } - let response = client.get(server.url(path)).send().await?; - assert!(!response.headers().contains_key("cache-control")); Ok(()) } @@ -94,9 +84,7 @@ async fn ranged_and_error_responses() -> Result<()> { .await? .error_for_status()?; let mut url = Url::parse(&server.url(path))?; - url.set_query(Some( - "response-content-disposition=inline&response-cache-control=public%2C%20max-age%3D3600", - )); + url.set_query(Some("response-content-disposition=inline")); let response = client .get(url.clone()) .header("range", "bytes=1-3") @@ -125,7 +113,6 @@ async fn ranged_and_error_responses() -> Result<()> { assert_eq!(response.status(), expected); for name in [ "content-disposition", - "cache-control", "x-content-type-options", "content-security-policy", ] { @@ -140,7 +127,6 @@ async fn ranged_and_error_responses() -> Result<()> { assert_eq!(response.status(), StatusCode::NOT_FOUND); for name in [ "content-disposition", - "cache-control", "x-content-type-options", "content-security-policy", ] { @@ -157,10 +143,7 @@ async fn invalid_and_duplicate_options_are_rejected() -> Result<()> { for query in [ "response-content-disposition=other", "response-content-disposition=inline%3Bfilename%3Dx", - "response-cache-control=public%0D%0AX-Evil%3Ayes", - "response-cache-control=%00", "response-content-disposition=inline&RESPONSE-CONTENT-DISPOSITION=attachment", - "response-cache-control=public&%72esponse-cache-control=private", ] { let mut url = Url::parse(&server.url("/v1/objects/test/org=1/invalid-options"))?; url.set_query(Some(query)); From 8970aa277d2ebe57a4d50107b1af7d04aa813c46 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 21 Oct 2026 12:03:35 +0200 Subject: [PATCH 3/6] test: Trim download response option coverage --- clients/python/tests/test_e2e.py | 24 ++++--------- clients/python/tests/test_presign.py | 54 ++++------------------------ objectstore-types/src/presign.rs | 18 ---------- 3 files changed, 12 insertions(+), 84 deletions(-) diff --git a/clients/python/tests/test_e2e.py b/clients/python/tests/test_e2e.py index 1f96c01c6..309d1d358 100644 --- a/clients/python/tests/test_e2e.py +++ b/clients/python/tests/test_e2e.py @@ -1094,35 +1094,25 @@ def test_presigned_get_encoding_corner_cases(server_url: str, key: str) -> None: assert body == payload -@pytest.mark.parametrize("method", ["GET", "HEAD"]) -def test_presigned_response_options(server_url: str, method: str) -> None: +def test_presigned_response_options(server_url: str) -> None: session = _presign_session(server_url) - key = "inline-café/looks%20encoded&+" + key = "inline-example" session.put(b"example", key=key, content_type="text/html") url = session.presigned_object_url( "GET", key, response_content_disposition="inline", ) - with urllib.request.urlopen(urllib.request.Request(url, method=method)) as response: + with urllib.request.urlopen(url) as response: assert response.status == 200 assert response.headers["content-disposition"] == "inline" assert response.headers["content-type"] == "text/html" assert response.headers["content-security-policy"] == "sandbox" assert response.headers["x-content-type-options"] == "nosniff" - assert response.read() == (b"" if method == "HEAD" else b"example") + assert response.read() == b"example" - changed_case = url.replace( - "response-content-disposition", "RESPONSE-CONTENT-DISPOSITION" - ) - status, _ = _fetch(changed_case, method=method) - assert status == 200 - for tampered in [ - url.replace("=inline", "=attachment"), - url.replace("response-content-disposition=inline&", ""), - ]: - status, _ = _fetch(tampered, method=method) - assert status == 401 + status, _ = _fetch(url.replace("=inline", "=attachment")) + assert status == 401 def test_jwt_response_options_remain_editable(server_url: str) -> None: @@ -1153,8 +1143,6 @@ def test_inline_preserves_stored_mime_type_and_compression(server_url: str) -> N token_validity=timedelta(minutes=5), response_content_disposition="inline", ) - # Unsupported overrides cannot change the stored metadata. - url += "&response-content-type=text/html&response-content-encoding=gzip" with urllib.request.urlopen(url) as response: assert response.headers["content-type"] == "image/svg+xml" assert response.headers["content-encoding"] == "zstd" diff --git a/clients/python/tests/test_presign.py b/clients/python/tests/test_presign.py index 7fa79374c..c5b3cbae8 100644 --- a/clients/python/tests/test_presign.py +++ b/clients/python/tests/test_presign.py @@ -6,11 +6,7 @@ from unittest.mock import patch from urllib.parse import parse_qs, urlsplit -import pytest -from cryptography.hazmat.primitives.asymmetric.ed25519 import ( - Ed25519PrivateKey, - Ed25519PublicKey, -) +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PublicKey from cryptography.hazmat.primitives.serialization import load_pem_public_key from objectstore_client import Client, Usecase, presign, utils from objectstore_client.auth import SecretKey @@ -82,8 +78,7 @@ def test_sign_canonical_form_roundtrips_with_public_key() -> None: public_key.verify(signature, canonical.encode()) # raises on mismatch -@pytest.mark.parametrize("key", ["café/looks%20encoded&+", "nested/space key"]) -def test_response_options_are_encoded_and_signed(key: str) -> None: +def test_presigned_url_response_options() -> None: with open(TEST_EDDSA_PRIVKEY_PATH) as f: secret = SecretKey("test_kid", f.read()) session = Client("https://example.com/prefix", token=secret).session( @@ -91,59 +86,22 @@ def test_response_options_are_encoded_and_signed(key: str) -> None: ) with patch("objectstore_client.client.datetime") as clock: clock.now.return_value = datetime(2026, 4, 20, 13, 37, tzinfo=UTC) - legacy = session.presigned_object_url("GET", key) + legacy = session.presigned_object_url("GET", "key") explicit_none = session.presigned_object_url( - "GET", key, response_content_disposition=None + "GET", "key", response_content_disposition=None ) url = session.presigned_object_url( "GET", - key, + "key", response_content_disposition="inline", ) assert legacy == explicit_none - legacy_path = utils.encode_path(f"/prefix/v1/objects/testing/org=1/{key}") - legacy_query = "os_kid=test_kid&os_timestamp=2026-04-20T13:37:00Z&os_duration=3600" - legacy_signature = secret.signature_for_canonical_form( - presign.build_canonical_form("GET", legacy_path, legacy_query) - ) - assert ( - legacy - == f"https://example.com:443{legacy_path}?{legacy_query}&os_sig={legacy_signature}" - ) + assert "response-content-disposition" not in parse_qs(urlsplit(legacy).query) parsed = urlsplit(url) assert parsed.path == urlsplit(legacy).path query = parse_qs(parsed.query) assert query["response-content-disposition"] == ["inline"] assert len(query) == 5 - canonical = presign.build_canonical_form("GET", parsed.path, parsed.query) - assert secret.signature_for_canonical_form(canonical) == query["os_sig"][0] - altered = parsed.query.replace("=inline", "=attachment") - assert ( - secret.signature_for_canonical_form( - presign.build_canonical_form("GET", parsed.path, altered) - ) - != query["os_sig"][0] - ) - - -def test_legacy_canonical_vector() -> None: - path = "/prefix/v1/objects/testing/org=1;project=2/caf%C3%A9/looks%2520encoded&+" - query = ( - "unknown=b&os_sig=ignored&dup=b&&OS_KID=test&dup=a&flag" - "&encoded=a%26b%3D%2B&unknown=a" - ) - expected = ( - f"GET\n{path}\n" - "dup=a&dup=b&encoded=a%26b%3D%2B&flag&os_kid=test&unknown=a&unknown=b" - ) - assert presign.build_canonical_form("HEAD", path, query) == expected - signature = Ed25519PrivateKey.from_private_bytes(bytes([7]) * 32).sign( - expected.encode() - ) - assert base64.urlsafe_b64encode(signature).decode().rstrip("=") == ( - "2_8Qv-7PKMDMd-ku1kF33Xh8aPXjLj3QaIE_nLJuaDUGxtWUDQzCiU6Uz0Mb1zgH" - "tfsCTB6CcQoZitooeJgmBw" - ) def test_jwt_url_options_roundtrip_without_binding_token() -> None: diff --git a/objectstore-types/src/presign.rs b/objectstore-types/src/presign.rs index 1e434dd57..d815723db 100644 --- a/objectstore-types/src/presign.rs +++ b/objectstore-types/src/presign.rs @@ -221,24 +221,6 @@ mod tests { ); } - #[test] - fn legacy_signature_vector_preserves_exotic_paths_and_queries() { - let path = "/prefix/v1/objects/testing/org=1;project=2/caf%C3%A9/looks%2520encoded&+"; - let query = - "unknown=b&os_sig=ignored&dup=b&&OS_KID=test&dup=a&flag&encoded=a%26b%3D%2B&unknown=a"; - let canonical = CanonicalRequest::new(&Method::HEAD, path, Some(query)); - assert_eq!( - canonical.0, - format!( - "GET\n{path}\ndup=a&dup=b&encoded=a%26b%3D%2B&flag&os_kid=test&unknown=a&unknown=b" - ) - ); - assert_eq!( - canonical.sign(&[7; 32]), - "2_8Qv-7PKMDMd-ku1kF33Xh8aPXjLj3QaIE_nLJuaDUGxtWUDQzCiU6Uz0Mb1zgHtfsCTB6CcQoZitooeJgmBw" - ); - } - #[test] fn sign_and_verify_roundtrip() { let sk = test_signing_key(); From b55bc19b0fc0508c196fb166a56ed171f9fb6ee6 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 21 Oct 2026 13:21:22 +0200 Subject: [PATCH 4/6] ref: Organize download response options and headers --- Cargo.lock | 1 + objectstore-server/Cargo.toml | 1 + objectstore-server/docs/architecture.md | 2 +- objectstore-server/src/endpoints/common.rs | 117 ++++++++++++- objectstore-server/src/endpoints/mod.rs | 3 +- objectstore-server/src/endpoints/objects.rs | 16 +- objectstore-server/src/extractors/mod.rs | 2 +- .../src/extractors/response_options.rs | 83 ++++++--- objectstore-server/src/lib.rs | 1 - objectstore-server/src/response_options.rs | 164 ------------------ objectstore-types/src/metadata.rs | 5 +- 11 files changed, 195 insertions(+), 200 deletions(-) delete mode 100644 objectstore-server/src/response_options.rs diff --git a/Cargo.lock b/Cargo.lock index 7574e8aad..b3c9c8151 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2944,6 +2944,7 @@ dependencies = [ "tower", "tower-http 0.7.0", "tracing-subscriber", + "url", ] [[package]] diff --git a/objectstore-server/Cargo.toml b/objectstore-server/Cargo.toml index ee1cc168e..d7d68ba86 100644 --- a/objectstore-server/Cargo.toml +++ b/objectstore-server/Cargo.toml @@ -50,6 +50,7 @@ thread_local = { workspace = true } tokio = { workspace = true, features = ["fs", "macros", "net", "rt-multi-thread", "signal", "sync", "time"] } tower = { workspace = true } tower-http = { workspace = true, features = ["catch-panic", "metrics", "set-header", "trace"] } +url = { workspace = true } [target.'cfg(target_os = "linux")'.dependencies] jemalloc_pprof = { workspace = true, optional = true } diff --git a/objectstore-server/docs/architecture.md b/objectstore-server/docs/architecture.md index 22625a0f7..c8ddae350 100644 --- a/objectstore-server/docs/architecture.md +++ b/objectstore-server/docs/architecture.md @@ -42,7 +42,7 @@ A request flows through several layers before reaching the storage service: 6. **Response**: metadata is mapped to HTTP headers (see [`objectstore-types` docs](objectstore_types) for the header mapping) and the payload is streamed back. Successful individual-object downloads apply typed - [response-option headers](crate::response_options) for disposition and browser + [download headers](crate::endpoints::common) for disposition and browser safeguards; error responses retain their existing behavior. ## Authentication & Authorization diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 0ff0348d6..6b0432927 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -1,18 +1,22 @@ //! Common types and utilities for API endpoints. use std::borrow::Cow; +use std::convert::Infallible; use std::error::Error; +use std::fmt::Write as _; use axum::Json; use axum::http::StatusCode; -use axum::response::{IntoResponse, Response}; -use http::HeaderValue; +use axum::response::{IntoResponse, IntoResponseParts, Response, ResponseParts}; +use http::{HeaderValue, header}; use objectstore_service::error::{Error as ServiceError, ErrorKind as ServiceErrorKind}; +use objectstore_types::headers::ExtValue; use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::auth::AuthError; use crate::extractors::batch::BatchError; +use crate::extractors::response_options::ContentDisposition; /// A JSON error response returned by the API. #[derive(Serialize, Deserialize, Debug)] @@ -214,3 +218,112 @@ pub fn insert_accept_ranges(response: &mut Response) { HeaderValue::from_static("bytes"), ); } + +/// Object presentation headers, including mandatory browser safeguards. +/// +/// All successful downloads get `nosniff`; inline responses also get a CSP sandbox without +/// relaxation tokens. Sandboxing is a browser safeguard, not content sanitization, and does not +/// prevent all external resource loads. Apply these parts only to successful object responses. +#[derive(Debug)] +pub(crate) struct DownloadHeaders { + disposition: HeaderValue, + inline: bool, +} + +impl DownloadHeaders { + /// Builds success-only headers using the selected disposition and stored filename. + pub fn new(disposition: ContentDisposition, filename: Option<&str>) -> Self { + let header = match filename { + Some(filename) => format_content_disposition(disposition, filename), + None => HeaderValue::from_static(disposition.as_str()), + }; + Self { + disposition: header, + inline: disposition == ContentDisposition::Inline, + } + } +} + +impl IntoResponseParts for DownloadHeaders { + type Error = Infallible; + + fn into_response_parts( + self, + mut response: ResponseParts, + ) -> Result { + let headers = response.headers_mut(); + headers.insert(header::CONTENT_DISPOSITION, self.disposition); + headers.insert( + header::X_CONTENT_TYPE_OPTIONS, + HeaderValue::from_static("nosniff"), + ); + if self.inline { + headers.insert( + header::CONTENT_SECURITY_POLICY, + HeaderValue::from_static("sandbox"), + ); + } + Ok(response) + } +} + +/// Formats the selected disposition with a stored filename. +/// +/// 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). +/// +/// 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(disposition: ContentDisposition, filename: &str) -> HeaderValue { + let all_dots = filename.chars().all(|c| c == '.'); + + let mut result = format!("{}; filename=\"", disposition.as_str()); + for c in filename.chars() { + let c = match c { + '/' | '\\' => '-', + '.' if all_dots => '-', + '"' => { + result.push('\\'); + '"' + } + c if !c.is_ascii() || c.is_control() => '_', + c => c, + }; + result.push(c); + } + result.push('"'); + + 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. + HeaderValue::from_str(&result).expect("content disposition is a valid header value") +} + +#[cfg(test)] +mod tests { + use axum::response::IntoResponse; + + use crate::endpoints::common::DownloadHeaders; + use crate::extractors::response_options::ContentDisposition; + + #[test] + fn inline_filename_retains_escaping_and_unicode_encoding() { + let response = ( + DownloadHeaders::new(ContentDisposition::Inline, Some("réport.pdf")), + (), + ) + .into_response(); + assert_eq!( + response.headers()["content-disposition"], + "inline; filename=\"r_port.pdf\"; filename*=UTF-8''r%C3%A9port.pdf" + ); + assert_eq!(response.headers()["content-security-policy"], "sandbox"); + assert_eq!(response.headers()["x-content-type-options"], "nosniff"); + } +} diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index b7fd0f3cf..4f790a46f 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -25,7 +25,8 @@ //! //! GET and HEAD accept `response-content-disposition=inline|attachment` (default attachment). //! Response options apply only to successful object responses, including ranged GETs. See -//! [`crate::response_options`] for validation, browser safeguards, and filename formatting. +//! [`crate::extractors`] for validation and [`common`] for browser safeguards and filename +//! formatting. //! //! # Resumable Upload Endpoints //! diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index accd02e9f..fd9dcbc3e 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -40,12 +40,12 @@ use objectstore_types::range::ContentRange; use serde::Serialize; use crate::auth::AuthAwareService; -use crate::endpoints::common::{ApiError, ApiResult, insert_accept_ranges}; +use crate::endpoints::common::{ApiError, ApiResult, DownloadHeaders, insert_accept_ranges}; use crate::endpoints::resumable; use crate::extractors::byte_range::OptionalByteRange; use crate::extractors::request_time::RequestTime; +use crate::extractors::response_options::ResponseOptions; use crate::extractors::{Xt, body::MeteredBody}; -use crate::response_options::ResponseOptions; use crate::resumable::ResumableTarget; use crate::state::ServiceState; @@ -230,7 +230,11 @@ async fn object_get( insert_accept_ranges(&mut response); - Ok((options.into_headers(metadata.filename.as_deref()), response).into_response()) + Ok(( + DownloadHeaders::new(options.disposition, metadata.filename.as_deref()), + response, + ) + .into_response()) } async fn object_head( @@ -250,7 +254,11 @@ async fn object_head( let mut response = (StatusCode::OK, headers).into_response(); insert_accept_ranges(&mut response); - Ok((options.into_headers(metadata.filename.as_deref()), response).into_response()) + Ok(( + DownloadHeaders::new(options.disposition, metadata.filename.as_deref()), + response, + ) + .into_response()) } /// Inserts a `Content-Length` header covering the complete object. diff --git a/objectstore-server/src/extractors/mod.rs b/objectstore-server/src/extractors/mod.rs index f2379c6c8..8e92a68ac 100644 --- a/objectstore-server/src/extractors/mod.rs +++ b/objectstore-server/src/extractors/mod.rs @@ -6,7 +6,7 @@ pub mod byte_range; pub mod downstream_service; mod id; pub mod request_time; -mod response_options; +pub mod response_options; mod service; /// An extractor for a remote type. diff --git a/objectstore-server/src/extractors/response_options.rs b/objectstore-server/src/extractors/response_options.rs index 75fc64a0a..9dc91adc3 100644 --- a/objectstore-server/src/extractors/response_options.rs +++ b/objectstore-server/src/extractors/response_options.rs @@ -1,33 +1,72 @@ -//! Extracts validated response overrides while preserving the original URI for authentication. +//! Presentation options for successful individual-object downloads. +//! +//! The request extractor validates these options without modifying the signed URI. Handlers +//! attach their typed headers only to successful GET/HEAD responses, including partial content. +//! Error responses retain their existing headers. +//! +//! # Query options +//! +//! - `response-content-disposition`: exactly `inline` or `attachment` (the default). The stored +//! filename is retained and formatted using the existing download filename rules. +//! +//! Parameter names are case-insensitive, matching pre-signed URL canonicalization. Duplicate +//! supported options and invalid values are rejected with 400. Unrelated parameters are ignored +//! by this extractor and remain covered by the existing signature scheme. +//! +//! Content type and content encoding always come from stored metadata. Content-language and +//! expires overrides are not supported. No MIME allowlist is applied to inline responses. -use axum::extract::{FromRequestParts, Query}; +use axum::extract::FromRequestParts; use http::request::Parts; +use serde::Deserialize; +use serde::de::value::{Error, MapDeserializer}; +use url::form_urlencoded; use crate::endpoints::common::ApiError; -use crate::response_options::{ContentDisposition, ResponseOptions}; + +/// Whether a browser should download an object or display it inline. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum ContentDisposition { + /// Download the object (the default). + #[default] + Attachment, + /// Display the object with browser sandboxing enabled. + Inline, +} + +impl ContentDisposition { + /// Returns the disposition's HTTP header value. + pub fn as_str(self) -> &'static str { + match self { + Self::Attachment => "attachment", + Self::Inline => "inline", + } + } +} + +/// Validated presentation options for successful object downloads. +/// +/// Disposition defaults to attachment. Stored MIME type and encoding are never overridden, +/// and inline content is not restricted by a MIME allowlist. +#[derive(Debug, Default, Deserialize)] +pub struct ResponseOptions { + /// Selected browser disposition, defaulting to attachment. + #[serde(default, rename = "response-content-disposition")] + pub disposition: ContentDisposition, +} impl FromRequestParts for ResponseOptions { type Rejection = ApiError; async fn from_request_parts(parts: &mut Parts, _state: &S) -> Result { - let Query(pairs) = Query::>::try_from_uri(&parts.uri) - .map_err(|error| ApiError::map_client("invalid response options", error))?; - let mut disposition = None; - for (name, value) in pairs { - if name.eq_ignore_ascii_case("response-content-disposition") { - if disposition.is_some() { - return Err(ApiError::client("duplicate response-content-disposition")); - } - disposition = Some(match value.as_str() { - "inline" => ContentDisposition::Inline, - "attachment" => ContentDisposition::Attachment, - _ => return Err(ApiError::client("invalid response-content-disposition")), - }); - } - } - Ok(Self { - disposition: disposition.unwrap_or_default(), - }) + // Query matches names case-sensitively. Normalize them to match + // pre-signed URL canonicalization without modifying the original URI. + let pairs = form_urlencoded::parse(parts.uri.query().unwrap_or_default().as_bytes()) + .map(|(name, value)| (name.to_ascii_lowercase(), value.into_owned())); + + Self::deserialize(MapDeserializer::<_, Error>::new(pairs)) + .map_err(|error| ApiError::map_client("invalid response options", error)) } } @@ -36,7 +75,7 @@ mod tests { use axum::extract::FromRequestParts; use http::Request; - use crate::response_options::{ContentDisposition, ResponseOptions}; + use crate::extractors::response_options::{ContentDisposition, ResponseOptions}; #[tokio::test] async fn decodes_options_without_changing_signed_uri() { diff --git a/objectstore-server/src/lib.rs b/objectstore-server/src/lib.rs index f1ab51b32..e505bf073 100644 --- a/objectstore-server/src/lib.rs +++ b/objectstore-server/src/lib.rs @@ -13,7 +13,6 @@ pub mod killswitches; pub mod multipart; pub mod observability; pub mod rate_limits; -pub mod response_options; pub mod resumable; pub mod state; pub mod usecases; diff --git a/objectstore-server/src/response_options.rs b/objectstore-server/src/response_options.rs deleted file mode 100644 index 7074abd09..000000000 --- a/objectstore-server/src/response_options.rs +++ /dev/null @@ -1,164 +0,0 @@ -//! Presentation options for successful individual-object downloads. -//! -//! The request extractor validates these options without modifying the signed URI. Handlers -//! attach their typed headers only to successful GET/HEAD responses, including partial content. -//! Error responses retain their existing headers. -//! -//! # Query options -//! -//! - `response-content-disposition`: exactly `inline` or `attachment` (the default). The stored -//! filename is retained and formatted using the existing download filename rules. -//! -//! Parameter names are case-insensitive, matching pre-signed URL canonicalization. Duplicate -//! supported options and invalid values are rejected with 400. Unrelated parameters are ignored -//! by this extractor and remain covered by the existing signature scheme. -//! -//! Content type and content encoding always come from stored metadata. Content-language and -//! expires overrides are not supported. No MIME allowlist is applied to inline responses. - -use std::convert::Infallible; -use std::fmt::Write as _; - -use axum::response::{IntoResponseParts, ResponseParts}; -use http::{HeaderValue, header}; -use objectstore_types::headers::ExtValue; - -/// Whether a browser should download an object or display it inline. -#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] -pub(crate) enum ContentDisposition { - /// Download the object (the default). - #[default] - Attachment, - /// Display the object with browser sandboxing enabled. - Inline, -} - -impl ContentDisposition { - fn as_str(self) -> &'static str { - match self { - Self::Attachment => "attachment", - Self::Inline => "inline", - } - } -} - -/// Validated presentation options for successful object downloads. -/// -/// Disposition defaults to attachment. Stored MIME type and encoding are never overridden, -/// and inline content is not restricted by a MIME allowlist. -#[derive(Debug, Default)] -pub(crate) struct ResponseOptions { - /// Selected browser disposition, defaulting to attachment. - pub(crate) disposition: ContentDisposition, -} - -impl ResponseOptions { - /// Builds success-only headers using the stored filename, when present. - pub(crate) fn into_headers(self, filename: Option<&str>) -> ResponseOptionHeaders { - let disposition = match filename { - Some(filename) => format_content_disposition(self.disposition, filename), - None => HeaderValue::from_static(self.disposition.as_str()), - }; - ResponseOptionHeaders { - disposition, - inline: self.disposition == ContentDisposition::Inline, - } - } -} - -/// Object presentation headers, including mandatory browser safeguards. -/// -/// All successful downloads get `nosniff`; inline responses also get a CSP sandbox without -/// relaxation tokens. Sandboxing is a browser safeguard, not content sanitization, and does not -/// prevent all external resource loads. Apply these parts only to successful object responses. -#[derive(Debug)] -pub(crate) struct ResponseOptionHeaders { - disposition: HeaderValue, - inline: bool, -} - -impl IntoResponseParts for ResponseOptionHeaders { - type Error = Infallible; - - fn into_response_parts( - self, - mut response: ResponseParts, - ) -> Result { - let headers = response.headers_mut(); - headers.insert(header::CONTENT_DISPOSITION, self.disposition); - headers.insert( - header::X_CONTENT_TYPE_OPTIONS, - HeaderValue::from_static("nosniff"), - ); - if self.inline { - headers.insert( - header::CONTENT_SECURITY_POLICY, - HeaderValue::from_static("sandbox"), - ); - } - Ok(response) - } -} - -/// Formats the selected disposition with a stored filename. -/// -/// 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). -/// -/// 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(disposition: ContentDisposition, filename: &str) -> HeaderValue { - let all_dots = filename.chars().all(|c| c == '.'); - - let mut result = format!("{}; filename=\"", disposition.as_str()); - for c in filename.chars() { - let c = match c { - '/' | '\\' => '-', - '.' if all_dots => '-', - '"' => { - result.push('\\'); - '"' - } - c if !c.is_ascii() || c.is_control() => '_', - c => c, - }; - result.push(c); - } - result.push('"'); - - 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. - HeaderValue::from_str(&result).expect("content disposition is a valid header value") -} - -#[cfg(test)] -mod tests { - use axum::response::IntoResponse; - - use crate::response_options::{ContentDisposition, ResponseOptions}; - - #[test] - fn inline_filename_retains_escaping_and_unicode_encoding() { - let response = ( - ResponseOptions { - disposition: ContentDisposition::Inline, - } - .into_headers(Some("réport.pdf")), - (), - ) - .into_response(); - assert_eq!( - response.headers()["content-disposition"], - "inline; filename=\"r_port.pdf\"; filename*=UTF-8''r%C3%A9port.pdf" - ); - assert_eq!(response.headers()["content-security-policy"], "sandbox"); - assert_eq!(response.headers()["x-content-type-options"], "nosniff"); - } -} diff --git a/objectstore-types/src/metadata.rs b/objectstore-types/src/metadata.rs index 1c748c41f..f2515ae88 100644 --- a/objectstore-types/src/metadata.rs +++ b/objectstore-types/src/metadata.rs @@ -375,10 +375,7 @@ pub struct Metadata { /// An optional filename associated with this object (header: `x-sn-filename`). /// - /// Successful GET/HEAD responses include this name in `Content-Disposition`. - /// Disposition defaults to `attachment`; the `response-content-disposition` query option - /// can select `inline` while retaining this name. Non-ASCII filenames additionally get - /// an RFC 8187 `filename*` parameter. + /// This filename is returned when downloading the object. /// /// 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)). From f47922ae1ce8b8ec162989930fbded3ae7564c6f Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 21 Oct 2026 15:07:01 +0200 Subject: [PATCH 5/6] ref: Apply download response review feedback --- clients/CHANGELOG.md | 2 +- clients/python/README.md | 17 +- .../python/src/objectstore_client/client.py | 14 +- objectstore-server/docs/architecture.md | 4 +- objectstore-server/src/endpoints/common.rs | 117 +------------ objectstore-server/src/endpoints/mod.rs | 4 +- objectstore-server/src/endpoints/objects.rs | 94 +++++++++-- .../src/extractors/response_options.rs | 42 +++++ objectstore-server/tests/objects.rs | 145 ++++++++++++++++ objectstore-server/tests/response_options.rs | 157 ------------------ 10 files changed, 280 insertions(+), 316 deletions(-) delete mode 100644 objectstore-server/tests/response_options.rs diff --git a/clients/CHANGELOG.md b/clients/CHANGELOG.md index 0f8b57fe7..d30f26f9c 100644 --- a/clients/CHANGELOG.md +++ b/clients/CHANGELOG.md @@ -4,7 +4,7 @@ ### New Features ✨ -- (py-client) Add a content disposition option to object URLs and pre-signed URLs (FS-570). +- (Python) Add a content disposition option to object URLs and pre-signed URLs (FS-570). ## 0.2.10 diff --git a/clients/python/README.md b/clients/python/README.md index e3802162f..4254eff40 100644 --- a/clients/python/README.md +++ b/clients/python/README.md @@ -256,7 +256,7 @@ with urllib.request.urlopen(url) as resp: ### Download response options -Both `Session.object_url` and `Session.presigned_object_url` accept a keyword-only +Both `Session.object_url` and `Session.presigned_object_url` accept a `response_content_disposition` option: ```python @@ -270,21 +270,6 @@ url = session.presigned_object_url( Disposition accepts `inline` or `attachment`, defaulting to `attachment` when omitted. The stored filename is retained. -These options apply only to successful GET/HEAD responses, including ranged downloads. -404 and other error responses retain their existing headers. -Successful downloads include `X-Content-Type-Options: nosniff`; inline responses also include -`Content-Security-Policy: sandbox` without relaxation tokens. Inline MIME types are not -restricted by an allowlist. Sandboxing is a browser safeguard, not content sanitization, -and does not prevent all external resource loads. - -The object's declared content type and compression encoding cannot be overridden. -Content-language and expires overrides are not supported. - -Pre-signed URLs bind the disposition option into the signature. JWT-bearing `object_url` URLs -only authenticate the JWT's claims, so recipients can edit the response options. -Existing URLs and signatures are unchanged when the options are omitted. -Deploy server support before callers rely on these options; older servers ignore them. - ## Configuration In production, store the `Client` and `Usecase` at module level and reuse them. diff --git a/clients/python/src/objectstore_client/client.py b/clients/python/src/objectstore_client/client.py index 174d24144..56de8276e 100644 --- a/clients/python/src/objectstore_client/client.py +++ b/clients/python/src/objectstore_client/client.py @@ -727,7 +727,6 @@ def object_url( self, key: str, token_validity: timedelta | None = None, - *, response_content_disposition: Literal["inline", "attachment"] | None = None, ) -> str: """ @@ -741,11 +740,10 @@ def object_url( information is embedded in the returned URL's query string, valid for the given duration. - Response options apply only to successful GET/HEAD responses. Disposition - defaults to attachment; inline responses include browser sandboxing without - restricting MIME types. + ``response_content_disposition`` selects ``attachment`` (default) or ``inline`` + for successful GET/HEAD responses, retaining the stored filename. - These options are not bound by the JWT and can be changed by recipients. + These options are not bound by the auth token and can be changed by recipients. Use :meth:`presigned_object_url` to bind the options to a signature. Raises ``ValueError`` if ``token_validity`` is provided but no @@ -772,7 +770,6 @@ def presigned_object_url( method: Literal["GET", "HEAD"], key: str, duration: timedelta = timedelta(hours=1), - *, response_content_disposition: Literal["inline", "attachment"] | None = None, ) -> str: """ @@ -791,9 +788,8 @@ def presigned_object_url( a request without an auth token. It can be handed to any HTTP client; the URL is already percent-encoded, and must be transmitted verbatim. - Response options apply only to successful GET/HEAD responses. Disposition - defaults to attachment; inline responses include browser sandboxing without - restricting MIME types. + ``response_content_disposition`` selects ``attachment`` (default) or ``inline`` + for successful GET/HEAD responses, retaining the stored filename. Response options are included in the signature when supplied. diff --git a/objectstore-server/docs/architecture.md b/objectstore-server/docs/architecture.md index c8ddae350..1af031fe5 100644 --- a/objectstore-server/docs/architecture.md +++ b/objectstore-server/docs/architecture.md @@ -41,9 +41,7 @@ A request flows through several layers before reaching the storage service: operation. 6. **Response**: metadata is mapped to HTTP headers (see [`objectstore-types` docs](objectstore_types) for the header mapping) and - the payload is streamed back. Successful individual-object downloads apply typed - [download headers](crate::endpoints::common) for disposition and browser - safeguards; error responses retain their existing behavior. + the payload is streamed back. ## Authentication & Authorization diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 6b0432927..0ff0348d6 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -1,22 +1,18 @@ //! Common types and utilities for API endpoints. use std::borrow::Cow; -use std::convert::Infallible; use std::error::Error; -use std::fmt::Write as _; use axum::Json; use axum::http::StatusCode; -use axum::response::{IntoResponse, IntoResponseParts, Response, ResponseParts}; -use http::{HeaderValue, header}; +use axum::response::{IntoResponse, Response}; +use http::HeaderValue; use objectstore_service::error::{Error as ServiceError, ErrorKind as ServiceErrorKind}; -use objectstore_types::headers::ExtValue; use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::auth::AuthError; use crate::extractors::batch::BatchError; -use crate::extractors::response_options::ContentDisposition; /// A JSON error response returned by the API. #[derive(Serialize, Deserialize, Debug)] @@ -218,112 +214,3 @@ pub fn insert_accept_ranges(response: &mut Response) { HeaderValue::from_static("bytes"), ); } - -/// Object presentation headers, including mandatory browser safeguards. -/// -/// All successful downloads get `nosniff`; inline responses also get a CSP sandbox without -/// relaxation tokens. Sandboxing is a browser safeguard, not content sanitization, and does not -/// prevent all external resource loads. Apply these parts only to successful object responses. -#[derive(Debug)] -pub(crate) struct DownloadHeaders { - disposition: HeaderValue, - inline: bool, -} - -impl DownloadHeaders { - /// Builds success-only headers using the selected disposition and stored filename. - pub fn new(disposition: ContentDisposition, filename: Option<&str>) -> Self { - let header = match filename { - Some(filename) => format_content_disposition(disposition, filename), - None => HeaderValue::from_static(disposition.as_str()), - }; - Self { - disposition: header, - inline: disposition == ContentDisposition::Inline, - } - } -} - -impl IntoResponseParts for DownloadHeaders { - type Error = Infallible; - - fn into_response_parts( - self, - mut response: ResponseParts, - ) -> Result { - let headers = response.headers_mut(); - headers.insert(header::CONTENT_DISPOSITION, self.disposition); - headers.insert( - header::X_CONTENT_TYPE_OPTIONS, - HeaderValue::from_static("nosniff"), - ); - if self.inline { - headers.insert( - header::CONTENT_SECURITY_POLICY, - HeaderValue::from_static("sandbox"), - ); - } - Ok(response) - } -} - -/// Formats the selected disposition with a stored filename. -/// -/// 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). -/// -/// 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(disposition: ContentDisposition, filename: &str) -> HeaderValue { - let all_dots = filename.chars().all(|c| c == '.'); - - let mut result = format!("{}; filename=\"", disposition.as_str()); - for c in filename.chars() { - let c = match c { - '/' | '\\' => '-', - '.' if all_dots => '-', - '"' => { - result.push('\\'); - '"' - } - c if !c.is_ascii() || c.is_control() => '_', - c => c, - }; - result.push(c); - } - result.push('"'); - - 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. - HeaderValue::from_str(&result).expect("content disposition is a valid header value") -} - -#[cfg(test)] -mod tests { - use axum::response::IntoResponse; - - use crate::endpoints::common::DownloadHeaders; - use crate::extractors::response_options::ContentDisposition; - - #[test] - fn inline_filename_retains_escaping_and_unicode_encoding() { - let response = ( - DownloadHeaders::new(ContentDisposition::Inline, Some("réport.pdf")), - (), - ) - .into_response(); - assert_eq!( - response.headers()["content-disposition"], - "inline; filename=\"r_port.pdf\"; filename*=UTF-8''r%C3%A9port.pdf" - ); - assert_eq!(response.headers()["content-security-policy"], "sandbox"); - assert_eq!(response.headers()["x-content-type-options"], "nosniff"); - } -} diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index 4f790a46f..a661dd822 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -25,8 +25,8 @@ //! //! GET and HEAD accept `response-content-disposition=inline|attachment` (default attachment). //! Response options apply only to successful object responses, including ranged GETs. See -//! [`crate::extractors`] for validation and [`common`] for browser safeguards and filename -//! formatting. +//! [`crate::extractors::response_options`] for validation. The object handlers construct +//! download headers with browser safeguards and filename formatting. //! //! # Resumable Upload Endpoints //! diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index fd9dcbc3e..0013ad9f0 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/objectstore-server/src/endpoints/objects.rs @@ -25,13 +25,16 @@ //! strings are rejected by JSON deserialization with 422. Requests exceeding the //! usecase's `expiration.max`, measured from request time, return 400. +use std::convert::Infallible; + use axum::body::Body; use axum::extract::{Request, State}; use axum::handler::Handler; use axum::http::{HeaderMap, StatusCode}; -use axum::response::{IntoResponse, Response}; +use axum::response::{IntoResponse, IntoResponseParts, Response, ResponseParts}; use axum::routing; use axum::{Json, Router}; +use http::{HeaderValue, header}; use objectstore_service::backend::common::{ExpiryTarget, ExpiryUpdate, SetExpiryResponse}; use objectstore_service::error::ErrorKind; use objectstore_service::id::{ObjectContext, ObjectId}; @@ -40,11 +43,11 @@ use objectstore_types::range::ContentRange; use serde::Serialize; use crate::auth::AuthAwareService; -use crate::endpoints::common::{ApiError, ApiResult, DownloadHeaders, insert_accept_ranges}; +use crate::endpoints::common::{ApiError, ApiResult, insert_accept_ranges}; use crate::endpoints::resumable; use crate::extractors::byte_range::OptionalByteRange; use crate::extractors::request_time::RequestTime; -use crate::extractors::response_options::ResponseOptions; +use crate::extractors::response_options::{ContentDisposition, ResponseOptions}; use crate::extractors::{Xt, body::MeteredBody}; use crate::resumable::ResumableTarget; use crate::state::ServiceState; @@ -230,11 +233,8 @@ async fn object_get( insert_accept_ranges(&mut response); - Ok(( - DownloadHeaders::new(options.disposition, metadata.filename.as_deref()), - response, - ) - .into_response()) + let download_headers = DownloadHeaders::new(options, metadata.filename.as_deref()); + Ok((download_headers, response).into_response()) } async fn object_head( @@ -254,11 +254,8 @@ async fn object_head( let mut response = (StatusCode::OK, headers).into_response(); insert_accept_ranges(&mut response); - Ok(( - DownloadHeaders::new(options.disposition, metadata.filename.as_deref()), - response, - ) - .into_response()) + let download_headers = DownloadHeaders::new(options, metadata.filename.as_deref()); + Ok((download_headers, response).into_response()) } /// Inserts a `Content-Length` header covering the complete object. @@ -312,6 +309,61 @@ async fn delete_object( Ok(StatusCode::NO_CONTENT) } +/// Object presentation headers, including mandatory browser safeguards. +/// +/// All successful downloads get `nosniff` and `no-referrer`; inline responses also get a CSP +/// sandbox without relaxation tokens. Sandboxing is a browser safeguard, not content sanitization, +/// and does not prevent all external resource loads. Apply these parts only to successful responses. +#[derive(Debug)] +struct DownloadHeaders { + disposition: HeaderValue, + inline: bool, +} + +impl DownloadHeaders { + /// Builds success-only headers using the response options and stored filename. + fn new(options: ResponseOptions, filename: Option<&str>) -> Self { + let header = match filename { + Some(filename) => options.disposition.with_filename(filename), + None => HeaderValue::from_static(options.disposition.as_str()), + }; + + Self { + disposition: header, + inline: options.disposition == ContentDisposition::Inline, + } + } +} + +impl IntoResponseParts for DownloadHeaders { + type Error = Infallible; + + fn into_response_parts( + self, + mut response: ResponseParts, + ) -> Result { + let headers = response.headers_mut(); + headers.insert(header::CONTENT_DISPOSITION, self.disposition); + headers.insert( + header::X_CONTENT_TYPE_OPTIONS, + HeaderValue::from_static("nosniff"), + ); + // Inline documents can load external resources; avoid exposing URL query credentials + // through referrers as defense in depth alongside the sandbox. + headers.insert( + header::REFERRER_POLICY, + HeaderValue::from_static("no-referrer"), + ); + if self.inline { + headers.insert( + header::CONTENT_SECURITY_POLICY, + HeaderValue::from_static("sandbox"), + ); + } + Ok(response) + } +} + #[cfg(test)] mod tests { use std::sync::Arc; @@ -574,4 +626,20 @@ mod tests { assert!(matches!(error, ApiError::Service(_))); assert_eq!(error.status(), StatusCode::TOO_MANY_REQUESTS); } + + #[test] + fn inline_filename_retains_escaping_and_unicode_encoding() { + let options = ResponseOptions { + disposition: ContentDisposition::Inline, + }; + let download_headers = DownloadHeaders::new(options, Some("réport.pdf")); + let response = (download_headers, ()).into_response(); + assert_eq!( + response.headers()["content-disposition"], + "inline; filename=\"r_port.pdf\"; filename*=UTF-8''r%C3%A9port.pdf" + ); + assert_eq!(response.headers()["content-security-policy"], "sandbox"); + assert_eq!(response.headers()["x-content-type-options"], "nosniff"); + assert_eq!(response.headers()["referrer-policy"], "no-referrer"); + } } diff --git a/objectstore-server/src/extractors/response_options.rs b/objectstore-server/src/extractors/response_options.rs index 9dc91adc3..ba551cb34 100644 --- a/objectstore-server/src/extractors/response_options.rs +++ b/objectstore-server/src/extractors/response_options.rs @@ -16,8 +16,12 @@ //! Content type and content encoding always come from stored metadata. Content-language and //! expires overrides are not supported. No MIME allowlist is applied to inline responses. +use std::fmt::Write as _; + use axum::extract::FromRequestParts; +use http::HeaderValue; use http::request::Parts; +use objectstore_types::headers::ExtValue; use serde::Deserialize; use serde::de::value::{Error, MapDeserializer}; use url::form_urlencoded; @@ -43,6 +47,44 @@ impl ContentDisposition { Self::Inline => "inline", } } + + /// Formats the selected disposition with a stored filename. + /// + /// 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). + /// + /// 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*`. + pub fn with_filename(self, filename: &str) -> HeaderValue { + let all_dots = filename.chars().all(|c| c == '.'); + + let mut result = format!("{}; filename=\"", self.as_str()); + for c in filename.chars() { + let c = match c { + '/' | '\\' => '-', + '.' if all_dots => '-', + '"' => { + result.push('\\'); + '"' + } + c if !c.is_ascii() || c.is_control() => '_', + c => c, + }; + result.push(c); + } + result.push('"'); + + 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. + HeaderValue::from_str(&result).expect("content disposition is a valid header value") + } } /// Validated presentation options for successful object downloads. diff --git a/objectstore-server/tests/objects.rs b/objectstore-server/tests/objects.rs index 53503c581..c25b8000f 100644 --- a/objectstore-server/tests/objects.rs +++ b/objectstore-server/tests/objects.rs @@ -1,9 +1,11 @@ use std::time::Duration; use anyhow::Result; +use http::{Method, StatusCode}; use objectstore_server::config::{AuthZ, Config}; use objectstore_test::server::TestServer; use objectstore_types::metadata::{ExpirationPolicy, Metadata}; +use reqwest::Url; async fn test_server() -> TestServer { TestServer::with_config(Config { @@ -343,3 +345,146 @@ async fn filename_dot_and_dotdot_are_sanitized() -> Result<()> { Ok(()) } + +#[tokio::test] +async fn successful_downloads_apply_options_and_preserve_metadata() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + let path = "/v1/objects/test/org=1/options"; + client + .put(server.url(path)) + .header("content-type", "text/html") + .header("x-sn-filename", "report.html") + .body("example") + .send() + .await? + .error_for_status()?; + + for method in [Method::GET, Method::HEAD] { + for disposition in [None, Some("attachment"), Some("inline")] { + let mut url = Url::parse(&server.url(path))?; + if let Some(disposition) = disposition { + url.query_pairs_mut() + .append_pair("response-content-disposition", disposition); + } + let response = client.request(method.clone(), url).send().await?; + assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.headers()["content-type"], "text/html"); + assert_eq!(response.headers()["x-sn-filename"], "report.html"); + assert_eq!(response.headers()["content-length"], "20"); + assert_eq!( + response.headers()["content-disposition"], + format!( + "{}; filename=\"report.html\"", + disposition.unwrap_or("attachment") + ) + ); + assert_eq!(response.headers()["x-content-type-options"], "nosniff"); + assert_eq!(response.headers()["referrer-policy"], "no-referrer"); + assert_eq!( + response + .headers() + .get("content-security-policy") + .map(|v| v.to_str().unwrap()), + (disposition == Some("inline")).then_some("sandbox") + ); + let body = response.text().await?; + assert_eq!( + body, + if method == Method::HEAD { + "" + } else { + "example" + } + ); + } + } + Ok(()) +} + +#[tokio::test] +async fn ranged_and_error_responses() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + let path = "/v1/objects/test/org=1/range-options"; + client + .put(server.url(path)) + .body("abcdef") + .send() + .await? + .error_for_status()?; + let mut url = Url::parse(&server.url(path))?; + url.set_query(Some("response-content-disposition=inline")); + let response = client + .get(url.clone()) + .header("range", "bytes=1-3") + .send() + .await?; + assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); + assert_eq!(response.headers()["content-range"], "bytes 1-3/6"); + assert_eq!(response.headers()["content-length"], "3"); + assert_eq!(response.headers()["content-disposition"], "inline"); + assert_eq!(response.headers()["content-security-policy"], "sandbox"); + assert_eq!(response.headers()["referrer-policy"], "no-referrer"); + assert_eq!(response.text().await?, "bcd"); + + for (target, range, expected) in [ + (url.clone(), "bytes=99-", StatusCode::RANGE_NOT_SATISFIABLE), + ( + { + let mut missing = url.clone(); + missing.set_path("/v1/objects/test/org=1/missing-options"); + missing + }, + "bytes=0-", + StatusCode::NOT_FOUND, + ), + ] { + let response = client.get(target).header("range", range).send().await?; + assert_eq!(response.status(), expected); + for name in [ + "content-disposition", + "x-content-type-options", + "content-security-policy", + "referrer-policy", + ] { + assert!( + !response.headers().contains_key(name), + "unexpected {name} on {expected}" + ); + } + } + url.set_path("/v1/objects/test/org=1/missing-options"); + let response = client.head(url).send().await?; + assert_eq!(response.status(), StatusCode::NOT_FOUND); + for name in [ + "content-disposition", + "x-content-type-options", + "content-security-policy", + "referrer-policy", + ] { + assert!(!response.headers().contains_key(name)); + } + + Ok(()) +} + +#[tokio::test] +async fn invalid_and_duplicate_options_are_rejected() -> Result<()> { + let server = test_server().await; + let client = reqwest::Client::new(); + for query in [ + "response-content-disposition=other", + "response-content-disposition=inline%3Bfilename%3Dx", + "response-content-disposition=inline&RESPONSE-CONTENT-DISPOSITION=attachment", + ] { + let mut url = Url::parse(&server.url("/v1/objects/test/org=1/invalid-options"))?; + url.set_query(Some(query)); + assert_eq!( + client.get(url).send().await?.status(), + StatusCode::BAD_REQUEST, + "{query}" + ); + } + Ok(()) +} diff --git a/objectstore-server/tests/response_options.rs b/objectstore-server/tests/response_options.rs deleted file mode 100644 index 68e228938..000000000 --- a/objectstore-server/tests/response_options.rs +++ /dev/null @@ -1,157 +0,0 @@ -//! Response overrides on successful downloads, with error behavior left unchanged. - -use anyhow::Result; -use http::{Method, StatusCode}; -use objectstore_server::config::{AuthZ, Config}; -use objectstore_test::server::TestServer; -use reqwest::Url; - -async fn test_server() -> TestServer { - TestServer::with_config(Config { - auth: AuthZ { - enforce: false, - ..Default::default() - }, - ..Default::default() - }) - .await -} - -#[tokio::test] -async fn successful_downloads_apply_options_and_preserve_metadata() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - let path = "/v1/objects/test/org=1/options"; - client - .put(server.url(path)) - .header("content-type", "text/html") - .header("x-sn-filename", "report.html") - .body("example") - .send() - .await? - .error_for_status()?; - - for method in [Method::GET, Method::HEAD] { - for disposition in [None, Some("attachment"), Some("inline")] { - let mut url = Url::parse(&server.url(path))?; - if let Some(disposition) = disposition { - url.query_pairs_mut() - .append_pair("response-content-disposition", disposition); - } - let response = client.request(method.clone(), url).send().await?; - assert_eq!(response.status(), StatusCode::OK); - assert_eq!(response.headers()["content-type"], "text/html"); - assert_eq!(response.headers()["x-sn-filename"], "report.html"); - assert_eq!(response.headers()["content-length"], "20"); - assert_eq!( - response.headers()["content-disposition"], - format!( - "{}; filename=\"report.html\"", - disposition.unwrap_or("attachment") - ) - ); - assert_eq!(response.headers()["x-content-type-options"], "nosniff"); - assert_eq!( - response - .headers() - .get("content-security-policy") - .map(|v| v.to_str().unwrap()), - (disposition == Some("inline")).then_some("sandbox") - ); - let body = response.text().await?; - assert_eq!( - body, - if method == Method::HEAD { - "" - } else { - "example" - } - ); - } - } - Ok(()) -} - -#[tokio::test] -async fn ranged_and_error_responses() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - let path = "/v1/objects/test/org=1/range-options"; - client - .put(server.url(path)) - .body("abcdef") - .send() - .await? - .error_for_status()?; - let mut url = Url::parse(&server.url(path))?; - url.set_query(Some("response-content-disposition=inline")); - let response = client - .get(url.clone()) - .header("range", "bytes=1-3") - .send() - .await?; - assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); - assert_eq!(response.headers()["content-range"], "bytes 1-3/6"); - assert_eq!(response.headers()["content-length"], "3"); - assert_eq!(response.headers()["content-disposition"], "inline"); - assert_eq!(response.headers()["content-security-policy"], "sandbox"); - assert_eq!(response.text().await?, "bcd"); - - for (target, range, expected) in [ - (url.clone(), "bytes=99-", StatusCode::RANGE_NOT_SATISFIABLE), - ( - { - let mut missing = url.clone(); - missing.set_path("/v1/objects/test/org=1/missing-options"); - missing - }, - "bytes=0-", - StatusCode::NOT_FOUND, - ), - ] { - let response = client.get(target).header("range", range).send().await?; - assert_eq!(response.status(), expected); - for name in [ - "content-disposition", - "x-content-type-options", - "content-security-policy", - ] { - assert!( - !response.headers().contains_key(name), - "unexpected {name} on {expected}" - ); - } - } - url.set_path("/v1/objects/test/org=1/missing-options"); - let response = client.head(url).send().await?; - assert_eq!(response.status(), StatusCode::NOT_FOUND); - for name in [ - "content-disposition", - "x-content-type-options", - "content-security-policy", - ] { - assert!(!response.headers().contains_key(name)); - } - - Ok(()) -} - -#[tokio::test] -async fn invalid_and_duplicate_options_are_rejected() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - for query in [ - "response-content-disposition=other", - "response-content-disposition=inline%3Bfilename%3Dx", - "response-content-disposition=inline&RESPONSE-CONTENT-DISPOSITION=attachment", - ] { - let mut url = Url::parse(&server.url("/v1/objects/test/org=1/invalid-options"))?; - url.set_query(Some(query)); - assert_eq!( - client.get(url).send().await?.status(), - StatusCode::BAD_REQUEST, - "{query}" - ); - } - Ok(()) -} From 893bb47a092251f11230fbc5ad76c4c2f85b79ca Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Thu, 22 Oct 2026 13:02:03 +0200 Subject: [PATCH 6/6] Simplify response disposition query encoding --- .../python/src/objectstore_client/client.py | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/clients/python/src/objectstore_client/client.py b/clients/python/src/objectstore_client/client.py index 56de8276e..34b045305 100644 --- a/clients/python/src/objectstore_client/client.py +++ b/clients/python/src/objectstore_client/client.py @@ -10,7 +10,7 @@ from io import BytesIO from os import PathLike from typing import IO, Any, Literal, NamedTuple, cast -from urllib.parse import quote, urlencode, urlparse +from urllib.parse import urlparse import sentry_sdk import urllib3 @@ -53,17 +53,12 @@ def _response_options_query( - disposition: Literal["inline", "attachment"] | None, + disposition: Literal["inline", "attachment"], ) -> str: - """Encode individual response options without changing legacy URLs.""" - pairs: list[tuple[str, str]] = [] - if disposition is not None: - if disposition not in ("inline", "attachment"): - raise ValueError( - f"unsupported response content disposition {disposition!r}" - ) - pairs.append(("response-content-disposition", disposition)) - return urlencode(pairs, quote_via=quote) + """Validate and encode the response-content-disposition query parameter.""" + if disposition not in ("inline", "attachment"): + raise ValueError(f"unsupported response content disposition {disposition!r}") + return utils.encode_query(f"response-content-disposition={disposition}") class GetResponse(NamedTuple): @@ -750,9 +745,8 @@ def object_url( ``SecretKey`` is configured on this session, or if disposition is invalid. """ url = self._make_url(key, full=True) - query = _response_options_query(response_content_disposition) - if query: - url = f"{url}?{query}" + if response_content_disposition is not None: + url = f"{url}?{_response_options_query(response_content_disposition)}" if token_validity is None: return url @@ -762,7 +756,7 @@ def object_url( ) # A JWT's compact serialization is base64url, whose alphabet is URL-safe, # so it can go in the query string as-is with no further encoding. - separator = "&" if query else "?" + separator = "&" if response_content_disposition is not None else "?" return f"{url}{separator}{PARAM_AUTH}={token}" def presigned_object_url( @@ -830,9 +824,10 @@ def presigned_object_url( f"&{presign.PARAM_DURATION}={duration_secs}" ) - response_query = _response_options_query(response_content_disposition) - if response_query: - encoded_query = f"{encoded_query}&{response_query}" + if response_content_disposition is not None: + encoded_query += ( + f"&{_response_options_query(response_content_disposition)}" + ) canonical = presign.build_canonical_form( method, encoded_path, encoded_query