Lbh
Lbh

Reputation: 67

js - jquery : prevent input from code injection

Context

I'm creating a tiny jQuery script : By clicking on a word, user can erase it and rewrite what he wants.

Original world is in a <span>. When clicked, the <span>is replaced by an <input>. no <form>, no submit button, only this <input>.

My problem

When pressing enter key to validate the <input>, the rest of my code works well, except for javascript injection :

<input type="text" value="<script>alert('Evil Script')</script>">

My question

What is the best method to prevent my <input> from executing this evil script when pressing enter key?

Please forgive my ingenuous question, and thank you for your advice.

My wrong code

jQuery(document).ready(function($) {

    initNom();

    function initNom() {
        var nomPerso = $('#nomPerso');
        var cheminNom = localStorage.getItem('userName');

        montrerNom();

        nomPerso.on('click', function() {
            $(this).replaceWith(
                    $('<input id="nomPerso" type="text" value="" placeholder="' + nomPerso.text() + '">')
            );
            $("#nomPerso")
                    .focus()
                    .on('keypress', function (e) {
                        if(e.which === 13) {

                            e.preventDefault();
                            //Disable textbox to prevent multiple submit
                            $(this).attr("disabled", "disabled");

                            sauverNom();
                            montrerNom();

                            $(this).replaceWith($('<span id="nomPerso">' + localStorage.getItem('userName') + '</span>'));

                            $(this).removeAttr("disabled");

                            initNom();

                        }
                    });
        });

        function sauverNom() {
            var nom = $('#nomPerso').val();

            if (typeof(Storage) !== "undefined" && nom) {
                localStorage.setItem('userName', nom);
            }
        }

        function montrerNom() {
            if (!cheminNom) {
                nomPerso.text('{Inserer_nom}');
            } else {
                nomPerso.text(cheminNom);
            }
        }
    }
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

<div id="nomPerso"><script>alert('Evil Script')</script></div>

Also on CodePen: https://codepen.io/LaBriqueHurlante/pen/mwxjRL

Upvotes: 2

Views: 5266

Answers (1)

Alon Eitan
Alon Eitan

Reputation: 12025

Just replace the content with .text() instead of .html() so it will escape all the html tags and display them as HTML entities.

The first example will convert the script tags to html entities, so it will be added to the DOM as

&lt;script&gt;alert('Evil Script')&lt;/script&gt;

GOOD / SAFE Example:

$(function() {
  $('span').on('click', function() {
    $(this).text( $('#replace').val() ); // GOOD
    
    //$(this).html( $('#replace').val() ); // BAD
  });
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<h3>Click on a word:</h3>
<hr>
<p>
  <span>Hello</span> <span>World</span>
</p>

<input type="text" id="replace" value="<script>alert('Evil Script')</script>" />

BAD / DANGEROUS example

$(function() {
  $('span').on('click', function() {
    // $(this).text( $('#replace').val() ); // GOOD
    
    $(this).html( $('#replace').val() ); // BAD
  });
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<h3>Click on a word:</h3>
<hr>
<p>
  <span>Hello</span> <span>World</span>
</p>

<input type="text" id="replace" value="<script>alert('Evil Script')</script>" />

Now, with your code that you added on your last edit, change:

$(this).replaceWith(
    $('<input id="nomPerso" type="text" value="" placeholder="' + nomPerso.text() + '">')
);

With:

$(this).replaceWith(
    $('<input id="nomPerso" type="text" value="" placeholder="">').attr('placeholder', nomPerso.text())
);

And also change:

$(this).replaceWith($('<span id="nomPerso">' + localStorage.getItem('userName') + '</span>'));

With:

$(this).replaceWith(
    $('<span id="nomPerso"></span>').text( localStorage.getItem('userName') )
);

Those changes will ensure that you're adding elements, and setting their properties with escaped html entities, instead of dumping them unsafely to the DOM: https://codepen.io/anon/pen/JJLBLy

Please note that the first time you run the fiddle it will display the alert because it is embedded in the HTML code itself, but when you change the text to <script>alert('Evil Script')</script> it will be displayed as actual text.

Upvotes: 2

Related Questions