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: drop spaces in SetCookie values #3033

Closed
gopherbot opened this issue Feb 15, 2012 · 16 comments
Closed

net/http: drop spaces in SetCookie values #3033

gopherbot opened this issue Feb 15, 2012 · 16 comments
Milestone

Comments

@gopherbot
Copy link

by frel8817:

What steps will reproduce the problem?

1. Set a cookie in a http-handler.
 
expiration := time.Now()
expiration.AddDate(1, 0, 0)
cookie := http.Cookie{Name: "browserID", Value: "my value", Expires:
expiration}
http.SetCookie(w, &cookie) 

2. Read the cookie.

cookie, err := r.Cookie("browserID")
if err != nil {
    fmt.Fprintf(w, err.Error())
    return
}
fmt.Fprintf(w, cookie.Name) 



What is the expected output?

browserID



What do you see instead?

nothing



Which compiler are you using (5g, 6g, 8g, gccgo)?

go_appengine_sdk_linux_amd64-go1beta2



Which operating system are you using?

Linux Mint 11 x64



Please provide any additional information below.

See discussion,
https://groups.google.com/forum/?fromgroups#!topic/golang-nuts/UNzkKcQxC9Y
@bradfitz
Copy link
Contributor

Comment 1:

Your expiration value is wrong.
AddDate doesn't mutate its receiver; it returns a new time.

Status changed to Invalid.

@gopherbot
Copy link
Author

Comment 2 by frel8817:

Sorry, my mistake. Replace first two lines with:
expiration := time.Now().AddDate(1, 0, 0)

@rsc
Copy link
Contributor

rsc commented Feb 16, 2012

Comment 3:

Don't use cookies with spaces in them.
Your example works fine if you use "myvalue" instead of "my value".
The Cookie parser rejects cookie values with spaces in them.
This might or might not be a mistake
SetCookie blindly sends cookies with spaces in them,
which also might or might not be a mistake.
This can probably wait until after Go 1.
Full test program:
package main
import (
    "fmt"
    "net/http"
    "log"
    "time"
)
func main() {
    http.HandleFunc("/", setcookie)
    http.HandleFunc("/readcookie", readcookie)
    log.Fatal(http.ListenAndServe("127.0.0.1:8080", nil))
}
func setcookie(w http.ResponseWriter, req *http.Request) {
    expiration := time.Now().AddDate(1, 0, 0)
    cookie := http.Cookie{Name: "foo", Value: "myvalue", Expires: expiration}
    http.SetCookie(w, &cookie)
    http.Redirect(w, req, "/readcookie", 307)
}
func readcookie(w http.ResponseWriter, req *http.Request) {
    c, err := req.Cookie("foo")
    fmt.Fprintf(w, "foo: %v %v\n", c, err)
    fmt.Fprintf(w, "all:\n")
    for _, c := range req.Cookies() {
        fmt.Fprintf(w, "\t%s\n", c)
    }
}

Labels changed: added priority-later, removed priority-triage.

Owner changed to builder@golang.org.

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented Sep 12, 2012

Comment 4:

Labels changed: added go1.1maybe.

@vdobler
Copy link
Contributor

vdobler commented Oct 9, 2012

Comment 5:

According to RFC 6265 spaces are not allowed in the
value of a cookie (not even if double-quoted). 
See http://tools.ietf.org/html/rfc6265#section-4.1.1
Unfortunately RFC 2109 did allow spaces in
double quoted values. 
RFC 6265 is pretty clear that the cookie-value is
not a string but just some octets.  Section 4.1.1
states that values should be encoded and suggests
Base64. See also note at end of section 5.4.
As SetCookie does not return an error its possibilities
on an invalid cookie value are limited:
 - set the given (invalid) value (current behaviour)
 - ignore the cookie
 - panic 
 - encode the value
As it is called SetCookie and not MustSetCookie panic is
not an option.  Just ignoring it without any feedback
is unpolite.  Encoding the value is a hairy issue as
there is no single right way to do it.
Go typically is very explicit, so an implicit, automatic
encoding in SetCookie if the value contains illegal 
characters feels strange. Additionally it adds the 
overhead of deciding whether to encode or not to any
call.
I'd suggest a better documentation of SetCookie with 
an example of how to manually encode the cookie value
beforehand.

@rsc
Copy link
Contributor

rsc commented Jan 6, 2013

Comment 6:

Issue #4613 has been merged into this issue.

@rsc
Copy link
Contributor

rsc commented Mar 12, 2013

Comment 7:

[The time for maybe has passed.]

Labels changed: removed go1.1maybe.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 8:

Of the various bad choices, eliding spaces seems best.

Labels changed: added go1.2.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 9:

Labels changed: added feature.

@bradfitz
Copy link
Contributor

Comment 10:

Was logging + ignoring considered? That's not without precedent in the http package.

@rsc
Copy link
Contributor

rsc commented Jul 31, 2013

Comment 11:

Can you point to one? I looked and found some log + die but no log + ignore.

@bradfitz
Copy link
Contributor

Comment 12:

func (w *response) WriteHeader(code int) {
    if w.conn.hijacked() {
                log.Print("http: response.WriteHeader on hijacked connection")
                return
        }
        if w.wroteHeader {
                log.Print("http: multiple response.WriteHeader calls")
                return
        }

@rsc
Copy link
Contributor

rsc commented Jul 31, 2013

Comment 13:

Dunno how I missed those. Okay log.Print + drop (ignore) spaces, then?
I don't think we should omit the cookie entirely: I think it will be easier
for people to debug if we're sending _something_. Rewriting spaces to
underscores is fine too. I thought about rewriting to ␣ but I bet UTF-8 is
not allowed in cookie values.

@bradfitz
Copy link
Contributor

Comment 14:

https://golang.org/cl/12204043

Status changed to Started.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 1, 2013

Comment 15:

This issue was closed by revision 17d803d.

Status changed to Fixed.

@rsc
Copy link
Contributor

rsc commented Aug 2, 2013

Comment 16:

Issue #5999 has been merged into this issue.

This was referenced Dec 8, 2014
@rsc rsc added this to the Go1.2 milestone Apr 14, 2015
@rsc rsc removed the go1.2 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants