Issue 2234 - Add a WSGI controller to collect email addresses for the Adblock Browser iOS launch

sitescripts/submitEmail/web/submitEmail.py

+# coding: utf-8
+
+# This file is part of the Adblock Plus web scripts,
+# Copyright (C) 2006-2015 Eyeo GmbH
+#
+# Adblock Plus is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# Adblock Plus is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import re
+import errno
+import fcntl
+import random
+import string
+import wsgiref.util
+from urlparse import parse_qs, urljoin
+from urllib import urlencode
+
+from sitescripts.utils import get_config, sendMail
+from sitescripts.web import url_handler, form_handler
+
+VERIFICATION_PATH = '/verifyEmail'
+VERIFICATION_CODE_PARAM = 'code'
+VERIFICATION_CODE_CHARS = string.letters + string.digits
+
+verification_code_regexp = re.compile(r'^[%s]+$' % re.escape(VERIFICATION_CODE_CHARS))

[kzar, 2015/04/22 12:48:13]

Shouldn't this be called VERIFICATION_CODE_REGEXP?

[Sebastian Noack, 2015/04/22 13:09:49]

I usually only use constants for simple values. Having a constant for a mutable
instance of a complex object, initialized function call seems weird to me.

+
+def generate_verification_code():
+  return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32))

[kzar, 2015/04/22 12:21:54]

If we're going to use a constant for the chars shouldn't we use a constant for
the length too? We can then use it in the regexp as well to ensure the code is
the correct length.

[Sebastian Noack, 2015/04/22 12:28:45]

We don't need to check for the exact length when validation the verification
code. We merely need to make sure that it can be safely joined to a dirname
resulting into a valid filename in that directory. The case that the file
doesn't exist must be handled anyway. Checking also for the length would just
add unnecessary complexity, and makes it harder to change the length later.

[kzar, 2015/04/22 12:48:13]

Well using a constant for the length would mean that it would not be harder to
change later. The code would be more consistent too. We could call it
VERIFICATION_CODE_LENGTH.

[Sebastian Noack, 2015/04/22 13:09:49]

I won't mind adding a constant, but I'm opposed to using it in the regexp
validating the verification code. However, if the value is only used once there
isn't really a point of using a constant. Note that if we change that constant
later all so far issued verification codes would become invalid, if the length
is considered for validation.

[kzar, 2015/04/22 13:31:34]

Sure and if we change the character set constant all previously issued codes
become invalid too...

[Sebastian Noack, 2015/04/22 13:42:28]

Yes (if some of the previous characters aren't included anymore). However, we
check for those characters because it's the simplest way to make sure that we'll
get a legit path. Not doing that potentially causes issues. But checking for the
length will just add complexity but won't have any effect on the result.

+
+def get_filename_for_verification_code(config, verification_code):
+  directory = config.get('submitEmail', 'pendingVerficationsDirectory')
+  return os.path.join(directory, verification_code)
+
+def create_file_for_verification_code(config):
+  flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY | getattr(os, 'O_BINARY', 0)
+
+  while True:
+    verification_code = generate_verification_code()
+    filename = get_filename_for_verification_code(config, verification_code)
+
+    try:
+      return (os.open(filename, flags), verification_code)
+    except OSError as e:
+      if e.errno != errno.EEXIST:
+        raise
+
+def verify_email_address(config, verification_code):
+  if not verification_code_regexp.search(verification_code):
+    return False
+
+  verification_filename = get_filename_for_verification_code(config, verification_code)
+  try:
+    file = open(verification_filename, 'rb')
+  except IOError as e:
+    if e.errno == errno.ENOENT:
+      return False
+    raise
+
+  with file:
+    email = file.read()
+
+  verified_filename = config.get('submitEmail', 'verifiedEmailAddressesFile')
+  with open(verified_filename, 'ab', 0) as file:
+    fcntl.lockf(file, fcntl.LOCK_EX)
+    try:
+      print >>file, email
+    finally:
+      fcntl.lockf(file, fcntl.LOCK_UN)
+
+  try:
+    os.unlink(verification_filename)
+  except OSError as e:
+    if e.errno != errno.ENOENT:
+      raise
+
+  return True
+
+@url_handler('/submitEmail')
+@form_handler
+def submitEmail(environ, start_response, data):
+  email = data.get('email', '').strip()
+  if not email or '\r' in email or '\n' in email:

[Wladimir Palant, 2015/04/22 14:46:58]

If you don't want to verify email addresses, why are you still checking for
newlines then? We use r'^\w[\w.+!-]+@\w[\w.-]+\.[a-zA-Z]{2,6}$' to verify email
addresses in formmail.py - that's very permissive and should be good enough here
as well.

[Sebastian Noack, 2015/04/23 14:29:34]

I forbid newlines to ..

1. Prevent header injection (however, I realized that the "mime" template filter
does that as well)
2. The file of verified email addresses gets messed up if newlines are included,
as the email addresses in there are separated by newline.
This regexp isn't permissive at all, just some examples of valid email addresses
that don't match:

x@exmaple.com
foo~bar@example.com
+foo@example.com
-foo@example.com

I've implemented a more reasonable validation approach now.

+    start_response('400 Bad Request', [('Content-Type', 'text/plain')])
+    if email:
+      return ['No newlines allowed in email address.']
+    return ['No email address given.']
+
+  config = get_config()
+  fd, verification_code = create_file_for_verification_code(config)

[Wladimir Palant, 2015/04/22 14:46:58]

Storing pending email addresses is pointless:

1. If they aren't confirmed then we shouldn't store them at all.
2. This file will grow in size continuously.

Please add a secret key to the sitescripts.ini file. The verification code can
then be generated as:

hmac.new(secret, email).hexdigest()

The verification URL should receive both the email address and this verification
code - if they match we should add the email address to the list.

[Sebastian Noack, 2015/04/23 14:29:34]

I initially considered a similar approach, but found it more straight-forward to
use random verification codes instead involving a secrect key (that can be
leaked) and crypto functions (that can be broke). And it keeps the link short,
as from my experience with generated emails, some mail clients out there tend to
break long links.

But on the other hand, not storing unverified email addresses simplifies the
logic here quite a lot, and is a little more privacy friendly. So fair enough.

+  try:
+    os.write(fd, email.encode('utf-8'))
+  finally:
+    os.close(fd)
+
+  sendMail(
+    config.get('submitEmail', 'verificationEmailTemplate'),
+    {
+      'recipient': email,
+      'verification_url': '%s?%s' % (
+        urljoin(wsgiref.util.application_uri(environ), VERIFICATION_PATH),
+        urlencode([(VERIFICATION_CODE_PARAM, verification_code)])
+      )
+    }
+  )
+
+  start_response('200 OK', [('Content-Type', 'text/plain')])
+  return ["Thanks for your submission! You'll receive a verification email shortly."]
+
+@url_handler(VERIFICATION_PATH)
+def verifyEmail(environ, start_response):
+  config = get_config()
+
+  params = parse_qs(environ.get('QUERY_STRING', ''))
+  verification_code = params.get(VERIFICATION_CODE_PARAM, [''])[0]
+
+  if verify_email_address(config, verification_code):
+    option = 'successfulVerificationRedirectLocation'
+  else:
+    option = 'unknownVerificationCodeRedirectLocation'
+
+  start_response('303 See other', [('Location', config.get('submitEmail', option))])

[Wladimir Palant, 2015/04/22 14:46:58]

I see that you studied HTTP response codes in much detail but 302 Found please -
KISS. A 303 response is really not appropriate in this situation.

[Sebastian Noack, 2015/04/23 14:29:34]

Well, 302 is implemented inconsistently across browsers, and seems to be
superseded by 303 and 307 in HTTP 1.1. When searching the web for HTTP redirect
status codes it seems to be generally advised to use 303 in this case.

+  return []