patternphpMinor
PHP contact form using PHPMailer and Google Recaptcha
Viewed 0 times
contactgooglerecaptchaphpusingandformphpmailer
Problem
My company is designing a new site, and because I'm currently on bench I've been tasked with writing any PHP required. Pretty simple stuff, just a contact form and a resume upload page, both using a form to email the details to management. No database, no SQL, no business logic. The designer had already built the bulk of the form, so most of the bootstrap was existing when I started.
After some research, I decided on using PHPMailer and Google Recaptcha. The code works as expected, however, as a junior C#/.net dev, PHP is somewhat new to me. I've trawled through many similar code-review posts, and made improvements where I could, but still have some questions (as would I appreciate an overall review!):
I currently have all of the code in a single file, with the header and footer added through include statements.
contact.php
```
isSMTP();
// Enable SMTP debugging and HTML-friendly debug output
$mail->SMTPDebug = 2;
$mail->Debugoutput = 'html';
$m
After some research, I decided on using PHPMailer and Google Recaptcha. The code works as expected, however, as a junior C#/.net dev, PHP is somewhat new to me. I've trawled through many similar code-review posts, and made improvements where I could, but still have some questions (as would I appreciate an overall review!):
- Is it standard to have simple scripts such as this in the same file as the HTML? I've seen it both ways.
- I've read that using globals is usually not best practice. However, I'm unsure how to set the errors from within the validate_form function otherwise. Is there a better way?
- I stole the construct_error_html function from the old site, but I haven't seen similar examples of it. Is this an adequate way to build error messages? Or should I just print the error message within the HTML like I did with the recaptcha error? (
$recaptcha_error";?>) These messages should rarely show, as I'm using HTML5 validation as well.
- My code (especially the HTML/PHP mix) seems really messy. I tried to standardize formatting, but I'm still not completely happy. Any suggestions for making it look cleaner?
- Security - I've followed along with several security-minded posts, any glaring mistakes?
- I looked into AJAX, but it seems to be overkill for such a small form as this. Opinions?
I currently have all of the code in a single file, with the header and footer added through include statements.
contact.php
```
isSMTP();
// Enable SMTP debugging and HTML-friendly debug output
$mail->SMTPDebug = 2;
$mail->Debugoutput = 'html';
$m
Solution
Huge problem
You have 1 HUGE problem in your code:
If you enter correct captcha - the form is sent. Doesn't matter if it passed validation or not.
It is a logic error in your post processing. You check if form is valid - if it is not - you display error. Then you check if captcha is valid - if it is - you send email.
Bad practice for modern environment
Also, an incoveniece and bad form of web programming is not redirecting the page after the form was submitted.
Take into consideration that some mobile devices automatically refresh pages in browsers in certain situations. So, if you submit form and it sends an email - you can leave the browser, come back later - and it will get sent again.
Bad practice 2
Email spam prevention. Nothing is done to prevent numerous messages to be sent through your form - you don't check that there were say less than 10 messages sent from that IP address or anything like that.
Also, I guess you rely on
Also, someone could insert a fake link into your form field and it would be delivered to the recipient of the email.
More than the syntax and other beauty enhancements you should first ensure that a) your code works and b) your code is safe.
You have 1 HUGE problem in your code:
If you enter correct captcha - the form is sent. Doesn't matter if it passed validation or not.
It is a logic error in your post processing. You check if form is valid - if it is not - you display error. Then you check if captcha is valid - if it is - you send email.
Bad practice for modern environment
Also, an incoveniece and bad form of web programming is not redirecting the page after the form was submitted.
Take into consideration that some mobile devices automatically refresh pages in browsers in certain situations. So, if you submit form and it sends an email - you can leave the browser, come back later - and it will get sent again.
Bad practice 2
Email spam prevention. Nothing is done to prevent numerous messages to be sent through your form - you don't check that there were say less than 10 messages sent from that IP address or anything like that.
Also, I guess you rely on
PHPMailer to avoid someone hijacking fields in your form to add recipients, something like 'subject: header:Message-To: afasdfad@gmail.com' might or might not be catched. Also, someone could insert a fake link into your form field and it would be delivered to the recipient of the email.
More than the syntax and other beauty enhancements you should first ensure that a) your code works and b) your code is safe.
Context
StackExchange Code Review Q#142158, answer score: 3
Revisions (0)
No revisions yet.