s3: fix DeleteObjects XML parsing with pretty-printed bodies (#1374)

## Summary

This PR fixes S3 `DeleteObjects` XML parsing when the request body is pretty-printed (contains indentation/newlines as whitespace text nodes).

Although PR #1324 already tried to address this, parsing could still fail with:

`InvalidRequest: Bad request: Invalid delete XML query`

because non-element nodes were validated but not actually skipped in the parsing loop.

## What changed

- In `src/api/s3/delete.rs`:
  - Properly skip non-element whitespace text nodes while iterating over `<Delete>` children.
  - Keep rejecting non-whitespace stray text content.
  - Parse the root `<Delete>` element more robustly by selecting the first element child.

## Tests added

New unit tests in `src/api/s3/delete.rs`:

- `parse_delete_objects_xml_with_formatting`
  - pretty-printed valid XML is accepted.
- `parse_delete_objects_xml_accepts_compact_valid_xml`
  - compact valid XML is accepted.
- `parse_delete_objects_xml_rejects_non_whitespace_text_node`
  - compact XML with stray text is rejected.
- `parse_delete_objects_xml_rejects_pretty_print_with_stray_text`
  - pretty-printed XML with stray text is rejected.

## Validation

Executed:

```bash
cargo test -p garage_api_s3 parse_delete_objects_xml -- --nocapture
```

Result: all parser tests pass.
Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/1374
Co-authored-by: milouz1985 <francois.hoyez@gmail.com>
Co-committed-by: milouz1985 <francois.hoyez@gmail.com>
This commit is contained in:
milouz1985
2026-03-15 10:40:50 +00:00
committed by Alex
parent 76592723de
commit 836657565e
+59 -2
View File
@@ -125,19 +125,20 @@ fn parse_delete_objects_xml(xml: &roxmltree::Document) -> Option<DeleteRequest>
};
let root = xml.root();
let delete = root.first_child()?;
let delete = root.children().find(|n| n.is_element())?;
if !delete.has_tag_name("Delete") {
return None;
}
for item in delete.children() {
// Only parse <Part> nodes
// Skip text nodes introduced by formatted XML.
if !item.is_element() {
// text nodes are allowed only if they contain whitespace characters only
if !item.text()?.trim().is_empty() {
return None;
}
continue;
}
if item.has_tag_name("Object") {
@@ -155,3 +156,59 @@ fn parse_delete_objects_xml(xml: &roxmltree::Document) -> Option<DeleteRequest>
Some(ret)
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn parse_delete_objects_xml_with_formatting() {
let body = r#"
<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<Object>
<Key>1_746573745f66696c65</Key>
</Object>
<Quiet>true</Quiet>
</Delete>
"#;
let xml = roxmltree::Document::parse(body).expect("valid delete XML");
let req = parse_delete_objects_xml(&xml).expect("request should be parsed");
assert_eq!(req.objects.len(), 1);
assert_eq!(req.objects[0].key, "1_746573745f66696c65");
assert!(req.quiet);
}
#[test]
fn parse_delete_objects_xml_rejects_non_whitespace_text_node() {
let body = r#"<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/">oops<Object><Key>1_746573745f66696c65</Key></Object></Delete>"#;
let xml = roxmltree::Document::parse(body).expect("valid XML");
let req = parse_delete_objects_xml(&xml);
assert!(req.is_none());
}
#[test]
fn parse_delete_objects_xml_rejects_pretty_print_with_stray_text() {
let body = r#"
<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
oops
<Object>
<Key>1_746573745f66696c65</Key>
</Object>
</Delete>
"#;
let xml = roxmltree::Document::parse(body).expect("valid XML");
let req = parse_delete_objects_xml(&xml);
assert!(req.is_none());
}
#[test]
fn parse_delete_objects_xml_accepts_compact_valid_xml() {
let body = r#"<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Object><Key>1_746573745f66696c65</Key></Object><Quiet>false</Quiet></Delete>"#;
let xml = roxmltree::Document::parse(body).expect("valid XML");
let req = parse_delete_objects_xml(&xml).expect("request should be parsed");
assert_eq!(req.objects.len(), 1);
assert_eq!(req.objects[0].key, "1_746573745f66696c65");
assert!(!req.quiet);
}
}