The Daily WTF

http://thedailywtf.com/

CodeSOD: Magical Bytes

Mon, 25 Nov 2024 06:30:00 GMT

Link

"Magic bytes" are a common part of a file header. The first few bytes of a file can often be used to identify what type of file it is. For example, a bitmap file starts with "BM", and a PGM file always starts with "PN" where "N" is a number between 1 and 6, describing the specific variant in use, and WAV files start with "RIFF".

Many files have less human-readable magic bytes, like the ones Christer was working with. His team was working on software to manipulate a variety of different CAD file types. One thing this code needed to do is identify when the loaded file was a CAD file, but not the specific UFF file type they were looking for. In this case, they need to check that the file does not start with 0xabb0, 0xabb1, or 0xabb3. It was trivially easy to write up a validation check to ensure that the files had the correct magic bytes. And yet, there is no task so easy that someone can't fall flat on their face while doing it.

This is how Christer's co-worker solved this problem:

const uint16_t *id = (uint16_t*)data.GetBuffer();
if (*id == 0xabb0 || *id == 0xABB0 || *id == 0xabb1 || *id == 0xABB1 || *id == 0xabb3 || *id == 0xABB3)
{
    return 0;
}

Here we have a case of someone who isn't clear on the difference between hexadecimal numbers and strings. Now, you (and the compiler) might think that 0xABB0 and 0xabb0 are, quite clearly, the same thing. But you don't understand the power of lowercase numbers. Here we have an entirely new numbering system where 0xABB0 and 0xabb0 are not equal, which also means 0xABB0 - 0xabb0 is non-zero. An entirely new field of mathematics lies before us, with new questions to be asked. If 0xABB0 < 0xABB1, is 0xABB0 < 0xabb1 also true? From this little code sample, we can't make any inferences, but these questions give us a rich field of useless mathematics to write papers about.

The biggest question of all, is that we know how to write lowercase numbers for A-F, but how do we write a lowercase 3?

https://thedailywtf.com/images/inedo/buildmaster-icon.png

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

Error'd: Three Little Nyms

Fri, 22 Nov 2024 06:30:00 GMT

Link

"Because 9.975 was just a *little* bit too small," explains our first anonymous helper.

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/247/skrmbild20241120100957.png

Our second anonymous helper tells us "While looking up how to find my banks branch using a blank check, I came across this site that seems to have used AI to write their posts. Didn't expect to learn about git while reading about checks. I included the navbar because its just as bad."

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/247/screenshot20241119093625.png

Our third anonymous helper snickered "I guess I was just a bit over quota." Nicely done.

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/247/screenshot20241114at2.39.42pm.png

Our fourth anonymous helper isn't actually anonymous, alas. He signed off as the plausibly-named Vincent R, muttering "I dunno, it's all Greek to me. Or at least it *was* Greek until Firefox thoughtfully translated all the lambdas and mus and sigmas in these probability formulas..."

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/247/math.png

Finally for Friday, the fifth from Dan W. "On my way to the airport, I checked my route on the Trainline app. I think I'll have just enough time to make this connection in Wolverhampton." Walk, don't run.

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/247/screenshot_20241116133730.png

https://thedailywtf.com/images/inedo/proget-icon.png

[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.

CodeSOD: Contact Us

Thu, 21 Nov 2024 06:30:00 GMT

Link

Charles is supporting a PHP based application. One feature of the application is a standard "Contact Us" form. I'll let Charles take on the introduction:

While it looks fine on the outside, the code is a complete mess. The entire site is built with bad practices, redundant variables, poor validation, insecure cookie checks, and zero focus on maintainability or security. Even the core parts of the platform are a nightmare

We're going to take this one in chunks, because it's big and ugly.

try {
    if (isset($_POST)) {
        $name = $_POST['objName'];
        $lst_name = $_POST['objLstName'];
        $email = $_POST['objEmail'];
        $phone = $_POST['objGsm'];
        $message = $_POST['objMsg'];
        $verifycode = $_POST['objVerifyCode'];
        /******************************************************/
        $objCmpT = $_POST['objCmpT'];
        $objCmpS = $_POST['objCmpS'];
        $objCountry = $_POST['objCountry'];
        $objCity = $_POST['objCity'];
        $objName2 = $_POST['objName2'];
        $objLstName2 = $_POST['objLstName2'];
        $objQuality = $_POST['objQuality'];
        $objEmail = $_POST['objEmail'];
        $objMsg2 = $_POST['objMsg2'];
        $objVerifyCode2 = $_POST['objVerifyCode2'];

I don't love that there's no structure or class here, to organize these fields, but this isn't bad, per se. We have a bunch of form fields, and we jam them into a bunch of variables. I am going to, with no small degree of willpower, not comment on the hungarian notation present in the field names. Look at me not commenting on it. I'm definitely not commenting on it. Look at me not commenting that some, but not all, of the variables also get the same hungarian prefix.

What's the point of hungarian notation when everything just gets the same thing anyway; like hungarian is always bad, but this is just USELESS

Ahem.

Let's continue with the code.

        $ok = 0;
        $ok2 = 0;
        $sendTo = "example@example.com";
        $golableMSG = '
        -First Name & Last Name :' . $name . ' ' . $lst_name . '
        -email :' . $email . '
        -Phone Number : 0' . $phone . '
        -Message : ' . $message;
        $globaleMSG2 = '
        -First Name & Last Name :' . $objName2 . ' ' . $objLstName2 . '
        -Email :' . $objEmail . '
        -Type of company : ' . $objCmpT . '
        -Sector of activity : ' . $objCmpS . '
        -Country : ' . $objCountry . '
        -City : ' . $objCity . '
        -Your position within the company : ' . $objQuality . '
        -Message : ' . $objMsg2;

We munge all those form fields into strings. These are clearly going to be the bodies of our messages. Only now I'm noticing that the user had to supply two different names- $name and $objName2. Extra points here, as I believe they meant to name both of these message variables globaleMSG but misspelled the first one, golableMSG.

Well, let's continue.

        if (!$name) {
            $data['msg1'] = '*';
        } else {
            $ok++;
            $data['msg1'] = '';
        }
        if (!$lst_name) {
            $data['msg2'] = '*';
        } else {
            $ok++;
            $data['msg2'] = '';
        }
        if (!$email) {
            $data['msg3'] = '*';
        } else {
            $ok++;
            $data['msg3'] = '';
        }
        if ($phone <= 0) {
            $data['msg4'] = '*';
        } else {
            $ok++;
            $data['msg4'] = '';
        }
        if (!$message) {
            $data['msg5'] = '*';
        } else {
            $ok++;
            $data['msg5'] = '';
        }
        if (!$verifycode) {
            $data['msg6'] = '*';
        } else {
            $ok++;
            $data['msg6'] = '';
        }
        /*********************************************************************************/
        if (!$objCmpS) {
            $data['msg7'] = '*';
        } else {
            $ok2++;
            $data['msg7'] = '';
        }
        if (!$objCountry) {
            $data['msg8'] = '*';
        } else {
            $ok2++;
            $data['msg8'] = '';
        }
        if (!$objCity) {
            $data['msg9'] = '*';
        } else {
            $ok2++;
            $data['msg9'] = '';
        }
        if (!$objName2) {
            $data['msg10'] = '*';
        } else {
            $ok2++;
            $data['msg10'] = '';
        }
        if (!$objLstName2) {
            $data['msg11'] = '*';
        } else {
            $ok2++;
            $data['msg11'] = '';
        }
        if (!$objQuality) {
            $data['msg12'] = '*';
        } else {
            $ok2++;
            $data['msg12'] = '';
        }
        if (!$objMsg2) {
            $data['msg13'] = '*';
        } else {
            $ok2++;
            $data['msg13'] = '';
        }
        if (!$objVerifyCode2) {
            $data['msg14'] = '*';
        } else {
            $ok2++;
            $data['msg14'] = '';
        }

What… what are we doing here? I worry that what I'm looking at here is some sort of preamble to verification code. But why is it like this? Why?

        /********************************************************************************/
        if ($ok == 6) {
            if (preg_match("/^[ a-z,.+!:;()-]+$/", $name)) {
                $data['msg1_1'] = '';
                if (preg_match("/^[ a-z,.+!:;()-]+$/", $lst_name)) {
                    $data['msg2_2'] = '';
                    $subject = $name . " " . $lst_name;
                    if (filter_var($email, FILTER_VALIDATE_EMAIL)) {
                        $data['msg3_3'] = '';
                        $from = $email;
                        if (preg_match("/^[6-9][0-9]{8}$/", $phone)) {
                            $data['msg4_4'] = '';
                            if (intval($verifycode) == intval($_COOKIE['nmbr1']) + intval($_COOKIE['nmbr2'])) {
                                $data['msg6_6'] = '';
                                $headers = 'From: ' . $from . "\r\n" .
                                    'Reply-To: ' . $sendTo . "\r\n" .
                                    'X-Mailer: PHP/' . phpversion();
                                mail($sendTo, $subject, $golableMSG, $headers);
                                $data['msgfinal'] = 'Votre Messsage est bien envoyer';
                                /*$data = array('success' => 'Votre Messsage est bien envoyer', 'postData' => $_POST);*/
                            } else {
                                $data['msg6_6'] = 'votre resultat est incorrect';
                            }
                        } else {
                            $data['msg4_4'] = 'Votre Numéro est incorrect';
                        }
                    } else {
                        $data['msg3_3'] = 'Votre Email est incorrect';
                    }
                } else {
                    $data['msg2_2'] = 'Votre Prénom est Incorrect';
                }
            } else {
                $data['msg1_1'] = 'Votre Nom est Incorrect';
            }
        }

Oh look, it is validation code. Their verification code system, presumably to prevent spamming messages, is not particularly secure or useful. The real thing I see here, though, is the namespaced keys. Earlier, we set $data['msg1'], and now we're setting $data['msg1_1'] which is a code stench that could kill from a hundred yards.

And don't worry, we do the same thing for the other message we send:

        /**************************************************************/
        if ($ok2 == 8) {
            if (preg_match("/^[ a-z,.+!:;()-]+$/", $objName2)) {
                $data['msg10_10'] = '';
                if (preg_match("/^[ a-z,.+!:;()-]+$/", $objLstName2)) {
                    $data['msg11_11'] = '';
                    $subject2 = $objName2 . " " . $objLstName2;
                    if (intval($objVerifyCode2) == intval($_COOKIE['nmbr3']) + intval($_COOKIE['nmbr4'])) {
                        $from2 = $objEmail;
                        $data['msg14_14'] = '';
                        $headers2 = 'From: ' . $from2 . "\r\n" .
                            'Reply-To: ' . $sendTo . "\r\n" .
                            'X-Mailer: PHP/' . phpversion();
                        mail($sendTo, $subject2, $globaleMSG2, $headers2);
                        $data['msgfinal'] = 'Votre Messsage est bien envoyer';
                    } else {
                        $data['msg14_14'] = 'votre resultat est incorrect';
                    }
                } else {
                    $data['msg11_11'] = 'Votre Prénom est Incorrect';
                }
            } else {
                $data['msg10_10'] = 'Votre Nom est Incorrect';
            }
        }

Phew. Hey, remember way back at the top, when we checked to see if the $_POST variable were set? Well, we do have an else clause for that.

    } else {
        throw new \Exception($mot[86]);
    }

Who doesn't love throwing messages by hard-coded array indexes in your array of possible error messages? Couldn't be bothered with a constant, could we? Nope, message 86 it is.

But don't worry about that exception going uncaught. Remember, this whole thing was inside of a try:

} catch (\Exception $e) {
    $data['msgfinal'] = "Votre Messsage n'est pas bien envoyer";
    /*$data = array('danger' => 'Votre Messsage pas bien envoyer', 'postData' => $_POST);*/
}

Yeah, it didn't matter what message we picked, because we just catch the exception and hard-code out an error message.

Also, I don't speak French, but is "message" supposed to have an extra "s" in it?

Charles writes:

It’s crazy to see such sloppy work on a platform that seems okay at first glance. Honestly, this platform is the holy grail of messy code—it could have its own course on how not to code because of how bad and error-prone it is. There are also even worse scenarios of bad code, but it's too long to share, and honestly, they're too deep and fundamentally ingrained in the system to even begin explaining.

Oh, I'm sure we could explain it. The explanation may be "there was a severe and fatal lack of oxygen in the office, and this is what hypoxia looks like in code," but I'm certain there'd be an explanation.

.comment { border: none; } [Advertisement] Plan Your .NET 9 Migration with ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!

CodeSOD: Plugin Acrobatics

Wed, 20 Nov 2024 06:30:00 GMT

Link

Once upon a time, web browsers weren't the one-stop-shop for all kinds of possible content that they are today. Aside from the most basic media types, your browser depended on content plugins to display different media types. Yes, there was an era where, if you wanted to watch a video in a web browser, you may need to have QuickTime or… (shudder) Real Player installed.

As a web developer, you'd need to write code to check which plugins were installed. If they don't have Adobe Acrobat Reader installed, there's no point in serving them up a PDF file- you'll need instead to give them an install link.

Which brings us to Ido's submission. This code is intended to find the Acrobat Reader plugin version.

acrobatVersion: function GetAcrobatVersion() {
	// Check acrobat is Enabled or not and its version
	acrobatVersion = 0;
	if (navigator.plugins && navigator.plugins.length) {
		for (intLoop = 0; intLoop <= 15; intLoop++) {
			if (navigator.plugins[intLoop] != -1) {
				acrobatVersion = parseFloat(navigator.plugins[intLoop].version);
				isAcrobatInstalled = true;
				break;
			}
		}
	}
	else {...}
}

So, we start by checking for the navigator.plugins array. This is a wildly outdated thing to do, as the MDN is quite emphatic about, but I'm not going to to get hung up on that- this code is likely old.

But what I do want to pay attention to is that they check navigator.plugins.length. Then they loop across the set of plugins using a for loop. And don't use the length! They bound the loop at 15, arbitrarily. Why? No idea- I suspect it's for the same reason they named the variable intLoop and not i like a normal human.

Then they check to ensure that the entry at plugins[intLoop] is not equal to -1. I'm not sure what the expected behavior was here- if you're accessing an array out of bounds in JavaScript, I'd expect it to return undefined. Perhaps some antique version of Internet Explorer did something differently? Sadly plausible.

Okay, we've found something we believe to be a plugin, because it's not -1, we'll grab the version property off of it and… parseFloat. On a version number. Which ignores the fact that 1.1 and 1.10 are different versions. Version numbers, like phone numbers, are not actually numbers. We don't do arithmetic on them, treat them like text.

That done, we can say isAcrobatInstalled is true- despite the fact that we didn't check to see if this plugin was actually an Acrobat plugin. It could have been Flash. Or QuickTime.

Then we break out of the loop. A loop that, I strongly suspect, would only ever have one iteration, because undefined != -1.

So there we have it: code that doesn't do what it intends to, and even if it did, is doing it the absolute wrong way, and is also epically deprecated.

[Advertisement] Plan Your .NET 9 Migration with ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!

CodeSOD: Recursive Search

Tue, 19 Nov 2024 06:30:00 GMT

Link

Sometimes, there's code so bad you simply know it's unused and never called. Bernard sends us one such method, in Java:

  /**
   * Finds a <code>GroupEntity</code> by group number.
   *
   * @param  group the group number.
   * @return the <code>GroupEntity</code> object.
   */
  public static GroupEntity find(String group) {
    return GroupEntity.find(group);
  }

This is a static method on the GroupEntity class called find, which calls a static method on the GroupEntity class called find, which calls a static method on the GroupEntity class called find and it goes on and on my friend.

Clearly, this is a mistake. Bernard didn't supply much more context, so perhaps the String was supposed to be turned into some other type, and there's an overload which would break the recursion. Regardless, there was an antediluvian ticket on the backlog requesting that the feature to allow finding groups via a search input that no one had yet worked on.

I'm sure they'll get around to it, once the first call finishes.

.comment { border: none; } [Advertisement] Plan Your .NET 9 Migration with ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!

CodeSOD: Objectified

Mon, 18 Nov 2024 06:30:00 GMT

Link

Simon recently found himself working alongside a "very senior" developer- who had a whopping 5 years of experience. This developer was also aggrieved that in recent years, Object Oriented programming had developed a bad reputation. "Functional this, functional that, people really just don't understand how clean and clear objects make your code."

For example, here are a few Java objects which they wrote to power a web scraping tool:

class UrlHolder {

    private String url;

    public UrlHolder(String url) {
        this.url = url;
    }
}

class UrlDownloader {

    private UrlHolder url;
    public String downloadPage;

    public UrlDownLoader(String url) {
        this.url = new UrlHolder(Url);
    }
}

class UrlLinkExtractor {

   private UrlDownloader url;

   public UrlLinkExtractor(UrlDownloader url) {
        this.url = url;
   }

   public String[] extract() {
       String page = Url.downloadPage;
       ...
   }
}

UrlHolder is just a wrapper around string, but also makes that string private and provides no accessors. Anything shoved into an instance of that may as well be thrown into oblivion.

UrlDownloader wraps a UrlHolder, again, as a private member with no accessors. It also has a random public string called downloadPage.

UrlLinkExtractor wraps a UrlDownloader, and at least UrlLinkExtractor has a function- which presumably downloads the page. It uses UrlDownloader#downloadPage- the public string property. It doesn't use the UrlHolder, because of course it couldn't. The entire goal of this code is to pass a string to the extract function.

I guess I don't understand object oriented programming. I thought I did, but after reading this code, I don't.

https://thedailywtf.com/images/inedo/proget-icon.png

[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.

Error'd: Tangled Up In Blue

Fri, 15 Nov 2024 06:30:00 GMT

Link

...Screens of Death. Photos of failures in kiosk-mode always strike me as akin to the wizard being exposed behind his curtain. Yeah, that shiny thing is after all just some Windows PC on a stick. Here are a few that aren't particularly recent, but they're real.

Jared S. augurs ill: "Seen in downtown Mountain View, CA: In Silicon Valley AI has taken over. There is no past, there is no future, and strangely, even the present is totally buggered. However, you're free to restore the present if you wish."

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/246/img_2411sm.jpeg

Windows crashed Maurizio De Cecco's party and he is vexé. "Some OS just doesn’t belong in the parisian nightlife," he grumbled. But neither does pulled pork barbecue and yet there it is.

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/246/img_4360.jpeg

Máté cut Windows down cold. "Looks like the glaciers are not the only thing frozen at Matterhorn Glacier Paradise..."

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/246/zermattbsod.jpeg

Thomas found an installer trying to apply updates "in the Northwestern University's visitor welcome center, right smack in the middle of a nine-screen video display. I can only imagine why they might have iTunes or iCloud installed on their massive embedded display." I certainly can't.

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/246/errord20221121.jpeg

Finally, Charles T. found a fast-food failure and was left entirely wordless. And hungry.

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/246/4c84fe8013874954858ce632474fe5b1.jpeg

[Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.

CodeSOD: Secondary Waits

Thu, 14 Nov 2024 06:30:00 GMT

Link

ArSo works at a small company. It's the kind of place that has one software developer, and ArSo isn't it. But ArSo is curious about programming, and has enough of a technical background that small tasks should be achievable. After some conversations with management, an arrangement was made: Kurt, their developer, would identify a few tasks that were suitable for a beginner, and would then take some time to mentor ArSo through completing them.

It sounded great, especially because Kurt was going to provide sample code which would give ArSo a head start on getting things done. What better way to learn than by watching a professional at work?

DateTime datTmp;

File.Copy(strFileOld, strFileNew);
// 2 seconds delay
datTmp = DateTime.Now;
while (datTmp.Second == DateTime.Now.Second);
datTmp = DateTime.Now;
while (datTmp.Second == DateTime.Now.Second);
File.Delete(strFileOld);

This code copies a file from an old path to a new path, and then deletes the old path after a two second delay. Why is there a delay? I don't know. Why is the delay written like this? I can't possibly explain that.

Check the time at the start of the loop. When the second part of that time stops matching the second part of the current time, we assume one second has passed. This is, of course, inaccurate- if I check the time at 0:00:00.9999 a lot less than a second will pass. This delay is at most one second.

In any case, ArSo has some serious questions about Kurt's mentorship, and writes:

Now I don't know if I should ask for more coding tasks.

Honestly, I think you should ask for more. Like, I think you should just take Kurt's job. You may be a beginner, but honestly, you're likely going to do a better job than this.

[Advertisement] Plan Your .NET 9 Migration with ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!

CodeSOD: The First 10,000

Wed, 13 Nov 2024 06:30:00 GMT

Link

Alicia recently inherited a whole suite of home-grown enterprise applications. Like a lot of these kinds of systems, it needs to do batch processing. She went tracking down a mysterious IllegalStateException only to find this query causing the problem:

select * from data_import where id > 10000

The query itself is fine, but the code calling it checks to see if this query returned any rows- if it did, the code throws the IllegalStateException.

First, of course, this should be a COUNT(*) query- no need to actually return rows here. But also… what? Why do we fail if there are any transactions with an ID greater than 10000? Why on Earth would we care?

Well, the next query it runs is this:

update data_import set id=id+10000

Oh. Oh no. Oh nooooo. Are they… are they using the ID to also represent some state information about the status of the record? It sure seems like it!

The program then starts INSERTing data, using a counter which starts at 1. Once all the new data is added, the program then does:

delete from data_import where id > 10000

All this is done within a single method, with no transactions and no error handling. And yes, this is by design. You see, if anything goes wrong during the inserts, then the old records don't get deleted, so we can see that processing failed and correct it. And since the IDs are sequential and always start at 1, we can easily find which row caused the problem. Who needs logging or any sort of exception handling- just check your IDs.

The underlying reason why this started failing was because the inbound data started trying to add more than 10,000 rows, which meant the INSERTs started failing (since we already had rows there for this). Alicia wanted to fix this and clean up the process, but too many things depended on it working in this broken fashion. Instead, her boss implemented a quick and easy fix: they changed "10000" to "100000".

[Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.

Representative Line: How is an Array like a Banana?

Tue, 12 Nov 2024 06:30:00 GMT

Link

Some time ago, poor Keith found himself working on an antique Classic ASP codebase. Classic ASP uses VBScript, which is like VisualBasic 6.0, but worse in most ways. That's not to say that VBScript code is automatically bad, but the language certainly doesn't help you write clean code.

In any case, the previous developer needed to make an 8 element array to store some data. Traditionally, in VBScript, you might declare it like so:

Dim params(8)

That's the easy, obvious way a normal developer might do it.

Keith's co-worker did this instead:

Dim params : params = Split(",,,,,,,", ",")

Yes, this creates an array using the Split function on a string of only commas. 7, to be exact. Which, when split, creates 8 empty substrings.

We make fun of stringly typed data a lot here, but this is an entirely new level of stringly typed initialization.

We can only hope that this code has finally been retired, but given that it was still in use well past the end-of-life for Classic ASP, it may continue to lurk out there, waiting for another hapless developer to stumble into its grasp.

[Advertisement] Plan Your .NET 9 Migration with ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!

CodeSOD: Pay for this Later

Mon, 11 Nov 2024 06:30:00 GMT

Link

Ross needed to write software to integrate with a credit card payment gateway. The one his company chose was relatively small, and only served a handful of countries- but it covered the markets they cared about and the transaction fees were cheap. They used XML for data interchange, and while they had no published schema document, they did have some handy-dandy sample code which let you parse their XML messages.

$response = curl_exec($ch);
$authecode = fetch_data($response, '<authCode>', '</authCode>');
$responsecode = fetch_data($response, '<responsecode>', '</responsecode>');
$retrunamount = fetch_data($response, '<returnamount>', '</returnamount>');
$trxnnumber = fetch_data($response, '<trxnnumber>', '</trxnnumber>');
$trxnstatus = fetch_data($response, '<trxnstatus>', '</trxnstatus>');
$trxnresponsemessage = fetch_data($response, '<trxnresponsemessage>', '</trxnresponsemessage>');

Well, this looks… worrying. At first glance, I wonder if we're going to have to kneel before Z̸̭͖͔͂̀ā̸̡͖͕͊l̴̜͕͋͌̕g̸͉̳͂͊ȯ̷͙͂̐. What exactly does fetch_data actually do?

function fetch_data($string, $start_tag, $end_tag)
{

  $position = stripos($string, $start_tag);
  $str = substr($string, $position);
  $str_second = substr($str, strlen($start_tag));
  $second_positon = stripos($str_second, $end_tag);
  $str_third = substr($str_second, 0, $second_positon);
  $fetch_data = trim($str_third);
  return $fetch_data;
}

Phew, no regular expressions, just… lots of substrings. This parses the XML document with no sense of the document's structure- it literally just searches for specific tags, grabs whatever is between them, and calls it done. Nested tags? Attributes? Self-closing tags? Forget about it. Since it doesn't enforce that your open and closing tags match, it also lets you grab arbitrary (and invalid) document fragments- fetch_data($response, "<fooTag>", "<barTag>"), for example.

And it's not like this needs to be implemented from scratch- PHP has built-in XML parsing classes. We could argue that by limiting ourselves to a subset of XML (which I can only hope this document does) and doing basic string parsing, we've built a much simpler approach, but I suspect that after doing a big pile of linear searches through the document, we're not really going to see any performance benefits from this version- and maintenance is going to be a nightmare, as it's so fragile and won't work for many very valid XML documents.

It's always amazing when TRWTF is neither PHP nor XML but… whatever this is.

[Advertisement] Plan Your .NET 9 Migration with ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!

Error'd: Relatively Speaking

Fri, 08 Nov 2024 06:30:00 GMT

Link

Amateur physicist B.J. is going on vacation, but he likes to plan things right down to the zeptosecond. "Assume the flight accelerates at a constant speed for the first half of the flight, and decelerates at the same rate for the second half. 1) What speed does the plane need to reach to have that level of time dilation? 2) What is the distance between the airports?"

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/245/screenshot20241104at150550.png

Contrarily, Eddie R. was tired of vacation so got a new job, but right away he's having second thoughts. "Doing my onboarding, but they seem to have trouble with the idea of optional."

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/245/emailoptional.jpeg

"Forget UTF-8! Have you heard about the new, hot encoding standard for 2024?!" exclaimed Daniel , kvetching "Well, if you haven't then Gravity Forms co. is going to change your mind: URLEncode everything now! Specially if you need to display some diacritics on your website. Throw away the old, forgotten UTF-8. Be a cool guy, just use that urlencode!"

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/245/screenshot_20241017_100103.png

Immediately afterward, Daniel also sent us another good example, this time from Hetzner. He complains "Hetzner says the value is invalid. Of course they won't say what is or isn't allowed. It wasn't the slash character, it was... a character with diacritics! Hetzner is clearly using US-ASCII created in 1960's."

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/245/screenshot_20241022_111334.png

Finally this week, we pulled something out of the archive from Boule de Berlin who wrote "Telekom, the biggest German ISP, shows email address validation is hard. They use a regex that limits the TLD part of an email address to 4 chars." Old but timeless.

https://d3hvi6t161kfmf.cloudfront.net/images/24/q1/245/2022_12_02_21_00_53_persnliche_daten_telekom_vivaldi.jpeg

https://thedailywtf.com/images/inedo/proget-icon.png

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

Representative Line: One More Parameter, Bro

Thu, 07 Nov 2024 06:30:00 GMT

Link

Matt needed to add a new field to a form. This simple task was made complicated by the method used to save changes back to the database. Let's see if you can spot what the challenge was:

public int saveQualif(String docClass, String transcomId, String cptyCod, String tradeId, String originalDealId, String codeEvent, String multiDeal,
            String foNumber, String codeInstrfamily, String terminationDate, String premiumAmount, String premiumCurrency, String notionalAmount,
            String codeCurrency, String notionalAmount2, String codeCurrency2, String fixedRate, String payout, String maType, String maDate,
            String isdaZoneCode, String tradeDate, String externalReference, String entityCode, String investigationFileReference,
            String investigationFileStartDate, String productType, String effectiveDate, String expiryDate, String paymentDate, String settInstrucTyp,
            String opDirection, String pdfPassword, String extlSysCod, String extlDeaId, String agrDt) throws TechnicalException, DfException

That's 36 parameters right there. This function, internally, creates a data access object which takes just as many parameters in its constructor, and then does a check: if a field is non-null, it updates that field in the database, otherwise it doesn't.

Of course, every single one of those parameters is stringly typed, which makes it super fun. Tracking premiumAmount and terminationDate as strings is certainly never going to lead to problems. I especially like the pdfPassword being stored, which is clearly just the low-security password meant to be used for encrypting a transaction statement or similar: "the last 4 digits of your SSN" or whatever. So I guess it's okay that it's being stored in the clear in the database, but also I still hate it. Do better!

In any case, this function was called twice. Once from the form that Matt was editing, where every parameter was filled in. The second time, it was called like this:

int nbUpdates = incoming.saveQualif(docClass, null, null, null, null, null, multiDeal, null,
                null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null,
                null, null, null, null, null, null, null, null, null, null, null, null);

As tempted as Matt was to fix this method and break it up into multiple calls or change the parameters to a set of classes or anything better, he was too concerned about breaking something and spending a lot of time on something which was meant to be a small, fast task. So like everyone who'd come before him, he just slapped in another parameter, tested it, and called it a day.

Refactoring is a problem for tomorrow's developer.

https://thedailywtf.com/images/inedo/buildmaster-icon.png

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

CodeSOD: Uniquely Validated

Wed, 06 Nov 2024 06:30:00 GMT

Link

There's the potential for endless installments of "programmers not understanding how UUIDs work." Frankly, I think the fact that we represent them as human readable strings is part of the problem; sure, it's readable, but conceals the fact that it's just a large integer.

Which brings us to this snippet, from Capybara James.

    if (!StringUtils.hasLength(uuid) || uuid.length() != 36) {
        throw new RequestParameterNotFoundException(ErrorCodeCostants.UUID_MANDATORY_OR_FORMAT);
    }

StringUtils.hasLength comes from the Spring library, and it's a simple "is not null or empty" check. So- we're testing to see if a string is null or empty, or isn't exactly 36 characters long. That tells us the input is bad, so we throw a RequestParameterNotFoundException, along with an error code.

So, as already pointed out, a UUID is just a large integer that we render as a 36 character string, and there are better ways to validate a UUID. But this also will accept any 36 character string- as long as you've got 36 characters, we'll call it a UUID. "This is valid, really valid, dumbass" is now a valid UUID.

With that in mind, I also like the bonus of it not distinguishing between whether or not the input was missing or invalid, because that'll make it real easy for users to understand why their input is getting rejected.

https://thedailywtf.com/images/inedo/proget-icon.png

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

CodeSOD: Counting it All

Tue, 05 Nov 2024 06:30:00 GMT

Link

Since it's election day in the US, many people are thinking about counting today. We frequently discuss counting here, and how to do it wrong, so let's look at some code from RK.

This code may not be counting votes, but whatever it's counting, we're not going to enjoy it:

case LogMode.Row_limit: // row limit excel = 65536 rows
    if (File.Exists(personalFolder + @"\" + fileName + ".CSV"))
    {
        using (StreamReader reader = new StreamReader(personalFolder + @"\" + fileName + ".CSV"))
        {
            countRows = reader.ReadToEnd().Split(new char[] { '\n' }).Length;
        }
    }

Now, this code is from a rather old application, originally released in 2007. So the comment about Excel's row limit really puts us in a moment in time- Excel 2007 raised the row limit to 1,000,000 rows. But older versions of Excel did cap out at 65,536. And it wasn't the case that everyone just up and switched to Excel 2007 when it came out- transitioning to the new Office file formats was a conversion which took years.

But we're not even reading an Excel file, we're reading a CSV.

I enjoy that we construct the name twice, because that's useful. But the real magic of this one is how we count the rows. Because while Excel can handle 65,536 rows at this time, I don't think this program is going to do a great job of it- because we read the entire file into memory with ReadToEnd, then Split on newlines, then count the length that way.

As you can imagine, in practice, this performed terribly on large files, of which there were many.

Unfortunately for RK, there's one rule about old, legacy code: don't touch it. So despite fixing this being a rather easy task, nobody is working on fixing it, because nobody wants to be the one who touched it last. Instead, management is promising to launch a greenfield replacement project any day now…

https://thedailywtf.com/images/inedo/proget-icon.png

[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.