fix(security): add privacy-safe administrative audit boundary - #363
fix(security): add privacy-safe administrative audit boundary#363seonghobae wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough관리자 감사 로거와 관리자 식별자용 도메인 분리 pseudonymization 팩토리를 추가했다. 인증 컨텍스트와 요청 헤더를 지원하며, 원시 식별자 대신 fingerprint와 감사 결과를 구조화된 로그로 기록한다. 관련 동작을 Log4j 캡처 테스트로 검증한다. Changes관리자 감사 추적
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Request as 요청
participant Logger as AdministrativeAuditLogger
participant Pseudonymizer as AuditPseudonymizer
participant Log4j as Log4j
Request->>Logger: 컨텍스트 또는 헤더와 감사 이벤트 전달
Logger->>Pseudonymizer: 식별자 pseudonymization 요청
Pseudonymizer-->>Logger: 도메인별 fingerprint 반환
Logger->>Log4j: 구조화 감사 로그 기록
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java (2)
175-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win로거 레벨을 원래 값으로 복원하십시오.
attachAppender는logger.setLevel(Level.INFO)로 전역 로거 상태를 변경합니다.closeAndDetach는 appender만 제거하고 레벨은 복원하지 않습니다. 변경된 레벨은 같은 JVM의 이후 테스트에 남습니다. 실행 순서에 따라 다른 테스트가 영향을 받을 수 있습니다.이전 레벨을 저장하고 detach 시 복원하십시오.
♻️ 레벨 복원 적용
private static CapturingAppender attachAppender() { Logger logger = (Logger) LogManager.getLogger(AdministrativeAuditLogger.class); - CapturingAppender appender = new CapturingAppender(logger); + CapturingAppender appender = new CapturingAppender(logger, logger.getLevel()); appender.start(); logger.addAppender(appender); logger.setLevel(Level.INFO); return appender; }private final Logger logger; + private final Level previousLevel; private final List<String> messages = new ArrayList<>(); - private CapturingAppender(Logger logger) { + private CapturingAppender(Logger logger, Level previousLevel) { super( "administrative-audit-test-appender", null, PatternLayout.newBuilder().withPattern("%m").build(), false, null ); this.logger = logger; + this.previousLevel = previousLevel; }private void closeAndDetach() { logger.removeAppender(this); + logger.setLevel(previousLevel); stop(); }🤖 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 `@src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java` around lines 175 - 182, Update attachAppender and closeAndDetach to capture the logger’s existing level before setting Level.INFO, then restore that saved level when detaching the appender. Ensure cleanup restores the original logger state for subsequent tests.
105-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win동일한 식별자 값으로 도메인 분리를 검증하십시오.
현재 assertion은 tenant, actor, job fingerprint가 서로 다름을 확인합니다. 그러나 세 입력값이 이미 서로 다릅니다. 따라서 이 assertion은 입력값 차이 때문에도 통과합니다. 도메인 분리 자체를 증명하지 못합니다.
세 도메인에 같은 식별자 문자열을 넣고 fingerprint가 다른지 확인하는 assertion을 추가하십시오.
♻️ 도메인 분리 전용 검증 추가 예시
+ `@Test` + void separatesDomainsForIdenticalIdentifierValues() { + String sharedId = "11111111-1111-1111-1111-111111111111"; + String actor = AuditPseudonymizer.forAdministrativeActor(AUDIT_SECRET, "v2") + .fingerprint(sharedId); + String tenant = AuditPseudonymizer.forAdministrativeTenant(AUDIT_SECRET, "v2") + .fingerprint(sharedId); + String job = AuditPseudonymizer.forAdministrativeJob(AUDIT_SECRET, "v2") + .fingerprint(sharedId); + + assertNotEquals(actor, tenant); + assertNotEquals(actor, job); + assertNotEquals(tenant, job); + }🤖 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 `@src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java` around lines 105 - 110, Update the test around AdministrativeAuditLoggerTest to use the same identifier string for tenant, actor, and job inputs, then assert their resulting tenantFingerprint, actorFingerprint, and jobFingerprint values remain distinct. Preserve the existing fingerprint extraction while ensuring the assertions validate domain separation rather than differing input values.
🤖 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
`@src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java`:
- Around line 161-166: Update the fieldValue helper in
AdministrativeAuditLoggerTest to explicitly assert that fieldName is present
before calculating the substring bounds. When message.indexOf(prefix) returns
-1, fail with a clear assertion message identifying the missing field instead of
continuing to substring.
---
Nitpick comments:
In
`@src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java`:
- Around line 175-182: Update attachAppender and closeAndDetach to capture the
logger’s existing level before setting Level.INFO, then restore that saved level
when detaching the appender. Ensure cleanup restores the original logger state
for subsequent tests.
- Around line 105-110: Update the test around AdministrativeAuditLoggerTest to
use the same identifier string for tenant, actor, and job inputs, then assert
their resulting tenantFingerprint, actorFingerprint, and jobFingerprint values
remain distinct. Preserve the existing fingerprint extraction while ensuring the
assertions validate domain separation rather than differing input values.
🪄 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: 61f85ca8-96c1-4e80-9b36-554227234c0b
📒 Files selected for processing (3)
src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.javasrc/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.javasrc/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java
1 participant
Objective
Reconcile the privacy-safe administrative audit primitive from stale descendant #268 directly on current protected
main, without carrying its divergent ancestry. This slice establishes domain-separated keyed audit fingerprints for actor, tenant, and job identifiers before later controller wiring.Test-first state
This Draft starts intentionally RED at exact test-only head
025b8004ff59bc13710e25fc59f328010d34c7ef. The focused test specifies that authenticated and untrusted identifiers never appear raw in administrative audit output; actor/tenant/job correlation must use separate HMAC domains; absent and unconfigured-key states must use controlled markers.Scope
Only the reusable administrative audit logger primitive, the required
AuditPseudonymizeradministrative domains/factories, and focused tests. It does not modifyAdminController, active PR #341, tenant query PR #342/#361, deletion lifecycle PRs, credential/OIDC work, or canonical documentation.Acceptance
Observe exact-head RED -> smallest current-base production implementation -> exact-head
mvn -B --no-transfer-progress verifywith zero missed owned production coverage/public Javadocs -> CI/Security Scan/SAST/fuzz -> review/thread/live-base refetch. Keep Draft until GREEN exact-head evidence exists. Independent write-authorized approval remains a separate merge gate.Summary by CodeRabbit
새 기능
버그 수정