From 8ca6267a78c738a736a9310bb6f22cf8ed4d1d56 Mon Sep 17 00:00:00 2001 From: Denis Cornehl Date: Mon, 31 Aug 2026 11:27:44 +0200 Subject: [PATCH] storage: fix s3 batch delete with unicode filenames --- Cargo.lock | 1 + Cargo.toml | 9 +-- crates/lib/docs_rs_storage/Cargo.toml | 1 + crates/lib/docs_rs_storage/src/backends/s3.rs | 58 ++++++++++++++++++- .../src/storage/non_blocking.rs | 3 +- crates/lib/docs_rs_uri/Cargo.toml | 2 +- 6 files changed, 65 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 48b171579a..19856a00e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2406,6 +2406,7 @@ dependencies = [ "mime", "moka", "opentelemetry", + "percent-encoding", "rand 0.10.2", "sqlx", "strum", diff --git a/Cargo.toml b/Cargo.toml index 1684d19f5f..71360dc485 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,5 @@ [workspace] -resolver = "2" +resolver = "2" members = ["crates/bin/*", "crates/lib/*", "integration_tests"] @@ -38,7 +38,7 @@ chrono = { version = "0.4.11", default-features = false, features = ["clock", "s clap = { version = "4.0.22", features = ["derive"] } futures-util = "0.3.5" http = "1.0.0" -itertools = "0.15.0" +itertools = "0.15.0" mime = "0.3.16" mockito = "1.0.2" num_cpus = "1.15.0" @@ -46,6 +46,7 @@ opentelemetry = "0.32.0" opentelemetry-otlp = { version = "0.32.0", features = ["grpc-tonic", "metrics"] } opentelemetry-resource-detectors = "0.11.0" opentelemetry_sdk = { version = "0.32.0", features = ["rt-tokio"] } +percent-encoding = "2.2.0" postcard = { version = "1.1.3", default-features = false, features = ["use-std"] } pretty_assertions = "1.4.0" rand = "0.10" @@ -75,7 +76,7 @@ dbg_macro = "warn" todo = "warn" [profile.dev] -# recommendation coming from +# recommendation coming from # https://doc.rust-lang.org/nightly/cargo/guide/build-performance.html#reduce-amount-of-generated-debug-information # for our normal dev work, line-tables are good enough to see line numbers in backtraces. debug = "line-tables-only" @@ -84,7 +85,7 @@ debug = "line-tables-only" split-debuginfo = "unpacked" [profile.dev.build-override] -# optimize proc macros & build scripts, make them execute faster +# optimize proc macros & build scripts, make them execute faster # https://corrode.dev/blog/tips-for-faster-rust-compile-times/#avoid-procedural-macro-crates opt-level = 3 diff --git a/crates/lib/docs_rs_storage/Cargo.toml b/crates/lib/docs_rs_storage/Cargo.toml index 1980060474..7d13458e6a 100644 --- a/crates/lib/docs_rs_storage/Cargo.toml +++ b/crates/lib/docs_rs_storage/Cargo.toml @@ -44,6 +44,7 @@ itertools = { workspace = true } mime = { workspace = true } moka = { version = "0.12.14", features = ["future"] } opentelemetry = { workspace = true } +percent-encoding = { workspace = true } rand = { workspace = true, optional = true } sqlx = { workspace = true } # for sqlite strum = { workspace = true } diff --git a/crates/lib/docs_rs_storage/src/backends/s3.rs b/crates/lib/docs_rs_storage/src/backends/s3.rs index 132147b2f3..652308df07 100644 --- a/crates/lib/docs_rs_storage/src/backends/s3.rs +++ b/crates/lib/docs_rs_storage/src/backends/s3.rs @@ -16,7 +16,10 @@ use aws_sdk_s3::{ config::{Region, retry::RetryConfig}, error::{ProvideErrorMetadata, SdkError}, primitives::{ByteStream, Length}, - types::{ChecksumAlgorithm, CompletedMultipartUpload, CompletedPart, Delete, ObjectIdentifier}, + types::{ + ChecksumAlgorithm, CompletedMultipartUpload, CompletedPart, Delete, EncodingType, + ObjectIdentifier, + }, }; use aws_smithy_types_convert::date_time::DateTimeExt; use base64::{Engine as _, engine::general_purpose::STANDARD as b64}; @@ -26,6 +29,7 @@ use docs_rs_types::CompressionAlgorithm; use docs_rs_utils::{retry_backoff, spawn_blocking}; use futures_util::stream::{self, BoxStream, StreamExt, TryStreamExt}; use mime::Mime; +use percent_encoding::percent_decode_str; use std::path::Path; use tokio::{fs, time}; use tracing::{error, warn}; @@ -530,6 +534,10 @@ impl StorageBackendMethods for S3Backend { .list_objects_v2() .bucket(&self.bucket) .prefix(prefix) + // Without URL encoding, object keys containing characters + // forbidden by XML 1.0 cannot be represented faithfully in + // the ListObjects response. + .encoding_type(EncodingType::Url) .set_continuation_token(continuation_token) .send() .await?; @@ -537,7 +545,10 @@ impl StorageBackendMethods for S3Backend { if let Some(contents) = list.contents { for obj in contents { if let Some(key) = obj.key() { - yield key.to_owned(); + yield percent_decode_str(key) + .decode_utf8() + .with_context(|| format!("S3 returned a non-UTF-8 object key: {key:?}"))? + .into_owned(); } } } @@ -562,7 +573,27 @@ impl StorageBackendMethods for S3Backend { impl S3Backend { async fn delete_batch_with_retry(&self, keys: Vec) -> Result<(), Error> { - let mut remaining = keys; + let (mut remaining, xml_incompatible_keys): (Vec<_>, Vec<_>) = + keys.into_iter().partition(|key| is_xml_1_0_compatible(key)); + + // DeleteObjects puts keys in an XML 1.0 request body. S3 object keys can + // contain characters which XML 1.0 cannot represent (for example ESC), + // causing the whole request to fail with MalformedXML. Delete those keys + // individually instead, since DeleteObject encodes the key in the URI. + for key in xml_incompatible_keys { + self.client + .delete_object() + .bucket(&self.bucket) + .key(&key) + .send() + .await + .with_context(|| format!("failed to delete file from s3: {key:?}"))?; + } + + if remaining.is_empty() { + return Ok(()); + } + for attempt in 1.. { // Request-level failures are retried by the AWS SDK. We only retry // per-object failures returned in a successful `DeleteObjects` @@ -642,3 +673,24 @@ impl S3Backend { .context("could not build delete request") } } + +/// Whether a string can be represented as character data in an XML 1.0 document. +fn is_xml_1_0_compatible(value: &str) -> bool { + value + .chars() + .all(|c| matches!(c, '\u{9}' | '\u{a}' | '\u{d}' | '\u{20}'..='\u{d7ff}' | '\u{e000}'..='\u{fffd}' | '\u{10000}'..='\u{10ffff}')) +} + +#[cfg(test)] +mod tests { + use super::is_xml_1_0_compatible; + + #[test] + fn detects_xml_1_0_incompatible_characters() { + assert!(is_xml_1_0_compatible("sources/crate/1.0.0/lib.rs")); + assert!(is_xml_1_0_compatible("tabs\tand\nnewlines\r")); + assert!(is_xml_1_0_compatible("unicode-é-🦀")); + assert!(!is_xml_1_0_compatible("null-\0")); + assert!(!is_xml_1_0_compatible("escape-\u{1b}")); + } +} diff --git a/crates/lib/docs_rs_storage/src/storage/non_blocking.rs b/crates/lib/docs_rs_storage/src/storage/non_blocking.rs index cbd8fe172a..a2828cc58f 100644 --- a/crates/lib/docs_rs_storage/src/storage/non_blocking.rs +++ b/crates/lib/docs_rs_storage/src/storage/non_blocking.rs @@ -1000,10 +1000,11 @@ mod backend_tests { "foo/bar.txt", "foo/bar/baz.txt", "foo/bar/foobar.txt", + "foo/bar/\u{1b}", "bar.txt", ], &["foo.txt", "foo/bar.txt", "bar.txt"], - &["foo/bar/baz.txt", "foo/bar/foobar.txt"], + &["foo/bar/baz.txt", "foo/bar/foobar.txt", "foo/bar/\u{1b}"], ) .await } diff --git a/crates/lib/docs_rs_uri/Cargo.toml b/crates/lib/docs_rs_uri/Cargo.toml index 3d80f2f9c7..41167f7d5c 100644 --- a/crates/lib/docs_rs_uri/Cargo.toml +++ b/crates/lib/docs_rs_uri/Cargo.toml @@ -8,7 +8,7 @@ edition = "2024" [dependencies] askama = { workspace = true } http = { workspace = true } -percent-encoding = "2.2.0" +percent-encoding = { workspace = true } serde_with = { workspace = true } thiserror = { workspace = true } url = { workspace = true }