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

@@ -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

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<syn::Attribute> {
+        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 {

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<syn::Attribute> {
+        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() {

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<syn::ItemMod> 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 = <ir::ItemMod as TryFrom<syn::ItemMod>>::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 = <ir::ItemMod as TryFrom<syn::ItemMod>>::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 = <ir::ItemMod as TryFrom<syn::ItemMod>>::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 = <ir::ItemMod as TryFrom<syn::ItemMod>>::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."));
+    }
 }

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<syn::Attribute> {
+    attrs
+        .iter()
+        .filter(|a| a.path().is_ident(super::CFG_IDENT))
+        .cloned()
+        .collect()
+}

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() {}

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"))]
+   |                         ^^^^^^^^^^^^^

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() {}

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"))]
+   |                   ^^^^^^^

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() {}

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")))]
+   |                       ^^^^^^^