fix: 추출 경로의 안전 공백 3건 (헤드리스 redirect SSRF·null 바이트 오분류·이미지 경로 검증 우회) - #20
Conversation
주석 정리(#18) 중 코드를 정독하다 드러난 것들이다. 셋 다 회귀 테스트를 함께 넣었고, 수정 전에는 네 테스트가 모두 실패함을 확인했다. 1) 헤드리스 경로의 redirect SSRF — HttpHeadlessRenderer 는 원본 URL 만 InternalHostGuard 로 검증하고, 렌더 서비스가 따라간 최종 URL(final_url)은 검증 없이 그대로 썼다. '외부 URL → 내부 주소' redirect 를 렌더 서비스가 대신 따라가 주면 내부망 응답이 상품 HTML 로 흘러들고, #17 이후로는 그 주소가 응답 계약의 finalUrl 로 호출자의 정체성(canonical) 입력까지 나간다. 정적 fetch 가 매 hop 을 검증하는 것과 같은 기준을 세웠다 — BLOCKED_HOST 면 렌더 전체를 거부하고, DNS 미해결처럼 '검증 불가'인 경우만 원본 link 로 폴백해 렌더 결과를 살린다. 2) ProductImage.of(null, ...) 가 500 — null 검사 전에 bytes.length 를 읽어 NPE 가 됐다. 계약상 확정 실패(422)여야 할 입력이 일시 실패로 오분류돼 호출자가 무의미한 재시도를 한다. 3) 이미지 경로가 정규화를 우회 — GeminiImageResult 가 ProductSnapshot.fromExtracted 를 건너뛰고 직접 생성해, 같은 LLM 이 만든 값인데도 음수 가격·공백 이름이 link 경로에서는 막히고 이미지 경로에서만 호출자에게 새어 나갔다. 같은 팩토리를 태워 검증을 일치시켰다.
|
Warning Review limit reached
Next review available in: 55 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 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough렌더 서비스의 Changes최종 URL SSRF 검증
이미지 추출 결과 검증
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HttpHeadlessRenderer
participant RenderService
participant InternalHostGuard
RenderService-->>HttpHeadlessRenderer: final_url 반환
HttpHeadlessRenderer->>InternalHostGuard: 최종 URL 검증
InternalHostGuard-->>HttpHeadlessRenderer: 검증 결과 반환
HttpHeadlessRenderer-->>HttpHeadlessRenderer: 차단 예외 전파 또는 원본 링크 폴백
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/main/java/com/depromeet/piki/extractor/extraction/headless/HttpHeadlessRenderer.java`:
- Around line 202-229: Update the headless navigation flow used by requestRender
so redirects are not followed automatically; validate every Location target with
internalHostGuard before issuing the next request, rejecting blocked hosts and
preserving the existing handling for non-blocking validation failures. Ensure
the egress policy is explicitly enforced and treated as this renderer path’s
security boundary, rather than relying only on resolveFinalUrl after rendering
completes.
In
`@src/test/java/com/depromeet/piki/extractor/extraction/headless/HttpHeadlessRendererTest.java`:
- Around line 249-270: Move the internalFinalUrlIsBlocked test out of
HttpHeadlessRendererTest into a feature-focused integration test class named
using the {feature}IntegrationTest convention, such as
HeadlessRenderIntegrationTest. Preserve its MockRestServiceServer HTTP contract
setup, renderer configuration, assertions, and existing test behavior unchanged.
🪄 Autofix (Beta)
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: ecb8a838-2bd0-4468-b764-a3fe27200944
📒 Files selected for processing (6)
src/main/java/com/depromeet/piki/extractor/extraction/headless/HttpHeadlessRenderer.javasrc/main/java/com/depromeet/piki/extractor/image/domain/ProductImage.javasrc/main/java/com/depromeet/piki/extractor/image/gemini/GeminiImageResult.javasrc/test/java/com/depromeet/piki/extractor/extraction/headless/HttpHeadlessRendererTest.javasrc/test/java/com/depromeet/piki/extractor/image/domain/ProductImageTest.javasrc/test/java/com/depromeet/piki/extractor/image/gemini/GeminiImageResultTest.java
| * 렌더 서비스가 redirect 를 따라간 최종 URL. Jsoup baseUri 이자 응답 계약의 finalUrl 로 호출자에게 나간다. | ||
| * <p>형식 위반은 원본 link 로 폴백한다 — baseUri 부정확은 치명이 아니고, 여기서 INVALID_URL 을 새면 렌더는 | ||
| * 성공했는데 확정 실패로 종결되는 오판이 된다. | ||
| * <p>단 <b>SSRF 판정은 폴백하지 않고 렌더 전체를 거부</b>한다. 원본 URL 만 검증하면 "외부 URL → 내부 주소" | ||
| * redirect 를 렌더 서비스가 대신 따라가 준 셈이 되어, 내부망 응답이 상품 HTML 로 흘러들고 그 주소가 호출자의 | ||
| * 정체성(canonical) 입력으로까지 나간다. 정적 fetch 가 매 hop 을 검증하는 것과 같은 기준을 여기에도 세운다. | ||
| */ | ||
| private ProductLink resolveFinalUrl(String finalUrl, ProductLink link) { | ||
| if (finalUrl == null || finalUrl.isBlank()) { | ||
| return link; | ||
| } | ||
| ProductLink parsed; | ||
| try { | ||
| return ProductLink.parse(finalUrl); | ||
| parsed = ProductLink.parse(finalUrl); | ||
| } catch (ExtractionException e) { | ||
| return link; | ||
| } | ||
| try { | ||
| internalHostGuard.verify(parsed); | ||
| } catch (PageFetchException e) { | ||
| if (e.code() == ExtractionErrorCode.BLOCKED_HOST) { | ||
| throw e; | ||
| } | ||
| // 그 외(DNS 미해결 등)는 검증 불가일 뿐 내부망 근거가 아니다 — 원본 link 로 폴백해 렌더 결과는 살린다. | ||
| log.warn("headless render finalUrl 검증 실패 code={} url={}", e.code(), link.safeLogString()); | ||
| return link; | ||
| } | ||
| return parsed; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
렌더러가 각 redirect hop을 요청 전에 검증하도록 변경하십시오.
resolveFinalUrl은 requestRender()가 응답을 받은 후에만 실행됩니다. 따라서 외부 URL이 내부 주소를 거친 뒤 다시 외부 URL로 redirect되면, finalUrl 검증은 마지막 외부 URL만 통과시킵니다. 이 경우 헤드리스 렌더러는 이미 내부 주소에 요청을 보냈습니다.
최종 URL이 내부 주소인 경우에도 현재 코드는 결과 소비만 차단합니다. 내부 주소 요청 자체는 방지하지 못합니다. 헤드리스 렌더러의 navigation 계층에서 자동 redirect를 끄고 각 Location을 따라가기 전에 InternalHostGuard와 동등한 검증을 수행하십시오. 렌더러의 egress 정책으로 이를 보장하는 경우에는 그 정책을 이 경로의 보안 경계로 명시하고 검증하십시오.
🤖 Prompt for 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.
In
`@src/main/java/com/depromeet/piki/extractor/extraction/headless/HttpHeadlessRenderer.java`
around lines 202 - 229, Update the headless navigation flow used by
requestRender so redirects are not followed automatically; validate every
Location target with internalHostGuard before issuing the next request,
rejecting blocked hosts and preserving the existing handling for non-blocking
validation failures. Ensure the egress policy is explicitly enforced and treated
as this renderer path’s security boundary, rather than relying only on
resolveFinalUrl after rendering completes.
| @Test | ||
| @DisplayName("final_url 이 내부망으로 resolve 되면 렌더 전체를 거부한다 — 원본만 검증하면 redirect 로 가드를 우회한다") | ||
| void internalFinalUrlIsBlocked() { | ||
| // 원본 host 는 공인 IP, 렌더 서비스가 따라간 최종 host 만 내부망인 상황 — 정적 fetch 가 매 hop 을 | ||
| // 검증하는 것과 달리 여기엔 검증이 없어, 내부망 응답이 상품 HTML 로 흘러들 수 있었다. | ||
| RequestScopedDnsResolver.HostResolver byHost = host -> "metadata.internal".equals(host) | ||
| ? new InetAddress[] {InetAddress.getByName("169.254.169.254")} | ||
| : new InetAddress[] {InetAddress.getByName("93.184.216.34")}; | ||
|
|
||
| HttpHeadlessRenderer renderer = rendererWith( | ||
| HeadlessExtractionProperties.of(true), byHost, ZstdDictionaries.none(), server -> server | ||
| .expect(requestTo(BASE_URL + "/render")) | ||
| .andRespond(withSuccess( | ||
| "{\"verdict\":\"OK\",\"html\":\"<html>ok</html>\"," | ||
| + "\"final_url\":\"https://metadata.internal/latest/meta-data/\"}", | ||
| MediaType.APPLICATION_JSON | ||
| ))); | ||
|
|
||
| PageFetchException ex = assertThrows(PageFetchException.class, () -> renderer.render(link)); | ||
|
|
||
| assertEquals(ExtractionErrorCode.BLOCKED_HOST, ex.code()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
이 테스트를 통합 테스트 클래스로 이동하십시오.
이 테스트는 MockRestServiceServer로 headless renderer의 HTTP 계약을 검증합니다. 따라서 단위 테스트가 아니라 통합 테스트입니다. 이 테스트와 같은 계약 테스트를 {기능}IntegrationTest 형식의 클래스(예: HeadlessRenderIntegrationTest)로 이동하십시오.
As per coding guidelines, "통합은 HTTP 계약을 외부 stub으로 검증하며" 및 "통합 {기능}IntegrationTest"를 사용해야 합니다.
🤖 Prompt for 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.
In
`@src/test/java/com/depromeet/piki/extractor/extraction/headless/HttpHeadlessRendererTest.java`
around lines 249 - 270, Move the internalFinalUrlIsBlocked test out of
HttpHeadlessRendererTest into a feature-focused integration test class named
using the {feature}IntegrationTest convention, such as
HeadlessRenderIntegrationTest. Preserve its MockRestServiceServer HTTP contract
setup, renderer configuration, assertions, and existing test behavior unchanged.
Source: Coding guidelines
CodeRabbit 이 지적한 대로 resolveFinalUrl 의 검증은 렌더 응답 이후에 도는 사후 검증이라, 내부 주소로의 요청 자체와 '외부 → 내부 → 외부' 체인은 막지 못한다. 코드가 그 한계를 스스로 말하도록 Javadoc 에 명시하고(다층 방어의 마지막 층이라는 위치까지), hop 단위 차단이 renderer 소관으로 남았음을 백로그에 남긴다. renderer 에 SSRF·egress 가드가 없음은 실측 확인했다.
CodeRabbit 리뷰 판정 (2건)1. 렌더 hop 단위 검증 (Major, Security) — 타당. 한계를 명시하고 후속을 분리한다지적이 정확하다.
renderer 쪽 실측 확인: 다만 그 차단은 브라우저 navigation 계층(자동 redirect off + Location 마다 판정)이나 렌더 박스 egress 정책에 있어야 하고, 이 repo 규약상 그 계층은 renderer 소관이다. 그래서 이 PR 에서는:
이 PR 의 완화만으로도 얻는 것은 분명하다 — 내부망 콘텐츠를 상품 HTML 로 소비하는 것과 그 주소가 2. 테스트를 통합 테스트 클래스로 이동 (Minor) — 반려이 repo 의 분류 기준은 CLAUDE.md
덧붙여 이 클래스는 같은 방식의 기존 테스트 10여 개를 이미 담고 있어, 새로 넣은 하나만 옮기면 wire 계약 검증이 두 곳으로 쪼개진다. |
1 participant
배경
주석 정리(#18) 중 코드를 정독하다 드러난 결함들이다. 그때는
docs/style-decisions.md§4 에 기록만 하고 손대지 않았고, 이번에 그중 안전·정합성에 걸리는 셋을 고친다. 셋 다 회귀 테스트를 함께 넣었고, 수정 전에는 네 테스트가 모두 실패함을 확인했다.1. 헤드리스 경로의 redirect SSRF (가장 중요)
HttpHeadlessRenderer는 원본 URL 만InternalHostGuard로 검증하고, 렌더 서비스가 따라간 최종 URL(final_url)은 검증 없이 그대로 썼다. 정적 fetch(HttpPageFetcher)가 매 hop 을 재검증하는 것과 비대칭이다.#17 이후로 위험이 커졌다 — 그 주소가 응답 계약의
finalUrl로 나가 호출자의 상품 정체성(canonical) 정규화 입력이 된다.수정:
final_url도 같은 가드를 태운다.BLOCKED_HOST면 폴백하지 않고 렌더 전체를 거부한다 — 그 HTML 자체가 내부망 산출물이라 폴백은 출처만 감추는 셈이다. 반면 DNS 미해결처럼 "검증 불가"인 경우는 내부망 근거가 아니므로 기존대로 원본 link 폴백 + warn 으로 렌더 결과를 살린다(형식 위반 폴백도 그대로).2.
ProductImage.of(null, ...)가 500null 검사 전에
bytes.length를 읽어 NPE 였다. 계약상 확정 실패(422 IMAGE_UNSUPPORTED) 여야 할 입력이 일시 실패로 오분류돼, 호출자가 같은 바이트로 무의미한 재시도를 한다.3. 이미지 경로가 정규화를 우회
GeminiImageResult가ProductSnapshot.fromExtracted를 건너뛰고 record 를 직접 생성했다. 같은 Gemini 가 만든 값인데도 link 경로에서는 막히는 음수 가격·공백 이름이 이미지 경로로만 호출자에게 새어 나갔다.ExtractionResponse.from도 null/blank 만 보므로 음수 가격은 그대로 통과한다.수정: 같은 팩토리를 태워 검증을 일치시켰다. 이제 음수 가격은 이미지 경로에서도
UNTRUSTWORTHY_VALUE(422)로 떨어진다.검증
./gradlew test·javadoc통과src/main수정만 되돌리고 돌리면 새 테스트 4개가 정확히 실패한다남은 백로그
docs/style-decisions.md§4 의 나머지(설계 약속·효율·잔가지)는 이 PR 범위 밖이다 —sanitize의 regex 주석 제거가 JSON 아일랜드를 오염시킬 수 있는 건,mimeTypeOfExtension하드코딩 switch,MAX_LLM_CHARS·Gemini 타임아웃 외부화 등.Summary by CodeRabbit
보안 개선
버그 수정