HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Basic contact form

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
contactformbasic

Problem

This is for commercial purposes. It's not going to a database, but will just be emailed to a person via phpMailer.

I am not good at PHP (truly awful). I'd like to know whether this is good enough for going production or not.


  
  
    
    
      
        Toggle navigation
        
        
        
      
      New Daimaru Hotel345 E 1st Street Los Angeles, CA 90012
    

    
    
      
        About
        Rooms & information
        Book to Reserve
        Location Info
        Additional Info
        Contact
      
    
    
        English
        Japanese
    
  

  ">

  " />
    
      First Name
      
      ">
       * ' . $firstNameErr . ''); ?>
      
    
    
      Last Name
      
      ">
       * ' . $lastNameErr . ''); ?>
     
    
      
      Email
      
      ">
       * ' . $emailErr . ''); ?>
      
    
    
      Message
      
      ">
       * ' . $messageErr . ''); ?>
      
    

    
    
      Submit
    
  


Here's the page that does the actual mailing:

``
SMTPDebug = 3; // Enable verbose debug output

$mail->isSMTP(); // Set mailer to use SMTP
$mail->Host = 'hosting_info'; // Specify main and backup SMTP servers
$mail->SMTPAuth = true; // Enable SMTP authentication
$mail->Username = 'testy@URL.com'; // SMTP username
$mail->Password = 'ugh'; // SMTP password
$mail->SMTPSecure = 'tls'; // Enable TLS encryption,
ssl` also accepted
$mail->Port = 587;

$mail->addReplyTo( $email, $first_name );
$mail->addAddress( $email, $first_name );
$mail->addAddress( 'my_email_address', 'Staff' );
$mail->From = 'my_email_address';
$mail->FromName = 'Staff';

$mail->isHTML(true); // Set email format to HTML

$mail->Subject = 'Contact';
$mail->Body = "First Name: $first_name"
."Last Name: $last_nam

Solution

Input Validation

The test_input function seems to be from here, and it's really not a good function to use (see here and here).

In short: it isn't recommended defense against anything (the recommendation for XSS are to HTML encode when outputting data, not when receiving it, and the function doesn't provide defense against anything else), and it makes your data dirty, leading to usability problems.

Misc

  • Your indentation is off, and your bracket positioning is inconsistent, making your code harder to read.



  • Using a new CSRF token for each request is good for security, but not that good for usability (breaks back button, etc). Evaluate if this is really necessary, and if not, use a token per session.



  • Having file names such as contact9SessionsCRSF2 or contactResult2 is a sign that you have files in your code base which are not actually used, but kept "just in case". This can easily lead to confusion, think about deleting them and removing the 2 from the file names, and/or use version control.



  • The token generation and setting is duplicated code, which is not a good idea as it makes it hard to change and clutters up your code. You could extract it into a generateAndSetToken function.

Context

StackExchange Code Review Q#115296, answer score: 2

Revisions (0)

No revisions yet.