core: Remove unsafe methods when entropy feature is enabled

CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - \[all\] Code words with a weak password section or a non-integer nameplate will throw an error in the long. This error can be upgraded to a hard error by enabling the "entropy" feature. This feature will become the default in the next major release.
 - \[lib\] Implemented FromStr for `Code` and `Nameplate`
 - \[lib\] Added new checked type for the `Password` section of a wormhole code
+- \[lib\] Added new `entropy` feature. When enabled, the entropy of the passed password will be checked on creation. This will change the signature of `MailboxConnection::create_with_password` to require the password to be passed via the new `Password` wrapper type.
 - \[lib\]\[deprecated\] Deprecated the `Code` and `Nameplate` `From<impl Into<String>>` implementations and `new()` methods. They are unchecked and will print a warning for now. These will be removed in the next breaking release.
 
 ## [0.7.1] - 2024-07-25

cli/src/main.rs

@@ -608,27 +608,37 @@ async fn parse_and_connect(
     // Split the nameplate parsing from the code parsing to ensure we allow non-integer nameplates
     // until the next breaking release
     let res: Option<Result<magic_wormhole::Code, _>> = code.as_ref().map(|c| c.parse());
-    let code: Option<magic_wormhole::Code> =
+    let code: Option<magic_wormhole::Code> = match res {
+        Some(Ok(code)) => Some(code),
+        // Check if an interactive terminal is connected
+        Some(Err(
+            err @ (ParseCodeError::SeparatorMissing
+            | ParseCodeError::Password(ParsePasswordError::TooShort { .. })),
+        )) => {
+            // Only fail for the case where the password is < 4 characters.
+            // Anything else will just print an error for now.
+            return Err(err.into());
+        },
+        // If we have an interactive terminal connected, also fail for low entropy
+        Some(Err(err @ ParseCodeError::Password(ParsePasswordError::LittleEntropy { .. })))
+            if std::io::stdin().is_terminal() =>
         {
-            match res {
-                Some(Ok(code)) => Some(code),
-                // Check if an interactive terminal is connected
-                Some(Err(err @ ParseCodeError::Password(ParsePasswordError::TooShort { .. }))) => {
-                    // Only fail for the case where the password is < 4 characters.
-                    // Anything else will just print an error for now.
-                    return Err(err.into());
-                },
-                // If we have an interactive terminal connected, also fail for low entropy
-                Some(Err(
-                    err @ ParseCodeError::Password(ParsePasswordError::LittleEntropy { .. }),
-                )) if std::io::stdin().is_terminal() => return Err(err.into()),
-                Some(Err(_)) => {
-                    // The library crate already emits an error log for this.
-                    code.map(|c| c.into())
-                },
-                None => None,
-            }
-        };
+            return Err(err.into())
+        },
+        Some(Err(err)) => {
+            tracing::error!("{} This will fail in the next release.", err);
+            code.map(|c| {
+                let (nameplate, password) = c.split_once("-").unwrap();
+                unsafe {
+                    magic_wormhole::Code::from_components(
+                        magic_wormhole::Nameplate::new_unchecked(nameplate),
+                        magic_wormhole::Password::new_unchecked(password),
+                    )
+                }
+            })
+        },
+        None => None,
+    };
 
     /* We need to track that information for when we generate a QR code */
     let mut uri_rendezvous = None;

src/core.rs

@@ -162,9 +162,12 @@ impl<V: serde::Serialize + Send + Sync + 'static> MailboxConnection<V> {
     /// TODO: Replace this with create_with_validated_password
     pub async fn create_with_password(
         config: AppConfig<V>,
-        password: &str,
+        #[cfg(not(feature = "entropy"))] password: &str,
+        #[cfg(feature = "entropy")] password: Password,
     ) -> Result<Self, WormholeError> {
+        #[cfg(not(feature = "entropy"))]
         let password = password.parse().map_err(ParseCodeError::from)?;
+
         Self::create_with_validated_password(config, password).await
     }
 
@@ -790,10 +793,19 @@ pub struct ParseNameplateError {}
 #[serde(transparent)]
 #[deref(forward)]
 #[display("{}", _0)]
+#[cfg(not(feature = "entropy"))]
 pub struct Nameplate(
     #[deprecated(since = "0.7.0", note = "use the AsRef<str> implementation")] pub String,
 );
 
+/// Wormhole codes look like 4-purple-sausages, consisting of a number followed by some random words.
+/// This number is called a "Nameplate".
+#[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize, derive_more::Display)]
+#[serde(transparent)]
+#[display("{}", _0)]
+#[cfg(feature = "entropy")]
+pub struct Nameplate(String);
+
 #[allow(deprecated)]
 impl Nameplate {
     /// Create a new nameplate from a string.
@@ -803,6 +815,7 @@ impl Nameplate {
         since = "0.7.2",
         note = "Nameplates will be required to be numbers soon. Use the [std::str::FromStr] implementation"
     )]
+    #[cfg(not(feature = "entropy"))]
     pub fn new(n: impl Into<String>) -> Self {
         let nameplate = n.into();
         if let Err(err) = Nameplate::from_str(&nameplate) {
@@ -815,8 +828,12 @@ impl Nameplate {
         }
     }
 
+    /// Create a new nameplate from a string.
+    ///
+    /// Safety: Nameplate will be [required to be numbers](https://github.com/magic-wormhole/magic-wormhole-mailbox-server/issues/45) soon.
     #[allow(unsafe_code)]
-    unsafe fn new_unchecked(n: impl Into<String>) -> Self {
+    #[doc(hidden)]
+    pub unsafe fn new_unchecked(n: &str) -> Self {
         Nameplate(n.into())
     }
 }
@@ -845,6 +862,7 @@ impl From<Nameplate> for String {
 ///
 /// Does not check if the nameplate is a number. This may be incompatible.
 #[allow(deprecated)]
+#[cfg(not(feature = "entropy"))]
 impl From<String> for Nameplate {
     fn from(value: String) -> Self {
         tracing::debug!(
@@ -930,9 +948,12 @@ impl Eq for Password {}
 impl Password {
     /// Create a new password from a string. Does not check the entropy of the password.
     ///
+    /// You should use [`Password::from_str`] / [`String::parse`] instead.
+    ///
     /// Safety: Does not check the entropy of the password, or if one exists at all. This can be a security risk.
     #[allow(unsafe_code)]
-    unsafe fn new_unchecked(n: impl Into<String>) -> Self {
+    #[doc(hidden)]
+    pub unsafe fn new_unchecked(n: impl Into<String>) -> Self {
         let password = n.into();
         #[cfg(feature = "entropy")]
         let entropy = Self::calculate_entropy(&password);
@@ -1024,10 +1045,22 @@ pub enum ParseCodeError {
  */
 #[derive(PartialEq, Eq, Clone, Debug, derive_more::Display, derive_more::Deref)]
 #[display("{}", _0)]
+#[cfg(not(feature = "entropy"))]
 pub struct Code(
     #[deprecated(since = "0.7.0", note = "use the std::fmt::Display implementation")] pub String,
 );
 
+/** A wormhole code à la 15-foo-bar
+ *
+ * The part until the first dash is called the "nameplate" and is purely numeric.
+ * The rest is the password and may be arbitrary, although dash-joining words from
+ * a wordlist is a common convention.
+ */
+#[derive(PartialEq, Eq, Clone, Debug, derive_more::Display)]
+#[display("{}", _0)]
+#[cfg(feature = "entropy")]
+pub struct Code(String);
+
 #[allow(deprecated)]
 impl Code {
     /// Create a new code, comprised of a [`Nameplate`] and a password.
@@ -1037,6 +1070,7 @@ impl Code {
         since = "0.7.2",
         note = "Use [`from_components`] or the [std::str::FromStr] implementation"
     )]
+    #[cfg(not(feature = "entropy"))]
     pub fn new(nameplate: &Nameplate, password: &str) -> Self {
         if let Err(err) = Password::from_str(password) {
             tracing::error!("{err}");
@@ -1057,7 +1091,8 @@ impl Code {
     #[deprecated(since = "0.7.2", note = "Use [Self::nameplate] and [Self::password]")]
     pub fn split(&self) -> (Nameplate, String) {
         let mut iter = self.0.splitn(2, '-');
-        let nameplate = Nameplate::new(iter.next().unwrap());
+        #[allow(unsafe_code)]
+        let nameplate = unsafe { Nameplate::new_unchecked(iter.next().unwrap()) };
         let password = iter.next().unwrap();
         (nameplate, password.to_string())
     }
@@ -1092,6 +1127,7 @@ impl From<Code> for String {
 /// Deprecated: Use the [`std::str::FromStr`] implementation
 ///
 /// Safety: Does not check the entropy of the password, or if one exists at all. This can be a security risk.
+#[cfg(not(feature = "entropy"))]
 impl From<String> for Code {
     fn from(value: String) -> Self {
         tracing::debug!(
@@ -1112,8 +1148,8 @@ impl FromStr for Code {
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         match s.split_once('-') {
             Some((n, p)) => {
-                let nameplate: Nameplate = n.parse()?;
                 let password: Password = p.parse()?;
+                let nameplate: Nameplate = n.parse()?;
 
                 Ok(Self(format!("{}-{}", nameplate, password)))
             },

src/core/rendezvous.rs

@@ -572,8 +572,7 @@ impl RendezvousServer {
             .and_then(|state| state.nameplate.clone())
             .expect("Can only release an allocated nameplate, and only once");
 
-        use std::ops::Deref;
-        self.send_message(&OutboundMessage::release(nameplate.deref().deref()))
+        self.send_message(&OutboundMessage::release(nameplate.as_ref()))
             .await?;
         match self.receive_reply().await? {
             RendezvousReply::Released => (),

src/core/test.rs

@@ -1,15 +1,16 @@
 #![allow(irrefutable_let_patterns)]
 
 use super::{Mood, Phase};
+use rand::Rng;
+use std::{borrow::Cow, str::FromStr, time::Duration};
+
 #[cfg(feature = "transfer")]
 use crate::transfer;
 use crate::{
     self as magic_wormhole,
     core::{MailboxConnection, Nameplate},
     transit, AppConfig, AppID, Code, Wormhole, WormholeError,
 };
-use rand::Rng;
-use std::{borrow::Cow, time::Duration};
 use test_log::test;
 
 pub const TEST_APPID: AppID = AppID(std::borrow::Cow::Borrowed(
@@ -576,7 +577,7 @@ pub async fn test_connect_with_code_expecting_nameplate() -> eyre::Result<()> {
 fn generate_random_code() -> Code {
     let mut rng = rand::thread_rng();
     let nameplate_string = format!("{}-guitarist-revenge", rng.gen_range(1000..10000));
-    let nameplate = Nameplate::new(&nameplate_string);
+    let nameplate = Nameplate::from_str(&nameplate_string).unwrap();
     Code::from_components(nameplate, "guitarist-revenge".parse().unwrap())
 }
 

src/lib.rs

@@ -41,5 +41,5 @@ pub mod uri;
 pub use crate::core::{
     key::{GenericKey, Key, KeyPurpose, WormholeKey},
     rendezvous, AppConfig, AppID, Code, MailboxConnection, Mood, Nameplate, ParseCodeError,
-    ParseNameplateError, ParsePasswordError, Wormhole, WormholeError, WormholeWelcome,
+    ParseNameplateError, ParsePasswordError, Password, Wormhole, WormholeError, WormholeWelcome,
 };

src/uri.rs

@@ -39,6 +39,9 @@ pub enum ParseError {
         #[source]
         std::str::Utf8Error,
     ),
+    /// Error parsing the mailbox code
+    #[error("Error parsing the mailbox code")]
+    ParseCodeError(#[from] ParseCodeError),
 }
 
 /// The wormhole-transfer URI Scheme is used to encode a wormhole code for file transfer as a URI.
@@ -100,15 +103,9 @@ impl TryFrom<&url::Url> for WormholeTransferUri {
             "follower" => false,
             invalid => return Err(ParseError::InvalidRole(invalid.into())),
         };
-        let code = Code(
-            percent_encoding::percent_decode_str(url.path())
-                .decode_utf8()?
-                .into(),
-        );
-        // TODO move the code validation to somewhere else and also add more checks
-        if code.is_empty() {
-            return Err(ParseError::MissingCode);
-        }
+        let code: Code = percent_encoding::percent_decode_str(url.path())
+            .decode_utf8()?
+            .parse()?;
 
         Ok(WormholeTransferUri {
             code,
@@ -137,7 +134,7 @@ impl std::str::FromStr for WormholeTransferUri {
 impl From<&WormholeTransferUri> for url::Url {
     fn from(val: &WormholeTransferUri) -> Self {
         let mut url = url::Url::parse("wormhole-transfer:").unwrap();
-        url.set_path(&val.code);
+        url.set_path(val.code.as_ref());
         /* Only do this if there are any query parameteres at all, otherwise the URL will have an ugly trailing '?'. */
         if val.rendezvous_server.is_some() || val.is_leader {
             let mut query = url.query_pairs_mut();
@@ -171,18 +168,18 @@ mod test {
     #[test]
     fn test_uri() {
         test_eq(
-            WormholeTransferUri::new(Code("4-hurricane-equipment".to_owned())),
+            WormholeTransferUri::new("4-hurricane-equipment".parse().unwrap()),
             "wormhole-transfer:4-hurricane-equipment",
         );
 
         test_eq(
-            WormholeTransferUri::new(Code("8-🙈-🙉-🙊".to_owned())),
+            WormholeTransferUri::new("8-🙈-🙉-🙊".parse().unwrap()),
             "wormhole-transfer:8-%F0%9F%99%88-%F0%9F%99%89-%F0%9F%99%8A",
         );
 
         test_eq(
             WormholeTransferUri {
-                code: Code("8-🙈-🙉-🙊".to_owned()),
+                code: "8-🙈-🙉-🙊".parse().unwrap(),
                 rendezvous_server: Some(url::Url::parse("ws://localhost:4000").unwrap()),
                 is_leader: true,
             },