0

I have simple comment script that enable to add / edit / remove comments. I have a problem with special characters.

If var currentMessage contains ' or " then the code breaks.

I save the comments in the db as follows :

$Message = clean_data($_POST["txtmessage"]); Then do a typical mysqli_query save ....

function clean_data($input) {
    $input = trim(htmlentities(strip_tags($input,",")));
    if (get_magic_quotes_gpc())
    $input = stripslashes($input);
    $input = stripslashes($input);
    $input = htmlspecialchars($input, ENT_IGNORE, 'utf-8');
    $input = trim($input);
    $sqllink = new mysqli(MySQL_Host, MySQL_UserName, MySQL_Password, MySQL_Database);
    $input = mysqli_real_escape_string($sqllink, $input);
    no_magic_quotes($input);
$input = str_replace("'", "'", $input);
    $input = str_replace("&", "&", $input);
    $input = str_replace('"', """, $input);
    $input = strip_tags($input);
    return $input;
}

The data saved in MySQL is as follows : let's do it !

My edit button is like this:

<button id="BtnEdit_<?php echo $r_c['CommentID']; ?>" class="btn btn-xs btn-warning" onClick="showEditBox(this,<?php echo $r_c['CommentID']; ?>)" name="edit"><i class="fas fa-edit"></i> <? echo _("To Edit"); ?></button>

My message displayed like this:

<div class="message-content"><? echo $r_c['Message']; ?></div>

If I show the special characters in php echo, then the javascript doesn't work. If I remove all ' or "" then it's ok. But it is not practical.

function showEditBox(editobj,id) {
    $('#BtnEdit_'+ id).hide();
    $('#frmAdd').show();

    var currentMessage = $("#message_" + id + " .message-content").html();
    var editMarkUp = '<input type="text" class="form-control input-sm" name="txtmessage" id="txtmessage_'+id+'" value="'+currentMessage+'"><br><div class="btn-group pull-left"><button class="btn btn-xs btn-success" name="ok" onClick="callCrudAction(\'edit\','+id+')"><i class="fas fa-check"></i> <? echo _("Save"); ?></button>&nbsp;&nbsp;&nbsp;&nbsp;<button class="btn btn-xs btn-default" name="cancel" onClick="cancelEdit(\''+currentMessage+'\','+id+')"><i class="fas fa-times"></i> <? echo _("Cancel"); ?></button></div>';
    $("#message_" + id + " .message-content").html(editMarkUp);
}

The data is displayed as follows : let\'s do it ! (it would be better if it would be - let's do it !)

The edit button doesn't even work if there is ' in a comment.

2 Answers 2

1

You would not need all that cleaning with calls to htmlentities, strip_tags, stripslashes, htmlspecialchars, mysqli_real_escape_string or no_magic_quotes. Although you haven't included the code where you insert the data, it shows that you are not inserting it with a prepared statement. And that is exactly what you should go for.

It could look like this:

Writing to the DB in PHP:

$message = $_POST["txtmessage"]; 
$sqllink = new mysqli(MySQL_Host, MySQL_UserName, MySQL_Password, MySQL_Database);
$stmt = $sqllink->prepare("INSERT INTO messages(message) VALUES(?)");
$stmt->bind_param("s", $message);
$stmt->execute();
$stmt->close();

The HTML generation should make sure to use HTML entities where necessary, notably for the ampersand. So call htmlspecialchars here:

<div class="message-content"><? echo htmlspecialchars($r_c['Message']); ?></div>

The JavaScript button click handler should not take the html from the message, but the text. Also, don't set the value of the input in a HTML snippet; instead assign the value with val():

function showEditBox(editobj, id) {
    $('#BtnEdit_'+ id).hide();
    $('#frmAdd').show();

    // Use text():
    var currentMessage = $("#message_" + id + " .message-content").text();
    // Instead of building HTML with string concatenation, build nodes with jQuery.
    // That way you don't have the problem of escaping/encoding
    var nodes = [
        $("<input>").addClass("form-control input-sm").attr({
            type: "text", name: "txtmessage", id: "txtmessage_"+id
        }).val(currentMessage), // <-- here is where the value is set
        $("<br>"),
        $("<div>").addClass("btn-group pull-left").append(
            $("<button>").addClass("btn btn-xs btn-success").attr({ name: "ok" }).append(
                $("<i>").addClass("fas fa-check"),
                "<? echo _("Save"); ?>"
            ).click(callCrudAction.bind(null, 'edit', id))
        ),
        "\xA0\xA0\xA0\xA0", // non-breaking spaces
        $("<button>").addClass("btn btn-xs btn-default").attr({ name: "cancel" }).append(
            $("<i>").addClass("fas fa-times"),
            "<? echo _("Cancel"); ?>"
        ).click(cancelEdit.bind(null, currentMessage, id))
    ];
    $("#message_" + id + " .message-content").empty().append(nodes);
}

I did not change the HTML, but instead of producing non-breaking spaces you should add margin to the buttons with CSS.

1

If you used prepared statements you wouldn't have to corrupt your data like this. And even so, you've stacked up so many conflicting layers of data corruption-- oops I mean "sanitization"-- I would be surprised if this is the first or only problem you're having.

// Validate that the data is the type/size you expect, but don't modify it
$message = basicValidation($_POST["txtmessage"]);

$stmt = $mysqli->prepare('INSERT INTO foo VALUES (?)')
$stmt->bind_param("s", $message);
$stmt->execute();

This protects you from SQL injection and ensures that your data is inserted exactly as specified without having to do any corrupting nonsense with HTML entities, stripslashes, or other things that should only be applied to output.

Retrieving data for display is when you should apply transformations to make it safe to include in HTML or whatever other format you might use. You wouldn't want to inject a bunch of nonsense HTML entities into a PDF document, would you?

$message = mysqli_nonsense();
?><div>
<p><?= htmlentities($message) ?></p>
</div>

This protects you from JavaScript injection.

So much simpler, isn't it?

1
  • Ok, I understand what you propose, but I am using a code that was not started from me. I cannot now change everything. Is there a way to easily accept special characters from this output without redoing everything ? Jan 4, 2019 at 20:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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