diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md index ac81a54..eedf9fb 100644 --- a/PR_DESCRIPTION.md +++ b/PR_DESCRIPTION.md @@ -1,227 +1,38 @@ -# ๐Ÿš€ Major Feature Implementation: Rule Documentation, Pipeline Optimization, Audit Logging & Configurable Rules +## Summary -## Overview +This pull request introduces a new security rule to detect potentially unsafe operations in Soroban contracts. The rule flags a variety of patterns that can lead to security vulnerabilities and unexpected behavior, providing developers with actionable recommendations to improve the safety and reliability of their contracts. -This PR implements four major issues that significantly enhance the GasGuard ecosystem: +## What Changed -- **#209** ๐Ÿ“˜ Rule Documentation Generator -- **#210** ๐Ÿ”„ Rule Execution Pipeline Refactor -- **#211** ๐Ÿงพ Audit Logging System -- **#212** โš™๏ธ Configurable Rule Engine +- Implemented a new security rule, `detect-unsafe-operations`, to identify unsafe patterns in Soroban contracts. +- The rule detects the following unsafe operations: + - `unsafe` blocks + - `unsafe fn` + - `.unwrap()` calls + - `panic!` macros + - `unreachable!` macros + - `std::mem::transmute` + - Raw pointer usage + - Unchecked arithmetic operators +- For each detected violation, the rule provides a detailed description of the issue and a recommendation for how to fix it. -## ๐Ÿ“‹ Changes Summary +## Why -### โœ… Issue #209 - Rule Documentation Generator -**Location**: `scripts/generate-rule-docs.ts`, `docs/rules/` +The use of unsafe operations in Soroban contracts can introduce serious security vulnerabilities, such as memory corruption, integer overflows, and unexpected panics. By detecting these patterns and providing clear guidance on how to avoid them, this new rule helps developers write more secure and robust contracts. -**Features**: -- Auto-scans TypeScript and Rust rule files for metadata extraction -- Generates organized markdown documentation by category (Solidity, Soroban, General) -- Creates searchable index with cross-references -- Supports examples, dependencies, and parameter documentation -- CLI-ready script for automated documentation generation +## Testing Performed -**Files Added**: -- `scripts/generate-rule-docs.ts` - Main documentation generator -- `docs/RULES.md` - Generated index -- `docs/rules/general.md` - General rules documentation -- `docs/rules/solidity.md` - Solidity rules documentation +- [x] Lint +- [x] Tests +- [x] Build -### โœ… Issue #210 - Rule Execution Pipeline Refactor -**Location**: `src/analysis/pipeline/pipeline-executor.ts` +## Edge Cases Considered -**Features**: -- **Parallel Execution**: Configurable concurrency for independent rules -- **Dependency-Aware Ordering**: Automatic level-based grouping -- **Performance Optimization**: Up to 4x faster execution for compatible rule sets -- **Backward Compatibility**: Sequential execution still available -- **Enhanced Metrics**: Execution time tracking and performance monitoring +- The rule correctly handles a variety of code formatting and style variations. +- The rule avoids false positives by ignoring non-arithmetic uses of `+`, `-`, and `*` operators. -**Key Improvements**: -```typescript -// New parallel execution with dependency resolution -const executor = new PipelineExecutor({ - maxConcurrency: 4, - enableParallelExecution: true -}); -``` +## Risks -### โœ… Issue #211 - Audit Logging System -**Location**: `src/logger/` +None. The new rule is purely additive and does not introduce any breaking changes. -**Features**: -- **Centralized Logging**: Multi-provider architecture (console, file, audit) -- **Audit Trail**: Specialized logging for security and compliance -- **Real-time Events**: EventEmitter for live monitoring -- **Configurable Retention**: Automatic cleanup with configurable policies -- **Comprehensive Types**: Full TypeScript support with strict typing - -**Core Components**: -- `LoggerService` - Main logging orchestration -- `AuditLogger` - Specialized audit event tracking -- `LoggerConfigManager` - Environment-based configuration - -### โœ… Issue #212 - Configurable Rule Engine -**Location**: `src/config/`, `packages/config/` - -**Features**: -- **Dynamic Rule Loading**: Enable/disable rules at runtime -- **JSON Configuration**: Human-readable config files with validation -- **Configuration Profiles**: Predefined rule sets for different use cases -- **Real-time Updates**: File watching for hot configuration reloads -- **Import/Export**: Backup and share configurations easily - -**Configuration Example**: -```json -{ - "rules": [ - { - "id": "reentrancy-guard", - "enabled": true, - "severity": "critical", - "parameters": { "checkExternalCalls": true } - } - ], - "profiles": [ - { - "name": "strict-security", - "description": "High security profile" - } - ] -} -``` - -## ๐Ÿงช Testing - -- โœ… Rule documentation generator tested (found 2 rules, generated docs) -- โœ… Configuration system loads and validates properly -- โœ… All TypeScript compilation errors resolved -- โœ… Pipeline refactor maintains backward compatibility -- โœ… Audit logging system integrates with existing infrastructure - -## ๐Ÿ“ File Structure - -``` -GasGuard/ -โ”œโ”€โ”€ scripts/ -โ”‚ โ””โ”€โ”€ generate-rule-docs.ts # Documentation generator -โ”œโ”€โ”€ docs/ -โ”‚ โ”œโ”€โ”€ RULES.md # Generated rule index -โ”‚ โ””โ”€โ”€ rules/ # Rule documentation by category -โ”œโ”€โ”€ src/ -โ”‚ โ”œโ”€โ”€ logger/ # Audit logging system -โ”‚ โ”‚ โ”œโ”€โ”€ index.ts -โ”‚ โ”‚ โ”œโ”€โ”€ logger.service.ts -โ”‚ โ”‚ โ”œโ”€โ”€ audit-logger.ts -โ”‚ โ”‚ โ”œโ”€โ”€ logger.config.ts -โ”‚ โ”‚ โ””โ”€โ”€ logger.types.ts -โ”‚ โ”œโ”€โ”€ config/ # Configuration system -โ”‚ โ”‚ โ”œโ”€โ”€ index.ts -โ”‚ โ”‚ โ”œโ”€โ”€ config-manager.ts -โ”‚ โ”‚ โ”œโ”€โ”€ rule-config.ts -โ”‚ โ”‚ โ””โ”€โ”€ config.types.ts -โ”‚ โ””โ”€โ”€ analysis/pipeline/ -โ”‚ โ””โ”€โ”€ pipeline-executor.ts # Enhanced with parallel execution -โ”œโ”€โ”€ packages/config/ -โ”‚ โ”œโ”€โ”€ index.ts -โ”‚ โ”œโ”€โ”€ rule-loader.ts -โ”‚ โ”œโ”€โ”€ config-validator.ts -โ”‚ โ””โ”€โ”€ config-schema.ts -โ””โ”€โ”€ config/ - โ””โ”€โ”€ gasguard.config.json # Sample configuration -``` - -## ๐Ÿš€ Usage Examples - -### Generate Rule Documentation -```bash -npx ts-node scripts/generate-rule-docs.ts -``` - -### Configure Rules -```typescript -import { ConfigManager } from './src/config'; - -const config = ConfigManager.getInstance(); -await config.enableRule('reentrancy-guard'); -await config.updateRule('unused-state-variables', { - severity: 'high' -}); -``` - -### Use Audit Logging -```typescript -import { AuditLogger } from './src/logger'; - -const audit = AuditLogger.getInstance(); -await audit.logApiRequest({ - userId: 'user123', - method: 'POST', - endpoint: '/api/analyze', - statusCode: 200 -}); -``` - -### Parallel Pipeline Execution -```typescript -const executor = new PipelineExecutor({ - maxConcurrency: 8, - enableParallelExecution: true -}); -``` - -## ๐Ÿ”ง Configuration - -The system includes comprehensive configuration options: - -- **Logging Levels**: debug, info, warn, error -- **Performance**: Concurrency limits, timeouts, parallel execution -- **Security**: API key validation, rate limiting -- **Features**: Auto-fix, detailed reporting, real-time monitoring - -## ๐Ÿ“Š Performance Improvements - -- **Pipeline Execution**: Up to 4x faster for independent rules -- **Documentation Generation**: Automated, consistent, and searchable -- **Configuration Management**: Hot reload without service restart -- **Audit Logging**: Minimal performance impact with async processing - -## ๐Ÿ”’ Security & Compliance - -- **Audit Trail**: Complete traceability of all system actions -- **Immutable Logs**: Tamper-evident logging with integrity checks -- **Access Control**: Role-based configuration management -- **Data Protection**: Configurable retention and cleanup policies - -## ๐ŸŽฏ Acceptance Criteria Met - -- โœ… **#209**: Docs generated automatically with metadata extraction -- โœ… **#210**: Pipeline optimized with parallel execution and dependency-aware ordering -- โœ… **#211**: Logs stored and accessible with comprehensive audit trail -- โœ… **#212**: Rules configurable with dynamic loading and JSON-based management - -## ๐Ÿ“ Breaking Changes - -- **None** - All changes are backward compatible -- **Optional Features**: Parallel execution can be disabled -- **Configuration**: Default values provided for all new options - -## ๐Ÿ”„ Migration Guide - -1. **No immediate action required** - existing functionality preserved -2. **Optional**: Enable parallel execution for performance gains -3. **Optional**: Configure audit logging for compliance requirements -4. **Optional**: Use rule configuration for dynamic rule management - -## ๐Ÿง  Future Enhancements - -- Web UI for configuration management -- Advanced rule dependency visualization -- Real-time audit dashboard -- Automated rule performance profiling - ---- - -**Total Changes**: 21 files, 3,546 insertions, 44 deletions -**Testing**: All implementations tested and verified -**Documentation**: Comprehensive docs and examples provided +Closes #496 \ No newline at end of file diff --git a/rules/stellar/security/unsafe-operations/detect-unsafe-operations.ts b/rules/stellar/security/unsafe-operations/detect-unsafe-operations.ts new file mode 100644 index 0000000..e61846c --- /dev/null +++ b/rules/stellar/security/unsafe-operations/detect-unsafe-operations.ts @@ -0,0 +1,172 @@ +/** + * Detect Unsafe Operations in Soroban Contracts (#496) + * Flags patterns that may introduce security vulnerabilities or unexpected behaviour + * in Soroban smart contracts written in Rust. + */ + +export interface UnsafeOperationViolation { + type: string; + line: number; + description: string; + recommendation: string; +} + +export interface UnsafeOperationsResult { + detected: boolean; + violations: UnsafeOperationViolation[]; + message: string; + suggestion: string; +} + +interface UnsafePattern { + type: string; + pattern: RegExp; + description: string; + recommendation: string; +} + +const UNSAFE_PATTERNS: UnsafePattern[] = [ + { + type: 'unsafe-block', + pattern: /\bunsafe\s*\{/, + description: 'Use of `unsafe` block bypasses Rust safety guarantees.', + recommendation: + 'Avoid `unsafe` blocks in Soroban contracts. If absolutely necessary, isolate the unsafe code behind a well-tested, audited helper with clear pre/post-conditions.', + }, + { + type: 'unsafe-function', + pattern: /\bunsafe\s+fn\s+\w+/, + description: 'Declaration of an `unsafe fn` exposes callers to undefined behaviour if misused.', + recommendation: + 'Prefer safe function signatures. If the function must be unsafe, document the safety contract and ensure all callers uphold it.', + }, + { + type: 'unchecked-unwrap', + pattern: /\.unwrap\(\)/, + description: '`.unwrap()` will panic on `None` or `Err`, crashing the contract.', + recommendation: + 'Replace `.unwrap()` with `.unwrap_or()`, `.unwrap_or_else()`, `?` operator, or explicit `match` to handle failure gracefully.', + }, + { + type: 'panic-macro', + pattern: /\bpanic!\s*\(/, + description: '`panic!()` aborts execution and consumes all remaining gas.', + recommendation: + 'Return a `Result` with a descriptive error instead of panicking so callers can react appropriately.', + }, + { + type: 'unreachable-macro', + pattern: /\bunreachable!\s*\(/, + description: '`unreachable!()` panics if the code path is ever reached.', + recommendation: + 'Return an explicit error via `Result` or use `debug_assert!` during development only.', + }, + { + type: 'transmute', + pattern: /\bstd::mem::transmute\b|\bmem::transmute\b|\btransmute\s* v.type))]; + + return { + detected: true, + violations, + message: `Detected ${violations.length} unsafe operation(s): ${uniqueTypes.join(', ')}.`, + suggestion: + 'Review each flagged operation and replace with safe alternatives. Prefer explicit error handling over panics and use Soroban SDK helpers for arithmetic and type conversions.', + }; +} diff --git a/tests/rules/detect-unsafe-operations.spec.ts b/tests/rules/detect-unsafe-operations.spec.ts new file mode 100644 index 0000000..1dfc542 --- /dev/null +++ b/tests/rules/detect-unsafe-operations.spec.ts @@ -0,0 +1,202 @@ +import { detectUnsafeOperations } from '../../rules/stellar/security/unsafe-operations/detect-unsafe-operations'; +import { FixtureLoader } from '../../libs/testing/src/fixture-loader'; + +describe('detectUnsafeOperations', () => { + describe('unsafe blocks and functions', () => { + it('flags an unsafe block', () => { + const code = `unsafe { *ptr = 42; }`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'unsafe-block')).toBe(true); + }); + + it('flags an unsafe function declaration', () => { + const code = `unsafe fn deref(ptr: *const u32) -> u32 { *ptr }`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'unsafe-function')).toBe(true); + }); + }); + + describe('unchecked unwrap', () => { + it('flags .unwrap() usage', () => { + const code = `let val = storage.get("key").unwrap();`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'unchecked-unwrap')).toBe(true); + expect(result.violations.find((v) => v.type === 'unchecked-unwrap')!.recommendation).toMatch(/unwrap_or/); + }); + }); + + describe('panic and unreachable macros', () => { + it('flags panic! macro', () => { + const code = `panic!("not allowed");`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'panic-macro')).toBe(true); + }); + + it('flags unreachable! macro', () => { + const code = `unreachable!("should never happen");`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'unreachable-macro')).toBe(true); + }); + }); + + describe('transmute', () => { + it('flags std::mem::transmute', () => { + const code = `let x: u64 = std::mem::transmute(val);`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'transmute')).toBe(true); + }); + + it('flags mem::transmute', () => { + const code = `let x: u64 = mem::transmute(val);`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'transmute')).toBe(true); + }); + }); + + describe('raw pointers', () => { + it('flags *const T pointer type', () => { + const code = `let ptr: *const u32 = &val;`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'raw-pointer')).toBe(true); + }); + + it('flags *mut T pointer type', () => { + const code = `let ptr: *mut u32 = &mut val;`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'raw-pointer')).toBe(true); + }); + }); + + describe('unchecked arithmetic operators', () => { + it('flags raw addition operator', () => { + const code = `let total = amount + fee;`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'unchecked-arithmetic-operator')).toBe(true); + }); + + it('flags raw subtraction operator', () => { + const code = `let remaining = balance - amount;`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'unchecked-arithmetic-operator')).toBe(true); + }); + + it('flags raw multiplication operator', () => { + const code = `let product = price * quantity;`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.some((v) => v.type === 'unchecked-arithmetic-operator')).toBe(true); + }); + }); + + describe('safe cases', () => { + it('does not flag checked arithmetic helpers', () => { + const code = `let total = amount.checked_add(fee).unwrap_or(0);`; + const result = detectUnsafeOperations(code); + // Should not flag the arithmetic operator since checked_add is used + const arithViolations = result.violations.filter((v) => v.type === 'unchecked-arithmetic-operator'); + expect(arithViolations).toHaveLength(0); + }); + + it('does not flag saturating helpers', () => { + const code = `let total = amount.saturating_add(fee);`; + const result = detectUnsafeOperations(code); + const arithViolations = result.violations.filter((v) => v.type === 'unchecked-arithmetic-operator'); + expect(arithViolations).toHaveLength(0); + }); + + it('does not flag code without any unsafe patterns', () => { + const code = ` +use soroban_sdk::{contract, contractimpl, Env}; + +#[contract] +pub struct Counter; + +#[contractimpl] +impl Counter { + pub fn increment(env: Env) -> u64 { + let count: u64 = env.storage().instance().get("count").unwrap_or(0); + let next = count.saturating_add(1); + env.storage().instance().set("count", &next); + next + } +} + `; + const result = detectUnsafeOperations(code); + // Only the use statement line with "Counter" - should have no unsafe violations + const unsafeViolations = result.violations.filter( + (v) => v.type !== 'unchecked-arithmetic-operator' || !code.includes('saturating_add'), + ); + // unwrap_or is safe, saturating_add is safe, no unsafe blocks + expect(result.violations.filter((v) => v.type === 'unsafe-block')).toHaveLength(0); + expect(result.violations.filter((v) => v.type === 'panic-macro')).toHaveLength(0); + expect(result.violations.filter((v) => v.type === 'unchecked-unwrap')).toHaveLength(0); + }); + + it('does not flag comments containing unsafe keywords', () => { + const code = `// This function uses panic!("test") but it is just a comment\nlet x = 1;`; + const result = detectUnsafeOperations(code); + const panicViolations = result.violations.filter((v) => v.type === 'panic-macro'); + expect(panicViolations).toHaveLength(0); + }); + }); + + describe('multiple violations', () => { + it('reports all violations in one result', () => { + const code = ` +let val = storage.get("key").unwrap(); +panic!("bad state"); +let total = a + b; +unsafe { *ptr = val; } + `; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(true); + expect(result.violations.length).toBeGreaterThanOrEqual(4); + expect(result.message).toMatch(/unsafe operation/); + expect(result.suggestion).toMatch(/safe alternatives/); + }); + }); + + describe('no violations', () => { + it('returns detected false for clean code', () => { + const code = `let x = 1;`; + const result = detectUnsafeOperations(code); + expect(result.detected).toBe(false); + expect(result.violations).toHaveLength(0); + expect(result.message).toMatch(/No unsafe operations/); + }); + }); + + describe('fixture validation', () => { + it('fixture matches expected structure', () => { + const fixture = FixtureLoader.loadFixture( + './tests/rules/fixtures/stellar-unsafe-operations.json', + ); + expect(fixture.id).toBe('stellar-unsafe-operations-1'); + expect(fixture.expectedFindings.length).toBeGreaterThan(0); + expect(fixture.metadata?.category).toBe('security'); + }); + + it('detector agrees with fixture violations', () => { + const fixture = FixtureLoader.loadFixture( + './tests/rules/fixtures/stellar-unsafe-operations.json', + ); + const result = detectUnsafeOperations(fixture.input); + expect(result.detected).toBe(true); + expect(result.violations.length).toBeGreaterThanOrEqual(fixture.expectedFindings.length); + for (const finding of fixture.expectedFindings) { + expect(result.violations.some((v) => v.type === finding.type)).toBe(true); + } + }); + }); +}); diff --git a/tests/rules/fixtures/stellar-unsafe-operations.json b/tests/rules/fixtures/stellar-unsafe-operations.json new file mode 100644 index 0000000..f4f03d5 --- /dev/null +++ b/tests/rules/fixtures/stellar-unsafe-operations.json @@ -0,0 +1,48 @@ +{ + "id": "stellar-unsafe-operations-1", + "name": "Unsafe Operations Detection in Soroban Contracts", + "description": "Flags unsafe blocks, unchecked unwrap, panic macros, raw pointers, and unchecked arithmetic in Soroban contracts", + "input": "use soroban_sdk::{contract, contractimpl, Env, Address};\n\n#[contract]\npub struct Vault;\n\n#[contractimpl]\nimpl Vault {\n pub fn withdraw(env: Env, user: Address, amount: i128) -> i128 {\n // Unsafe: unchecked unwrap\n let balance: i128 = env.storage().instance().get(\"balance\").unwrap();\n\n // Unsafe: raw arithmetic\n let remaining = balance - amount;\n\n // Unsafe: panic macro\n if remaining < 0 {\n panic!(\"insufficient balance\");\n }\n\n // Unsafe: unsafe block\n unsafe {\n let raw = 0 as *const i128;\n }\n\n remaining\n }\n}", + "expectedFindings": [ + { + "ruleId": "detect-unsafe-operations", + "type": "unchecked-unwrap", + "severity": "Warning", + "messagePattern": "unwrap\\(\\)", + "line": 11 + }, + { + "ruleId": "detect-unsafe-operations", + "type": "unchecked-arithmetic-operator", + "severity": "Warning", + "messagePattern": "arithmetic operator", + "line": 14 + }, + { + "ruleId": "detect-unsafe-operations", + "type": "panic-macro", + "severity": "Warning", + "messagePattern": "panic!", + "line": 17 + }, + { + "ruleId": "detect-unsafe-operations", + "type": "unsafe-block", + "severity": "Critical", + "messagePattern": "unsafe", + "line": 21 + }, + { + "ruleId": "detect-unsafe-operations", + "type": "raw-pointer", + "severity": "Critical", + "messagePattern": "Raw pointer", + "line": 22 + } + ], + "metadata": { + "language": "soroban", + "category": "security", + "tags": ["security", "unsafe-operations", "soroban"] + } +}