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: allow checking if http.Hijacker was already Hijacked #16456

Closed
glerchundi opened this issue Jul 21, 2016 · 15 comments

Comments

Projects
None yet
5 participants
@glerchundi
Copy link

commented Jul 21, 2016

1.- What version of Go are you using (go version)?
go version go1.6.3 darwin/amd64
but should happen in all versions which has Hijacker support.

2.- What operating system and processor architecture are you using (go env)?
darwin_amd64

3.- What did you do?
I'm using httprouter package to handle all kind of requests, these include websocket related ones. The router has a panic handler which helps writing useful info based on http.Request, http.ResponseWriter and an interface which defines the panic itself. But this problem is not related to the package, the absence of something to check if the connection is hijacked could derive in server errors due to an hijacked connection was in place and trying to write any kind of http data.

func handler(w http.ResponseWriter, _ *http.Request)
{
  hijacked := false
  if h, ok := w.(http.Hijacker); ok {
    hijacked = h.Hijacked
  }

  if !hijacked {
    http.Error(w, "", http.StatusInternalServerError)
  }
}

4.- What did you expect to see?
Nothing if connection was already Hijacked.

5.- What did you see instead?

2016/07/21 10:25:23 http: response.WriteHeader on hijacked connection
2016/07/21 10:25:23 http: response.Write on hijacked connection

Any chance to accept a PR?

@glerchundi glerchundi changed the title net/http: allow checking if ResponseWriter is already Hijacked net/http: allow checking if http.Hijacker was already Hijacked Jul 21, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

I'm reluctant to add too much new API to the net/http package at this point. What did you have in mind?

@bradfitz bradfitz added this to the Go1.8Maybe milestone Jul 21, 2016

@glerchundi

This comment has been minimized.

Copy link
Author

commented Jul 21, 2016

Thanks for your quick answer.

What about something like this?

[...]

type Hijacker interface {
    Hijack() (net.Conn, *bufio.ReadWriter, error)
    Hijacked() bool
}

[...]

func (w *response) Hijacked() bool {
    return w.conn.hijacked()
}

This should be backward compatible change unless i'm missing something.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

No, you can't enlarge an interface and still be backwards compatible.

See also: http://blog.merovius.de/2015/07/29/backwards-compatibility-in-go.html

@glerchundi

This comment has been minimized.

Copy link
Author

commented Jul 21, 2016

Ups, sorry i missed it.

Ignore what I said, what about another interface (too much?) for the sole purpose of knowing if it's hijacked or not? Please don't take into account the interface name, we can think something that it's more accurate if you feel this can be included.

[...]

type Hijackeder interface {
    Hijacked() bool
}

[...]

func (w *response) Hijacked() bool {
    return w.conn.hijacked()
}
@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

Yeah, any new interface feels like too much. Maybe we can shove it (it == some new func?) away in the httputil package or something. Not sure if that's gross itself, but since it's already a random package, maybe it's okay.

@glerchundi

This comment has been minimized.

Copy link
Author

commented Jul 21, 2016

I'm lost, how to get an internal property value in a subpackage which doesn't have parent package visibility? Or i'm completely wrong?

Exposing a boolean (through method/field which breaks compatibility) seems like I'm choosing which one to cut the red or blue wire :)

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

One option is to declare an unexported helper function in net/http, and then use //go:linkname to make it accessible from net/http/httputil. We use this trick a fair amount in the standard library. That said, it's kind of a hack, so I'd only resort to that if necessary.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

@mdempsky, I don't think we'd even need that trick. Both net/http and net/http/httputil could depend on an shared internal package and pass around the func through that.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

Actually, something else we could do is just not spew to the server log if lenData == 0, but still return ErrHijacked like we do today:

func (w *response) Write(data []byte) (n int, err error) {
        return w.write(len(data), data, "")
}

func (w *response) WriteString(data string) (n int, err error) {
        return w.write(len(data), nil, data)
}

// either dataB or dataS is non-zero.    
func (w *response) write(lenData int, dataB []byte, dataS string) (n int, err error) {
        if w.conn.hijacked() {
                w.conn.server.logf("http: response.Write on hijacked connection")
                return 0, ErrHijacked
        }

Changing the code to be:

func (w *response) write(lenData int, dataB []byte, dataS string) (n int, err error) {
        if w.conn.hijacked() {
                if lenData > 0 {
                        w.conn.server.logf("http: response.Write on hijacked connection")
                }
                return 0, ErrHijacked
        }

Then you just do a Write(nil) and check the error value, but we still spam the logs in the majority of cases for people making actual coding mistakes, which is why the log was there in the first place.

No new API.

@glerchundi

This comment has been minimized.

Copy link
Author

commented Jul 22, 2016

Although it's a little bit obscure, I think that brad's approach it's the best one, at least for the 1.X branch.

I don't mind to create a patch with your solution, do you want me to proceed?

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

ping @bradfitz

@gopherbot

This comment has been minimized.

Copy link

commented Oct 11, 2016

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

Done. I was leaving the easy ones to the end. @quentinmit, want to review?

@glerchundi

This comment has been minimized.

Copy link
Author

commented Oct 11, 2016

thanks @bradfitz 👍

@gopherbot gopherbot closed this in abbd502 Oct 13, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Oct 16, 2016

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

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.