From 6696703343fd18df4884f5c6a61346673e613d20 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= <anzhengchao@gmail.com>
Date: Fri, 3 Apr 2026 11:30:56 +0800
Subject: [PATCH] test: cover delete-group percent decoding (#2373)

---
 rustfs/src/admin/handlers/group.rs | 109 +++++++++++++++++++++++------
 1 file changed, 88 insertions(+), 21 deletions(-)

diff --git a/rustfs/src/admin/handlers/group.rs b/rustfs/src/admin/handlers/group.rs
index 5db394645..b99977e6e 100644
--- a/rustfs/src/admin/handlers/group.rs
+++ b/rustfs/src/admin/handlers/group.rs
@@ -207,30 +207,11 @@ impl Operation for DeleteGroup {
         )
         .await?;
 
-        let group_raw = params
-            .get("group")
-            .ok_or_else(|| s3_error!(InvalidArgument, "missing group name in request"))?
-            .trim();
-
-        // Path segments stay percent-encoded in `req.uri.path()` / matchit; IAM uses decoded names (same as GET query).
-        let group_decoded = percent_decode_str(group_raw)
-            .decode_utf8()
-            .map_err(|_| s3_error!(InvalidArgument, "invalid group name encoding"))?;
-        let group = group_decoded.trim();
-
-        // Validate the group name format
-        if group.is_empty() || group.len() > 256 {
-            return Err(s3_error!(InvalidArgument, "invalid group name"));
-        }
-
-        // Sanity check the group name
-        if group.contains(['/', '\\', '\0']) {
-            return Err(s3_error!(InvalidArgument, "group name contains invalid characters"));
-        }
+        let group = decode_delete_group_name(&params)?;
 
         let Ok(iam_store) = rustfs_iam::get() else { return Err(s3_error!(InternalError, "iam not init")) };
 
-        let updated_at = iam_store.remove_users_from_group(group, vec![]).await.map_err(|e| {
+        let updated_at = iam_store.remove_users_from_group(&group, vec![]).await.map_err(|e| {
             warn!("delete group failed, e: {:?}", e);
             match e {
                 rustfs_iam::error::Error::GroupNotEmpty => {
@@ -276,6 +257,33 @@ impl Operation for DeleteGroup {
     }
 }
 
+fn decode_delete_group_name<'a>(params: &'a Params<'_, '_>) -> S3Result<std::borrow::Cow<'a, str>> {
+    let group_raw = params
+        .get("group")
+        .ok_or_else(|| s3_error!(InvalidArgument, "missing group name in request"))?
+        .trim();
+
+    // Path segments stay percent-encoded in `req.uri.path()` / matchit; IAM uses decoded names (same as GET query).
+    let decoded = percent_decode_str(group_raw)
+        .decode_utf8()
+        .map_err(|_| s3_error!(InvalidArgument, "invalid group name encoding"))?;
+    let group = decoded.trim();
+
+    if group.is_empty() || group.len() > 256 {
+        return Err(s3_error!(InvalidArgument, "invalid group name"));
+    }
+
+    if group.contains(['/', '\\', '\0']) {
+        return Err(s3_error!(InvalidArgument, "group name contains invalid characters"));
+    }
+
+    if group.len() == decoded.len() {
+        Ok(decoded)
+    } else {
+        Ok(std::borrow::Cow::Owned(group.to_string()))
+    }
+}
+
 pub struct SetGroupStatus {}
 #[async_trait::async_trait]
 impl Operation for SetGroupStatus {
@@ -484,3 +492,62 @@ impl Operation for UpdateGroupMembers {
         Ok(S3Response::with_headers((StatusCode::OK, Body::empty()), header))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use matchit::Router;
+
+    fn with_delete_group_params<T>(path: &str, f: impl FnOnce(&Params<'_, '_>) -> T) -> T {
+        let mut router = Router::new();
+        router
+            .insert("/rustfs/admin/v3/group/{group}", ())
+            .expect("route should insert");
+
+        let matched = router.at(path).expect("route should match");
+        f(&matched.params)
+    }
+
+    #[test]
+    fn decode_delete_group_name_percent_decodes_path_segment() {
+        let group = with_delete_group_params("/rustfs/admin/v3/group/dev%2Bops%20team", |params| {
+            decode_delete_group_name(params).map(|group| group.into_owned())
+        })
+        .expect("encoded group name should decode");
+
+        assert_eq!(group, "dev+ops team");
+    }
+
+    #[test]
+    fn decode_delete_group_name_rejects_invalid_utf8() {
+        let err = with_delete_group_params("/rustfs/admin/v3/group/%FF", |params| {
+            decode_delete_group_name(params).map(|group| group.into_owned())
+        })
+        .expect_err("invalid utf-8 should fail");
+
+        assert_eq!(err.code(), &S3ErrorCode::InvalidArgument);
+        assert_eq!(err.message(), Some("invalid group name encoding"));
+    }
+
+    #[test]
+    fn decode_delete_group_name_rejects_blank_name_after_decoding() {
+        let err = with_delete_group_params("/rustfs/admin/v3/group/%20", |params| {
+            decode_delete_group_name(params).map(|group| group.into_owned())
+        })
+        .expect_err("blank group should fail");
+
+        assert_eq!(err.code(), &S3ErrorCode::InvalidArgument);
+        assert_eq!(err.message(), Some("invalid group name"));
+    }
+
+    #[test]
+    fn decode_delete_group_name_rejects_path_separator_after_decoding() {
+        let err = with_delete_group_params("/rustfs/admin/v3/group/team%2Fops", |params| {
+            decode_delete_group_name(params).map(|group| group.into_owned())
+        })
+        .expect_err("decoded slash should fail");
+
+        assert_eq!(err.code(), &S3ErrorCode::InvalidArgument);
+        assert_eq!(err.message(), Some("group name contains invalid characters"));
+    }
+}