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`](../../../CLAUDE.md) — build commands, test style (JUnit 3 vs 4), PR conventions - [`SECURITY.md`](../../../SECURITY.md) — the vulnerability reporting process (source of truth) - [`THREAT_MODEL.md`](../../../THREAT_MODEL.md) — scope, trust boundaries, known non-findings - [`AGENTS.md`](../../../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. Run tests with `mvn test -DskipAssembly`; a single test with `mvn test -DskipAssembly -pl core -Dtest=MyClassTest#testMethodName`. ## 2. Security-sensitive changes Watch for diffs touching OGNL expression evaluation, the OGNL allowlist and member access policies, parameter filtering and `@StrutsParameter` gating, file upload handling, action mapping and name cleanup, deserialization, path handling, or escaping in framework components and UI tag templates. **When a change looks like it fixes a vulnerability rather than an ordinary bug, say so in neutral terms and stop there.** A pull request is public, so a review comment that names the weakness, explains how it is reached, or estimates its impact is itself a disclosure — precisely what the project's private process exists to prevent. Use wording of this shape, and no more than this: > This change touches security-sensitive framework code. Please confirm it is > not a fix for a suspected vulnerability before merging — see `SECURITY.md`. > Vulnerability fixes go through the private process at > `security@struts.apache.org`, not a public pull request. Do **not**, in a review comment: describe the suspected weakness or its class, sketch an exploit or a triggering input, assess exploitability or severity, speculate about affected versions, or link the change to a specific CVE or security bulletin. If a reviewer needs to raise any of that, it belongs in private mail to `security@struts.apache.org`. ## 3. Pull request and commit hygiene - **Title** — `WW-XXXX Description`. A Jira ticket ID is required for any code change; the tracker is <https://issues.apache.org/jira/projects/WW>. - **Description** — links the ticket: `Fixes [WW-XXXX](https://issues.apache.org/jira/browse/WW-XXXX)`. - **Exception** — pure documentation and build/CI changes (`SECURITY.md`, `AGENTS.md`, `CLAUDE.md`, `.github/`, workflows) take no ticket and use conventional-commit form instead: `docs: ...`, `build(ci): ...`, `chore: ...`. - Commit messages follow the same rule as the title. Flag a missing or malformed ticket reference as a non-blocking comment, not as a code defect. ## 4. Struts implementation patterns - **Temporary files** get UUID-based names in a controlled location, never a name derived from user input: ```java protected File createTemporaryFile(String fileName, Path location) { String uid = UUID.randomUUID().toString().replace("-", "_"); return location.resolve("upload_" + uid + ".tmp").toFile(); } ``` - **OGNL** — evaluate only framework-generated expressions, and keep member access on an allowlist. Treat any new path that evaluates a request-derived string as OGNL as a blocking finding under section 2. - **Parameters** — request-settable action properties need `@StrutsParameter`; use `ParameterNameAware` to restrict accepted parameter names. Note that a `ModelDriven` action's own setters are a known exemption, so review changes there with care. - **Uploads** — validate content types, sanitise file names, enforce size limits. - **Naming** — framework default implementations are prefixed `Struts` (`StrutsBeanSelectionProvider`), not `Default`, for new classes. - **No placeholder TODOs** — a comment must not reference a Jira ticket that has not been filed. ## Reviewing the review Keep findings proportionate. Blocking comments are for correctness, the JUnit 5 trap, vacuous tests, and security-sensitive code paths. Everything else — naming, ticket references, style — is a suggestion or a nitpick, and should be labelled as such.