fix(local-http): bound nested percent decoding under security owner - #1635
fix(local-http): bound nested percent decoding under security owner#1635seonghobae wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe request target validator now iteratively decodes percent-encoded path segments, rejects invalid decoded escapes, and limits decoding depth. Tests cover safe literals, nested unsafe segments, excessive nesting, and invalid encoding. ChangesLocal request validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Local HTTP request-target validation now rejects nested unsafe or malformed percent encodings while preserving safe literal percent paths. The bounded decoding behavior and relevant edge cases are covered, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@codex review Review exact head |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review Please review exact |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='17a7618eda2b212b691f08fa936e042b34258fc9'
head='ff7f85a54f2027fc7d35748c916aa066e8072739'
git cat-file -e "${base}^{commit}"
git cat-file -e "${head}^{commit}"
printf '%s\n' '=== ancestry ==='
git merge-base --is-ancestor "$base" "$head" && echo 'base-is-ancestor=yes'
printf '%s\n' '=== changed files ==='
git diff --name-status "$base" "$head"
printf '%s\n' '=== exact diff ==='
git diff --find-renames --find-copies --unified=80 "$base" "$head" -- backend/core/local_http.py backend/tests/test_local_http.py
printf '%s\n' '=== source outline ==='
ast-grep outline backend/core/local_http.py --items all || true
printf '%s\n' '=== test outline ==='
ast-grep outline backend/tests/test_local_http.py --items all || true
printf '%s\n' '=== head source ==='
cat -n backend/core/local_http.py
printf '%s\n' '=== head test file ==='
cat -n backend/tests/test_local_http.py
printf '%s\n' '=== source-order RED commit ==='
git show --format=fuller --stat 763854fe229ca89e39497d00dea6e2b4848ee310
git show --format= --unified=50 763854fe229ca89e39497d00dea6e2b4848ee310 -- backend/core/local_http.py backend/tests/test_local_http.pyLength of output: 27417 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='ff7f85a54f2027fc7d35748c916aa066e8072739'
printf '%s\n' '=== commits from base to head ==='
git log --reverse --format='%H %s' '17a7618eda2b212b691f08fa936e042b34258fc9..'"$head"
printf '%s\n' '=== validate_local_request_target definitions and call sites at head ==='
git grep -n -C 6 'validate_local_request_target' "$head" -- ':!backend/tests/test_local_http.py'
printf '%s\n' '=== request construction near every call-site file ==='
git grep -l 'validate_local_request_target' "$head" -- ':!backend/tests/test_local_http.py' |
while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
git show "$head:$file" | nl -ba | sed -n '/validate_local_request_target/,+35p'
done
printf '%s\n' '=== percent-related tests at head ==='
git grep -n -i -C 3 'percent\|literal' "$head" -- backend/tests/test_local_http.py backend/core/local_http.py
printf '%s\n' '=== direct URL/request calls using validated target at head ==='
git grep -n -E -C 5 '(urljoin|requests\.|httpx\.|urllib\.request|client\.(get|post|request)|get\(|post\(|request\()' \
"$head" -- backend | head -n 500Length of output: 9549 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='ff7f85a54f2027fc7d35748c916aa066e8072739'
printf '%s\n' '=== exact-head consumer files ==='
git grep -l 'validate_local_request_target' "$head" -- ':!backend/core/local_http.py' ':!backend/tests/test_local_http.py' |
sed -E 's|^[^:]+:||' |
while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
git show "$head:$file" | nl -ba | sed -n '/def _validated_/,/^[[:space:]]*$/p;/validate_local_request_target/,+100p'
done
printf '%s\n' '=== decoding calls in consumers ==='
git grep -n -E -C 4 '(unquote|unquote_plus|percent[-_ ]decode|decode\()' "$head" -- \
backend/scripts/private_mail_http_smoke.py backend/tests/live/test_live_api_sequence.py || true
printf '%s\n' '=== exact-head local-http test cases for decoding boundary ==='
git show "$head:backend/tests/test_local_http.py" | nl -ba |
grep -n -E -C 5 'nested|excessive|percent|literal|preserves_safe' || true
printf '%s\n' '=== exact-head line-numbered changed helper ==='
git show "$head:backend/core/local_http.py" | nl -ba | sed -n '90,145p'Length of output: 71249
P3 — Reject malformed percent escapes after nested decoding
Validate The ten-round boundary is correct. A segment that becomes stable after exactly ten decode changes is accepted. A segment that needs an eleventh change is rejected. Valid The static consumers pass the original validated target directly to You are interacting with an AI system. |
|
@coderabbitai review Please re-review current exact |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='17a7618eda2b212b691f08fa936e042b34258fc9'
red1='763854fe229ca89e39497d00dea6e2b4848ee310'
fix1='ff7f85a54f2027fc7d35748c916aa066e8072739'
red2='709cb1b4bf05fa549be66c0e448e32902dbeffcc'
head='558c830651b64c2434cb64791a804f8059da6773'
for commit in "$base" "$red1" "$fix1" "$red2" "$head"; do
git cat-file -e "${commit}^{commit}"
done
printf '%s\n' '=== ancestry and commits ==='
git merge-base --is-ancestor "$base" "$head" && echo 'base-is-ancestor=yes'
git log --reverse --format='%H %s' "${base}..${head}"
printf '%s\n' '=== exact effective delta ==='
git diff --name-status "$base" "$head"
git diff --unified=100 "$base" "$head" -- backend/core/local_http.py backend/tests/test_local_http.py
printf '%s\n' '=== commit progression ==='
for commit in "$red1" "$fix1" "$red2" "$head"; do
printf '\n--- %s ---\n' "$commit"
git show --format='%H%n%s' --stat "$commit"
git show --format= --unified=60 "$commit" -- backend/core/local_http.py backend/tests/test_local_http.py
done
printf '%s\n' '=== exact-head helper structure ==='
ast-grep outline backend/core/local_http.py --items all
printf '%s\n' '=== exact-head helper and decode-related tests ==='
nl -ba backend/core/local_http.py | sed -n '1,220p'
nl -ba backend/tests/test_local_http.py | grep -E -C 12 'validate_local_request_target|percent|nested|literal|excessive|round|traversal|backslash|control'
printf '%s\n' '=== exact-head consumers and decode calls ==='
git grep -n -C 10 'validate_local_request_target' "$head" -- ':!backend/core/local_http.py' ':!backend/tests/test_local_http.py' || true
git grep -n -E -C 4 '\b(unquote|unquote_plus|decode)\s*\(' "$head" -- backend || trueLength of output: 131022
I found no unresolved findings in the requested scope.
This remains a narrow defense-in-depth validation change. The reviewed code does not support a broader production SSRF claim.
|
|
@coderabbitai review Re-review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='17a7618eda2b212b691f08fa936e042b34258fc9'
head='558c830651b64c2434cb64791a804f8059da6773'
red1='763854fe229ca89e39497d00dea6e2b4848ee310'
red2='709cb1b4bf05fa549be66c0e448e32902dbeffcc'
for commit in "$base" "$head" "$red1" "$red2"; do
git cat-file -e "${commit}^{commit}"
done
printf '%s\n' '=== ancestry and effective delta ==='
git merge-base --is-ancestor "$base" "$head" && echo 'base-is-ancestor=yes'
git diff --name-status "$base" "$head"
git diff --unified=100 "$base" "$head" -- backend/core/local_http.py backend/tests/test_local_http.py
printf '%s\n' '=== exact-head structure ==='
ast-grep outline backend/core/local_http.py --items all
ast-grep outline backend/tests/test_local_http.py --items all
printf '%s\n' '=== exact-head validation helper ==='
git show "$head:backend/core/local_http.py" | nl -ba | sed -n '1,240p'
printf '%s\n' '=== exact-head relevant tests ==='
git show "$head:backend/tests/test_local_http.py" | nl -ba | \
grep -E -C 14 'validate_local_request_target|percent|nested|literal|excessive|round|traversal|backslash|control'
printf '%s\n' '=== source-order RED patches ==='
for commit in "$red1" "$red2"; do
printf '\n--- %s ---\n' "$commit"
git show --format='%H%n%s' --unified=80 "$commit" -- \
backend/core/local_http.py backend/tests/test_local_http.py
done
printf '%s\n' '=== exact-head consumers and decode operations ==='
git grep -n -C 12 'validate_local_request_target' "$head" -- \
':!backend/core/local_http.py' ':!backend/tests/test_local_http.py' || true
git grep -n -E -C 4 '\b(unquote|unquote_plus|decode)\s*\(' "$head" -- backend || trueLength of output: 123704 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='558c830651b64c2434cb64791a804f8059da6773'
for file in \
backend/scripts/private_mail_http_smoke.py \
backend/tests/live/test_live_api_sequence.py
do
printf '\n=== %s: structure ===\n' "$file"
ast-grep outline "$file" --items all
printf '\n=== %s: validated target and HTTP request contexts ===\n' "$file"
git show "$head:$file" | nl -ba | \
grep -E -C 18 'validate_local_request_target|HTTP(S)?Connection|\.request\s*\('
doneLength of output: 13894
I found no unresolved findings in
This is a narrow defense-in-depth validation change. The reviewed code does not establish a production SSRF chain. ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Current authority — 2026-09-10
#1623@17a7618eda2b212b691f08fa936e042b34258fc9763854fe229ca89e39497d00dea6e2b4848ee310ff7f85a54f2027fc7d35748c916aa066e8072739709cb1b4bf05fa549be66c0e448e32902dbeffcc558c830651b64c2434cb64791a804f8059da6773backend/core/local_http.py,backend/tests/test_local_http.py.Generated PR #1634 identified a real helper-level validation gap but was based directly on stale protected
develop, omitted a retained regression, carried an unrelated generated security-history note, and overstated the live exploit as CRITICAL SSRF/arbitrary local endpoint access. This successor keeps only the verified causal delta and inherits the current frontend dependency-security owner rather than reproducing its Trivy RED.The helper is explicitly for local HTTP smoke/live-test request targets. Static call-graph inspection found consumers passing the original validated target directly to
HTTPConnection.request/HTTPSConnection.requestand found no explicit downstream decode. This remains defense-in-depth/future-proofing, not a verified production SSRF.TDD/review lineage is explicit. RED
763854f...adds nested traversal/backslash/control and excessive-depth cases on unchanged #1623 source.ff7f85a...implements bounded decode-until-stable semantics. CodeRabbit then found a valid P3 contract hole: a valid outer encoding could reveal a malformed nested escape (%2525GG -> %25GG -> %GG, or%252 -> %2) thaturllib.parse.unquote()leaves stable. RED709cb1b...adds those cases while also locking preservation of a safe encoded literal percent (/api/literal%25). Exact head558c830...adds a decoded-layer malformed-escape check that permits a terminal literal%but rejects a percent followed by characters that are not a complete hex escape before continuing recursive decoding. The ten-round ceiling and original-path return semantics are unchanged.Current review and execution evidence
CodeRabbit re-reviewed the exact base/head
17a7618... -> 558c830...after the earlier transient capacity limit, selected exactly the two owned files, generated no actionable comments, and submitted formalAPPROVEDreviewPRR_kwDOSNjZ2s8AAAABM7VI_Aat 2026-09-10T03:57:39Z. Current review-thread inventory is empty. This satisfies the independent current-head review lane; it does not substitute for executable product tests.Exact
558c830...still has zero PR-triggered Actions workflow runs. Fresh protected Naruon workflows explain the absence: feature-base stacked PRs do not match the current Application CI/Bandit base filters even though protected guidance describes stacked verification. Canonical CI-owner defect.github#2073now owns that mismatch. Do not retarget todevelop, push a dummy commit, copy central workflow source, or promote status-only review signals as test evidence.Keep Draft until exact-head executable checks exist through the canonical owner path and upstream #1623 is protected-integrated. No generated
.julesnote, duplicate dependency source, dummy requeue, self-approval, provider/model workaround, or gate weakening is carried here.