feat(core): one definition of what a redirect hop must pass (RedirectGuard) - #21
Merged
Conversation
…Guard) Answers a question the owner asked while reviewing the parity audit: why fix redirect re-validation per adapter instead of moving it into core? Mostly right — and the split is worth naming. The CHECK already lived in core (UrlPolicy.validate). What could not move is the interception point: the JS sibling owns its redirect loop, so it writes "what do we check on a hop" once, while on the JVM each adapter wraps someone else's client and the client owns the loop. What can and must move is the DECISION, because each adapter was improvising it: httpclient5 scheme + DNS — not port, userinfo, IP-literal okhttp host + private IP (Dns layer) — nothing else jdkhttp NOTHING; the JDK client follows redirects internally A redirect off an allowlisted host is the shape SSRF actually takes. A hop checked more weakly than the first request is a hole with extra steps. core: RedirectGuard is now the single definition — the full policy, re-thrown as blocked_redirect — plus the origin comparison, the fetch-spec method-downgrade rule and the credential-header list, so adapters stop re-deriving those too. jdkhttp: SsrfGuardedHttpClient drives the loop itself, with the same semantics as the JS sibling (303 and POST 301/302 downgrade to GET and drop the body, credentials stripped cross-origin, maxRedirects bounds the chain). It REQUIRES a delegate built with Redirect.NEVER and throws otherwise: a delegate that follows redirects internally would bypass the policy on every hop, and this class cannot honour its contract over one. Failing loudly beats a guard that quietly does nothing. Eight tests drive it against a real loopback server. httpclient5: SafeRedirectStrategy takes the UrlPolicy and calls the seam. The three-arg constructor is deprecated and keeps the old scheme-only behaviour so existing code compiles. OkHttp is deliberately NOT included. I wrote a network interceptor for it and the tests failed with "Network is unreachable" — because OkHttp invokes network interceptors AFTER the connection is established, so the request had already reached the metadata address. It looked like the seam and is not one; for SSRF the connection IS the attack. Closing it needs the same loop treatment as jdkhttp, which changes that adapter's contract, so it is tracked separately rather than half-done here. Verified: ./gradlew build BUILD SUCCESSFUL, 237 tests across all modules (was 231), failures=0 errors=0. Changelog updated in all three surfaces.
# Conflicts: # CHANGELOG.md # docs/changelog.ko.md # docs/changelog.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
말씀하신 "어댑터로 분리하지 말고 코어로 옮기면 되지 않냐"에 대한 답입니다. 대체로 맞고, 나뉘는 지점을 짚을 값어치가 있습니다.
무엇이 옮겨지고 무엇이 못 옮겨지나
UrlPolicy.validate). 그게 문제가 아니었습니다.safeFetch가 리다이렉트 루프를 소유해서 "홉에서 뭘 검사하나"를 한 번만 씁니다. JVM은 어댑터가 남의 클라이언트를 감싸고 루프는 그 클라이언트 것입니다.Dns계층) — 그 외 없음허용된 호스트에서 튕겨 나가는 리다이렉트가 SSRF의 실제 형태입니다. 첫 요청보다 약하게 검사되는 홉은 단계만 늘어난 구멍입니다.
변경
core —
RedirectGuard가 단일 정의입니다. 정책 전체를 적용하고blocked_redirect로 재던짐. origin 비교·fetch 스펙 메서드 강등 규칙·자격증명 헤더 목록도 함께 둬서 어댑터가 그것들까지 재유도하지 않게 했습니다.jdkhttp — 말씀하신 1번.
SsrfGuardedHttpClient가 루프를 직접 돕니다. JS와 같은 시맨틱(303·POST의301/302→GET강등 + 본문 폐기, origin 넘으면 자격증명 제거,maxRedirects기본 5).Redirect.NEVERdelegate를 요구하고 아니면 던집니다. 내부에서 리다이렉트를 따라가는 delegate 위에서는 이 클래스가 자기 계약을 지킬 수 없습니다 — 조용히 아무것도 안 하는 가드보다 시끄럽게 실패하는 게 낫습니다. 실제 루프백 서버 대상 테스트 8개로 검증했습니다.httpclient5 —
SafeRedirectStrategy가UrlPolicy를 받아 같은 이음매를 호출. 3인자 생성자는 deprecated로 남겨 기존 코드가 컴파일됩니다.OkHttp는 일부러 뺐습니다 — 시도했다가 틀린 걸 확인했습니다
network interceptor를 만들었는데 테스트가
SocketException: Network is unreachable로 실패했습니다. OkHttp는 연결이 맺어진 뒤에 network interceptor를 호출합니다. 요청이 이미 메타데이터 주소에 닿은 뒤였습니다.이음매처럼 보였지만 아니었습니다. SSRF에서는 연결 자체가 공격입니다. 제대로 닫으려면 jdkhttp와 같은 루프 처리가 필요하고 그건 그 어댑터의 계약을 바꾸는 일이라, 반쯤 해두지 않고 별건으로 뺐습니다. 만들었던 클래스는 지웠습니다.
검증
CHANGELOG 3표면 갱신. 미출시 3.2.0에 들어갑니다.
PR #20(스캐너 수정)과 독립입니다 — 둘 다
main기준이고 순서 무관하게 머지 가능합니다.