avoid manually managing query strings (#1421)

SAS tokens are query strings and follow the encoding rules required by Url query strings. Hand crafting Url query strings could lead us into failure points in the future.
Azure · Sep 27, 2023 · 66008eb · 66008eb
1 parent d8bf5ed
commit 66008eb
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 53 deletions.
diff --git a/sdk/storage/src/shared_access_signature/account_sas.rs b/sdk/storage/src/shared_access_signature/account_sas.rs
@@ -1,9 +1,10 @@
 use crate::{
     hmac::sign,
-    shared_access_signature::{format_date, format_form, SasProtocol, SasToken},
+    shared_access_signature::{format_date, SasProtocol, SasToken},
 };
 use std::fmt;
 use time::OffsetDateTime;
+use url::form_urlencoded;
 
 /// Service version of the shared access signature ([Azure documentation](https://docs.microsoft.com/rest/api/storageservices/create-service-sas#specifying-the-signed-version-field)).
 #[derive(Copy, Clone)]
@@ -173,7 +174,7 @@ impl AccountSharedAccessSignature {
     }
 
     // Azure documentation: https://docs.microsoft.com/rest/api/storageservices/create-service-sas#constructing-the-signature-string
-    fn signature(&self) -> String {
+    fn sign(&self) -> String {
         match self.version {
             AccountSasVersion::V20181109 => {
                 let string_to_sign = format!(
@@ -204,38 +205,38 @@ impl AccountSharedAccessSignature {
 impl SasToken for AccountSharedAccessSignature {
     /// [Example](https://docs.microsoft.com/rest/api/storageservices/create-service-sas#service-sas-example) from Azure documentation.
     fn token(&self) -> String {
-        let mut elements: Vec<String> = vec![
-            format!("sv={}", self.version),
-            format!("ss={}", self.resource),
-            format!("srt={}", self.resource_type),
-            format!("se={}", format_form(format_date(self.expiry))),
-            format!("sp={}", self.permissions),
-        ];
+        let mut form = form_urlencoded::Serializer::new(String::new());
+        form.extend_pairs(&[
+            ("sv", &self.version.to_string()),
+            ("ss", &self.resource.to_string()),
+            ("srt", &self.resource_type.to_string()),
+            ("se", &format_date(self.expiry)),
+            ("sp", &self.permissions.to_string()),
+        ]);
 
         if let Some(start) = &self.start {
-            elements.push(format!("st={}", format_form(format_date(*start))));
+            form.append_pair("st", &format_date(*start));
         }
         if let Some(ip) = &self.ip {
-            elements.push(format!("sip={ip}"));
+            form.append_pair("sip", ip);
         }
         if let Some(protocol) = &self.protocol {
-            elements.push(format!("spr={protocol}"));
+            form.append_pair("spr", &protocol.to_string());
         }
-        let sig = AccountSharedAccessSignature::signature(self);
-        elements.push(format!("sig={}", format_form(sig)));
-
-        elements.join("&")
+        let sig = self.sign();
+        form.append_pair("sig", &sig);
+        form.finish()
     }
 }
 
 impl PartialEq for AccountSharedAccessSignature {
     fn eq(&self, other: &Self) -> bool {
-        self.signature() == other.signature()
+        self.sign() == other.sign()
     }
 }
 
 impl std::fmt::Debug for AccountSharedAccessSignature {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-        write!(f, "SharedAccessSignature {{{}}}", self.signature())
+        write!(f, "SharedAccessSignature {{{}}}", self.sign())
     }
 }
diff --git a/sdk/storage/src/shared_access_signature/mod.rs b/sdk/storage/src/shared_access_signature/mod.rs
@@ -1,6 +1,5 @@
 use std::fmt;
 use time::OffsetDateTime;
-use url::form_urlencoded;
 
 pub mod account_sas;
 pub mod service_sas;
@@ -24,10 +23,6 @@ pub(crate) fn format_date(d: OffsetDateTime) -> String {
     azure_core::date::to_rfc3339(&d.replace_nanosecond(0).unwrap())
 }
 
-pub(crate) fn format_form(d: String) -> String {
-    form_urlencoded::byte_serialize(d.as_bytes()).collect::<String>()
-}
-
 /// Specifies the protocol permitted for a request made with the SAS ([Azure documentation](https://docs.microsoft.com/rest/api/storageservices/create-service-sas#specifying-the-http-protocol)).
 #[derive(Copy, Clone)]
 pub enum SasProtocol {

diff --git a/sdk/storage/src/shared_access_signature/service_sas.rs b/sdk/storage/src/shared_access_signature/service_sas.rs
@@ -1,9 +1,10 @@
 use crate::{
     hmac,
-    shared_access_signature::{format_date, format_form, SasProtocol, SasToken},
+    shared_access_signature::{format_date, SasProtocol, SasToken},
 };
 use std::fmt;
 use time::OffsetDateTime;
+use url::form_urlencoded;
 
 const SERVICE_SAS_VERSION: &str = "2020-06-12";
 
@@ -162,40 +163,40 @@ impl BlobSharedAccessSignature {
 
 impl SasToken for BlobSharedAccessSignature {
     fn token(&self) -> String {
-        let mut elements: Vec<String> = vec![
-            format!("sv={SERVICE_SAS_VERSION}"),
-            format!("sp={}", self.permissions),
-            format!("sr={}", self.resource),
-            format!("se={}", format_form(format_date(self.expiry))),
-        ];
+        let mut form = form_urlencoded::Serializer::new(String::new());
+
+        form.extend_pairs(&[
+            ("sv", SERVICE_SAS_VERSION),
+            ("sp", &self.permissions.to_string()),
+            ("sr", &self.resource.to_string()),
+            ("se", &format_date(self.expiry)),
+        ]);
 
         if let Some(start) = &self.start {
-            elements.push(format!("st={}", format_form(format_date(*start))));
+            form.append_pair("st", &format_date(*start));
         }
 
         if let Some(ip) = &self.ip {
-            elements.push(format!("sip={ip}"));
+            form.append_pair("sip", ip);
         }
 
         if let Some(protocol) = &self.protocol {
-            elements.push(format!("spr={protocol}"));
+            form.append_pair("spr", &protocol.to_string());
         }
 
         if let Some(signed_directory_depth) = &self.signed_directory_depth {
-            elements.push(format!("sdd={signed_directory_depth}"));
+            form.append_pair("sdd", &signed_directory_depth.to_string());
         }
 
         let sig = self.sign();
-        elements.push(format!("sig={}", format_form(sig)));
-
-        elements.join("&")
+        form.append_pair("sig", &sig);
+        form.finish()
     }
 }
 
 #[cfg(test)]
 mod test {
     use super::*;
-    use std::collections::HashSet;
     use time::Duration;
 
     const MOCK_SECRET_KEY: &str = "RZfi3m1W7eyQ5zD4ymSmGANVdJ2SDQmg4sE89SW104s=";
@@ -216,16 +217,15 @@ mod test {
         )
         .token();
 
-        // splitting by & is only safe if & is not part of any fields
-        // if that changes in the future we might want to use a proper query string parser
-        let elements = signed_token.split('&').collect::<HashSet<_>>();
+        assert_eq!(signed_token, "sv=2020-06-12&sp=r&sr=b&se=1970-01-08T00%3A00%3A00Z&sig=alEGfKjtiLs5LO%2FyrfPkzjQBHbk4Uda9XOezbRyKwEM%3D");
+
+        let mut parsed = url::form_urlencoded::parse(&signed_token.as_bytes());
 
         // BlobSignedResource::Blob
-        assert!(elements.contains("sr=b"));
-        // signed_directory_depth NOT set
-        assert!(!elements.iter().any(|element| element.starts_with("sdd=")));
+        assert!(parsed.find(|(k, v)| k == "sr" && v == "b").is_some());
 
-        assert_eq!(signed_token, "sv=2020-06-12&sp=r&sr=b&se=1970-01-08T00%3A00%3A00Z&sig=alEGfKjtiLs5LO%2FyrfPkzjQBHbk4Uda9XOezbRyKwEM%3D");
+        // signed_directory_depth NOT set
+        assert!(parsed.find(|(k, _)| k == "sdd").is_none());
     }
 
     #[test]
@@ -244,15 +244,14 @@ mod test {
         .signed_directory_depth(2_usize)
         .token();
 
-        // splitting by & is only safe if & is not part of any fields
-        // if that changes in the future we might want to use a proper query string parser
-        let elements = signed_token.split('&').collect::<HashSet<_>>();
+        assert_eq!(signed_token, "sv=2020-06-12&sp=r&sr=d&se=1970-01-08T00%3A00%3A00Z&sdd=2&sig=e0eoY169%2Bex4AnI9ZAOiOaX49snoJiuvyJ22XV6qW2k%3D");
 
-        // BlobSignedResource::Blob
-        assert!(elements.contains("sr=d"));
-        // signed_directory_depth = 2
-        assert!(elements.contains("sdd=2"));
+        let mut parsed = url::form_urlencoded::parse(&signed_token.as_bytes());
 
-        assert_eq!(signed_token, "sv=2020-06-12&sp=r&sr=d&se=1970-01-08T00%3A00%3A00Z&sdd=2&sig=e0eoY169%2Bex4AnI9ZAOiOaX49snoJiuvyJ22XV6qW2k%3D");
+        // BlobSignedResource::Directory
+        assert!(parsed.find(|(k, v)| k == "sr" && v == "d").is_some());
+
+        // signed_directory_depth NOT set
+        assert!(parsed.find(|(k, v)| k == "sdd" && v == "2").is_some());
     }
 }