Sign the auth token cookie and make it httpOnly

[tms]

Since this seems to be a non-expiring value, eliminating the potential to make a lucky guess and protecting the cookie from XSS sniffing seems like the best call.

[eviltrout]

Really nice, thanks so much! 🐟

[SamSaffron]

@eviltrout @tms this broke the message bus cause there is a duplicate code path in current user that was not touched.

I am fine with http only, but a lucky guess of a sha1 hash is pretty much as lucky as an astroid hitting while I am typing.

Don't see any point in signing these cookies.

[SamSaffron]

correction, its actually more secure than a sha

SecureRandom.hex(EmailToken.token_length)

Its basically a GUID, you cant guess a GUID

[tms]

Wait, really? Hm, I searched for references and all the test cases passed, not sure how that got through. Oh, yeah, I see now...that was rather daft on my part, sorry.

My main concern here is that the value never, ever changes, and you don't have to know anything other than the value. So with enough values in use, it becomes less impractical that someone could guess the token of some existing user. Not a specific user, of course, but a random one, and that's enough for bad publicity.

[tms]

Signing the cookie requires them to also know the server's secret key, which would be practically impossible without them having compromised the system in a way that made these safeguards irrelevant, but I guess we could also just change _t to have a pair of values that have to match the the user in question instead (token and ID, for example).

That'd move it back into "asteroid hitting" territory, since it would impose the condition you'd have to have the right hash for a specific user, instead of just any user.

[SamSaffron]

you can't guess a guid no matter how hard you try, the only thing you are
eliminating is a db lookup, its not really worth the effort imho, think of
these auth keys as impossible to guess sequences of numbers

On Sun, Feb 24, 2013 at 12:32 AM, tms notifications@github.com wrote:

Signing the cookie requires them to also know the server's secret key,
which would be practically impossible without them having compromised the
system in a way that made these safeguards irrelevant, but I guess we could
also just change _t to have a pair of values that have to match the the
user in question instead (token and ID, for example).

That'd move it back into "asteroid hitting" territory, since it would
impose the condition you'd have to have the right hash for a specific user,
instead of just any user.

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

[tms]

I know it's not practical, it just feels wrong to make it easier than necessary. But that's fine, I'll revert the signing if that's the way you want to go, since the risk isn't appreciable.

My other consideration in signing the token was that you could invalidate all of them by changing the site's secret key if there was some situation that warranted that. Currently you'd have to change everyone's token value to get the same result.

[eviltrout]

It doesn't sound like it's needed but I think the bigger problem here is
why did no tests fail when it broke? I think we should keep the httpOnly
but also add a test to make sure this can't happen again.

On Saturday, February 23, 2013, tms wrote:

I know it's not practical, it just feels wrong to make it easier than
necessary. But that's fine, I'll revert the signing if that's the way you
want to go, since the risk isn't appreciable.

My other consideration in signing the token was that you could invalidate
all of them by changing the site's secret key if there was some situation
that warranted that. Currently you'd have to change everyone's token value
to get the same result.

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

[tms]

Yeah, the httpOnly is absolutely necessary from a security perspective.

Alright, I'll go ahead and remove the signing and take care of the test case then.