fix(api): harden request parsing against panics and oversized bodies (EN-1201)#107
Conversation
parsePageSize panicked on a non-numeric pageSize (e.g. ?pageSize=abc) and the router had no recoverer, so the client got a reset connection instead of a 400. Negative and unbounded page sizes were also passed straight to the ledger. - parsePageSize returns an error instead of panicking; reject < 1 and clamp to maxPageSize (100); list handlers return 400 VALIDATION on bad input - add middleware.Recoverer as a safety net - cap request bodies with http.MaxBytesReader (1 MiB) ahead of the audit middleware, which buffers the whole body, to prevent memory-exhaustion DoS Adds unit tests for parsePageSize bounds and a list handler 400 case.
|
Warning Review limit reached
More reviews will be available in 9 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 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 |
flemzord
changed the title
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Revue inline: le plafond de body protège la mémoire, mais le chemin d’erreur renvoie encore un mauvais statut.
| r.Use(middleware.Recoverer) | ||
| r.Use(func(handler http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| r.Body = http.MaxBytesReader(w, r.Body, maxRequestBodyBytes) |
There was a problem hiding this comment.
Avec ce placement, un body trop gros est bien plafonné, mais il ressort en 500. httpaudit.Middleware lit r.Body avec io.ReadAll avant le handler et transforme toute erreur non-EOF en http.StatusInternalServerError; MaxBytesReader renvoie justement *http.MaxBytesError au-delà de 1 MiB. Un client qui dépasse la limite reçoit donc 500 failed to read request body au lieu de 413 Request Entity Too Large. Il faudrait gérer ce cas avant l’audit, ou faire traiter *http.MaxBytesError spécialement par l’audit, avec un test >1 MiB.
There was a problem hiding this comment.
Fixed in 7545f1b. Replaced http.MaxBytesReader with a limitRequestBody middleware that reads the body bounded to maxRequestBodyBytes+1, returns 413 REQUEST_TOO_LARGE on overflow, and resets the buffered (≤1 MiB) body so the audit middleware/handlers can still read it. This avoids the audit io.ReadAll → 500 path you flagged. Added TestRequestBodyTooLarge (>1 MiB → 413).
Address review feedback: http.MaxBytesReader surfaced its overflow error through the audit middleware's io.ReadAll, which maps any non-EOF read error to 500 — so a >1 MiB body returned "500 failed to read request body" instead of 413. Replace it with a limitRequestBody middleware that reads the body bounded to maxRequestBodyBytes+1, rejects oversize with 413 (REQUEST_TOO_LARGE), and resets the (bounded) body for the audit middleware and handlers. Adds TestRequestBodyTooLarge (>1 MiB → 413).
2 participants
Problem (High — H3, H4)
parsePageSizecalledpanic(err)on a non-numericpageSize(?pageSize=abc). With no recoverer middleware mounted,net/httpaborted the connection (client reset) instead of returning a 400. Negative and unbounded values were also passed straight to the ledger (resource amplification). Affects all four list endpoints.httpaudit) doesio.ReadAll(r.Body)with no cap and buffers both request and response in memory → memory-exhaustion DoS, triggerable even unauthenticated.Fix
parsePageSizereturns an error instead of panicking; rejects< 1and clamps tomaxPageSize(100). List handlers return 400 VALIDATION on bad input.middleware.Recovererat the top of the chain (safety net for residual panics).http.MaxBytesReader(1 MiB) ahead of the audit middleware.Tests
TestParsePageSize: empty→default, value, clamp>100, and errors onabc/0/-5.TestListHandlerRejectsInvalidPageSize:GET /wallets?pageSize=abc→ 400.From the in-depth repository review.