test(cdc): rebuild replica diagnostic confidentiality on live develop - #283
Closed
seonghobae wants to merge 5 commits into
Closed
test(cdc): rebuild replica diagnostic confidentiality on live develop#283seonghobae wants to merge 5 commits into
seonghobae wants to merge 5 commits into
Conversation
Contributor
📝 WalkthroughWalkthrough검증 및 복제본 초기화 오류 메시지에서 입력값과 원인 예외를 제거했습니다. 포트·호스트·식별자 검증과 초기화 타임아웃 파싱에 대한 민감정보 비노출 테스트를 추가했습니다. Changes진단 정보 비노출 처리
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
marked this pull request as ready for review
August 11, 2026 21:19
seonghobae
marked this pull request as draft
August 11, 2026 21:42
seonghobae
marked this pull request as ready for review
August 11, 2026 22:19
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cdc-service/src/test/java/com/xtrmetl/cdc/util/ValidationUtilsTest.java`:
- Around line 88-104: Update ValidationUtilsTest.java at lines 88-104 so each
validation failure passes the complete rejected value to assertSafeDiagnostic,
and have the helper verify that the complete value is absent along with the
stable error-message safety checks. Update ReplicaJdbcTemplateConfigTest.java at
lines 102-105 to validate the complete timeout value and the “not-a-number?”
fragment, while confirming the resulting IllegalStateException message is safe.
- Around line 60-105: Expand assertSafeDiagnostic in ValidationUtilsTest so each
failure message and entire exception-cause chain are checked for absence of the
complete rejected input, not only sensitiveFragment and forged-log-line. Pass
the full host, port, and identifier inputs to the helper, and add a separate
full-input assertion for oversizedPortFailure while preserving key and newline
checks. Apply the same diagnostic-safety assertions to
ReplicaJdbcTemplateConfigTest.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90bd866e-5b4f-4906-bc65-b81d862fecb3
📒 Files selected for processing (4)
cdc-service/src/main/java/com/xtrmetl/cdc/config/ReplicaJdbcTemplateConfig.javacdc-service/src/main/java/com/xtrmetl/cdc/util/ValidationUtils.javacdc-service/src/test/java/com/xtrmetl/cdc/config/ReplicaJdbcTemplateConfigTest.javacdc-service/src/test/java/com/xtrmetl/cdc/util/ValidationUtilsTest.java
Comment on lines
+60
to
+105
|
|
||
| @Test | ||
| void invalidConfigurationDiagnosticsDoNotRepublishRejectedValues() { | ||
| String sensitiveFragment = "password=secret-8472"; | ||
|
|
||
| IllegalStateException hostFailure = assertThrows( | ||
| IllegalStateException.class, | ||
| () -> ValidationUtils.requireValidHost( | ||
| "replica-host?" + sensitiveFragment + "\r\nforged-log-line", | ||
| "REPLICA_PGHOST" | ||
| ) | ||
| ); | ||
| IllegalStateException portFailure = assertThrows( | ||
| IllegalStateException.class, | ||
| () -> ValidationUtils.requireValidPort("5432?" + sensitiveFragment, "REPLICA_PGPORT") | ||
| ); | ||
| IllegalStateException oversizedPortFailure = assertThrows( | ||
| IllegalStateException.class, | ||
| () -> ValidationUtils.requireValidPort("999999999999999999999999999999999999", "REPLICA_PGPORT") | ||
| ); | ||
| IllegalStateException identifierFailure = assertThrows( | ||
| IllegalStateException.class, | ||
| () -> ValidationUtils.requireValidIdentifier( | ||
| "customer_db?" + sensitiveFragment, | ||
| "REPLICA_PGDATABASE" | ||
| ) | ||
| ); | ||
|
|
||
| assertSafeDiagnostic(hostFailure, "REPLICA_PGHOST", sensitiveFragment); | ||
| assertSafeDiagnostic(portFailure, "REPLICA_PGPORT", sensitiveFragment); | ||
| assertSafeDiagnostic(oversizedPortFailure, "REPLICA_PGPORT", sensitiveFragment); | ||
| assertNull(oversizedPortFailure.getCause()); | ||
| assertSafeDiagnostic(identifierFailure, "REPLICA_PGDATABASE", sensitiveFragment); | ||
| } | ||
|
|
||
| private static void assertSafeDiagnostic( | ||
| IllegalStateException failure, | ||
| String expectedKey, | ||
| String sensitiveFragment | ||
| ) { | ||
| assertTrue(failure.getMessage().contains(expectedKey)); | ||
| assertFalse(failure.getMessage().contains(sensitiveFragment)); | ||
| assertFalse(failure.getMessage().contains("forged-log-line")); | ||
| assertFalse(failure.getMessage().contains("\r")); | ||
| assertFalse(failure.getMessage().contains("\n")); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
while IFS= read -r -d '' file; do
printf '\n== %s ==\n' "$file"
cat "$file"
done < <(fd --hidden --type f --glob 'AGENTS.md' --print0 .)
./mvnw -B testRepository: ContextualWisdomLab/mightyETL
Length of output: 219
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '== AGENTS.md files =='
find .. -name AGENTS.md -type f -print 2>/dev/null | while IFS= read -r file; do
printf '\n== %s ==\n' "$file"
cat "$file"
done
printf '\n%s\n' '== Relevant test and implementation files =='
find . -type f \( -name 'ValidationUtilsTest.java' -o -name 'ValidationUtils.java' \) -print
printf '\n%s\n' '== Test file excerpt =='
test_file="$(find . -type f -name 'ValidationUtilsTest.java' -print -quit)"
if [ -n "$test_file" ]; then
cat -n "$test_file" | sed -n '1,150p'
fi
printf '\n%s\n' '== Implementation excerpt =='
impl_file="$(find . -type f -name 'ValidationUtils.java' -print -quit)"
if [ -n "$impl_file" ]; then
cat -n "$impl_file" | sed -n '1,220p'
fi
printf '\n%s\n' '== Maven wrapper =='
if [ -x ./mvnw ]; then
./mvnw -B test
else
printf '%s\n' './mvnw is missing or is not executable'
exit 2
fiRepository: ContextualWisdomLab/mightyETL
Length of output: 50386
🏁 Script executed:
#!/usr/bin/env bash
set -u
log=/tmp/coderabbit-shell-logs/shell-output-Ttu25l
printf '%s\n' '== Maven failure summary =='
if [ -f "$log" ]; then
grep -E '(^\[ERROR\]|BUILD |Tests run:|Failures:|Errors:|There are test failures|Failed to execute)' "$log" | tail -n 120
printf '\n%s\n' '== Maven output tail =='
tail -n 120 "$log"
else
printf '%s\n' "Missing log: $log"
fi
printf '\n%s\n' '== Targeted ValidationUtilsTest =='
set -o pipefail
./mvnw -B -pl cdc-service -Dtest=ValidationUtilsTest test 2>&1 \
| tee /tmp/validation-utils-test.log \
| grep -E '(^\[INFO\] ---|Tests run:|(^\[ERROR\])|BUILD (SUCCESS|FAILURE)|Failed to execute|No tests were executed)' \
| tail -n 160
status=${PIPESTATUS[0]}
printf '\nMaven exit status: %s\n' "$status"
exit "$status"Repository: ContextualWisdomLab/mightyETL
Length of output: 15644
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '== Related diagnostic helpers and timeout tests =='
rg -n -C 4 'assertSafeDiagnostic|timeout|DoNotRepublish|republish|sensitiveFragment' \
cdc-service/src/test/java cdc-service/src/main/java 2>/dev/null || true
printf '\n%s\n' '== Java runtime =='
java -version 2>&1 || true
javac -version 2>&1 || true
printf '\n%s\n' '== Targeted cdc-service test =='
log=/tmp/validation-utils-test-direct.log
./mvnw -B -pl cdc-service -Dtest=ValidationUtilsTest test >"$log" 2>&1
status=$?
grep -E '(^\[INFO\] ---|Tests run:|(^\[ERROR\])|BUILD (SUCCESS|FAILURE)|Failed to execute|No tests were executed|SKIPPED)' "$log" | tail -n 160
printf '\nMaven exit status: %s\n' "$status"
exit "$status"Repository: ContextualWisdomLab/mightyETL
Length of output: 13117
🏁 Script executed:
#!/usr/bin/env bash
python3 - <<'PY'
def helper_passes(message, expected_key, sensitive_fragment):
return (
expected_key in message
and sensitive_fragment not in message
and "forged-log-line" not in message
and "\r" not in message
and "\n" not in message
)
cases = [
(
"host",
"Invalid host for REPLICA_PGHOST: replica-host?",
"REPLICA_PGHOST",
"password=secret-8472",
),
(
"port",
"Invalid port for REPLICA_PGPORT: 5432?",
"REPLICA_PGPORT",
"password=secret-8472",
),
(
"identifier",
"Invalid value for REPLICA_PGDATABASE: customer_db?",
"REPLICA_PGDATABASE",
"password=secret-8472",
),
]
for name, message, key, fragment in cases:
print(f"{name}: {'passes' if helper_passes(message, key, fragment) else 'fails'}")
PYRepository: ContextualWisdomLab/mightyETL
Length of output: 212
거부된 입력의 비노출 범위를 확대해 검사하세요.
assertSafeDiagnostic는 민감한 조각과 "forged-log-line"만 검사합니다. 따라서 "Invalid host for REPLICA_PGHOST: replica-host?"와 같은 부분 누출은 통과합니다. 각 거부 입력 전체와 예외 원인 체인에 입력 값이 없는지 검사하세요. oversizedPortFailure에는 sensitiveFragment가 전달되지 않으므로 별도의 입력값 검사가 필요합니다. 동일한 검사를 ReplicaJdbcTemplateConfigTest에도 적용하세요.
./mvnw -B test와 대상 테스트는 JDK 17이 Java release 25를 지원하지 않아 실행되지 않았습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cdc-service/src/test/java/com/xtrmetl/cdc/util/ValidationUtilsTest.java`
around lines 60 - 105, Expand assertSafeDiagnostic in ValidationUtilsTest so
each failure message and entire exception-cause chain are checked for absence of
the complete rejected input, not only sensitiveFragment and forged-log-line.
Pass the full host, port, and identifier inputs to the helper, and add a
separate full-input assertion for oversizedPortFailure while preserving key and
newline checks. Apply the same diagnostic-safety assertions to
ReplicaJdbcTemplateConfigTest.
Source: Coding guidelines
Comment on lines
+88
to
+104
| assertSafeDiagnostic(hostFailure, "REPLICA_PGHOST", sensitiveFragment); | ||
| assertSafeDiagnostic(portFailure, "REPLICA_PGPORT", sensitiveFragment); | ||
| assertSafeDiagnostic(oversizedPortFailure, "REPLICA_PGPORT", sensitiveFragment); | ||
| assertNull(oversizedPortFailure.getCause()); | ||
| assertSafeDiagnostic(identifierFailure, "REPLICA_PGDATABASE", sensitiveFragment); | ||
| } | ||
|
|
||
| private static void assertSafeDiagnostic( | ||
| IllegalStateException failure, | ||
| String expectedKey, | ||
| String sensitiveFragment | ||
| ) { | ||
| assertTrue(failure.getMessage().contains(expectedKey)); | ||
| assertFalse(failure.getMessage().contains(sensitiveFragment)); | ||
| assertFalse(failure.getMessage().contains("forged-log-line")); | ||
| assertFalse(failure.getMessage().contains("\r")); | ||
| assertFalse(failure.getMessage().contains("\n")); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
두 진단 테스트가 거부된 입력의 부분 재출력을 허용합니다.
두 헬퍼는 선택한 민감한 조각만 검사합니다. 거부된 입력의 다른 부분이 진단에 남아도 회귀가 통과할 수 있습니다. 전체 입력과 안전한 오류 메시지를 함께 검증해 주세요.
cdc-service/src/test/java/com/xtrmetl/cdc/util/ValidationUtilsTest.java#L88-L104: 각 검증 실패에 전체 거부 값을 전달하고, 전체 값과 안정적인 오류 메시지를 검사하세요.cdc-service/src/test/java/com/xtrmetl/cdc/config/ReplicaJdbcTemplateConfigTest.java#L102-L105: 전체 타임아웃 값과not-a-number?부분을 검사하고, 안전한IllegalStateException메시지를 확인하세요.
📍 Affects 2 files
cdc-service/src/test/java/com/xtrmetl/cdc/util/ValidationUtilsTest.java#L88-L104(this comment)cdc-service/src/test/java/com/xtrmetl/cdc/config/ReplicaJdbcTemplateConfigTest.java#L102-L105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cdc-service/src/test/java/com/xtrmetl/cdc/util/ValidationUtilsTest.java`
around lines 88 - 104, Update ValidationUtilsTest.java at lines 88-104 so each
validation failure passes the complete rejected value to assertSafeDiagnostic,
and have the helper verify that the complete value is absent along with the
stable error-message safety checks. Update ReplicaJdbcTemplateConfigTest.java at
lines 102-105 to validate the complete timeout value and the “not-a-number?”
fragment, while confirming the resulting IllegalStateException message is safe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes stale/diverged PR #211 for issue #210 without transferring old-head checks or reviews.
Exact live base at creation:
develop@106add38465937f6eb4e4accc185fd30aab4446f.Superseded by #301
Closed as superseded after unique-work preservation was revalidated against #301 current head
9eef90b09bb14a72ac63478dacaacae8222f2b51.ValidationUtilsin test(cdc): replay replica diagnostic confidentiality on live develop #301 retains the same fail-closed diagnostic behavior for host, port, and identifiers while adding public Javadocs.ReplicaJdbcTemplateConfigin test(cdc): replay replica diagnostic confidentiality on live develop #301 retains the same Hikari timeout diagnostic confidentiality behavior and drops the parser cause while adding public Javadocs.ValidationUtilsTest.invalidConfigurationDiagnosticsDoNotRepublishRejectedValuesis preserved in test(cdc): replay replica diagnostic confidentiality on live develop #301, including credential-like fragments, CRLF/log-forging rejection, oversized-port coverage, stable configuration-key diagnostics, and no parser cause for overflow.ReplicaJdbcTemplateConfigTestnow preserves and strengthens this PR's timeout coverage: rejected values are not republished, CR and LF are independently excluded, authorization-like content is excluded, and the directIllegalStateExceptioncause is asserted null. This also addresses the current CodeRabbit finding on test(cdc): replay replica diagnostic confidentiality on live develop #301.No checks, reviews, or approval evidence from this PR transfer to #301. #301 remains responsible for fresh exact-head CI/security/non-vacuous coverage and qualifying independent approval before merge.
Summary by CodeRabbit
버그 수정
테스트