net/http: 400 Bad Request not logged #12745
Comments
ianlancetaylor
changed the title
|
Sure. Want to do it? |
|
Yes sure, I'll have a look through the contribution guidelines and see if I can produce a patch for you. My preference would be to add a c.server.logf call for both: 413 Request Entity Too Large and then add the error text to the response body in each case too as above. Does that sound ok as a starting point? |
|
Why don't you start with just the response body/bodies CL (change) first and then do the logging as a follow-up? I'd rather see a number of smaller changes than one containing a bunch of unrelated stuff. This is always true, but especially true when you're first learning the code review system. |
|
OK Will do. Thanks. |
|
I've added this for code review - net/http: add response body to 413 and 400 errors https://go-review.googlesource.com/15018 hopefully I did that correctly. |
|
CL https://golang.org/cl/15018 mentions this issue. |
|
I've added a revision to take in your comments, which adds the new headers you recommended. Since it's a bit more complex now (several lines of headers) and both are similar I've put it into an unexported sendError method, which simplifies the response inside serve() to just c.sendError(StatusBadRequest). |
|
Did the followup ever happened? I'd like to be able to report-up the bad requests, and it seems they are not getting logged through |
|
Looks like it didn't. @kennygrant sent https://golang.org/cl/15018 with the text "Fixes #nnn" (which closed this issue), but the original issue was never addressed. We don't normally reopen issues, but it seems appropriate here. @kennygrant, do you want to continue? |
bradfitz
added
the
NeedsFix
|
@bradfitz yes sure, I'll put in a separate CL logging the errors as well. |
|
I've put up a proposed change to log as well on error in CL 27950 |
|
CL https://golang.org/cl/27950 mentions this issue. |
Currently at net/http/server.go:1339 c.serve() responds to a bad request (e.g. http://golang.org/%L3 ) by responding with 400 Bad request without a body. This results in a blank browser window when not behind a proxy, which is not helpful for end users, and nothing in logs to indicate the cause. Would it be possible to add a simple body to this request with something like:
and/or to insert a line of logging before the response:
so that the server logs all significant errors encountered during this serve() method, as it does for TLS handshake error and panics in the handlers?
The text was updated successfully, but these errors were encountered: