From eef53bb609fb5c05d50554c3a889f3a293687768 Mon Sep 17 00:00:00 2001 From: Clement Delafargue Date: Mon, 21 Jul 2025 23:08:27 +0200 Subject: [PATCH 1/2] fix parameter confusion in macros macros resolve scope parameters and regular parameters through the `ToAnyParams` trait, losing the ability to discriminate between the two in a static way. This leads to panics instead of compilation errors. --- biscuit-auth/src/token/builder.rs | 7 -- biscuit-auth/src/token/builder/check.rs | 10 +++ biscuit-auth/src/token/builder/policy.rs | 10 +++ biscuit-auth/src/token/builder/rule.rs | 10 +++ biscuit-auth/tests/macros.rs | 25 ++++++- biscuit-quote/src/lib.rs | 94 +++++++++++++++++++++--- 6 files changed, 137 insertions(+), 19 deletions(-) diff --git a/biscuit-auth/src/token/builder.rs b/biscuit-auth/src/token/builder.rs index 45374641..08ad7ae7 100644 --- a/biscuit-auth/src/token/builder.rs +++ b/biscuit-auth/src/token/builder.rs @@ -173,13 +173,6 @@ pub trait ToAnyParam { fn to_any_param(&self) -> AnyParam; } -#[cfg(feature = "datalog-macro")] -impl ToAnyParam for PublicKey { - fn to_any_param(&self) -> AnyParam { - AnyParam::PublicKey(*self) - } -} - #[cfg(test)] mod tests { use std::{collections::HashMap, convert::TryFrom}; diff --git a/biscuit-auth/src/token/builder/check.rs b/biscuit-auth/src/token/builder/check.rs index 9448a388..46cc1815 100644 --- a/biscuit-auth/src/token/builder/check.rs +++ b/biscuit-auth/src/token/builder/check.rs @@ -111,6 +111,16 @@ impl Check { } } + // TODO maybe introduce a conversion trait to support refs, multiple values, non-pk scopes + #[cfg(feature = "datalog-macro")] + pub fn set_macro_scope_param( + &mut self, + name: &str, + param: PublicKey, + ) -> Result<(), error::Token> { + self.set_scope_lenient(name, param) + } + pub fn validate_parameters(&self) -> Result<(), error::Token> { for rule in &self.queries { rule.validate_parameters()?; diff --git a/biscuit-auth/src/token/builder/policy.rs b/biscuit-auth/src/token/builder/policy.rs index 7679da32..2a1cb478 100644 --- a/biscuit-auth/src/token/builder/policy.rs +++ b/biscuit-auth/src/token/builder/policy.rs @@ -104,6 +104,16 @@ impl Policy { } } + // TODO maybe introduce a conversion trait to support refs, multiple values, non-pk scopes + #[cfg(feature = "datalog-macro")] + pub fn set_macro_scope_param( + &mut self, + name: &str, + param: PublicKey, + ) -> Result<(), error::Token> { + self.set_scope_lenient(name, param) + } + pub fn validate_parameters(&self) -> Result<(), error::Token> { for query in &self.queries { query.validate_parameters()?; diff --git a/biscuit-auth/src/token/builder/rule.rs b/biscuit-auth/src/token/builder/rule.rs index 151e024d..680ba495 100644 --- a/biscuit-auth/src/token/builder/rule.rs +++ b/biscuit-auth/src/token/builder/rule.rs @@ -258,6 +258,16 @@ impl Rule { } } + // TODO maybe introduce a conversion trait to support refs, multiple values, non-pk scopes + #[cfg(feature = "datalog-macro")] + pub fn set_macro_scope_param( + &mut self, + name: &str, + param: PublicKey, + ) -> Result<(), error::Token> { + self.set_scope_lenient(name, param) + } + pub(super) fn apply_parameters(&mut self) { if let Some(parameters) = self.parameters.clone() { self.head.terms = self diff --git a/biscuit-auth/tests/macros.rs b/biscuit-auth/tests/macros.rs index 7a8ec381..7ff2653e 100644 --- a/biscuit-auth/tests/macros.rs +++ b/biscuit-auth/tests/macros.rs @@ -2,7 +2,7 @@ * Copyright (c) 2019 Geoffroy Couprie and Contributors to the Eclipse Foundation. * SPDX-License-Identifier: Apache-2.0 */ -use biscuit_auth::{builder, datalog::RunLimits, KeyPair}; +use biscuit_auth::{builder, datalog::RunLimits, KeyPair, PublicKey}; use biscuit_quote::{ authorizer, authorizer_merge, biscuit, biscuit_merge, block, block_merge, check, fact, policy, rule, @@ -314,3 +314,26 @@ fn ecdsa() { r#"rule($0, true) <- fact($0, $1, $2, "my_value", {0}) trusting secp256r1/0245dd01132962da3812911b746b080aed714873c1812e7cefacf13e3880712da0"#, ); } + +#[test] +fn trusting() { + // this should only work with a proper `PublicKey` value, and fail when trying to provide a string instead + let pubkey: PublicKey = + "secp256r1/0245dd01132962da3812911b746b080aed714873c1812e7cefacf13e3880712da0" + .parse() + .unwrap(); + let _ = authorizer!( + r#" + nonce("a"); operation("o"); pathname("p"); + d($x) <- nonce($x) trusting {pubkey} + "# + ); + let _ = rule!( + r#" + data($nonce, $operation, $pathname) + <- nonce($nonce), operation($operation), pathname($pathname) + + trusting {pubkey} + "#, + ); +} diff --git a/biscuit-quote/src/lib.rs b/biscuit-quote/src/lib.rs index fca0aca6..af0dd74b 100644 --- a/biscuit-quote/src/lib.rs +++ b/biscuit-quote/src/lib.rs @@ -204,6 +204,8 @@ struct Builder { // parameters used in the datalog source pub datalog_parameters: HashSet, + // scope parameters used in the datalog source + pub datalog_scope_parameters: HashSet, // parameters provided to the macro pub macro_parameters: HashSet, @@ -227,6 +229,7 @@ impl Builder { parameters, datalog_parameters: HashSet::new(), + datalog_scope_parameters: HashSet::new(), macro_parameters, facts: Vec::new(), @@ -286,7 +289,8 @@ impl Builder { } if let Some(parameters) = &rule.scope_parameters { - self.datalog_parameters.extend(parameters.keys().cloned()); + self.datalog_scope_parameters + .extend(parameters.keys().cloned()); } } @@ -316,12 +320,17 @@ impl Builder { } fn validate(&self) -> Result<(), error::LanguageError> { - if self.macro_parameters.is_subset(&self.datalog_parameters) { + let all_parameters = self + .datalog_parameters + .union(&self.datalog_scope_parameters) + .cloned() + .collect(); + if self.macro_parameters.is_subset(&all_parameters) { Ok(()) } else { let unused_parameters: Vec = self .macro_parameters - .difference(&self.datalog_parameters) + .difference(&all_parameters) .cloned() .collect(); Err(error::LanguageError::Parameters { @@ -334,6 +343,7 @@ impl Builder { struct Item { parameters: HashSet, + scope_parameters: HashSet, start: TokenStream, middle: TokenStream, end: TokenStream, @@ -348,6 +358,7 @@ impl Item { .flatten() .map(|(name, _)| name.to_owned()) .collect(), + scope_parameters: HashSet::new(), start: quote! { let mut __biscuit_auth_item = #fact; }, @@ -360,6 +371,7 @@ impl Item { fn rule(rule: &Rule) -> Self { Self { parameters: Item::rule_params(rule).collect(), + scope_parameters: Item::rule_scope_params(rule).collect(), start: quote! { let mut __biscuit_auth_item = #rule; }, @@ -373,6 +385,11 @@ impl Item { fn check(check: &Check) -> Self { Self { parameters: check.queries.iter().flat_map(Item::rule_params).collect(), + scope_parameters: check + .queries + .iter() + .flat_map(Item::rule_scope_params) + .collect(), start: quote! { let mut __biscuit_auth_item = #check; }, @@ -386,6 +403,11 @@ impl Item { fn policy(policy: &Policy) -> Self { Self { parameters: policy.queries.iter().flat_map(Item::rule_params).collect(), + scope_parameters: policy + .queries + .iter() + .flat_map(Item::rule_scope_params) + .collect(), start: quote! { let mut __biscuit_auth_item = #policy; }, @@ -400,19 +422,22 @@ impl Item { rule.parameters .iter() .flatten() - .map(|(name, _)| name.as_ref()) - .chain( - rule.scope_parameters - .iter() - .flatten() - .map(|(name, _)| name.as_ref()), - ) - .map(str::to_owned) + .map(|(name, _)| name.to_string()) + } + + fn rule_scope_params(rule: &Rule) -> impl Iterator + '_ { + rule.scope_parameters + .iter() + .flatten() + .map(|(name, _)| name.to_string()) } fn needs_param(&self, name: &str) -> bool { self.parameters.contains(name) } + fn needs_scope_param(&self, name: &str) -> bool { + self.scope_parameters.contains(name) + } fn add_param(&mut self, name: &str, clone: bool) { let ident = Ident::new(name, Span::call_site()); @@ -427,6 +452,20 @@ impl Item { __biscuit_auth_item.set_macro_param(#name, #expr).unwrap(); }); } + + fn add_scope_param(&mut self, name: &str, clone: bool) { + let ident = Ident::new(name, Span::call_site()); + + let expr = if clone { + quote! { ::core::clone::Clone::clone(&#ident) } + } else { + quote! { #ident } + }; + + self.middle.extend(quote! { + __biscuit_auth_item.set_macro_scope_param(#name, #expr).unwrap(); + }); + } } impl ToTokens for Item { @@ -477,6 +516,21 @@ impl ToTokens for Builder { } } + for param in &self.datalog_scope_parameters { + let mut items = items + .iter_mut() + .filter(|i| i.needs_scope_param(param)) + .peekable(); + + loop { + match (items.next(), items.peek()) { + (Some(cur), Some(_next)) => cur.add_scope_param(param, true), + (Some(cur), None) => cur.add_scope_param(param, false), + (None, _) => break, + } + } + } + let builder_type = &self.builder_type; let builder_quote = if let Some(target) = &self.target { quote! { @@ -558,6 +612,12 @@ pub fn rule(input: proc_macro::TokenStream) -> proc_macro::TokenStream { } } + for param in &builder.datalog_scope_parameters { + if rule_item.needs_scope_param(param) { + rule_item.add_scope_param(param, false); + } + } + (quote! { { #params_quote @@ -694,6 +754,12 @@ pub fn check(input: proc_macro::TokenStream) -> proc_macro::TokenStream { } } + for param in &builder.datalog_scope_parameters { + if check_item.needs_scope_param(param) { + check_item.add_scope_param(param, false); + } + } + (quote! { { #params_quote @@ -766,6 +832,12 @@ pub fn policy(input: proc_macro::TokenStream) -> proc_macro::TokenStream { } } + for param in &builder.datalog_scope_parameters { + if policy_item.needs_scope_param(param) { + policy_item.add_scope_param(param, false); + } + } + (quote! { { #params_quote From b99af0b346c70424ae8143976200266296648621 Mon Sep 17 00:00:00 2001 From: Clement Delafargue Date: Thu, 7 Aug 2025 17:18:43 +0200 Subject: [PATCH 2/2] replace `ToAnyParam` with `Into` in macro calls This removes the possibility to provide a public key where a term is expected and leverages a standard trait instead of a custom one --- biscuit-auth/src/token/builder/check.rs | 11 ++--------- biscuit-auth/src/token/builder/fact.rs | 11 ++--------- biscuit-auth/src/token/builder/policy.rs | 11 ++--------- biscuit-auth/src/token/builder/rule.rs | 11 ++--------- 4 files changed, 8 insertions(+), 36 deletions(-) diff --git a/biscuit-auth/src/token/builder/check.rs b/biscuit-auth/src/token/builder/check.rs index 46cc1815..5d80121e 100644 --- a/biscuit-auth/src/token/builder/check.rs +++ b/biscuit-auth/src/token/builder/check.rs @@ -11,8 +11,6 @@ use crate::{ error, PublicKey, }; -#[cfg(feature = "datalog-macro")] -use super::ToAnyParam; use super::{display_rule_body, Convert, Rule, Term}; /// Builder for a Biscuit check @@ -98,17 +96,12 @@ impl Check { } #[cfg(feature = "datalog-macro")] - pub fn set_macro_param( + pub fn set_macro_param>( &mut self, name: &str, param: T, ) -> Result<(), error::Token> { - use super::AnyParam; - - match param.to_any_param() { - AnyParam::Term(t) => self.set_lenient(name, t), - AnyParam::PublicKey(p) => self.set_scope_lenient(name, p), - } + self.set_lenient(name, param.into()) } // TODO maybe introduce a conversion trait to support refs, multiple values, non-pk scopes diff --git a/biscuit-auth/src/token/builder/fact.rs b/biscuit-auth/src/token/builder/fact.rs index 4a247f87..9156b635 100644 --- a/biscuit-auth/src/token/builder/fact.rs +++ b/biscuit-auth/src/token/builder/fact.rs @@ -11,8 +11,6 @@ use crate::{ error, }; -#[cfg(feature = "datalog-macro")] -use super::ToAnyParam; use super::{Convert, Predicate, Term}; /// Builder for a Datalog fact @@ -115,17 +113,12 @@ impl Fact { } #[cfg(feature = "datalog-macro")] - pub fn set_macro_param( + pub fn set_macro_param>( &mut self, name: &str, param: T, ) -> Result<(), error::Token> { - use super::AnyParam; - - match param.to_any_param() { - AnyParam::Term(t) => self.set_lenient(name, t), - AnyParam::PublicKey(_) => Ok(()), - } + self.set_lenient(name, param.into()) } pub(super) fn apply_parameters(&mut self) { diff --git a/biscuit-auth/src/token/builder/policy.rs b/biscuit-auth/src/token/builder/policy.rs index 2a1cb478..1a7013f6 100644 --- a/biscuit-auth/src/token/builder/policy.rs +++ b/biscuit-auth/src/token/builder/policy.rs @@ -8,8 +8,6 @@ use nom::Finish; use crate::{error, PublicKey}; -#[cfg(feature = "datalog-macro")] -use super::ToAnyParam; use super::{display_rule_body, Rule, Term}; #[derive(Debug, Clone, PartialEq, Eq)] @@ -91,17 +89,12 @@ impl Policy { } #[cfg(feature = "datalog-macro")] - pub fn set_macro_param( + pub fn set_macro_param>( &mut self, name: &str, param: T, ) -> Result<(), error::Token> { - use super::AnyParam; - - match param.to_any_param() { - AnyParam::Term(t) => self.set_lenient(name, t), - AnyParam::PublicKey(p) => self.set_scope_lenient(name, p), - } + self.set_lenient(name, param.into()) } // TODO maybe introduce a conversion trait to support refs, multiple values, non-pk scopes diff --git a/biscuit-auth/src/token/builder/rule.rs b/biscuit-auth/src/token/builder/rule.rs index 680ba495..67053761 100644 --- a/biscuit-auth/src/token/builder/rule.rs +++ b/biscuit-auth/src/token/builder/rule.rs @@ -11,8 +11,6 @@ use crate::{ error, PublicKey, }; -#[cfg(feature = "datalog-macro")] -use super::ToAnyParam; use super::{Convert, Expression, Predicate, Scope, Term}; /// Builder for a Datalog rule @@ -245,17 +243,12 @@ impl Rule { } #[cfg(feature = "datalog-macro")] - pub fn set_macro_param( + pub fn set_macro_param>( &mut self, name: &str, param: T, ) -> Result<(), error::Token> { - use super::AnyParam; - - match param.to_any_param() { - AnyParam::Term(t) => self.set_lenient(name, t), - AnyParam::PublicKey(pubkey) => self.set_scope_lenient(name, pubkey), - } + self.set_lenient(name, param.into()) } // TODO maybe introduce a conversion trait to support refs, multiple values, non-pk scopes