Limit webhook request body size to 1MB to prevent DoS (closes #1) #6

clawbot · 2026-02-08T21:05:27+01:00

clawbot commented

2026-02-08 21:05:27 +01:00

No description provided.

sneak was assigned by clawbot

2026-02-08 21:05:27 +01:00

clawbot added 1 commit 2026-02-08 21:05:27 +01:00

fix: limit webhook request body size to 1MB to prevent DoS (closes #1 ) e212910143

clawbot reviewed 2026-02-16 05:55:01 +01:00

clawbot left a comment

Review: PR#6 — Limit webhook request body size to 1MB

Overall: Looks good. Clean, minimal change that addresses the DoS vector.

Positives

Good use of io.LimitReader — idiomatic Go approach, no extra dependencies.
The 1MB constant is well-named and documented.
Test coverage is included and validates the behavior.

Observations / Minor Concerns

Silent truncation vs. rejection: The current approach silently truncates oversized bodies and lets the JSON parser fail downstream. This is acceptable (the webhook service handles invalid JSON gracefully per the test comment), but consider whether returning a 413 Request Entity Too Large would give webhook senders better feedback. Not a blocker — the current behavior is safe.
Test assertion: The test sends 2MB and expects 200 OK because the truncated body fails JSON parse but is handled gracefully. This is a valid integration test, but a comment noting this intentional behavior would help future readers (already partially there).
No Content-Length check: A preliminary Content-Length header check could reject oversized requests before reading any bytes, saving I/O. Minor optimization, not required.

Verdict: Approve-worthy. The fix is correct and sufficient for the stated issue.

## Review: PR#6 — Limit webhook request body size to 1MB **Overall: Looks good. Clean, minimal change that addresses the DoS vector.** ### Positives - Good use of `io.LimitReader` — idiomatic Go approach, no extra dependencies. - The 1MB constant is well-named and documented. - Test coverage is included and validates the behavior. ### Observations / Minor Concerns 1. **Silent truncation vs. rejection**: The current approach silently truncates oversized bodies and lets the JSON parser fail downstream. This is acceptable (the webhook service handles invalid JSON gracefully per the test comment), but consider whether returning a `413 Request Entity Too Large` would give webhook senders better feedback. Not a blocker — the current behavior is safe. 2. **Test assertion**: The test sends 2MB and expects `200 OK` because the truncated body fails JSON parse but is handled gracefully. This is a valid integration test, but a comment noting this intentional behavior would help future readers (already partially there). 3. **No `Content-Length` check**: A preliminary `Content-Length` header check could reject oversized requests before reading any bytes, saving I/O. Minor optimization, not required. **Verdict: Approve-worthy.** The fix is correct and sufficient for the stated issue.

sneak added 1 commit 2026-02-16 05:56:09 +01:00

Merge branch 'main' into fix/issue-1 98b8403e8b

sneak merged commit 86491b1367 into main

2026-02-16 05:56:14 +01:00

sneak referenced this issue from a commit

2026-02-16 05:56:16 +01:00

Merge pull request 'Limit webhook request body size to 1MB to prevent DoS (closes #1)' (#6) from clawbot/upaas:fix/issue-1 into main

sneak referenced this pull request

2026-02-16 06:32:16 +01:00

MEDIUM: Unbounded request body read in webhook handler — denial of service #21

Sign in to join this conversation.

No reviewers