From c60196921ef3f56546c343e8418832ac2261e311 Mon Sep 17 00:00:00 2001
From: John Kastner <jkastner@amazon.com>
Date: Fri, 22 Nov 2024 22:20:53 +0000
Subject: [PATCH] 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 <jkastner@amazon.com>
---
 cedar-policy-validator/src/diagnostics.rs     |  2 +
 .../src/diagnostics/validation_warnings.rs    |  4 +-
 cedar-policy-validator/src/str_checks.rs      | 62 ++++++++++++++++---
 3 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/cedar-policy-validator/src/diagnostics.rs b/cedar-policy-validator/src/diagnostics.rs
index f79ae2eb9..4ee0fd200 100644
--- 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
index f8a2e8c60..77f7604b8 100644
--- a/cedar-policy-validator/src/diagnostics/validation_warnings.rs
+++ b/cedar-policy-validator/src/diagnostics/validation_warnings.rs
@@ -100,7 +100,7 @@ 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>,
@@ -108,6 +108,8 @@ pub struct ConfusableIdentifier {
     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
index 203a151a6..1fd9cf570 100644
--- 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"), "is​Admin") {
-            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"),
+                "example_user+1@example.com"
+            ),
+            None
+        );
+
+        assert_matches!(permissable_ident(None, &PolicyID::from_string("0"), "is​Admin"), Some(warning) => {
+            expect_err(
+                "",
+                &miette::Report::new(warning),
+                &ExpectedErrorMessageBuilder::error(r#"for policy `0`, identifier `is​Admin` 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]