diff --git a/.gitignore b/.gitignore index af4e6813cc..4105b54628 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,5 @@ # IDEA -.idea +.idea/ *.iml *.ipr *.iws @@ -38,11 +38,14 @@ buildNumber.properties .mvn/timing.properties .mvn/wrapper/maven-wrapper.jar -plugins/testng/test-output -test-output +plugins/testng/test-output/ +test-output/ # Sonar -/.sonar/ +.sonar/ # Tidelift CLI scanner .tidelift + +# Claude Code specific local settings +.claude/ diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..3710f03544 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,160 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Apache Struts 2 Framework (Version 6.7.x) + +This is the Apache Struts 2 web framework, a free open-source solution for creating Java web applications. The codebase is a multi-module Maven project with comprehensive plugin architecture. + +## Build System & Common Commands + +### Maven Commands +```bash +# Build entire project +./mvnw clean install + +# Build without running tests +./mvnw clean install -DskipTests + +# Build without assembly +./mvnw clean install -DskipAssembly + +# Run all tests +./mvnw clean test + +# Run tests with coverage +./mvnw clean verify -Pcoverage -DskipAssembly + +# Run integration tests +./mvnw clean verify -DskipAssembly + +# Test specific module +./mvnw -pl core clean test +./mvnw -pl plugins/spring clean test + +# Check for security vulnerabilities +./mvnw clean verify -Pdependency-check + +# Run Apache RAT license check +./mvnw clean prepare-package +``` + +### Test Framework +- **Primary**: JUnit 4.13.2 with Maven Surefire Plugin 3.5.1 +- **Pattern**: `**/*Test.java` (excludes `**/TestBean.java`) +- **Coverage**: JaCoCo 0.8.12 +- **Additional**: Mockito, EasyMock, AssertJ, Spring Test +- **Integration**: Maven Failsafe Plugin with Jetty on port 8090 + +## Project Architecture + +### Core Structure +``` +struts6/ +├── core/ # Core Struts 2 framework (main dependency) +├── plugins/ # Plugin modules +│ ├── spring/ # Spring integration +│ ├── json/ # JSON support +│ ├── tiles/ # Apache Tiles integration +│ ├── velocity/ # Velocity template engine +│ └── [20+ other plugins] +├── apps/ # Sample applications +│ ├── showcase/ # Feature demonstration app +│ └── rest-showcase/ # REST API examples +├── bundles/ # OSGi bundles +├── bom/ # Bill of Materials +└── assembly/ # Distribution packaging +``` + +### Key Technologies +- **Java**: Minimum JDK 8, supports up to JDK 21 +- **Servlet API**: 3.1+ required +- **Expression Language**: OGNL 3.3.5 +- **Dependency Injection**: Custom container (`com.opensymphony.xwork2.inject`) +- **Templating**: FreeMarker 2.3.33 (default), Velocity, JSP +- **Logging**: SLF4J 2.0.16 with Log4j2 2.24.1 +- **Build**: Maven with wrapper (3.9.6) + +### Core Components Architecture + +#### Action Framework (MVC Pattern) +- **Actions**: Located in `core/src/main/java/org/apache/struts2/action/` +- **Action Support**: `ActionSupport` base class with validation and i18n +- **Action Context**: `ActionContext` provides access to servlet objects +- **Action Invocation**: `DefaultActionInvocation` handles action execution + +#### Configuration System +- **XML-based**: Primary configuration via `struts.xml` files +- **Annotation-based**: Convention plugin for zero-config approach +- **Java-based**: `StrutsJavaConfiguration` for programmatic setup +- **Property files**: `struts.properties` for framework settings + +#### Interceptor Chain +- **Framework Core**: All requests processed through interceptor chain +- **Built-in Interceptors**: 20+ interceptors in `org.apache.struts2.interceptor` +- **Validation**: `ValidationInterceptor` with annotation support +- **File Upload**: `FileUploadInterceptor` with security controls +- **Parameters**: `ParametersInterceptor` with OGNL expression handling + +#### Result Framework +- **Result Types**: JSP, FreeMarker, Redirect, Stream, JSON, etc. +- **Chaining**: `ActionChainResult` for action-to-action calls +- **Templates**: Pluggable result renderers + +#### Value Stack (OGNL Integration) +- **Expression Language**: OGNL for property access and method calls +- **Security**: `SecurityMemberAccess` prevents dangerous operations +- **Performance**: Caffeine-based expression caching +- **Context**: CompoundRoot provides hierarchical value resolution + +### Plugin Architecture +Each plugin is a separate Maven module with: +- **Plugin Descriptor**: `struts-plugin.xml` defines beans and configuration +- **Dependency Isolation**: Separate classloaders for plugin resources +- **Extension Points**: Configurable via dependency injection +- **Popular Plugins**: Spring (DI), JSON (REST), Tiles (Layout), Bean Validation (JSR-303) + +### Security Architecture +- **OGNL Security**: Restricted method access and class loading +- **CSRF Protection**: Token-based with `TokenInterceptor` +- **File Upload Security**: Type and size restrictions +- **Content Security Policy**: Built-in CSP support +- **Input Validation**: Server-side validation framework +- **Pattern Matching**: Configurable allowed/excluded patterns + +## Development Guidelines + +### Code Organization +- **Package Structure**: Follow existing `org.apache.struts2.*` hierarchy +- **Naming Conventions**: Use Struts conventions (Actions end with `Action`) +- **Configuration**: Prefer XML configuration in `struts.xml` for complex setups +- **Testing**: Each module has comprehensive unit and integration tests + +### Plugin Development +```java +// Plugin descriptor example (struts-plugin.xml) + +``` + +### Common Patterns +- **Action Implementation**: Extend `ActionSupport` or implement `Action` +- **Result Mapping**: Use result configuration in `struts.xml` +- **Interceptor Development**: Extend `AbstractInterceptor` +- **Type Conversion**: Implement `TypeConverter` for custom types +- **Validation**: Use validation XML or annotations + +### Important Notes +- **Version**: Currently 6.7.5-SNAPSHOT (release branch: `release/struts-6-7-x`) +- **Java Compatibility**: Compiled for Java 8, tested through Java 21 +- **Security**: Always validate inputs and follow OWASP guidelines +- **Performance**: Leverage built-in caching (OGNL expressions, templates) +- **Deprecation**: Some legacy XWork components marked for removal + +### Build Profiles +- **coverage**: Enables JaCoCo coverage reporting +- **dependency-check**: OWASP dependency vulnerability scanning +- **jdk17**: Special configuration for Java 17+ module system + +This is a mature, enterprise-grade framework with extensive documentation at https://struts.apache.org/ and active community support through Apache mailing lists and JIRA (project WW). \ No newline at end of file diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java index d55e350015..d27b2159d0 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java @@ -42,7 +42,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Set; import static org.apache.commons.lang3.StringUtils.normalizeSpace; @@ -59,6 +58,9 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { // maps parameter name -> List of param values protected Map> params = new HashMap<>(); + // List to track all FileItem instances for comprehensive cleanup + protected List allFileItems = new ArrayList<>(); + /** * Creates a new request wrapper to handle multi-part data using methods adapted from Jason Pell's * multipart classes (see class description). @@ -103,6 +105,10 @@ protected void processUpload(HttpServletRequest request, String saveDir) throws if (ServletFileUpload.isMultipartContent(request)) { for (FileItem item : parseRequest(request, saveDir)) { LOG.debug("Found file item: [{}]", normalizeSpace(item.getFieldName())); + + // Track all FileItem instances for comprehensive cleanup + allFileItems.add(item); + if (item.isFormField()) { processNormalFormField(item, request.getCharacterEncoding()); } else { @@ -240,7 +246,11 @@ public UploadedFile[] getFile(String fieldName) { // Ensure file exists even if it is empty. if (diskFileItem.getSize() == 0 && storeLocation != null && !storeLocation.exists()) { try { - storeLocation.createNewFile(); + if (storeLocation.createNewFile()) { + LOG.debug("File {} has been created", storeLocation.getAbsolutePath()); + } else { + LOG.warn("File {} already exists", storeLocation.getAbsolutePath()); + } } catch (IOException e) { LOG.error("Cannot write uploaded empty file to disk: {}", storeLocation.getAbsolutePath(), e); } @@ -357,15 +367,31 @@ public InputStream getInputStream() throws IOException { * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp() */ public void cleanUp() { - Set names = files.keySet(); - for (String name : names) { - List items = files.get(name); - for (FileItem item : items) { - LOG.debug("Removing file [{}]", normalizeSpace(name)); - if (!item.isInMemory()) { - item.delete(); + try { + LOG.debug("Performing comprehensive cleanup for {} file items.", allFileItems.size()); + for (FileItem item : allFileItems) { + try { + if (item instanceof DiskFileItem) { + DiskFileItem diskItem = (DiskFileItem) item; + File storeLocation = diskItem.getStoreLocation(); + if (storeLocation != null && storeLocation.exists()) { + LOG.debug("Deleting temporary file: [{}]", storeLocation.getName()); + if (!storeLocation.delete()) { + LOG.warn("Unable to delete temporary file: [{}]", storeLocation.getName()); + } + } + } + // Also call the item's delete method as backup + if (!item.isInMemory()) { + item.delete(); + } + } catch (Exception e) { + LOG.warn("Error during cleanup of file item: [{}]", normalizeSpace(item.getFieldName()), e); } } + } finally { + // Clear only the tracking collection, preserve parsed data + allFileItems.clear(); } } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java new file mode 100644 index 0000000000..9c6abcd080 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java @@ -0,0 +1,183 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.dispatcher.multipart; + +import org.apache.commons.fileupload.FileItem; +import org.apache.struts2.StrutsInternalTestCase; +import org.springframework.mock.web.MockHttpServletRequest; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +/** + * Test cases for {@link JakartaMultiPartRequest} that verify security-related functionality, + * specifically comprehensive cleanup of temporary files. + */ +public class JakartaMultiPartRequestTest extends StrutsInternalTestCase { + + private File tempDir; + private JakartaMultiPartRequest multiPartRequest; + + @Override + protected void setUp() throws Exception { + super.setUp(); + + // Create a temporary directory for test files + Path tempPath = Files.createTempDirectory("struts-multipart-test"); + tempDir = tempPath.toFile(); + + multiPartRequest = new TestableJakartaMultiPartRequest(); + multiPartRequest.setMaxSize("2048"); + multiPartRequest.setMaxFiles("10"); + multiPartRequest.setMaxFileSize("1024"); + } + + @Override + protected void tearDown() throws Exception { + if (tempDir != null && tempDir.exists()) { + // Clean up test directory + File[] files = tempDir.listFiles(); + if (files != null) { + for (File file : files) { + file.delete(); + } + } + tempDir.delete(); + } + super.tearDown(); + } + + /** + * Test that comprehensive cleanup removes all temporary files created during multipart processing. + * This addresses the security vulnerability where temporary files could be leaked. + */ + public void testComprehensiveCleanupRemovesAllTempFiles() throws Exception { + // Create a mock multipart request with both file upload and form field + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setContentType("multipart/form-data; boundary=----WebKitFormBoundary7MA4YWxkTrZu0gW"); + request.setMethod("POST"); + + String multipartContent = createMultipartContent(); + request.setContent(multipartContent.getBytes(StandardCharsets.UTF_8)); + + // Count files before processing + int filesBefore = countTempFiles(); + + // Process the multipart request + multiPartRequest.parse(request, tempDir.getAbsolutePath()); + + // Count files after processing (should be more due to temp files) + int filesAfterProcessing = countTempFiles(); + + // Verify that temp files were created during processing + assertTrue("Temporary files should be created during multipart processing", + filesAfterProcessing > filesBefore); + + // Perform cleanup + multiPartRequest.cleanUp(); + + // Count files after cleanup + int filesAfterCleanup = countTempFiles(); + + // Verify comprehensive cleanup removed all temporary files + assertEquals("All temporary files should be cleaned up", filesBefore, filesAfterCleanup); + } + + /** + * Test that all FileItem instances are tracked for cleanup, including both file uploads and form fields. + */ + public void testAllFileItemsAreTrackedForCleanup() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setContentType("multipart/form-data; boundary=----WebKitFormBoundary7MA4YWxkTrZu0gW"); + request.setMethod("POST"); + + String multipartContent = createMultipartContentWithMultipleFields(); + request.setContent(multipartContent.getBytes(StandardCharsets.UTF_8)); + + // Process the multipart request + multiPartRequest.parse(request, tempDir.getAbsolutePath()); + + // Access the tracked items through our testable implementation + TestableJakartaMultiPartRequest testable = (TestableJakartaMultiPartRequest) multiPartRequest; + List trackedItems = testable.getAllFileItems(); + + // Verify that all items (both files and form fields) are tracked + assertTrue("Should track multiple FileItem instances", trackedItems.size() >= 3); + + // Verify tracking includes both form fields and file uploads + boolean hasFormField = false; + boolean hasFileUpload = false; + + for (FileItem item : trackedItems) { + if (item.isFormField()) { + hasFormField = true; + } else { + hasFileUpload = true; + } + } + + assertTrue("Should track form field items", hasFormField); + assertTrue("Should track file upload items", hasFileUpload); + } + + private String createMultipartContent() { + return "------WebKitFormBoundary7MA4YWxkTrZu0gW\r\n" + + "Content-Disposition: form-data; name=\"textField\"\r\n\r\n" + + "test value\r\n" + + "------WebKitFormBoundary7MA4YWxkTrZu0gW\r\n" + + "Content-Disposition: form-data; name=\"fileField\"; filename=\"test.txt\"\r\n" + + "Content-Type: text/plain\r\n\r\n" + + "file content\r\n" + + "------WebKitFormBoundary7MA4YWxkTrZu0gW--\r\n"; + } + + private String createMultipartContentWithMultipleFields() { + return "------WebKitFormBoundary7MA4YWxkTrZu0gW\r\n" + + "Content-Disposition: form-data; name=\"textField1\"\r\n\r\n" + + "value1\r\n" + + "------WebKitFormBoundary7MA4YWxkTrZu0gW\r\n" + + "Content-Disposition: form-data; name=\"textField2\"\r\n\r\n" + + "value2\r\n" + + "------WebKitFormBoundary7MA4YWxkTrZu0gW\r\n" + + "Content-Disposition: form-data; name=\"fileField\"; filename=\"test.txt\"\r\n" + + "Content-Type: text/plain\r\n\r\n" + + "file content\r\n" + + "------WebKitFormBoundary7MA4YWxkTrZu0gW--\r\n"; + } + + private int countTempFiles() { + if (tempDir == null || !tempDir.exists()) { + return 0; + } + File[] files = tempDir.listFiles(); + return files != null ? files.length : 0; + } + + /** + * Testable subclass that exposes internal state for verification + */ + private static class TestableJakartaMultiPartRequest extends JakartaMultiPartRequest { + public List getAllFileItems() { + return allFileItems; + } + } +} \ No newline at end of file