From 069d9f8296f1fde737f96f6287751d1fc2d2f6d3 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 29 Oct 2024 15:07:16 -0400 Subject: [PATCH 1/8] Support request.query decoding Signed-off-by: Alex Snaps --- Cargo.lock | 7 ++++++ Cargo.toml | 1 + src/data/cel.rs | 64 +++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9c5c5b5e..0e98929a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1907,6 +1907,12 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "urlencoding" +version = "2.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "daf8dba3b7eb870caf1ddeed7bc9d2a049f3cfdfae7cb521b087cc33ae4c49da" + [[package]] name = "uuid" version = "1.10.0" @@ -1958,6 +1964,7 @@ dependencies = [ "serde_json", "serial_test", "thiserror", + "urlencoding", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 9042e8b6..f4622878 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,7 @@ const_format = "0.2.31" chrono = { version = "0.4.38", default-features = false, features = ["alloc", "std"] } cel-interpreter = "0.8.1" cel-parser = "0.7.1" +urlencoding = "2.1.3" [dev-dependencies] proxy-wasm-test-framework = { git = "https://github.com/Kuadrant/wasm-test-framework.git", branch = "kuadrant" } diff --git a/src/data/cel.rs b/src/data/cel.rs index 09e1aba0..7a2a96ac 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -1,15 +1,17 @@ use crate::data::get_attribute; use crate::data::property::{host_get_map, Path}; use cel_interpreter::extractors::This; -use cel_interpreter::objects::{Map, ValueType}; +use cel_interpreter::objects::{Key, Map, ValueType}; use cel_interpreter::{Context, ExecutionError, ResolveResult, Value}; use cel_parser::{parse, Expression as CelExpression, Member, ParseError}; use chrono::{DateTime, FixedOffset}; use proxy_wasm::types::{Bytes, Status}; use serde_json::Value as JsonValue; +use std::collections::hash_map::Entry; use std::collections::HashMap; use std::fmt::{Debug, Formatter}; -use std::sync::OnceLock; +use std::sync::{Arc, OnceLock}; +use urlencoding::decode; #[derive(Clone, Debug)] pub struct Expression { @@ -45,6 +47,7 @@ impl Expression { pub fn eval(&self) -> Value { let mut ctx = create_context(); + ctx.add_function("urlDecode", url_decode); let Map { map } = self.build_data_map(); ctx.add_function("getHostProperty", get_host_property); @@ -67,6 +70,33 @@ impl Expression { } } +fn url_decode(This(s): This>) -> ResolveResult { + let mut map: HashMap = HashMap::default(); + for part in s.split('&') { + let mut kv = part.split('='); + if let (Some(key), Some(value)) = (kv.next(), kv.next().or(Some(""))) { + let new_v: Value = decode(value).unwrap().into_owned().into(); + match map.entry(decode(key).unwrap().into_owned().into()) { + Entry::Occupied(mut e) => { + if let Value::List(ref mut list) = e.get_mut() { + Arc::get_mut(list) + .expect("This isn't ever shared!") + .push(new_v); + } else { + let v = e.get().clone(); + let list = Value::List([v, new_v].to_vec().into()); + e.insert(list); + } + } + Entry::Vacant(e) => { + e.insert(decode(value).unwrap().into_owned().into()); + } + } + } + } + Ok(map.into()) +} + #[cfg(test)] pub fn inner_host_get_property(path: Vec<&str>) -> Result, Status> { super::property::host_get_property(&Path::new(path)) @@ -578,6 +608,36 @@ mod tests { assert_eq!(value, "some random crap".into()); } + #[test] + fn decodes_query_string() { + property::test::TEST_PROPERTY_VALUE.set(Some(( + "request.query".into(), + "param1=%F0%9F%91%BE%20¶m2=Exterminate%21&%F0%9F%91%BE=123&%F0%9F%91%BE=456&%F0%9F%91%BE" + .bytes() + .collect(), + ))); + let predicate = Predicate::new( + "urlDecode(request.query)['param1'] == '👾 ' && \ + urlDecode(request.query)['param2'] == 'Exterminate!' && \ + urlDecode(request.query)['👾'][0] == '123' && \ + urlDecode(request.query)['👾'][1] == '456' && \ + urlDecode(request.query)['👾'][2] == '' \ + ", + ) + .expect("This is valid!"); + assert!(predicate.test()); + + property::test::TEST_PROPERTY_VALUE.set(Some(( + "request.query".into(), + "%F0%9F%91%BE".bytes().collect(), + ))); + let predicate = Predicate::new( + "urlDecode(request.query) == {'👾': ''}", + ) + .expect("This is valid!"); + assert!(predicate.test()); + } + #[test] fn attribute_resolve() { property::test::TEST_PROPERTY_VALUE.set(Some(( From e8b1b08342e7d69d8240b6646f4a83dcb7d10912 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Sat, 2 Nov 2024 07:54:46 -0400 Subject: [PATCH 2/8] Renamed fn, and made it 'extended' only Signed-off-by: Alex Snaps --- src/data/cel.rs | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/data/cel.rs b/src/data/cel.rs index 7a2a96ac..31b920b1 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -45,17 +45,15 @@ impl Expression { }) } - pub fn eval(&self) -> Value { + pub fn eval_extended(&self, extended: bool) -> Value { let mut ctx = create_context(); - ctx.add_function("urlDecode", url_decode); + if extended { + ctx.add_function("decodeQueryString", url_decode); + } let Map { map } = self.build_data_map(); ctx.add_function("getHostProperty", get_host_property); - // if expression was "auth.identity.anonymous", - // { - // "auth": { "identity": { "anonymous": true } } - // } for binding in ["request", "metadata", "source", "destination", "auth"] { ctx.add_variable_from_value( binding, @@ -65,6 +63,10 @@ impl Expression { Value::resolve(&self.expression, &ctx).expect("Cel expression couldn't be evaluated") } + pub fn eval(&self) -> Value { + self.eval_extended(false) + } + fn build_data_map(&self) -> Map { data::AttributeMap::new(self.attributes.clone()).into() } @@ -163,7 +165,11 @@ impl Predicate { } pub fn test(&self) -> bool { - match self.expression.eval() { + self.test_extended(false) + } + + pub fn test_extended(&self, extended: bool) -> bool { + match self.expression.eval_extended(extended) { Value::Bool(result) => result, _ => false, } @@ -617,25 +623,25 @@ mod tests { .collect(), ))); let predicate = Predicate::new( - "urlDecode(request.query)['param1'] == '👾 ' && \ - urlDecode(request.query)['param2'] == 'Exterminate!' && \ - urlDecode(request.query)['👾'][0] == '123' && \ - urlDecode(request.query)['👾'][1] == '456' && \ - urlDecode(request.query)['👾'][2] == '' \ + "decodeQueryString(request.query)['param1'] == '👾 ' && \ + decodeQueryString(request.query)['param2'] == 'Exterminate!' && \ + decodeQueryString(request.query)['👾'][0] == '123' && \ + decodeQueryString(request.query)['👾'][1] == '456' && \ + decodeQueryString(request.query)['👾'][2] == '' \ ", ) .expect("This is valid!"); - assert!(predicate.test()); + assert!(predicate.test_extended(true)); property::test::TEST_PROPERTY_VALUE.set(Some(( "request.query".into(), "%F0%9F%91%BE".bytes().collect(), ))); let predicate = Predicate::new( - "urlDecode(request.query) == {'👾': ''}", + "decodeQueryString(request.query) == {'👾': ''}", ) .expect("This is valid!"); - assert!(predicate.test()); + assert!(predicate.test_extended(true)); } #[test] From 6dc72bf52b245384a29428432643cf5802f526f9 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Sat, 2 Nov 2024 08:48:49 -0400 Subject: [PATCH 3/8] Wired support for decodeQueryString in routeRule predicates only Signed-off-by: Alex Snaps --- src/configuration.rs | 2 +- src/data/cel.rs | 38 +++++++++++++++++++++++--------------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/configuration.rs b/src/configuration.rs index 22c23380..f29eff95 100644 --- a/src/configuration.rs +++ b/src/configuration.rs @@ -468,7 +468,7 @@ impl TryFrom for FilterConfig { } let mut predicates = Vec::default(); for predicate in &action_set.route_rule_conditions.predicates { - predicates.push(Predicate::new(predicate).map_err(|e| e.to_string())?); + predicates.push(Predicate::route_rule(predicate).map_err(|e| e.to_string())?); } action_set .route_rule_conditions diff --git a/src/data/cel.rs b/src/data/cel.rs index 31b920b1..7c21faac 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -17,10 +17,11 @@ use urlencoding::decode; pub struct Expression { attributes: Vec, expression: CelExpression, + extended: bool, } impl Expression { - pub fn new(expression: &str) -> Result { + pub fn new_expression(expression: &str, extended: bool) -> Result { let expression = parse(expression)?; let mut props = Vec::with_capacity(5); @@ -42,12 +43,21 @@ impl Expression { Ok(Self { attributes, expression, + extended, }) } - pub fn eval_extended(&self, extended: bool) -> Value { + pub fn new(expression: &str) -> Result { + Self::new_expression(expression, false) + } + + pub fn new_extended(expression: &str) -> Result { + Self::new_expression(expression, true) + } + + pub fn eval(&self) -> Value { let mut ctx = create_context(); - if extended { + if self.extended { ctx.add_function("decodeQueryString", url_decode); } let Map { map } = self.build_data_map(); @@ -63,10 +73,6 @@ impl Expression { Value::resolve(&self.expression, &ctx).expect("Cel expression couldn't be evaluated") } - pub fn eval(&self) -> Value { - self.eval_extended(false) - } - fn build_data_map(&self) -> Map { data::AttributeMap::new(self.attributes.clone()).into() } @@ -164,12 +170,14 @@ impl Predicate { }) } - pub fn test(&self) -> bool { - self.test_extended(false) + pub fn route_rule(predicate: &str) -> Result { + Ok(Self { + expression: Expression::new_extended(predicate)?, + }) } - pub fn test_extended(&self, extended: bool) -> bool { - match self.expression.eval_extended(extended) { + pub fn test(&self) -> bool { + match self.expression.eval() { Value::Bool(result) => result, _ => false, } @@ -622,7 +630,7 @@ mod tests { .bytes() .collect(), ))); - let predicate = Predicate::new( + let predicate = Predicate::route_rule( "decodeQueryString(request.query)['param1'] == '👾 ' && \ decodeQueryString(request.query)['param2'] == 'Exterminate!' && \ decodeQueryString(request.query)['👾'][0] == '123' && \ @@ -631,17 +639,17 @@ mod tests { ", ) .expect("This is valid!"); - assert!(predicate.test_extended(true)); + assert!(predicate.test()); property::test::TEST_PROPERTY_VALUE.set(Some(( "request.query".into(), "%F0%9F%91%BE".bytes().collect(), ))); - let predicate = Predicate::new( + let predicate = Predicate::route_rule( "decodeQueryString(request.query) == {'👾': ''}", ) .expect("This is valid!"); - assert!(predicate.test_extended(true)); + assert!(predicate.test()); } #[test] From 743425b84768bd9238d271e8d378f02d62374711 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Sat, 2 Nov 2024 09:22:44 -0400 Subject: [PATCH 4/8] Added rustdoc Signed-off-by: Alex Snaps --- src/data/cel.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/data/cel.rs b/src/data/cel.rs index 7c21faac..a45d13b2 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -58,7 +58,7 @@ impl Expression { pub fn eval(&self) -> Value { let mut ctx = create_context(); if self.extended { - ctx.add_function("decodeQueryString", url_decode); + Self::add_extended_capabilities(&mut ctx) } let Map { map } = self.build_data_map(); @@ -73,12 +73,20 @@ impl Expression { Value::resolve(&self.expression, &ctx).expect("Cel expression couldn't be evaluated") } + /// Add support for `decodeQueryString`, see [`decode_query_string`] + fn add_extended_capabilities(ctx: &mut Context) { + ctx.add_function("decodeQueryString", decode_query_string); + } + fn build_data_map(&self) -> Map { data::AttributeMap::new(self.attributes.clone()).into() } } -fn url_decode(This(s): This>) -> ResolveResult { +/// Decodes the query string and returns a Map where the key is the parameter's name and +/// the value is either a [`Value::String`] or a [`Value::List`] if the parameter's name is repeated +/// see [`tests::decodes_query_string`] +fn decode_query_string(This(s): This>) -> ResolveResult { let mut map: HashMap = HashMap::default(); for part in s.split('&') { let mut kv = part.split('='); @@ -170,6 +178,9 @@ impl Predicate { }) } + /// Unlike with [`Predicate::new`], a `Predicate::route_rule` is backed by an + /// `Expression` that has extended capabilities enabled. + /// See [`Expression::add_extended_capabilities`] pub fn route_rule(predicate: &str) -> Result { Ok(Self { expression: Expression::new_extended(predicate)?, From cdc899d84575e7610227dca7c005767845bfbb4f Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 5 Nov 2024 07:12:13 -0500 Subject: [PATCH 5/8] Typing Signed-off-by: Alex Snaps --- README.md | 2 +- src/data/cel.rs | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index cb525337..a638a363 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ actionSets: - service: ratelimit-service scope: ratelimit-scope-a predicates: - - auth.identity.anonymous == "true" + - auth.identity.anonymous == true data: - expression: key: my_header diff --git a/src/data/cel.rs b/src/data/cel.rs index a45d13b2..a4c01055 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -656,10 +656,8 @@ mod tests { "request.query".into(), "%F0%9F%91%BE".bytes().collect(), ))); - let predicate = Predicate::route_rule( - "decodeQueryString(request.query) == {'👾': ''}", - ) - .expect("This is valid!"); + let predicate = Predicate::route_rule("decodeQueryString(request.query) == {'👾': ''}") + .expect("This is valid!"); assert!(predicate.test()); } From a8632ce208c2f5ab33103b17d5e5df8d6d4c23c8 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 5 Nov 2024 14:48:42 -0500 Subject: [PATCH 6/8] Added second arg to skip param repeating Signed-off-by: Alex Snaps --- src/data/cel.rs | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/src/data/cel.rs b/src/data/cel.rs index a4c01055..882dddb9 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -1,6 +1,6 @@ use crate::data::get_attribute; use crate::data::property::{host_get_map, Path}; -use cel_interpreter::extractors::This; +use cel_interpreter::extractors::{Arguments, This}; use cel_interpreter::objects::{Key, Map, ValueType}; use cel_interpreter::{Context, ExecutionError, ResolveResult, Value}; use cel_parser::{parse, Expression as CelExpression, Member, ParseError}; @@ -85,8 +85,17 @@ impl Expression { /// Decodes the query string and returns a Map where the key is the parameter's name and /// the value is either a [`Value::String`] or a [`Value::List`] if the parameter's name is repeated +/// and the second arg is set not set to `false`. /// see [`tests::decodes_query_string`] -fn decode_query_string(This(s): This>) -> ResolveResult { +fn decode_query_string(This(s): This>, Arguments(args): Arguments) -> ResolveResult { + let allow_repeats = if args.len() == 2 { + match &args[1] { + Value::Bool(b) => *b, + _ => true, + } + } else { + true + }; let mut map: HashMap = HashMap::default(); for part in s.split('&') { let mut kv = part.split('='); @@ -94,14 +103,16 @@ fn decode_query_string(This(s): This>) -> ResolveResult { let new_v: Value = decode(value).unwrap().into_owned().into(); match map.entry(decode(key).unwrap().into_owned().into()) { Entry::Occupied(mut e) => { - if let Value::List(ref mut list) = e.get_mut() { - Arc::get_mut(list) - .expect("This isn't ever shared!") - .push(new_v); - } else { - let v = e.get().clone(); - let list = Value::List([v, new_v].to_vec().into()); - e.insert(list); + if allow_repeats { + if let Value::List(ref mut list) = e.get_mut() { + Arc::get_mut(list) + .expect("This isn't ever shared!") + .push(new_v); + } else { + let v = e.get().clone(); + let list = Value::List([v, new_v].to_vec().into()); + e.insert(list); + } } } Entry::Vacant(e) => { @@ -652,6 +663,20 @@ mod tests { .expect("This is valid!"); assert!(predicate.test()); + property::test::TEST_PROPERTY_VALUE.set(Some(( + "request.query".into(), + "param1=%F0%9F%91%BE%20¶m2=Exterminate%21&%F0%9F%91%BE=123&%F0%9F%91%BE=456&%F0%9F%91%BE" + .bytes() + .collect(), + ))); + let predicate = Predicate::route_rule( + "decodeQueryString(request.query, false)['param2'] == 'Exterminate!' && \ + decodeQueryString(request.query, false)['👾'] == '123' \ + ", + ) + .expect("This is valid!"); + assert!(predicate.test()); + property::test::TEST_PROPERTY_VALUE.set(Some(( "request.query".into(), "%F0%9F%91%BE".bytes().collect(), From ae40336edc995a4191abbfe99cc4a14f13224ca4 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 6 Nov 2024 08:41:54 -0500 Subject: [PATCH 7/8] Default to not consider repeats Signed-off-by: Alex Snaps --- src/data/cel.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/data/cel.rs b/src/data/cel.rs index 882dddb9..c7a80bf0 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -91,10 +91,10 @@ fn decode_query_string(This(s): This>, Arguments(args): Arguments) - let allow_repeats = if args.len() == 2 { match &args[1] { Value::Bool(b) => *b, - _ => true, + _ => false, } } else { - true + false }; let mut map: HashMap = HashMap::default(); for part in s.split('&') { @@ -653,11 +653,11 @@ mod tests { .collect(), ))); let predicate = Predicate::route_rule( - "decodeQueryString(request.query)['param1'] == '👾 ' && \ - decodeQueryString(request.query)['param2'] == 'Exterminate!' && \ - decodeQueryString(request.query)['👾'][0] == '123' && \ - decodeQueryString(request.query)['👾'][1] == '456' && \ - decodeQueryString(request.query)['👾'][2] == '' \ + "decodeQueryString(request.query, true)['param1'] == '👾 ' && \ + decodeQueryString(request.query, true)['param2'] == 'Exterminate!' && \ + decodeQueryString(request.query, true)['👾'][0] == '123' && \ + decodeQueryString(request.query, true)['👾'][1] == '456' && \ + decodeQueryString(request.query, true)['👾'][2] == '' \ ", ) .expect("This is valid!"); From 309271358f59949a0fde727d1e2d908480076ea1 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 6 Nov 2024 14:47:01 -0500 Subject: [PATCH 8/8] Renamed to `decodeQueryString` to `queryMap`, short and internal use only Signed-off-by: Alex Snaps --- src/data/cel.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/data/cel.rs b/src/data/cel.rs index c7a80bf0..0f71ce16 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -73,9 +73,9 @@ impl Expression { Value::resolve(&self.expression, &ctx).expect("Cel expression couldn't be evaluated") } - /// Add support for `decodeQueryString`, see [`decode_query_string`] + /// Add support for `queryMap`, see [`decode_query_string`] fn add_extended_capabilities(ctx: &mut Context) { - ctx.add_function("decodeQueryString", decode_query_string); + ctx.add_function("queryMap", decode_query_string); } fn build_data_map(&self) -> Map { @@ -653,11 +653,11 @@ mod tests { .collect(), ))); let predicate = Predicate::route_rule( - "decodeQueryString(request.query, true)['param1'] == '👾 ' && \ - decodeQueryString(request.query, true)['param2'] == 'Exterminate!' && \ - decodeQueryString(request.query, true)['👾'][0] == '123' && \ - decodeQueryString(request.query, true)['👾'][1] == '456' && \ - decodeQueryString(request.query, true)['👾'][2] == '' \ + "queryMap(request.query, true)['param1'] == '👾 ' && \ + queryMap(request.query, true)['param2'] == 'Exterminate!' && \ + queryMap(request.query, true)['👾'][0] == '123' && \ + queryMap(request.query, true)['👾'][1] == '456' && \ + queryMap(request.query, true)['👾'][2] == '' \ ", ) .expect("This is valid!"); @@ -670,8 +670,8 @@ mod tests { .collect(), ))); let predicate = Predicate::route_rule( - "decodeQueryString(request.query, false)['param2'] == 'Exterminate!' && \ - decodeQueryString(request.query, false)['👾'] == '123' \ + "queryMap(request.query, false)['param2'] == 'Exterminate!' && \ + queryMap(request.query, false)['👾'] == '123' \ ", ) .expect("This is valid!"); @@ -681,8 +681,8 @@ mod tests { "request.query".into(), "%F0%9F%91%BE".bytes().collect(), ))); - let predicate = Predicate::route_rule("decodeQueryString(request.query) == {'👾': ''}") - .expect("This is valid!"); + let predicate = + Predicate::route_rule("queryMap(request.query) == {'👾': ''}").expect("This is valid!"); assert!(predicate.test()); }