From f56ed918b0c1c2ef9ea7eccb97d236d34a97c223 Mon Sep 17 00:00:00 2001
From: Gabriela Moldovan <gabi@torproject.org>
Date: Mon, 19 Jun 2023 13:13:39 +0100
Subject: [PATCH] keymgr: Validate ArtiPath and ArtiPathComponent.

---
 crates/tor-keymgr/src/key_specifier.rs | 57 ++++++++++++++++++++------
 1 file changed, 45 insertions(+), 12 deletions(-)

diff --git a/crates/tor-keymgr/src/key_specifier.rs b/crates/tor-keymgr/src/key_specifier.rs
index 1ac36293f..2084f107a 100644
--- a/crates/tor-keymgr/src/key_specifier.rs
+++ b/crates/tor-keymgr/src/key_specifier.rs
@@ -1,6 +1,7 @@
 //! The [`KeySpecifier`] trait and its implementations.
 
 use crate::{KeystoreError, Result};
+use std::path;
 use tor_error::HasKind;
 
 /// The path of a key in the Arti key store.
@@ -32,13 +33,38 @@ impl KeystoreError for InvalidArtiPathError {}
 impl ArtiPath {
     /// Create a new [`ArtiPath`].
     ///
-    /// This function returns an error if the specified string is not a valid Arti path.
-    // TODO hs: restrict the character set and syntax for values of this type (it should not be
-    // possible to construct an ArtiPath out of a String that uses disallowed chars, or one that is in
-    // the wrong format (TBD exactly what this format is supposed to look like)
-    #[allow(clippy::unnecessary_wraps)] // TODO hs: remove
+    /// An `ArtiPath` may only consist of UTF-8 alphanumeric, dash (`-`), underscore (`_`), and
+    /// path separator characters.
+    ///
+    /// The specified string is normalized by replacing any consecutive occurrences of the path
+    /// separator character with a single path separator.
+    ///
+    /// This function returns an error if `inner` contains any disallowed characters, or if it
+    /// consists solely of path sepatators.
     pub fn new(inner: String) -> Result<Self> {
-        Ok(Self(inner))
+        let is_allowed =
+            |c: char| ArtiPathComponent::is_allowed_char(c) || c == path::MAIN_SEPARATOR;
+
+        if inner.chars().any(|c| !is_allowed(c)) {
+            Err(Box::new(InvalidArtiPathError(inner)))
+        } else {
+            Self::normalize_string(&inner).map(Self)
+        }
+    }
+
+    /// Remove all but the first of consecutive [MAIN_SEPARATOR](path::MAIN_SEPARATOR) elements
+    /// from `s`.
+    ///
+    /// This function returns an error if `s` consists solely of path sepatators.
+    fn normalize_string(s: &str) -> Result<String> {
+        if s.chars().all(|c| c == path::MAIN_SEPARATOR) {
+            return Err(Box::new(InvalidArtiPathError(s.into())));
+        }
+
+        let mut chars = s.chars().collect::<Vec<_>>();
+        chars.dedup_by(|a, b| *a == path::MAIN_SEPARATOR && *b == path::MAIN_SEPARATOR);
+
+        Ok(chars.into_iter().collect::<String>())
     }
 }
 
@@ -53,14 +79,21 @@ pub struct ArtiPathComponent(String);
 impl ArtiPathComponent {
     /// Create a new [`ArtiPathComponent`].
     ///
-    /// This function returns an error if the specified string contains any disallowed characters.
+    /// An `ArtiPathComponent` may only consist of UTF-8 alphanumeric, dash
+    /// (`-`), and underscore (`_`) characters.
     ///
-    /// TODO hs: restrict the character set and syntax for values of this type (it should not be
-    /// possible to construct an ArtiPathComponent out of a String that uses disallowed chars, or
-    /// one that is in the wrong format (TBD exactly what this format is supposed to look like)
-    #[allow(clippy::unnecessary_wraps)] // TODO hs: remove
+    /// This function returns an error if `inner` contains any disallowed characters.
     pub fn new(inner: String) -> Result<Self> {
-        Ok(Self(inner))
+        if inner.chars().any(|c| !Self::is_allowed_char(c)) {
+            Err(Box::new(InvalidArtiPathError(inner)))
+        } else {
+            Ok(Self(inner))
+        }
+    }
+
+    /// Check whether `c` can be used within an `ArtiPathComponent`.
+    fn is_allowed_char(c: char) -> bool {
+        c.is_alphanumeric() || c == '_' || c == '-'
     }
 }