This module currently lets someone send emails to an address they choose. Someone could get a site into a spam block-list by changing their address over and over to be legitimate people's addresses.

There should be flood control to prevent someone from changing their email address more than ~5 times within a configurable time period.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

purushotam.rai’s picture

Status: Active » Needs review
FileSize
5.34 KB
greggles’s picture

Status: Needs review » Needs work

Thanks for the patch!

It seems like it would be good to have a test for this as well, yeah?

+      $msg = email_confirm_mail_text('email_change_request_flood_message', $language, []);
+      drupal_set_message($msg, 'warning');

It seems like this is a path for cross-site-scripting security attack, isn't it?

Also, I believe the standard is to use full words, so $message instead of $msg.

purushotam.rai’s picture

hi @greggles,

I have added tests for this functionality too (Need your guidance over here), besides above correction. Kindly review.

Thanks and Regards

purushotam.rai’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: flood-control-2305993-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

purushotam.rai’s picture

Status: Needs work » Needs review
FileSize
7.3 KB
1.11 KB

Sorry, I missed one thing. Patch updated.

Status: Needs review » Needs work

The last submitted patch, 6: flood-control-2305993-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

navneet0693’s picture

+++ b/tests/email_confirm.test
@@ -176,4 +184,27 @@ class EmailConfirmTestCase extends DrupalWebTestCase {
+      $this->assertText(t("A confirmation email has been sent to your new email address. You must follow the link provided in that email within 24 hours in order to confirm the change to your account email address."));

Avoid t() functions in assertion.

+++ b/tests/email_confirm.test
@@ -176,4 +184,27 @@ class EmailConfirmTestCase extends DrupalWebTestCase {
+      $this->drupalPost("user/" . $this->spamUser->uid . "/edit", $edit, t('Save'));

Form post will be unsuccessful without current password.

purushotam.rai’s picture

purushotam.rai’s picture

Status: Needs work » Needs review
greggles’s picture

What do you think about using filter_xss or filter_xss_admin instead of check_plain here? I wonder which one is best for this message.

greggles’s picture

Nitpicky requests:

+    // Get Flooding Limit

Comments should be sentences with closing punctuation and sentence casing.

+    $maxLimit = variable_get('email_confirm_email_change_request_flood_frequency', 10);

Variables should be consistent throughout a file (e.g. $max_limit instead of $maxLimit).

greggles’s picture

Status: Needs review » Needs work
purushotam.rai’s picture

Status: Needs work » Needs review
FileSize
7.35 KB
1.09 KB

Generally, I avoid such mistakes, probably this mistake has been by mistake :p.

Thanks and Regards

Status: Needs review » Needs work

The last submitted patch, 14: flood-control-2305993-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

purushotam.rai’s picture

Status: Needs work » Needs review