Skip to content

fix(security): reject unsafe frame-ancestor configuration - #382

Open
seonghobae wants to merge 4 commits into
mainfrom
fix/frame-ancestors-csp-injection-main-20260811
Open

fix(security): reject unsafe frame-ancestor configuration#382
seonghobae wants to merge 4 commits into
mainfrom
fix/frame-ancestors-csp-injection-main-20260811

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Objective

Fail closed when the server-owned viewer.security.frame-ancestors configuration contains CSP directive delimiters or HTTP control characters. Protected main currently concatenates the configured value directly into the Content-Security-Policy header, so a malformed value such as https://trusted.example; script-src * can inject an unintended CSP directive and an embedded line break can cross the response-header safety boundary.

Test-first state

This Draft starts intentionally RED at exact test-only head f827aaed03460204a38caf928cfcbba366006750, directly based on protected main 55d7ae8647208e301f282350f076eeddaba61d11. ViewerSecurityHeadersWebFilterTest.rejectsFrameAncestorDirectiveAndHeaderInjection requires constructor-time rejection of both a semicolon directive injection and an embedded newline/header injection. Protected main accepts both values, so the focused regression must fail before the production repair.

Scope

ViewerSecurityHeadersWebFilter configuration validation and its focused regression only. This does not change the normal 'self' default, valid custom frame-ancestor source lists, viewer routing, PDF.js worker policy, auth, persistence, Office conversion, or repository governance. PR #306 carries historical broad ancestry that includes this filter but does not own this new current-base security invariant; its eventual stale-descendant reconciliation must preserve the protected-main result rather than overwrite it.

Acceptance

Observe exact-head RED -> add the smallest fail-closed character-boundary validation -> exact-head mvn -B --no-transfer-progress verify with zero missed owned production lines/branches and warning-free public Javadocs -> CI/Security Scan/SAST/fuzz where emitted -> current review/thread/live-base refetch. Keep Draft until the unchanged exact head is GREEN. Independent write-authorized approval remains a separate protected-merge gate.

Summary by CodeRabbit

  • 버그 수정
    • CSP frame-ancestors 설정값에 세미콜론이나 제어 문자가 포함될 경우 설정 오류를 명확히 감지합니다.
    • 악의적인 CSP 지시문 삽입 및 HTTP 헤더 주입 가능성을 방지합니다.
  • 테스트
    • 잘못된 설정값에 대해 적절한 예외가 발생하는지 검증하는 테스트를 추가했습니다.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d64c9f2-1892-4573-9efc-a21ad3cee5ef

📥 Commits

Reviewing files that changed from the base of the PR and between 55d7ae8 and 829d677.

📒 Files selected for processing (2)
  • src/main/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilter.java
  • src/test/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilterTest.java

📝 Walkthrough

Walkthrough

frame-ancestors 설정값에 CSP 확장 문자와 제어 문자가 포함되지 않도록 검증을 추가했다. 잘못된 설정값에 대한 예외 메시지와 동작을 테스트한다.

Changes

CSP 설정 검증

Layer / File(s) Summary
frame-ancestors 안전 문자 검증
src/main/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilter.java, src/test/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilterTest.java
frame-ancestors 값에 세미콜론 또는 제어 문자가 포함되면 IllegalArgumentException을 발생시킨다. 테스트는 예외 유형과 메시지를 검증한다.

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

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 안전하지 않은 frame-ancestor 설정을 거부하는 주요 변경 사항을 간결하고 정확하게 설명합니다.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/frame-ancestors-csp-injection-main-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 11, 2026 09:40
@seonghobae
seonghobae enabled auto-merge (squash) August 11, 2026 19:43
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