From 3f215cdb39e3214c3a36018e562ae8c84052918f Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 29 Mar 2018 01:22:54 -0600
Subject: [PATCH] Validation consistency between format & validation modules

The best place to catch share problems is immediately during parsing from
`&str`, however, because `validate_shares` takes any type that implements the
`IsShare` trait, and there's nothing about that trait that guarantees that the
share id, threshold, and secret length will be valid, I thought it best to leave
those three tests in `validate_shares` as a defensive coding practice.
---
 src/share/validation.rs  |  3 +++
 src/sss/format.rs        | 12 ++++++------
 tests/recovery_errors.rs | 15 +++++++++------
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/share/validation.rs b/src/share/validation.rs
index 9b4e158..d7d302f 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -40,6 +40,9 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
             share.get_data().len(),
         );
 
+        // Public-facing `Share::share_from_string` performs these three tests, but in case another
+        // type which implements `IsShare` is implemented later that doesn't do that validation,
+        // we'll leave them.
         if id < 1 {
             bail!(ErrorKind::ShareParsingInvalidShareId(id))
         } else if threshold_ < 2 {
diff --git a/src/sss/format.rs b/src/sss/format.rs
index 0e7c3ea..142ae8b 100644
--- a/src/sss/format.rs
+++ b/src/sss/format.rs
@@ -49,12 +49,12 @@ pub(crate) fn share_from_string(s: &str, is_signed: bool) -> Result<Share> {
         (k, i, p3)
     };
 
-    if k < 1 || i < 1 {
-        bail! {
-            ErrorKind::ShareParsingError(
-                format!("Found illegal share info: threshold = {}, identifier = {}.", k, i),
-            )
-        }
+    if i < 1 {
+        bail!(ErrorKind::ShareParsingInvalidShareId(i))
+    } else if k < 2 {
+        bail!(ErrorKind::ShareParsingInvalidShareThreshold(k, i))
+    } else if p3.is_empty() {
+        bail!(ErrorKind::ShareParsingErrorEmptyShare(i))
     }
 
     let raw_data = base64::decode_config(p3, BASE64_CONFIG).chain_err(|| {
diff --git a/tests/recovery_errors.rs b/tests/recovery_errors.rs
index fa862f3..0e1e8f5 100644
--- a/tests/recovery_errors.rs
+++ b/tests/recovery_errors.rs
@@ -11,6 +11,13 @@ fn test_recover_no_shares() {
     }
 }
 
+#[test]
+#[should_panic(expected = "ShareParsingErrorEmptyShare")]
+fn test_share_parsing_error_empty_share() {
+    let shares = vec!["2-1-".to_string()];
+    recover_secret(&shares, false).unwrap();
+}
+
 #[test]
 #[should_panic(expected = "ShareParsingError")]
 fn test_recover_2_parts_share() {
@@ -34,13 +41,9 @@ fn test_recover_incorrect_share_num() {
 }
 
 #[test]
-#[should_panic(expected = "ShareParsingError")]
+#[should_panic(expected = "ShareParsingInvalidShareId")]
 fn test_recover_0_share_num() {
-    let share1 = "2-0-1YAYwmOHqZ69jA".to_string();
-    let share2 = "2-1-YJZQDGm22Y77Gw".to_string();
-
-    let shares = vec![share1, share2];
-
+    let shares = vec!["2-0-1YAYwmOHqZ69jA".to_string()];
     recover_secret(&shares, false).unwrap();
 }