mirror of
https://github.com/deuxfleurs-org/garage.git
synced 2026-09-04 19:25:42 +00:00
Path-style URLs of the form /bucket//key address an object whose key begins with "/". Two greedy uses of `trim_start_matches('/')` were collapsing these leading slashes away:
- `uri.path().trim_start_matches('/')` stripped all leading slashes from the raw path before any further parsing.
- `p.trim_start_matches('/')` stripped leading slashes from the remainder after `split_once('/')` had already consumed the bucket/key separator
The combined effect wath that `HEAD /bucket//` and `GET /bucket//` produced an empty key, which the router treated as bucket-level operations (HeadBucket -> 200 OK, and ListObjectsV2) instead of an object-level op (HeadObject/GetObject -> 404 NoSuchKey).
The fix is simple: Replace the first `trim_start_matches` with `strip_prefix` (to strip exactly one separator slash) and remove the second one entirely. Path-style and vhost-style requests are now consistent: a double slash in the URL correctly addresses a key whose name begins with "/".
Regression tests added for `HEAD //` and `GET //` requests in both request styles.
Fixes: #1464
---
Disclaimer: I'm not fluent in Rust and I did use an LLM to explain the code to me. All code was written by me.
I'm not sure whether the large `test_cases!` block in the `test_aws_doc_examples` function is the right place for my tests (it certainly was a convenient one).
Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/1465
This commit is contained in:
+42
-2
@@ -315,7 +315,11 @@ impl Endpoint {
|
|||||||
bucket: Option<String>,
|
bucket: Option<String>,
|
||||||
) -> Result<(Self, Option<String>), Error> {
|
) -> Result<(Self, Option<String>), Error> {
|
||||||
let uri = req.uri();
|
let uri = req.uri();
|
||||||
let path = uri.path().trim_start_matches('/');
|
let path = uri.path().strip_prefix('/');
|
||||||
|
if path.is_none() {
|
||||||
|
return Err(Error::bad_request("URI path must start with a '/'"));
|
||||||
|
}
|
||||||
|
let path = path.unwrap();
|
||||||
let query = uri.query();
|
let query = uri.query();
|
||||||
if bucket.is_none() && path.is_empty() {
|
if bucket.is_none() && path.is_empty() {
|
||||||
if *req.method() == Method::OPTIONS {
|
if *req.method() == Method::OPTIONS {
|
||||||
@@ -329,7 +333,7 @@ impl Endpoint {
|
|||||||
(bucket, path)
|
(bucket, path)
|
||||||
} else {
|
} else {
|
||||||
path.split_once('/')
|
path.split_once('/')
|
||||||
.map(|(b, p)| (b.to_owned(), p.trim_start_matches('/')))
|
.map(|(b, p)| (b.to_owned(), p))
|
||||||
.unwrap_or_else(|| (path.to_owned(), ""))
|
.unwrap_or_else(|| (path.to_owned(), ""))
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -843,6 +847,40 @@ mod tests {
|
|||||||
"&+?%é/something"
|
"&+?%é/something"
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// A double-slash in the URL means the key begins with '/'.
|
||||||
|
// path-style: HEAD /bucket// → key "/"
|
||||||
|
assert_eq!(
|
||||||
|
parse("HEAD", "/my_bucket//", None, None)
|
||||||
|
.0
|
||||||
|
.get_key()
|
||||||
|
.unwrap(),
|
||||||
|
"/"
|
||||||
|
);
|
||||||
|
// virtual-hosted-style: HEAD // → key "/"
|
||||||
|
assert_eq!(
|
||||||
|
parse("HEAD", "//", Some("my_bucket".to_owned()), None)
|
||||||
|
.0
|
||||||
|
.get_key()
|
||||||
|
.unwrap(),
|
||||||
|
"/"
|
||||||
|
);
|
||||||
|
// same for GET: path-style GET /bucket// → key "/"
|
||||||
|
assert_eq!(
|
||||||
|
parse("GET", "/my_bucket//", None, None)
|
||||||
|
.0
|
||||||
|
.get_key()
|
||||||
|
.unwrap(),
|
||||||
|
"/"
|
||||||
|
);
|
||||||
|
// virtual-hosted-style: GET // → key "/"
|
||||||
|
assert_eq!(
|
||||||
|
parse("GET", "//", Some("my_bucket".to_owned()), None)
|
||||||
|
.0
|
||||||
|
.get_key()
|
||||||
|
.unwrap(),
|
||||||
|
"/"
|
||||||
|
);
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* this case is failing. We should verify how clients encode space in url
|
* this case is failing. We should verify how clients encode space in url
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -933,6 +971,7 @@ mod tests {
|
|||||||
GET "/{Key+}?torrent" => GetObjectTorrent
|
GET "/{Key+}?torrent" => GetObjectTorrent
|
||||||
GET "/?publicAccessBlock" => GetPublicAccessBlock
|
GET "/?publicAccessBlock" => GetPublicAccessBlock
|
||||||
HEAD "/" => HeadBucket
|
HEAD "/" => HeadBucket
|
||||||
|
HEAD "//" => HeadObject
|
||||||
HEAD "/my-image.jpg" => HeadObject
|
HEAD "/my-image.jpg" => HeadObject
|
||||||
HEAD "/my-image.jpg?versionId=3HL4kqCxf3vjVBH40Nrjfkd" => HeadObject
|
HEAD "/my-image.jpg?versionId=3HL4kqCxf3vjVBH40Nrjfkd" => HeadObject
|
||||||
HEAD "/Key+?partNumber=3&versionId=VersionId" => HeadObject
|
HEAD "/Key+?partNumber=3&versionId=VersionId" => HeadObject
|
||||||
@@ -949,6 +988,7 @@ mod tests {
|
|||||||
GET "/?uploads&delimiter=/&prefix=photos/2006/" => ListMultipartUploads
|
GET "/?uploads&delimiter=/&prefix=photos/2006/" => ListMultipartUploads
|
||||||
GET "/?uploads&delimiter=D&encoding-type=EncodingType&key-marker=KeyMarker&max-uploads=1&prefix=Prefix&upload-id-marker=UploadIdMarker" => ListMultipartUploads
|
GET "/?uploads&delimiter=D&encoding-type=EncodingType&key-marker=KeyMarker&max-uploads=1&prefix=Prefix&upload-id-marker=UploadIdMarker" => ListMultipartUploads
|
||||||
GET "/" => ListObjects
|
GET "/" => ListObjects
|
||||||
|
GET "//" => GetObject
|
||||||
GET "/?prefix=N&marker=Need&max-keys=40" => ListObjects
|
GET "/?prefix=N&marker=Need&max-keys=40" => ListObjects
|
||||||
GET "/?delimiter=/" => ListObjects
|
GET "/?delimiter=/" => ListObjects
|
||||||
GET "/?prefix=photos/2006/&delimiter=/" => ListObjects
|
GET "/?prefix=photos/2006/&delimiter=/" => ListObjects
|
||||||
|
|||||||
Reference in New Issue
Block a user