Code review comment for lp:~widelands-dev/widelands-website/django1_11

SirVer (sirver) wrote :

Thanks for this massive update and keeping our homepage alive and kicking!

Drive by comment, only refering to the questions you've asked. I did not review the rest of the change - it seems massive.

> Providing usernames for JS when writing PMs: This is maybe a security risk because a username can contain an at sign (@). The Django documentation says:

Are we sure usernames can only contain the following characters? @A-Za-z0-9 If yes, than that function is indeed safe. If they can contain other characters (."'/\) we will be vulnerable.

> RegEx urls

Seems correct to me.

> PBKDF2

This is as sufficiently good hasher for us.

> Replacing lambdas with callables: Django can't serialize lambdas for migrations. For the screens app i have replaced the lambdas with callables: https://bazaar.launchpad.net/~widelands-dev/widelands-website/django1_11/revision/494#wlscreens/views.py

For this I do not know. If it works in your tests, that is probably all fine.

review: Approve

« Back to merge proposal