From a0abf417626be8d120f660c582195747d131b88b Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Mon, 12 Dec 2022 11:56:40 +0100 Subject: Fix router keyword handling (fix #442) --- src/api/admin/router.rs | 15 +++--- src/api/k2v/router.rs | 43 ++++++++-------- src/api/router_macros.rs | 131 ++++++++++++++++++++++++++--------------------- src/api/s3/router.rs | 121 +++++++++++++++++++++---------------------- 4 files changed, 161 insertions(+), 149 deletions(-) diff --git a/src/api/admin/router.rs b/src/api/admin/router.rs index 3fa07b3c..62e6abc3 100644 --- a/src/api/admin/router.rs +++ b/src/api/admin/router.rs @@ -143,10 +143,13 @@ impl Endpoint { } generateQueryParameters! { - "format" => format, - "id" => id, - "search" => search, - "globalAlias" => global_alias, - "alias" => alias, - "accessKeyId" => access_key_id + keywords: [], + fields: [ + "format" => format, + "id" => id, + "search" => search, + "globalAlias" => global_alias, + "alias" => alias, + "accessKeyId" => access_key_id + ] } diff --git a/src/api/k2v/router.rs b/src/api/k2v/router.rs index 50e6965b..e7a3dd69 100644 --- a/src/api/k2v/router.rs +++ b/src/api/k2v/router.rs @@ -96,7 +96,7 @@ impl Endpoint { fn from_get(partition_key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), partition_key, query, None), + (query.keyword.take().unwrap_or_default(), partition_key, query, None), key: [ EMPTY if causality_token => PollItem (query::sort_key, query::causality_token, opt_parse::timeout), EMPTY => ReadItem (query::sort_key), @@ -111,7 +111,7 @@ impl Endpoint { fn from_search(partition_key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), partition_key, query, None), + (query.keyword.take().unwrap_or_default(), partition_key, query, None), key: [ ], no_key: [ @@ -125,7 +125,7 @@ impl Endpoint { fn from_head(partition_key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), partition_key, query, None), + (query.keyword.take().unwrap_or_default(), partition_key, query, None), key: [ EMPTY => HeadObject(opt_parse::part_number, query_opt::version_id), ], @@ -140,7 +140,7 @@ impl Endpoint { fn from_post(partition_key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), partition_key, query, None), + (query.keyword.take().unwrap_or_default(), partition_key, query, None), key: [ ], no_key: [ @@ -155,7 +155,7 @@ impl Endpoint { fn from_put(partition_key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), partition_key, query, None), + (query.keyword.take().unwrap_or_default(), partition_key, query, None), key: [ EMPTY => InsertItem (query::sort_key), @@ -169,7 +169,7 @@ impl Endpoint { fn from_delete(partition_key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), partition_key, query, None), + (query.keyword.take().unwrap_or_default(), partition_key, query, None), key: [ EMPTY => DeleteItem (query::sort_key), ], @@ -232,21 +232,18 @@ impl Endpoint { // parameter name => struct field generateQueryParameters! { - "prefix" => prefix, - "start" => start, - "causality_token" => causality_token, - "end" => end, - "limit" => limit, - "reverse" => reverse, - "sort_key" => sort_key, - "timeout" => timeout -} - -mod keywords { - //! This module contain all query parameters with no associated value - //! used to differentiate endpoints. - pub const EMPTY: &str = ""; - - pub const DELETE: &str = "delete"; - pub const SEARCH: &str = "search"; + keywords: [ + "delete" => DELETE, + "search" => SEARCH + ], + fields: [ + "prefix" => prefix, + "start" => start, + "causality_token" => causality_token, + "end" => end, + "limit" => limit, + "reverse" => reverse, + "sort_key" => sort_key, + "timeout" => timeout + ] } diff --git a/src/api/router_macros.rs b/src/api/router_macros.rs index 4c593300..959e69a3 100644 --- a/src/api/router_macros.rs +++ b/src/api/router_macros.rs @@ -4,10 +4,9 @@ macro_rules! router_match { (@match $enum:expr , [ $($endpoint:ident,)* ]) => {{ // usage: router_match {@match my_enum, [ VariantWithField1, VariantWithField2 ..] } // returns true if the variant was one of the listed variants, false otherwise. - use Endpoint::*; match $enum { $( - $endpoint { .. } => true, + Endpoint::$endpoint { .. } => true, )* _ => false } @@ -15,37 +14,35 @@ macro_rules! router_match { (@extract $enum:expr , $param:ident, [ $($endpoint:ident,)* ]) => {{ // usage: router_match {@extract my_enum, field_name, [ VariantWithField1, VariantWithField2 ..] } // returns Some(field_value), or None if the variant was not one of the listed variants. - use Endpoint::*; match $enum { $( - $endpoint {$param, ..} => Some($param), + Endpoint::$endpoint {$param, ..} => Some($param), )* _ => None } }}; - (@gen_path_parser ($method:expr, $reqpath:expr, $query:expr) - [ - $($meth:ident $path:pat $(if $required:ident)? => $api:ident $(($($conv:ident :: $param:ident),*))?,)* - ]) => {{ - { - use Endpoint::*; - match ($method, $reqpath) { - $( - (&Method::$meth, $path) if true $(&& $query.$required.is_some())? => $api { - $($( - $param: router_match!(@@parse_param $query, $conv, $param), - )*)? - }, - )* - (m, p) => { - return Err(Error::bad_request(format!( - "Unknown API endpoint: {} {}", - m, p - ))) - } - } - } - }}; + (@gen_path_parser ($method:expr, $reqpath:expr, $query:expr) + [ + $($meth:ident $path:pat $(if $required:ident)? => $api:ident $(($($conv:ident :: $param:ident),*))?,)* + ]) => {{ + { + match ($method, $reqpath) { + $( + (&Method::$meth, $path) if true $(&& $query.$required.is_some())? => Endpoint::$api { + $($( + $param: router_match!(@@parse_param $query, $conv, $param), + )*)? + }, + )* + (m, p) => { + return Err(Error::bad_request(format!( + "Unknown API endpoint: {} {}", + m, p + ))) + } + } + } + }}; (@gen_parser ($keyword:expr, $key:ident, $query:expr, $header:expr), key: [$($kw_k:ident $(if $required_k:ident)? $(header $header_k:expr)? => $api_k:ident $(($($conv_k:ident :: $param_k:ident),*))?,)*], no_key: [$($kw_nk:ident $(if $required_nk:ident)? $(if_header $header_nk:expr)? => $api_nk:ident $(($($conv_nk:ident :: $param_nk:ident),*))?,)*]) => {{ @@ -60,11 +57,9 @@ macro_rules! router_match { // ] // } // See in from_{method} for more detailed usage. - use Endpoint::*; - use keywords::*; match ($keyword, !$key.is_empty()){ $( - ($kw_k, true) if true $(&& $query.$required_k.is_some())? $(&& $header.contains_key($header_k))? => Ok($api_k { + (Keyword::$kw_k, true) if true $(&& $query.$required_k.is_some())? $(&& $header.contains_key($header_k))? => Ok(Endpoint::$api_k { $key, $($( $param_k: router_match!(@@parse_param $query, $conv_k, $param_k), @@ -72,7 +67,7 @@ macro_rules! router_match { }), )* $( - ($kw_nk, false) $(if $query.$required_nk.is_some())? $(if $header.contains($header_nk))? => Ok($api_nk { + (Keyword::$kw_nk, false) $(if $query.$required_nk.is_some())? $(if $header.contains($header_nk))? => Ok(Endpoint::$api_nk { $($( $param_nk: router_match!(@@parse_param $query, $conv_nk, $param_nk), )*)? @@ -84,7 +79,7 @@ macro_rules! router_match { (@@parse_param $query:expr, query_opt, $param:ident) => {{ // extract optional query parameter - $query.$param.take().map(|param| param.into_owned()) + $query.$param.take().map(|param| param.into_owned()) }}; (@@parse_param $query:expr, query, $param:ident) => {{ // extract mendatory query parameter @@ -93,7 +88,7 @@ macro_rules! router_match { (@@parse_param $query:expr, opt_parse, $param:ident) => {{ // extract and parse optional query parameter // missing parameter is file, however parse error is reported as an error - $query.$param + $query.$param .take() .map(|param| param.parse()) .transpose() @@ -144,14 +139,39 @@ macro_rules! router_match { /// This macro is used to generate part of the code in this module. It must be called only one, and /// is useless outside of this module. macro_rules! generateQueryParameters { - ( $($rest:expr => $name:ident),* ) => { + ( + keywords: [ $($kw_param:expr => $kw_name: ident),* ], + fields: [ $($f_param:expr => $f_name:ident),* ] + ) => { + #[derive(Debug)] + #[allow(non_camel_case_types)] + enum Keyword { + EMPTY, + $( $kw_name, )* + } + + impl std::fmt::Display for Keyword { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Keyword::EMPTY => write!(f, "``"), + $( Keyword::$kw_name => write!(f, "`{}`", $kw_param), )* + } + } + } + + impl Default for Keyword { + fn default() -> Self { + Keyword::EMPTY + } + } + /// Struct containing all query parameters used in endpoints. Think of it as an HashMap, /// but with keys statically known. #[derive(Debug, Default)] struct QueryParameters<'a> { - keyword: Option>, + keyword: Option, $( - $name: Option>, + $f_name: Option>, )* } @@ -160,34 +180,29 @@ macro_rules! generateQueryParameters { fn from_query(query: &'a str) -> Result { let mut res: Self = Default::default(); for (k, v) in url::form_urlencoded::parse(query.as_bytes()) { - let repeated = match k.as_ref() { + match k.as_ref() { $( - $rest => if !v.is_empty() { - res.$name.replace(v).is_some() - } else { - false + $kw_param => if let Some(prev_kw) = res.keyword.replace(Keyword::$kw_name) { + return Err(Error::bad_request(format!( + "Multiple keywords: '{}' and '{}'", prev_kw, $kw_param + ))); }, )* - _ => { - if k.starts_with("response-") || k.starts_with("X-Amz-") { - false - } else if v.as_ref().is_empty() { - if res.keyword.replace(k).is_some() { - return Err(Error::bad_request("Multiple keywords")); + $( + $f_param => if !v.is_empty() { + if res.$f_name.replace(v).is_some() { + return Err(Error::bad_request(format!( + "Query parameter repeated: '{}'", k + ))); } - continue; - } else { + }, + )* + _ => { + if !(k.starts_with("response-") || k.starts_with("X-Amz-")) { debug!("Received an unknown query parameter: '{}'", k); - false } } }; - if repeated { - return Err(Error::bad_request(format!( - "Query parameter repeated: '{}'", - k - ))); - } } Ok(res) } @@ -198,8 +213,8 @@ macro_rules! generateQueryParameters { if self.keyword.is_some() { Some("Keyword not used") } $( - else if self.$name.is_some() { - Some(concat!("'", $rest, "'")) + else if self.$f_name.is_some() { + Some(concat!("'", $f_param, "'")) } )* else { None diff --git a/src/api/s3/router.rs b/src/api/s3/router.rs index 44f581ff..821b0e07 100644 --- a/src/api/s3/router.rs +++ b/src/api/s3/router.rs @@ -355,7 +355,7 @@ impl Endpoint { fn from_get(key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), key, query, None), + (query.keyword.take().unwrap_or_default(), key, query, None), key: [ EMPTY if upload_id => ListParts (query::upload_id, opt_parse::max_parts, opt_parse::part_number_marker), EMPTY => GetObject (query_opt::version_id, opt_parse::part_number), @@ -412,7 +412,7 @@ impl Endpoint { fn from_head(key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), key, query, None), + (query.keyword.take().unwrap_or_default(), key, query, None), key: [ EMPTY => HeadObject(opt_parse::part_number, query_opt::version_id), ], @@ -426,7 +426,7 @@ impl Endpoint { fn from_post(key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), key, query, None), + (query.keyword.take().unwrap_or_default(), key, query, None), key: [ EMPTY if upload_id => CompleteMultipartUpload (query::upload_id), RESTORE => RestoreObject (query_opt::version_id), @@ -448,7 +448,7 @@ impl Endpoint { ) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), key, query, headers), + (query.keyword.take().unwrap_or_default(), key, query, headers), key: [ EMPTY if part_number header "x-amz-copy-source" => UploadPartCopy (parse::part_number, query::upload_id), EMPTY header "x-amz-copy-source" => CopyObject, @@ -490,7 +490,7 @@ impl Endpoint { fn from_delete(key: String, query: &mut QueryParameters<'_>) -> Result { router_match! { @gen_parser - (query.keyword.take().unwrap_or_default().as_ref(), key, query, None), + (query.keyword.take().unwrap_or_default(), key, query, None), key: [ EMPTY if upload_id => AbortMultipartUpload (query::upload_id), EMPTY => DeleteObject (query_opt::version_id), @@ -624,63 +624,60 @@ impl Endpoint { // parameter name => struct field generateQueryParameters! { - "continuation-token" => continuation_token, - "delimiter" => delimiter, - "encoding-type" => encoding_type, - "fetch-owner" => fetch_owner, - "id" => id, - "key-marker" => key_marker, - "list-type" => list_type, - "marker" => marker, - "max-keys" => max_keys, - "max-parts" => max_parts, - "max-uploads" => max_uploads, - "partNumber" => part_number, - "part-number-marker" => part_number_marker, - "prefix" => prefix, - "select-type" => select_type, - "start-after" => start_after, - "uploadId" => upload_id, - "upload-id-marker" => upload_id_marker, - "versionId" => version_id, - "version-id-marker" => version_id_marker -} - -mod keywords { - //! This module contain all query parameters with no associated value S3 uses to differentiate - //! endpoints. - pub const EMPTY: &str = ""; - - pub const ACCELERATE: &str = "accelerate"; - pub const ACL: &str = "acl"; - pub const ANALYTICS: &str = "analytics"; - pub const CORS: &str = "cors"; - pub const DELETE: &str = "delete"; - pub const ENCRYPTION: &str = "encryption"; - pub const INTELLIGENT_TIERING: &str = "intelligent-tiering"; - pub const INVENTORY: &str = "inventory"; - pub const LEGAL_HOLD: &str = "legal-hold"; - pub const LIFECYCLE: &str = "lifecycle"; - pub const LOCATION: &str = "location"; - pub const LOGGING: &str = "logging"; - pub const METRICS: &str = "metrics"; - pub const NOTIFICATION: &str = "notification"; - pub const OBJECT_LOCK: &str = "object-lock"; - pub const OWNERSHIP_CONTROLS: &str = "ownershipControls"; - pub const POLICY: &str = "policy"; - pub const POLICY_STATUS: &str = "policyStatus"; - pub const PUBLIC_ACCESS_BLOCK: &str = "publicAccessBlock"; - pub const REPLICATION: &str = "replication"; - pub const REQUEST_PAYMENT: &str = "requestPayment"; - pub const RESTORE: &str = "restore"; - pub const RETENTION: &str = "retention"; - pub const SELECT: &str = "select"; - pub const TAGGING: &str = "tagging"; - pub const TORRENT: &str = "torrent"; - pub const UPLOADS: &str = "uploads"; - pub const VERSIONING: &str = "versioning"; - pub const VERSIONS: &str = "versions"; - pub const WEBSITE: &str = "website"; + keywords: [ + "accelerate" => ACCELERATE, + "acl" => ACL, + "analytics" => ANALYTICS, + "cors" => CORS, + "delete" => DELETE, + "encryption" => ENCRYPTION, + "intelligent-tiering" => INTELLIGENT_TIERING, + "inventory" => INVENTORY, + "legal-hold" => LEGAL_HOLD, + "lifecycle" => LIFECYCLE, + "location" => LOCATION, + "logging" => LOGGING, + "metrics" => METRICS, + "notification" => NOTIFICATION, + "object-lock" => OBJECT_LOCK, + "ownershipControls" => OWNERSHIP_CONTROLS, + "policy" => POLICY, + "policyStatus" => POLICY_STATUS, + "publicAccessBlock" => PUBLIC_ACCESS_BLOCK, + "replication" => REPLICATION, + "requestPayment" => REQUEST_PAYMENT, + "restore" => RESTORE, + "retention" => RETENTION, + "select" => SELECT, + "tagging" => TAGGING, + "torrent" => TORRENT, + "uploads" => UPLOADS, + "versioning" => VERSIONING, + "versions" => VERSIONS, + "website" => WEBSITE + ], + fields: [ + "continuation-token" => continuation_token, + "delimiter" => delimiter, + "encoding-type" => encoding_type, + "fetch-owner" => fetch_owner, + "id" => id, + "key-marker" => key_marker, + "list-type" => list_type, + "marker" => marker, + "max-keys" => max_keys, + "max-parts" => max_parts, + "max-uploads" => max_uploads, + "partNumber" => part_number, + "part-number-marker" => part_number_marker, + "prefix" => prefix, + "select-type" => select_type, + "start-after" => start_after, + "uploadId" => upload_id, + "upload-id-marker" => upload_id_marker, + "versionId" => version_id, + "version-id-marker" => version_id_marker + ] } #[cfg(test)] -- cgit v1.2.3