code-review

Reviewing Apache Struts pull requests

Apache Struts is a mature MVC framework for Java. It uses OGNL for value stack expressions and FreeMarker for UI tag templates, and it has a long history of security vulnerabilities in exactly those areas. Review accordingly: the conventions below are not style preferences, they are the places where changes tend to go wrong in this codebase.

Deeper references, when a review needs them:

• CLAUDE.md — build commands, module layout, request lifecycle
• SECURITY.md — the vulnerability reporting process (source of truth)
• THREAT_MODEL.md — scope, trust boundaries, known non-findings
• AGENTS.md — rules for AI agents working on security findings

1. Tests

This repository is JUnit 4. There is no JUnit 5 anywhere in it. parent/pom.xml declares junit:junit:4.13.2; there are zero org.junit.jupiter imports. Two styles coexist and a new test must match the style already in the file it joins:

• JUnit 3 style — classes extending XWorkTestCase, which extends junit.framework.TestCase. Test methods must be named testXxx(). A Jupiter @Test annotation added to one of these silently never runs — it does not fail, it is simply not collected. Flag this as blocking whenever you see org.junit.jupiter in a diff.
• JUnit 4 style — classes using import org.junit.Test.

Both styles are widespread and neither is being migrated away from.

AssertJ assertions and Mockito mocks are both available and widely used. Introducing JUnit 5 is a build-infrastructure change that needs its own WW- ticket; it is never a side effect of a feature PR.

Tests that pass without testing anything

Three traps in this codebase produce green tests that assert nothing. Check for them whenever a PR adds a test in these areas:

• Unpushed action. An action object that was never pushed onto the value stack binds no parameters at all, so an assertion that "the parameter was not bound" passes for the wrong reason. Confirm the fixture pushes the action.
• requireAnnotations is off by default in a bare harness. ParametersInterceptor.requireAnnotations is a Java field initialised to false; production turns it on through struts.parameters.requireAnnotations=true in default.properties. A test that constructs the interceptor directly and expects an unannotated parameter to be rejected will pass with the check disabled. The test must set the flag.
• Interned string literals defeat identity checks. Where the framework compares against a marker constant with ==, a String literal in a test is interned to the same instance as the constant, so the test passes vacuously even against unfixed code. Such fixtures must build the value at runtime, with an assertNotSame guard proving they did.