Skip to content

net/http: unwrap ResponseWriter in MaxBytesReader for finding requestTooLarger #73754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EricGusmao opened this issue May 17, 2025 · 4 comments · May be fixed by #73893
Open

net/http: unwrap ResponseWriter in MaxBytesReader for finding requestTooLarger #73754

EricGusmao opened this issue May 17, 2025 · 4 comments · May be fixed by #73893
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@EricGusmao
Copy link

EricGusmao commented May 17, 2025

Summary

http.MaxBytesReader in the standard library uses an internal interface (requestTooLarger) on the provided http.ResponseWriter to notify the server when a request body exceeds the specified limit. However, when a custom ResponseWriter wrapper is used, this internal callback is not invoked because the library does not unwrap the writer chain to find an implementation.

This causes the server to miss the "request too large" event, not closing the connection.

Proposal

Modify http.MaxBytesReader to recursively unwrap the given ResponseWriter if it implements an Unwrap() http.ResponseWriter method (similar to how http.ResponseController is handled), and call the internal requestTooLarge() callback on the innermost ResponseWriter that implements it.

This change would maintain backward compatibility, and improve behavior in common scenarios where middleware wraps the writer.

Motivation

When developers wrap http.ResponseWriter to implement middleware features like logging, metrics, or response modification, they typically embed the original ResponseWriter.

The current design of MaxBytesReader checks only the top-level writer for the internal requestTooLarger interface. As a result, an oversized request does not close the connection.

Example and Reproduction Steps

Here is a minimal example that illustrates the problem when wrapping ResponseWriter:

package main

import (
	"io"
	"log"
	"net/http"
)

type myWrapper struct {
	http.ResponseWriter
}

func handler(w http.ResponseWriter, r *http.Request) {
	wrapped := myWrapper{w}

	r.Body = http.MaxBytesReader(wrapped, r.Body, 10)

	body, err := io.ReadAll(r.Body)
	if err != nil {
		log.Println("Read error:", err)
	} else {
		log.Println("Read body:", string(body))
	}

	w.WriteHeader(http.StatusOK)
	w.Write([]byte("done"))
}

func main() {
	http.HandleFunc("/", handler)
	log.Fatal(http.ListenAndServe(":8080", nil))
}

Curl with a request body exceeding the limit doesn't close the connection.

I have a patch ready implementing this fix and can submit a PR upon approval.

@gopherbot gopherbot added this to the Proposal milestone May 17, 2025
@seankhliao seankhliao changed the title proposal: net/http: Support Unwrap() in http.MaxBytesReader for requestTooLarge detection proposal: net/http: support Unwrap() in http.MaxBytesReader for requestTooLarge detection May 17, 2025
@seankhliao seankhliao changed the title proposal: net/http: support Unwrap() in http.MaxBytesReader for requestTooLarge detection proposal: net/http: unwrap ResponseWriter in MaxBytesReader for finding requestTooLarger May 24, 2025
@seankhliao
Copy link
Member

cc @neild, not sure if this needs to be a proposal?

@neild
Copy link
Contributor

neild commented May 27, 2025

This change seems fine to me. I don't think it needs a proposal. This doesn't add any new exported APIs, and while there's a user-visible behavioral change I think the current behavior of not unwrapping the ResponseWriter is closer to a bug than a missing feature.

@neild neild changed the title proposal: net/http: unwrap ResponseWriter in MaxBytesReader for finding requestTooLarger net/http: unwrap ResponseWriter in MaxBytesReader for finding requestTooLarger May 27, 2025
@mknyszek mknyszek modified the milestones: Proposal, Backlog May 27, 2025
@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label May 27, 2025
EricGusmao added a commit to EricGusmao/go that referenced this issue May 27, 2025
…sReader

The current implementation of MaxBytesReader only calls requestTooLarge when the provided ResponseWriter directly implements the internal requestTooLarger interface. This breaks expected behavior when the ResponseWriter is wrapped (e.g., middleware) and the inner writer implements requestTooLarger.

This change adds an unwrapping loop, similar to what http.newResponseController does, to traverse through any Unwrap() chain and properly call requestTooLarge if found.

This improves compatibility with custom or middleware-wrapped writers while preserving current behavior for unwrapped ones.

Fixes golang#73754
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/676676 mentions this issue: net/http: call requestTooLarge on unwrapped ResponseWriter in MaxBytesReader

EricGusmao added a commit to EricGusmao/go that referenced this issue May 27, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/677055 mentions this issue: net/http: test server behavior when hitting a MaxBytesReader limit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants