Issue 5177883412660224: Issue 2234 - Add a WSGI controller to collect email addresses for the Adblock Browser iOS launch (Closed)
|
Created:
April 2, 2015, 1:57 p.m. by Sebastian Noack Modified:
Jan. 18, 2019, 11:33 p.m. CC:
Felix Dahlke, saroyanm, Philip Hill, mathias Visibility:
Public. |
DescriptionIssue 2234 - Add a WSGI controller to collect email addresses for the Adblock Browser iOS launch
Patch Set 1 : #Patch Set 2 : Use codecs.open() #
Total comments: 12
Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : Implemented double opt-in #Patch Set 5 : Forbid new lines in email address #Patch Set 6 : Use print statement #Patch Set 7 : Added verification email template #Patch Set 8 : Use parse_qs() instead dict(parse_qsl()) #
Total comments: 24
Patch Set 9 : Added missing colon #Patch Set 10 : Got rid of redundant variable #Patch Set 11 : Simplified verification code validation #
Total comments: 16
Patch Set 12 : Addressed comment #
Total comments: 8
Patch Set 13 : Renamed files #
Total comments: 2
Patch Set 14 : Moved signing logic into submit_email module #Patch Set 15 : Changed response texts for consistency with landing page #Patch Set 16 : Send error response when verification request is invalid #Patch Set 17 : Removed "beta" from email text #Patch Set 18 : Refactored #
Total comments: 7
Patch Set 19 : Addressed comments #Patch Set 20 : URL-encode language before inserting into URL #
Total comments: 4
MessagesTotal messages: 42
http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/.sit... .sitescripts.example:18: sitescripts.extensions.web.downloads = I think you might need to rebase this, there should be a filerhits listing here as well. http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... File sitescripts/formsubscribe/web/formsubscribe.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/formsubscribe/web/formsubscribe.py:24: @url_handler('/formsubscribe') Nit: Aren't we supposed to use double quotes for strings unless there is a reason to use single quotes? (Same in other files too obviously.) http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/formsubscribe/web/formsubscribe.py:34: fcntl.lockf(file, fcntl.LOCK_EX) How does this work if the file is already locked? Is it blocking, just delaying the new write slightly or would an exception be raised? http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... File sitescripts/web.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/web.py:63: These additions are cool, perhaps we should even open a new ticket to remove the random definitions of the `show_error` fn scattered about and replace their usage with `sitescripts.web.send_status_response`?
http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/.sit... .sitescripts.example:18: sitescripts.extensions.web.downloads = On 2015/04/05 15:45:14, kzar wrote: > I think you might need to rebase this, there should be a filerhits listing here > as well. I know, but this is the only file who were changed by both patches. http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... File sitescripts/formsubscribe/web/formsubscribe.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/formsubscribe/web/formsubscribe.py:24: @url_handler('/formsubscribe') On 2015/04/05 15:45:14, kzar wrote: > Nit: Aren't we supposed to use double quotes for strings unless there is a > reason to use single quotes? (Same in other files too obviously.) Well, double-/single-quotes are currently quite mixed through our Python code. The only rule we have so far is to stick to one of them consistently in the same file. http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/formsubscribe/web/formsubscribe.py:34: fcntl.lockf(file, fcntl.LOCK_EX) On 2015/04/05 15:45:14, kzar wrote: > How does this work if the file is already locked? Is it blocking, just delaying > the new write slightly or would an exception be raised? It blocks. http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... File sitescripts/web.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/web.py:63: On 2015/04/05 15:45:14, kzar wrote: > These additions are cool, perhaps we should even open a new ticket to remove the > random definitions of the `show_error` fn scattered about and replace their > usage with `sitescripts.web.send_status_response`? Yes, feel free to do so. ;) Note that there is already an issue to ue the form_handler decorator consistently: https://issues.adblockplus.org/ticket/2267
LGTM
Only nits below, I commented on the conceptual concerns in the issue. http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... File sitescripts/formsubscribe/web/formsubscribe.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/formsubscribe/web/formsubscribe.py:33: with codecs.open(filename, 'a', 'utf-8', buffering=0) as file: My understanding is that codecs is essentially superseded by the io module, shouldn't this use io.open()? Note that our code currently using codecs.open() is simply too old. http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/formsubscribe/web/formsubscribe.py:41: return ["Thanks for your submission! We'll notify you before the launch."] This seems silly, what exactly are we thanking for? Also, people want to be notified when we launch, not before that. How about: "Your email address has been recorded, we will notify you when we launch."
http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... File sitescripts/formsubscribe/web/formsubscribe.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/formsubscribe/web/formsubscribe.py:33: with codecs.open(filename, 'a', 'utf-8', buffering=0) as file: On 2015/04/05 21:15:17, Wladimir Palant wrote: > My understanding is that codecs is essentially superseded by the io module, > shouldn't this use io.open()? Note that our code currently using codecs.open() > is simply too old. In fact there are attempts to deprecate codecs.open for the past 5 years: https://bugs.python.org/issue8796 http://codereview.adblockplus.org/5177883412660224/diff/5676830073815040/site... sitescripts/formsubscribe/web/formsubscribe.py:41: return ["Thanks for your submission! We'll notify you before the launch."] On 2015/04/05 21:15:17, Wladimir Palant wrote: > This seems silly, what exactly are we thanking for? Also, people want to be > notified when we launch, not before that. How about: "Your email address has > been recorded, we will notify you when we launch." This comment is out of place here. I didn't came up with the text. It is specified in the mockup attached in https://issues.adblockplus.org/ticket/2213.
Implementation looks fine, there are some conceptual issues being discussed in Trac however. http://codereview.adblockplus.org/5177883412660224/diff/5747976207073280/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5747976207073280/site... sitescripts/submitEmail/web/submitEmail.py:37: file.write(u'\n') Nit: just '\n', not a Unicode string?
http://codereview.adblockplus.org/5177883412660224/diff/5747976207073280/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5747976207073280/site... sitescripts/submitEmail/web/submitEmail.py:37: file.write(u'\n') On 2015/04/06 18:07:30, Wladimir Palant wrote: > Nit: just '\n', not a Unicode string? Nope, doesn't work, since we have a TextIOWrapper, whcih raises a ValueError if you attempt to write str objects. This is also the reason I can't use the print statement here, because it converts the argument given apparently to str regardless of the kind of file-like object used.
I have uploaded a new patchset featuring double opt-in.
I've added the template for the verification email. So now this patch is no longer agnostic of Adblock Browser. So we might want to do one of following: 1. Land it just when publishing the website changes, to avoid leaking details prior to the release. 2. Make it a "shadow" sitescript, outside of the public repository. This will require some structural changes, moving the form_handler decorator into a separate change, and also we currently require email templates to live inside the sitescripts directory. And I suppose that we would like to move it into the sitescripts repository later, once the release has been announced.
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt Nit: These option names seem a bit long, also aren't we using "snake case" for options generally? http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:31: VERIFICATION_CODE_PARAM = 'code' Nit: Seems to me that the constant VERIFICATION_CODE_PARAM is uglier than the string 'code', and doesn't really add anything. It's only used twice, perhaps we should just use the string directly instead? http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:54: if verification_code in ('', os.path.curdir, os.path.pardir): Maybe we should check that the code matches a regexp of exactly what we're expecting? (A 32 character alpha-numeric string.) http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:90: if not email or '\r' in email or '\n' in email: Should we also check that the email address hasn't already been submitted and verified? http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:96: return [message] Nit: Instead of assigning `message` wouldn't it be easier to just return the message string wrapped in a list directly?
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/21 17:14:31, kzar wrote: > Nit: These option names seem a bit long, also aren't we using "snake case" for > options generally? Do you have a better (meaningful) name? The notation of option names seems to be mixed. @Wladimir: Your call. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:31: VERIFICATION_CODE_PARAM = 'code' On 2015/04/21 17:14:31, kzar wrote: > Nit: Seems to me that the constant VERIFICATION_CODE_PARAM is uglier than the > string 'code', and doesn't really add anything. It's only used twice, perhaps we > should just use the string directly instead? What's ugly about it? Yes, the code would be shorted without using a constant, but code almost always is. However, the point of using a constant is to make it more obvious that it must to be the same string in both places it is used, and documenting it's purpose. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:54: if verification_code in ('', os.path.curdir, os.path.pardir): On 2015/04/21 17:14:31, kzar wrote: > Maybe we should check that the code matches a regexp of exactly what we're > expecting? (A 32 character alpha-numeric string.) This would duplicate the format of the verification code. And even then, omitting the logic here would imply that neither dots nor dashes can occur in the verification code, wile this assumption is currently true, code assumptions are always foot guns waiting to triggered by future changes. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:90: if not email or '\r' in email or '\n' in email: On 2015/04/21 17:14:31, kzar wrote: > Should we also check that the email address hasn't already been submitted and > verified? That won't scale the way data are stored. But we can simply remove duplicates before sending the notification email later. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:96: return [message] On 2015/04/21 17:14:31, kzar wrote: > Nit: Instead of assigning `message` wouldn't it be easier to just return the > message string wrapped in a list directly? Then the else block would be redundant. And removing it would introduce inconsistent code structure. So I found it best to use a variable here.
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/21 18:10:30, Sebastian Noack wrote: > On 2015/04/21 17:14:31, kzar wrote: > > Nit: These option names seem a bit long, also aren't we using "snake case" for > > options generally? > > Do you have a better (meaningful) name? > > The notation of option names seems to be mixed. @Wladimir: Your call. Well for example instead of unknownVerificationCodeRedirectLocation how about failedVerificationURL or failedVerificationPage? http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:31: VERIFICATION_CODE_PARAM = 'code' On 2015/04/21 18:10:30, Sebastian Noack wrote: > On 2015/04/21 17:14:31, kzar wrote: > > Nit: Seems to me that the constant VERIFICATION_CODE_PARAM is uglier than the > > string 'code', and doesn't really add anything. It's only used twice, perhaps > we > > should just use the string directly instead? > > What's ugly about it? Yes, the code would be shorted without using a constant, > but code almost always is. However, the point of using a constant is to make it > more obvious that it must to be the same string in both places it is used, and > documenting it's purpose. Well ugliness is subjective but to me VERIFICATION_CODE_PARAM looks a lot uglier than "code". http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:54: if verification_code in ('', os.path.curdir, os.path.pardir): On 2015/04/21 18:10:30, Sebastian Noack wrote: > On 2015/04/21 17:14:31, kzar wrote: > > Maybe we should check that the code matches a regexp of exactly what we're > > expecting? (A 32 character alpha-numeric string.) > > This would duplicate the format of the verification code. And even then, > omitting the logic here would imply that neither dots nor dashes can occur in > the verification code, wile this assumption is currently true, code assumptions > are always foot guns waiting to triggered by future changes. Well if the filterhits code review is anything to go by Wladimir will agree with me here. I'm not saying you should remove the other checks, just that you should probably check that it's a 32 character alpha-numeric string as well. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:90: if not email or '\r' in email or '\n' in email: On 2015/04/21 18:10:30, Sebastian Noack wrote: > On 2015/04/21 17:14:31, kzar wrote: > > Should we also check that the email address hasn't already been submitted and > > verified? > > That won't scale the way data are stored. But we can simply remove duplicates > before sending the notification email later. OK, fair enough. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:96: return [message] On 2015/04/21 18:10:30, Sebastian Noack wrote: > On 2015/04/21 17:14:31, kzar wrote: > > Nit: Instead of assigning `message` wouldn't it be easier to just return the > > message string wrapped in a list directly? > > Then the else block would be redundant. And removing it would introduce > inconsistent code structure. So I found it best to use a variable here. Well it just seems like busy-work to me. Keeping something redundant in the code in order to avoid something else becoming redundant seems like weird logic. (Also it looks like the else clause is missing the trailing colon.)
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/22 09:17:46, kzar wrote: > On 2015/04/21 18:10:30, Sebastian Noack wrote: > > On 2015/04/21 17:14:31, kzar wrote: > > > Nit: These option names seem a bit long, also aren't we using "snake case" > for > > > options generally? > > > > Do you have a better (meaningful) name? > > > > The notation of option names seems to be mixed. @Wladimir: Your call. > > Well for example instead of unknownVerificationCodeRedirectLocation how about > failedVerificationURL or failedVerificationPage? I think I prefer the more descriptive names I use there, making it clear that a redirect is issued to that location. If possible I prefer single word names too, but this isn't possible here and whether 3 or 5 words doesn't really matter IMO. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:54: if verification_code in ('', os.path.curdir, os.path.pardir): On 2015/04/22 09:17:46, kzar wrote: > On 2015/04/21 18:10:30, Sebastian Noack wrote: > > On 2015/04/21 17:14:31, kzar wrote: > > > Maybe we should check that the code matches a regexp of exactly what we're > > > expecting? (A 32 character alpha-numeric string.) > > > > This would duplicate the format of the verification code. And even then, > > omitting the logic here would imply that neither dots nor dashes can occur in > > the verification code, wile this assumption is currently true, code > assumptions > > are always foot guns waiting to triggered by future changes. > > Well if the filterhits code review is anything to go by Wladimir will agree with > me here. I'm not saying you should remove the other checks, just that you should > probably check that it's a 32 character alpha-numeric string as well. I don't know which comment in that other review you are referring to. But I can't remember any case where we encouraged duplication of logic. It is arguable whether following check would be simpler than the logic currently used here, and therefore preferable despite the points I outlined above: if not re.search('^[a-z0-9]+$', verification_code, re.I): return False But introducing redundant logic is certainly not what we are used to do. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:96: return [message] On 2015/04/22 09:17:46, kzar wrote: > On 2015/04/21 18:10:30, Sebastian Noack wrote: > > On 2015/04/21 17:14:31, kzar wrote: > > > Nit: Instead of assigning `message` wouldn't it be easier to just return the > > > message string wrapped in a list directly? > > > > Then the else block would be redundant. And removing it would introduce > > inconsistent code structure. So I found it best to use a variable here. > > Well it just seems like busy-work to me. Keeping something redundant in the code > in order to avoid something else becoming redundant seems like weird logic. Wouldn't it be the same when doing it the other way around? However, it doesn't make much a difference which poison to pick here. So done now. > (Also it looks like the else clause is missing the trailing colon.) This has already been fixed (you are looking on an outdated version of the patch).
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/22 10:06:00, Sebastian Noack wrote: > On 2015/04/22 09:17:46, kzar wrote: > > On 2015/04/21 18:10:30, Sebastian Noack wrote: > > > On 2015/04/21 17:14:31, kzar wrote: > > > > Nit: These option names seem a bit long, also aren't we using "snake case" > > for > > > > options generally? > > > > > > Do you have a better (meaningful) name? > > > > > > The notation of option names seems to be mixed. @Wladimir: Your call. > > > > Well for example instead of unknownVerificationCodeRedirectLocation how about > > failedVerificationURL or failedVerificationPage? > > I think I prefer the more descriptive names I use there, making it clear that a > redirect is issued to that location. > > If possible I prefer single word names too, but this isn't possible here and > whether 3 or 5 words doesn't really matter IMO. I don't think the extra words add anything, but it doesn't really matter enough to waste time discussing further I guess. http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:54: if verification_code in ('', os.path.curdir, os.path.pardir): On 2015/04/22 10:06:00, Sebastian Noack wrote: > On 2015/04/22 09:17:46, kzar wrote: > > On 2015/04/21 18:10:30, Sebastian Noack wrote: > > > On 2015/04/21 17:14:31, kzar wrote: > > > > Maybe we should check that the code matches a regexp of exactly what we're > > > > expecting? (A 32 character alpha-numeric string.) > > > > > > This would duplicate the format of the verification code. And even then, > > > omitting the logic here would imply that neither dots nor dashes can occur > in > > > the verification code, wile this assumption is currently true, code > > assumptions > > > are always foot guns waiting to triggered by future changes. > > > > Well if the filterhits code review is anything to go by Wladimir will agree > with > > me here. I'm not saying you should remove the other checks, just that you > should > > probably check that it's a 32 character alpha-numeric string as well. > > I don't know which comment in that other review you are referring to. But I > can't remember any case where we encouraged duplication of logic. > > It is arguable whether following check would be simpler than the logic currently > used here, and therefore preferable despite the points I outlined above: > > if not re.search('^[a-z0-9]+$', verification_code, re.I): > return False > > But introducing redundant logic is certainly not what we are used to do. Exactly, that's what I meant, I think we should add that check.
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/site... sitescripts/submitEmail/web/submitEmail.py:54: if verification_code in ('', os.path.curdir, os.path.pardir): On 2015/04/22 10:13:04, kzar wrote: > On 2015/04/22 10:06:00, Sebastian Noack wrote: > > On 2015/04/22 09:17:46, kzar wrote: > > > On 2015/04/21 18:10:30, Sebastian Noack wrote: > > > > On 2015/04/21 17:14:31, kzar wrote: > > > > > Maybe we should check that the code matches a regexp of exactly what > we're > > > > > expecting? (A 32 character alpha-numeric string.) > > > > > > > > This would duplicate the format of the verification code. And even then, > > > > omitting the logic here would imply that neither dots nor dashes can occur > > in > > > > the verification code, wile this assumption is currently true, code > > > assumptions > > > > are always foot guns waiting to triggered by future changes. > > > > > > Well if the filterhits code review is anything to go by Wladimir will agree > > with > > > me here. I'm not saying you should remove the other checks, just that you > > should > > > probably check that it's a 32 character alpha-numeric string as well. > > > > I don't know which comment in that other review you are referring to. But I > > can't remember any case where we encouraged duplication of logic. > > > > It is arguable whether following check would be simpler than the logic > currently > > used here, and therefore preferable despite the points I outlined above: > > > > if not re.search('^[a-z0-9]+$', verification_code, re.I): > > return False > > > > But introducing redundant logic is certainly not what we are used to do. > > Exactly, that's what I meant, I think we should add that check. Ok, went with this now, but without duplicating the list of characters.
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) 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.
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) On 2015/04/22 12:21:54, kzar wrote: > 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. 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.
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:35: verification_code_regexp = re.compile(r'^[%s]+$' % re.escape(VERIFICATION_CODE_CHARS)) Shouldn't this be called VERIFICATION_CODE_REGEXP? http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) On 2015/04/22 12:28:45, Sebastian Noack wrote: > On 2015/04/22 12:21:54, kzar wrote: > > 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. > > 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. 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.
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:35: verification_code_regexp = re.compile(r'^[%s]+$' % re.escape(VERIFICATION_CODE_CHARS)) On 2015/04/22 12:48:13, kzar wrote: > Shouldn't this be called VERIFICATION_CODE_REGEXP? 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. http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) On 2015/04/22 12:48:13, kzar wrote: > On 2015/04/22 12:28:45, Sebastian Noack wrote: > > On 2015/04/22 12:21:54, kzar wrote: > > > 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. > > > > 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. > > 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. 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.
LGTM http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) On 2015/04/22 13:09:49, Sebastian Noack wrote: > On 2015/04/22 12:48:13, kzar wrote: > > On 2015/04/22 12:28:45, Sebastian Noack wrote: > > > On 2015/04/22 12:21:54, kzar wrote: > > > > 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. > > > > > > 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. > > > > 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. > > 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. Sure and if we change the character set constant all previously issued codes become invalid too...
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:38: return ''.join(random.sample(VERIFICATION_CODE_CHARS, 32)) On 2015/04/22 13:31:34, kzar wrote: > On 2015/04/22 13:09:49, Sebastian Noack wrote: > > On 2015/04/22 12:48:13, kzar wrote: > > > On 2015/04/22 12:28:45, Sebastian Noack wrote: > > > > On 2015/04/22 12:21:54, kzar wrote: > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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. > > Sure and if we change the character set constant all previously issued codes > become invalid too... 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.
http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/template/verification.mail (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/template/verification.mail:3: Subject: Please confirm your Adblock Browser notification Ok, now it mentions Adblock Browser explicitly. What's the plan? http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:92: if not email or '\r' in email or '\n' in email: 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. http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:99: fd, verification_code = create_file_for_verification_code(config) 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. http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:131: start_response('303 See other', [('Location', config.get('submitEmail', option))]) 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.
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/21 18:10:30, Sebastian Noack wrote: > On 2015/04/21 17:14:31, kzar wrote: > > Nit: These option names seem a bit long, also aren't we using "snake case" for > > options generally? > The notation of option names seems to be mixed. @Wladimir: Your call. I discussed that with Wladimir on IRC. And we agreed to use underscores for new options and sections. I changed the names here receptively. While on it, I also renamed the modules to use PEP-8 compliant underscore notation. http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/template/verification.mail (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/template/verification.mail:3: Subject: Please confirm your Adblock Browser notification On 2015/04/22 14:46:58, Wladimir Palant wrote: > Ok, now it mentions Adblock Browser explicitly. What's the plan? Yep, as I already pointed out and made suggestions here: http://codereview.adblockplus.org/5177883412660224/#msg12 Afterwards, I also asked Philip how we are going to test the corresponding website changes, and it turned out that he will test the landing pages with a local VM running Anwiki, manually adding the pages from the review. So I suppose he can test the backend the same way, and we can land this patch just when publishing the website changes. http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:92: if not email or '\r' in email or '\n' in email: On 2015/04/22 14:46:58, Wladimir Palant wrote: > If you don't want to verify email addresses, why are you still checking for > newlines then? 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. > 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. 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. http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:99: fd, verification_code = create_file_for_verification_code(config) On 2015/04/22 14:46:58, Wladimir Palant wrote: > 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. 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. http://codereview.adblockplus.org/5177883412660224/diff/5740423507083264/site... sitescripts/submitEmail/web/submitEmail.py:131: start_response('303 See other', [('Location', config.get('submitEmail', option))]) On 2015/04/22 14:46:58, Wladimir Palant wrote: > 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. 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.
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/23 14:29:34, Sebastian Noack wrote: > While on it, I also renamed the modules to use PEP-8 compliant underscore > notation. Could you please rename the files back now and do the renaming as a separate patchset? Not sure whether you noticed but Rietveld doesn't understand renaming and treats each renamed file as a new file. Consequently, interdiffs no longer work and figuring out what changed is tedious to say the least.
http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5655608640405504/.sit... .sitescripts.example:166: verifiedEmailAddressesFile=%(root)s/logs/verified_email_addresses.txt On 2015/04/23 14:39:25, Wladimir Palant wrote: > On 2015/04/23 14:29:34, Sebastian Noack wrote: > > While on it, I also renamed the modules to use PEP-8 compliant underscore > > notation. > > Could you please rename the files back now and do the renaming as a separate > patchset? Not sure whether you noticed but Rietveld doesn't understand renaming > and treats each renamed file as a new file. Consequently, interdiffs no longer > work and figuring out what changed is tedious to say the least. Done.
http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... File sitescripts/signing.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... sitescripts/signing.py:6: _SECRET = get_config().get('DEFAULT', 'secret') No, it should be .get('submit_email', 'secret') - the secret is specific to the application. The fact that our example configuration puts it under DEFAULT should probably be considered a bug, even though it works correctly that way. http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... sitescripts/signing.py:11: return reduce(lambda a, b: a | b, (ord(c1) ^ ord(c2) for c1, c2 in zip(s1, s2))) == 0 Do I get it correctly that you are trying to address timing attacks here? We are comparing strings that are merely 32 characters long, and the potential attacker isn't exactly sitting in front of our server. The comparison takes a bunch of nanoseconds on my computer - that's several magnitudes smaller than the variation introduced by the HTTP processing (never mind the network). You don't need to overthink this... Without this function this module isn't justified and I don't think we should have it. http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... sitescripts/submitEmail/web/submitEmail.py:64: fcntl.lockf(file, fcntl.LOCK_EX) Will locks work correctly with append mode? I think that the file pointer is set before you lock the file - meaning that once you get the lock you might not be at the end of the file any more. http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... File sitescripts/utils.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... sitescripts/utils.py:133: match.group(2).encode('idna')) This seems to have two issues: 1. match.group(1).encode('ascii') can fail and will produce another UnicodeEncodeError - yet the caller is only expecting ValueError. 2. I'm unsure whether match.group(2).encode('idna') will work - it requires unicode, not str. What do we have here?
http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... File sitescripts/signing.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... sitescripts/signing.py:6: _SECRET = get_config().get('DEFAULT', 'secret') On 2015/04/23 16:04:40, Wladimir Palant wrote: > No, it should be .get('submit_email', 'secret') - the secret is specific to the > application. The fact that our example configuration puts it under DEFAULT > should probably be considered a bug, even though it works correctly that way. Done. http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... sitescripts/signing.py:11: return reduce(lambda a, b: a | b, (ord(c1) ^ ord(c2) for c1, c2 in zip(s1, s2))) == 0 On 2015/04/23 16:04:40, Wladimir Palant wrote: > Do I get it correctly that you are trying to address timing attacks here? We are > comparing strings that are merely 32 characters long, and the potential attacker > isn't exactly sitting in front of our server. The comparison takes a bunch of > nanoseconds on my computer - that's several magnitudes smaller than the > variation introduced by the HTTP processing (never mind the network). You don't > need to overthink this... > > Without this function this module isn't justified and I don't think we should > have it. Sure, that was the idea. But fair enough. http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... File sitescripts/submitEmail/web/submitEmail.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... sitescripts/submitEmail/web/submitEmail.py:64: fcntl.lockf(file, fcntl.LOCK_EX) On 2015/04/23 16:04:40, Wladimir Palant wrote: > Will locks work correctly with append mode? I think that the file pointer is set > before you lock the file - meaning that once you get the lock you might not be > at the end of the file any more. It does work correctly on Linux. I tested it with two python shells opening the the same file simultaneously in append mode. Despite the tell() method reporting the position of the end of the file when it was opened, data written to the file are still appended to the end of the file, behind other data that have been written by the other process meanwhile. http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... File sitescripts/utils.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5651874166341632/site... sitescripts/utils.py:133: match.group(2).encode('idna')) On 2015/04/23 16:04:40, Wladimir Palant wrote: > This seems to have two issues: > > 1. match.group(1).encode('ascii') can fail and will produce another > UnicodeEncodeError - yet the caller is only expecting ValueError. issubclass(UnicodeEncodeError, ValueError) => True > 2. I'm unsure whether match.group(2).encode('idna') will work - it requires > unicode, not str. What do we have here? Sure, a function supposed to encode text, is supposed to be called with a unicode object, as we do here.
LGTM
The patchset has been updated, according to the changes documented in the issue.
And there is one more change: Apparently there won't be a beta, but only a stable release for iOS.
http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... sitescripts/submit_email/web/submit_email.py:41: return send_simple_response(start_response, 400, 'Please enter a valid email address.') Nit: Move the last parameter to the next line? http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... sitescripts/submit_email/web/submit_email.py:58: 'confirmation link.') Nit: Move the last parameter to the next line and add two additional indentation levels? That's better than trying to squeeze it into whatever space you have left in the line. http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... File sitescripts/web.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... sitescripts/web.py:94: return send_simple_response(start_response, 400, 'Form data must use UTF-8') 'Invalid form data encoding'?
http://codereview.adblockplus.org/5177883412660224/diff/5756035713204224/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5756035713204224/.sit... .sitescripts.example:168: successful_verification_redirect_location=https://adblockplus.org/en/adblock-browser/verification-success It seems wrong to have the locale "en" hardcoded in the URLs here? http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... File sitescripts/submit_email/template/verification.mail (right): http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... sitescripts/submit_email/template/verification.mail:14: You can simply ignore this email if you didn't sign up. I think this text "You can simply ignore this email if you didn't sign up." should be at the end of the email. It makes more sense to have the order "Thanks for signing up, here's the link, if you change you're mind do this..., otherwise if you didn't sign up you can ignore." Otherwise it might imply that more emails are on the way for the person who didn't sign up and wants to ignore.
http://codereview.adblockplus.org/5177883412660224/diff/5756035713204224/.sit... File .sitescripts.example (right): http://codereview.adblockplus.org/5177883412660224/diff/5756035713204224/.sit... .sitescripts.example:168: successful_verification_redirect_location=https://adblockplus.org/en/adblock-browser/verification-success On 2015/04/27 10:07:02, kzar wrote: > It seems wrong to have the locale "en" hardcoded in the URLs here? Done. http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... File sitescripts/submit_email/template/verification.mail (right): http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... sitescripts/submit_email/template/verification.mail:14: You can simply ignore this email if you didn't sign up. On 2015/04/27 10:07:02, kzar wrote: > I think this text "You can simply ignore this email if you didn't sign up." > should be at the end of the email. It makes more sense to have the order "Thanks > for signing up, here's the link, if you change you're mind do this..., otherwise > if you didn't sign up you can ignore." > > Otherwise it might imply that more emails are on the way for the person who > didn't sign up and wants to ignore. This comment is out of place here. We discussed the email text before and this version has been approved by Ann-Lee, Lisa and Wladimir. Feel free to post your suggestion in the draft on Google Docs: https://docs.google.com/document/d/1Jw0AmDKPLK8Vc-qgoRArtraGlHCESs3YA9sqoXzjI... http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... sitescripts/submit_email/web/submit_email.py:58: 'confirmation link.') On 2015/04/24 22:50:48, Wladimir Palant wrote: > Nit: Move the last parameter to the next line and add two additional indentation > levels? That's better than trying to squeeze it into whatever space you have > left in the line. I actually like it as it is. I certainly dislike starting arguments on lines with different level of indentation. But well, then lets put all arguments on intended lines below. http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... File sitescripts/web.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5730827476402176/site... sitescripts/web.py:94: return send_simple_response(start_response, 400, 'Form data must use UTF-8') On 2015/04/24 22:50:48, Wladimir Palant wrote: > 'Invalid form data encoding'? Not sure why this message would be any better, but fair enough.
LGTM
http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site... File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site... sitescripts/submit_email/web/submit_email.py:91: location = location.format(lang=quote(params.get('lang') or 'en', '')) Looks like a typo at the end there? ", ''"
http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site... File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site... sitescripts/submit_email/web/submit_email.py:91: location = location.format(lang=quote(params.get('lang') or 'en', '')) On 2015/04/28 11:13:42, kzar wrote: > Looks like a typo at the end there? ", ''" The empty string at the end is the second argument to quote() to make it escape backslashes as well.
LGTM http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site... File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site... sitescripts/submit_email/web/submit_email.py:91: location = location.format(lang=quote(params.get('lang') or 'en', '')) On 2015/04/28 11:31:25, Sebastian Noack wrote: > On 2015/04/28 11:13:42, kzar wrote: > > Looks like a typo at the end there? ", ''" > > The empty string at the end is the second argument to quote() to make it escape > backslashes as well. Oh I see.
http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site... File sitescripts/submit_email/web/submit_email.py (right): http://codereview.adblockplus.org/5177883412660224/diff/5859269547130880/site... sitescripts/submit_email/web/submit_email.py:91: location = location.format(lang=quote(params.get('lang') or 'en', '')) On 2015/04/28 11:31:25, Sebastian Noack wrote: > On 2015/04/28 11:13:42, kzar wrote: > > Looks like a typo at the end there? ", ''" > > The empty string at the end is the second argument to quote() to make it escape > backslashes as well. I meant (forward) slashes. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
