Skip to content
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

net/http: 400 Bad Request not logged #12745

Closed
kennygrant opened this issue Sep 25, 2015 · 12 comments

Comments

Projects
None yet
6 participants
@kennygrant
Copy link
Contributor

commented Sep 25, 2015

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:

io.WriteString(c.rwc, "HTTP/1.1 400 Bad Request\r\n\r\n400 Bad Request")

and/or to insert a line of logging before the response:

c.server.logf("http: 400 Bad Request: %v", err)

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?

@ianlancetaylor ianlancetaylor changed the title 400 Bad Request not logged net/http: 400 Bad Request not logged Sep 25, 2015

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Sep 25, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 27, 2015

Sure. Want to do it?

@kennygrant

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2015

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
400 Bad Request

and then add the error text to the response body in each case too as above. Does that sound ok as a starting point?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 27, 2015

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.

@kennygrant

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2015

OK Will do. Thanks.

@kennygrant

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2015

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.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 27, 2015

CL https://golang.org/cl/15018 mentions this issue.

@kennygrant

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2015

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).

@bradfitz bradfitz closed this in 4a6326e Oct 5, 2015

@dpc

This comment has been minimized.

Copy link

commented Jun 24, 2016

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 http.Server.LogError.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 24, 2016

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 bradfitz reopened this Jun 24, 2016

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Unplanned Jun 24, 2016

@bradfitz bradfitz added the NeedsFix label Jun 24, 2016

@kennygrant

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2016

@bradfitz yes sure, I'll put in a separate CL logging the errors as well.

@kennygrant

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2016

I've put up a proposed change to log as well on error in CL 27950

https://go-review.googlesource.com/#/c/27950/

@gopherbot

This comment has been minimized.

Copy link

commented Aug 26, 2016

CL https://golang.org/cl/27950 mentions this issue.

@rsc rsc modified the milestones: Go1.8, Go1.8Maybe Nov 11, 2016

@gopherbot gopherbot closed this in 84ded8b Nov 11, 2016

@golang golang locked and limited conversation to collaborators Nov 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.