Skip to content

fix(security): reconcile administrative audit on current main - #434

Open
seonghobae wants to merge 7 commits into
mainfrom
fix/admin-audit-pseudonym-current-main-20260813
Open

fix(security): reconcile administrative audit on current main#434
seonghobae wants to merge 7 commits into
mainfrom
fix/admin-audit-pseudonym-current-main-20260813

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Objective

Reconcile stale PR #363's privacy-safe administrative-audit primitive directly on the protected-main line, without importing divergent ancestry or racing the separate AdminController authorization slice #440.

Exact current-head evidence

  • protected base at reconstruction: 961f44a19c4ced87f8062b3ab9842a9a08d05235;
  • exact head: f0d275749811ceaf9f0d9fe0a300b15c86a853ad;
  • changed paths: AdministrativeAuditLogger.java, administrative-domain factories in AuditPseudonymizer.java, and the focused logger regression only;
  • CI, Security Scan, SAST Semgrep, and fuzz: terminal-success;
  • mergeable: true;
  • unresolved inline findings: zero;
  • stale predecessor fix(security): add privacy-safe administrative audit boundary #363 is closed.

Privacy contract

Administrative actor, tenant, and conversion-job identifiers are transformed through separate keyed HMAC domains before structured logging. Raw claim headers, tenant/subject/job identifiers, tokens, filenames, job messages, and document content are never rendered. Missing identifiers and unavailable pseudonym authority use controlled markers; key-version metadata remains non-secret and bounded by the existing pseudonymizer contract.

Scope boundary

This PR establishes the reusable audit primitive only. It does not modify or authorize AdminController routes, change the documented tenant:configure RBAC contract, persist an audit ledger, replace the broader privacy/telemetry architecture, or close the administrative-authorization gap. #440 owns controller authorization and tenant-safe operation semantics; subsequent wiring must consume this primitive in a separately reviewable slice after both foundations integrate.

Merge gate

Keep this exact head unchanged. Auto-merge may act only after all live required checks remain successful, zero valid unresolved findings remain, and a qualifying independent non-author approval is attached to this exact head. If live protection requires an updated base, rebuild or update against protected main and regenerate all base-sensitive checks and reviews. No predecessor evidence transfers.

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 112 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 53a37aae-403d-43f6-99f0-b7417f0ee225

📥 Commits

Reviewing files that changed from the base of the PR and between 961f44a and f0d2757.

📒 Files selected for processing (3)
  • src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java
  • src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java
  • src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as ready for review August 14, 2026 07:58
@seonghobae
seonghobae enabled auto-merge (squash) August 14, 2026 07:58

Copy link
Copy Markdown
Collaborator Author

Exact-current-head review request for f0d275749811ceaf9f0d9fe0a300b15c86a853ad on protected main 961f44a19c4ced87f8062b3ab9842a9a08d05235. CI, Security Scan, SAST Semgrep, and fuzz are terminal-success; the PR is mergeable and has no unresolved review threads. This branch is the current-main replacement for stale #363. Review read-only; do not mutate the branch or bypass independent approval.

@opencode-agent review
@cwl-noema-review review
@strix review
@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== checked-out commit and base relation =='
git rev-parse HEAD
git merge-base HEAD 961f44a19c4ced87f8062b3ab9842a9a08d05235
git diff --stat 961f44a19c4ced87f8062b3ab9842a9a08d05235...HEAD
git diff --name-status 961f44a19c4ced87f8062b3ab9842a9a08d05235...HEAD

printf '%s\n' '== pull request metadata and checks =='
gh pr view 434 --repo ContextualWisdomLab/clearfolio \
  --json number,title,state,isDraft,headRefOid,baseRefOid,mergeStateStatus,reviewDecision,statusCheckRollup

printf '%s\n' '== exact diff =='
git diff --find-renames --find-copies --no-ext-diff \
  961f44a19c4ced87f8062b3ab9842a9a08d05235...HEAD

Length of output: 33361


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== changed class outlines =='
ast-grep outline src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java --items all
ast-grep outline src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java --items all

printf '%s\n' '== administrative audit logger usages =='
rg -n -C 4 \
  '(new\s+AdministrativeAuditLogger\b|AdministrativeAuditLogger\b|\.(recordHeaders|record|actorFingerprint)\s*\()' \
  src/main/java src/test/java || true

printf '%s\n' '== administrative conversion-job operation candidates =='
rg -n -i -C 5 \
  '(list.*job|job.*list|delete.*job|job.*delete|retry.*job|job.*retry|dead.?letter)' \
  src/main/java/com/clearfolio/viewer

printf '%s\n' '== configuration and pseudonymizer implementation =='
config_file=$(fd -a '^ConversionProperties\.java$' src/main/java | head -n 1)
printf 'ConversionProperties: %s\n' "$config_file"
if [ -n "$config_file" ]; then
  ast-grep outline "$config_file" --items all
  rg -n -C 3 '(auditPseudonym|AuditPseudonym)' "$config_file"
fi
sed -n '1,260p' src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java

Length of output: 143040


@seonghobae I found one blocking issue.

  • AdministrativeAuditLogger has no production call site. The only references are in its own class and in AdministrativeAuditLoggerTest.
  • AdminController still handles GET /api/v1/admin/convert/jobs, DELETE /api/v1/admin/convert/jobs/{jobId}, and POST /api/v1/admin/convert/jobs/{jobId}/retry without injecting or calling the logger.
  • As a result, the new component emits no administrative audit records. The PR adds the audit mechanism but does not implement the audit control.

Add the logger to each administrative operation. Record allowed, denied, not-found, not-eligible, and failure outcomes. Add endpoint-level tests that assert the records.

The exact head and base match the request. The check runs shown are successful. GitHub currently reports mergeStateStatus: BLOCKED and reviewDecision: REVIEW_REQUIRED.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent review
@coderabbitai review
@cwl-noema-review review
@strix review

Review exact head f0d275749811ceaf9f0d9fe0a300b15c86a853ad only. Exact-head CI, Security Scan, SAST Semgrep, and fuzz are terminal-success; unresolved inline findings are zero. Validate separate actor/tenant/job HMAC domains, controlled absent/unavailable markers, bounded key-version metadata, and complete exclusion of raw identifiers, headers, tokens, filenames, messages, or document content.

Review read-only; preserve independent reviewer identity. Do not wire this primitive into #440, mutate the branch, synthesize author approval, weaken protection, or merge.

@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base='961f44a19c4ced87f8062b3ab9842a9a08d05235'
head='f0d275749811ceaf9f0d9fe0a300b15c86a853ad'

printf '%s\n' '== exact revisions =='
printf 'HEAD: '; git rev-parse HEAD
printf 'expected head: %s\n' "$head"
printf 'merge base: '; git merge-base HEAD "$base"
printf '%s\n' '== changed paths =='
git diff --name-status "$base...$head"

printf '%s\n' '== changed Java outlines =='
for f in \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java \
  src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java
do
  if [ -f "$f" ]; then
    printf '\n-- %s --\n' "$f"
    ast-grep outline "$f" --items all || true
  fi
done

printf '%s\n' '== exact patch =='
git diff --no-ext-diff --unified=80 "$base...$head" -- \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java \
  src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java

printf '%s\n' '== sensitive logging candidates in changed production files =='
rg -n -i -C 3 \
  '(logger|log)\.(trace|debug|info|warn|error)\s*\(|StructuredArguments|keyValue|addKeyValue|MDC|headers?|token|filename|message|document|subject|tenant|job)' \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java || true

printf '%s\n' '== live PR metadata and checks =='
gh pr view 434 --repo ContextualWisdomLab/clearfolio \
  --json headRefOid,baseRefOid,mergeStateStatus,reviewDecision,statusCheckRollup,reviews

Length of output: 42030


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== AuditPseudonymizer remainder =='
sed -n '150,260p' src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java

printf '%s\n' '== ConversionProperties audit configuration =='
config=$(fd -a '^ConversionProperties\.java$' src/main/java | head -n 1)
printf 'file: %s\n' "$config"
if [ -n "$config" ]; then
  ast-grep outline "$config" --items all
  rg -n -C 5 'AuditPseudonym|auditPseudonym' "$config"
fi

printf '%s\n' '== AdministrativeAuditLogger production and test references =='
rg -n -C 4 '\bAdministrativeAuditLogger\b|\.recordHeaders\s*\(|\.actorFingerprint\s*\(' \
  src/main/java src/test/java || true

printf '%s\n' '== all logging calls and sensitive-value symbols in changed production code =='
rg -n -i -C 3 \
  '(LOGGER\.(trace|debug|info|warn|error)\s*\(|LoggerFactory|getFirst\s*\(|tenantId|subjectId|jobId|headers|token|filename|message|document)' \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java

printf '%s\n' '== tests covering pseudonymizer contract =='
test=$(fd -a '^AuditPseudonymizerTest\.java$' src/test/java | head -n 1)
printf 'file: %s\n' "$test"
if [ -n "$test" ]; then
  ast-grep outline "$test" --items all
  rg -n -C 3 '(keyVersion|unavailable|absent|Administrative|fingerprint|domain)' "$test"
fi

Length of output: 36545


@seonghobae Review complete for exact head f0d275749811ceaf9f0d9fe0a300b15c86a853ad.

I found no valid unresolved finding in this PR’s primitive-only scope.

  • Actor, tenant, and conversion-job identifiers use separate fixed HMAC domains.
  • The logger renders only action, outcome, status, result count, and pseudonymized identifiers.
  • The logger does not render raw headers, tenant IDs, subject IDs, job IDs, tokens, filenames, job messages, or document content.
  • Null identifiers produce absent:<key-version>.
  • An unavailable pseudonym secret produces unavailable:<key-version> for present identifiers.
  • Key-version metadata is constrained to 32 characters and a safe character set.
  • Focused tests cover identifier exclusion, domain separation, and controlled markers.
  • CI, Security Scan, SAST Semgrep, and fuzz are successful for this exact head.

The earlier request to wire AdministrativeAuditLogger into AdminController does not apply. This PR explicitly establishes the reusable primitive only. Pull request #440 owns endpoint integration.

GitHub still reports REVIEW_REQUIRED and BLOCKED. An independent non-author approval remains required before merge.

You are interacting with an AI system.

Copy link
Copy Markdown
Collaborator Author

Exact-current-head independent review request for f0d275749811ceaf9f0d9fe0a300b15c86a853ad.

The three-file administrative-audit foundation is mergeable; CI, Security Scan, SAST Semgrep, and fuzz are terminal-success; unresolved inline findings are zero. Actor, tenant, and job authority use separate keyed HMAC domains; raw headers, identifiers, tokens, filenames, messages, and document content never enter the fixed structured log template. Protected main has advanced since construction, so any base update must regenerate all exact-head evidence.

@opencode-agent review
@coderabbitai review
@cwl-noema-review review
@strix review

Review this exact head read-only. Do not mutate the branch, synthesize author approval, weaken protection, or merge.

@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown

Rate Limit Exceeded

@seonghobae have exceeded the limit for the number of chat messages per hour. Please wait 23 minutes and 58 seconds before sending another message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant