DanielCarter-stack commented on PR #10595:
URL: https://github.com/apache/seatunnel/pull/10595#issuecomment-4046288623
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10595", "part": 1,
"total": 1} -->
### Issue 2: Insufficient Test Coverage
**Location**: `RestApiSubmitJobConfigShadeDecryptTest.java`
**Modified Code**:
Added 364 lines of integration tests
**Related Context**:
- Parent class: None
- Child class/Implementation class: None
- Caller: JUnit 5 test framework
**Problem Description**:
Although integration tests were added, the following test coverage gaps
exist:
1. **Insufficient Boundary Condition Testing**:
- No testing for missing or empty `shade.identifier`
- No testing for decryption failures (e.g., invalid base64 strings)
- No testing for configurations missing required fields (e.g., empty
source or sink)
2. **Insufficient File Upload Testing**:
- Only tested HOCON file upload (`.conf`)
- No testing for SQL file upload (`.sql`)
- No testing for JSON file upload (`.json`)
3. **Missing Error Scenario Testing**:
- No testing for configuration format errors (e.g., HOCON syntax errors)
- No testing for HTTP response codes and error messages when decryption
throws exceptions
4. **Missing Concurrency and Performance Testing**:
- No testing for concurrent submission scenarios
- No testing for performance with large configuration files
**Potential Risks**:
- **Medium Risk**: Boundary conditions and error scenarios could lead to
runtime exceptions, returning 500 errors instead of user-friendly 400 errors
- **Low Risk**: Untested scenarios may have issues in actual use
**Impact Scope**:
- **Direct Impact**: REST API error handling and user experience
- **Indirect Impact**: Production environment stability
- **Affected Area**: REST API module
**Severity**: MINOR
**Improvement Suggestions**:
It is recommended to add the following test cases (can be done in a
follow-up PR):
```java
@Test
public void testSubmitJobWithHoconFormatInvalidBase64() throws Exception {
String invalidBase64Body = buildHoconBody()
.replace("c2VhdHVubmVs", "invalid_base64!");
String requestUrl = "http://localhost:"; + restPort +
"/submit-job?format=hocon";
HttpResponse response = post(requestUrl, "text/plain",
invalidBase64Body);
// Should return 400 Bad Request or 500 Internal Server Error
Assertions.assertTrue(response.code == 400 || response.code == 500);
Assertions.assertTrue(response.body.contains("error") ||
response.body.contains("Illegal"));
}
@Test
public void testSubmitJobByUploadSqlFileDecryptsConfig() throws Exception {
String requestUrl = "http://localhost:"; + restPort +
"/submit-job/upload";
HttpResponse response = postMultipart(requestUrl, "job.sql",
buildSqlBody());
Assertions.assertEquals(200, response.code);
Assertions.assertTrue(response.body.contains("jobId"));
}
@Test
public void testSubmitJobWithHoconFormatMissingShadeIdentifier() throws
Exception {
String bodyWithoutShade = "env {\n"
+ " job.mode = \"BATCH\"\n"
+ "}\n"
+ "source {\n"
+ " FakeSource {\n"
+ " row.num = 5\n"
+ " username = \"plain_text_username\"\n"
+ " password = \"plain_text_password\"\n"
+ " }\n"
+ "}\n"
+ "sink { Console {} }";
String requestUrl = "http://localhost:"; + restPort +
"/submit-job?format=hocon";
HttpResponse response = post(requestUrl, "text/plain", bodyWithoutShade);
// Should handle normally (without decryption)
Assertions.assertEquals(200, response.code);
}
```
**Rationale**:
- Although this is not a blocking issue, more comprehensive testing can
improve code quality and confidence
- Especially testing of error scenarios can ensure users receive friendly
error messages
- This is a best practice and recommended to be supplemented in a follow-up
PR
---
### Issue 3: Potential Log Security Issue (Non-blocking)
**Location**: `ConfigShadeUtils.java:173`
**Related Context**:
- In `ConfigShadeUtils.processConfig()` method
- Line 173: `String jsonString =
config.root().render(ConfigRenderOptions.concise());`
**Problem Description**:
Inside the `processConfig()` method, the configuration is rendered as a JSON
string (including **decrypted passwords**). If an exception occurs at this
stage (e.g., `Preconditions.checkArgument` on lines 183-186 throws an
exception), the unmasked configuration may be logged in logs or exception
stacks.
Checking `ConfigBuilder` line 118, it uses `configDesensitization()` for
masking, but this is called only after `decryptConfig()` completes successfully.
**Potential Risks**:
- **Low Risk**: Exposed only when an exception is thrown
- Production environment log levels are typically set to INFO or WARN,
making it unlikely to log detailed exception stacks
- This is not an issue introduced by this PR, but a potential risk in the
original code
**Impact Scope**:
- **Direct Impact**: Logging system
- **Indirect Impact**: Security (log leakage of sensitive information)
- **Affected Area**: All places using `ConfigShadeUtils.decryptConfig()`
**Severity**: MINOR
**Improvement Suggestions**:
It is recommended to add exception handling in
`ConfigShadeUtils.processConfig()` to ensure sensitive information is not
logged when exceptions occur:
```java
private static Config processConfig(
String identifier, Config config, boolean isDecrypted, Map<String,
Object> props) {
try {
ConfigShade configShade = CONFIG_SHADES.getOrDefault(identifier,
DEFAULT_SHADE);
configShade.open(props);
Set<String> sensitiveOptions = new
HashSet<>(getSensitiveOptions(config));
sensitiveOptions.addAll(Arrays.asList(configShade.sensitiveOptions()));
// ... existing processing logic ...
return ConfigFactory.parseMap(configMap);
} catch (Exception e) {
// Log desensitized error information
log.error("Failed to {} config with identifier: {}",
isDecrypted ? "decrypt" : "encrypt",
identifier,
e);
// Rethrow exception without sensitive information
throw new IllegalArgumentException(
String.format("Failed to %s config. Please check your
configuration.",
isDecrypted ? "decrypt" : "encrypt"),
e);
}
}
```
**Rationale**:
- This is a security enhancement suggestion, not a blocking issue
- Can be handled separately as a security improvement in a follow-up PR
- Not related to the fix target of this PR (this PR ensures decryption is
called, rather than improving the decryption logic itself)
---
### Issue 4: Missing Explicit Decryption Call for JSON Path (Inconsistency)
**Location**: `JobInfoService.java:190-193`
**Modified Code**:
```java
case JSON:
default:
config = RestUtil.buildConfig(requestHandle(requestBody), false);
break;
```
**Related Context**:
- Line 192 calls `RestUtil.buildConfig(JsonNode, false)`
- `RestUtil.buildConfig()` internally calls `ConfigBuilder.of(Map, boolean)`
- `ConfigBuilder.of()` line 115 decides whether to decrypt based on the
second parameter
**Problem Description**:
The JSON path does not explicitly call `ConfigShadeUtils.decryptConfig()`,
but instead relies on the decryption logic inside `RestUtil.buildConfig()` →
`ConfigBuilder.of()`.
This inconsistency may lead to:
1. Code review requiring tracing multiple layers of calls to confirm that
the JSON path is also decrypted
2. If the implementation of `ConfigBuilder.of()` changes (e.g., the meaning
of the `isEncrypt` parameter changes), it may lead to bugs
**Potential Risks**:
- **Low Risk**: Current implementation is correct, but not obvious enough
- If someone modifies the implementation of `ConfigBuilder.of()` in the
future, it may introduce bugs
**Impact Scope**:
- **Direct Impact**: Code maintainability
- **Indirect Impact**: Possible future bugs
- **Affected Area**: REST API submissions in JSON format
**Severity**: MINOR
**Improvement Suggestions**:
It is recommended to unify the decryption call method for all paths,
explicitly calling `ConfigShadeUtils.decryptConfig()`:
```java
// fix issue: https://github.com/apache/seatunnel/issues/10590
switch (configFormat) {
case HOCON:
config = ConfigFactory.parseString(new String(requestBody,
StandardCharsets.UTF_8));
break;
case SQL:
config = SqlConfigBuilder.of(new String(requestBody,
StandardCharsets.UTF_8));
break;
case JSON:
default:
config = RestUtil.buildConfig(requestHandle(requestBody), false);
break;
}
// Unified decryption after switch
config = ConfigShadeUtils.decryptConfig(config);
```
**But note**: `RestUtil.buildConfig(requestHandle(requestBody), false)`
already calls `ConfigBuilder.of(Map, false)` → `decryptConfig()`, so this
modification would result in **duplicate decryption**.
A better approach is to modify `RestUtil.buildConfig()` and
`ConfigBuilder.of()`:
1. **Option A** (Recommended): Modify the semantics of
`RestUtil.buildConfig(JsonNode, boolean isEncrypt)` to no longer decrypt, and
decrypt uniformly in `JobInfoService`:
```java
// RestUtil.java
public static Config buildConfig(JsonNode jsonNode) {
// Remove isEncrypt parameter, no longer decrypt
Map<String, Object> objectMap = JsonUtils.toMap(jsonNode);
return ConfigFactory.parseMap(objectMap)
.resolve(ConfigResolveOptions.defaults().setAllowUnresolved(true))
.resolveWith(ConfigFactory.systemProperties(),
ConfigResolveOptions.defaults().setAllowUnresolved(true));
}
// JobInfoService.java
switch (configFormat) {
case JSON:
default:
config = RestUtil.buildConfig(requestHandle(requestBody));
break;
}
// Unified decryption
config = ConfigShadeUtils.decryptConfig(config);
```
2. **Option B**: Keep the current implementation, but add comments
explaining that the JSON path is decrypted internally through
`RestUtil.buildConfig()`.
**Rationale**:
- This is a code consistency and maintainability issue
- Although the current implementation is functionally correct, it is not
obvious enough
- Recommended as a follow-up improvement for code refactoring, not a
blocking issue for this PR
---
--
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]
