From 93d1c3ececa5d70be84ed0de404429248c85393e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20M=C3=BCller?= Date: Tue, 3 Dec 2024 08:00:21 +0100 Subject: [PATCH] Restrict which `cfg` attributes can be used (#2313) * Allow only whitelisted `cfg` attributes * Add tests * Update changelog * Add unit tests * Panic at unreachable code * Adjust for testing constructor instead --- CHANGELOG.md | 5 + crates/ink/ir/src/ir/item_impl/constructor.rs | 10 +- crates/ink/ir/src/ir/item_impl/message.rs | 10 +- crates/ink/ir/src/ir/item_mod.rs | 185 ++++++++++++++++++ crates/ink/ir/src/ir/utils.rs | 9 + .../contract/fail/cfg-forbidden-usage-01.rs | 21 ++ .../fail/cfg-forbidden-usage-01.stderr | 12 ++ .../contract/fail/cfg-forbidden-usage-02.rs | 21 ++ .../fail/cfg-forbidden-usage-02.stderr | 12 ++ .../contract/fail/cfg-forbidden-usage-03.rs | 21 ++ .../fail/cfg-forbidden-usage-03.stderr | 12 ++ .../contract/fail/cfg-forbidden-usage-04.rs | 21 ++ .../fail/cfg-forbidden-usage-04.stderr | 12 ++ .../contract/fail/cfg-forbidden-usage-05.rs | 21 ++ .../fail/cfg-forbidden-usage-05.stderr | 12 ++ .../contract/fail/cfg-forbidden-usage-06.rs | 21 ++ .../fail/cfg-forbidden-usage-06.stderr | 12 ++ .../ui/contract/pass/cfg-allowed-usage-01.rs | 37 ++++ .../ui/contract/pass/cfg-allowed-usage-02.rs | 27 +++ 19 files changed, 479 insertions(+), 2 deletions(-) create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-01.rs create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-01.stderr create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-02.rs create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-02.stderr create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-03.rs create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-03.stderr create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-04.rs create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-04.stderr create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-05.rs create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-05.stderr create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-06.rs create mode 100644 crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-06.stderr create mode 100644 crates/ink/tests/ui/contract/pass/cfg-allowed-usage-01.rs create mode 100644 crates/ink/tests/ui/contract/pass/cfg-allowed-usage-02.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index b8cbc62879d..e9f8ff4054f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +[Unreleased] + +## Changed +- Restrict which `cfg` attributes can be used ‒ [#2313](https://github.com/use-ink/ink/pull/2313) + ## Version 5.1.0 This is the first ink! release outside of Parity. ink! was started at Parity and diff --git a/crates/ink/ir/src/ir/item_impl/constructor.rs b/crates/ink/ir/src/ir/item_impl/constructor.rs index 0319f158e5a..1955334ec41 100644 --- a/crates/ink/ir/src/ir/item_impl/constructor.rs +++ b/crates/ink/ir/src/ir/item_impl/constructor.rs @@ -23,7 +23,10 @@ use crate::{ ir, ir::{ attrs::SelectorOrWildcard, - utils::extract_cfg_attributes, + utils::{ + extract_cfg_attributes, + extract_cfg_syn_attributes, + }, }, }; use proc_macro2::{ @@ -237,6 +240,11 @@ impl Constructor { extract_cfg_attributes(self.attrs(), span) } + /// Returns a list of `cfg` attributes as `syn::Attribute` if any. + pub fn get_cfg_syn_attrs(&self) -> Vec { + extract_cfg_syn_attributes(self.attrs()) + } + /// Returns the return type of the ink! constructor if any. pub fn output(&self) -> Option<&syn::Type> { match &self.item.sig.output { diff --git a/crates/ink/ir/src/ir/item_impl/message.rs b/crates/ink/ir/src/ir/item_impl/message.rs index 70e7be654ef..47d3ab71c4c 100644 --- a/crates/ink/ir/src/ir/item_impl/message.rs +++ b/crates/ink/ir/src/ir/item_impl/message.rs @@ -23,7 +23,10 @@ use crate::ir::{ self, attrs::SelectorOrWildcard, utils, - utils::extract_cfg_attributes, + utils::{ + extract_cfg_attributes, + extract_cfg_syn_attributes, + }, }; use proc_macro2::{ Ident, @@ -283,6 +286,11 @@ impl Message { extract_cfg_attributes(self.attrs(), span) } + /// Returns a list of `cfg` attributes as `syn::Attribute` if any. + pub fn get_cfg_syn_attrs(&self) -> Vec { + extract_cfg_syn_attributes(self.attrs()) + } + /// Returns the `self` receiver of the ink! message. pub fn receiver(&self) -> Receiver { match self.item.sig.inputs.iter().next() { diff --git a/crates/ink/ir/src/ir/item_mod.rs b/crates/ink/ir/src/ir/item_mod.rs index 9754a934b28..865bd2f8aee 100644 --- a/crates/ink/ir/src/ir/item_mod.rs +++ b/crates/ink/ir/src/ir/item_mod.rs @@ -239,6 +239,96 @@ impl ItemMod { Ok(()) } + /// Ensures that the `#[cfg(…)]` contains only valid attributes. + /// + /// # Note + /// + /// This restriction was added to prevent contract developers from + /// adding public constructors/messages that don't show up in the + /// ink! metadata, but are compiled into the Wasm. + /// + /// Or formulated differently: we allow only `#[cfg(…)]`'s that don't + /// allow differentiating between compiling for Wasm vs. native. + /// + /// Without this restriction users that view the metadata can be + /// deceived as to what functions the contract provides to the public. + fn ensure_only_allowed_cfgs(items: &[ir::Item]) -> Result<(), syn::Error> { + const ERR_HELP: &str = "Allowed in `#[cfg(…)]`:\n\ + - `test`\n\ + - `feature` (without `std`)\n\ + - `any`\n\ + - `not`\n\ + - `all`"; + + fn verify_attr(a: &syn::Attribute) -> Result<(), syn::Error> { + match &a.meta { + syn::Meta::List(list) => { + if let Some(ident) = list.path.get_ident() { + if ident.eq("cfg") { + return list.parse_nested_meta(verify_cfg_attrs); + } + } + unreachable!("`verify_attr` can only be called for `#[cfg(…)]`, not for other `List`"); + } + syn::Meta::Path(_) => { + // not relevant, we are only looking at `#[cfg(…)]` + unreachable!("`verify_attr` can only be called for `#[cfg(…)]`, not for `Path`"); + } + syn::Meta::NameValue(_) => { + // not relevant, we are only looking at `#[cfg(…)]` + unreachable!("`verify_attr` can only be called for `#[cfg(…)]`, not for `NameValue`"); + } + } + } + + fn verify_cfg_attrs(meta: syn::meta::ParseNestedMeta) -> Result<(), syn::Error> { + if meta.path.is_ident("test") { + return Ok(()); + } + if meta.path.is_ident("any") + || meta.path.is_ident("all") + || meta.path.is_ident("not") + { + return meta.parse_nested_meta(verify_cfg_attrs); + } + + if meta.path.is_ident("feature") { + let value = meta.value()?; + let value: syn::LitStr = value.parse()?; + if value.value().eq("std") { + return Err(format_err_spanned!( + meta.path, + "The feature `std` is not allowed in `cfg`.\n\n{ERR_HELP}" + )) + } + return Ok(()); + } + + Err(format_err_spanned!( + meta.path, + "This `cfg` attribute is not allowed.\n\n{ERR_HELP}" + )) + } + + for item_impl in items + .iter() + .filter_map(ir::Item::map_ink_item) + .filter_map(ir::InkItem::filter_map_impl_block) + { + for message in item_impl.iter_messages() { + for a in message.get_cfg_syn_attrs() { + verify_attr(&a)?; + } + } + for constructor in item_impl.iter_constructors() { + for a in constructor.get_cfg_syn_attrs() { + verify_attr(&a)?; + } + } + } + Ok(()) + } + /// Ensures that: /// - At most one wildcard selector exists among ink! messages, as well as ink! /// constructors. @@ -383,6 +473,7 @@ impl TryFrom for ItemMod { Self::ensure_contains_constructor(module_span, &items)?; Self::ensure_no_overlapping_selectors(&items)?; Self::ensure_valid_wildcard_selector_usage(&items)?; + Self::ensure_only_allowed_cfgs(&items)?; Ok(Self { attrs: other_attrs, vis: module.vis, @@ -1170,4 +1261,98 @@ mod tests { wildcard `selector = _` defined", ) } + + #[test] + fn cfg_feature_std_not_allowed() { + let item_mod = syn::parse_quote! { + mod my_module { + #[ink(storage)] + pub struct MyStorage {} + + impl MyStorage { + #[ink(constructor)] + pub fn my_constructor() -> Self {} + + #[ink(message)] + #[cfg(feature = "std")] + pub fn not_allowed(&self) {} + } + } + }; + let res = >::try_from(item_mod) + .map_err(|err| err.to_string()); + assert!(res.is_err()); + assert!(res + .unwrap_err() + .starts_with("The feature `std` is not allowed in `cfg`.")); + } + + #[test] + fn cfg_feature_other_than_std_allowed() { + let item_mod = syn::parse_quote! { + mod my_module { + #[ink(storage)] + pub struct MyStorage {} + + impl MyStorage { + #[ink(constructor)] + pub fn my_constructor() -> Self {} + + #[ink(message)] + #[cfg(feature = "foo")] + pub fn not_allowed(&self) {} + } + } + }; + let res = >::try_from(item_mod) + .map_err(|err| err.to_string()); + assert!(res.is_ok()); + } + + #[test] + fn cfg_test_allowed() { + let item_mod = syn::parse_quote! { + mod my_module { + #[ink(storage)] + pub struct MyStorage {} + + impl MyStorage { + #[ink(constructor)] + pub fn my_constructor() -> Self {} + + #[ink(message)] + #[cfg(test)] + pub fn not_allowed(&self) {} + } + } + }; + let res = >::try_from(item_mod) + .map_err(|err| err.to_string()); + assert!(res.is_ok()); + } + + #[test] + fn cfg_nested_forbidden_must_be_found() { + let item_mod = syn::parse_quote! { + mod my_module { + #[ink(storage)] + pub struct MyStorage {} + + impl MyStorage { + #[ink(constructor)] + #[cfg(any(not(target_os = "wasm")))] + pub fn my_constructor() -> Self {} + + #[ink(message)] + pub fn not_allowed(&self) {} + } + } + }; + let res = >::try_from(item_mod) + .map_err(|err| err.to_string()); + assert!(res.is_err()); + assert!(res + .unwrap_err() + .starts_with("This `cfg` attribute is not allowed.")); + } } diff --git a/crates/ink/ir/src/ir/utils.rs b/crates/ink/ir/src/ir/utils.rs index 638886c7c49..5854dbfdb9a 100644 --- a/crates/ink/ir/src/ir/utils.rs +++ b/crates/ink/ir/src/ir/utils.rs @@ -177,3 +177,12 @@ pub fn extract_cfg_attributes( .map(|a| quote::quote_spanned!(span=> #a )) .collect() } + +/// Extracts `cfg` attributes from the given set of attributes +pub fn extract_cfg_syn_attributes(attrs: &[syn::Attribute]) -> Vec { + attrs + .iter() + .filter(|a| a.path().is_ident(super::CFG_IDENT)) + .cloned() + .collect() +} diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-01.rs b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-01.rs new file mode 100644 index 00000000000..639c5cc0080 --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-01.rs @@ -0,0 +1,21 @@ +#[ink::contract] +mod contract { + #[ink(storage)] + pub struct Contract {} + + impl Contract { + #[ink(constructor)] + pub fn constructor() -> Self { + Self {} + } + + #[ink(message)] + pub fn message1(&self) {} + + #[ink(message)] + #[cfg(any(test, target_family = "wasm"))] + pub fn message2(&self) {} + } +} + +fn main() {} diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-01.stderr b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-01.stderr new file mode 100644 index 00000000000..3fca50b6209 --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-01.stderr @@ -0,0 +1,12 @@ +error: This `cfg` attribute is not allowed. + + Allowed in `#[cfg(…)]`: + - `test` + - `feature` (without `std`) + - `any` + - `not` + - `all` + --> tests/ui/contract/fail/cfg-forbidden-usage-01.rs:16:25 + | +16 | #[cfg(any(test, target_family = "wasm"))] + | ^^^^^^^^^^^^^ diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-02.rs b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-02.rs new file mode 100644 index 00000000000..a926fbeae5d --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-02.rs @@ -0,0 +1,21 @@ +#[ink::contract] +mod contract { + #[ink(storage)] + pub struct Contract {} + + impl Contract { + #[ink(constructor)] + pub fn constructor() -> Self { + Self {} + } + + #[ink(message)] + pub fn message1(&self) {} + + #[ink(message)] + #[cfg(not(feature = "std"))] + pub fn message2(&self) {} + } +} + +fn main() {} diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-02.stderr b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-02.stderr new file mode 100644 index 00000000000..a3f5a4c7d81 --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-02.stderr @@ -0,0 +1,12 @@ +error: The feature `std` is not allowed in `cfg`. + + Allowed in `#[cfg(…)]`: + - `test` + - `feature` (without `std`) + - `any` + - `not` + - `all` + --> tests/ui/contract/fail/cfg-forbidden-usage-02.rs:16:19 + | +16 | #[cfg(not(feature = "std"))] + | ^^^^^^^ diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-03.rs b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-03.rs new file mode 100644 index 00000000000..428138f7230 --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-03.rs @@ -0,0 +1,21 @@ +#[ink::contract] +mod contract { + #[ink(storage)] + pub struct Contract {} + + impl Contract { + #[ink(constructor)] + pub fn constructor() -> Self { + Self {} + } + + #[ink(message)] + pub fn message1(&self) {} + + #[ink(message)] + #[cfg(any(not(feature = "std")))] + pub fn message2(&self) {} + } +} + +fn main() {} diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-03.stderr b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-03.stderr new file mode 100644 index 00000000000..d376edfbc3a --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-03.stderr @@ -0,0 +1,12 @@ +error: The feature `std` is not allowed in `cfg`. + + Allowed in `#[cfg(…)]`: + - `test` + - `feature` (without `std`) + - `any` + - `not` + - `all` + --> tests/ui/contract/fail/cfg-forbidden-usage-03.rs:16:23 + | +16 | #[cfg(any(not(feature = "std")))] + | ^^^^^^^ diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-04.rs b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-04.rs new file mode 100644 index 00000000000..6cefb0115fc --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-04.rs @@ -0,0 +1,21 @@ +#[ink::contract] +mod contract { + #[ink(storage)] + pub struct Contract {} + + impl Contract { + #[cfg(any(test, target_family = "wasm", feature = "std"))] + #[ink(constructor)] + pub fn constructor() -> Self { + Self {} + } + + #[ink(message)] + pub fn message1(&self) {} + + #[ink(message)] + pub fn message2(&self) {} + } +} + +fn main() {} diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-04.stderr b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-04.stderr new file mode 100644 index 00000000000..d56a3dd44a5 --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-04.stderr @@ -0,0 +1,12 @@ +error: This `cfg` attribute is not allowed. + + Allowed in `#[cfg(…)]`: + - `test` + - `feature` (without `std`) + - `any` + - `not` + - `all` + --> tests/ui/contract/fail/cfg-forbidden-usage-04.rs:7:25 + | +7 | #[cfg(any(test, target_family = "wasm", feature = "std"))] + | ^^^^^^^^^^^^^ diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-05.rs b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-05.rs new file mode 100644 index 00000000000..bacde3f86d7 --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-05.rs @@ -0,0 +1,21 @@ +#[ink::contract] +mod contract { + #[ink(storage)] + pub struct Contract {} + + impl Contract { + #[ink(constructor)] + pub fn constructor() -> Self { + Self {} + } + + #[ink(message)] + pub fn message1(&self) {} + + #[ink(message)] + #[cfg(target_pointer_width = "32")] + pub fn message2(&self) {} + } +} + +fn main() {} diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-05.stderr b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-05.stderr new file mode 100644 index 00000000000..0047a0fbd8a --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-05.stderr @@ -0,0 +1,12 @@ +error: This `cfg` attribute is not allowed. + + Allowed in `#[cfg(…)]`: + - `test` + - `feature` (without `std`) + - `any` + - `not` + - `all` + --> tests/ui/contract/fail/cfg-forbidden-usage-05.rs:16:15 + | +16 | #[cfg(target_pointer_width = "32")] + | ^^^^^^^^^^^^^^^^^^^^ diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-06.rs b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-06.rs new file mode 100644 index 00000000000..1f626401db1 --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-06.rs @@ -0,0 +1,21 @@ +#[ink::contract(keep_attr = "cfg")] +mod contract { + #[ink(storage)] + pub struct Contract {} + + impl Contract { + #[ink(constructor)] + pub fn constructor() -> Self { + Self {} + } + + #[ink(message)] + pub fn message1(&self) {} + + #[ink(message)] + #[cfg(target_os = "wasm")] + pub fn message2(&self) {} + } +} + +fn main() {} diff --git a/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-06.stderr b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-06.stderr new file mode 100644 index 00000000000..ede0f6d3960 --- /dev/null +++ b/crates/ink/tests/ui/contract/fail/cfg-forbidden-usage-06.stderr @@ -0,0 +1,12 @@ +error: This `cfg` attribute is not allowed. + + Allowed in `#[cfg(…)]`: + - `test` + - `feature` (without `std`) + - `any` + - `not` + - `all` + --> tests/ui/contract/fail/cfg-forbidden-usage-06.rs:16:15 + | +16 | #[cfg(target_os = "wasm")] + | ^^^^^^^^^ diff --git a/crates/ink/tests/ui/contract/pass/cfg-allowed-usage-01.rs b/crates/ink/tests/ui/contract/pass/cfg-allowed-usage-01.rs new file mode 100644 index 00000000000..94c34474f9f --- /dev/null +++ b/crates/ink/tests/ui/contract/pass/cfg-allowed-usage-01.rs @@ -0,0 +1,37 @@ +#[ink::contract] +mod contract { + #[ink(storage)] + pub struct Contract {} + + impl Contract { + #[ink(constructor)] + pub fn constructor() -> Self { + Self {} + } + + #[cfg(any(test, feature = "ink-debug"))] + #[ink(constructor)] + pub fn constructor2() -> Self { + Self {} + } + + #[cfg(feature = "ink-debug")] + #[ink(constructor)] + pub fn constructor3() -> Self { + Self {} + } + + #[ink(message)] + pub fn message1(&self) {} + + #[ink(message)] + #[cfg(any(test, feature = "ink-debug"))] + pub fn message2(&self) {} + + #[ink(message)] + #[cfg(feature = "ink-debug")] + pub fn message3(&self) {} + } +} + +fn main() {} diff --git a/crates/ink/tests/ui/contract/pass/cfg-allowed-usage-02.rs b/crates/ink/tests/ui/contract/pass/cfg-allowed-usage-02.rs new file mode 100644 index 00000000000..65e44806121 --- /dev/null +++ b/crates/ink/tests/ui/contract/pass/cfg-allowed-usage-02.rs @@ -0,0 +1,27 @@ +#[ink::contract] +mod contract { + #[ink(storage)] + pub struct Contract {} + + impl Contract { + #[ink(constructor)] + pub fn constructor() -> Self { + Self {} + } + + #[cfg(test)] + #[ink(constructor)] + pub fn constructor2() -> Self { + Self {} + } + + #[ink(message)] + pub fn message1(&self) {} + + #[ink(message)] + #[cfg(test)] + pub fn message2(&self) {} + } +} + +fn main() {}