I’ll give an example. At my previous company there was a program where you basically select a start date, select an end date, select the system and press a button and it reaches out to a database and pulls all the data following that matches those parameters. The horrors of this were 1. The queries were hard coded.
-
They were stored in a configuration file, in xml format.
-
The queries were not 1 entry. It was 4, a start, the part between start date and end date, the part between end date and system and then the end part. All of these were then concatenated in the program intermixed with variables.
-
This was then sent to the server as pure sql, no orm.
-
Here’s my favorite part. You obviously don’t want anyone modifying the configuration file so they encrypted it. Now I know what you’re thinking at some point you probably will need to modify or add to the configuration so you store an unencrypted version in a secure location. Nope! The program had the ability to encrypt and decrypt but there were no visible buttons to access those functions. The program was written in winforms. You had to open the program in visual studio, manually expand the size of the window(locked size in regular use) and that shows the buttons. Now run the program in debug. Press the decrypt button. DO NOT EXIT THE PROGRAM! Edit the file in a text editor. Save file. Press the encrypt button. Copy the encrypted file to any other location on your computer. Close the program. Manually email the encrypted file to anybody using the file.
A program that HR had built so that all employees could they their payment receipts online
The username was the companies’ email address, the password was a government personal id code that you can lookup online, a don’t change, and you can’t update the password to something else.
So I told the director of HR this was a bad idea. She told me I was overreacting until I showed her her own receipt, then she finally understood that this is a really fucking bad idea.
Okay, so now she out me in charge of debugging that program.
So I setup a meeting with the director of the company they hired, he came by with the developer: a 21 yo girl who I think hadn’t finished college yet. Great start! Apparently it was her idea to do the authentication like that so that explains a few things.
So we dive in to the code.
First of all, the “passwords” were stored in blank, no hashing, no encryption, nothing. That wasn’t the worst.
For the authentication she made a single query to check if the user email existed. Of that was true, then step two was a second query to see if the password existed. If that were true, the email had been authenticated.
So let’s say, hypothetically, that they had actual passwords that people could change… I could still login with the email from anyone, and then use MY OWN password to authenticate.
This just blew my mind so hard that I don’t think I ever fully recovered, I still need treatment. The stupidity hurts
I wouldnt blame that on stupidity as much as on ignorance and naivety. Many people simply don’t think about anybody deliberately misusing their design. The idea that somebody could even want to access somebody elses receipts didn’t occur to them. And if they were still doing their studies they might not have known that you can “combine” SQL queries and ask for two things at once.
I don’t blame the girl, but whoever chose her to design a system with sensitive information.
I don’t blame a girl for doing a job that lands her food on the table. I blame the guy employing her because she’s the cheapest option
Having said that, this design was so bad that she should not have been doing any of this. If you don’t know that SQL allows you to select multiple columns then by all means, do a tutorial, it’s not that hard.
If you don’t even know what encryption is, that passwords need hashing and what not, then you should really question what you’re doing
OPs question was about the worst code I’ve seen, that was the worst I’ve seen
If you don’t even know what encryption is, that passwords need hashing and what not, then you should really question what you’re doing
I agree with your point, but I would phrase it more generally: when we’re assigned a task in a problem space we are unfamiliar with, we should always take some time to research that space before designing our solution.
After all, if we don’t know what encryption or password hashing are, how could we know that we need to learn about them first? But spending just a couple hours one morning reading about password and authentication management would have given the developer a good sense of best practices.
So she either, A) didn’t think to familiarize herself with a new topic prior to working on it, or B) did read about it and ignored general industry guidance. Both of those options are more problematic to me than simply not knowing specific things. Those are process problems that need to be addressed to build her skills as a developer.
But ultimately, in my opinion, this is really all the fault of the cheapass director who didn’t want to pay any experienced professionals to handle the task.
It wouldn’t take much google-fu to get a worked example of good authentication in whatever language. She can’t have tried, she must have just gone “programming 104 covered how to SQL, I can use that”
There was something like
# sleep for about a second on modern processors math.factorial(10000)After it was found we left it in the code but commented out along with a
sleep(1)for posterity.In the readme: if you want this program to be usable, press the turbo button until the turbo light is OFF.
I saw one where the program ran a busy loop on startup to calculate how long it took. Then it used that as an iterations-to-seconds conversion for busy loops between scheduled actions.
That’s atleast pretty creative
So this is not as bad as some of the other stories I’ve seen, but I’ll bite.
It was an old .NET Framework MVC app. Some internal product management system or something. There was a need to do a PDF export in one of the use cases, so someone implemented it. It wasn’t a good implementation: one big controller, mixing UI and business logic, etc. However, it basically came down to a single private method in a specific controller for a page.
Now time passes and lo and behold, we need a PDF export in another page for a different use case. “No problem,” - same dev, probably - “I already solved this problem. I’ll just reuse the PDF generation logic.”
Now, any sane person would probably try to refactor the code responsible for PDF stuff into a separate service (class) and reuse it. A less sane, but somewhat, acceptable approach would have been to just copy paste the thing into another controller and call it a day.Ha! No no no no no no… Copy pasting is bad, code should be reused…
The end solution: REFLECTION. So the dev decided that the easiest way to make it work was to: 1) use reflection to inject one controller into another; 2) then use reflection again to get access and call that private method for PDF rendering into a stream.
Fortunately I didn’t have to fix that fragile mess. But I did my fair share of DevExpress corpse hacking and horrible angular “server side rendering” workarounds.
Private key for a third-party API hard-coded into the front-end web app
Lots. But one that springs to mind is a custom CMS where a new dev decided to print out the sql generated for a particular content type on paper. He took it to the CTO without comment.
What was wrong?
It was 12 pages.
Am I reading that right, that he printed out the generated sql query?
If so depending on context that may make sense to complain about. A 12 page sql query would be insane, something sounds like their are other issues.
That said I probably wouldn’t go to cto, I would go to manager or a senior dev and ask why it was so complex to get a particular content type. If there were no performance issues or bugs I would just ask out of curiosity.
Yes, the generated SQL query. It basically consisted of a lot of WHERE x IN (1,2,3,4) clauses for all the document IDs that matched something or other, and then repeated for the next JOIN. Small company, CTO was our direct boss and in the same open-plan office.
The C++ code went something like this:
- Conver pointer to int
- Serialize the int over IPC to self using Linux Message Queues
- Delete/free the pointer
- Read the int from the queue
- Convert to pointer
- “Use” the pointer
Only reason I can think to do that is to “verify” the data in the pointer is not null/empty and is a valid int???
There are much better ways to do that but I can’t think of any other intent the programmer had.
Oh no, when they deserialized the int/pointer they used it like the original structure (which now points to freed memory). They meant to serialize the data structure across, but only sent the memory address.
Ok so this one is someone trying to move to “the cloud.”
They had a database they used. It was on a server in the office. We were tasked to clone the db server to a hosted VM. Due to order of creation this got put on a new host without anything yet on it.
They needed a site to site VPN to keep privacy, that was all fine. However after the clone and during testing, their guy there said that this one part was really slow. We take a look and everything is good with performance of the server and of the VPN. I have to pop on to take a look.
It was in an office app and written in VB. (I forgot which one.) It was indeed slower on the hosted server. So I took a look at the function (he got it up for me) and I could instantly tell the issue.
This part was a lookup page that searched for you input. The function retrieved the entire table, then filtered the results in the client. I explained that transferring the whole table over the internet would be slower than on the local lan.
This guy said he originally wrote this, but “forgot VB.”
In the end they decided not to update the app or keep the server in the office, but instead they rented some VDIs in the same data centre as the db.
Sounds like he didn’t have much to forget
I saw a talk recently, I can find the video if you like but pretty sure it was the most recent ND conference, where they made the point that a lot of lack of efficiency in modern code is because of large companies. Basically in alot of cases it’s more important to get a product out ASAP then to care if it was well done. Ok, a poorly written program may cost an extra $10,000 a month to run but if it earns them a million a month and saves 6 months of development time it pays for itself and they can eat the cost.
This seems like the case with renting vdis instead of fixing the program.
This one is funny because it 100% still exists somewhere, but I haven’t had the chance to verify it again.
Okay so basically its a data recorder box (ex: brainbox) that connects to a bunch of industrial sensors and sends the data over the network with your preferred method.
Builtin firmware gives you an HTTP webui to login and configure the device, with a user # and password.
I think the user itself had a builtin default admin which was #0, which everyone uses since there wasn’t really much use for other users.
Anyway, I was looking at the small JS code for the webui and noticed it had an MD5 hashing code that was very detailed with comments. It carefully laid out each operation, and explained each step to generate a hash, and then even why hashes should be used for passwords.
Here’s the kicker: It was all client side JS, so the login page would take your password, hash it, and then send the hash over plaintext HTTP POST to the server, where it would be authenticated.
Meaning you could just mitm the connection to grab the hash, and then login with the hash.
I sat there for like 10 minutes looking at the request over and over again. Like someone was smart enough to think “hey let’s use password hashing to keep this secure” and then proceeded to use it in the compleltly wrong way. And not even part of like a challenge/handshake where the server gives you a token to hash with. Just straight up MD5(password).
It was so funny because there were like a hundred of these on a network, so getting a valid hash was laughably easy.
I never got to check if this was fixed in a newer firmware version.
#4 is a good thing. ORMs do not make queries better or safer, they make them easier for devs that don’t learn SQL or safe calls. In some cases, they have been shown to cause slowdowns.
Whatever is happening in Monster Hunter Wilds.
Weather forecasting software that maintains a linked list. When it eventually freed the memory used by the list, it would walk to the end of the list and free the last item. Then it would go back to the beginning of the list and do it again - rinse and repeat. Wonder why it was having performance issues 🙄
What were they storing in the linked list?
A bit late to the party on this one, but Facepunch just opensourced a bunch of their code, I nominate that.
I don’t recognize the name, what dud facepunch make?
Garry’s Mod. Rust (the game, not the programming language).
Ahh, ok yeah makes sense
Doesn’t that mean that your encryption algorithm and key is stored inside the code?
And since you are opening the code in Visual Studio anyway, just follow the function called by the “Decrypt” button, copy the function into another project and now you have a decryptor.I believe so but I don’t remember the exact encryption algorithm and don’t have access to the code anymore.
This was the same place that had a 500 line file named glob_vars.cs which you can guess the content of because “passing around variables cause memory leak issues”.
Long time ago, but by far the worst for me was when I inherited some code that a previous programmer had done. Every variable was a breakfast item. So if biscuit>bacon then scrambledeggs=10. Shit like that. It was a nightmare and luckily I only had to deal with it infrequently.
Why do people do stuff like this, is the logic not difficult enough to follow on it’s own without a secondary definition table to consult!? Fucking hell.
secondary definition breakfast table
Had a programmer like this when I was still an apprentice. He was so full of himself. Was originally a Java programmer but had to program in PHP because that was what ran on the server. I never found out why he couldn’t just put Java on the server. We had full control.
All his variables were first names. Like
$klausand$grobi. Because he was afraid of clashing with reserved keywords. The thing is, in PHP all variables begin withexactly to prevent this issue. So he brought that habit over from Java which was far superior and not such a “Mickey Mouse language”.I mean, he wasn’t totally wrong, especially back then PHP was awful. But he surrounded every function with
and(PHP was designed to be combined with HTML output outside of these tags) and had plenty of whitespace between them and couldn’t fathom why all his html files had huge swaths of whitespace at the start.His way of preventing SQL injection was to look for SQL keywords in user input and then throwing an error in the log files.
I don’t know what’s worse… That program or that you put biscuits greater than bacon…
Actually I think the greater crime is biscuits being greater than bacon
but BiscuitTop + Bacon + Eggs + BiscuitBottom is definitely better than biscuit, or bacon or eggs.
True, all are good.
bacon++;
unless they’re bacon biscuits
I don’t know how old you are but when I was in school, this was just going out of style. They saw this as job security. If you’re the only one who can work on the code, then they won’t fire you
Oh god, that’s worse than I’ve seen where a SQL query joining 10 tables aliased all of the tables as a, b, c, d, e, f, g, h, i, j.
It was a mess, and as a new dev on the project, trying to figure out which where clause was for which table and how things worked was a fucking nightmare. Trying to keep a dictionary of letters to real table names in your head as you looked at the query was very taxing. In the end, I just fixed it all to stop using aliases. Or to use short abbreviations.
Here’s a mock example:
SELECT j.delivery_eta, c.cat_desc, a.part_number, h.region_label, f.wh_loc, e.emp_last, g.state_flag, b.mfg_title, i.ship_track_code, d.order_sum, a.created_on, j.last_scanned_at, e.emp_first, c.cat_code, g.state_level FROM parts AS a INNER JOIN manufacturers AS b ON a.manufacturers_id = b.id INNER JOIN categories AS c ON a.categories_id = c.id INNER JOIN orders AS d ON a.orders_id = d.id INNER JOIN employees AS e ON d.employees_id = e.id INNER JOIN warehouses AS f ON a.warehouses_id = f.id INNER JOIN inv_state AS g ON a.inv_state_id = g.id INNER JOIN regions AS h ON f.regions_id = h.id INNER JOIN shipments AS i ON d.shipments_id = i.id INNER JOIN logistics AS j ON i.logistics_id = j.id WHERE (b.mfg_title LIKE '%Corp%' OR b.mfg_title LIKE '%Global%') AND c.cat_desc NOT IN ('Unknown', 'None', 'Legacy') AND (d.order_sum > 1000 OR d.order_sum BETWEEN 250 AND 275) AND e.emp_last ILIKE '%berg' AND (f.wh_loc IN ('A1', 'Z9', 'M3') OR f.wh_loc IS NULL) AND g.state_flag IN ('ACT', 'PENDING') AND h.region_label NOT LIKE 'EXT-%' AND (i.ship_track_code IS NOT NULL AND i.ship_track_code <> '') AND (j.delivery_eta < NOW() + INTERVAL '90 days' OR j.last_scanned_at IS NULL) AND (a.part_number ~ '^[A-Z0-9]+$' OR a.part_number IS NULL) AND ( (c.cat_code = 'X1' AND g.state_level > 2) OR (e.emp_first ILIKE 'J%' AND d.orders_id IS NOT NULL) );That’s how mainframe programmers at my workplace do SQL. I think they do it due to long table and field names and narrow mainframe COBOL files
I have a friend that uses swear words 🤷♂️
So, this is completely off topic, but some of the comments here reminded me of it:
An elderly family friend was spending a lot of her time using Photoshop to make whimsy collages and stuff to give as gifts to friends and family.
I discovered that when she wanted to add text to an image, she would type it out in Microsoft Word, print it, scan the printed page, then overlay the resulting image over the background with a 50% opacity.
I showed her the type tool in Photoshop and it blew her mind.I am simultaneously horrified that she didn’t do any research to see if she could insert text into the image and incredibly impressed at her problem solving skills. Honestly, the more I think about it, the more I lean towards impressed; good on her!
Haha that’s so dumb. She could’ve just taken a screenshot!
I showed her the type tool in Photoshop and it blew her mind.
Or well. That.
Aw really wholesome actually. Some libraries in my area have senior friendly editing classes, I think it’s becoming more popular. Good looking out for them!
Photoshop is amazing. That said you kinda need to take a course in it to use 80% of the functionality.
And almost always, if you are not a professional, that could be done with any image editing program.
