Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

By utilizing an existing PHP encryption library (defuse/php-encryption) and a flat file database library (jamesmoss/flywheel), my application take a secret message from a user, encrypts it using a key derived from a password, and saves it to a flat file. The message can then be decrypted by visiting a unique link and providing the same password. After the message is viewed it is automatically deleted.

Encryption (encrypt.php):

<?php
require_once 'vendor/autoload.php';
require_once 'config.php';

// Configure the data store
$config = new \JamesMoss\Flywheel\Config(REPO_DIR);
$repo = new \JamesMoss\Flywheel\Repository('secrets', $config);

function response($msg, $error){

    $response_array = array(
        'msg' => $msg,
        'error' => $error
    );

    // Return a json object to the requesting page
    header('Content-type: application/json');
    die(json_encode($response_array));

}

// Validation checks
if ($_SERVER['REQUEST_METHOD'] == "POST") {

    $continue = true;

    // Validation: check if it's an ajax request
    if(!isset($_SERVER['HTTP_X_REQUESTED_WITH']) AND strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) != 'xmlhttprequest') {
        $continue = false;
        response('<strong>Hold on there...</strong> Submission must be an Ajax POST request.', true);
    } 

    // Validation: check if any of the fields aren't set
    if((!isset($_POST['ot_secret']))
       || (!isset($_POST['ot_encrypt_password']))
       || (!isset($_POST['ot_encrypt_password_confirm']))
       || (!isset($_POST['ot_email']))
      ){
        $continue = false;
        response('<strong>Hold on there...</strong> All fields are required.', true);
    } else {
        $secret = filter_var($_POST['ot_secret'], FILTER_SANITIZE_STRING);
        $password = $_POST['ot_encrypt_password'];
        $password_confirm = $_POST['ot_encrypt_password_confirm'];
        $email = filter_var($_POST['ot_email'], FILTER_SANITIZE_EMAIL);
    }         

    // Validation: check if any of the fields are blank
    if((empty($secret)) || (empty($password)) || (empty($password_confirm)) || (empty($email))){
        $continue = false;
        response('<strong>Hold on there...</strong> All fields are required.', true);
    }                      

    // Validation: check if passwords is long enough
    if(strlen($password) < 8) {
        $continue = false;
        response('<strong>Hold on there...</strong> Your password is not long enough.', true);
    }

    // Validation: check if passwords match
    if($password !== $password_confirm) {
        $continue = false;
        response('<strong>Hold on there...</strong> Your passwords do not match.', true);
    }       

    // Validation: check for proper email format
    if(!filter_var($email, FILTER_VALIDATE_EMAIL)){
        $continue = false;
        response('<strong>Hold on there...</strong> Please provide a valid email address.', true);
    }   

}

// If all of the above validation checks pass, continue on
if ((isset($continue)) && ($continue === true)) {

    // Create random encryption key
    $iterations = 10000;
    $salt = mcrypt_create_iv(16, MCRYPT_DEV_URANDOM);
    $key = hash_pbkdf2("sha256", $password, $salt, $iterations, 16);

    // Encrypt secret message, reference: https://github.com/defuse/php-encryption/blob/master/example.php
    try {
        $ciphertext = Crypto::Encrypt($secret, $key);
    } catch (CryptoTestFailedException $ex) {
        response('<strong>Hold on there...</strong> Cannot safely perform encryption.', true);
    } catch (CannotPerformOperationException $ex) {
        response('<strong>Hold on there...</strong> Cannot safely perform decryption.', true);
    }       

    // Save the data
    $secret = new \JamesMoss\Flywheel\Document(array(
        'salt' => base64_encode($salt),
        'secret' => base64_encode($ciphertext),
        'createdAt' => time()
    ));

    $repo->store($secret);

    // Send email to recipient using SendGrid API
    $sendgrid = new SendGrid(API_KEY_SENDGRID);
    $sendemail    = new SendGrid\Email();

    $message = '<p>A secret message has been sent to you.</p>
                <p>Access it at: ' . URL . '/?id=' . $secret->getId() . '</p>
                <p>Thank you!</p>';

    $sendemail->addTo($email)
        ->setFrom(EMAIL_FROM_ADDRESS)
        ->setSubject(EMAIL_SUBJECT)
        ->setHtml($message);

    //Provide response
    try {
        $sendgrid->send($sendemail);
        response('<strong>Message sent!</strong> Your secret message has been sent to ' . $email . '.', false);
    } catch(\SendGrid\Exception $e) {
        foreach($e->getErrors() as $er) {
            response('<strong>Hold on there...</strong> ' . $er, true);
        }
    }

} else {
    die('Access Denied.');
}

Decryption (decrypt.php):

<?php
require_once 'vendor/autoload.php';
require_once 'config.php';

// Configure the data store
$config = new \JamesMoss\Flywheel\Config(REPO_DIR);
$repo = new \JamesMoss\Flywheel\Repository('secrets', $config);

function response($msg, $error){

    $response_array = array(
        'msg' => $msg,
        'error' => $error
    );

    // Return a json object to the requesting page
    header('Content-type: application/json');
    die(json_encode($response_array));

}

// Validation checks
if ($_SERVER['REQUEST_METHOD'] == "POST") {

    $continue = true;

    // Validation: check if it's an ajax request
    if(!isset($_SERVER['HTTP_X_REQUESTED_WITH']) AND strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) != 'xmlhttprequest') {
        $continue = false;
        response('<strong>Hold on there...</strong> Submission must be an Ajax POST request.', true);
    } 

    // Validation: check if any of the fields aren't set
    if((!isset($_POST['ot_id'])) || (!isset($_POST['ot_decrypt_password']))){
        $continue = false;
        response('<strong>Hold on there...</strong> All fields are required.', true);
    } else {
        $id = $_POST['ot_id'];
        $password = $_POST['ot_decrypt_password'];
    }          

    // Validation: check if any of the fields are blank
    if((empty($id)) || (empty($password))){
        $continue = false;
        response('<strong>Hold on there...</strong> All fields are required.', true);
    }

    // Validation: check if message ID is too long
    if(strlen($password) > 9) {
        $continue = false;
        response('<strong>Hold on there...</strong> Your message ID is too long.', true);
    }   

    // Validation: check if message exists
    if($repo->findById($_POST["ot_id"]) === false) {
        $continue = false;
        response('<strong>Hold on there...</strong> The message ID you entered cannot be found.', true);
    } else {
        $secret = $repo->findById($id);
    }

}

// If all of the above validation checks pass, continue on
if ((isset($continue)) && ($continue === true)) {

    // Decrypt encyption key using salt and password
    $iterations = 10000;
    $salt = base64_decode($secret->salt);
    $key = hash_pbkdf2("sha256", $password, $salt, $iterations, 16);    

    // Decrypt message using decrypted key, reference: https://github.com/defuse/php-encryption/blob/master/example.php
    try {
        $decrypted = Crypto::Decrypt(base64_decode($secret->secret), $key);
    } catch (InvalidCiphertextException $ex) { // VERY IMPORTANT
        response('<strong>Hold on there...</strong> Something\'s wrong, please double check your password.', true);
    } catch (CryptoTestFailedException $ex) {
        response('<strong>Hold on there...</strong> Cannot safely perform encryption.', true);
    } catch (CannotPerformOperationException $ex) {
        response('<strong>Hold on there...</strong> Cannot safely perform decryption.', true);
    }           

    // Delete message
    $repo->delete($id);

    // Provide response
    response($decrypted, false);

} else {
    die('Access Denied.');
}

AJAX (script.js):

$(function() {

    // Connect to encrypt.php and return response
    $('#form_encrypt').validator().on('submit', function(e) {
        if (!e.isDefaultPrevented()) {
            var formData = $(this).serialize();
            $.ajax({
                type: "POST",
                dataType: "json",
                url: "encrypt.php",
                data: formData,
                success: function(data) {
                    if (!data.error) {
                        $("form").trigger("reset");
                        $("#results").removeClass().empty().addClass("alert alert-success fade in").html(data.msg);
                    } else {
                        $("#results").removeClass().empty().addClass("alert alert-danger fade in").html(data.msg);
                    }
                },
                error: function(xhr, status, error) {
                    $("#results").removeClass().empty().addClass("alert alert-danger fade in").html('<strong>Hold on there...</strong> An internal error has occured.');
                }
            });
            e.preventDefault();
        }
    });

    // Connect to decrypt.php and return response
    $('#form_decrypt').validator().on('submit', function(e) {
        if (!e.isDefaultPrevented()) {
            var formData = $(this).serialize();
            $.ajax({
                type: "POST",
                dataType: "json",
                url: "decrypt.php",
                data: formData,
                success: function(data) {
                    if (!data.error) {
                        $("form").trigger("reset");
                        $(".nav, .tab-content").remove();
                        $("#results").removeClass().empty().html("<pre>" + data.msg + "</pre>");
                    } else {
                        $("#results").removeClass().empty().addClass("alert alert-danger fade in").html(data.msg);
                    }
                },
                error: function(xhr, status, error) {
                    $("#results").removeClass().empty().addClass("alert alert-danger fade in").html('<strong>Hold on there...</strong> An internal error has occured.');
                }
            });
            e.preventDefault();
        }
    });

});

Front-end (index.php):

<?php
require_once 'vendor/autoload.php';
require_once 'config.php';

// Determine if a message is being accessed from link
if(isset($_GET["id"]) && (!empty($_GET["id"]))){
    $fromLink = true;
} else {
    $fromLink = false;
}

?>
<!DOCTYPE html>
<html lang="en">

    <head>
        <meta charset="utf-8">
        <meta http-equiv="X-UA-Compatible" content="IE=edge">
        <meta name="viewport" content="width=device-width, initial-scale=1">
        <title>OneTime</title>
        <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/css/bootstrap.min.css">
        <link rel="stylesheet" href="assets/css/styles.css">
        <!--[if lt IE 9]>
        <script src="https://oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js"></script>
        <script src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script>
        <![endif]-->
    </head>

    <body>

        <div class="container">

            <div class="col-md-6 col-md-offset-3">

                <div class="page-header text-center">
                    <h1>OneTime</h1>
                </div>

                <div id="results"></div>

                <!--LINKS-->
                <ul class="nav nav-tabs" role="tablist">
                    <li role="presentation" class="<?php if(!$fromLink){echo 'active';}?>">
                        <a href="#encrypt" aria-controls="encrypt" role="tab" data-toggle="tab">Encrypt</a>
                    </li>
                    <li role="presentation" class="<?php if($fromLink){echo 'active';}?>">
                        <a href="#decrypt" aria-controls="decrypt" role="tab" data-toggle="tab">Decrypt</a>
                    </li>
                </ul>

                <!--TABS-->
                <div class="tab-content">

                    <!--ENCRYPT-->
                    <div role="tabpanel" class="tab-pane <?php if(!$fromLink){echo 'active';}?>" id="encrypt">
                        <form id="form_encrypt">
                            <div class="form-group">
                                <label for="ot_secret">Your secret message:</label>
                                <textarea class="form-control" id="ot_secret" name="ot_secret" rows="3" required></textarea>
                                <span class="help-block with-errors">Plain text only.</span>
                            </div>
                            <div class="form-group">
                                <label for="ot_encrypt_password">Add a complex password to your message:</label>
                                <div class="form-inline row">
                                    <div class="form-group col-sm-6">
                                        <input type="password" class="form-control" id="ot_encrypt_password" name="ot_encrypt_password" placeholder="Password" required data-minlength="8">
                                        <span class="help-block">Minimum of 8 characters.</span>
                                    </div>
                                    <div class="form-group col-sm-6">
                                        <input type="password" class="form-control" id="ot_encrypt_password_confirm" name="ot_encrypt_password_confirm" placeholder="Confirm Password" required data-match="#ot_encrypt_password">
                                        <span class="help-block with-errors"></span>
                                    </div>
                                </div>
                            </div>
                            <div class="form-group">
                                <label for="ot_email">Who should we send the message link to?</label>
                                <input type="email" class="form-control" id="ot_email" name="ot_email" placeholder="[email protected]" required>
                                <span class="help-block with-errors"></span>
                            </div>
                            <button type="submit" class="btn btn-primary btn-lg btn-block"><span class="glyphicon glyphicon-lock" aria-hidden="true"></span> Encrypt Secret Message</button>
                        </form>
                    </div>

                    <!--DECRYPT-->
                    <div role="tabpanel" class="tab-pane <?php if($fromLink){echo 'active';}?>" id="decrypt">
                        <form id="form_decrypt">
                            <div class="form-group">
                                <label for="ot_id">Message ID</label>
                                <input type="text" class="form-control" id="ot_id" name="ot_id" placeholder="o8AZv0hGh" required  maxlength="9" value="<?php if($fromLink){echo $_GET["id"];}?>">
                            </div>
                            <div class="form-group">
                                <label for="ot_decrypt_password">Password</label>
                                <input type="password" class="form-control" id="ot_decrypt_password" name="ot_decrypt_password" placeholder="Password" required>
                            </div>
                            <button type="submit" class="btn btn-primary btn-lg btn-block">Decrypt Secret Message</button>
                        </form>
                    </div>
                </div>

                <hr />

                <!--COPYRIGHT-->
                <p class="small text-center">&copy; <?php echo date('Y'); ?> <a href="index.php">OneTime</a>. All rights reserved.</p>

            </div>

        </div>

        <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js"></script>
        <script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/js/bootstrap.min.js"></script>
        <script src="assets/js/validator.js"></script>
        <script src="assets/js/script.js"></script>

    </body>

</html>

I am hoping to get feedback on all aspects of my project - code style, proper PHP/AJAX use, validation/sanitization of user input, and any other security considerations I should be following. For the purpose of this project, I'm going to assume that the people maintaining the php-encryption library know way more about encryption than I ever will, so I'm not really looking for feedback on their library. Similarly, I know a flat file database isn't ideal, but it was the easiest to get up and running - if I were to ever move this to a production server I would likely utilize a more traditional database.

share|improve this question

A few things to suggest:

  • You should move all your <strong></strong> returned messages to a config file, rather than keeping them inline.
  • In the following code block, you can return the condition instead of the if-else statement:

    if(isset($_GET["id"]) && (!empty($_GET["id"]))){
        $fromLink = true;
    } else {
        $fromLink = false;
    }
    

    into:

    $fromLink = (isset($_GET["id"]) && (!empty($_GET["id"])));
    
  • I would avoid having the MAGIC NUMBER \$16\$ in the following line, you should move it to a variable and declare it's purpose, just like $iterations:

    hash_pbkdf2("sha256", $password, $salt, $iterations, 16);
                                                         ^
    
  • In the future, you wouldn't need the === (triple equals) in the following line, you can just use ==, but the whole line is wrong anyway. You test if $continue isset, and then test whether it's true. If it was true, it would be set. Also, comparing a boolean explicitly to ==/=== true is really bad.

    if ((isset($continue)) && ($continue === true)) {
    

    into:

    if ($continue){
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.