golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.68k stars 17.62k forks source link

net/http: add Cookie.Valid method #46370

Closed nulltrope closed 3 years ago

nulltrope commented 3 years ago

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

$ go version
go version go1.16 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GOARCH="amd64"
GOOS="darwin"

What did you do?

I am attempting to construct an http.Cookie that will be set on an http.Server's response

http.HandleFunc("/foo", func(w http.ResponseWriter, r *http.Request) {
    cookie := &http.Cookie{
        Name:   "my-cookie",
        Value:  "bar",
        Domain: "example.com:8080",
    }
    http.SetCookie(w, cookie)
    fmt.Fprintf(w, "cookie: %s", cookie.String())
})

What did you expect to see?

Being that the http.SetCookie (or more specifically, the cookie.String method) function validates various fields of the cookie struct and returns no error, I would expect there to be a way to validate the cookie myself before attempting to set it.

Unfortunately, these validator functions are all unexported and the only way I can see to achieve this today would be to re-write the validation logic in my own code, which comes with its own set of concerns.

I can see why generally there isn't always a need to validate a cookie yourself before writing, but when dealing with cookies containing lots of dynamic attribute data (e.g. an app implementing oauth/oidc flows) it becomes paramount that the cookies being returned are written as expected.

What did you see instead?

The http.SetCookie function will log to stderr (or not, depending on your logging setup) and silently discard the portion of the cookie that failed validation from the final header.

For myself, this caused the Domain attribute to be dropped from the final cookie in the response. That in turn caused the cookie to not be sent in subsequent requests to a subdomain on the same host, which resulted in an error.

Proposal

Cookies in net/http have a concept of validity, but what constitutes a "Go valid cookie" is deliberately left undocumented.

Additionally, it is quite easy to construct an "invalid" cookie (by Go standards) and have it go unnoticed, due to the (*http.Cookie).String() method silently discarding any invalid fields.

I propose adding a new utility function that allows one to programmatically check a cookie's validity under the same rules used by net/http.

This can be either a validity check method, a serialization method, or both:

// Valid reports whether the cookie is valid.
func (c *Cookie) Valid() error

// Marshal returns the serialization of the cookie for use in a Cookie
// header (if only Name and Value are set) or a Set-Cookie response
// header (if other fields are set).
// If c is nil, the empty string is returned.
// If the cookie is invalid, an error is retuned.
func (c *Cookie) Marshal() (string, error)

Use Cases

  1. When a cookie is populated with dynamic data from an http request or some other external source. Rather than having the (*http.Cookie).String() method just log to stderr and discard fields (which may be entirely swallowed depending on your logging setup) you'd be able check cookie validity beforehand, and handle invalid cookies gracefully by logging your own structured error, incrementing a failure metric, returning a different http response, etc.
http.HandleFunc("/foo", func(w http.ResponseWriter, r *http.Request) {
    cookie := &http.Cookie{
        Name:   "my-cookie",
        Value:  "bar",
        Domain: r.Host,
    }

  err := cookie.Valid()
  if err != nil {
    log.Printf("invalid cookie: %v", err)
    http.Error(w, http.StatusInternalServerError, 500)
    return
  }

    http.SetCookie(w, cookie)
    fmt.Fprintf(w, "cookie: %s", cookie.String())
})
  1. When using statically defined cookie data, a validation check can still be valuable to both assert that the value as you define it is correct, and that the meaning of a "go valid cookie" has not changed.
gopherbot commented 3 years ago

Change https://golang.org/cl/338590 mentions this issue: net/http: add Cookie.Valid method