allow the expect 100 continue workflow to deny requests #787
Conversation
| if s.DenyRequest != nil { | ||
| if deniedRequest = s.DenyRequest(&ctx.Request.Header); deniedRequest { | ||
| if br != nil { | ||
| br.Reset(ctx.c) |
There was a problem hiding this comment.
@erikdubbelboer After debugging the strange relationship between the headers and request bodies, this ended up being the "correct" way to reproduce whatever is currently happening inside of serveConn.
Someone more familiar with the code definitely needs to weigh in on this piece here.
There was a problem hiding this comment.
Is this really needed? Shouldn't the reader be empty at this point?
There was a problem hiding this comment.
From what I have tested it is 100% needed. If not the loop will come back around, attempt a call on if err = ctx.Request.Header.Read(br); err == nil { and fail with server_test.go:1705: Unexpected error from serveConn: error when reading request headers: EOF.
I believe this is because br is not nil so
br = acquireReader(ctx)
Does not get called, which means it's already in a place somewhere but hasn't been fully read or reset. The function that would normally change it's position or use it later on ctx.Request.ContinueReadBody hasn't been called in this case because the request was abandoned.
At least that's what I think?
| br.Reset(ctx.c) | ||
| } | ||
|
|
||
| ctx.SetStatusCode(StatusExpectationFailed) |
There was a problem hiding this comment.
This seemed to be the most reasonable and clean way to allow setting the response code without calling flush etc, and letting the connection continue as it would have if someone's handler been called and written the same status code.
server.go
Outdated
| //https://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.2.3 | ||
| //https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.1.1 | ||
| //Using DenyRequest a server can make decisioning on whether or not | ||
| //to read a potentially large request body based on the headers |
There was a problem hiding this comment.
There are spaces missing after the //, in many other places as well.
server.go
Outdated
| @@ -172,6 +172,17 @@ type Server struct { | |||
| // non zero RequestConfig field values will overwrite the default configs | |||
| HeaderReceived func(header *RequestHeader) RequestConfig | |||
|
|
|||
| // DenyRequest is called after receiving the Expect 100 Continue Header | |||
There was a problem hiding this comment.
I think DenyRequest is not a clear name for this. Maybe just call it ContinueHandler?
| if s.DenyRequest != nil { | ||
| if deniedRequest = s.DenyRequest(&ctx.Request.Header); deniedRequest { | ||
| if br != nil { | ||
| br.Reset(ctx.c) |
There was a problem hiding this comment.
Is this really needed? Shouldn't the reader be empty at this point?
|
Thanks! |
|
👍 Happy to help. |
Fixes #783
An okay solution right now which allows servers to deny reading the request body after reading the request headers via a server callback configuration.
A more long term solution later on might be to give full control to the user during their handler callback, but that would definitely require a lot more work and be more error prone for the user.
One unfortunate issue with this is, if you do any major work in processing headers before accepting or denying a request, you likely will have to re-process the headers on the other side. It might be worth sticking a
callback out of the deny which can be passed to a new handler, so people can pass state between the two.