|
| 1 | +# Claude Code Best Practices for Apache Struts |
| 2 | + |
| 3 | +This document outlines essential practices for working with Claude Code on the Apache Struts project, based on security improvements and testing implementations. |
| 4 | + |
| 5 | +## Project Context |
| 6 | +- **Framework**: Apache Struts 2 (Java-based web framework) |
| 7 | +- **Technology Stack**: Jakarta EE, Maven, Java 17 |
| 8 | +- **Key Libraries**: OGNL, Commons FileUpload2, Log4j2, JUnit, AssertJ |
| 9 | +- **Build System**: Maven with standard lifecycle |
| 10 | + |
| 11 | +## Security-First Development |
| 12 | + |
| 13 | +### Critical Security Principles |
| 14 | +1. **Never create files in system temp directories** - always use controlled application directories |
| 15 | +2. **Use UUID-based naming** for temporary files to prevent collisions and path traversal |
| 16 | +3. **Implement proper resource cleanup** with try-with-resources and finally blocks |
| 17 | +4. **Track all temporary resources** for explicit cleanup (security critical) |
| 18 | +5. **Validate all user inputs** and sanitize filenames before processing |
| 19 | + |
| 20 | +### Security Implementation Patterns |
| 21 | +```java |
| 22 | +// GOOD: Secure temporary file creation |
| 23 | +protected File createTemporaryFile(String fileName, Path location) { |
| 24 | + String uid = UUID.randomUUID().toString().replace("-", "_"); |
| 25 | + File file = location.resolve("upload_" + uid + ".tmp").toFile(); |
| 26 | + LOG.debug("Creating temporary file: {} (originally: {})", file.getName(), fileName); |
| 27 | + return file; |
| 28 | +} |
| 29 | + |
| 30 | +// BAD: Insecure system temp usage |
| 31 | +File tempFile = File.createTempFile("struts_upload_", "_" + item.getName()); |
| 32 | +``` |
| 33 | + |
| 34 | +### Resource Management |
| 35 | +- Always use tracking collections for cleanup: `List<File> temporaryFiles`, `List<DiskFileItem> diskFileItems` |
| 36 | +- Implement protected cleanup methods for extensibility |
| 37 | +- Make cleanup idempotent and exception-safe |
| 38 | +- Use try-with-resources for streams and I/O operations |
| 39 | + |
| 40 | +## Testing Implementation |
| 41 | + |
| 42 | +### Test Structure & Coverage |
| 43 | +- **Unit Tests**: Test individual methods with mocked dependencies |
| 44 | +- **Integration Tests**: Test complete workflows with real file I/O |
| 45 | +- **Security Tests**: Verify directory traversal prevention, secure naming |
| 46 | +- **Error Handling Tests**: Test exception scenarios and error reporting |
| 47 | +- **Cleanup Tests**: Verify resource cleanup and tracking |
| 48 | + |
| 49 | +### Testing Commands |
| 50 | +```bash |
| 51 | +# Run all tests |
| 52 | +mvn test -DskipAssembly |
| 53 | + |
| 54 | +# Run specific test class |
| 55 | +mvn test -Dtest=JakartaMultiPartRequestTest |
| 56 | + |
| 57 | +# Run tests with specific method pattern |
| 58 | +mvn test -Dtest=*MultiPartRequestTest#temporal* |
| 59 | +``` |
| 60 | + |
| 61 | +use `-DskipAssembly` to avoid building zip files with docs, examples, etc. |
| 62 | + |
| 63 | +### Test Implementation Patterns |
| 64 | +```java |
| 65 | +@Test |
| 66 | +public void securityTestExample() throws Exception { |
| 67 | + // given - malicious input |
| 68 | + String maliciousFilename = "malicious../../../etc/passwd"; |
| 69 | + |
| 70 | + // when - process input |
| 71 | + processFile(maliciousFilename); |
| 72 | + |
| 73 | + // then - verify security measures |
| 74 | + assertThat(tempFile.getParent()).isEqualTo(saveDir); |
| 75 | + assertThat(tempFile.getName()).doesNotContain(".."); |
| 76 | +} |
| 77 | +``` |
| 78 | + |
| 79 | +### Reflection-Based Testing for Private Members |
| 80 | +```java |
| 81 | +Field privateField = ClassName.class.getDeclaredField("fieldName"); |
| 82 | +privateField.setAccessible(true); |
| 83 | +@SuppressWarnings("unchecked") |
| 84 | +List<Type> values = (List<Type>) privateField.get(instance); |
| 85 | +``` |
| 86 | + |
| 87 | +## JavaDoc Documentation Standards |
| 88 | + |
| 89 | +### Class-Level Documentation |
| 90 | +```java |
| 91 | +/** |
| 92 | + * Brief description of the class purpose and functionality. |
| 93 | + * |
| 94 | + * <p>Detailed description with multiple paragraphs explaining:</p> |
| 95 | + * <ul> |
| 96 | + * <li>Key features and capabilities</li> |
| 97 | + * <li>Security considerations</li> |
| 98 | + * <li>Resource management approach</li> |
| 99 | + * <li>Usage patterns and examples</li> |
| 100 | + * </ul> |
| 101 | + * |
| 102 | + * <p>Usage example:</p> |
| 103 | + * <pre> |
| 104 | + * ClassName instance = new ClassName(); |
| 105 | + * try { |
| 106 | + * instance.process(data); |
| 107 | + * } finally { |
| 108 | + * instance.cleanUp(); // Always clean up resources |
| 109 | + * } |
| 110 | + * </pre> |
| 111 | + * |
| 112 | + * @see RelatedClass |
| 113 | + * @see org.apache.package.ImportantInterface |
| 114 | + */ |
| 115 | +``` |
| 116 | + |
| 117 | +### Method-Level Documentation |
| 118 | +```java |
| 119 | +/** |
| 120 | + * Brief description of what the method does. |
| 121 | + * |
| 122 | + * <p>Detailed description explaining:</p> |
| 123 | + * <ol> |
| 124 | + * <li>Step-by-step process</li> |
| 125 | + * <li>Security considerations</li> |
| 126 | + * <li>Error handling behavior</li> |
| 127 | + * <li>Resource management</li> |
| 128 | + * </ol> |
| 129 | + * |
| 130 | + * <p>Security note: This method creates files in controlled directory |
| 131 | + * to prevent security vulnerabilities.</p> |
| 132 | + * |
| 133 | + * @param paramName description of parameter and constraints |
| 134 | + * @param saveDir the directory where files will be created (must exist) |
| 135 | + * @return description of return value |
| 136 | + * @throws IOException if file creation fails or I/O error occurs |
| 137 | + * @see #relatedMethod(Type) |
| 138 | + * @see #cleanUpMethod() |
| 139 | + */ |
| 140 | +``` |
| 141 | + |
| 142 | +### Documentation Best Practices |
| 143 | +- **Always document security implications** in methods handling files/user input |
| 144 | +- **Include usage examples** for complex methods and classes |
| 145 | +- **Document exception conditions** and error handling behavior |
| 146 | +- **Reference related methods** using `@see` tags |
| 147 | +- **Explain resource management** responsibilities |
| 148 | +- **Use `<p>`, `<ol>`, `<ul>`, `<li>` for structured content |
| 149 | +- **Include `<pre>` blocks** for code examples |
| 150 | + |
| 151 | +## Error Handling & Logging |
| 152 | + |
| 153 | +### Error Message Patterns |
| 154 | +```java |
| 155 | +// Localized error messages |
| 156 | +LocalizedMessage errorMessage = buildErrorMessage( |
| 157 | + e.getClass(), |
| 158 | + e.getMessage(), |
| 159 | + new Object[]{fileName} |
| 160 | +); |
| 161 | +if (!errors.contains(errorMessage)) { |
| 162 | + errors.add(errorMessage); |
| 163 | +} |
| 164 | +``` |
| 165 | + |
| 166 | +### Logging Best Practices |
| 167 | +```java |
| 168 | +// Use parameterized logging for performance |
| 169 | +LOG.debug("Processing file: {} in directory: {}", |
| 170 | + normalizeSpace(fileName), saveDir); |
| 171 | + |
| 172 | +// Log security-relevant operations |
| 173 | +LOG.warn("Failed to delete temporary file: {}", tempFile.getAbsolutePath()); |
| 174 | + |
| 175 | +// Use appropriate log levels |
| 176 | +LOG.debug() // Development details |
| 177 | +LOG.info() // General information |
| 178 | +LOG.warn() // Potential issues |
| 179 | +LOG.error() // Serious problems |
| 180 | +``` |
| 181 | + |
| 182 | +## Code Quality Standards |
| 183 | + |
| 184 | +### Method Scope & Extensibility |
| 185 | +- Use `protected` for methods that subclasses might override |
| 186 | +- Implement cleanup methods as separate `protected` methods |
| 187 | +- Make core functionality extensible while maintaining security |
| 188 | + |
| 189 | +### Exception Handling |
| 190 | +- Catch specific exceptions rather than generic `Exception` |
| 191 | +- Log exceptions with context but continue cleanup operations |
| 192 | +- Use try-finally blocks to ensure cleanup always occurs |
| 193 | + |
| 194 | +### Code Organization |
| 195 | +- Group related methods together (processing, cleanup, utilities) |
| 196 | +- Keep security-critical code in dedicated methods |
| 197 | +- Use clear, descriptive method and variable names |
| 198 | +- Follow existing project conventions and patterns |
| 199 | + |
| 200 | +## Common Pitfalls to Avoid |
| 201 | + |
| 202 | +1. **File Security**: Never use `File.createTempFile()` without directory control |
| 203 | +2. **Resource Leaks**: Always track and clean up temporary files |
| 204 | +3. **Test Coverage**: Don't forget to test error conditions and cleanup |
| 205 | +4. **Documentation**: Always document security implications |
| 206 | +5. **Exception Handling**: Don't let cleanup failures affect main operations |
| 207 | +6. **Path Validation**: Always validate and sanitize file paths |
| 208 | +7. **Reflection Testing**: Use `@SuppressWarnings("unchecked")` appropriately |
| 209 | + |
| 210 | +This document should be updated as new patterns and practices emerge during development. |
0 commit comments