diff --git a/detectors/unnecessary-lint-allow/Cargo.toml b/detectors/unnecessary-lint-allow/Cargo.toml new file mode 100644 index 00000000..cd203eeb --- /dev/null +++ b/detectors/unnecessary-lint-allow/Cargo.toml @@ -0,0 +1,18 @@ +[package] +edition = "2021" +name = "unnecessary-lint-allow" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +clippy_wrappers = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } +serde = { version = "1", features = ["derive"] } +serde_json = "=1.0.120" + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/unnecessary-lint-allow/src/lib.rs b/detectors/unnecessary-lint-allow/src/lib.rs new file mode 100644 index 00000000..bbeb5952 --- /dev/null +++ b/detectors/unnecessary-lint-allow/src/lib.rs @@ -0,0 +1,99 @@ +#![feature(rustc_private)] + +extern crate rustc_ast; +extern crate rustc_hir; +extern crate rustc_span; + +mod processor; +pub use processor::process_findings; + +use clippy_wrappers::span_lint_and_help; +use if_chain::if_chain; +use rustc_ast::{ + token::{Delimiter, Token, TokenKind}, + tokenstream::{TokenStream, TokenTree}, + AttrArgs, AttrKind, Attribute, Item, ItemKind, +}; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_span::{Span, Symbol}; +use std::collections::VecDeque; + +const LINT_MESSAGE: &str = "This `#[scout_allow]` attribute may be unnecessary. Consider removing it if the lint is no longer triggered."; + +dylint_linting::declare_pre_expansion_lint! { + pub UNNECESSARY_LINT_ALLOW, + Warn, + LINT_MESSAGE, + { + name: "Unnecessary Lint Allow", + long_message: "The `#[scout_allow]` attribute may be unnecessary. Consider removing it if the lint is no longer triggered.", + severity: "Enhancement", + help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/unnecessary-lint-allow", + vulnerability_class: "Code Quality", + } +} + +impl UnnecessaryLintAllow { + fn extract_lint_names(&self, tokens: &TokenStream) -> Vec { + let mut lint_names = Vec::new(); + let mut stack = VecDeque::from([tokens]); + + while let Some(current_stream) = stack.pop_front() { + for tree in current_stream.trees() { + match tree { + TokenTree::Token( + Token { + kind: TokenKind::Ident(ident, _), + .. + }, + _, + ) => { + lint_names.push(ident.to_string()); + } + TokenTree::Delimited(_, _, Delimiter::Parenthesis, inner_stream) => { + stack.push_back(inner_stream); + } + _ => {} + } + } + } + + lint_names + } + + fn check_scout_allow_attrs(&self, cx: &EarlyContext<'_>, attrs: &[Attribute], span: Span) { + for attr in attrs { + if_chain! { + if !attr.span.from_expansion(); + if attr.has_name(Symbol::intern("scout_allow")); + if let AttrKind::Normal(item) = &attr.kind; + if let AttrArgs::Delimited(delimited_args) = &item.item.args; + then { + let lint_names = self.extract_lint_names(&delimited_args.tokens); + for lint_name in lint_names { + span_lint_and_help( + cx, + UNNECESSARY_LINT_ALLOW, + span, + LINT_MESSAGE, + None, + format!("The detector `{}` is no longer triggered. Consider removing the `#[scout_allow({})]` attribute if the lint is no longer triggered.", lint_name, lint_name) + ); + } + } + } + } + } +} + +impl EarlyLintPass for UnnecessaryLintAllow { + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + self.check_scout_allow_attrs(cx, &item.attrs, item.span); + + if let ItemKind::Impl(impl_) = &item.kind { + for impl_item in &impl_.items { + self.check_scout_allow_attrs(cx, &impl_item.attrs, impl_item.span); + } + } + } +} diff --git a/detectors/unnecessary-lint-allow/src/processor.rs b/detectors/unnecessary-lint-allow/src/processor.rs new file mode 100644 index 00000000..dfc1b3af --- /dev/null +++ b/detectors/unnecessary-lint-allow/src/processor.rs @@ -0,0 +1,201 @@ +use serde::de::Error; +use serde_json::Value; +use std::{collections::HashMap, ffi::CStr, ffi::CString, os::raw::c_char}; + +#[derive(Debug, Clone)] +struct Finding { + detector: String, + file_name: String, + span: (usize, usize), + allowed_lint: Option, +} + +struct FileFindings { + unnecessary_allows: Vec, + other_findings: Vec, +} + +struct FindingsCache { + by_file: HashMap, +} + +impl FindingsCache { + fn new(all_findings: &[Value]) -> Self { + let mut by_file: HashMap = HashMap::new(); + + for finding in all_findings { + if let Some(parsed) = parse_finding(finding) { + by_file + .entry(parsed.file_name.clone()) + .or_insert_with(|| FileFindings { + unnecessary_allows: Vec::new(), + other_findings: Vec::new(), + }) + .add_finding(parsed); + } + } + + FindingsCache { by_file } + } +} + +impl FileFindings { + fn add_finding(&mut self, finding: Finding) { + if finding.detector == "unnecessary_lint_allow" { + self.unnecessary_allows.push(finding); + } else { + self.other_findings.push(finding); + } + } +} + +fn parse_finding(finding: &Value) -> Option { + let detector = finding.get("code")?.get("code")?.as_str()?; + let span = finding.get("spans")?.as_array()?.first()?; + let file_name = span.get("file_name")?.as_str()?; + let line_start = span.get("line_start")?.as_u64()?; + let line_end = span.get("line_end")?.as_u64()?; + + let allowed_lint = if detector == "unnecessary_lint_allow" { + finding + .get("children")? + .get(0)? + .get("message")? + .as_str() + .and_then(|msg| msg.split('`').nth(1).map(String::from)) + } else { + None + }; + + let start = usize::try_from(line_start).ok()?; + let end = usize::try_from(line_end).ok()?; + + Some(Finding { + detector: detector.to_owned(), + file_name: file_name.to_owned(), + span: (start, end), + allowed_lint, + }) +} + +fn spans_overlap(span1: (usize, usize), span2: (usize, usize)) -> bool { + span1.0 <= span2.1 && span2.0 <= span1.1 +} + +fn process_findings_impl( + successful_findings: Vec, + output: Vec, + inside_vscode: bool, +) -> (Vec, String) { + let findings_cache = FindingsCache::new(&successful_findings); + + let console_findings: Vec<_> = successful_findings + .into_iter() + .filter(|finding| should_include_finding_impl(finding, &findings_cache)) + .collect(); + + let output_vscode: Vec<_> = if inside_vscode { + output + .into_iter() + .filter(|val| { + val.get("message") + .map(|message| should_include_finding_impl(message, &findings_cache)) + .unwrap_or(true) + }) + .collect() + } else { + Vec::new() + }; + + let output_string_vscode = output_vscode + .into_iter() + .filter_map(|finding| serde_json::to_string(&finding).ok()) + .collect::>() + .join("\n"); + + (console_findings, output_string_vscode) +} + +fn should_include_finding_impl(finding: &Value, cache: &FindingsCache) -> bool { + let current_finding = match parse_finding(finding) { + Some(f) => f, + None => return false, // If we can't parse the finding, we don't include it + }; + + if let Some(file_findings) = cache.by_file.get(¤t_finding.file_name) { + if current_finding.detector == "unnecessary_lint_allow" { + if let Some(allowed_lint) = ¤t_finding.allowed_lint { + !file_findings.other_findings.iter().any(|f| { + &f.detector == allowed_lint && spans_overlap(f.span, current_finding.span) + }) + } else { + true // Include if we can't determine the allowed lint + } + } else { + !file_findings.unnecessary_allows.iter().any(|allow| { + allow + .allowed_lint + .as_ref() + .map_or(false, |lint| lint == ¤t_finding.detector) + && spans_overlap(allow.span, current_finding.span) + }) + } + } else { + true // If we can't find the file, we include it by default + } +} + +/// Process the findings to filter out unnecessary findings. +/// +/// # Safety +/// +/// This function is marked as unsafe because it deals with raw pointers. +/// The caller is responsible for ensuring the safety of the pointers passed as arguments. +#[no_mangle] +pub unsafe extern "C" fn process_findings( + successful_findings_json: *const c_char, + output_json: *const c_char, + inside_vscode: bool, +) -> *mut c_char { + let successful_findings = match parse_json(successful_findings_json) { + Ok(Value::Array(v)) => v, + _ => return std::ptr::null_mut(), + }; + + let output = match parse_json(output_json) { + Ok(Value::Array(v)) => v, + _ => return std::ptr::null_mut(), + }; + + let (console_findings, output_string_vscode) = + process_findings_impl(successful_findings, output, inside_vscode); + + let result = serde_json::json!({ + "console_findings": console_findings, + "output_string_vscode": output_string_vscode + }); + + match serde_json::to_string(&result) { + Ok(result_string) => match CString::new(result_string) { + Ok(c_str) => c_str.into_raw(), + Err(_) => std::ptr::null_mut(), + }, + Err(_) => std::ptr::null_mut(), + } +} + +unsafe fn parse_json(input: *const c_char) -> Result { + let c_str = CStr::from_ptr(input); + let input_str = c_str + .to_str() + .map_err(|_| serde_json::Error::custom("Invalid UTF-8"))?; + serde_json::from_str(input_str) +} + +// Free the string allocated by the `process_findings` function. Should be called after processing everything +#[no_mangle] +pub unsafe extern "C" fn free_string(ptr: *mut c_char) { + if !ptr.is_null() { + let _ = CString::from_raw(ptr); + } +} diff --git a/docs/docs/detectors/12-soroban-version.md b/docs/docs/detectors/12-soroban-version.md index f6a1c0d6..ac83cae3 100644 --- a/docs/docs/detectors/12-soroban-version.md +++ b/docs/docs/detectors/12-soroban-version.md @@ -1,19 +1,19 @@ # Soroban version -## Description +## Description - Category: `Best practices` - Severity: `Enhacement` - Detector: [`soroban-version`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version) -- Test Cases: [`soroban-version-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1) +- Test Cases: [`soroban-version-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1) Using an outdated version of Soroban can lead to issues in our contract. It's a good practice to use the latest version. -## Why is this bad? +## Why is this bad? Using an old version of Soroban can be dangerous, as it may have bugs or security issues. -## Issue example +## Issue example Consider the following `Cargo.toml`: @@ -39,7 +39,7 @@ The code example can be found [here](https://github.com/CoinFabrik/scout-soroban soroban-sdk = { workspace = true } [dev_dependencies] - soroban-sdk = { workspace = true, features = ["testutils"] } + soroban-sdk = { workspace = true, features = ["testutils"] } ``` The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1/remediated-example) @@ -53,4 +53,4 @@ Warns you if you are using an old version of Soroban in the `Cargo.toml`. - [Floating Pragma](https://swcregistry.io/docs/SWC-103/) - [outdated Compiler Version](https://swcregistry.io/docs/SWC-102/) - + diff --git a/test-cases/unnecessary-lint-allow/Cargo.toml b/test-cases/unnecessary-lint-allow/Cargo.toml new file mode 100644 index 00000000..e0576682 --- /dev/null +++ b/test-cases/unnecessary-lint-allow/Cargo.toml @@ -0,0 +1,21 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["unnecessary-lint-allow-*/*"] +resolver = "2" + +[workspace.dependencies] +soroban-sdk = { version = "=21.4.0" } + +[profile.release] +codegen-units = 1 +debug = 0 +debug-assertions = false +lto = true +opt-level = "z" +overflow-checks = true +panic = "abort" +strip = "symbols" + +[profile.release-with-logs] +debug-assertions = true +inherits = "release" diff --git a/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/remediated-example/Cargo.toml b/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..42527218 --- /dev/null +++ b/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/remediated-example/Cargo.toml @@ -0,0 +1,18 @@ + +[package] +edition = "2021" +name = "unnecessary-lint-allow-remediated-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +scout-utils = { version = "0.1.0" } +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/remediated-example/src/lib.rs b/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..0e218e20 --- /dev/null +++ b/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/remediated-example/src/lib.rs @@ -0,0 +1,15 @@ +#![no_std] +use scout_utils::scout_allow; +use soroban_sdk::{contract, contractimpl}; + +#[contract] +pub struct UnnecessaryLintAllow; + +#[contractimpl] +#[scout_allow(assert_violation)] +impl UnnecessaryLintAllow { + pub fn assert_if_greater_than_10(value: u128) -> bool { + assert!(value <= 10); + true + } +} diff --git a/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/vulnerable-example/Cargo.toml b/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..35927951 --- /dev/null +++ b/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/vulnerable-example/Cargo.toml @@ -0,0 +1,18 @@ + +[package] +edition = "2021" +name = "unnecessary-lint-allow-vulnerable-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +scout-utils = { version = "0.1.0" } +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/vulnerable-example/src/lib.rs b/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..56de7638 --- /dev/null +++ b/test-cases/unnecessary-lint-allow/unnecessary-lint-allow-1/vulnerable-example/src/lib.rs @@ -0,0 +1,24 @@ +#![no_std] +use scout_utils::scout_allow; +use soroban_sdk::{contract, contracterror, contractimpl}; + +#[contract] +pub struct UnnecessaryLintAllow; + +#[contracterror] +#[derive(Copy, Clone)] +pub enum AssertError { + GreaterThan10 = 1, +} + +#[contractimpl] +#[scout_allow(assert_violation)] +impl UnnecessaryLintAllow { + pub fn assert_if_greater_than_10(value: u128) -> Result { + if value <= 10 { + Ok(true) + } else { + Err(AssertError::GreaterThan10) + } + } +}