Don't warn on graphic ascii idents

This still warns on ascii control characters and any non-ascii characters we previosly warned about. Signed-off-by: John Kastner <[email protected]>
cedar-policy · Nov 22, 2024 · c601969 · c601969
1 parent 3166f9b
commit c601969
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 10 deletions.
diff --git a/cedar-policy-validator/src/diagnostics.rs b/cedar-policy-validator/src/diagnostics.rs
@@ -487,11 +487,13 @@ impl ValidationWarning {
         source_loc: Option<Loc>,
         policy_id: PolicyID,
         id: impl Into<String>,
+        confusable_character: char,
     ) -> Self {
         validation_warnings::ConfusableIdentifier {
             source_loc,
             policy_id,
             id: id.into(),
+            confusable_character,
         }
         .into()
     }

diff --git a/cedar-policy-validator/src/diagnostics/validation_warnings.rs b/cedar-policy-validator/src/diagnostics/validation_warnings.rs
@@ -100,14 +100,16 @@ impl Diagnostic for MixedScriptIdentifier {
 
 /// Warning for identifiers containing confusable characters
 #[derive(Debug, Clone, PartialEq, Error, Eq, Hash)]
-#[error("for policy `{policy_id}`, identifier `{id}` contains characters that fall outside of the General Security Profile for Identifiers")]
+#[error("for policy `{policy_id}`, identifier `{id}` contains the character `{}` which falls outside of the General Security Profile for Identifiers", .confusable_character.escape_debug())]
 pub struct ConfusableIdentifier {
     /// Source location
     pub source_loc: Option<Loc>,
     /// Policy ID where the warning occurred
     pub policy_id: PolicyID,
     /// Identifier containing confusable characters
     pub id: String,
+    /// The specific character we're not happy about
+    pub confusable_character: char,
 }
 
 impl Diagnostic for ConfusableIdentifier {

diff --git a/cedar-policy-validator/src/str_checks.rs b/cedar-policy-validator/src/str_checks.rs
@@ -78,11 +78,15 @@ fn permissable_ident(
             policy_id.clone(),
             s,
         ))
-    } else if !s.chars().all(|c| c.identifier_allowed()) {
+    } else if let Some(c) = s
+        .chars()
+        .find(|c| !c.is_ascii_graphic() && !c.identifier_allowed())
+    {
         Some(ValidationWarning::confusable_identifier(
             loc.cloned(),
             policy_id.clone(),
             s,
+            c,
         ))
     } else if !s.is_single_script() {
         Some(ValidationWarning::mixed_script_identifier(
@@ -119,7 +123,9 @@ mod test {
     use cedar_policy_core::{
         ast::PolicySet,
         parser::{parse_policy, Loc},
+        test_utils::{expect_err, ExpectedErrorMessageBuilder},
     };
+    use cool_asserts::assert_matches;
     use std::sync::Arc;
     #[test]
     fn strs() {
@@ -148,14 +154,52 @@ mod test {
             permissable_ident(None, &PolicyID::from_string("0"), "test"),
             None
         );
-        match permissable_ident(None, &PolicyID::from_string("0"), "isAdmin") {
-            Some(ValidationWarning::ConfusableIdentifier(_)) => (),
-            o => panic!("should have produced ConfusableIdentifier: {:?}", o),
-        };
-        match permissable_ident(None, &PolicyID::from_string("0"), "say_һello") {
-            Some(ValidationWarning::MixedScriptIdentifier(_)) => (),
-            o => panic!("should have produced MixedScriptIdentifier: {:?}", o),
-        };
+        assert_eq!(
+            permissable_ident(
+                None,
+                &PolicyID::from_string("0"),
+                "https://www.example.com/test?foo=bar&bar=baz#buz"
+            ),
+            None
+        );
+        assert_eq!(
+            permissable_ident(
+                None,
+                &PolicyID::from_string("0"),
+                "http://example.com/query{firstName}-{lastName}"
+            ),
+            None
+        );
+        assert_eq!(
+            permissable_ident(
+                None,
+                &PolicyID::from_string("0"),
+                "[email protected]"
+            ),
+            None
+        );
+
+        assert_matches!(permissable_ident(None, &PolicyID::from_string("0"), "isAdmin"), Some(warning) => {
+            expect_err(
+                "",
+                &miette::Report::new(warning),
+                &ExpectedErrorMessageBuilder::error(r#"for policy `0`, identifier `isAdmin` contains the character `\u{200b}` which falls outside of the General Security Profile for Identifiers"#)
+                    .build());
+        });
+        assert_matches!(permissable_ident(None, &PolicyID::from_string("0"), "🍌"), Some(warning) => {
+            expect_err(
+                "",
+                &miette::Report::new(warning),
+                &ExpectedErrorMessageBuilder::error(r#"for policy `0`, identifier `🍌` contains the character `🍌` which falls outside of the General Security Profile for Identifiers"#)
+                    .build());
+        });
+        assert_matches!(permissable_ident(None, &PolicyID::from_string("0"), "say_һello") , Some(warning) => {
+            expect_err(
+                "",
+                &miette::Report::new(warning),
+                &ExpectedErrorMessageBuilder::error(r#"for policy `0`, identifier `say_һello` contains mixed scripts"#)
+                    .build());
+        });
     }
 
     #[test]