Skip to content

Commit 094f64c

Browse files
alukachclaude
andauthored
fix(core): byte-faithful, consistently validated object keys across all backend paths (#108)
* fix(core): percent-encode object keys in raw-signed backend URLs Multipart operations (CreateMultipartUpload, UploadPart, Complete, Abort) build their backend URL by splicing the decoded object key into a string. For key characters that url::Url leaves literal in paths but that are outside the RFC 3986 unreserved set (`=`, `!`, `(`, `)`, `:`, `@`, ...), the request went out — and was signed — with the literal byte, while S3/MinIO reconstruct the SigV4 canonical URI by strict-encoding the decoded path. The signatures never matched, so multipart uploads to Hive-style partition keys (`country_iso=ETH/...`) failed with 403 SignatureDoesNotMatch at CreateMultipartUpload. Encode the assembled prefix+key with the SigV4 strict set (unreserved chars, `/` kept as separator) before splicing. The URL string is both the signing input and the wire bytes, so the two stay byte-identical on any backend. Presigned CRUD ops already encode this way via object_store's STRICT_ENCODE_SET, which is why single PUT/GET on such keys worked. Also syncs Cargo.lock with the 0.6.3 release version bump. Reported downstream as source-cooperative/data.source.coop#180. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: state the encode-set rationale timelessly Comments described the pre-fix state ('already work', 'pre-existing'); reword to describe the invariant instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(core): build presigned object paths byte-faithfully Path::from percent-encodes characters object_store deems unsafe (*, %, ~, #, ...) into the logical path, so presigned CRUD silently renamed such objects on the backend (a*.bin stored as a%2A.bin) while the raw-signed multipart path stores the true key. Consequences: multipart- written keys were unreadable through GET, listings showed names that 404 on fetch, and 100%.txt / 100%25.txt could alias to one backend object — serving wrong content with a 200. Use Path::parse in build_object_path: byte-faithful, with object_store encoding the wire URL exactly once. Keys with empty or relative segments (a//b, a/../b), which Path::from silently collapsed to a different key, now return 400 InvalidRequest. Objects stored under mangled names by earlier versions keep their mangled backend names and must be addressed accordingly (or renamed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(core): cross-path key-encoding contract; encode keys in unsigned URLs (#109) Add a contract test asserting the three backend URL builders — the authenticated presigned signer, the anonymous UnsignedUrlSigner, and the raw-signed build_backend_url — emit byte-identical wire paths that percent-decode back to the logical key, over a corpus covering every character class that has diverged before (=, spaces, *, %, ~, #, unicode, literal %3D). If an object_store upgrade shifts its path encoding, this is the loud alarm. The test immediately caught a third instance of the #105 bug class: UnsignedUrlSigner spliced the raw key with no encoding, so anonymous URLs carried literal key bytes — a key holding %3D decodes to = on the backend, and a # truncates the path as a URL fragment. Encode with the same strict set as the signed builders. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * chore(core): apply review trims - drop dead `let key = key.as_str()` rebinding in UnsignedUrlSigner - decode one builder in the contract's decode test; byte-equality test pins the other builders to it - fix stale multipart-matrix comment claiming the presigned path still rewrites INVALID-set chars (this PR removes that behavior) - doc build_object_path's residual leading/trailing-slash stripping Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(core): validate object keys once for every keyed operation Reject keys with empty, `.`, or `..` path segments (including leading/trailing slashes) or ASCII control characters with 400 InvalidRequest at operation-parse time (build_s3_operation), for every keyed operation. The presigned path already rejected interior degenerate segments via Path::parse but silently stripped leading/trailing slashes (a DELETE of `dir/` deleted `dir`); the raw-signed multipart path accepted all of them — writing objects the presigned path can't address, breaking listings that cover them (object_store fails parsing listed keys with empty segments), and letting a literal `..` reach URL normalization, which on non-WHATWG-normalizing runtimes retargets the signed backend request across buckets. build_backend_url enforces the same rule as a backstop for hand-built operations. Batch-delete body keys stay exempt: they never enter a URL path, and permissiveness there is the remediation route for legacy degenerate keys. Documented in docs/reference/operations.md alongside the byte-faithfulness guarantee and the pre-0.6.4 mangled-name migration note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 5fd7c19 commit 094f64c

7 files changed

Lines changed: 375 additions & 16 deletions

File tree

crates/core/src/api/request.rs

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ pub fn build_s3_operation(
5757
key: String,
5858
query: Option<&str>,
5959
) -> Result<S3Operation, ProxyError> {
60+
validate_key(&key)?;
6061
let query_params = parse_query_params(query);
6162

6263
// Check for multipart upload query params
@@ -155,6 +156,35 @@ pub fn build_s3_operation(
155156
}
156157
}
157158

159+
/// Validate a client-visible object key before any backend path is built.
160+
///
161+
/// Every backend URL builder must agree on what a key addresses. The
162+
/// presigned path's `Path::parse` rejects empty and `.`/`..` segments but
163+
/// silently strips a leading/trailing `/`; the raw-signed path
164+
/// (`build_backend_url`) would accept all of them — writing objects the
165+
/// presigned path can't address, breaking listings (object_store fails to
166+
/// parse listed keys with empty segments), and letting a literal `..` reach
167+
/// URL normalization. Reject the whole class loudly, once, for every keyed
168+
/// operation. Real S3 accepts these keys; the proxy is deliberately
169+
/// stricter. Batch-delete body keys are deliberately exempt — they never
170+
/// enter a URL path, and permissiveness there is the remediation route for
171+
/// legacy degenerate keys already on a backend.
172+
pub fn validate_key(key: &str) -> Result<(), ProxyError> {
173+
if key.is_empty() {
174+
return Ok(()); // bucket-level operation
175+
}
176+
let degenerate_segment = key
177+
.split('/')
178+
.any(|seg| seg.is_empty() || seg == "." || seg == "..");
179+
if degenerate_segment || key.bytes().any(|b| b < 0x20 || b == 0x7f) {
180+
return Err(ProxyError::InvalidRequest(format!(
181+
"invalid object key {key:?}: empty, `.`, or `..` path segments, \
182+
leading/trailing slashes, and control characters are not allowed"
183+
)));
184+
}
185+
Ok(())
186+
}
187+
158188
#[derive(Debug, Clone)]
159189
pub enum HostStyle {
160190
/// Path-style: `/{bucket}/{key}`
@@ -241,4 +271,33 @@ mod tests {
241271
let op = parse(Method::PUT, "/b/k.txt", None, &http::HeaderMap::new()).unwrap();
242272
assert!(matches!(op, S3Operation::PutObject { .. }));
243273
}
274+
275+
#[test]
276+
fn degenerate_keys_are_rejected_for_every_keyed_operation() {
277+
let headers = http::HeaderMap::new();
278+
for key in [
279+
"a//b.txt",
280+
"a/./b.txt",
281+
"a/../b.txt",
282+
"/a.txt",
283+
"dir/",
284+
"a\nb",
285+
] {
286+
for (method, query) in [
287+
(Method::GET, None),
288+
(Method::PUT, None),
289+
(Method::DELETE, None),
290+
(Method::POST, Some("uploads")),
291+
] {
292+
let err = build_s3_operation(&method, "b".into(), key.into(), query).unwrap_err();
293+
assert!(
294+
matches!(err, ProxyError::InvalidRequest(_)),
295+
"{method} of key {key:?} must be InvalidRequest, got {err:?}"
296+
);
297+
}
298+
}
299+
// Bucket-level operations (empty key) are unaffected.
300+
assert!(parse(Method::GET, "/b", None, &headers).is_ok());
301+
assert!(parse(Method::POST, "/b", Some("delete"), &headers).is_ok());
302+
}
244303
}

crates/core/src/backend/multipart.rs

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,13 @@ use url::Url;
2222
/// spliced into the URL, or the backend signature won't match
2323
/// (`SignatureDoesNotMatch`). This is the same strict set `object_store`
2424
/// applies when building presigned URLs for the CRUD operations.
25-
const S3_PATH_ENCODE_SET: &percent_encoding::AsciiSet = &percent_encoding::NON_ALPHANUMERIC
26-
.remove(b'-')
27-
.remove(b'.')
28-
.remove(b'_')
29-
.remove(b'~')
30-
.remove(b'/');
25+
pub(crate) const S3_PATH_ENCODE_SET: &percent_encoding::AsciiSet =
26+
&percent_encoding::NON_ALPHANUMERIC
27+
.remove(b'-')
28+
.remove(b'.')
29+
.remove(b'_')
30+
.remove(b'~')
31+
.remove(b'/');
3132

3233
/// Build the backend URL for an S3 operation.
3334
///
@@ -50,6 +51,12 @@ pub fn build_backend_url(
5051
});
5152
}
5253

54+
// Backstop: keys are validated at operation-parse time
55+
// (`build_s3_operation`); enforce here too so a hand-built operation can
56+
// never splice a degenerate key (`a//b`, `..`) into a URL that
57+
// normalizes to a different — possibly cross-bucket — target.
58+
crate::api::request::validate_key(operation.key())?;
59+
5360
let mut key = String::new();
5461
if let Some(prefix) = &config.backend_prefix {
5562
key.push_str(prefix.trim_end_matches('/'));
@@ -158,6 +165,21 @@ mod tests {
158165
}
159166
}
160167

168+
#[test]
169+
fn degenerate_keys_are_rejected_before_url_build() {
170+
let config = test_bucket_config();
171+
for key in ["a//b.txt", "a/../b.txt"] {
172+
let op = S3Operation::CreateMultipartUpload {
173+
bucket: "test".into(),
174+
key: key.into(),
175+
};
176+
assert!(
177+
build_backend_url(&config, &op).is_err(),
178+
"hand-built op with key {key:?} must not reach the URL"
179+
);
180+
}
181+
}
182+
161183
#[test]
162184
fn upload_id_with_special_chars_is_encoded() {
163185
let config = test_bucket_config();

crates/core/src/backend/url_signer.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,16 @@ impl Signer for UnsignedUrlSigner {
8080
path: &object_store::path::Path,
8181
_expires_in: std::time::Duration,
8282
) -> object_store::Result<url::Url> {
83-
let key = path.as_ref();
83+
// Percent-encode the key with the same strict set as the signed
84+
// builders: `url::Url` leaves `%` (and `=`, `*`, ...) literal in
85+
// paths, so an unencoded splice puts raw key bytes on the wire and
86+
// the backend's decode addresses a different object (a key holding
87+
// a literal `%3D` decodes to `=`).
88+
let key = percent_encoding::utf8_percent_encode(
89+
path.as_ref(),
90+
super::multipart::S3_PATH_ENCODE_SET,
91+
)
92+
.to_string();
8493
let url_str = if self.bucket.is_empty() {
8594
if key.is_empty() {
8695
format!("{}/", self.endpoint)

crates/core/src/proxy.rs

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,7 +1059,7 @@ where
10591059
request_id: &str,
10601060
) -> Result<ForwardRequest, ProxyError> {
10611061
let signer = self.backend.create_signer(config)?;
1062-
let path = build_object_path(config, key);
1062+
let path = build_object_path(config, key)?;
10631063

10641064
let url = signer
10651065
.signed_url(method.clone(), &path, PRESIGNED_URL_TTL)
@@ -1545,8 +1545,27 @@ fn error_response(err: &ProxyError, resource: &str, request_id: &str, debug: boo
15451545
}
15461546

15471547
/// Build an object_store Path from a bucket config and client-visible key.
1548-
fn build_object_path(config: &BucketConfig, key: &str) -> object_store::path::Path {
1549-
object_store::path::Path::from(apply_backend_prefix(config, key))
1548+
///
1549+
/// Uses `Path::parse` (byte-faithful) rather than `Path::from`: `Path::from`
1550+
/// percent-encodes characters object_store considers unsafe (`*`, `%`, `~`,
1551+
/// `#`, ...) into the *logical* path, silently renaming the backend object
1552+
/// (`a*.bin` is stored as `a%2A.bin`) and splitting the key namespace from
1553+
/// the raw-signed multipart path, which stores keys byte-faithfully. With
1554+
/// `Path::parse` the raw path is the key itself, and object_store's URL
1555+
/// builder percent-encodes it exactly once at the wire boundary.
1556+
///
1557+
/// `Path::parse` rejects keys with empty (`a//b`) or relative (`.`, `..`)
1558+
/// segments; surface those as `InvalidRequest` (400) rather than silently
1559+
/// collapsing them to a different key as `Path::from` did. This is a
1560+
/// backstop: `validate_key` already rejects that class — plus leading and
1561+
/// trailing slashes, which `Path::parse` would silently strip — for every
1562+
/// keyed operation at parse time.
1563+
fn build_object_path(
1564+
config: &BucketConfig,
1565+
key: &str,
1566+
) -> Result<object_store::path::Path, ProxyError> {
1567+
object_store::path::Path::parse(apply_backend_prefix(config, key))
1568+
.map_err(|e| ProxyError::InvalidRequest(format!("invalid object key: {e}")))
15501569
}
15511570

15521571
/// Parse the declared `Content-Length` header as a byte count, if present and valid.
@@ -2668,4 +2687,30 @@ mod tests {
26682687
);
26692688
});
26702689
}
2690+
2691+
#[test]
2692+
fn object_path_is_byte_faithful() {
2693+
let config = test_bucket_config("test");
2694+
for key in ["report*.pdf", "100%.txt", "a~b#c.bin", "dir/%3D-lit.txt"] {
2695+
let path = build_object_path(&config, key).unwrap();
2696+
assert_eq!(path.as_ref(), key, "logical key must not be rewritten");
2697+
}
2698+
}
2699+
2700+
#[test]
2701+
fn object_path_applies_backend_prefix_byte_faithfully() {
2702+
let mut config = test_bucket_config("test");
2703+
config.backend_prefix = Some("data/".into());
2704+
let path = build_object_path(&config, "report*.pdf").unwrap();
2705+
assert_eq!(path.as_ref(), "data/report*.pdf");
2706+
}
2707+
2708+
#[test]
2709+
fn object_path_rejects_degenerate_segments() {
2710+
let config = test_bucket_config("test");
2711+
for key in ["a//b.txt", "a/./b.txt", "a/../b.txt"] {
2712+
let err = build_object_path(&config, key).unwrap_err();
2713+
assert_eq!(err.status_code(), 400, "key {key:?} must be a 400");
2714+
}
2715+
}
26712716
}
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
//! Cross-path key-encoding contract.
2+
//!
3+
//! The proxy builds backend request paths in three places: object_store
4+
//! presigned URLs (authenticated CRUD), `UnsignedUrlSigner` (anonymous
5+
//! CRUD), and `build_backend_url` (raw-signed multipart and batch delete).
6+
//! A backend percent-decodes each wire path to decide which object a
7+
//! request addresses, so every builder must emit a path that decodes to
8+
//! the same logical key — and the builders must agree byte-for-byte, or
9+
//! the same logical key addresses different backend objects depending on
10+
//! upload size, client payload mode, or bucket auth (see #105, #108).
11+
//!
12+
//! If an object_store upgrade changes its path encoding, these tests are
13+
//! the loud alarm.
14+
15+
use std::collections::HashMap;
16+
use std::time::Duration;
17+
18+
use multistore::backend::multipart::build_backend_url;
19+
use multistore::backend::url_signer::build_signer;
20+
use multistore::types::{BucketConfig, S3Operation};
21+
use object_store::path::Path;
22+
use percent_encoding::percent_decode_str;
23+
24+
/// Corpus of logical keys: the plain case, every character class the url
25+
/// crate leaves literal in paths, and the characters object_store's
26+
/// `Path::from` used to rewrite.
27+
const KEYS: &[&str] = &[
28+
"plain/file.bin",
29+
"by_country/country_iso=ETH/ETH.pmtiles",
30+
"spaces in/every segment.txt",
31+
"specials !('):@+,;$&.bin",
32+
"unicode/café/naïve.txt",
33+
"report*.pdf",
34+
"100%.txt",
35+
"tilde~hash#pipe|.bin",
36+
"brackets[1]{2}.bin",
37+
"literal-%3D-triplet.txt",
38+
];
39+
40+
fn bucket_config(with_creds: bool) -> BucketConfig {
41+
let mut backend_options: HashMap<String, String> = HashMap::new();
42+
backend_options.insert(
43+
"endpoint".into(),
44+
"https://s3.us-east-1.amazonaws.com".into(),
45+
);
46+
backend_options.insert("bucket_name".into(), "backend-bucket".into());
47+
backend_options.insert("region".into(), "us-east-1".into());
48+
if with_creds {
49+
backend_options.insert("access_key_id".into(), "AKIAIOSFODNN7EXAMPLE".into());
50+
backend_options.insert(
51+
"secret_access_key".into(),
52+
"wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY".into(),
53+
);
54+
}
55+
BucketConfig {
56+
name: "test".into(),
57+
backend_type: "s3".into(),
58+
backend_prefix: None,
59+
anonymous_access: !with_creds,
60+
allowed_roles: vec![],
61+
backend_options,
62+
}
63+
}
64+
65+
/// Wire path emitted by the presigned CRUD builder (object_store signer
66+
/// for authenticated buckets, `UnsignedUrlSigner` for anonymous ones).
67+
/// `Path::parse` mirrors `build_object_path`.
68+
fn presigned_path(config: &BucketConfig, key: &str) -> String {
69+
let signer = build_signer(config).unwrap();
70+
let path = Path::parse(key).unwrap();
71+
let url = futures::executor::block_on(signer.signed_url(
72+
http::Method::GET,
73+
&path,
74+
Duration::from_secs(60),
75+
))
76+
.unwrap();
77+
url.path().to_string()
78+
}
79+
80+
/// Wire path emitted by the raw-signed builder (multipart, batch delete).
81+
fn raw_signed_path(config: &BucketConfig, key: &str) -> String {
82+
let op = S3Operation::CreateMultipartUpload {
83+
bucket: "test".into(),
84+
key: key.into(),
85+
};
86+
let url = build_backend_url(config, &op).unwrap();
87+
url::Url::parse(&url).unwrap().path().to_string()
88+
}
89+
90+
/// The two SigV4 builders (authenticated presign, raw-signed) and the
91+
/// anonymous builder must produce byte-identical wire paths for the same
92+
/// logical key.
93+
#[test]
94+
fn all_builders_agree_byte_for_byte() {
95+
let authed = bucket_config(true);
96+
let anon = bucket_config(false);
97+
for key in KEYS {
98+
let presigned = presigned_path(&authed, key);
99+
let raw = raw_signed_path(&authed, key);
100+
let unsigned = presigned_path(&anon, key);
101+
assert_eq!(presigned, raw, "presigned vs raw-signed for key {key:?}");
102+
assert_eq!(
103+
presigned, unsigned,
104+
"presigned vs anonymous for key {key:?}"
105+
);
106+
}
107+
}
108+
109+
/// Every wire path must percent-decode back to exactly the logical key —
110+
/// that decode is what the backend uses to pick the object. One builder
111+
/// suffices: `all_builders_agree_byte_for_byte` pins the others to it.
112+
#[test]
113+
fn wire_paths_decode_to_the_logical_key() {
114+
let config = bucket_config(true);
115+
for key in KEYS {
116+
let path = presigned_path(&config, key);
117+
let decoded = percent_decode_str(&path).decode_utf8().unwrap();
118+
assert_eq!(decoded, format!("/backend-bucket/{key}"), "key {key:?}");
119+
}
120+
}

docs/reference/operations.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ Handled by OIDC discovery closures (registered on the `Router` via `OidcRouterEx
5555
> - **Server-side copy is not supported** — A `PUT` carrying `x-amz-copy-source` (CopyObject / UploadPartCopy) is rejected with `501 NotImplemented` rather than silently overwriting the destination.
5656
> - **`x-amz-*` write headers are dropped** — Object metadata, storage class, tagging, ACLs, SSE, and checksum headers on writes are not forwarded (see "Writes and request headers" above).
5757
> - **Versioned/MFA delete is not handled** — A `?versionId=` on a delete is ignored; the current object version is deleted.
58+
> - **Degenerate object keys are rejected** — Keys containing empty, `.`, or `..` path segments (including leading/trailing slashes, e.g. `dir/` folder markers), or ASCII control characters, return `400 InvalidRequest` on every keyed operation. Real S3 accepts such keys; the proxy is deliberately stricter because they cannot be addressed consistently across its presigned and raw-signed backend paths. Batch-delete body keys are exempt, as the remediation route for legacy keys already stored under such names.
59+
> - **Keys are otherwise byte-faithful** — All other keys (including `*`, `%`, `~`, `#`, unicode) are stored on the backend exactly as sent. Objects written through versions **before 0.6.4** via single presigned PUT with characters in object_store's rewrite set (`*`, `%`, `~`, `#`, `?`, `[`, `]`, ...) were silently stored under percent-encoded names (`a*.bin` as `a%2A.bin`); they remain addressable only by that literal mangled name and need a one-time rename to recover their logical keys.
5860
5961
### Upload size on the Cloudflare Workers runtime
6062

0 commit comments

Comments
 (0)