gwron / fluxbb_addon_verysimpleantibot Goto Github PK

These functions $this->get_chosen_question() and $this->get_chosen_question_hash() don't need to be called, but instead use their respective variables $this->chosen_question and $this->chosen_question_hash due to this if (!$this->do_choose_question()):

        //select a random question and skip showing the captcha form
        //when failing (no questions available)
        if (!$this->do_choose_question())
            return;

        global $lang_addon_vsab, $lang_common;
        ?>
        <div class="inform">
            <fieldset>
                <legend><?php echo $lang_addon_vsab['title'] ?></legend>
                <div class="infldset">
                    <p><?php echo $lang_addon_vsab['info'] ?></p>
                    <label class="required">
                        <strong><?php echo sprintf($lang_addon_vsab['question'], $this->get_chosen_question()) ?></strong>
                        <br />
                        <strong><?php echo $lang_common['Required'] ?></strong>
                        <input name="vsab_question" value="<?php echo   $this->get_chosen_question_hash() ?>" type="hidden" />

This calculation is in both functions.
md5($text . date('dmYH') . $pun_config['vsab_salt'])

Perhaps, use create_hash in is_valid_hash. If yes, create_hash will have to be modified to accommodate the previous hour checking.

This could be simplified:

//load in the questions of the question=>answer array
$questions = array_keys($addon_vsab_questions);
//choose random question index and generate hash
$this->chosen_question = $questions[ rand(0, count($questions)-1) ];

to:
$this->chosen_question = array_rand($addon_vsab_questions);

'Fill in the missing letters: Are you a human or a c**puter??' => "om"
Just in case this entry is copied and the admin forgets to add a comma before pasting it after this entry.

Related to this mod or added bonus?

        //if the hidden field username contains something, then it was
        //completed by a bot.
        if(!empty($_REQUEST['username']))
        {
            global $lang_register;
            message($lang_register['No new regs']);
        }

This might not be worth it or make a difference, use sha1 instead of md5 for the hash.

In 2 places, simplify this since it does the same test in load_language_and_questions:

        if (!$this->language_file_loaded)
            $this->load_language_and_questions();

to:

            $this->load_language_and_questions();

For example, in verify_question_answer, it checks for language_file_loaded and then again in load_language_and_questions.

line 239: $questions = array();
Remove since unused in the function.

line 276: <?php if (isset($action) && $action == 'registration') : ?>
$action defaults to empty string in the argument so not necessary to check with isset.

Answer comparison is case-sensitive. I can see it as an additional layer of defense. However, should it be? I can see a user not realizing this and has Caps Lock on in which case the check will fail. Thoughts?

It is set to false initially, but I don't see it toggle to true anywhere else.

Currently, not being used.

    //return the index of the currently chosen question
    //selects a question if not done yet
    function get_chosen_question_index()
    {
        if ($this->chosen_question_index < 0)
            do_choose_question();
        return $this->chosen_question_index;
    }

I am not sure if this is better to do have_to_check_user in register function and only register the applicable hooks. Then you don't need to do this check within the other functions.