-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Handle request URI containing query parameters but no path. #1661
Handle request URI containing query parameters but no path. #1661
Conversation
header.go
Outdated
@@ -681,6 +681,8 @@ func (h *RequestHeader) RequestURI() []byte { | |||
requestURI := h.requestURI | |||
if len(requestURI) == 0 { | |||
requestURI = strSlash | |||
} else if requestURI[0] == '?' { | |||
requestURI = append(strSlash, requestURI...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would cause an allocation, which we always try to avoid in fasthttp. I'm wondering if it isn't better to add the /
here when needed:
Line 2815 in 699af40
h.requestURI = append(h.requestURI[:0], b[:n]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, great point. I've updated the change to modify the request URI when set through either the SetRequestURI
or SetRequestURIBytes
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry but I don't think SetRequestURI
and SetRequestURIBytes
need to be fixed. Users have control over these and can append the /
themselves.
Your issue was with clients sending a GET ?foo=bar
request. In that case this is not going to fix it. You need to fix it here in the request parsing code:
Line 2815 in 699af40
h.requestURI = append(h.requestURI[:0], b[:n]...) |
Ah, I see. I'm only using the FastHTTP client, not the FastHTTP server. My expectation was that the FastHTTP client would handle URLs using this format since that's the behavior of the built-in |
I'm a little bit afraid to make backwards breaking changes like this. I'll leave this pull request open for now and see if anyone else runs into this. |
Fixes #1659
When returning a request URI, check whether it begins with a query (i.e.
?
), in which case we should apply a default path of/
). This is required to conform to the HTTP 1.1 specification.