fix plusses in emails

[chadwhitacre]

Let's fix #3110. I don't know yet what the issue is, but so far here's a passing test for escaping plusses in emails in the links we send out. At what point are those getting unescaped?

[aandis]

I tested this with a couple of emails having plus sign and couldn't reproduce this. It's probably (selectively) happening on mandrill's end? If yes, what can we do about that?

[chadwhitacre]

Either on Mandrill's end or in actual mail clients (Gmail, etc.). More research required ...

[chadwhitacre]

@aandis We should just be able to base64-encode the email address for transport across the URL. Working on a commit ...

[chadwhitacre]

Actually, you wanna do that? :-)

[chadwhitacre]

@aandis Does it make sense what I'm suggesting? In other words, instead of sending this link in email:

https://gratipay.com/Foo/emails/verify.html?nonce=...&email=jay+waterpolice@jays.net

We would send this:

https://gratipay.com/Foo/emails/verify.html?nonce=...&email=amF5K3dhdGVycG9saWNlQGpheXMubmV0

Then we would decode on the receiving end. That should work, no?

[rohitpaulk]

^ I like that idea

[aandis]

Sounds good. I'll get to this tomorrow.

[aandis]

ankane/mailkick#5 (comment) suggests this will work!

[chadwhitacre]

ankane/mailkick#5 (comment) suggests this will work!

There we go. :-)

!m @aandis

[aandis]

Out of curiosity, why do we send the email in the verification link in the first place? The nonce generated is pretty much unique. Why not add a unique constraint on the nonce in db and use the nonce to get the email and verify it? Gets rid of this problem all together.

[chadwhitacre]

@aandis Also an option. Whatever is lowest-hanging fruit here, I think. We've got bigger fish to fry.

[chadwhitacre]

Which is to say, follow your ❤️, @aandis! :-)

[aandis]

Alright. :)

[rohitpaulk]

#3022 (comment) for reference

[rohitpaulk]

By requiring the user ID, an attacker can only brute-force one account at a time, instead of being able to attack all accounts at the same time. That means the probability of success becomes lower and constant (it won't grow with the number of users).

Right now it's not a big problem, but the probability of finding a good key increases with the number of users.

I know it still might not make a practical difference - but removing the email seems like a step backward to me :)

[Changaco]

Email verification nonces aren't much of a security issue since they're not passwords (they don't log you in, they only set the verified bit on the address). Still, you don't want to use a random nonce as both an ID and a password, even when there is no real threat.

@aandis "pretty much unique" isn't enough, if it's not 100% unique then you need extra code to deal with conflicts, otherwise a few unlucky users could be prevented from verifying their emails. So, encoding the address with base64 is simpler.

Two more things:

• it's probably better to have a way to differentiate links in the new and old formats, for example by using another querystring key such as email64
• you can't use standard base64, it's not querystring-safe (it can have plusses, and equal signs for padding), in Liberapay we have a b64encode_s function

[aandis]

Sorry I haven't gotten around to doing this yet. 😞

@Changaco my reason for wording it as "pretty much unique" and not "perfectly unique" was exactly that. There's a very (very) low probability that they can collide, but nonetheless, there is a chance and I had it in mind to handle the collisions if we went this route. :)
Anyway, base_64 should be the way to go here then. I'll try to push the fix to liberypay as well. :)

[Changaco]

Another thing I forgot to mention, including in the original comment that @rohitpaulk quoted, is that a DB index lookup is not guaranteed to run in constant time, so if you lookup a password (nonce or permanent, it doesn't matter) you're potentially exposed to a timing attack.

In short: never lookup a password, never put a UNIQUE constraint on a password column, always use constant_time_compare().

I'll try to push the fix to liberypay as well. :)

I look forward to it. :-)

[aandis]

Ready for review. :)

[aandis]

How is this still passing? 😱

[aandis]

I modified the verification link to contain email64=<encoded-email>. Here's the test

[aandis]

@whit537 before merge, we should probably remove this test as well. But I still don't understand why this is passing in the first place. The link now starts with /~alice/emails/verify.html?email64=, so the assert should fail. Something I don't know about mock yet?

[chadwhitacre]

Roger.

[chadwhitacre]

Something I don't know about mock yet?

Yup, you called it: link is a mock, which thinks everything is a-okay. Guido pointed this up recently.

(Pdb) l
246         def test_emails_with_plus_are_linked_properly(self, send_email):
247             send_email.return_value = 1
248             self.alice.add_email("foo+bar@example.com")
249             link = send_email.args[1]
250             import pdb; pdb.set_trace()
251  ->         assert link.startswith('/~alice/emails/verify.html?email=foo%2Bbar%40example.com&nonce=')
252     
253         def test_can_dequeue_an_email(self):
254             larry = self.make_participant('larry', email_address='larry@example.com')
255             larry.queue_email("verification")
256     
(Pdb) pp link
<MagicMock name=u'send_email.args.__getitem__()' id='4383627408'>
(Pdb)

[chadwhitacre]

... with liberapay's version?

[chadwhitacre]

This question would be obsolete with #3977.

[Changaco]

... with liberapay's version?

No, compatibility with standard base64.

[chadwhitacre]

Okay! Punchlist:

• rebase
• remove/update old test—fix plusses in emails #3965 (comment) —done in 8a73270

[chadwhitacre]

Rebased on master at e2c6062, previous head was 1d7e0cd.

[chadwhitacre]

@aandis I'm ready to land this when you are. What's left?

[chadwhitacre]

Derp. Missed an import. Force-pushed 770f0b2 over top of 8a73270.

[chadwhitacre]

Alright, @aandis, travis is green. We ready to merge? 😮

[aandis]

Yep! 💃