Skip to content

fix(security): pseudonymize policy override audit identifiers - #267

Closed
seonghobae wants to merge 26 commits into
mainfrom
fix/pii-logging-clean
Closed

fix(security): pseudonymize policy override audit identifiers#267
seonghobae wants to merge 26 commits into
mainfrom
fix/pii-logging-clean

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Security objective

Remove raw policy-override approver identifiers from application logs without replacing them with dictionary-attackable unkeyed hashes, and reject weak or reused HMAC material before conversion endpoints accept traffic.

This clean pull request supersedes #165. The prior agent-managed branch repeatedly received unrelated Netty/SBOM workflow commits after its reviewed exact head, including write-scoped self-mutating workflows. This branch is pinned directly to the last reviewed privacy tree and contains none of those unrelated commits.

Changes

  • Add a dedicated, domain-separated HMAC-SHA-256 AuditPseudonymizer for low-entropy audit identifiers.
  • Require configured audit pseudonym and policy-override keys to contain at least 32 UTF-8 bytes.
  • Keep policy approval signing and audit pseudonymization keys separate and fail startup on key reuse.
  • Record approverFingerprint=<key-version>:<128-bit hex> instead of approverId.
  • Emit fixed absent and non-correlatable unavailable markers; never fall back to plaintext or unkeyed identifier hashing.
  • Load key material through Spring Boot config-tree secret mounts.
  • Default a missing key version to v1, while rejecting every invalid explicit value without trimming it into a different valid identifier.
  • Add deterministic key-strength, key/domain/version-separation, Unicode/control-character, missing-key, missing-provider, startup-separation, padded-version rejection, captured-log, and disabled-signing regression tests.
  • Prove captured audit logs contain neither the raw approver identifier nor approval token.
  • Align authoritative diagrams, product documentation, APA 7th references, key rotation/retention/incident-response guidance, and CHANGELOG.md.

Review history carried forward

CodeRabbit's valid findings on the reviewed tree were addressed: duplicate [Unreleased] sections, invalid trailing-newline handling, the uncovered disabled policy-key startup path, key-version validation, key separation, config-tree sourcing, provider restoration, documentation precision, and captured-log requirements. The prior Strix weak-policy-key finding was also addressed. The final tree removes unreachable branches rather than weakening coverage and exercises carriage-return sanitization.

Exact-head evidence

Exact head 6e311edd1c21f30eacbfcdb8d92a903e587c76d0 is based directly on protected main at f3cc09a9838f0f88c81a2ceae22138fab80a2edb.

Fresh checks attached to this clean PR completed successfully:

  • CI 31005771331: success.
  • Security Scan 31005771301: success.
  • SAST Semgrep 31005771293: success.
  • fuzz 31005771307: success.
  • CodeRabbit commit status: success.
  • Inline review threads: zero.

No cancelled run from the earlier duplicate trigger is counted as evidence.

Security ordering

Exact-head Strix review of this source tree also surfaced a repository-wide Netty 4.1.135.Final dependency finding that is unrelated to the audit-pseudonymization code. That remediation is intentionally isolated in draft PR #269. Do not fold #269's POM, generated SBOM, attribution, or workflow history into this PR.

#269 must complete its generated buyer evidence, pass every exact-head gate, receive counted independent approval, and integrate first. This branch must then be reconciled onto the resulting protected main and receive fresh checks/reviews. Previous green results remain provenance only after that reconciliation.

Merge gate

Keep this PR draft. After #269 integrates, reconcile the same bounded privacy tree onto protected main, then require fresh exact-head CI, Security Scan, SAST, every fuzz job, Strix/OpenCode/Noema evidence, CodeRabbit review, zero unresolved threads, and an independent approving reviewer with repository write permission. Do not bypass branch protection, weaken tests, or treat advisory bot output as counted approval.

@coderabbitai

coderabbitai Bot commented Aug 5, 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: 22 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

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: dc74e344-34b1-4d48-8d75-b64f52a96979

📥 Commits

Reviewing files that changed from the base of the PR and between f3cc09a and 6e311ed.

📒 Files selected for processing (30)
  • .github/workflows/ci.yml
  • .github/workflows/fuzz.yml
  • CHANGELOG.md
  • docs/diagrams/submit-flow.md
  • docs/diagrams/submit-policy-adapter-flow.md
  • docs/prd/clearfolio-viewer-unified-document-preview-prd.md
  • docs/security/2026-08-04-audit-pseudonymization.md
  • pom.xml
  • src/main/java/com/clearfolio/viewer/config/ConversionProperties.java
  • src/main/java/com/clearfolio/viewer/controller/ApiExceptionHandler.java
  • src/main/java/com/clearfolio/viewer/controller/ConversionController.java
  • src/main/java/com/clearfolio/viewer/security/AuditKeySeparationGuard.java
  • src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java
  • src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java
  • src/main/java/com/clearfolio/viewer/service/PolicyOverrideRequest.java
  • src/main/resources/application.yml
  • src/test/java/com/clearfolio/viewer/artifact/FileSystemArtifactStoreCoverageTest.java
  • src/test/java/com/clearfolio/viewer/config/ConversionPropertiesCoverageTest.java
  • src/test/java/com/clearfolio/viewer/controller/ApiExceptionHandlerCoverageTest.java
  • src/test/java/com/clearfolio/viewer/controller/ConversionControllerCoverageTest.java
  • src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryCoverageTest.java
  • src/test/java/com/clearfolio/viewer/security/AuditKeySeparationGuardTest.java
  • src/test/java/com/clearfolio/viewer/security/AuditPseudonymizerKeyStrengthTest.java
  • src/test/java/com/clearfolio/viewer/security/AuditPseudonymizerTest.java
  • src/test/java/com/clearfolio/viewer/service/DefaultDocumentConversionServiceCoverageTest.java
  • src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationCoverageTest.java
  • src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceAuditTest.java
  • src/test/java/com/clearfolio/viewer/service/DocumentConversionServiceCoverageTest.java
  • src/test/java/com/clearfolio/viewer/service/PolicyOverrideRequestTest.java
  • src/test/java/com/clearfolio/viewer/testsupport/SecurityProviderTestSupport.java

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

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@opencode-agent @cwl-noema-review Independently review exact current head 6e311edd1c21f30eacbfcdb8d92a903e587c76d0 on clean replacement PR #267. Verify the domain-separated keyed audit pseudonymization, UTF-8 byte-strength checks, policy/audit key separation, strict key-version validation, config-tree sourcing, raw-identifier/token non-disclosure, provider restoration, disabled-signing path, consolidated changelog contract, zero missed production lines/branches, and warning-free public Javadocs. Treat this PR's fresh exact-head CI/Security/SAST/fuzz/Strix results and a counted independent approval as required; previous-PR evidence is supporting provenance only.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Rate Limit Exceeded

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

@seonghobae
seonghobae enabled auto-merge (squash) August 5, 2026 12:47
@seonghobae
seonghobae marked this pull request as draft August 5, 2026 12:49
auto-merge was automatically disabled August 5, 2026 12:49

Pull request was converted to draft

Copy link
Copy Markdown
Collaborator Author

Superseded by draft #270. #270 preserves this exact reviewed audit-pseudonymization tree and adds the repository-wide Netty 4.1.136 remediation plus deterministic CycloneDX/attribution evidence required by the current security gate. Keeping both PRs open would duplicate ownership and force the same privacy tree through two independent merge paths. No #267 check or review is reused as final evidence for #270; #270 must pass fresh exact-head checks, current reviews, counted independent approval, and branch protection.

Copy link
Copy Markdown
Collaborator Author

Closing as superseded by integrated replacement #270. #270 exact head 06452c3f39f2deb38d31d189e46de4b25512baa0 is a strict descendant of this clean privacy head and adds only the bounded Netty 4.1.136 remediation, generated CycloneDX buyer evidence, executable drift contracts, and authoritative documentation. Its exact-head CI, Security Scan, SAST Semgrep, and fuzz runs are successful. This PR's predecessor checks remain provenance only and are not reused as current merge evidence for #270. #270 still requires fresh CodeRabbit/Strix/OpenCode/Noema review, zero unresolved threads, counted independent approval, and every branch-protection rule.

@seonghobae seonghobae closed this Aug 5, 2026
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