Session module for managing session cookies

[lhft]

The new module, can be used to create a secure session cookie and to check it on every request. The functions have been implemented following the "A Secure Cookie Protocol" paper that can be found here http://www.cse.msu.edu/~alexliu/publications/Cookie/cookie.pdf

[doubleyou]

Overall, I like the addition. We have something similar to this, but specifically for Django cookie sessions.

Here are some notes:

1. I'm not sure separating username and extra data is reasonable.
2. We should avoid term_to_binary, since those cookies might be reused by non-Erlang applications.
3. Summing up points 1 and 2, I'd replace Username and ExtraData with a single parameter named Data::iolist() and let the user decide himself what to store there and how to encode it. For those who want compatibility and flexibility, it will be something like JSON. For those who are after compaction, it will be zlib or something.

[lhft]

Good points, thanks.
I will have a look.

On Tue, Jan 8, 2013 at 6:19 PM, Dmitry Demeshchuk
notifications@git.luolix.topwrote:

are after compacti

[lhft]

I have made the suggested changes and also corrected some bugs found on the way. I do not undertand the implications of getting rid of the username and only leaving the expiration time to generate the keys.
On the other hand, the code it's free of term_to_binary now.
One draw back with the changes is that the data to be saved on the cookie, must be a list.

[doubleyou]

Sorry for the delay on this. I've taken a brief look, overall looks good. Will give it a full review later today.

[doubleyou]

Last final notes.

1. cookie_encode/1, cookie_decode/1 and especially timestamp_sec/1 shouldn't be exported.
2. Let's use base64 instad of using bin2hex and hex2bin. First of all, it's already implemented, like, in all possible languages. Secondly, it's better in terms of compatibility (say, URL-encoding, if you need to send the token as a GET parameter). Finally, it's more compact.
3. Allow using binaries for data bulks, keys and so on. I recommend using something like that:

ensure_binary(B) when is_binary(B) ->
    B;
ensure_binary(L) when is_list(L) ->
    iolist_to_binary(L).  %% note, iolist_to_binary is better, since we might use something like mochijson2 here

and then

BData = ensure_binary(Data),
...

Don't forget to modify the function guards.

1. Fix the docs/specs, some of them became outdated I believe.

[doubleyou]

I would either separate all the parameters with a comma or remove comma at all (since expiration time length is a known thing).

If you choose the latter, swap Hmac and EData, you'll be able to do simple pattern-matching instead of binary module splits.

[doubleyou]

Also, see some in-code notes I left.

[doubleyou]

Actually, Data can be a binary here too.

[doubleyou]

Looks great now!

Besides a small note above, I'd like you to do some formatting:

1. Use spaces in assignment syntax (A=1 vs A = 1)
2. Use spaces after commas
3. Make all lines fit into standard 80 symbols length

Sorry if I seem way too critical, it's just important to make the code consistent and re-usable. Your addition is really appreciated, as well as your patience! :)

[doubleyou]

Just a hint: if you're using vim, you could run the following commands for the points 1 and 2:

:%s/,/, /g
:%s/,  /, /g
:%s/=/ = /g
:%s/  =  / = /g

The extra two commands are here to fix the places where you actually used the correct spacing.

[doubleyou]

Good, thanks for the addition!

[lhft]

There is a bug. The generated cookie data does not avoid RFC 2616 separators.

[etrepum]

Do you have an example?

[lhft]

mochiweb_session:generate_session_cookie(mochiwebf:timestamp_sec(now()), "a@gmail.com",fun(N)->N end,"asdjfjwejriwejsd").

We are using base64:encode for encoding. Any ideas of what might be the way to explore?

[etrepum]

It does look like this module is broken, and shouldn't be used until this
is fixed. There hasn't been a tagged release of mochiweb with it yet, so
don't consider it stable. The easiest fix is probably to use base64url
encoding rather than base64 encoding.

On Fri, Mar 15, 2013 at 2:32 AM, lhft notifications@github.com wrote:

mochiweb_session:generate_session_cookie(mochiwebf:timestamp_sec(now()), "
a@gmail.com",fun(N)->N end,"asdjfjwejriwejsd").

We are using base64:encode for encoding. Any ideas of what might be the
way to explore?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/94#issuecomment-14951508
.

[etrepum]

I've added a base64url codec and changed mochiweb_session to use it. It
might be usable now.

On Fri, Mar 15, 2013 at 10:30 AM, Bob Ippolito bob@redivi.com wrote:

It does look like this module is broken, and shouldn't be used until this
is fixed. There hasn't been a tagged release of mochiweb with it yet, so
don't consider it stable. The easiest fix is probably to use base64url
encoding rather than base64 encoding.

On Fri, Mar 15, 2013 at 2:32 AM, lhft notifications@github.com wrote:

mochiweb_session:generate_session_cookie(mochiwebf:timestamp_sec(now()), "
a@gmail.com",fun(N)->N end,"asdjfjwejriwejsd").

We are using base64:encode for encoding. Any ideas of what might be the
way to explore?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/94#issuecomment-14951508
.