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

x/net/http2: http get with a path starting by `//` started failing in 1.8 #19103

Closed
azr opened this issue Feb 15, 2017 · 13 comments
Closed

x/net/http2: http get with a path starting by `//` started failing in 1.8 #19103

azr opened this issue Feb 15, 2017 · 13 comments
Assignees
Milestone

Comments

@azr
Copy link

@azr azr commented Feb 15, 2017

Howdy, I think go 1.8 has a small breaking change from 1.7 here.

What version of Go are you using (go version)?

1.8

What operating system and processor architecture are you using (go env)?

its running on the Docker image golang:1.8

Digest: sha256:84b74893325b4e81bc871ecaa5317730c64bc807b29b660443af6fdbc34cf94e
Status: Downloaded newer image for golang:1.8
 ---> f98b82c517db

What did you do?

An http request with a path starting by //

package main

import "net/http"

func main() {
	_, err := http.Get(`https://graph.facebook.com//v2.4/oauth/access_token`)
	if err != nil {
		panic(err.Error())
	}
}

What did you expect to see?

Nothing.

What did you see instead?

panic: Get https://graph.facebook.com//v2.4/oauth/access_token: invalid request :path "//v2.4/oauth/access_token"
@azr

This comment has been minimized.

Copy link
Author

@azr azr commented Feb 15, 2017

Same happens with go1.8rc3

@bradfitz bradfitz changed the title Http get with a path starting by `//` started failing in 1.8 x/net/http2: http get with a path starting by `//` started failing in 1.8 Feb 15, 2017
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 15, 2017

I can reproduce:

Go 1.7 http1: okay
Go 1.7 http2: okay
Go 1.8 http1: okay
Go 1.8 http2: broken

This came out because of the fix to #16847:

func validPseudoPath(v string) bool {
       return len(v) > 0 && v[0] == '/' && (len(v) == 1 || v[1] != '/')
}

But perhaps that check is too strict. The URL RFC https://tools.ietf.org/html/rfc3986#section-3.3 says:

When authority is not present, the path cannot begin with two slash characters ("//").

But in this case, there is an authority present, so the double slash I think is fine.

Is this actually affecting you, or is this hypothetical?

How did you discover this?

I wish I would've gotten this report a couple months ago when Go 1.8 testing began. It's probably too late for a Go 1.8 fix at this point. Maybe Go 1.8.1.

/cc @rsc

@bradfitz bradfitz added this to the Go1.8.1 milestone Feb 15, 2017
@bradfitz bradfitz self-assigned this Feb 15, 2017
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 15, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 15, 2017

Facebook doesn't require the double-slash in the URL, right?

At this point I would consider Go 1.8 frozen. We can discuss whether to put this into Go 1.8.1 but I would tend to think not.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Feb 15, 2017

I admit I don't fully understand what's going on. It seems impossible to construct an HTTP2 request when given a URL with Opaque != "". The godoc for URL says:

A URL represents a parsed URL (technically, a URI reference).
The general form represented is:
   scheme://[userinfo@]host/path[?query][#fragment]

URLs that do not start with a slash after the scheme are interpreted as:
   scheme:opaque[?query][#fragment]

That is, URLs with Opaque don't have a host, which means it's impossible to set the :authority field. The original report in #16847 did:

req, _ := http.NewRequest("GET", "https://nginx.0f.io/hello", nil)
req.URL.Opaque = "//nginx.0f.io/hello"

I guess they assumed we'd parse the host from URL.Opaque, even though that's completely undocumented in the godoc for URL? I'm not sure. The fix for #16847 did (paraphrasing):

path = req.URL.RequestURI()
if !validPseudoPath(path) {
	orig := path
	path = strings.TrimPrefix(path, req.URL.Scheme+"://"+req.Host)
	if !validPseudoPath(path) {
		return error
	}
}

This code makes two assumptions. The first assumption: req.Host is set when req.Opaque is set. Nothing in the godoc for URL makes me believe this would be true. The godoc actually has this under the "Opaque" example:

URL: &url.URL{
	Host:   "ignored",  // hmm
	Scheme: "https",
	Opaque: "/%2f/",
}

The second assumption: RequestURI includes the scheme in the output. But the godoc has no mention of the scheme:

RequestURI returns the encoded path?query or opaque?query
string that would be used in an HTTP request for u.

The implementation has:

if strings.HasPrefix(u.Opaque, "//") {
	result = u.Scheme + ":" + u.Opaque
}

tl;dr, based on the godoc for URL, it seems impossible to construct an HTTP2 request from a URL with Opaque != "", however, there appears to be undocumented behavior that intends to make this possible.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 15, 2017

I think we can break anyone setting Opaque at this point. People used Opaque to get literal %2f in their URLs, and we fixed that a while back. We've now removed the Opaque example from the docs, and no supported version of Go requires using Opaque in this way. If googleapi or any other package we know about still does this, it should be fixed ASAP.

What shouldn't break is plain

http.Get(`https://graph.facebook.com//v2.4/oauth/access_token`)

as in the report.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Feb 15, 2017

Just a random drive by comment for @bradfitz: This line here https://go-review.googlesource.com/c/27632/3/http2/transport.go#1006 looks suspicious and from bare observation looks like it will never match

path = strings.TrimPrefix(path, req.URL.Scheme + "://" + host)

because path is already unschemeified and stripped until after the host, since we first got it from req.URL.RequestURI() so "/apath" or "//apath" or "*"

but then the prefix to trim is "http://golang.org".

Perhaps we meant:

path = strings.TrimPrefix(req.URL.String(), req.URL.Scheme + "://" + host + "/")

which would catch the problem and correct the path.

I haven't yet filed a CL yet because I don't fully understand what's going on: I just thought I should first get your nod of approval with the thought process.

@azr

This comment has been minimized.

Copy link
Author

@azr azr commented Feb 15, 2017

@bradfitz some unit tests were crafting the url up there, I just made a branch of our project to see if everything was fine under 1.8, I fixed them very easily but I thought may be this could be of interest.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 5, 2017

Given the lack of activity (=> interest) here I think we should probably kick this to Go 1.9 instead of rushing something into Go 1.8.1.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Apr 5, 2017

Sorry, I had forgotten about this.

Go 1.9 is fine, or I can work on this today. If it's trivial and obviously safe we can consider for Go 1.8.1.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 5, 2017

I think it's too late for Go 1.8.1 for the same reason it was too late for Go 1.8. No need to rush something that isn't bothering most people. If a fix is available and need is demonstrated when we're preparing Go 1.8.2, fine. But for now I'll mark this Go 1.9.

@rsc rsc modified the milestones: Go1.9, Go1.8.1 Apr 5, 2017
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 14, 2017

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

gopherbot pushed a commit to golang/net that referenced this issue Jun 14, 2017
Updates golang/go#19103 (fixes after bundle into std)

Change-Id: I847133e289e210dedf2b89ab529478edc5de11d1
Reviewed-on: https://go-review.googlesource.com/45773
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 14, 2017

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

@gopherbot gopherbot closed this in 8c4bec8 Jun 14, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
Updates golang/go#19103 (fixes after bundle into std)

Change-Id: I847133e289e210dedf2b89ab529478edc5de11d1
Reviewed-on: https://go-review.googlesource.com/45773
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.