Validate registered identifiers

Author: marmeladema

engine/src/lib.rs

@@ -118,8 +118,8 @@ pub use self::rhs_types::{
 };
 pub use self::scheme::{
     Field, FieldIndex, FieldRedefinitionError, FieldRef, Function, FunctionRedefinitionError,
-    FunctionRef, IdentifierRedefinitionError, IndexAccessError, List, ListRef, Scheme,
-    SchemeBuilder, SchemeMismatchError, UnknownFieldError,
+    FunctionRef, IdentifierRedefinitionError, IdentifierRegistrationError, IndexAccessError, List,
+    ListRef, Scheme, SchemeBuilder, SchemeMismatchError, UnknownFieldError,
 };
 pub use self::types::{
     CompoundType, ExpectedType, ExpectedTypeList, GetType, LhsValue, LiteralSet, LiteralValue,

engine/src/scheme.rs

@@ -426,22 +426,8 @@ pub(crate) enum Identifier<'s> {
 }
 
 impl<'i, 's> LexWith<'i, &'s Scheme> for Identifier<'s> {
-    fn lex_with(mut input: &'i str, scheme: &'s Scheme) -> LexResult<'i, Self> {
-        let initial_input = input;
-
-        loop {
-            input = take_while(input, "identifier character", |c| {
-                c.is_ascii_alphanumeric() || c == '_'
-            })?
-            .1;
-
-            match expect(input, ".") {
-                Ok(rest) => input = rest,
-                Err(_) => break,
-            };
-        }
-
-        let name = span(initial_input, input);
+    fn lex_with(input: &'i str, scheme: &'s Scheme) -> LexResult<'i, Self> {
+        let (name, input) = lex_identifier_name(input)?;
 
         let field = scheme
             .get(name)
@@ -451,6 +437,25 @@ impl<'i, 's> LexWith<'i, &'s Scheme> for Identifier<'s> {
     }
 }
 
+#[inline]
+fn lex_identifier_name(mut input: &str) -> LexResult<'_, &str> {
+    let initial_input = input;
+
+    loop {
+        input = take_while(input, "identifier character", |c| {
+            c.is_ascii_alphanumeric() || c == '_'
+        })?
+        .1;
+
+        match expect(input, ".") {
+            Ok(rest) => input = rest,
+            Err(_) => break,
+        };
+    }
+
+    Ok((span(initial_input, input), input))
+}
+
 /// An error that occurs if an unregistered field name was queried from a
 /// [`Scheme`](struct@Scheme).
 #[derive(Debug, PartialEq, Eq, Error)]
@@ -485,6 +490,28 @@ pub enum IdentifierRedefinitionError {
     Function(#[source] FunctionRedefinitionError),
 }
 
+/// An error that occurs when registering a field or function identifier.
+#[derive(Debug, PartialEq, Eq, Error)]
+pub enum IdentifierRegistrationError {
+    /// The identifier does not follow the syntax accepted for registration.
+    #[error("invalid identifier {0}")]
+    InvalidIdentifier(String),
+
+    /// The identifier is already registered as a field or function.
+    #[error("{0}")]
+    Redefinition(#[from] IdentifierRedefinitionError),
+}
+
+fn is_valid_identifier(name: &str) -> bool {
+    matches!(lex_identifier_name(name), Ok((_, "")))
+        && matches!(name.as_bytes().first(), Some(b) if b.is_ascii_alphabetic() || *b == b'_')
+        && name.split('.').all(|segment| {
+            segment
+                .bytes()
+                .any(|b| b.is_ascii_alphabetic() || b == b'_')
+        })
+}
+
 #[derive(Clone, Copy, Debug)]
 enum SchemeItem {
     Field(usize),
@@ -647,16 +674,23 @@ impl SchemeBuilder {
         name: Arc<str>,
         ty: Type,
         optional: bool,
-    ) -> Result<(), IdentifierRedefinitionError> {
+    ) -> Result<(), IdentifierRegistrationError> {
+        if !is_valid_identifier(&name) {
+            return Err(IdentifierRegistrationError::InvalidIdentifier(
+                name.to_string(),
+            ));
+        }
+
         match self.items.entry(name) {
-            Entry::Occupied(entry) => match entry.get() {
-                SchemeItem::Field(_) => Err(IdentifierRedefinitionError::Field(
-                    FieldRedefinitionError(entry.key().to_string()),
+            Entry::Occupied(entry) => Err(match entry.get() {
+                SchemeItem::Field(_) => IdentifierRedefinitionError::Field(FieldRedefinitionError(
+                    entry.key().to_string(),
                 )),
-                SchemeItem::Function(_) => Err(IdentifierRedefinitionError::Function(
+                SchemeItem::Function(_) => IdentifierRedefinitionError::Function(
                     FunctionRedefinitionError(entry.key().to_string()),
-                )),
-            },
+                ),
+            }
+            .into()),
             Entry::Vacant(entry) => {
                 let index = self.fields.len();
                 self.fields.push(FieldDefinition {
@@ -675,7 +709,7 @@ impl SchemeBuilder {
         &mut self,
         name: N,
         ty: Type,
-    ) -> Result<(), IdentifierRedefinitionError> {
+    ) -> Result<(), IdentifierRegistrationError> {
         self.add_field_full(name.as_ref().into(), ty, false)
     }
 
@@ -684,7 +718,7 @@ impl SchemeBuilder {
         &mut self,
         name: N,
         ty: Type,
-    ) -> Result<(), IdentifierRedefinitionError> {
+    ) -> Result<(), IdentifierRegistrationError> {
         self.add_field_full(name.as_ref().into(), ty, true)
     }
 
@@ -693,16 +727,24 @@ impl SchemeBuilder {
         &mut self,
         name: N,
         function: impl FunctionDefinition + 'static,
-    ) -> Result<(), IdentifierRedefinitionError> {
-        match self.items.entry(name.as_ref().into()) {
-            Entry::Occupied(entry) => match entry.get() {
-                SchemeItem::Field(_) => Err(IdentifierRedefinitionError::Field(
-                    FieldRedefinitionError(entry.key().to_string()),
+    ) -> Result<(), IdentifierRegistrationError> {
+        let name = name.as_ref();
+        if !is_valid_identifier(name) {
+            return Err(IdentifierRegistrationError::InvalidIdentifier(
+                name.to_string(),
+            ));
+        }
+
+        match self.items.entry(name.into()) {
+            Entry::Occupied(entry) => Err(match entry.get() {
+                SchemeItem::Field(_) => IdentifierRedefinitionError::Field(FieldRedefinitionError(
+                    entry.key().to_string(),
                 )),
-                SchemeItem::Function(_) => Err(IdentifierRedefinitionError::Function(
+                SchemeItem::Function(_) => IdentifierRedefinitionError::Function(
                     FunctionRedefinitionError(entry.key().to_string()),
-                )),
-            },
+                ),
+            }
+            .into()),
             Entry::Vacant(entry) => {
                 let index = self.functions.len();
                 self.functions
@@ -1854,10 +1896,80 @@ fn test_field_type_override() {
 
     assert_eq!(
         builder.add_field("foo", Type::Bytes),
-        Err(IdentifierRedefinitionError::Field(FieldRedefinitionError(
-            "foo".into()
-        )))
+        Err(IdentifierRegistrationError::Redefinition(
+            IdentifierRedefinitionError::Field(FieldRedefinitionError("foo".into()))
+        ))
+    );
+}
+
+#[test]
+fn test_identifier_registration() {
+    use crate::ConcatFunction;
+
+    let mut builder = SchemeBuilder::new();
+    let valid_field_names = [
+        "_",
+        "field",
+        "Field_0",
+        "http._request",
+        "a.b2._c3",
+        "cf.response.1xxx_code",
+    ];
+    for name in valid_field_names {
+        assert_eq!(builder.add_field(name, Type::Bytes), Ok(()));
+    }
+    assert_eq!(
+        builder.add_optional_field("optional.field", Type::Int),
+        Ok(())
+    );
+    assert_eq!(
+        builder.add_function("function.name", ConcatFunction::new()),
+        Ok(())
+    );
+
+    for name in [
+        "",
+        ".field",
+        "field.",
+        "field..name",
+        "0field",
+        "123",
+        "192.0.2.1",
+        "a.1",
+        "a.123",
+        "a.1.2",
+        "f-ield",
+        "féield",
+    ] {
+        assert_eq!(
+            builder.add_field(name, Type::Bytes),
+            Err(IdentifierRegistrationError::InvalidIdentifier(name.into()))
+        );
+    }
+
+    assert_eq!(
+        builder.add_optional_field("optional.", Type::Int),
+        Err(IdentifierRegistrationError::InvalidIdentifier(
+            "optional.".into()
+        ))
+    );
+    assert_eq!(
+        builder.add_function("1function", ConcatFunction::new()),
+        Err(IdentifierRegistrationError::InvalidIdentifier(
+            "1function".into()
+        ))
     );
+
+    let scheme = builder.build();
+    for name in valid_field_names.into_iter().chain(["optional.field"]) {
+        let (field, rest) = FieldRef::lex_with(name, &scheme).unwrap();
+        assert_eq!(field.name(), name);
+        assert_eq!(rest, "");
+    }
+
+    let (function, rest) = FunctionRef::lex_with("function.name", &scheme).unwrap();
+    assert_eq!(function.name(), "function.name");
+    assert_eq!(rest, "");
 }
 
 #[test]
@@ -1920,6 +2032,17 @@ fn test_scheme_json_serialization() {
     assert_eq!(scheme.inner.fields, new_scheme.inner.fields);
 }
 
+#[test]
+fn test_scheme_json_rejects_invalid_identifier() {
+    let scheme = Scheme! { valid: Int }.build();
+    let json = serde_json::to_string(&scheme)
+        .unwrap()
+        .replace("\"valid\"", "\"1invalid\"");
+
+    let err = serde_json::from_str::<Scheme>(&json).unwrap_err();
+    assert!(err.to_string().contains("invalid identifier 1invalid"));
+}
+
 #[test]
 fn test_nil_not_equal_behavior_true() {
     use crate::{Array, ExecutionContext, Map};

ffi/src/lib.rs

@@ -888,6 +888,19 @@ mod ffi_test {
         wirefilter_build_scheme(builder)
     }
 
+    #[test]
+    fn reject_invalid_field_identifier() {
+        let mut builder = wirefilter_create_scheme_builder();
+        let name = "2http.host";
+
+        assert!(!wirefilter_add_type_field_to_scheme(
+            &mut builder,
+            name.as_ptr().cast(),
+            name.len(),
+            Type::Bytes.into(),
+        ));
+    }
+
     fn create_execution_context<'e, 's: 'e>(scheme: &'s Scheme) -> Box<ExecutionContext<'e>> {
         let mut exec_context = wirefilter_create_execution_context(scheme);
         let invalid_key = &b"\xc3\x28"[..];