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

Handle request URI containing query parameters but no path. #1661

Conversation

skidder
Copy link
Contributor

@skidder skidder commented Nov 14, 2023

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.

"If the target URI's path component is empty, the client MUST send "/" as the path within the origin-form of request-target."

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...)
Copy link
Collaborator

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:

h.requestURI = append(h.requestURI[:0], b[:n]...)

Copy link
Contributor Author

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.

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a 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:

h.requestURI = append(h.requestURI[:0], b[:n]...)

@skidder
Copy link
Contributor Author

skidder commented Nov 27, 2023

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.

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 net/http client. I've been able to implement a work-around in my application, but thought this might be worth covering in FastHTTP.

@erikdubbelboer
Copy link
Collaborator

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.

@skidder skidder closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue: Default Path Not Set in FastHTTP Client for URIs with Only Query Parameters
2 participants