Category: Humor

Denilson uses a password manager, like one should. Except there was a router which simply would not let the password manager fill the password field. Sure, Denilson could just copy and paste, but the question of why remained.

And that meant checking the HTML and JavaScript code the router served up. Just pulling up the dev tools brought up all sorts of “fun” discoveries. For example, the application was built in Vue, a front-end framework. But in addition to using Vue, it also used jQuery for some DOM manipulations. But it didn’t just use jQuery. It loaded jquery-3.5.1.slim.min.js directly from its static files. It also loaded vendor.js which also contained the same version of jQuery. At least it was the same version.

While browsing, Denilson found a function called reloadOnF5, which raises an interesting question: isn’t that just what the browser does anyway?

function reloadOnF5(router) {
  document.onkeydown = function (event) {
    if (
      event.key == 'F5' ||
      event.code == 'F5' ||
      event.which == 116 ||
      event.keyCode == 116
    ) {
      event.returnValue = false;
      
	  router.go(router.currentRoute)
    }
  };
}

The best part of this is that at one point they used router.push('/') to navigate, which wouldn’t refresh the page, but simply re-render the root page of the app and add an entry to the browser history.

var MD5 = function(d){result = M();

That they include a minified md5 function isn’t a WTF, but what’s notable is that this is the only piece of their own code that is minified. It’s mixed in with a file that has no other minified functions, which implies that someone copy/pasted this function out of a minified library.

Now, a common piece of validation you might want for a router is requiring inputs to be hexadecimal numbers. Now, for most of us, that’d be a short regex one liner. But if you’re getting paid by the line, you can bloat that out to 30 lines without breaking a sweat:

function isHexaDigit(digit) {
   var hexVals = new Array("0", "1", "2", "3", "4", "5", "6", "7", "8", "9",
                           "A", "B", "C", "D", "E", "F", "a", "b", "c", "d", "e", "f");
   var len = hexVals.length;
   var i = 0;
   var ret = false;

   for ( i = 0; i < len; i++ )
      if ( digit == hexVals[i] ) break;

   if ( i < len )
      ret = true;

   return ret;
}

function isValidHexKey(val, size) {
   var ret = false;
   if (val.length == size) {
      for ( i = 0; i < val.length; i++ ) {
         if ( isHexaDigit(val.charAt(i)) == false ) {
            break;
         }
      }
      if ( i == val.length ) {
         ret = true;
      }
   }

   return ret;
}

None of that explains why Denilson’s password manager didn’t work, but it’s a pretty clear example of the overall level of code quality. Note how in isHexaDigit they are using i as a function-scoped variable, and then use that i to determine if the validation passes- if we found a match before the end of the list, it must be a hex digit.

So why didn’t the password manager work? Well, we have a snippet of the generated DOM, that points at what’s going on:

for="userpassword">Password

Now, you’ll first notice that there are two inputs with the same id. That’s not valid HTML, which is likely enough to throw a password manager for a loop. But it’s actually even worse than that. Notice how one is classed maskPassword and the other is unmaskPassword, and the unmasked one is set to display: none. The way this actually works is that JavaScript intercepts your key events in the maskPassword box, appends the key to the unmaskPassword box, and then replaces the key with a “*” and puts that in the maskPassword box.

So, instead of using input=type="password", which automatically masks a password, they just made up their own version. But this version is special, since it only detects input events, it doesn’t let you do radical things, like use “backspace” or “navigate with arrow keys”. In fact, because that would cause some unexpected behavior, the page actually clears the password box entirely if you use backspace or arrow keys.

Denilson writes:

And this is the quality of code that gets shipped into our homes. This is not the exception, this is the norm.

I think we all understand how bad the UI to pretty much every router is. Not to defend it, but to explain it, most of the router control interfaces for commodity routers are often written by the same networking engineers who are writing the network stack on the router. These are folks who are really good in low-level networking in C, suddenly being tossed a pile of HTML and JavaScript and being told, “Make this work.”

Which is to say, it’s not the programmers’ fault, but the organization that thinks “enh, programmer is programmer, make software go brrrr”.

Thomas worked for a company based in Germany which was looking to expand internationally. Once they started servicing other locales, things started to break. It didn’t take long to track the problem down to a very “percise” numeric parser.

handleInput( value ){
   let value_ = value;
   if( value.substring( 0, 1 ) === '+' ){
      value_ = value.substring( 1 );
   }

   value_ = value_.split( '.' ).join( '' );

   if( this.usePercisionIfPercentage && value_.indexOf( ',' ) >= 0 ) {
      const parsedPreValue = value_.split( ',' )[ 0 ];
      const parsedCommaValue = parseInt( value_.split( ',' )[ 1 ], 10 ) < 10 ?
         parseInt( value_.split( ',' )[ 1 ], 10 ) * 10 : value_.split( ',' )[ 1 ].substring( 0, 2 );

      if( parsedCommaValue === 0 ) {
         value_ = parseInt( parsedPreValue, 10 );
      }
      else {
         const parsedValue = parseInt( parsedPreValue + parsedCommaValue, 10 );
         value_ = parseInt( parsedValue, 10 ) / 100;
      }
   }
   
   
}

We start by checking if the first character of our input value is a “+”, and if it is, we strip it off, storing the result in value_. Then, we split on “.”- the thousands separator in their locale- and join it all back together.

Then we attempt to parse the number, first by checking if this.usePercisionIfPercentage is true, and if the value_ contains a “,”- our decimal separator.

If it does, we split the string, taking the whole numbered portion in one variable, and doing a song and dance to ensure we only grab two characters of the decimal version. The song and dance involves splitting the string multiple times, parsing it into an int multiple times, and a spare ternary for good measure.

Finally, we put the halves of the number back together… by adding them together, taking advantage of string munging to do it. We add parsedPreValue to parsedCommaValue which, because this is JavaScript and parsedPreValue is still a string (despite parsedCommaValue being an integer), we’re concatenating, not adding. We concatenate the values together and divide by 100 to get the “percision” we want.

Notably: if usePercisionIfPercentage is set, and the input has a fractional portion, we end up populating value_ with an integer. But if that’s not true, by the time we hit // do stuff with value_, it’s still a string.

It wasn’t a huge amount of effort for Thomas to strip this out and replace it with a call to a locale-aware number parser. It was much more effort to understand how this code happened in the first place.

Tim H writes:

Some say that OOP was the greatest mistake of all. I say they weren’t trying hard enough.

This code is C++, though Tim submits it as “C with classes.” That usually means “we write it as much like C as possible, but use classes to organize our modules.” In this case, I think it means “we use classes to punish all who read our code”.

Let’s look at an example. They’ve been anonymized, but the shape of the code is there.

class Base {
public:
  enum class Type {
    derived_1,
    derived_2
  };

  Base(Type t) : t_{t} {}
  
  Type getType() const { return t_; }

private:
  Type t_;
};

class Derived_1 : public Base {
public:
  Derived_1() : Base(Base::Type::derived_1) {}
};

This is what one might call “inheritance”. You shouldn’t, but you might. Here, the base class has an enumerated type which declares the possible child classes, and a field to hold that type. The child classes, then, must set that type when they’re constructed.

This is inheritance and polymorphism implemented from first principles, badly. And you can see how badly when it comes time to use the classes:

void Foo(Base *b) {
  if(b->getType() == Base::Type::derived_1) {
    
  }
}

That’s right, they need to check the type field and branch, instead of leveraging polymorphism at all.

But this isn’t the only way they’ve reinvented inheritance. I mean, why limit yourself to just one wrong way of doing things, when you can use two wrong ways of doing things?

class Derived_1;
class Derived_2;

class Base {
public:
  Derived_1* getDerived_1() {
    return dynamic_cast(this);
  }
  Derived_1& getDerived_1_ref() {
    return dynamic_cast(this);
  }
};

class Derived_1 : public Base {}

Here, the base class implements methods to get instances of child classes, or more accurately, pointers (or references) to instances of the child class… by applying the cast to itself. The base class contains logic which casts it to a child class.

Once again, we’ve reinvented a kind of inheritance which requires the base class to know about all its derived classes. I believe the intended use here is that you may have a variable of Base* that is pointing to an instance of Derived_1, and you want to cast it to access the derived class. The problem is that it may be a pointer to Derived_2, so what happens when you ask for getDerived_1()? dynamic_cast will check the runtime type information- if you compiled with RTTI enabled. Otherwise you’re flirting with nasal goblins.

Tim writes:

These “idioms” are inconsistently used everywhere in the codebase. The member function names really do have the “_ref” for the return-a-ref version.

Now, I know that they are running with RTTI enabled, and thus when they do a bad cast, dynamic_cast will throw a bad_cast exception? How do I know?

Double fun is when users report an unreproducible std::bad_cast.

As Tim points out, they do get bad_cast exceptions. And because this is a pile of bad choices and risky code, they can’t trace why it happened or how.

Sometimes, things just blow up.