modxcms / Quip

A commenting component for MODX Revolution
https://modx.com/extras/package/quip
4 stars 16 forks source link

undefined index in ThreadReply.php (and undefined variable recaptcha) #30

Open smxi opened 2 years ago

smxi commented 2 years ago

Thanks for doing Quip.

While testing with strict full php error reporting, I found this issue in quip/controllers/web/ThreadReply.php

Notice: Undefined index: name in /modx/components/quip/controllers/web/ThreadReply.php on line 188
Notice: Undefined index: email in /modx/components/quip/controllers/web/ThreadReply.php on line 189
Notice: Undefined index: website in /modx/components/quip/controllers/web/ThreadReply.php on line 190
        $fields['name']    = strip_tags($fields['name']);
        $fields['email']   = strip_tags($fields['email']);
        $fields['website'] = strip_tags($fields['website']);

should be:

if (isset($fields['name'])){        
        $fields['name']    = strip_tags($fields['name']);
}
if (isset($fields['email'])){  
        $fields['email']   = strip_tags($fields['email']);
}
if (isset($fields['website'])){  
        $fields['website'] = strip_tags($fields['website']);
}

Obviously no need to strip tags off an unset index value.

And also one more in: Notice: Undefined variable: hook in /modx/cache/includes/elements/modsnippet/50.include.cache.php on line 48

Snippet 50 is: Renders ReCaptcha V2 form: recaptchav2_render

Again, a test without checking if is set:

if ($hook) { 
    $hook->setValue('recaptchav2_html', $recaptcha_html); // This won't re-render on page reload there's validation errors
    return true;
} else { // This works at least
    return $recaptcha_html;
}

Should be:

if (isset($hook)) { 
    $hook->setValue('recaptchav2_html', $recaptcha_html); // This won't re-render on page reload there's validation errors
    return true;
} else { // This works at least
    return $recaptcha_html;
}

I wasn't getting these error/warnings on my local system even with all error reporting turned on, but I was getting it on a production server, not sure why the difference.

smxi commented 2 years ago

I should add, there are more of these undefined index popping up, all based on trying to test undefined indexes, I may work iup a more complete list and patch if work will pay me for that.