Skip to content

fix(security): validate client trace identifiers - #352

Open
seonghobae wants to merge 3 commits into
mainfrom
fix/validate-client-trace-id-20260811
Open

fix(security): validate client trace identifiers#352
seonghobae wants to merge 3 commits into
mainfrom
fix/validate-client-trace-id-20260811

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Objective

Bound the client-controlled X-Trace-Id diagnostic boundary so API error envelopes cannot echo arbitrary path-like or oversized identifiers while still preserving a conventional opaque correlation identifier.

Fresh exact state — 2026-08-11

  • exact source head: 4bd555bab85741dbfa6fce9ee18e748f14d75551;
  • exact protected main: 55d7ae8647208e301f282350f076eeddaba61d11;
  • source is two commits ahead of protected main and mergeable;
  • exact-head CI 31414660717: success;
  • exact-head Security Scan 31414660570: success;
  • exact-head SAST Semgrep 31414660667: success;
  • exact-head fuzz 31414660610: success;
  • CI Maven test and synthetic-merge compatibility both succeeded on the exact source head, including repository coverage/Javadoc acceptance;
  • no formal reviews and no unresolved inline review threads at the latest direct refetch;
  • no automated check, status or model output is treated as qualifying independent approval.

Test-first evidence

RED

Test-only head 64a5ee31f5ac5e659f0966d84aaa2011b08a29c1 added focused regressions without changing production. Exact-head CI Maven job 93539677347 checked out that SHA and ran 499 tests. Exactly two intended tests failed: protected code echoed ../../tenant-secret and a 129-character client trace identifier instead of falling back to the server request id. The bounded conventional identifier case remained green. Security Scan and Semgrep were already green on the RED head.

GREEN

Current production code validates X-Trace-Id against a bounded opaque identifier grammar: one leading ASCII alphanumeric followed by at most 127 ASCII alphanumeric, dot, underscore, colon or hyphen characters. Invalid or oversized client identifiers are ignored in favor of the server request id; if that server identifier is also unsuitable, the existing generated UUID fallback remains authoritative.

The exact-head regression proves path-like and oversized values are not echoed while req-20260811_02:17.55 remains accepted. Exact-head CI, Security Scan, SAST and fuzz are all GREEN.

Scope

Merge gate

Move to Ready for ordinary independent review. Keep this exact head unchanged. Merge or auto-merge may complete only after live repository protection still sees every applicable required check passing, zero valid unresolved findings, and a counted approving review from a qualifying independent reviewer with write access. Automated comments, checks, statuses and model output are evidence, not approval.

Summary by CodeRabbit

  • 버그 수정
    • 요청 추적 ID의 형식과 길이를 검증해 안전하지 않거나 비정상적인 값이 사용되지 않도록 개선했습니다.
    • 유효하지 않은 추적 ID가 전달되면 자동으로 새로운 ID를 생성합니다.
    • 유효한 추적 ID는 기존처럼 오류 응답에 유지됩니다.
  • 테스트
    • 다양한 길이와 형식의 추적 ID 처리 동작을 검증하는 테스트를 추가했습니다.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

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

Next review available in: 30 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: b2a0386d-9c20-430f-af75-b0bc9587f2ee

📥 Commits

Reviewing files that changed from the base of the PR and between 4bd555b and e5f344e.

📒 Files selected for processing (1)
  • src/test/java/com/clearfolio/viewer/controller/ApiExceptionHandlerTraceIdValidationTest.java
📝 Walkthrough

Walkthrough

ApiExceptionHandler가 trace ID의 허용 문자와 최대 길이를 검증합니다. 검증에 실패한 값은 UUID로 대체합니다. 유효한 trace ID는 유지하며, 이 동작을 테스트합니다.

Changes

Trace ID 검증

Layer / File(s) Summary
Trace ID 검증 및 대체 처리
src/main/java/com/clearfolio/viewer/controller/ApiExceptionHandler.java, src/test/java/com/clearfolio/viewer/controller/ApiExceptionHandlerTraceIdValidationTest.java
trace ID가 영숫자로 시작하고 허용 문자만 포함하며 128자 이하인지 검증합니다. 검증에 실패하면 UUID를 생성합니다. 테스트는 위험한 값, 초과 길이 값, 유효한 불투명 trace ID를 확인합니다.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 클라이언트 trace identifier 검증이라는 pull request의 주요 변경 사항을 정확하고 간결하게 설명합니다.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/validate-client-trace-id-20260811

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 10, 2026 17:38

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/controller/ApiExceptionHandlerTraceIdValidationTest.java`:
- Around line 20-68: Extend ApiExceptionHandlerTraceIdValidationTest with a case
where both the X-Trace-Id header and the exchange request ID are unsafe, then
assert that handleBadRequest returns a UUID.randomUUID()-style trace ID rather
than either input. Configure the exchange or request-ID source to provide the
unsafe server request ID and verify the fallback branch is exercised.
🪄 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: 0c147ae0-8fc6-4b18-90e5-a23f535a582e

📥 Commits

Reviewing files that changed from the base of the PR and between 55d7ae8 and 4bd555b.

📒 Files selected for processing (2)
  • src/main/java/com/clearfolio/viewer/controller/ApiExceptionHandler.java
  • src/test/java/com/clearfolio/viewer/controller/ApiExceptionHandlerTraceIdValidationTest.java

@seonghobae
seonghobae enabled auto-merge (squash) August 10, 2026 18:38
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