Secure Code Review Skill

Purpose

This skill provides strategic guidance for conducting thorough security code reviews that identify vulnerabilities before they reach production. It implements defense-in-depth principles aligned with OWASP Top 10, SANS Top 25, and Hack23 ISMS Secure Development Policy.

When to Use This Skill

Apply this skill when:

✅ Reviewing pull requests before merge
✅ Conducting periodic security audits of existing code
✅ Implementing new features that handle sensitive data
✅ Integrating third-party libraries or APIs
✅ Refactoring authentication/authorization logic
✅ Before major releases or production deployments
✅ After security incidents or vulnerability disclosures

Do NOT use for:

❌ General code style reviews (use code-quality-checks skill)
❌ Performance optimization (different concern)
❌ Business logic validation (functional testing)

Decision Tree

START: Code Review Required
    │
    ├─→ Does code handle user input?
    │   ├─→ YES → Apply Input Validation Checklist
    │   └─→ NO → Continue
    │
    ├─→ Does code access database?
    │   ├─→ YES → Apply SQL Injection Prevention
    │   └─→ NO → Continue
    │
    ├─→ Does code handle authentication/authorization?
    │   ├─→ YES → Apply Authentication Security Checklist
    │   └─→ NO → Continue
    │
    ├─→ Does code process sensitive data?
    │   ├─→ YES → Apply Data Protection Checklist
    │   └─→ NO → Continue
    │
    ├─→ Does code interact with external systems?
    │   ├─→ YES → Apply API Security Checklist
    │   └─→ NO → Continue
    │
    └─→ Run SAST tools (SonarCloud, CodeQL)
        └─→ Document findings and mitigation

OWASP Top 10 Security Review Checklist

A01:2021 – Broken Access Control

What to Check:

✅ Verify authorization checks exist before resource access
✅ Ensure users cannot access resources outside their permissions
✅ Check for insecure direct object references (IDOR)
✅ Validate that horizontal and vertical privilege escalation is prevented
✅ Confirm Spring Security annotations (@PreAuthorize, @Secured) are correctly applied

Code Patterns:

✅ SECURE - Proper Authorization Check:

@Service
public class DocumentService {
    @Autowired
    private SecurityContext securityContext;
    
    @PreAuthorize("hasRole('USER')")
    public Document getDocument(Long documentId) {
        Document doc = documentRepository.findById(documentId)
            .orElseThrow(() -> new ResourceNotFoundException("Document not found"));
        
        // Verify ownership before returning
        if (!doc.getOwnerId().equals(getCurrentUserId())) {
            throw new AccessDeniedException("Cannot access document owned by another user");
        }
        
        return doc;
    }
}

❌ INSECURE - Missing Authorization:

// BAD: No authorization check, anyone can access any document
@GetMapping("/documents/{id}")
public Document getDocument(@PathVariable Long id) {
    return documentRepository.findById(id).orElse(null);
}

A02:2021 – Cryptographic Failures

What to Check:

✅ Verify sensitive data is encrypted at rest and in transit
✅ Ensure strong encryption algorithms (AES-256, RSA-2048+)
✅ Check that passwords are hashed with bcrypt/argon2 (never plain text)
✅ Validate TLS 1.2+ is enforced for all connections
✅ Confirm encryption keys are managed securely (never hardcoded)

Code Patterns:

✅ SECURE - Proper Password Hashing:

@Service
public class UserService {
    @Autowired
    private PasswordEncoder passwordEncoder; // BCryptPasswordEncoder
    
    public void createUser(String username, String password) {
        String hashedPassword = passwordEncoder.encode(password);
        User user = new User(username, hashedPassword);
        userRepository.save(user);
    }
    
    public boolean verifyPassword(String rawPassword, String hashedPassword) {
        return passwordEncoder.matches(rawPassword, hashedPassword);
    }
}

❌ INSECURE - Plain Text Password:

// BAD: Storing passwords in plain text
public void createUser(String username, String password) {
    User user = new User(username, password); // NEVER DO THIS
    userRepository.save(user);
}

A03:2021 – Injection

What to Check:

✅ All SQL queries use parameterized statements or JPA criteria queries
✅ User input is validated before use in queries
✅ Command injection is prevented (no Runtime.exec with user input)
✅ LDAP, XML, and OS command injection vectors are addressed
✅ ORM queries are not constructed with string concatenation

Code Patterns:

✅ SECURE - Parameterized Query:

@Repository
public interface PoliticianRepository extends JpaRepository<Politician, Long> {
    // JPA query with named parameter - safe from SQL injection
    @Query("SELECT p FROM Politician p WHERE p.firstName = :firstName AND p.lastName = :lastName")
    List<Politician> findByName(@Param("firstName") String firstName, 
                                @Param("lastName") String lastName);
}

// Using JPA Criteria API - also safe
public List<Politician> searchPoliticians(String searchTerm) {
    CriteriaBuilder cb = entityManager.getCriteriaBuilder();
    CriteriaQuery<Politician> query = cb.createQuery(Politician.class);
    Root<Politician> politician = query.from(Politician.class);
    
    query.where(cb.like(politician.get("firstName"), "%" + searchTerm + "%"));
    return entityManager.createQuery(query).getResultList();
}

❌ INSECURE - SQL Injection Vulnerable:

// BAD: String concatenation in query
@Query(value = "SELECT * FROM politician WHERE first_name = '" + firstName + "'", 
       nativeQuery = true)
List<Politician> findByName(String firstName); // SQL INJECTION RISK!

// BAD: Direct JDBC with concatenation
public List<Politician> search(String name) {
    String sql = "SELECT * FROM politician WHERE name = '" + name + "'";
    return jdbcTemplate.query(sql, new PoliticianMapper()); // VULNERABLE!
}

A04:2021 – Insecure Design

What to Check:

✅ Security requirements defined before implementation
✅ Threat modeling completed for sensitive features
✅ Security controls are built into the architecture
✅ Rate limiting implemented for sensitive operations
✅ Business logic flaws identified and mitigated

Secure Design Principles:

// Example: Rate limiting for login attempts
@Service
public class LoginService {
    private static final int MAX_ATTEMPTS = 5;
    private static final Duration LOCKOUT_DURATION = Duration.ofMinutes(15);
    
    private final LoadingCache<String, Integer> loginAttempts = CacheBuilder.newBuilder()
        .expireAfterWrite(LOCKOUT_DURATION)
        .build(new CacheLoader<String, Integer>() {
            public Integer load(String key) {
                return 0;
            }
        });
    
    public void login(String username, String password) {
        // Check if account is locked
        int attempts = loginAttempts.get(username);
        if (attempts >= MAX_ATTEMPTS) {
            throw new AccountLockedException(
                "Account locked due to too many failed attempts. Try again in 15 minutes.");
        }
        
        // Attempt authentication
        boolean authenticated = authenticate(username, password);
        
        if (!authenticated) {
            loginAttempts.put(username, attempts + 1);
            throw new BadCredentialsException("Invalid credentials");
        }
        
        // Success - reset counter
        loginAttempts.invalidate(username);
    }
}

A05:2021 – Security Misconfiguration

What to Check:

✅ Default credentials are changed
✅ Error messages don't leak sensitive information
✅ Debug mode disabled in production
✅ Unnecessary features/services are disabled
✅ Security headers configured (CSP, HSTS, X-Frame-Options)
✅ Dependency versions are up to date

Configuration Checklist:

✅ SECURE - Production Configuration:

# application-production.yml
spring:
  profiles:
    active: production
  
  # Disable debug endpoints
  boot:
    admin:
      client:
        enabled: false
  
  # Secure session management
  session:
    timeout: 30m
    cookie:
      secure: true
      http-only: true
      same-site: strict
  
  # Hide implementation details
  mvc:
    throw-exception-if-no-handler-found: true
  resources:
    add-mappings: false

# Security headers
server:
  error:
    include-message: never
    include-stacktrace: never
    include-exception: false

❌ INSECURE - Development Settings in Production:

# BAD: Debug enabled in production
spring:
  profiles:
    active: development
  
  # Exposes sensitive endpoints
  boot:
    admin:
      client:
        enabled: true
  
  # Leak stack traces
server:
  error:
    include-stacktrace: always
    include-exception: true

A06:2021 – Vulnerable and Outdated Components

What to Check:

✅ All dependencies scanned with OWASP Dependency-Check
✅ No dependencies with known high/critical CVEs
✅ Dependencies are from trusted sources
✅ Unused dependencies are removed
✅ Security patches applied promptly

Maven Security Check:

# Run OWASP Dependency Check before adding new dependencies
mvn org.owasp:dependency-check-maven:check

# Review the report
ls -la target/dependency-check-report.html

# Fail build on high severity vulnerabilities
mvn verify -Dowasp.failOnCVSS=7

pom.xml Best Practices:

<!-- Use dependencyManagement to control versions -->
<dependencyManagement>
    <dependencies>
        <dependency>
            <groupId>org.springframework</groupId>
            <artifactId>spring-framework-bom</artifactId>
            <version>5.3.34</version>
            <type>pom</type>
            <scope>import</scope>
        </dependency>
    </dependencies>
</dependencyManagement>

<!-- Enable OWASP Dependency Check -->
<plugin>
    <groupId>org.owasp</groupId>
    <artifactId>dependency-check-maven</artifactId>
    <version>9.0.7</version>
    <configuration>
        <failBuildOnCVSS>7</failBuildOnCVSS>
        <suppressionFiles>
            <suppressionFile>dependency-check-suppressions.xml</suppressionFile>
        </suppressionFiles>
    </configuration>
</plugin>

A07:2021 – Identification and Authentication Failures

What to Check:

✅ Multi-factor authentication implemented for sensitive operations
✅ Session management is secure (secure cookies, timeout)
✅ Password requirements enforce strong passwords
✅ Credential stuffing protection (rate limiting, CAPTCHA)
✅ Session fixation attacks prevented

Code Patterns:

✅ SECURE - Spring Security Configuration:

@Configuration
@EnableWebSecurity
public class SecurityConfig {
    
    @Bean
    public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
        http
            .authorizeHttpRequests(auth -> auth
                .requestMatchers("/public/**").permitAll()
                .requestMatchers("/admin/**").hasRole("ADMIN")
                .anyRequest().authenticated()
            )
            .formLogin(form -> form
                .loginPage("/login")
                .failureHandler(authenticationFailureHandler())
                .successHandler(authenticationSuccessHandler())
            )
            .logout(logout -> logout
                .logoutSuccessUrl("/login?logout")
                .invalidateHttpSession(true)
                .deleteCookies("JSESSIONID")
            )
            .sessionManagement(session -> session
                .sessionCreationPolicy(SessionCreationPolicy.IF_REQUIRED)
                .maximumSessions(1)
                .expiredUrl("/login?expired")
            )
            .csrf(csrf -> csrf.csrfTokenRepository(CookieCsrfTokenRepository.withHttpOnlyFalse()));
        
        return http.build();
    }
    
    @Bean
    public PasswordEncoder passwordEncoder() {
        // BCrypt with strength 12
        return new BCryptPasswordEncoder(12);
    }
}

A08:2021 – Software and Data Integrity Failures

What to Check:

✅ Deserialization of untrusted data is prevented
✅ Code signing and integrity verification in CI/CD
✅ Dependency checksums verified
✅ No auto-updates without integrity verification
✅ Git commits are signed

Secure Deserialization:

// Use safe deserialization with explicit type validation
@Bean
public ObjectMapper objectMapper() {
    ObjectMapper mapper = new ObjectMapper();
    
    // Do NOT use activateDefaultTyping for untrusted input
    // If polymorphism is required, use @JsonTypeInfo on specific DTOs
    // with a strict allowlist-based PolymorphicTypeValidator
    
    // Adjust serialization behavior if needed
    mapper.disable(SerializationFeature.FAIL_ON_EMPTY_BEANS);
    // Keep FAIL_ON_UNKNOWN_PROPERTIES enabled by default to catch unexpected/malicious fields
    
    return mapper;
}

A09:2021 – Security Logging and Monitoring Failures

What to Check:

✅ Authentication/authorization failures are logged
✅ High-value transactions are audited
✅ Logs don't contain sensitive data (passwords, tokens)
✅ Log injection attacks prevented
✅ Alerting configured for suspicious activities

Secure Logging Pattern:

@Component
public class SecurityAuditLogger {
    private static final Logger auditLog = LoggerFactory.getLogger("SECURITY_AUDIT");
    
    public void logAuthenticationSuccess(String username, String ipAddress) {
        auditLog.info("Authentication successful - User: {}, IP: {}", 
            sanitizeForLog(username), 
            sanitizeForLog(ipAddress));
    }
    
    public void logAuthenticationFailure(String username, String ipAddress, String reason) {
        auditLog.warn("Authentication failed - User: {}, IP: {}, Reason: {}", 
            sanitizeForLog(username), 
            sanitizeForLog(ipAddress),
            reason);
    }
    
    public void logAccessDenied(String username, String resource) {
        auditLog.warn("Access denied - User: {}, Resource: {}", 
            sanitizeForLog(username), 
            sanitizeForLog(resource));
    }
    
    // Prevent log injection
    private String sanitizeForLog(String input) {
        if (input == null) return "null";
        return input.replaceAll("[\n\r\t]", "_");
    }
}

A10:2021 – Server-Side Request Forgery (SSRF)

What to Check:

✅ URLs from user input are validated against allowlist
✅ Network segmentation prevents access to internal resources
✅ DNS rebinding attacks prevented
✅ Disable unused URL schemas (file://, gopher://)

SSRF Prevention:

@Service
public class ExternalDataService {
    private static final Set<String> ALLOWED_HOSTS = Set.of(
        "api.riksdagen.se",
        "api.worldbank.org",
        "data.val.se"
    );
    
    public String fetchExternalData(String url) throws IOException {
        URL parsedUrl;
        try {
            parsedUrl = new URL(url);
        } catch (MalformedURLException e) {
            throw new IllegalArgumentException("Invalid URL", e);
        }
        
        // Validate protocol
        if (!parsedUrl.getProtocol().equals("https")) {
            throw new IllegalArgumentException("Only HTTPS URLs allowed");
        }
        
        // Validate host against allowlist
        if (!ALLOWED_HOSTS.contains(parsedUrl.getHost())) {
            throw new IllegalArgumentException("Host not in allowlist: " + parsedUrl.getHost());
        }
        
        // Fetch data with timeout
        HttpURLConnection conn = (HttpURLConnection) parsedUrl.openConnection();
        conn.setConnectTimeout(5000);
        conn.setReadTimeout(5000);
        
        return IOUtils.toString(conn.getInputStream(), StandardCharsets.UTF_8);
    }
}

SAST/DAST Integration

CodeQL Configuration

Ensure .github/workflows/codeql.yml includes:

name: "CodeQL Security Analysis"
on:
  push:
    branches: [ main, develop ]
  pull_request:
    branches: [ main ]
  schedule:
    - cron: '0 0 * * 0' # Weekly scan

jobs:
  analyze:
    name: Analyze
    runs-on: ubuntu-latest
    permissions:
      actions: read
      contents: read
      security-events: write
    
    steps:
    - name: Checkout repository
      uses: actions/checkout@v4
    
    - name: Initialize CodeQL
      uses: github/codeql-action/init@v3
      with:
        languages: java
        queries: security-and-quality
    
    - name: Build
      run: mvn clean compile -DskipTests
    
    - name: Perform CodeQL Analysis
      uses: github/codeql-action/analyze@v3

SonarCloud Quality Gates

Required quality gate thresholds:

✅ Security Rating: A (0 vulnerabilities)
✅ Security Hotspots: All reviewed
✅ Coverage: > 80%
✅ Duplications: < 3%
✅ Maintainability Rating: A

ISMS Compliance Mapping

ISO 27001:2022 Controls

A.8.1 - User Endpoint Devices: Secure code prevents endpoint exploitation
A.8.2 - Privileged Access Rights: Authorization checks enforce least privilege
A.8.3 - Information Access Restriction: Access control mechanisms properly implemented
A.8.8 - Management of Technical Vulnerabilities: Vulnerability scanning and remediation
A.8.28 - Secure Coding: Adherence to secure coding standards

NIST Cybersecurity Framework

PR.DS-6: Integrity checking mechanisms in place
PR.IP-1: Baseline security configurations
DE.CM-4: Malicious code detected
RS.AN-5: Processes for vulnerability response

CIS Controls v8

Control 16.11: Remediate detected vulnerabilities
Control 4.1: Establish and maintain secure configuration
Control 16.1: Application software security
Control 16.14: Establish process for accepting application risk

Hack23 ISMS Policy References

Review these policies before code review:

Secure Development Policy: https://github.com/Hack23/ISMS-PUBLIC/blob/main/policies/secure-development-policy.md
Vulnerability Management Policy: https://github.com/Hack23/ISMS-PUBLIC/blob/main/policies/vulnerability-management-policy.md
Access Control Policy: https://github.com/Hack23/ISMS-PUBLIC/blob/main/policies/access-control-policy.md

Review Workflow

Pre-Review Setup

# Checkout PR branch
gh pr checkout <PR-NUMBER>

# Run security scans
mvn clean verify
mvn org.owasp:dependency-check-maven:check

# Review CodeQL alerts
gh api /repos/Hack23/cia/code-scanning/alerts --jq '.[] | select(.state=="open")'

Manual Code Review
- Apply OWASP Top 10 checklist to changed files
- Review authentication/authorization changes
- Verify input validation on all user inputs
- Check for hardcoded secrets or credentials
- Validate error handling doesn't leak sensitive info

Document Findings

## Security Review Findings

### Critical Issues
- [ ] SQL injection in `PoliticianController.search()` - line 45

### High Priority
- [ ] Missing authorization check in `DocumentService.getDocument()`

### Medium Priority
- [ ] Weak password validation in `UserRegistrationForm`

### Low Priority / Informational
- [ ] Consider adding rate limiting to login endpoint

Request Changes or Approve
- If critical/high issues found: Request changes with detailed explanation
- If only medium/low: Approve with recommendations
- Always provide actionable feedback with code examples

Security Review Exit Criteria

Before approving a PR, verify:

✅ No new CodeQL alerts introduced
✅ SonarCloud quality gate passed
✅ OWASP Dependency Check shows no new high/critical CVEs
✅ All authentication/authorization logic reviewed
✅ Input validation present on all user inputs
✅ No secrets or credentials in code
✅ Security test cases added for sensitive features
✅ Documentation updated if security architecture changed

Hack23 ISMS Policy References

Secure Development Framework:

Secure Development Policy - Comprehensive SDLC security requirements
Information Security Policy - Master governance and security principles
Cryptography Policy - Approved algorithms and key management
Vulnerability Management - SLAs and remediation procedures
Threat Modeling - STRIDE framework and attack trees
Change Management - Secure change control procedures
Incident Response Plan - Security incident handling

All Hack23 ISMS Policies: https://github.com/Hack23/ISMS-PUBLIC

CIA Platform Architecture References

Security Architecture: CIA SECURITY_ARCHITECTURE.md - Current security controls
Future Security: CIA FUTURE_SECURITY_ARCHITECTURE.md - Planned enhancements
Threat Model: CIA THREAT_MODEL.md - STRIDE analysis and attack trees
Architecture: CIA ARCHITECTURE.md - System design
Security Policy: CIA SECURITY.md - Vulnerability disclosure

References

Official Standards

OWASP Top 10: https://owasp.org/www-project-top-ten/
OWASP ASVS: https://owasp.org/www-project-application-security-verification-standard/
CWE Top 25: https://cwe.mitre.org/top25/
SANS Top 25: https://www.sans.org/top25-software-errors/
ISO 27001:2022: https://www.iso.org/standard/27001
NIST SP 800-53: https://csrc.nist.gov/publications/detail/sp/800-53/rev-5/final

Tools & Frameworks

CodeQL: https://codeql.github.com/
SonarCloud: https://sonarcloud.io/
OWASP Dependency Check: https://owasp.org/www-project-dependency-check/
SpotBugs with FindSecBugs: https://find-sec-bugs.github.io/
Spring Security: https://docs.spring.io/spring-security/reference/

Success Metrics

Track these KPIs to measure secure code review effectiveness:

Zero security vulnerabilities in production
Mean time to remediate (MTTR) for vulnerabilities < 30 days
100% of PRs pass security review before merge
Decrease in vulnerability count over time
Security training completion rate for all contributors

secure-code-review

Secure Code Review Skill

Purpose

When to Use This Skill

Decision Tree

OWASP Top 10 Security Review Checklist

A01:2021 – Broken Access Control

A02:2021 – Cryptographic Failures

A03:2021 – Injection

A04:2021 – Insecure Design

A05:2021 – Security Misconfiguration

A06:2021 – Vulnerable and Outdated Components

A07:2021 – Identification and Authentication Failures

A08:2021 – Software and Data Integrity Failures

A09:2021 – Security Logging and Monitoring Failures

A10:2021 – Server-Side Request Forgery (SSRF)

SAST/DAST Integration

CodeQL Configuration

SonarCloud Quality Gates

ISMS Compliance Mapping

ISO 27001:2022 Controls

NIST Cybersecurity Framework

CIS Controls v8

Hack23 ISMS Policy References

Review Workflow

Security Review Exit Criteria

Hack23 ISMS Policy References

CIA Platform Architecture References

References

Official Standards

Tools & Frameworks

Success Metrics