Merge pull request #7621 from influxdata/idpe-17434/namespace-validation-alignment

chore(idpe-17434): remove utf8-percent encoding from onWrite path

data_types/src/namespace_name.rs

@@ -1,6 +1,5 @@
 use std::{borrow::Cow, ops::RangeInclusive};
 
-use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
 use thiserror::Error;
 
 /// Length constraints for a [`NamespaceName`] name.
@@ -8,6 +7,13 @@ use thiserror::Error;
 /// A `RangeInclusive` is a closed interval, covering [1, 64]
 const LENGTH_CONSTRAINT: RangeInclusive<usize> = 1..=64;
 
+/// Allowlist of chars for a [`NamespaceName`] name.
+///
+/// '/' | '_' | '-' are utilized by the platforms.
+fn is_allowed(c: char) -> bool {
+    c.is_alphanumeric() || matches!(c, '/' | '_' | '-')
+}
+
 /// Errors returned when attempting to construct a [`NamespaceName`] from an org
 /// & bucket string pair.
 #[derive(Debug, Error)]
@@ -41,7 +47,7 @@ pub enum NamespaceNameError {
     /// The provided namespace name contains an unacceptable character.
     #[error(
         "namespace name '{}' contains invalid character, character number {} \
-        is a control which is not allowed",
+        is not whitelisted",
         name,
         bad_char_offset
     )]
@@ -92,7 +98,7 @@ impl<'a> NamespaceName<'a> {
         //
         // NOTE: If changing these characters, please update the error message
         // above.
-        if let Some(bad_char_offset) = name.chars().position(|c| c.is_control()) {
+        if let Some(bad_char_offset) = name.chars().position(|c| !is_allowed(c)) {
             return Err(NamespaceNameError::BadChars {
                 bad_char_offset,
                 name: name.to_string(),
@@ -123,11 +129,7 @@ impl<'a> NamespaceName<'a> {
             return Err(OrgBucketMappingError::NoOrgBucketSpecified);
         }
 
-        let prefix: Cow<'_, str> = utf8_percent_encode(org, NON_ALPHANUMERIC).into();
-        let suffix: Cow<'_, str> = utf8_percent_encode(bucket, NON_ALPHANUMERIC).into();
-
-        let db_name = format!("{}_{}", prefix, suffix);
-        Ok(Self::new(db_name)?)
+        Ok(Self::new(format!("{}_{}", org, bucket))?)
     }
 }
 
@@ -188,34 +190,46 @@ mod tests {
     #[test]
     fn test_org_bucket_map_db_contains_underscore() {
         let got = NamespaceName::from_org_and_bucket("my_org", "bucket").unwrap();
-        assert_eq!(got.as_str(), "my%5Forg_bucket");
+        assert_eq!(got.as_str(), "my_org_bucket");
 
         let got = NamespaceName::from_org_and_bucket("org", "my_bucket").unwrap();
-        assert_eq!(got.as_str(), "org_my%5Fbucket");
+        assert_eq!(got.as_str(), "org_my_bucket");
 
         let got = NamespaceName::from_org_and_bucket("org", "my__bucket").unwrap();
-        assert_eq!(got.as_str(), "org_my%5F%5Fbucket");
+        assert_eq!(got.as_str(), "org_my__bucket");
 
         let got = NamespaceName::from_org_and_bucket("my_org", "my_bucket").unwrap();
-        assert_eq!(got.as_str(), "my%5Forg_my%5Fbucket");
+        assert_eq!(got.as_str(), "my_org_my_bucket");
     }
 
     #[test]
     fn test_org_bucket_map_db_contains_underscore_and_percent() {
-        let got = NamespaceName::from_org_and_bucket("my%5Forg", "bucket").unwrap();
-        assert_eq!(got.as_str(), "my%255Forg_bucket");
+        let err = NamespaceName::from_org_and_bucket("my%5Forg", "bucket");
+        assert!(matches!(
+            err,
+            Err(OrgBucketMappingError::InvalidNamespaceName { .. })
+        ));
 
-        let got = NamespaceName::from_org_and_bucket("my%5Forg_", "bucket").unwrap();
-        assert_eq!(got.as_str(), "my%255Forg%5F_bucket");
+        let err = NamespaceName::from_org_and_bucket("my%5Forg_", "bucket");
+        assert!(matches!(
+            err,
+            Err(OrgBucketMappingError::InvalidNamespaceName { .. })
+        ));
     }
 
     #[test]
-    fn test_bad_namespace_name_is_encoded() {
-        let got = NamespaceName::from_org_and_bucket("org", "bucket?").unwrap();
-        assert_eq!(got.as_str(), "org_bucket%3F");
+    fn test_bad_namespace_name_fails_validation() {
+        let err = NamespaceName::from_org_and_bucket("org", "bucket?");
+        assert!(matches!(
+            err,
+            Err(OrgBucketMappingError::InvalidNamespaceName { .. })
+        ));
 
-        let got = NamespaceName::from_org_and_bucket("org!", "bucket").unwrap();
-        assert_eq!(got.as_str(), "org%21_bucket");
+        let err = NamespaceName::from_org_and_bucket("org!", "bucket");
+        assert!(matches!(
+            err,
+            Err(OrgBucketMappingError::InvalidNamespaceName { .. })
+        ));
     }
 
     #[test]
@@ -256,30 +270,50 @@ mod tests {
     #[test]
     fn test_bad_chars_null() {
         let got = NamespaceName::new("example\x00").unwrap_err();
-        assert_eq!(got.to_string() , "namespace name 'example\x00' contains invalid character, character number 7 is a control which is not allowed");
+        assert_eq!(got.to_string() , "namespace name 'example\x00' contains invalid character, character number 7 is not whitelisted");
     }
 
     #[test]
     fn test_bad_chars_high_control() {
         let got = NamespaceName::new("\u{007f}example").unwrap_err();
-        assert_eq!(got.to_string() , "namespace name '\u{007f}example' contains invalid character, character number 0 is a control which is not allowed");
+        assert_eq!(got.to_string() , "namespace name '\u{007f}example' contains invalid character, character number 0 is not whitelisted");
     }
 
     #[test]
     fn test_bad_chars_tab() {
         let got = NamespaceName::new("example\tdb").unwrap_err();
-        assert_eq!(got.to_string() , "namespace name 'example\tdb' contains invalid character, character number 7 is a control which is not allowed");
+        assert_eq!(got.to_string() , "namespace name 'example\tdb' contains invalid character, character number 7 is not whitelisted");
     }
 
     #[test]
     fn test_bad_chars_newline() {
         let got = NamespaceName::new("my_example\ndb").unwrap_err();
-        assert_eq!(got.to_string() , "namespace name 'my_example\ndb' contains invalid character, character number 10 is a control which is not allowed");
+        assert_eq!(got.to_string() , "namespace name 'my_example\ndb' contains invalid character, character number 10 is not whitelisted");
+    }
+
+    #[test]
+    fn test_bad_chars_whitespace() {
+        let got = NamespaceName::new("my_example db").unwrap_err();
+        assert_eq!(got.to_string() , "namespace name 'my_example db' contains invalid character, character number 10 is not whitelisted");
+    }
+
+    #[test]
+    fn test_bad_chars_single_quote() {
+        let got = NamespaceName::new("my_example'db").unwrap_err();
+        assert_eq!(got.to_string() , "namespace name 'my_example\'db' contains invalid character, character number 10 is not whitelisted");
     }
 
     #[test]
     fn test_ok_chars() {
-        let db = NamespaceName::new("my-example-db_with_underscores and spaces").unwrap();
-        assert_eq!(&*db, "my-example-db_with_underscores and spaces");
+        let db =
+            NamespaceName::new("my-example-db_with_underscores/and/fwd/slash/AndCaseSensitive")
+                .unwrap();
+        assert_eq!(
+            &*db,
+            "my-example-db_with_underscores/and/fwd/slash/AndCaseSensitive"
+        );
+
+        let db = NamespaceName::new("a_ã_京").unwrap();
+        assert_eq!(&*db, "a_ã_京");
     }
 }

router/src/server/http/write/multi_tenant.rs

@@ -78,6 +78,7 @@ fn parse_v2(req: &Request<Body>) -> Result<WriteParams, MultiTenantExtractError>
 #[cfg(test)]
 mod tests {
     use assert_matches::assert_matches;
+    use data_types::NamespaceNameError;
 
     use super::*;
     use crate::server::http::write::Precision;
@@ -171,7 +172,7 @@ mod tests {
             namespace,
             ..
         }) => {
-            assert_eq!(namespace.as_str(), "cool%5Fconfusing_bucket");
+            assert_eq!(namespace.as_str(), "cool_confusing_bucket");
         }
     );
 
@@ -189,12 +190,11 @@ mod tests {
     test_parse_v2!(
         encoded_quotation,
         query_string = "?org=cool'confusing&bucket=bucket",
-        want = Ok(WriteParams {
-            namespace,
-            ..
-        }) => {
-            assert_eq!(namespace.as_str(), "cool%27confusing_bucket");
-        }
+        want = Err(Error::MultiTenantError(
+            MultiTenantExtractError::InvalidOrgAndBucket(
+                OrgBucketMappingError::InvalidNamespaceName(NamespaceNameError::BadChars { .. })
+            )
+        ))
     );
 
     test_parse_v2!(
@@ -204,7 +204,7 @@ mod tests {
             namespace,
             ..
         }) => {
-            assert_eq!(namespace.as_str(), "%5Fcoolconfusing_bucket");
+            assert_eq!(namespace.as_str(), "_coolconfusing_bucket");
         }
     );
 

router/src/server/http/write/single_tenant/mod.rs

@@ -381,9 +381,9 @@ mod tests {
     test_parse_v1!(
         encoded_quotation,
         query_string = "?db=ban'anas",
-        want = Ok(WriteParams{ namespace, precision: _ }) => {
-            assert_eq!(namespace.as_str(), "ban'anas");
-        }
+        want = Err(Error::SingleTenantError(
+            SingleTenantExtractError::InvalidNamespace(NamespaceNameError::BadChars { .. })
+        ))
     );
 
     test_parse_v1!(
@@ -483,17 +483,24 @@ mod tests {
         ))
     );
 
-    // Do not encode potentially problematic input.
     test_parse_v2!(
-        no_encoding,
+        url_encoding,
         // URL-encoded input that is decoded in the HTTP layer
-        query_string = "?bucket=cool%2Fconfusing%F0%9F%8D%8C&prg=org",
+        query_string = "?bucket=cool%2Fconfusing&prg=org",
         want = Ok(WriteParams {namespace, ..}) => {
             // Yielding a not-encoded string as the namespace.
-            assert_eq!(namespace.as_str(), "cool/confusing🍌");
+            assert_eq!(namespace.as_str(), "cool/confusing");
         }
     );
 
+    test_parse_v2!(
+        encoded_emoji,
+        query_string = "?bucket=confusing%F0%9F%8D%8C&prg=org",
+        want = Err(Error::SingleTenantError(
+            SingleTenantExtractError::InvalidNamespace(NamespaceNameError::BadChars { .. })
+        ))
+    );
+
     test_parse_v2!(
         org_ignored,
         query_string = "?org=wat&bucket=bananas",
@@ -518,14 +525,11 @@ mod tests {
     );
 
     test_parse_v2!(
-        encoded_quotation,
+        single_quotation,
         query_string = "?bucket=buc'ket",
-        want = Ok(WriteParams {
-            namespace,
-            ..
-        }) => {
-            assert_eq!(namespace.as_str(), "buc'ket");
-        }
+        want = Err(Error::SingleTenantError(
+            SingleTenantExtractError::InvalidNamespace(NamespaceNameError::BadChars { .. })
+        ))
     );
 
     test_parse_v2!(

service_grpc_namespace/src/lib.rs

@@ -633,23 +633,26 @@ mod tests {
 
     test_create_namespace_name!(ok, name = "bananas", want = Ok("bananas"));
 
-    test_create_namespace_name!(multi_byte, name = "🍌", want = Ok("🍌"));
+    test_create_namespace_name!(multi_byte, name = "🍌", want = Err(e) => {
+        assert_eq!(e.code(), Code::InvalidArgument);
+        assert_eq!(e.message(), "namespace name '🍌' contains invalid character, character number 0 is not whitelisted");
+    });
 
     test_create_namespace_name!(
         tab,
         name = "it\tis\ttabtasitc",
         want = Err(e) => {
             assert_eq!(e.code(), Code::InvalidArgument);
-            assert_eq!(e.message(), "namespace name 'it\tis\ttabtasitc' contains invalid character, character number 2 is a control which is not allowed");
+            assert_eq!(e.message(), "namespace name 'it\tis\ttabtasitc' contains invalid character, character number 2 is not whitelisted");
         }
     );
 
     test_create_namespace_name!(
         null,
-        name = "bad \0 bananas",
+        name = "bad\0bananas",
         want = Err(e) => {
             assert_eq!(e.code(), Code::InvalidArgument);
-            assert_eq!(e.message(), "namespace name 'bad \0 bananas' contains invalid character, character number 4 is a control which is not allowed");
+            assert_eq!(e.message(), "namespace name 'bad\0bananas' contains invalid character, character number 3 is not whitelisted");
         }
     );