pfernie / user_agent Goto Github PK

For consistency, we should use error-chain for CookieError as well.

Might make sense to re-org errors in general into a single set.

There are unwrap() sprinkled throughout the library, mainly for unexpected-correct cases. These should either be handled via Result-style error propagation, or at least converted to expect() to document expected cases.

Don't force a dependency on reqwest in the core crate, rather provide as a separate crate.

There are some extra fields/parsing requirements user_agent uses (access timestamps, etc.), that may not be appropriate for upstreaming. Furthermore, cookie-rs strives to be no allocation where possible, which may be at odds with some of the normalization/canonicalization steps taken in user_agent.

However, maybe moving some fields to a more aptly named CookieMetadata or similar struct, containing a cookie::Cookie might make sense.

Currently, support for public suffix handling is implemented as providing a builder-style CookieStore::with_suffix_list. This is probably fine; in cases where e.g. a user wants to use a cache (psl crate), they may do so outside of user_agent and use the builder method.

For the case of de/serializing a CookieStore, however, should we indicate/enforce that a stored CookieStore previously utilized a public suffix list? That is, they create a CookieStore and specify a public suffix list, and serialize it to disk. Should we then enforce that the deserialization of that CookieStore must be accompanied by a public suffix list again?

Rather than store the provided public suffix list, I think it would make more sense to simply indicate whether the serialized store had a list (bool flag), and change load() to take an Option<publicsuffix::List>. If the deserialized store's list flag is true, and the provided Option<_> is None, return an error.

This variant was originally intended to disambiguate between an empty string Domain: value being specified (undefined behavior) and no Domain attribute being present on the source cookie. Within the store, however, such variants eventually end up being converted to the HostOnly case per the RFC.

Is their utility in retaining this distinction? As it stands, there is logic around this in CookieStore::insert() that basically assumes this is an invalid case (resulting in CookieError::UnspecifiedDomain). It may make sense to simplify by removing this particular variant.

The code makes liberal use of .clone() and similar methods, some of which can probably be avoided.

I wanted to give you a heads up that someone's implemented a publicsuffix crate, and a publicsuffix crate cache, so it might be pretty simple to get this integrated.

I had previously been using reqwest in such a way that I had purely Rust code and didn't need a C compiler and/or openssl.

Now that I've switched to user_agent, this no longer is possible. I think that it may require work in user_agent, cookie_store and publicsuffix, since publicsuffix seems to be the culprit pulling openssl into the fray.

Currently, serde is used for both serialization and to handle some internal cases of dealing with the cookie-rs version of Cookie ("RawCookie"). Once the latter case is no longer required, the serde dependency should most likely become optional.

The spec calls for maintaining created and last accessed timestamps for cookies, which should be used when implementing eviction policies.

reqwest and other clients have support for HTTP redirects. Depending on the control flow, we may or may not be handling such cases properly, and should. We would need to verify what is considered correct behavior here:

• only provide cookies for re-directed domain (I believe this is correct)
• only provide cookies from directed-from domain (?)
• provide cookies for either

Similar questions, when storing cookies resulting from a re-direct.

Hi,

while doing some initial tests, I noticed that cookies being sent in a 302 response are not getting set.
This does not seem to be correct and was quite confusing for me.

Here's a sample test case:

fn main() {
	let mut session = user_agent::ReqwestSession::new(reqwest::Client::new());
	let mut response = session.get("https://httpbin.org/cookies/set/test/123").unwrap();
	assert_eq!(response.url().as_str(), "https://httpbin.org/cookies");
	assert_eq!(response.text().unwrap(), "{\n  \"cookies\": {\n    \"test\": \"123\"\n  }\n}\n")
}

Hello @pfernie! I had some free time, so I wanted to try to push the whole Rust cookie session management story a little down the road. I was wondering if you could put a license on this project. I suggest MIT/Apache 2.0, which is what most of our stuff uses, and would make it easy to have this code be used by hyper/reqwest and etc. But it of course up to you. Thanks!

It would be very useful to have some examples to work from when using user_agent with reqwest, thanks!

The current public suffix support has no test coverage; tests should be introduced. Specifically checking cases such as the domain-attribute being an identical match to the request-uri host being allowed even for public suffixes (see Step 5 of the Storage Model). Also verifying whitespace is handled correctly in values.