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/cookiejar: Domain matching against IP address #12610

Open
sebcat opened this issue Sep 14, 2015 · 6 comments

Comments

Projects
None yet
5 participants
@sebcat
Copy link

commented Sep 14, 2015

Hello,

The following test fails:

package foo 

import (
    "net/http"
    "net/http/cookiejar"
    "net/url"
    "testing"
)

func TestIPDomain(t *testing.T) {
    u, err := url.Parse("http://127.0.0.1/")
    if err != nil {
        t.Fatal(err)
    }   

    jar, err := cookiejar.New(nil)
    if err != nil {
        t.Fatal(err)
    }   

    c := &http.Cookie{Name: "foo", Value: "bar", Domain: "127.0.0.1"}
    jar.SetCookies(u, []*http.Cookie{c})
    cs := jar.Cookies(u)
    if len(cs) != 1 { 
        t.Fatalf("Got %v cookies, expected 1\n", len(cs))
    } else if cs[0].Name != "foo" || cs[0].Value != "bar" {
        t.Fatal("Invalid cookie name/value")
    }   
}

Further inspection shows that it fails with errNoHostname here:

if isIP(host) {
// According to RFC 6265 domain-matching includes not being
// an IP address.
// TODO: This might be relaxed as in common browsers.
return "", false, errNoHostname
}

I believe the comment refers to RFC6265 5.1.3. It states that the domain string must either be identical with the string it's matched against, OR that all of the three subsequently listed conditions are met, one of which are that the string being matched must be a host name.

In the case of the matched string being an IP address and the domain string being the same IP address, the first condition is met. I therefor believe that L447-L452 should be removed.

@vdobler

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2015

The situation is not really simple: RFC 6265 is not clear whether to allow or deny
domain cookies on IP addresses and browsers handle this case differently.
That was the reason for the TODO in line 450 [1] of the cookiejar implementation.
I have the impression that half a decade ago domain cookies on IP addresses
where commonly accepted, sometimes just to match the behavior of Internet Explorer
(see e.g. [2] [3] [4] [10]).

The current implementation in Chromium rejects domain cookies in IP addresses
[5] lines 457-477. If I my understanding of the Mozilla code [6] is correct then
Firefox silently converts a domain cookie for an IP address to a host cookie.
(I have not done tests on any browsers recently.)

My gut feeling is that cookies with an IP address as the domain-attribute should be
either rejected (an IP address is not a domain) or be converted to host cookie (i.e.
pretending the domain-attribute was not set, a forgivable user error).

RFC 6265 is not really clear here. I read the following

The string is a host name (i.e., not an IP address).

RFC 6265 section 5.1.3, second bullet, third star [7] as a definition of host name in
the sense of: "a host name is not an IP address".

Therefore my understanding of the algorithm to produce a canonicalized host name

Convert the host name to a sequence of individual domain name labels.

RFC 6265 section 5.1.2 step 1 [8] is that an IP address does not qualify as a
canonicalized host name.
As an IP address is not a canonicalized host name I think that RFC 6265 section 5.3
step 6 first case [9] applies:

If the canonicalized request-host does not domain-match the domain-attribute:
Ignore the cookie entirely and abort these steps.

and the cookie should be dropped as it is done currently.

[1]

if isIP(host) {
// According to RFC 6265 domain-matching includes not being
// an IP address.
// TODO: This might be relaxed as in common browsers.
return "", false, errNoHostname
}

[2] https://code.google.com/p/chromium/issues/detail?id=3699
[3] https://codereview.chromium.org/18657/diff/1/net/base/cookie_monster_unittest.cc
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=125344
[5] http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_store_unittest.h
[6] https://hg.mozilla.org/mozilla-central/file/3e8dde8f8c17/netwerk/cookie/nsCookieService.cpp#l3502

[7] http://tools.ietf.org/html/rfc6265#section-5.1.3
[8] http://tools.ietf.org/html/rfc6265#section-5.1.2
[9] http://tools.ietf.org/html/rfc6265#section-5.3
[10] https://bugzilla.mozilla.org/show_bug.cgi?id=125344

@sebcat

This comment has been minimized.

Copy link
Author

commented Sep 16, 2015

The Chromium test case you linked as [5] shows that Chromium allows a cookie to be set with a domain attribute if that attribute is an exact match (L470-L472 in the same file).

RFC 6265 is not really clear here. I read the following

The string is a host name (i.e., not an IP address).

RFC 6265 section 5.1.3, second bullet, third star [7] as a definition of host name in
the sense of: "a host name is not an IP address".

The third star is so that suffix matching is only applied on host names. 5.1.3 also states that if the two strings matches each other exactly, they domain-match.

Regarding RFC6265 5.3:

If the canonicalized request-host does not domain-match the domain-attribute:
Ignore the cookie entirely and abort these steps.

If I visit the URL http://127.0.0.1/ in my browser, it is my understanding that "the canonicalized request-host" is "127.0.0.1". If the domain-attribute in the Set-Cookie header is set to 127.0.0.1, the canonicalized request-host domain-matches the domain-attribute under the rules of 5.1.3 (exact match).

@vdobler

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2015

The comment in the Chromium test case is interesting: "This matches IE/Firefox, even though it seems a bit wrong." Maybe that behaviour is wrong.

One more from the Chromium test case: Chromium treats domain=.1.2.3.4 as an illegal domain attribute. But if you follow RFC 6265 (as package cookiejar tries to) you would chop off the leading trail first (see 5.3.2), even before domain matching it. Chromiums behaviour here is not RFC 6265 compliant no matter if RFC 6265 allows domain cookies on IP addresses or not.

As I said RFC 6265 is not clear here.

I think silently converting the domain cookie to a host cookie is okay, but I do not think that domain cookies on IP addresses make sense at all.

Would you have time to test the behaviour on Chrome, FF, IE, Safari and curl? I thinks showing that all these "browsers" handle domain attributes equal to the request IP address the same would add considerable weight to the proposed change.

@sebcat

This comment has been minimized.

Copy link
Author

commented Sep 16, 2015

For the following web service:

package main

import (
    "net/http"
    "log"
)

func CookieHandler(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Set-Cookie", "foo=bar; domain=127.0.0.1")
    w.Write([]byte("foo"))
}

func main() {
    http.HandleFunc("/", CookieHandler)
    log.Fatal(http.ListenAndServe(":8080", nil))
}

curl 7.40.0
Google Chrome 47.0.2503.0 dev
Mozilla Firefox 40.0.3

sets the cookie "foo" for 127.0.0.1

I will be able to test IE, Safari in the beginning of next week.

@sebcat

This comment has been minimized.

Copy link
Author

commented Sep 21, 2015

Safari 9.0 (11601.1.56)
IE 10.0.9200.17492 (Update version 10.0.31, Windows 2012 Server) *

sets the cookie "foo" for 127.0.0.1 for the previously mentioned test case.

*: Added Expires attribute to make sure a persistent cookie file was created in "Temporary Internet Files".

@dcormier

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

This certainly makes it challenging to use httptest.Server and try to set cookies with the Domain attribute set to the httptest.Server's address.

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