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: arbitrary string between port and path in url is silently accepted #14353

Closed
mpl opened this issue Feb 16, 2016 · 10 comments

Comments

Projects
None yet
9 participants
@mpl
Copy link
Contributor

commented Feb 16, 2016

  1. Run a (Go) webserver (on e.g. localhost:8080)

  2. Run this as client:

package main

import (
    "io"
    "log"
    "os"
    "net/http"
)

func main() {
     // works with "http://localhost:8080foobar/valid_path" too
    resp, err := http.Get("http://localhost:8080foobar/")
    if err != nil {
        log.Fatal(err)
    }
    defer resp.Body.Close()
    if _, err := io.Copy(os.Stdout, resp.Body); err != nil {
        log.Fatal(err)
    }
}
  1. Notice how the "foobar" string between the port and the url path does not seem to be a problem.
    I was expecting to get an error with such an URL.

I did not dig any further to see where that leniency comes from, because maybe that's not an actual concern. Let me know and I can look into it.

go version: go1.6beta2 linux/amd64

@bradfitz bradfitz added this to the Go1.7 milestone Feb 16, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 16, 2016

Maybe a dup of or at least similar to #14322 (port "9pfs" looking like "9").

@cespare

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

I didn't look at any relevant specs, but I note that curl behaves the same way.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2016

The description of Get function in net/http package says that "Get issues a GET to the specified URL." So the question here is what's URL? Is that a subset of URIs described in RFC 3986? If so, the port subcomponent of authority must be decimal digits.

port = *DIGIT

Otherwise I have no strong opinions. I'm fine it becomes part of service discovery feature based on DNS or other fancy techniques. In that case it would look like scheme names of URI:

port = *( ALPHA / DIGIT / "+" / "-" / "." )
@msiebuhr

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2016

I've submitted a fix for #14322 over at https://go-review.googlesource.com/#/c/19720/, that seem fix this too, as @bradfitz suggests.

I didn't write in a test for this issue, mostly because I couldn't find any obvious place to add it in the net/http-tests (suggestions welcome). Running it manually after the above is patched gives the error

… Get http://localhost:8080foobar/: dial tcp: lookup tcp/8080foobar: nodename nor servname provided, or not known
exit status 1

For reference, go1.6 returns

… Get http://localhost:8080foobar/: dial tcp [::1]:8080: getsockopt: connection refused
exit status 1
@mpl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2016

You're getting that error even when you have a server listening on :8080 ? I don't.
(I know it's not the main point of that issue. Just saying.)

For reference, go1.6 returns
Get http://localhost:8080foobar/: dial tcp [::1]:8080: getsockopt: connection refused exit status 1

@msiebuhr

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2016

Totally forgot about the server. Stock go 1.6 returns whatever is on /. Returns the above error when the patch is applied.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 21, 2016

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

@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016

@odeke-em

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

@mikioh, @bradfitz or @quentinmit might y'all have thoughts on if it was merging CL https://go-review.googlesource.com/#/c/19720 was what fixed this issue?

I've tested this locally with https://github.com/odeke-em/bugs/tree/master/golang/14353 and we now reject it with

$ go run client.go 
2016/10/05 01:50:55 Get http://localhost:8080foobar/: dial tcp: lookup tcp/8080foobar: nodename nor servname provided, or not known
exit status 1
@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

No, CL 19720 is too late. If we're doing a DNS lookup, we've already lost.

The original fix was https://go-review.googlesource.com/22351 (for #14860) but it was rolled back in https://go-review.googlesource.com/22861

We should move the validation to net/http if we can't move it to net/url.

@quentinmit quentinmit added the NeedsFix label Oct 7, 2016

@bradfitz bradfitz self-assigned this Nov 1, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Nov 1, 2016

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

@gopherbot gopherbot closed this in d89ab13 Nov 1, 2016

@golang golang locked and limited conversation to collaborators Nov 1, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.