DanielCarter-stack commented on PR #10628:
URL: https://github.com/apache/seatunnel/pull/10628#issuecomment-4095422284
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10628", "part": 1,
"total": 1} -->
### Issue 1: Missing test cases for path traversal attacks
**Location**:
`seatunnel-e2e/seatunnel-engine-e2e/connector-seatunnel-e2e-base/src/test/java/org/apache/seatunnel/engine/e2e/joblog/JobLogIT.java`
**Related context**:
- Modified classes: `RestHttpGetCommandProcessor`, `LogBaseServlet`
- Existing tests: `JobLogIT.java` (lines 108-136)
**Problem description**:
The PR fixes a path traversal vulnerability, but **no test cases were added
to verify the fix's effectiveness**. Although existing `JobLogIT` tests normal
log reading functionality, it doesn't test attack scenarios (such as
`../../etc/passwd`).
**Potential risks**:
- Cannot automatically verify security fixes through CI/CD
- Future modifications may inadvertently break protection logic
- Reduces confidence in fix effectiveness
**Impact scope**:
- Direct impact: `JobLogIT` test suite
- Indirect impact: Security vulnerability regression risk
**Severity**: MAJOR
**Improvement suggestions**:
```java
// Add test method in JobLogIT.java:
@Test
public void testPathTraversalAttackPrevention() throws IOException,
InterruptedException {
// Test Unix path traversal
Container.ExecResult result = server.execInContainer("sh", "-c",
"curl -s -o /dev/null -w '%{http_code}'
http://localhost:8080/log/../../etc/passwd";);
Assertions.assertEquals("400", result.getStdout().trim(),
"Path traversal attack should be blocked with HTTP 400");
// Test Windows style path traversal (if applicable)
result = server.execInContainer("sh", "-c",
"curl -s -o /dev/null -w '%{http_code}'
'http://localhost:8080/log/..\\..\\..\\..\\etc\\passwd'");
Assertions.assertEquals("400", result.getStdout().trim(),
"Windows-style path traversal should also be blocked");
// Test symlink attack (if operable in container)
// server.execInContainer("sh", "-c", "ln -s /etc/passwd
/tmp/seatunnel/logs/evil.log");
// result = server.execInContainer("sh", "-c",
// "curl -s -o /dev/null -w '%{http_code}'
http://localhost:8080/log/evil.log";);
// Assertions.assertEquals("400", result.getStdout().trim());
}
```
**Rationale**: Security fixes must be verified through automated testing to
prevent regression
---
### Issue 2: Incomplete JavaDoc comments
**Location**:
- `RestHttpGetCommandProcessor.java:324`
- `LogBaseServlet.java:40`
**Related context**:
- Method signature: `private void prepareLogResponse(HttpGetCommand
httpGetCommand, String logPath, String logName)`
- Parent class/interface: None
- Callers: `getCurrentNodeLog()`, `getAllNodeLog()`
**Problem description**:
The modified method changed from JavaDoc-style comments (`/** */`) to
single-line comments (`//`), without explaining:
- Parameter meanings (`logPath`, `logName`)
- Path validation logic
- Security check details
- Exception handling behavior
**Potential risks**:
- Reduces code maintainability
- Difficult for new developers to understand security logic
- May lead to incorrect modifications
**Impact scope**:
- Direct impact: Readability of `prepareLogResponse()` method
- Indirect impact: Future code review and maintenance
**Severity**: MINOR
**Improvement suggestions**:
```java
/**
* Prepares log file response with path traversal protection.
*
* <p>This method validates that the requested log file is within the
configured
* log directory boundary to prevent CWE-22 path traversal attacks.
*
* <p>The validation is performed by:
* <ol>
* <li>Resolving both log directory and requested file to canonical paths
* (which resolves all {@code ..} and symbolic links)</li>
* <li>Verifying the canonical file path starts with the canonical log
directory
* followed by {@link File#separator}</li>
* </ol>
*
* <p>Requests attempting to access files outside the log directory are
rejected
* with HTTP 400 and a warning is logged.
*
* @param httpGetCommand the HTTP GET command to send response
* @param logPath the configured log directory path (must not be null/empty)
* @param logName the requested log file name from URI parameter
* @throws IOException if canonical path resolution fails (results in HTTP
400)
*/
private void prepareLogResponse(HttpGetCommand httpGetCommand, String
logPath, String logName) {
// ... implementation
}
```
**Rationale**: Security-critical code must include detailed documentation to
ensure long-term maintainability
---
### Issue 3: Security audit logs not detailed enough
**Location**:
- `RestHttpGetCommandProcessor.java:332`
- `LogBaseServlet.java:54`
**Related context**:
- Log statement: `logger.warning(String.format("Illegal log file path
detected: %s", logName))`
**Problem description**:
When a path traversal attack is detected, the log only records `logName`,
not:
- Client IP address
- Complete request URI
- Constructed complete file path
- Normalized path
**Potential risks**:
- Difficult to trace attack sources
- Difficult to analyze attack patterns
- Unable to perform security incident traceability
**Impact scope**:
- Direct impact: Security monitoring and incident response
- Indirect impact: Compliance requirements (such as SOC 2, ISO 27001)
**Severity**: MINOR
**Improvement suggestions**:
```java
// RestHttpGetCommandProcessor.java
if (!canonicalFilePath.startsWith(canonicalLogDir + File.separator)
&& !canonicalFilePath.equals(canonicalLogDir)) {
// Enhanced audit logging
String clientAddress = httpGetCommand.getSocketAddress().toString();
logger.warning(String.format(
"Path traversal attempt blocked - Client: %s, Requested path: %s,
Resolved path: %s, Log directory: %s",
clientAddress, logName, canonicalFilePath, canonicalLogDir));
httpGetCommand.send400();
return;
}
// LogBaseServlet.java
if (!canonicalFilePath.startsWith(canonicalLogDir + File.separator)
&& !canonicalFilePath.equals(canonicalLogDir)) {
String clientAddress = req.getRemoteAddr();
String requestUri = req.getRequestURI();
log.warn("Path traversal attempt blocked - Client: {}, URI: {},
Requested: {}, Resolved: {}, LogDir: {}",
clientAddress, requestUri, logName, canonicalFilePath,
canonicalLogDir);
resp.setStatus(HttpServletResponse.SC_BAD_REQUEST);
return;
}
```
**Rationale**: Security incidents should record complete audit trail
information
---
### Issue 4: Missing specific handling for `getCanonicalPath()` IOException
**Location**:
- `RestHttpGetCommandProcessor.java:337`
- `LogBaseServlet.java:59`
**Related context**:
- Exception handling: `} catch (SeaTunnelRuntimeException | IOException e)`
**Problem description**:
`IOException` may be triggered by multiple reasons:
1. `getCanonicalPath()` failure (I/O error)
2. `readFileToStr()` failure (file doesn't exist or no permission)
The current implementation uniformly returns HTTP 400 and the same error
message for all cases, resulting in:
- Unable to distinguish between "path traversal attack" and "file doesn't
exist"
- Error message "Log file content is empty" is inaccurate for I/O errors
**Potential risks**:
- Reduces problem diagnosis capability
- Attackers may fingerprint the system through I/O errors
**Impact scope**:
- Direct impact: Error diagnosis and log analysis
- Indirect impact: Operational efficiency
**Severity**: MINOR
**Improvement suggestions**:
```java
private void prepareLogResponse(HttpGetCommand httpGetCommand, String
logPath, String logName) {
String logFilePath = logPath + "/" + logName;
try {
String canonicalLogDir = new File(logPath).getCanonicalPath();
String canonicalFilePath = new File(logFilePath).getCanonicalPath();
if (!canonicalFilePath.startsWith(canonicalLogDir + File.separator)
&& !canonicalFilePath.equals(canonicalLogDir)) {
httpGetCommand.send400();
logger.warning(String.format("Illegal log file path detected:
%s", logName));
return;
}
String logContent = FileUtils.readFileToStr(new
File(canonicalFilePath).toPath());
this.prepareResponse(httpGetCommand, logContent);
} catch (IOException e) {
// getCanonicalPath() failed - possibly I/O error or permission issue
httpGetCommand.send400();
logger.warning(String.format("Failed to resolve log file path: %s,
error: %s",
logFilePath, e.getMessage()));
} catch (SeaTunnelRuntimeException e) {
// readFileToStr() failed - file not found or read error
httpGetCommand.send400();
logger.warning(String.format("Log file content is empty, get log
path : %s", logFilePath));
}
}
```
**Rationale**: Distinguish between different error scenarios to improve
diagnosability
---
### Issue 5: Not using `File` constructor's path separator concatenation
**Location**:
- `RestHttpGetCommandProcessor.java:325`
- `LogBaseServlet.java:47`
**Related context**:
- Path construction: `String logFilePath = logPath + "/" + logName;`
**Problem description**:
Although `File.separator` is used during path validation, path concatenation
still uses hardcoded `/`. On Windows systems, if `logPath` ends with `\`, it
will produce paths similar to `C:\logs\` + `/` + `file.log`.
Although the `File` constructor usually handles this, it's not elegant.
**Potential risks**:
- May cause path resolution issues in some edge cases
- Poor code consistency
**Impact scope**:
- Direct impact: Cross-platform compatibility
- Severity: MINOR (actual risk is low because `File` constructor has good
fault tolerance)
**Improvement suggestions**:
```java
// Solution 1: Use File constructor to automatically handle separators
String logFilePath = new File(logPath, logName).getPath();
// Solution 2: Use Paths (Java 7+)
String logFilePath = Paths.get(logPath, logName).toString();
```
**Rationale**: Improve code consistency and cross-platform robustness
---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
