fix(etl): bound HTTP payloads before MVC materialization - #248
Closed
seonghobae wants to merge 5 commits into
Closed
fix(etl): bound HTTP payloads before MVC materialization#248seonghobae wants to merge 5 commits into
seonghobae wants to merge 5 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughETL 컨트롤러의 문자열 요청 본문에 최대 바이트 제한을 추가했다. 알려진 초과 요청은 본문 읽기 전에 거부한다. 길이를 알 수 없는 요청은 제한 스트림으로 초과 바이트를 감지한다. 단위 테스트와 MVC 테스트를 추가했다. ChangesETL 페이로드 수용 제어
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EtlPayloadAdmissionAdvice
participant BoundedInputStream
participant EtlController
Client->>EtlPayloadAdmissionAdvice: HTTP 문자열 본문 전송
EtlPayloadAdmissionAdvice->>EtlPayloadAdmissionAdvice: Content-Length 검사
EtlPayloadAdmissionAdvice->>BoundedInputStream: 미상 길이 본문 래핑
BoundedInputStream->>BoundedInputStream: 허용량 및 초과 바이트 확인
BoundedInputStream-->>EtlPayloadAdmissionAdvice: 본문 또는 PAYLOAD_TOO_LARGE 예외
EtlPayloadAdmissionAdvice->>EtlController: 허용된 본문 전달
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
etl-service/src/test/java/com/xtrmetl/etl/controller/EtlPayloadAdmissionAdviceTest.java (1)
89-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
getBody()의IOException전파를 검증하세요.Line 96은
HttpInputMessage.getBody()를 호출합니다. 현재 테스트는 이 호출이IOException을 발생시키는 경우를 만들지 않습니다.beforeBodyRead가 해당 예외를 변경하지 않고 전파하는 테스트를 추가하세요. 이 테스트는 PR 목표의 stream acquisition error propagation을 검증합니다.🤖 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 `@etl-service/src/test/java/com/xtrmetl/etl/controller/EtlPayloadAdmissionAdviceTest.java` around lines 89 - 103, Extend EtlPayloadAdmissionAdviceTest with a beforeBodyRead case using an HttpInputMessage whose getBody() throws IOException, and assert that the same exception propagates unchanged. Reuse advice() and requestBodyParameter() while keeping the test focused on stream acquisition error propagation.
🤖 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.
Nitpick comments:
In
`@etl-service/src/test/java/com/xtrmetl/etl/controller/EtlPayloadAdmissionAdviceTest.java`:
- Around line 89-103: Extend EtlPayloadAdmissionAdviceTest with a beforeBodyRead
case using an HttpInputMessage whose getBody() throws IOException, and assert
that the same exception propagates unchanged. Reuse advice() and
requestBodyParameter() while keeping the test focused on stream acquisition
error propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dd9e9c6-831a-45fb-a91f-2a22b030dad6
📒 Files selected for processing (3)
etl-service/src/main/java/com/xtrmetl/etl/controller/EtlPayloadAdmissionAdvice.javaetl-service/src/test/java/com/xtrmetl/etl/controller/EtlHttpPayloadAdmissionTest.javaetl-service/src/test/java/com/xtrmetl/etl/controller/EtlPayloadAdmissionAdviceTest.java
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.
Purpose
Advance #247 from exact protected
develop@c2511f6b9d716ef9ab8de60a91fffd826714a4c5without touching the activeEtlController(#236),EtlService(#199),etl-service/pom.xml(#164), or canonical-documentation (#149) writer lanes.The protected implementation enforced
xtrmetl.etl.max-payload-bytesonly insideEtlService, after Spring MVC had materialized@RequestBody String. This Draft now adds an endpoint-scopedRequestBodyAdviceguard that rejects known oversizedContent-Lengthbefore reading the entity and bounds unknown/understated streams to at most configured-limit-plus-one byte before raising the existing typedetl_payload_too_largeerror. The service-level check remains defense in depth.TDD / exact-head evidence
fd8b330a09fb8ea6055974086241e0acc96ba2cc: endpoint tests proved both known-length and unknown/chunked oversized bodies still reached MVC materialization/controller handling without a transport guard.487d1ba950c26deb25495b367a4021a71be46180: addedEtlPayloadAdmissionAdvice; hosted CI exposed anIOExceptionpropagation compile defect.c77503a6b8867a0afa7d3f9b2c6ab4b57d24412f: propagated stream acquisition errors correctly; hosted full reactor tests passed.a5787de108ae5fcd0389a9b9746e69e7577348a4: added exact configured-limit and just-below-limit HTTP acceptance cases.4d928203f96c9f4dbcb2ac02846ae7ad531ba03f: added direct stream-consumption proof. Known oversize is rejected after zero body bytes; unknown oversize is rejected after exactly limit+1 bytes; exact-limit unknown-length input reaches EOF normally.Current hosted CI run
31428935345passed Ubuntu/macOS/Windows against synthetic PR merge4fc2ce6075e0b7c4197953b8e98c8c5923d11070(4d928203...intoc2511f6b...). On macOS,etl-serviceran 282 tests with zero failures, includingEtlPayloadAdmissionAdviceTest3/3 andEtlHttpPayloadAdmissionTest4/4; the whole reactor passed. Current-head SBOM, Dependency Review, Semgrep, and Security Scan workflow aggregates report success.Remaining non-passing gates
Keep Draft. The current ETL JaCoCo configuration is not acceptable coverage evidence: the successful CI log explicitly reports
Analyzed bundle 'etl-service' with 0 classesand then vacuously reports that coverage checks were met. PR #164 ownsetl-service/pom.xml, so this branch will not race that coverage-gate repair.Security aggregate success is also not complete vulnerability evidence. Hard Trivy job
93587281876checked out synthetic merge4fc2ce6075e0b7c4197953b8e98c8c5923d11070, not the literal source head, and emittedDependency version cannot be determined. Child dependencies will not be found.before its zero-findings SARIF; issue #196 owns that dependency-graph-completeness gate. It also reproduced the separately tracked 82 MBzipkin.jarlarge-file warning. Neither warning is converted into a passing gate here.Current workflows validate the GitHub-generated PR merge revision rather than a literal head checkout, so that distinction remains explicit. There are currently no formal reviews and no review threads on this exact head; therefore no qualifying independent non-author APPROVED review exists.
No
EtlController,EtlService, POM, or canonical-documentation file is changed by this PR.Summary by CodeRabbit
새로운 기능
413 Payload Too Large로 거부됩니다.테스트