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

One-time secret message application using PHP-encryption library

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

Problem

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):

```
$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('Hold on there... 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('Hold on there... 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('Hold on there... All fields are required.', true);
}

// Validation: check if passwords is long enough
if(strlen($password) Hold

Solution

A few things to suggest:

  • You should move all your ` 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){

Code Snippets

if(isset($_GET["id"]) && (!empty($_GET["id"]))){
    $fromLink = true;
} else {
    $fromLink = false;
}
$fromLink = (isset($_GET["id"]) && (!empty($_GET["id"])));
hash_pbkdf2("sha256", $password, $salt, $iterations, 16);
                                                     ^
if ((isset($continue)) && ($continue === true)) {
if ($continue){

Context

StackExchange Code Review Q#96509, answer score: 3

Revisions (0)

No revisions yet.