Anti flood system, deploy after certificate is bought
Review Request #188 - Created Jan. 30, 2012 and submitted
Information | |
---|---|
Adrian Budau | |
infoarena | |
Reviewers | |
hackers | |
Added a token based system for actions on site. Secured the login and register page Fixed the recaptcha https server There is a maximum amount of tokens per IP. Actions like register and login cost tokens. When there are not enough tokens a captcha is requested. The tokens regenerate at a constant rate. Example -> This is how the captcha is requested at this moment for register/login: You can login/logout as manytimes as you want. If you do 3 bad login attempts a captcha will appear and will be requested until you login correctly. You always need a captcha for registering and after that after only one bad login attempt a captcha will be requested. You can logout and login a different account without the need of a captcha(there is no way to use this as a brute-force entrance). All of these are checked on the client's IP. Token system description below: You can communicate with the tokens system with the functions get_tokens to get current tokens check_captcha_for_tokens to check for captcha submits and their correctness thus adding an amount of tokens, this function also returns the error of the captcha(it can be forced to search for all errors) pay_tokens which pays a certain amount of tokens or receives(if used with a negative value), it returns true or false weather it can pay or not(has enough) save_tokens(which pushes the tokens to the mysql db)
Good job for the speedy implementation. I have looked over everything except how the token system is implemented. A few more lines in the review request describing your design and decisions would help. :)
-
trunk/common/common.php (Diff revision 2) -
1 == '1' in php, since == does type conversions. Only need to do this if using ===
-
trunk/common/db/db.php (Diff revision 2) -
Does db_fetch_all never return null?
-
trunk/common/db/tokens.php (Diff revision 2) -
It's better to have them in config.php.sample even if we have to manually update it than to have them spread across a bunch of files.
-
trunk/common/db/tokens.php (Diff revision 2) -
Use the constant you defined.
-
trunk/common/external_libs/recaptchalib.php (Diff revision 2) -
I think it would be better to just download the newest version of recaptchalib.php.
-
trunk/www/controllers/login.php (Diff revision 2) -
There's no need to flash errors or anything. This is done transparently by most websites.
-
trunk/www/controllers/login.php (Diff revision 2) -
Define these as constants.
-
trunk/www/url.php (Diff revision 2) -
if ($secure_connection) either force $absolute to be true or log_assert($absolute == true), otherwise the behavior of the function will be confusing.
-
trunk/www/url.php (Diff revision 2) -
I would make this something like $force_secure_connection. If the user is already in a https session (he purposely went to https://infoarena.ro), he should not be redirected to a non-secure session because of random url_absolute in our code. Rendered textile is cached, so that should be made to not use absolute urls anymore.
-
trunk/www/views/form_login.php (Diff revision 2) -
Would this not go well in a template? Can we reuse same code in login and register?
Change Summary:
Repaired problems, modified the textile link to default relative paths(absolute links remain absolute). Updated description
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+269 -43) |
Change Summary:
Replaced constant values like 1000000 and 200000 with the constants IA_TOKENS_REGISTER and IA_TOKENS_LOGIN
Diff: |
Revision 4 (+269 -43) |
---|
I would suggest renaming push_tokens to save_tokens and add_tokens to something that includes the word captcha. It's not obvious what add_tokens is supposed to do. You aren't using this system consistently in the login and register controller. In the register controller, you pay tokens after there has been no captcha error. Why? You should do it like you did in the login controller: if ip doesn't have enough tokens to register, show the captcha. I don't think you should reset a user's token count to maximum if he passes a captcha. You'll see for Gmail login for example that once they show you a captcha, they will show it to you forever until you login correctly. You could give him a small amount of tokens back, so that after 5 successful captchas he gets one free login attempt and give him a slightly bigger amount of tokens back if he successfully logins. :)
-
trunk/www/url.php (Diff revisions 2 - 4) -
My comment was about more than just renaming a variable. It was about adding proper https support. If a user has visited https://infoarena.ro/something, that means he wants to use https://, but there are various random links throughout the site that redirect you to http://infoarena.ro because that's what url_absolute does. We need to keep track if the user wants https or http (based on what his last request was for example) and change url_absolute to keep using that protocol.
-
trunk/common/db/tokens.php (Diff revision 4) -
Use remote_ip_info() (and move it to common/ if it's in www/ like I remember). It returns more information, such as if users were proxied through various IPs.
-
trunk/common/db/tokens.php (Diff revision 4) -
I'm worried about this. Does it get freed between requests? If it does, then I don't think there's a need for the [$ip]. If it doesn't, then it will keep growing and fill up available memory. We already have a huge memory leak somewhere in our code that means we need to restart the server regularly.
-
trunk/common/db/tokens.php (Diff revision 4) -
Indenting
-
trunk/www/url.php (Diff revision 4) -
Why the extra variable?
-
trunk/www/wiki/MyTextile.php (Diff revision 4) -
Why do you need to do this? url parsing is not really that easy, what happens if there are more than 1 '?' in the url? Replacing IA_URL with IA_URL_PREFIX should suffice.
Change Summary:
Fixed a memory leak in common/textblock.php where an object of type simple_html_dom was unset before it's destructor was called. This added about 2 megabytes of data on average for each snippet in the news on each rendering.
Diff: |
Revision 6 (+302 -63) |
---|
Change Summary:
The system works right now like this: You get tokens for 3 login attempts afterwards you need to answer a correct captcha until you successfully login. After you login you receive enough tokens to logout and login as a different account (or the same) without a captcha. But only 1 try. You always need captcha for register, but after you register you get tokens for a "free" login attempt. This is just a fast security fix, it can be integrated in textblock_edits and task submits very easily. The tokens regenerate to 100% after 5 hours of idleness. Should this number be altered to something different?
Description: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+329 -66) |
Change Summary:
Tokens now regenerate gradually. 1 token back every 5 minutes.
Diff: |
Revision 8 (+338 -66) |
---|
Good job! Please summarize what was said in the comments into a better description of how the token system works (right now the review request describes an API, instead of how it actually works which is more important) and put that as a commit message. I realize that writing this may be unpleasant, but it's going to be the only source of documentation for future developers. :) Also, update the recaptchalib to the latest version from Google (there's more than one link change in the latest version).
-
trunk/common/db/tokens.php (Diff revisions 5 - 8) -
fits in 80 characters, yes?
-
trunk/common/common.php (Diff revision 8) -
whether
Change Summary:
Completed everything.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+349 -76) |
Description: |
|
---|