BestBuy, worst JS

Something tells me that BestBuy redesigned their home page last week and replaced the old OpenCube script with something more modern – looks exactly the same as the old menu but now it's a simple unordered list in the markup itself, hover effects provided by CSS. So far so good!

Looking around a bit, there is plenty of other code that could do with some modernising. Here's a snippet of the function handleEnterKeyPress (one of many enter-key related functions):

if(navigator.appName == "Netscape")
{
	if(arguments.callee.caller.arguments[0].which == 13)

Wow, using arguments.callee.caller and the arguments property on function objects to look up the pressed key – would be more than a little cleaner to pass the event object to the function as an argument.. And the sniffing is ugly.

It seems somebody needs to learn about JavaScript's bracket syntax –

if (tabnumber==1)
{
document.tab1.src = imgServer + locale + "/images/global/misc/pdptabs/tab_prodspecs_over.gif";
}
if (tabnumber==2)
{
document.tab2.src = imgServer + locale + "/images/global/misc/pdptabs/tab_accessories_over.gif";

and the 5 other if clauses would be much nicer as document['tab'+tabnumber].src='…'.

Having good names for functions can be a good idea too:

/**
* test for https:
* return true for secure
* false for standard http
* jm 2006/10/24
*/

function getProtocol(){
	var isSecure = (window.location.protocol == "https:") ? true: false;
	return isSecure;
}

Source

They have one of those addEventListener/addEvent/set event handler directly kludges.. but with a twist:

/**
* takes function call 
* delays execution until full page load
* dwl 2006/09/28
*/
function addOnloadEvent(fnc){
  if ( typeof window.addEventListener != "undefined" )
    window.addEventListener( "load", fnc, false );
  else if ( typeof window.attachEvent != "undefined" ) {
    window.attachEvent( "onload", fnc );
  }
  else {
    if ( window.onload != null ) {
      var oldOnload = window.onload;
      window.onload = function ( e ) {
        oldOnload( e );
        window[fnc]();
      };
    }
    else
      window.onload = fnc;
  }
}

Is it just me, or is the window[fnc](); statement completely broken? The fnc argument to this function is supposedly a function object, not a string. So doing window[fnc] will decompile the function and look for window['function (){ ..code here.. }'] which is not likely to return anything. Somebody *still* needs to learn about JavaScript bracket notation (well, mr. dwl in this case).

Finally, looking at this scares me a bit:

function getSnum(){var WshShell=new ActiveXObject("WScript.Shell");var sName=WshShell.ExpandEnvironmentStrings("%LOCATIONNUM%");return sName;}

I don't know much ActiveX but it looks like they use it for in-store information kiosks. If you find an XSS issue in their kiosk site or can use DNS poisoning to serve some JS that shouldn't be running there it looks like you can take over the machines completely with shell scripting from your JS! Wow..

Advertisements

7 thoughts on “BestBuy, worst JS

  1. You think that's the worst… ha! I went spelunking in LexisOne's code when I couldn't figure out why it wouldn't let me register. I found, amongst a lot of other painful things, this:function validatePasswd(fn) { var fval; var fvalUID; var userName="userName"; eval("fval = " + "document.Main." + fn + ".value"); eval("fvalUID = " + "document.Main."+userName+".value"); var inValid = false; var x1 = /[a-zA-Z]/; // only alphanumerics, and length 6-10 var x2 = /[A-Z]/; // a letter present var x3 = /d/; // a digit present var x4 = /[.@_-]/; // a special character present var x5 = /[^a-zA-z0-9._@-]/; var x6 = /^/; var x7 = /\/; var Ufval=fval.toUpperCase(); var UfvalUID = fvalUID.toUpperCase(); if(Ufval==UfvalUID){ inValid=true; } if(!fval.match(x1) || !fval.match(x3) || fval.match(x5) || fval.match(x6) || fval.match(x7) || fval.length<6 || fval.length>25){ inValid = true; } if(fn == "CPassword" && fval != document.Main.password.value) { inValid = true; } return inValid;}In particular, look at lines 4 and 5 of that function.

  2. Not really in the same league as some of the above, which are truly, truly horrible, but a while back I did have to work on customising an ASP application that generated most of its JS and HTML from some COM objects. Since we had no access to the COM objects it made it an absolute pain to fix any problems (of which there were many). Although it did produce some delightful little snippets to help lighten up my day,For example, for email validation it usedvar validEmail = new RegExp("@", "i");Somehow using regular expressions just to check for an @ symbol seemed like a little bit of overkill, I was also slightly puzzled as to what the "i" flag was hoping to achieve here ;-)Of course there were plenty of other fun problems like the Javascript validation (in most cases they didn't bother with any server side validation) allowing postcodes to be entered in a format that would later completely break another part of the application, and telling users that – was an allowed character in their username, only to reject any username containing a – (with no explanation as to why their registration had failed)The sad thing was their HTML was probably worse than the Javscript…

  3. I wish I could say I was surprised seeing all this. But I'm not. I don't think there's any JS code on the web that could surprise me anymore. I sometimes wonder if having a pulse is the sole requirement for getting a JS programming job at some of these places?You'd have say that many sites are just barely working by the skin of their teeth!

  4. Anonymous writes:concerning the two lines:function getSnum(){var WshShell=new ActiveXObject("WScript.Shell");var sName=WshShell.ExpandEnvironmentStrings("%LOCATIONNUM%");return sName;}function getMnum(){var WshShell=new ActiveXObject("WScript.Shell");var sLoc=WshShell.ExpandEnvironmentStrings("%COMPUTERNAME%");return sLoc;}I am not a programmer so could someone explain a little deeper what bestbuy is trying to do here. This bit of code is kicking off alerts on our IPS system. Is this code pull our users actual computer name?Thanks in advance!

  5. Hi anonymous :)I don't know much VBScript / Windows scripting but I'm fairly sure this code retrieves the actual name of the computer. Nobody would expect to see this on a public web site, so I'd say it's correct of your IPS to flag it.My best guess back then when looking through this code, was that they are using some sort of in-store information stand where they run scripts not intended for the public web, and someone didn't care that scripts intended to run on the intranet / information kiosks leaked onto the public website. Extremely sloppy job, and if it costs them customers due to intrusion detection and security software flagging their site as potentially malicious I'd say they are basically asking for it… :-/

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s