fix(security): reject unsafe frame-ancestor configuration on current main - #430
fix(security): reject unsafe frame-ancestor configuration on current main#430seonghobae wants to merge 3 commits into
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
|
Exact-current-head review request for @opencode-agent review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='961f44a19c4ced87f8062b3ab9842a9a08d05235'
head='032b75d2a1bd2c9488d3aa8198a46b0381641ef7'
printf '%s\n' '== Commit verification =='
git rev-parse HEAD
git show -s --format='head=%H%nparents=%P%nsubject=%s' "$head"
printf 'merge-base=%s\n' "$(git merge-base "$base" "$head")"
printf '%s\n' '== Changed files =='
git diff --name-status "$base" "$head"
printf '%s\n' '== AST outlines for changed Java files =='
while IFS= read -r file; do
case "$file" in
*.java)
printf '\n-- %s --\n' "$file"
ast-grep outline "$file" --items all --view expanded || true
;;
esac
done < <(git diff --name-only "$base" "$head")
printf '%s\n' '== Complete review patch =='
git diff --no-ext-diff --unified=80 "$base" "$head"Length of output: 1640 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='961f44a19c4ced87f8062b3ab9842a9a08d05235'
head='032b75d2a1bd2c9488d3aa8198a46b0381641ef7'
changed_list="$(mktemp)"
cleanup() {
rm -f "$changed_list"
}
trap cleanup EXIT
git diff --name-only "$base" "$head" > "$changed_list"
printf '%s\n' '== AST outlines for changed Java files =='
while IFS= read -r file; do
case "$file" in
*.java)
printf '\n-- %s --\n' "$file"
ast-grep outline "$file" --items all --view expanded || true
;;
esac
done < "$changed_list"
printf '%s\n' '== Complete review patch =='
git diff --no-ext-diff --unified=100 "$base" "$head"
printf '%s\n' '== Validation-related references in changed files =='
rg -n -C 8 --glob '*.java' \
'(frame-ancestors|ISO|control|semicolon|;|self|SecurityHeaders)' \
src/main/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilter.java \
src/test/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilterTest.javaLength of output: 61817
The checked-out head is The diff changes only the requested filter and focused test. I did not modify the branch or issue an approval. Independent approval requirements remain in effect.
|
|
@opencode-agent review Review exact head Review read-only; preserve independent reviewer identity. Do not mutate the branch, synthesize author approval, weaken protection, or merge. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='961f44a19c4ced87f8062b3ab9842a9a08d05235'
head='032b75d2a1bd2c9488d3aa8198a46b0381641ef7'
printf '%s\n' '== Commit range =='
printf 'checked_out=%s\n' "$(git rev-parse HEAD)"
printf 'head=%s\n' "$(git rev-parse "$head")"
printf 'merge_base=%s\n' "$(git merge-base "$base" "$head")"
git show -s --format='subject=%s%nparents=%P' "$head"
printf '%s\n' '== Changed files =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Full patch =='
git diff --no-ext-diff --unified=100 "$base" "$head"
printf '%s\n' '== Production and test structure =='
for file in \
src/main/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilter.java \
src/test/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilterTest.java
do
if [ -f "$file" ]; then
printf '\n-- %s --\n' "$file"
ast-grep outline "$file" --items all --view expanded || true
else
printf 'Missing expected file: %s\n' "$file"
fi
done
printf '%s\n' '== Validation and test references =='
rg -n -C 12 --glob '*.java' \
'(normalizeFrameAncestors|frame-ancestors|ISO.*CONTROL|isISOControl|semicolon|Content-Security-Policy|defaultFrameAncestors)' \
src/main/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilter.java \
src/test/java/com/clearfolio/viewer/config/ViewerSecurityHeadersWebFilterTest.java
printf '%s\n' '== Fuzz-target discovery for relevant security paths =='
rg -n -i -C 3 \
'(jazzer|clusterfuzz|fuzz.*(header|csp|viewer|security)|ViewerSecurityHeadersWebFilter)' \
. \
-g '!**/.git/**' -g '!**/node_modules/**' -g '!**/build/**' -g '!**/dist/**' \
|| trueLength of output: 88311
I found one blocking finding. [P1] Add a discoverable fuzz target for
The implementation otherwise rejects semicolons and ISO control characters before CSP construction. It also preserves the 🧠 Learnings usedYou are interacting with an AI system. |
|
Exact-current-head independent review request for The two-file CSP configuration boundary is mergeable; CI, Security Scan, SAST Semgrep, and fuzz are terminal-success; unresolved inline findings are zero. Semicolon directive delimiters and every ISO control character fail before response-header construction, while the default and valid custom source-list behavior remains unchanged. @opencode-agent review Review this exact head read-only. Do not mutate the branch, synthesize author approval, weaken protection, or merge. Any base update must regenerate all exact-head evidence. |
Rate Limit Exceeded
|
1 participant
Objective
Reject CSP directive delimiters and HTTP control characters in the server-owned
viewer.security.frame-ancestorsconfiguration before it can enter a response header.Exact current-head evidence
961f44a19c4ced87f8062b3ab9842a9a08d05235;032b75d2a1bd2c9488d3aa8198a46b0381641ef7;ViewerSecurityHeadersWebFilter.javaand its focused regression only;Security contract
Null, blank, and explicit
selfpreserve the documented'self'default. Valid custom source lists remain supported. A semicolon or any ISO control character fails construction with a controlled error, preventing additional CSP directive injection and CR/LF-style response-header injection before the policy is concatenated.Scope boundary
Configuration validation only. This does not change viewer routing, the rest of the CSP policy, PDF.js worker allowances, authentication, persistence, Office conversion, or central governance.
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. Protected
mainhas advanced since reconstruction; any required base update must regenerate every exact-head check and review. No predecessor evidence transfers.