vibe.d icon indicating copy to clipboard operation
vibe.d copied to clipboard

Cookies not working client-side ?

Open Geod24 opened this issue 10 years ago • 39 comments

It seems that when receiving a response, "the cookies" property of an HTTPClientResponse is not populated. Here's an example:

import vibe.vibe;
void main()
{
    setLogLevel(LogLevel.verbose4);
    requestHTTP("http://www.google.com",
                (scope req) { req.method = HTTPMethod.GET; logInfo("%s", req); },
                (scope res) { logInfo("Here are the cookies :"); foreach(v, k; res.cookies) logInfo("%s: %s", k, v); });
}

Output (beginning stripped for clarity):

[...]
evbuffer_read 2 bytes (fd 11)
 .. got 2 bytes
read data
---------------------
HTTP client response:
---------------------
HTTP/1.1 200 OK
Date: Thu, 08 May 2014 09:42:50 GMT
Expires: -1
Cache-Control: private, max-age=0
Content-Type: text/html; charset=ISO-8859-1
Set-Cookie: PREF=ID=3d9141136c1c76fc:FF=0:TM=1399542170:LM=1399542170:S=kETqNOhXHlbP98Fl; expires=Sat, 07-May-2016 09:42:50 GMT; path=/; domain=.google.com
Set-Cookie: NID=67=BZ19WwTE5SwH-bq08ia1Cp85rocksGbp8GFJ58DAVKwv15otHpnLWKEuu2wtCZsCl0OPnLeavhdn6_87O5VuL2SGJl7LpOB_4hYdmqtJgbEmF7BZf2Q-7IHpTtsJMLyu; expires=Fri, 07-Nov-2014 09:42:50 GMT; path=/; domain=.google.com; HttpOnly
P3P: CP="This is not a P3P policy! See http://www.google.com/support/accounts/bin/answer.py?hl=en&answer=151657 for more info."
Server: gws
X-XSS-Protection: 1; mode=block
X-Frame-Options: SAMEORIGIN
Alternate-Protocol: 80:quic
Transfer-Encoding: chunked
---------------------
Here are the cookies :
Failed to handle the complete response of the server - disconnecting.
bufferevent_flush
Closing socket 11...
...socket 11 closed.

I took a quick look on the client side and couldn't see any mention of "Set-Cookie" anywhere. So is that a bug, unimplemented, or am I doing something wrong ?

Geod24 avatar May 08 '14 09:05 Geod24

I'd say somewhere between bug and unimplemented ;) Would also be nice to have a client-side cookie store implementation some day.

s-ludwig avatar May 08 '14 11:05 s-ludwig

I think an array would be necessary because two cookie names can co-exist if it's for a different domain, or even within the same domain depending on if it's a secured cookie or a different path.

tbh I would just look at how Qt does it ;)

etcimon avatar May 08 '14 13:05 etcimon

It's also missing a multi-part form writer I think?

etcimon avatar May 08 '14 13:05 etcimon

Yes and yes. For the cookies, the vibe.http.common.CookieValueMap would be the appropriate container. And for that matter, CookieValueMap would today be implemented in terms of an alias to DictionaryList.

I think HTTPResponse.cookies can just be changed to CookieValueMap, since the interface should be more or less compatible to an AA.

s-ludwig avatar May 08 '14 15:05 s-ludwig

I was thinking of something more in line with this:

struct CookieJar { 
    Array!Cookie cookies; 
    bool insert(string setCookieStr); // Takes the value part from Set-Cookie: [...]
    bool insert(Cookie cookie);
    string keyValueFor(string URL);
    @property Cookie[] all();
    Cookie[] getFor(string URL);
}

The cookies can then be serialized for storage separately.

etcimon avatar May 08 '14 15:05 etcimon

Such API extensions are fine, CookieValueMap could be extended appropriately.

s-ludwig avatar May 08 '14 15:05 s-ludwig

How would the CookieValueMap know which Cookies it needs to replace or expire?

etcimon avatar May 08 '14 15:05 etcimon

It wouldn't. It's just a representation of what is transferred in the request/response. The actual local cookie storage would be handled by a different entity (I guess this is what CookieJar is supposed to do?).

s-ludwig avatar May 08 '14 15:05 s-ludwig

So CookieValueMap is a single Cookie from the HTTPClientResponse's Set-Cookie commands ?

etcimon avatar May 08 '14 15:05 etcimon

No, it contains all cookies that are set using Set-Cookie, or it contains all cookies that were sent in a Cookie header, depending on the direction.

s-ludwig avatar May 08 '14 15:05 s-ludwig

Isn't a DictionaryList a string string map? So it would map Cookie name and string values?

How would this fit in the DictionaryList for example: Set-Cookie: PREF=ID=3d9141136c1c76fc:FF=0:TM=1399542170:LM=1399542170:S=kETqNOhXHlbP98Fl; expires=Sat, 07-May-2016 09:42:50 GMT; path=/; domain=.google.com Set-Cookie: NID=67=BZ19WwTE5SwH-bq08ia1Cp85rocksGbp8GFJ58DAVKwv15otHpnLWKEuu2wtCZsCl0OPnLeavhdn6_87O5VuL2SGJl7LpOB_4hYdmqtJgbEmF7BZf2Q-7IHpTtsJMLyu; expires=Fri, 07-Nov-2014 09:42:50 GMT; path=/; domain=.google.com; HttpOnly

etcimon avatar May 08 '14 15:05 etcimon

You probably didn't refer to the type of HTTPResponse.cookies (Cookie[string]), right? That's what I thought.

s-ludwig avatar May 08 '14 15:05 s-ludwig

DictionaryList conceptually maps from string to T[], where T would be Cookie in this case.

s-ludwig avatar May 08 '14 15:05 s-ludwig

Ok! I was assuming it would stay string string[] to map the fields of a cookie like the InetHeaders. So the CookieJar would be handled outside of HTTPRequest / HTTPResponse?

etcimon avatar May 08 '14 15:05 etcimon

Yes, I'd say it should be a class and be a member of HTTPClient, so that it can set the appropriate Cookie headers automatically. And there should be a global setting to set a default cookie store that is used for the HTTP clients that are automatically created for requestHTTP.

BTW, "store" vs. "jar" - my preference would be "store", is "jar" some kind of standard jargon?

s-ludwig avatar May 08 '14 15:05 s-ludwig

Excuse the unwanted pun in "jar"gon ;)

s-ludwig avatar May 08 '14 16:05 s-ludwig

BTW, "store" vs. "jar" - my preference would be "store", is "jar" some kind of standard jargon?

Yes, it's very widespread, I saw it in every framework I came across ;)

Yes, I'd say it should be a class and be a member of HTTPClient, so that it can set the appropriate Cookie headers automatically

I was going to say that the CookieJar should be embedded like the SessionStore as an interface, but I feel like that would prevent templating and thus the serialization problem we talked about earlier. I'm tempted to think that there should be a void delegate(HTTPClientResponse) cookiesEvent instead, so that you can make a struct CookieJar(serialization, storage){ } template and just pass the pointer to the callback handler... For a templated SessionStore, I've worked around by fetching the SessionID but it would be nice to have a callback for when the new session ID is available or when it's ended... What do you think?

etcimon avatar May 08 '14 16:05 etcimon

For cookies, there shouldn't be a serialization problem, because they are represented as string data anyway, so that serialization can be layered on top, if desired. But yeah, there still needs to be a proper solution for the SessionStore (I need some time until I can really get to that).

Still not sure if I find the cookie jar class name funny or silly, but if everyone names it like that...

s-ludwig avatar May 08 '14 18:05 s-ludwig

Still not sure if I find the cookie jar class name funny or silly, but if everyone names it like that...

heh :-p

But yeah, there still needs to be a proper solution for the SessionStore (I need some time until I can really get to that).

I'd vote to be able to attach a more flexible interface (loadSession & createSession only) called something like SessionStoreAlt

For cookies, there shouldn't be a serialization problem, because they are represented as string data anyway

I know for a fact that Youtube, like many websites, uses objects in their cookies, in this case the objects/arrays are called PREF and 7rpej.resume:

PREF=f5=230&al=en+fr-CA&fv=13.0.0&f1=50000000; 7rpej.resume=8TV9ZZteYEU:1153,73XNtI0w7jA:2198,atM3ZhF8MVs:10714;

This is somewhat of a commonly used technique to change Javascript/Flash settings. Being careful about the design, this could allow syntax such as res.cookies.PREF.f5 = 230 (maybe through UFCS by importing the cookie store as a module?)

etcimon avatar May 08 '14 20:05 etcimon

But yeah, there still needs to be a proper solution for the SessionStore (I need some time until I can really get to that).

I'd vote to be able to attach a more flexible interface (loadSession & createSession only) called something like SessionStoreAlt

If you mean that the core session functionality only keeps track of the session itself and not the data contained in it, then I think this is the right way to go. There could be a default implementation that provides the current functionality, and any user defined storage solution can be bolted on at will.

For cookies, there shouldn't be a serialization problem, because they are represented as string data anyway

I know for a fact that Youtube, like many websites, uses objects in their cookies, in this case the objects/arrays are called PREF and 7rpej.resume:

PREF=f5=230&al=en+fr-CA&fv=13.0.0&f1=50000000; 7rpej.resume=8TV9ZZteYEU:1153,73XNtI0w7jA:2198,atM3ZhF8MVs:10714;

This is somewhat of a commonly used technique to change Javascript/Flash settings. Being careful about the design, this could allow syntax such as res.cookies.PREF.f5 = 230 (maybe through UFCS by importing the cookie store as a module?)

Yeah, the UFCS solution should work nicely.

Regarding serialization, the point is that the underlying storage format is always a string, so that this doesn't have to be templated. The SessionStore on the other hand has the problem that it may be more efficient to use another serialized storage format (BSON, JSON, Variant etc.).

s-ludwig avatar May 08 '14 20:05 s-ludwig

Regarding serialization, the point is that the underlying storage format is always a string, so that this doesn't have to be templated. The SessionStore on the other hand has the problem that it may be more efficient to use another serialized storage format (BSON, JSON, Variant etc.).

Yes, both would have the same problem if the idea is to keep a local copy that isn't simply a key-value pair of strings. While it's required for a session store, copying cookies locally would be more of a replication scheme as to always ensure a user has the same cookies for his corresponding session, across computers.

A good example would be with a extjs user interface, all the UI element settings (element size, which page, etc.) persist through cookies but opening the session on another computer would lose this personalized interface without cookie replication possibly in a relational database (which also allows some analytics). Meh, I'd say it's always best not to block it by design..

etcimon avatar May 08 '14 20:05 etcimon

I think we have some kind of misunderstanding there. Cookies are always strings on the wire, so the core API can safely use that as the common denominator. Any serialization is then layered on top of that.

There is no such intrinsic common denominator for storing session values.

s-ludwig avatar May 08 '14 20:05 s-ludwig

Cookies are always strings on the wire, so the core API can safely use that as the common denominator.

I'm not sure what it means. Does this mean we'd have an interface that looks like

class CookieStore {
    DicitionaryList!Cookie cookies;

    void set(string key, string value);
    ...

}

And adding cookies from the API would happen through res.cookieStore.set("key", "value")?

I'm just wondering because for this too, it would be nice to be able to push objects in there, and see them saved to a database at the same time, like UserParameters myParams; res.cookieStore.set("PARAMS", myParams);

etcimon avatar May 08 '14 21:05 etcimon

Basically, yes, the cookie's value would be a string. Something like the proposed database mirroring could happen in a system layered on top. It would call the database and it would set the proper cookies as strings. But the core cookie handling system doesn't have to know about objects or serialization.

s-ludwig avatar May 08 '14 21:05 s-ludwig

It would call the database and it would set the proper cookies as strings.

So the type info would have to be discarded if CookieStore calls the database mirroring, unless serialization happens within CookieStore, it's the same problem.

etcimon avatar May 08 '14 21:05 etcimon

CookieStore wouldn't call the database, the layered system would do that. How it preserves type information is up to that system. A possible implementation could be (very roughly) like this:

class MyCustomCookieStore(COOKIES...) {
    this(CookieStore cookie_store, MongoCollection db)
    {
        // ...
    }

    // generated:
    void setPrefs(Prefs prefs)
    {
        m_cookies.set("PREFS", prefs.serializeToJsonString());
        m_db.update("PREFS", prefs.serializeToBson());
    }
}

auto store = new MyCookieStore!(Prefs, Widgets)(httpclient.cookieStore);
store.setPrefs(Prefs(1, 2));

@cookieName("PREF")
struct Prefs {
    int somePref;
    int someOtherPref;
}

@cookieName("WIDGETS")
struct Widgets {
    int[] widgetPositions;
}

So the type information is statically encoded in the MyCustomCookieStore class, which is used by the application, and CookieStore still only knows about strings.

s-ludwig avatar May 08 '14 21:05 s-ludwig

Oh very nice design. Yes, you're right about that, it's not as necessary as I thought

etcimon avatar May 08 '14 21:05 etcimon

@Geod2, in your commit you define a new CookieStore struct, but I'm pretty sure that DictionaryList!Cookie would already be the right choice here and should be compatible enough with Cookie[string] that it should be ok to just replace it. CookieStore aka CookieJar would be a higher level construct to actually store cookies between requests.

s-ludwig avatar May 16 '14 19:05 s-ludwig

Didn't know they were visible from here, well played Github !

The only things CookieStore adds over DictionaryList!Cookie is a nicer replacement (adding a Cookie with a matching domain / path / name will replace the first one). It also prevent user from sending cookies that would override each other, such as:

Cookie: Name=value; Domain=vibed.org; Path=/
Cookie: Name=newValue; Domain=.vibed.org; Path=/

This should be interpreted in the client as:

Cookie: Name=newValue; Domain=.vibed.org; Path=/

But I agree that CookieStore needs a modification. So I can either:

  • rename it, and add CookieJar / CookieStore;
  • remove it, and just handle it on the client side;

Geod24 avatar May 16 '14 23:05 Geod24

Is there any reason these changes haven't been merged? What needs to be fixed still? Just modifying the code to use DictionaryList!Cookie instead of CookieStore?

MatthewCochrane avatar Apr 29 '15 12:04 MatthewCochrane