diff --git a/config/gasguard.config.json b/config/gasguard.config.json index 770c354..f6776d8 100644 --- a/config/gasguard.config.json +++ b/config/gasguard.config.json @@ -71,6 +71,23 @@ }, "dependencies": [], "tags": ["security", "reentrancy", "solidity"] + }, + { + "id": "detect-inconsistent-visibility", + "name": "Detect Inconsistent Soroban Visibility", + "enabled": true, + "severity": "high", + "category": "security", + "language": "rust", + "description": "Flags `pub fn` declarations inside `#[contractimpl]` impl blocks whose name or shape indicates an internal helper but whose visibility registers them as a Soroban contract entry point (issue #322)", + "parameters": { + "checkHelperNames": true, + "checkUnderscoreNames": true, + "checkPrivateNames": true, + "checkEnvParameter": true + }, + "dependencies": [], + "tags": ["security", "visibility", "gas-optimization", "soroban"] } ], "profiles": [ diff --git a/rules/stellar/security/visibility/detect-inconsistent-visibility.ts b/rules/stellar/security/visibility/detect-inconsistent-visibility.ts new file mode 100644 index 0000000..254e4dd --- /dev/null +++ b/rules/stellar/security/visibility/detect-inconsistent-visibility.ts @@ -0,0 +1,338 @@ +/** + * Detect Inconsistent Soroban Function Visibility (#322) + * + * Soroban treats every `pub fn` inside an `#[contractimpl]` impl block as an + * externally invokable contract entry point. Each entry point inflates Wasm + * size, increases dispatch cost, and broadens the contract attack surface. + * + * This rule flags functions whose visibility is inconsistent with their intent: + * 1. `pub fn _xxx` — leading-underscore names are a Rust convention for + * intentionally unused or private items; marking one `pub` is a strong + * signal that the author meant it to be hidden. + * 2. `pub fn` whose name suggests an internal helper + * (helper_, internal_, _inner, priv_, private_) — these were almost + * certainly meant to stay inside the contract. + * 3. `pub fn` with one of those helper-style names that does not even + * receive `Env` as its first parameter cannot be a real Soroban entry + * point and is reported with `helper-no-env` to indicate the stronger + * confidence. + * + * Only the literal `pub` keyword is checked; `pub(crate)` and `pub(super)` + * are ignored by the Soroban entry-point generator and are considered safe. + */ + +export type VisibilityKind = + | 'underscore-name' + | 'helper-name' + | 'helper-no-env'; + +export interface VisibilityViolation { + functionName: string; + line: number; + visibility: 'pub'; + kind: VisibilityKind; + reason: string; +} + +export interface InconsistentVisibilityResult { + detected: boolean; + violations: VisibilityViolation[]; + functionsScanned: number; + implementationsScanned: number; + message: string; + suggestion: string; +} + +interface FnInfo { + name: string; + paramList: string; + absIdx: number; +} + +const CONTRACT_IMPL_ATTRIBUTE = /#\s*\[\s*contractimpl\b/; + +// Tokens that strongly suggest an internal intent: matched when preceded by +// the start of the name or an underscore and followed by an underscore or +// end of name. `priv` is intentionally included as a token (so `priv_x` and +// `private_x` are caught) but it must be followed by `_` to avoid matching +// unrelated words like `privacy`. +const HELPER_TOKEN = /(?:^|_)(?:helper|internal|inner|priv|private)(?:_|$)/; + +const REASON_MAP: Record = { + 'underscore-name': + 'underscore-prefixed name suggests the function should be private', + 'helper-name': + 'name indicates a helper or internal utility but is exposed as a contract entry point', + 'helper-no-env': + 'helper-style function without an `Env` first parameter cannot be a real Soroban entry point', +}; + +// Locate the start and end brace range of an `impl { ... }` block whose +// preceding attribute is `#[contractimpl]`. Brace counting handles nested +// impl blocks correctly. +function findContractImplBlocks( + code: string, +): Array<{ start: number; end: number }> { + const blocks: Array<{ start: number; end: number }> = []; + let i = 0; + + while (i < code.length) { + const attrMatch = CONTRACT_IMPL_ATTRIBUTE.exec(code.slice(i)); + if (!attrMatch) break; + + const attrIdx = i + attrMatch.index; + const attrEnd = attrIdx + attrMatch[0].length; + + const implMatch = /\bimpl\b/.exec(code.slice(attrEnd)); + if (!implMatch) { + i = attrEnd; + continue; + } + + const nameStart = attrEnd + implMatch.index; + const braceStart = code.indexOf('{', nameStart); + if (braceStart < 0) { + i = nameStart; + continue; + } + + let depth = 0; + let cursor = braceStart; + let opened = false; + while (cursor < code.length) { + const ch = code[cursor]; + if (ch === '{') { + depth++; + opened = true; + } else if (ch === '}') { + depth--; + if (opened && depth === 0) { + blocks.push({ start: braceStart, end: cursor }); + i = cursor + 1; + break; + } + } + cursor++; + } + + if (cursor >= code.length) { + i = braceStart + 1; + } + } + + return blocks; +} + +// Walk a single impl block at brace-depth 0 so nested modules, functions, +// and sub-impls are skipped. Only the literal `pub fn` keyword is matched, +// excluding `pub(crate)`, `pub(super)`, and `pub(in path)`. The function +// body (if present) is tracked as +1 brace depth so any nested `pub fn` +// inside it is ignored. +function findTopLevelPubFns( + code: string, + blockStart: number, + blockEnd: number, +): FnInfo[] { + const fns: FnInfo[] = []; + // Move past the impl-block opening `{`. + let i = blockStart + 1; + let braceDepth = 0; + + while (i < blockEnd) { + if (braceDepth > 0) { + const ch = code[i]; + if (ch === '{') braceDepth++; + else if (ch === '}') braceDepth--; + i++; + continue; + } + + // Top-level of the impl block: only literal `pub fn` matches. + if ( + code.startsWith('pub fn ', i) || + code.startsWith('pub fn\t', i) || + code.startsWith('pub fn\n', i) + ) { + const fnStartIdx = i; + const nameStart = i + 'pub fn '.length; + const nameMatch = /^[A-Za-z_][A-Za-z0-9_]*/.exec( + code.slice(nameStart), + ); + if (!nameMatch) { + i++; + continue; + } + const fnName = nameMatch[0]; + const afterName = nameStart + fnName.length; + + // Skip whitespace, generics, and `where` clauses until `(` or end. + let cursor = afterName; + while ( + cursor < blockEnd && + code[cursor] !== '(' && + code[cursor] !== '{' && + code[cursor] !== ';' + ) { + cursor++; + } + if (cursor >= blockEnd || code[cursor] !== '(') { + i = cursor + 1; + continue; + } + + // Walk to the matching `)` tracking nested parens (for tuple types). + let parenDepth = 1; + cursor++; + const paramStart = cursor; + while (cursor < blockEnd && parenDepth > 0) { + const ch = code[cursor]; + if (ch === '(') parenDepth++; + else if (ch === ')') { + parenDepth--; + if (parenDepth === 0) break; + } + cursor++; + } + if (cursor >= blockEnd) { + i = cursor; + continue; + } + // `paramList` is the parameter *contents* only — outer parens are excluded. + const paramList = code.slice(paramStart, cursor); + + // Skip everything until the body `{` (return type, where, etc.). + let bodyStart = cursor + 1; + while ( + bodyStart < blockEnd && + code[bodyStart] !== '{' && + code[bodyStart] !== ';' + ) { + bodyStart++; + } + + fns.push({ + name: fnName, + paramList, + absIdx: fnStartIdx, + }); + + if (bodyStart < blockEnd && code[bodyStart] === '{') { + // Enter the function body: depth becomes 1 so nested `pub fn`s are + // skipped and the closing `}` brings us back to the impl top level. + braceDepth = 1; + i = bodyStart + 1; + } else { + i = bodyStart + 1; + } + continue; + } + + const ch = code[i]; + if (ch === '{') braceDepth++; + else if (ch === '}') braceDepth--; + i++; + } + + return fns; +} + +function takesEnvAsFirstArg(paramList: string): boolean { + const trimmed = paramList.trim(); + if (trimmed.length === 0) return false; + const firstParam = trimmed.split(/\s*,\s*/)[0].trim(); + // The first parameter of a Soroban entry point has the shape + // `env: Env` (or `&env`, `&mut env: Env`, `mut env: Env`). The pattern is + // anchored at the start of the first parameter and requires a `:` after + // `env`, so identifiers like `env_value: u32` are not misclassified as + // an environment handle. + return /^(?:&)?\s*(?:mut\s+)?env\b\s*:/i.test(firstParam); +} + +function classify(name: string, paramList: string): VisibilityKind | null { + if (name.startsWith('_')) { + return 'underscore-name'; + } + if (HELPER_TOKEN.test(name)) { + return takesEnvAsFirstArg(paramList) ? 'helper-name' : 'helper-no-env'; + } + return null; +} + +function lineNumberAt(code: string, offset: number): number { + let line = 1; + for (let i = 0; i < offset && i < code.length; i++) { + if (code[i] === '\n') line++; + } + return line; +} + +export function detectInconsistentVisibility( + code: string, +): InconsistentVisibilityResult { + const violations: VisibilityViolation[] = []; + let functionsScanned = 0; + const implBlocks = findContractImplBlocks(code); + + for (const block of implBlocks) { + const fns = findTopLevelPubFns(code, block.start, block.end); + for (const fn of fns) { + functionsScanned++; + const kind = classify(fn.name, fn.paramList); + if (kind === null) continue; + + violations.push({ + functionName: fn.name, + line: lineNumberAt(code, fn.absIdx), + visibility: 'pub', + kind, + reason: REASON_MAP[kind], + }); + } + } + + if (violations.length === 0) { + return { + detected: false, + violations: [], + functionsScanned, + implementationsScanned: implBlocks.length, + message: + implBlocks.length === 0 + ? 'No `#[contractimpl]` impl blocks found.' + : 'All `pub fn` declarations inside `#[contractimpl]` impls use consistent visibility.', + suggestion: '', + }; + } + + const namesByKind = new Map(); + for (const v of violations) { + const list = namesByKind.get(v.kind) ?? []; + list.push(v.functionName); + namesByKind.set(v.kind, list); + } + + const summary = Array.from(namesByKind.entries()) + .map(([kind, names]) => { + switch (kind) { + case 'underscore-name': + return `underscore-prefixed (${names.join(', ')})`; + case 'helper-name': + return `helper-style (${names.join(', ')})`; + case 'helper-no-env': + return `helper without an \`Env\` parameter (${names.join(', ')})`; + default: + return names.join(', '); + } + }) + .join('; '); + + return { + detected: true, + violations, + functionsScanned, + implementationsScanned: implBlocks.length, + message: `Inconsistent visibility detected on ${violations.length} function(s): ${summary}.`, + suggestion: + 'Drop the `pub` modifier on internal helpers or switch to `pub(crate)` / `pub(super)` so they are not registered as Soroban contract entry points. Reserve `pub fn` inside `#[contractimpl]` for methods that external transactions are meant to call.', + }; +} diff --git a/src/recommendation/rule-recommendation.ts b/src/recommendation/rule-recommendation.ts index 437bdc0..b1e2c01 100644 --- a/src/recommendation/rule-recommendation.ts +++ b/src/recommendation/rule-recommendation.ts @@ -37,7 +37,13 @@ const RULE_SETS: RuleSet[] = [ id: 'soroban-core', name: 'Soroban / Rust Core', language: 'rust', - rules: ['storage-efficiency', 'cpu-budget', 'ledger-limits', 'unused-state'], + rules: [ + 'storage-efficiency', + 'cpu-budget', + 'ledger-limits', + 'unused-state', + 'detect-inconsistent-visibility', + ], description: 'Optimization rules for Soroban (Stellar) contracts', }, { diff --git a/tests/rules/detect-inconsistent-visibility.spec.ts b/tests/rules/detect-inconsistent-visibility.spec.ts new file mode 100644 index 0000000..80cacf8 --- /dev/null +++ b/tests/rules/detect-inconsistent-visibility.spec.ts @@ -0,0 +1,263 @@ +import { detectInconsistentVisibility } from '../../rules/stellar/security/visibility/detect-inconsistent-visibility'; +import { FixtureLoader } from '../../libs/testing/src/fixture-loader'; + +describe('detectInconsistentVisibility', () => { + describe('underscore-prefixed names', () => { + it('flags pub fn that starts with an underscore', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn _do_thing(env: Env) -> u32 { let _ = env; 1 } + pub fn normal_entry(env: Env) -> u32 { let _ = env; 2 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.violations.map((v) => v.functionName)).toContain('_do_thing'); + expect(result.violations[0].kind).toBe('underscore-name'); + }); + + it('does not flag a non-prefixed underscore helper that is just a normal function', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn do_thing(env: Env) -> u32 { let _ = env; 1 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(false); + }); + }); + + describe('helper-name patterns', () => { + it('flags pub fn named helper_*', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn helper_compute(env: Env) -> u32 { let _ = env; 0 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.violations[0].functionName).toBe('helper_compute'); + }); + + it('flags pub fn named internal_*', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn internal_reset(env: Env) { let _ = env; } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.violations[0].functionName).toBe('internal_reset'); + }); + + it('flags pub fn named *_inner', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn state_inner(env: Env) -> u32 { let _ = env; 0 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.violations[0].functionName).toBe('state_inner'); + }); + + it('flags pub fn whose name contains private_', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn private_seed(env: Env) -> u32 { let _ = env; 7 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.violations[0].kind).toBe('helper-name'); + }); + + it('flags pub fn whose name contains priv_', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn priv_balance(env: Env, a: Address) -> i128 { let _ = (env, a); 0 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.violations[0].kind).toBe('helper-name'); + }); + }); + + describe('helper without Env parameter', () => { + it('flags a helper-named function that does not take Env', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn helper_sum(a: u32, b: u32) -> u32 { a + b } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.violations[0].functionName).toBe('helper_sum'); + expect(result.violations[0].kind).toBe('helper-no-env'); + }); + + it('flags a helper-named function with Env as helper-name (not no-env)', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn helper_sum(env: Env, a: u32, b: u32) -> u32 { let _ = env; a + b } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.violations[0].functionName).toBe('helper_sum'); + expect(result.violations[0].kind).toBe('helper-name'); + }); + }); + + describe('safe cases', () => { + it('does not flag pub(crate) fn helpers (Soroban ignores those)', () => { + const code = ` + #[contractimpl] + impl Contract { + pub(crate) fn helper(env: Env) -> u32 { let _ = env; 0 } + pub fn entry(env: Env) -> u32 { let _ = env; 1 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(false); + }); + + it('does not flag pub(super) fn', () => { + const code = ` + #[contractimpl] + impl Contract { + pub(super) fn internal_helper(env: Env) -> u32 { let _ = env; 0 } + pub fn entry(env: Env) -> u32 { let _ = env; 1 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(false); + }); + + it('does not flag normal pub fn with clear naming and Env parameter', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn transfer(env: Env, from: Address, to: Address) { let _ = (env, from, to); } + pub fn balance(env: Env, owner: Address) -> i128 { let _ = (env, owner); 0 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(false); + expect(result.violations).toHaveLength(0); + }); + + it('returns detected=false when there is no #[contractimpl] block', () => { + const code = ` + impl NotAContract { + pub fn _private_helper() { } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(false); + }); + + it('handles nested impl blocks (tracks only contractimpl ones)', () => { + const code = ` + #[contractimpl] + impl Outer { + pub fn entry(env: Env) -> u32 { let _ = env; 1 } + mod inner { + pub fn _private_thing() { } + } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(false); + expect(result.implementationsScanned).toBe(1); + }); + }); + + describe('reports state', () => { + it('reports the number of functions scanned and impl blocks', () => { + const code = ` + #[contractimpl] + impl C1 { + pub fn a(env: Env) -> u32 { let _ = env; 1 } + pub fn b(env: Env) -> u32 { let _ = env; 2 } + } + #[contractimpl] + impl C2 { + pub fn c(env: Env) -> u32 { let _ = env; 3 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.implementationsScanned).toBe(2); + expect(result.functionsScanned).toBe(3); + }); + + it('includes a useful message and suggestion when violations are found', () => { + const code = ` + #[contractimpl] + impl C { + pub fn _helper(env: Env) -> u32 { let _ = env; 0 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + expect(result.message).toMatch(/_helper/); + expect(result.suggestion).toMatch(/pub\(crate\)/); + }); + }); + + describe('multiple violations', () => { + it('reports every violating function', () => { + const code = ` + #[contractimpl] + impl Contract { + pub fn _alpha(env: Env) -> u32 { let _ = env; 1 } + pub fn _beta(env: Env) -> u32 { let _ = env; 2 } + pub fn helper_gamma(env: Env) -> u32 { let _ = env; 3 } + pub fn entry(env: Env) -> u32 { let _ = env; 4 } + } + `; + const result = detectInconsistentVisibility(code); + expect(result.detected).toBe(true); + const names = result.violations.map((v) => v.functionName).sort(); + expect(names).toEqual(['_alpha', '_beta', 'helper_gamma']); + }); + }); + + describe('fixture validation', () => { + it('fixture matches expected structure', () => { + const fixture = FixtureLoader.loadFixture( + './tests/rules/fixtures/stellar-inconsistent-visibility.json', + ); + expect(fixture.id).toBe('stellar-inconsistent-visibility-1'); + expect(fixture.expectedFindings).toHaveLength(4); + expect(fixture.metadata?.category).toBe('security'); + }); + + it('detector agrees with fixture violations', () => { + const fixture = FixtureLoader.loadFixture( + './tests/rules/fixtures/stellar-inconsistent-visibility.json', + ); + const result = detectInconsistentVisibility(fixture.input); + expect(result.detected).toBe(true); + + const fnNames = result.violations.map((v) => v.functionName).sort(); + expect(fnNames).toContain('_internal_seed'); + expect(fnNames).toContain('helper_compute_total'); + expect(fnNames).toContain('state_inner'); + expect(fnNames).toContain('priv_lock'); + // The safe entry point must not be flagged. + expect(fnNames).not.toContain('transfer'); + expect(fnNames).not.toContain('balance_of'); + }); + }); +}); diff --git a/tests/rules/fixtures/stellar-inconsistent-visibility.json b/tests/rules/fixtures/stellar-inconsistent-visibility.json new file mode 100644 index 0000000..97c5b97 --- /dev/null +++ b/tests/rules/fixtures/stellar-inconsistent-visibility.json @@ -0,0 +1,37 @@ +{ + "id": "stellar-inconsistent-visibility-1", + "name": "Inconsistent Soroban Function Visibility", + "description": "Flags `pub fn` declarations inside `#[contractimpl]` impls whose name or shape indicates an internal helper but whose visibility registers them as a Soroban contract entry point. Excess entry points inflate Wasm size and broaden the attack surface.", + "input": "use soroban_sdk::{contract, contractimpl, contracttype, Address, Env};\n\n#[contracttype]\npub struct Token { pub admin: Address }\n\n#[contractimpl]\nimpl Token {\n // Real contract entry point — must stay `pub`.\n pub fn transfer(env: Env, from: Address, to: Address) {\n let _ = (env, from, to);\n }\n\n // Real contract entry point — must stay `pub`.\n pub fn balance_of(env: Env, owner: Address) -> i128 {\n let _ = (env, owner);\n 0\n }\n\n // VIOLATION #1: leading-underscore name but still `pub`.\n pub fn _internal_seed(env: Env) -> u32 {\n let _ = env;\n 0\n }\n\n // VIOLATION #2: helper_ prefix — clearly an internal utility.\n pub fn helper_compute_total(env: Env) -> u64 {\n let _ = env;\n 0\n }\n\n // VIOLATION #3: *_inner suffix — internal helper.\n pub fn state_inner(env: Env) -> u64 {\n let _ = env;\n 0\n }\n\n // VIOLATION #4: priv_ prefix — explicitly names itself private.\n pub fn priv_lock(env: Env) -> bool {\n let _ = env;\n true\n }\n\n // Safe: `pub(crate)` is not seen by the Soroban entry-point generator.\n pub(crate) fn helper_recompute(env: Env) -> u64 {\n let _ = env;\n 0\n }\n}", + "expectedFindings": [ + { + "ruleId": "detect-inconsistent-visibility", + "severity": "High", + "messagePattern": "_internal_seed", + "line": 18 + }, + { + "ruleId": "detect-inconsistent-visibility", + "severity": "High", + "messagePattern": "helper_compute_total", + "line": 23 + }, + { + "ruleId": "detect-inconsistent-visibility", + "severity": "High", + "messagePattern": "state_inner", + "line": 28 + }, + { + "ruleId": "detect-inconsistent-visibility", + "severity": "Medium", + "messagePattern": "priv_lock", + "line": 33 + } + ], + "metadata": { + "language": "soroban", + "category": "security", + "tags": ["visibility", "gas-optimization", "entry-point", "soroban"] + } +}