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: support for bad clients that send literal spaces in the request URI? #17995

Closed
miquels opened this issue Nov 20, 2016 · 4 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@miquels
Copy link

miquels commented Nov 20, 2016

I've written an HTTP server using Go for XBMC/Kodi clients. Kodi is a widely used open-source media player. In fact Kodi uses webdav, so I've implemented this with x/net/webdav, which is built on the standard net/http.

The problem is that Kodi (and its predecessor, XBMC) will send plain spaces in URIs. They're not encoded as %20. This is certainly a bug- but Apache and Lighttpd accept that, as they parse the request line by finding the first space from the left, everything before that is the HTTP Method, and the first space from the right, everything after that is the HTTP Version, and anything in between is the request URI.

net/http/request.go however just looks for the first and second space:

   750	func parseRequestLine(line string) (method, requestURI, proto string, ok bool) {
   751		s1 := strings.Index(line, " ")
   752		s2 := strings.Index(line[s1+1:], " ")
   753		if s1 < 0 || s2 < 0 {
   754			return
   755		}
   756		s2 += s1 + 1
   757		return line[:s1], line[s1+1 : s2], line[s2+1:], true
   758	}

So it doesn't work with XBMC/Kodi clients, and possibly others.

This can be fixed easily with the following patch:

@@ -750,9 +750,8 @@
func parseRequestLine(line string) (method, requestURI, proto string, ok bool) {
        s1 := strings.Index(line, " ")
-       s2 := strings.Index(line[s1+1:], " ")
+      s2 := strings.LastIndex(line, " ")
        if s1 < 0 || s2 < 0 {
                return
        }
-       s2 += s1 + 1
        return line[:s1], line[s1+1 : s2], line[s2+1:], true
 }

Now, ofcourse, it is debatable whether net/http should support such buggy clients. So if you think that it should not, just close this bug report.

Thank you.

Mike.

@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 21, 2016
@quentinmit quentinmit added this to the Go1.9Maybe milestone Nov 21, 2016
@quentinmit
Copy link
Contributor

/cc @bradfitz
Too late for Go 1.8. I suspect you want to keep the substring to make sure that s1 and s2 do not end up with the same value.

@bradfitz
Copy link
Contributor

Which HTTP client library do XBMC/Kodi use? Is there an upstream bug already filed for them to fix their HTTP client?

If so, and this problem is temporary, I might be amenable to accepting this garbage since it fixes a real client, but I won't do it unless upstream is also fixed.

@miquels
Copy link
Author

miquels commented Dec 12, 2016

I've researched this some more, and I found that NGINX had a similar issue with XBMC/Kodi. It was because the nginx webdav module didn't properly escape 'href' in the XML PROPFIND response. XBMC/Kodi then uses that href url unmodified and unescaped.

Digging a bit further I found that golangs x/net/webdav had the exact same problem, but that it since has been fixed by:

x/net/webdav: percent-encode D:href in the XML.
golang/net@55cccaa

Indeed, with a newer x/net/webdav, the bug doesn't happen anymore.

I think you can close this bug now. Thanks.

@bradfitz
Copy link
Contributor

Great, thanks.

@golang golang locked and limited conversation to collaborators Dec 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants