lang: Error on undocumented unsafe account types (otter-sec#1452)

Author: tomlinton

.github/workflows/tests.yaml

@@ -278,6 +278,8 @@ jobs:
             path: tests/auction-house
           - cmd: cd tests/floats && yarn && anchor test
             path: tests/floats
+          - cmd: cd tests/safety-checks && ./test.sh
+            path: tests/safety-checks
     steps:
       - uses: actions/checkout@v2
       - uses: ./.github/actions/setup/

CHANGELOG.md

@@ -24,6 +24,7 @@ incremented for features.
 
 * lang: Enforce that the payer for an init-ed account be marked `mut` ([#1271](https://github.com/project-serum/anchor/pull/1271)).
 * lang: All error-related code is now in the error module ([#1426](https://github.com/project-serum/anchor/pull/1426)).
+* lang: Require doc comments when using AccountInfo or UncheckedAccount types ([#1452](https://github.com/project-serum/anchor/pull/1452)).
 
 ## [0.21.0] - 2022-02-07
 

cli/src/config.rs

@@ -166,6 +166,7 @@ impl WithPath<Config> {
                 path.join("src/lib.rs"),
                 version,
                 self.features.seeds,
+                false,
             )?;
             r.push(Program {
                 lib_name,
@@ -256,9 +257,26 @@ pub struct Config {
     pub test: Option<Test>,
 }
 
-#[derive(Default, Clone, Debug, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct FeaturesConfig {
+    #[serde(default)]
     pub seeds: bool,
+    #[serde(default = "default_safety_checks")]
+    pub safety_checks: bool,
+}
+
+impl Default for FeaturesConfig {
+    fn default() -> Self {
+        Self {
+            seeds: false,
+            // Anchor safety checks on by default
+            safety_checks: true,
+        }
+    }
+}
+
+fn default_safety_checks() -> bool {
+    true
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize)]

cli/src/lib.rs

@@ -1388,7 +1388,12 @@ fn extract_idl(cfg: &WithPath<Config>, file: &str) -> Result<Option<Idl>> {
     let manifest_from_path = std::env::current_dir()?.join(PathBuf::from(&*file).parent().unwrap());
     let cargo = Manifest::discover_from_path(manifest_from_path)?
         .ok_or_else(|| anyhow!("Cargo.toml not found"))?;
-    anchor_syn::idl::file::parse(&*file, cargo.version(), cfg.features.seeds)
+    anchor_syn::idl::file::parse(
+        &*file,
+        cargo.version(),
+        cfg.features.seeds,
+        cfg.features.safety_checks,
+    )
 }
 
 fn idl(cfg_override: &ConfigOverride, subcmd: IdlCommand) -> Result<()> {

lang/syn/Cargo.toml

@@ -16,7 +16,7 @@ anchor-debug = []
 seeds = []
 
 [dependencies]
-proc-macro2 = "1.0"
+proc-macro2 = { version = "1.0", features=["span-locations"]}
 proc-macro2-diagnostics = "0.9"
 quote = "1.0"
 syn = { version = "1.0.60", features = ["full", "extra-traits", "parsing"] }

lang/syn/src/idl/file.rs

@@ -18,8 +18,12 @@ pub fn parse(
     filename: impl AsRef<Path>,
     version: String,
     seeds_feature: bool,
+    safety_checks: bool,
 ) -> Result<Option<Idl>> {
     let ctx = CrateContext::parse(filename)?;
+    if safety_checks {
+        ctx.safety_checks()?;
+    }
 
     let program_mod = match parse_program_mod(&ctx) {
         None => return Ok(None),

lang/syn/src/parser/context.rs

@@ -1,6 +1,6 @@
+use anyhow::anyhow;
 use std::collections::BTreeMap;
 use std::path::{Path, PathBuf};
-
 use syn::parse::{Error as ParseError, Result as ParseResult};
 
 /// Crate parse context
@@ -40,6 +40,41 @@ impl CrateContext {
             modules: ParsedModule::parse_recursive(root.as_ref())?,
         })
     }
+
+    // Perform Anchor safety checks on the parsed create
+    pub fn safety_checks(&self) -> Result<(), anyhow::Error> {
+        // Check all structs for unsafe field types, i.e. AccountInfo and UncheckedAccount.
+        for (_, ctx) in self.modules.iter() {
+            for unsafe_field in ctx.unsafe_struct_fields() {
+                // Check if unsafe field type has been documented with a /// SAFETY: doc string.
+                let is_documented = unsafe_field.attrs.iter().any(|attr| {
+                    attr.tokens.clone().into_iter().any(|token| match token {
+                        // Check for doc comments containing CHECK
+                        proc_macro2::TokenTree::Literal(s) => s.to_string().contains("CHECK"),
+                        _ => false,
+                    })
+                });
+                if !is_documented {
+                    let ident = unsafe_field.ident.as_ref().unwrap();
+                    let span = ident.span();
+                    // Error if undocumented.
+                    return Err(anyhow!(
+                        r#"
+        {}:{}:{}
+        Struct field "{}" is unsafe, but is not documented.
+        Please add a `/// CHECK:` doc comment explaining why no checks through types are necessary.
+        See https://book.anchor-lang.com/chapter_3/the_accounts_struct.html#safety-checks for more information.
+                    "#,
+                        ctx.file.canonicalize().unwrap().display(),
+                        span.start().line,
+                        span.start().column,
+                        ident.to_string()
+                    ));
+                };
+            }
+        }
+        Ok(())
+    }
 }
 
 /// Module parse context
@@ -181,6 +216,21 @@ impl ParsedModule {
         })
     }
 
+    fn unsafe_struct_fields(&self) -> impl Iterator<Item = &syn::Field> {
+        self.structs()
+            .flat_map(|s| &s.fields)
+            .filter(|f| match &f.ty {
+                syn::Type::Path(syn::TypePath {
+                    path: syn::Path { segments, .. },
+                    ..
+                }) => {
+                    segments.len() == 1 && segments[0].ident == "UncheckedAccount"
+                        || segments[0].ident == "AccountInfo"
+                }
+                _ => false,
+            })
+    }
+
     fn enums(&self) -> impl Iterator<Item = &syn::ItemEnum> {
         self.items.iter().filter_map(|i| match i {
             syn::Item::Enum(item) => Some(item),

tests/auction-house

@@ -7,3 +7,6 @@ cashiers_check = "Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"
 
 [scripts]
 test = "yarn run mocha -t 1000000 tests/"
+
+[features]
+safety_checks = false

tests/cashiers-check/Anchor.toml

@@ -41,3 +41,6 @@ program = "./deps/stake/target/deploy/registry.so"
 [[test.genesis]]
 address = "6ebQNeTPZ1j7k3TtkCCtEPRvG7GQsucQrZ7sSEDQi9Ks"
 program = "./deps/stake/target/deploy/lockup.so"
+
+[features]
+safety_checks = false