Onclick syntax problem when passing two variables, classic asp

Question

My onclick function works beautifully when i pass one variable and doesn't respond once i try to pass two variables. is something wrong with my syntax?

everything supporting this code has been tested and works fine so i'm certain something is wrong with the syntax the onclick line of code. any help would be awesome.

------------onclick code---------------------------------------------------
    Response.Write "<td class=""alt""><input type=""button"" onclick=""deleteRecordAtt(" & AttID &","& StoredPath & " )"" value=""remove"" /></td></tr>"

-----------function reference code in main page---------------------------------------

function deleteRecordAtt(AttID, StoredPath){
if(confirm("This will delect the attachment path"))
{
document.location.href="delete_attachments.asp?EMAIL_LETTERS_HOLD_ID="+AttID+"&RedirURL="+escape(document.location.href);
}
}
-----------function code in source page ----------------------------------------------

<%


Dim AttID, RedirectURL, StoredPath
Dim objConn

AttID=request("EMAIL_LETTERS_HOLD_ID")
RedirectURL=request("RedirURL")


Set objConn = CreateObject("ADODB.Connection")
objConn.Open "DSN=MyDSN"
objConn.Execute("UPDATE EMAIL_SEND_ATTACHMENTS set ATTACHMENTS = Replace(LTRIM(RTRIM(ATTACHMENTS)), '"& StoredPath & "' ,'') WHERE EMAIL_LETTERS_HOLD_ID= "& AttID & " ")
objConn.Close
Set objConn = Nothing

response.redirect RedirectURL
%>

Answer 1

Could it be a problem in the javascript deleteRecordAtt function?
maybe the second parameter is treated as a variable that does not exist?
It would help if you add the Javascript function as well.

Answer 2

onclick=""deleteRecordAtt(" & AttID &","& StoredPath & " )""

Concatenating strings without escaping - never a good idea. Presumably AttID is an integer which is why you get away with that one argument, but StoredPath is a string. As you haven't escaped or wrapped that string, your Write above will end up with HTML code like (assuming 'storedpath' is, for example, a filename):

onclick="deleteRecordAtt(123, file.gif)"

an unquoted 'file.gif' is not parsable as JavaScript of course: a syntax error results. Make sure you have got script errors turned on in your browser so you can see when something like this goes wrong, instead of it just silently failing.

The naive solution is to add wrapping quotes:

onclick=""deleteRecordAtt(" & AttID &", '"& StoredPath & "' )""

which results in:

onclick="deleteRecordAtt(123, 'file.gif')"

which will work. But what if your StoredPath variable has a single quote in it? Or a < or & character - these always need encoding using Server.HTMLEncode() anyway, unless you want cross-site-scripting security holes in your app.

What you would need would be a VBScript function to escape characters into JavaScript string literals, by escaping out-of-band characters into JavaScript \xNN hex character escapes. A simple version would start with something like (untested, I am not a VBScript coder):

<%
    jsLiteral= Replace(StoredPath, "\", "\x5C")
    jsLiteral= Replace(jsLiteral, "'", "\x27")
    jsLiteral= "'" & jsLiteral & "'"
%>
<input ... onclick="deleteRecordAtt(<%= AttID %>, <%= Server.HTMLEncode(jsLiteral) >)">

Edit to add re question edit:

objConn.Execute("UPDATE EMAIL_SEND_ATTACHMENTS set ATTACHMENTS = Replace(LTRIM(RTRIM(ATTACHMENTS)), '"& StoredPath & "' ,'') WHERE EMAIL_LETTERS_HOLD_ID= "& AttID & " ")

Again, concatenating strings without escaping. This one gives you an SQL injection error - a single quote character in your StoredPath variable causes the query to blow up. And if an attacker said something like:

StoredPath=', ''));DROP TABLE EMAIL_SEND_ATTACHMENTS;--

then whoops, goodbye database! More likely is that you'll get hit by one of the many automated SQL injection attacks currently spreading Russian malware around the web.

Escape your SQL string literals or, better, use parameterised queries.

i click yes, and nothing happens

Again make sure JavaScript errors are turned on so you can see any problems. I don't know if it's the issue in this case, but:

document.location.href="delete_attachments.asp?EMAIL_LETTERS_HOLD_ID="+AttID+"&RedirURL="+escape(document.location.href);

should read:

location.href= "delete_attachments.asp?EMAIL_LETTERS_HOLD_ID="+AttID+"&RedirURL="+encodeURIComponent(location.href);

JavaScript's escape() shouldn't ever be used as it is subtly and annoyingly incompatible with proper URL encoding as performed by encodeURIComponent().

Also location - short for window.location - is actually a different object to document.location, and is the correct one to use if you wish to move the browser to a new page. Writing to document.location is not supposed to work, although it might still do on some browsers sometimes if you are lucky.