mirror of
https://github.com/apache/struts.git
synced 2026-08-07 07:37:20 +00:00
Merge pull request #1306 from apache/fix/clean-uploaded-files
Cleans up all uploaded files
This commit is contained in:
Lukasz Lenart
+7
-4
@@ -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/
|
||||
|
||||
@@ -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)
|
||||
<bean type="com.opensymphony.xwork2.ObjectFactory"
|
||||
name="myObjectFactory"
|
||||
class="com.example.MyObjectFactory" />
|
||||
```
|
||||
|
||||
### 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).
|
||||
+35
-9
@@ -42,7 +42,6 @@ import java.util.Enumeration;
|
||||
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<String, List<String>> params = new HashMap<>();
|
||||
|
||||
// List to track all FileItem instances for comprehensive cleanup
|
||||
protected List<FileItem> 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 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
|
||||
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 class JakartaMultiPartRequest extends AbstractMultiPartRequest {
|
||||
// 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 class JakartaMultiPartRequest extends AbstractMultiPartRequest {
|
||||
* @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp()
|
||||
*/
|
||||
public void cleanUp() {
|
||||
Set<String> names = files.keySet();
|
||||
for (String name : names) {
|
||||
List<FileItem> 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();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+183
@@ -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<FileItem> 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<FileItem> getAllFileItems() {
|
||||
return allFileItems;
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user