The following line in captcha.module:

      variable_set('captcha_wrong_response_counter', variable_get('captcha_wrong_response_counter', 0) + 1);

- is very expensive for site performance (call to variable_set() clears variable cache) and it opens doors to simple DOS attacks (bad guys can just keep anonymously submitting forms with wrong captchas).
Files: 
CommentFileSizeAuthor
#10 captcha-disable-captcha_wrong_response_counter-1002246-10.patch2.66 KBpbull
PASSED: [[SimpleTest]]: [MySQL] 731 pass(es).
[ View ]
#7 captcha-disable-captcha_wrong_response_counter-1002246-7.patch2.72 KBpbull
PASSED: [[SimpleTest]]: [MySQL] 731 pass(es).
[ View ]

Comments

Title:Cache clear and performance of variable_set('captcha_wrong_response_counter',...)variable_set('captcha_wrong_response_counter',...) is expensive performance wise (clears cache)

Good point.

But what is your feature request:
remove the counting of wrong responses or making it optional?

I would mark it as a bug, but nothing is broken, so I chose "feature request".
I think the module will benefit from a different accounting method, so it really is an "improvement request"

(duplicate reply removed)

Maybe, we can combine this with feature request #998326: Separate log of wrong and empty responses (especially see http://drupal.org/node/998326#comment-3827828)

Would be great if it is solved there. Unfortunately the patch in #998326: Separate log of wrong and empty responses does not change variable_set().

It is actually a design challenge to do it right. Drupal variable_*() API is not intended for often-changed values such as the counter in question. Own table is probably a way to go. I also thought of a separate module that would provide counter API to modules like CAPTCHA. "fastvars.module" anyone?

Yep, the patch even doesn't work yet (it breaks tests). But the counter could be moved to a separate "CAPTCHA logging and counting" module as suggested in http://drupal.org/node/998326#comment-3827828 :

  • CAPTCHA core module: exposes counting/logging with a hook, but doesn't do it itself.
  • "CAPTCHA logging and counting module": implements the counting with one of the following (for example)
    • variable_set: still not optimal, but at least the module is not required for CAPTCHA to work and can be disabled
    • a custom variation of variable_set that doesn't flush cache: a bit ugly/hackish, but it doesn't require to create/manage/maintain an extra table.
    • with its own table: seems a bit overkill for a single counter variable
    • by depending on another module (fastvars or something): also seems a bit overkill to depend on another module for storing a simple variable, unless the module is widely used and popular (genre CCK/views)

just thinking out loud here

Version:6.x-2.x-dev» 7.x-1.x-dev
StatusFileSize
new2.72 KB
PASSED: [[SimpleTest]]: [MySQL] 731 pass(es).
[ View ]

It's a serious concern that incorrect CAPTCHAs could be used as an attack vector. Attaching a patch against the 7.x-1.x branch that does the following:

* disables the updating of captcha_wrong_response_counter by default
* provides an admin option to enable the counter
* hides the counter from the Status report page if it's disabled, as it's not a meaningful data point if it's not being incremented

Status:Active» Needs review

Status:Needs review» Needs work

great, thanks

maybe use a more general variable 'captcha_enable_stats' instead of 'captcha_count_wrong_responses'. That way we can use it to guard more counters in the future that would require variable_set (e.g. total number of generated CAPTCHA's, number of correctly solved ones, ...)

Status:Needs work» Needs review
StatusFileSize
new2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 731 pass(es).
[ View ]

@soxofaan: Good idea, the 'captcha_count_wrong_responses' variable seemed a bit clunky to me. Attaching an updated patch to use 'captcha_enable_stats' and 'Enable statistics' on the admin side.

Status:Needs review» Fixed

tweaked the admin description a bit and committed

http://drupalcode.org/project/captcha.git/commit/ad165d1

Status:Fixed» Closed (fixed)
Issue tags:-cache, -variable_set

Automatically closed -- issue fixed for 2 weeks with no activity.