Cookie builder doesn't ignore leading dots (as the `FromStr` implementation does) about cookie-rs HOT 4 CLOSED

The RFC section that you link to describes how the domain field should be parsed in a SET-COOKIE header. This corresponds to the FromStr implementation of Cookie, i.e, the parser. The Cookie::set_domain() and corresponding CookieBuilder::domain() methods are not part of the parser, so the linked RFC section is not applicable. All this to say: there isn't an obvious logic bug here.

The question is whether the set_domain() method (and CookieBuilder::domain()) should encode the same logic as the parser, that is, strip the leading .. Whether they do or don't should specified more accurately in the methods' documentation. Currently, the docs say simply: "sets the domain attribute of the cookie".

I'm not certain if we should or shouldn't. On the one hand, these and all other setters are working in the "raw", validating little and setting the parameters as they're passed in. If you set .foo.com, you get .foo.com. On the other hand, at least one person (you) expected different behavior.

One major issue with removing a prefix of . when set via .domain() is that round-tripping no longer works as expected. That is, this would fail:

let c1 = Cookie::parse("name=value; domain=..foo.com").unwrap();
let c2 = Cookie::build(c1.name(), c1.value()).domain(c1.domain()).build();
assert_eq!(c1, c2);

On the other hand, it might be surprising that this fails, which appears to be the basis of this report:

let c1 = Cookie::parse("name=value; domain=..foo.com").unwrap();
let c2 = Cookie::build(c1.name(), c1.value()).domain("..foo.com").build();
assert_eq!(c1, c2);

Ideally, both of these cases would pass, somehow.

One idea is to always store the raw value, with the leading ., if any. From .domain(), return a Domain<'_> or &Domain proxy that contains the raw string, accessible via a method, but otherwise compares ignoring the leading . (and case insensitively). Implementations of From<&str> for Domain<'_> and Deref<Target=&str> for Domain would keep most code working while resolving this issue and allowing both examples above to work as expected.

How does this proposal sound? Would love your thoughts as well, @pfernie.

from cookie-rs.

@SergioBenitez: So, I'm not remotely an expert when it comes to cookies or web standards in general (I barely managed to add cookie support to my program, using pre-existing libraries), so I'd defer to anything you decide in the end.

I'd like to mention a few things, however:

(And I hope this doesn't come off as criticism, or as "harsh". That is not what I intend)

# 1: Cookies are used to implement authentication, and are therefore a pretty sensitive topic

# 2: I believe it wouldn't be reasonable to expect that end-users know everything about cookies, or their history

# 3: Differences in the way data is interpreted by a frontend and backend server is a common source of security vulnerabilities. For example, there were numerous security vulnerabilities during the past few years, where a reverse proxy did parse URLs or HTTP headers differently than the backend server.

Therefore, I believe that cookies should be handled consistently within cookie, and cookie should do generally what other software is doing (i.e., browsers and servers).

I can't come up with an exploit for the current behavior, but A) I'm not a professional hacker, and B) it's probably better to be safe now, than sorry later (see # 1).

The question is whether the set_domain() method (and CookieBuilder::domain()) should encode the same logic as the parser, that is, strip the leading ..

I'm not certain if we should or shouldn't. On the one hand, these and all other setters are working in the "raw", validating little and setting the parameters as they're passed in.

I believe, encoding the same logic would be the right thing to do (see # 1, # 2, and # 3 above). Adding a SafeCookieBuilder (if breaking changes are unacceptable) would be the next-best thing.

One major issue with removing a prefix of . when set via .domain() is that round-tripping no longer works as expected. That is, this would fail:

This would fail because of the double dots, of which the first one would be stripped?

One idea is to always store the raw value, with the leading ., if any.

Yes, that seems to be the most sensitive way to do it, and appears do be what browsers do.

The cookie that lead to the discovery of the behavior described in this issue originated from Chrome, extracted via Playwright in the form of JSON (from a website that probably wasn't updated in 15 years. Hence, I guess, the leading dot in the domain field).

This appears to prove, that browsers store cookies in their raw state. Otherwise, Playwright wouldn't return a cookie with a leading dot in the domain field.

from cookie-rs.

Documenting the "raw" nature of the builder API to clarify makes sense (and perhaps suggesting using the FromStr impl when parsing is desired). Personally, without the documentation I would also expect the behavior that the RFC parsing rules are in effect both in the FromStr and builder methods of construction. That is, I would expect that taking the attribute fields of a cookie seen on the wire and passing them to the builder API would end up with the same cookie as if the full cookie string had been parsed (as in the example in the opening post). But the idea that the role of the builder API is to give that raw access makes sense, too.

Changing the internal representation (to retain the original raw value) does indeed align the behaviors, but it seems to me this would be a change of behavior from the current API. That is, the "RFC conformance"/parsing behavior would move from the creation (FromStr or builder) to the domain() accessor (via the proposed Domain), so code which previously used the builder API expecting the current (non-stripping behavior would encounter a change (domain() would enforce the .-removal). Therefore, with the internal representation change, to retain/restore current behavior when utilizing the constructed Cookie, one would have to call set_domain with ..foo.com (or be aware to access the "raw value" of the proposed Domain struct). Which is perhaps fine, and the change in function signature would make the change obvious. But, if the intent of the builder API is to provide a means to directly set cookie values ("the ultimate value of the domain of the cookie should be X, regardless of parsing"), then I think actually documenting current behavior and not modifying the domain() behavior makes more sense. I would suggest if taking this route that in addition to clarifying documentation, add a set_domain_parsed (parse_domain?) method to the builder API. And/or, rename set_domain to set_domain_raw. And reference each from the other's documentation for discoverability.

from cookie-rs.

add a set_domain_parsed (parse_domain?) method to the builder API. And/or, rename set_domain to set_domain_raw. And reference each from the other's documentation for discoverability.

Adding set_domain_parsed and set_domain_raw and deprecating set_domain looks like a reasonable solution, and would probably involve minimal changes.

Are there other cookie fields where such behavior makes sense?

from cookie-rs.