12 ms·

Reviewing the worst piece of code ever

143 points by micheleriva 6y ago

kanobo 6y ago
I'm sure most of us have seen far worse and more nonsensical code, it's a nicely designed snippet to teach basic security issues and show silly mistakes though.
banana_giraffe 6y ago
According to a reddit thread [1] from a few years ago, this is from an internal application, though on a public facing server. Still, I agree with one of the posts on reddit, this is a decent ice breaker interview question to see how many problems the candidate can find. [1] https://www.reddit.com/r/programminghorror/comments/66klvc/this_javascript_code_powers_a_1500_user_intranet/ https://www.reddit.com/r/programminghorror/comments/66klvc/t...
ExcavateGrandMa 6y ago
some ppl are epic! particularly ppl who can't explain what they doing... :)
deleted 6y ago
[deleted]
29athrowaway 6y ago
1. SQL injection as a service. 2. It should implement this logic server-side. 3. No hashing + salting of passwords. 4. It should retrieve one user at a time, rather than all of them. 5. Not understanding basic control flow. ... There was probably a presentation about this project, where the author received a round of applause. This person was likely promoted for finishing this project in record-time. The developer that spent time reporting the issue lowered their own performance metrics in exchange for a bug report that was given a low priority. When the developer objected to the prioritization, the project leadership got pissed off and punished the developer evaluation citing reasons such as "does not align with business needs", "has poor communication skills" (talks abstract things that nobody understands or cares about). By the time this defect was found and fixed, the author likely pushed 10 different other defects just like this. Also, other engineers copied and pasted this code multiple times because it's working and has unit tests.
- Ar-Curunir 6y ago
  Your conclusions are entirely unsubstantiated...
- unsignedchar 6y ago
  Were you going for a comic effect or tragic?
  - 29athrowaway 6y ago
    Is Office Space a comedy or a documentary?
    - eckza 6y ago
      ... yes.
Sniffnoy 6y ago
One bit of awfulness the article missed: Rather than scanning for the username and then checking whether the password is correct, it scans for a match on both username and password. So if you have the right username but the wrong password, it still has to check every single username to conclude this; it won't stop when it gets to the username you entered. I mean, not that it should be scanning through a list to find the right username, but still... (And yeah, I suppose that would allow for some sort of username enumeration via a timing attack, but somehow I don't think that's the reason they didn't do that here...)
- champtar 6y ago
  You assume that username are unique in this DB, maybe they support multiple password :)
  - onemiketwelve 6y ago
    What a neat feature ;)
    - Viliam1234 6y ago
      If you forget a new password, you can create a new one, and then if you remember the old password later, you can use both. Is this so different in principle from supporting multiple methods of authentication? :D
- CodesInChaos 6y ago
  I don't think it matters. With an index on username you don't need to scan. Without an index (like in the example), you have to scan on average half the table for existing names and the full table for missing ones. Adding a check for the password won't affect performance in a way that matters. The main reason why you can't do this in a correctly written application is that it's incompatible with salted hashes, since you need to get the salt from the database to verify the hash. (You should calculate the hash in the app server, since it's expensive and scaling app servers is usually easier and cheaper than scaling db servers)
mrbonner 6y ago
Not the worst, though. It maybe insecure because the app was intended for internal use. I did, however, see some shitty stuff in my job. Like, someone iterates a hashtable to find a key. I didn’t even know what to explain to that person when I reviewed the code.
deleted 6y ago
[deleted]
anamexis 6y ago
Hilarious, and a great breakdown of what's wrong with the code! But also, in an article calling out programming falsehoods, this snippet is a bit odd: > Even if apiService.sql returns a value synchronously (which I doubt), internally it have to open a connection to a database, make a query and send back the response, which (as you may have guessed) can’t be synchronous. Sure it can, it's dead simple to write an API that accepts a database query and returns the result synchronously.
- ErikAugust 6y ago
  But how would you call this API from the browser synchronously? I guess in theory there is a synchronous XHR call but I’ve never seen it used. That could be something you add to the problems.
  - Izkata 6y ago
    In the early days of ajax proliferation, I remember reading blog posts where people recommended synchronous XHR to keep your code linear, instead of trying to understand callbacks, which would make the code too complicated.
    - bradleybuda 6y ago
      Obviously crazy advice that the current generation of Javascript programmers was wise to disregard. /s
      - idreyn 6y ago
        Well, yeah. You have one thread to work with in the browser, and if you tie it up with a synchronous XHR your application freezes until the request finishes.
  - cortesoft 6y ago
    Synchronous XHR calls were not that uncommon back in the day.
jl2718 6y ago
But the dev made 15 commits this week and knocked out 5 Trello tasks. All tests pass. Top performer.
kaetemi 6y ago
"Just throw more hardware at it."
Izkata 6y ago
Code formatting is indeed minor here compared to the rest, but you missed the one that popped out at me: The opening curly braces are sometimes on the same line as the condition, sometimes on the following line. That said, > I’m absolutely sure that the code above is fake. > That’s the first time that I see a synchronous SQL query: var accounts = apiService.sql( "SELECT * FROM users" ); Pretty sure you meant synchronous ajax/web request rather than sql query. This could be old code, because it totally used the be a thing: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Synchronous_and_Asynchronous_Requests#Synchronous_request https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequ... Edit: Actually hell, it still works, at least in Firefox. It just also logs the deprecation warning mentioned on that page. I thought this was already removed.
- tom_ 6y ago
  You'd have to see all of the code to be sure, but the braces here are on the same line as the statement that opened the scope, except one case where the opening line was split. You don't have to approve, but there may at least be a reason.
- lolc 6y ago
  Funny I almost commented that you must be mistaken because it would be positively insane to support synchronous requests. Wow, that is insane. In the end, synchronization can be achieved by busy waiting. So the implementation of apiService.sql() could just rely on that even if the browser API didn't support it. The code still looks fake to me. Because you have to know quite a bit to create a combination of fails like that. Natural bad code I've seen is less purposefully structured. But maybe I just haven't been exposed to this type.
  - Izkata 6y ago
    > In the end, synchronization can be achieved by busy waiting. So the implementation of apiService.sql() could just rely on that even if the browser API didn't support it. I haven't tested this, but I don't think it can. The result callback doesn't have a chance to run if there's a busy loop - javascript is single-threaded, and callbacks only fire when idle (think cooperative multitasking, not preemptive multitasking). Webworkers may get around this, but I've not used them and am not sure of the details on how they work. A quick look at a guide indicates they still rely on event callbacks in the main thread, so if my thought above is right, it looks like they wouldn't work either.
deleted 6y ago
[deleted]
jarym 6y ago
But the employer wanted a full-stack developer, everything else is just details! I’ve seen worse code than this. How about a hard coded backdoor check that looked like: || pw == “debug0”
- mercer 6y ago
  Hmm. I have hard-coded superusers for an app that are based on email address (where email is the login username). I'd love to hear from the HN experts if that is a very bad idea and how to do it better. (initially I had a 'superuser' permission that would sort of short-circuit the permission system, but I felt really uncomfortably about how much that affected my code. I figured having compile-time hard-coded superusers per-app-instance would make more sense).
  - rtlfe 6y ago
    > I'd love to hear from the HN experts if that is a very bad idea Yes > and how to do it better. Put these users/passwords in the database like all the other users so that you can shut them down when the passwords leak to the public.
    - bzb3 6y ago
      You can also shut the user down by removing the backdoor in the code.
      - rtlfe 6y ago
        To do it via code, you have to write the code change, run it to make sure you didn't break anything, get the code reviewed, wait for it to compile, and then deploy. In database, you run one simple query.
    - mercer 6y ago
      They are in the database and need to be added as users the regular way. It's just 'marking' them as superadmins that is done through a config file.
seanwilson 6y ago
I've seen something like this before that allowed login with any password as long as you gave a valid user name: > if (hash(password == hashedPassword)) { login(username); } "Don't write your own hash function" is common advice, but I would go much further and say don't write anything to do with passwords, logins, roles or sessions etc. if you can because one small mistake is all it takes to create a huge security hole.
- debarshri 6y ago
  It really depends what kind of product you are building. Sometimes, especially an enterprise product, monolithic in nature, you have to building user management, session management, role management into the product. It does take a seasoned engineer to do to so. But lot of these concepts are well established. I would say dont write your own hash function, dont create your own concepts in user, role and session management.
  - samus 6y ago
    Enterprises with a significant number of IT services are well-adviced to think about consolidation of authentication and authentication mechanisms. There is a huge zoo of architectures to choose from (LDAP/AD, Kerberos, OAuth, Radius, OpenID) and most off-the-shelf software has integrations for these. Every password that your users have to remember increases the chance that they use unsafe ones or even write them down on sticky notes on their desktop. Yes, middlewares have bugs too, but most of the trivial ones likely have been found long ago.
- jedimastert 6y ago
  Does anyone have any experience with password-less login? Like were you just do the other half of a 2fa like an email? And I mean both experience as a consumer or developer. I feel like if I were to have a service that required log in that's how I would do it, but with all the talking about it I've never actually seen it in the wild.
  - bobbylarrybobby 6y ago
moltar 6y ago
I call bs on the code being real, because of triple equal operator. Doubt this developer would know such thing.
- jschwartzi 6y ago
  Every JS IDE warns you about that so I could see a developer correcting the warning.
  - wonderlg 6y ago
    Do you think this dev uses an IDE?
- deleted 6y ago
  [deleted]
rl3 6y ago
>... why they’re not hashing passwords inside of their database? Sending passwords plaintext over the wire isn't good either, even with TLS. You don't want your server to have any knowledge of the plaintext password. For that reason, it's a good idea to salt and hash passwords client-side as well. An even better idea is to not roll your own authentication if you can help it.
- eat_veggies 6y ago
  What's the threat model you have in mind for salting and hashing on the client? TLS is quite strong, and if your adversary has broken your TLS then they can also intercept cookies or inject code to capture the plain text.
  - thaumasiotes 6y ago
    The only threat model this would actually address is a threat from the user himself. Hashing on the client side means the user probably doesn't know his own password. But he still knows how to log in, so the password he accidentally discloses to someone else is trivially convertible to his actual password. (The threat model has to involve an accident, because if the user wants to know his password, he can just look at it when he logs in.) The strategy is called "passing the hash", and it will be flagged as a low-priority problem if you get a security review. It doesn't introduce a weakness, but it's a sign that you don't know what you're doing. (The more classical form is that you _only_ do the hashing on the client side, in which case you've also introduced the issue that your database stores everyone's password in plaintext.)
    - rl3 6y ago
      >The only threat model this would actually address is a threat from the user himself. Incorrect. Please refer to other comments in reply to you.[0][1] >It doesn't introduce a weakness, but it's a sign that you don't know what you're doing. How's the old proverb go? "Those who live in glass houses should not throw stones" [0] https://news.ycombinator.com/item?id=24026585 https://news.ycombinator.com/item?id=24026585 [1] https://news.ycombinator.com/item?id=24026954 https://news.ycombinator.com/item?id=24026954
bane 6y ago
Ha, this is nothing. We once fired a programmer and had her turn over her code after she failed to turn any work in after a few months. Her code was atrocious and also none of it worked...or even was runnable. The biggest sin was that she wrote all of her code in MS-Word, smart-quotes and all on all the literals. Never once ever tried to run or test any code, just thousands of lines of useless stuff that "looked" like code. Oh yeah, she's also an adjunct CS professor at a local college.
- colmvp 6y ago
  > We once fired a programmer and had her turn over her code after she failed to turn any work in after a few months How did that go un-noticed for months?
  - watwut 6y ago
    We had something similar with a dude. Went "unnoticed" for months, then moved around teams doing nothing in multiple for months.
  - mercer 6y ago
    We had a guy who, after a month of sitting smack-dab in the center of the IT department being busy on his new MacBook, got caught with his pants down because he finally asked for help, and me and a bunch of other devs stood behind him watched in horror as we discovered that not only did he know nothing about programming, he didn't know /any/ keyboard shortcuts. When we asked him to copy some code from one file to another, he'd click on 'edit' and 'copy', struggle to get to the other file, and then click on 'edit' and 'paste'. How it went unnoticed is that we were all busy with our own stuff, he was put on some small project that none of us were working on, and we just assumed that nobody would be stupid enough to hire someone without at least /some/ vetting. He slipped through the cracks because he got hired into another department, and when they found out that he'd worked as a 'developer' at Microsoft they figured he'd be more useful in our department. In hindsight I feel I should've known something wasn't quite right when every time I walked past his brand-new MacBook, he seemed to /really/ be struggling with something and it never looked like it was code. I imagine he spent the entire month figuring out how to deal with not having a 'start' button. EDIT: I'll add that while this was in my top 10 of worst situations, there are many others that, for me, provided a convincing argument against the idea that corporations are somehow more efficient or whatever than the government. My experience with both the public and private sector, beyond a certain size at least, is that they're largely similar in wastefulness/inefficiency/idiocy/etc.
riffraff 6y ago
I swear, I conducted an interview once where the candidate actually did this (tho it was server side). They could write fizzbuzz tho.
fao_ 6y ago
Hold up. The expectation of junior developers seems pretty low. I think I've been job seeking below my mark...
- alephu5 6y ago
  Yes, this is beyond incompetent. Even someone without any programming knowledge would probably know that you can't have an API for checking the username and password combinations of all users.
toomanybeersies 6y ago
> Here we can see the use of the double quotes for writing strings ... Here we can see single quotes ... This may not look important, but it’s actually telling us that the developer has probably copied some code from StackOverflow without even rewriting it following a common style guide for the whole codebase. I have a habit of mixing double and single quotes, depending on what side of bed I woke up on. Doesn't mean I copied my code from Stackoverflow.
- rtlfe 6y ago
  I work in a language that only allows double quotes, but I still occasionally type a string with single quotes because that was the accepted style at my last job.
- ziml77 6y ago
  Same. It's very easy to mix them, sometimes on lines entered minutes apart. I'm used to double quotes in C#, C++, Rust, etc. so it's just my natural fallback when coding in Python. I find myself doing it with SQL occasionally too, but there it's quite obvious right after entering the double quote since the string syntax highlighting doesn't kick in.
  - mark-r 6y ago
    I don't seem to have a problem with context switching. I use C++ 90% of the time, but in Python I use single quotes just because I'm too lazy to use the Shift key.
- linkdd 6y ago
  When my linter screams at me for not using the correct quotes, I usually think "who cares... it's a string and you know it". I don't really understand why some languages allow multiple syntax for the same thing. But it's good to know that in some languages (most shell scripts, Groovy, ...) double-quotes and single-quotes are not exactly the same. String substitution does not exist with single quotes, and I always take far too long to notice that this is why my code is not working.
jedimastert 6y ago
Does anyone have any experience with password-less login? Like were you just do the other half of a 2fa like an email? And I mean both experience as a consumer or developer. I feel like if I were to make a service that required log in that's how I would do it, but with all the talking about it I've never actually seen it in the wild.
- jamil7 6y ago
  I believe notion used to do something similar with one time passwords but they seemed to have dropped that.
- mercer 6y ago
  Do you mean stuff like Medium's 'magic link' approach? enter email address, get a link via email, click on link: magically logged in! If so, that's exactly what I've discussed with various clients to implement for their projects, because I agree that in many cases it's a really nice and, as far as I can tell, safe approach. Personally I really don't like it, because I have a password manager and I rarely have my email open in my browser. But for many, if not most 'casual' computer users it does seem ideal to me.
- flak48 6y ago
  Firebase offers email login (passwordless) as an auth method. Where you are emailed a new URL that logs you in, each time you wish to. It also offers SMS based login in a similar manner where no passwords are involved and you just copy a 6 digit or so number from a SMS you receive upon clicking 'Login' In India this SMS based login method without passwords is the norm for almost all locally developed apps I've used (despite SMS as the sole authentication factor being not really secure due to the possibility of sim swapping, etc). Disney Plus (Hotstar) in India has recently started mandating users to switch to such SMS based single factor auth (from password based login), presumably to add friction for account sharers.
- Macha 6y ago
  It feels like a great way to lose users in the login process. They try to log in, go to their email, get distracted by a facebook/whatever login. Even as a user, it's more friction than a social login or password manager. This is effectively what I needed to do when I used vatsim prior to using a password manager as they made you use a password set by the app that was 8 characters alphanumeric so I used to end up having to reset it a lot.
jlengrand 6y ago
A colleague of mine once turned in A FULL JAVA APPLICATION all written in the static void main method. Using booleans in databases polled at 25k/s to check for completion of async tasks. Worst is, it was working. I spent 6 months rewriting it, using it as a black blox to recreate the same system, including bugs because we were interfacing with other systems. Infuriating given that we were a team people team, but also a great deal of fun from the engineering's side. EDIT: Forgot to mention I was the junior dev and he had in excess of 15 years of experience :D
- ur-whale 6y ago
  > he had in excess of 15 years of experience From your description, it looks like the code was optimized to irritate while remaining functional. Might he have been pissed with the institution he worked for?
  - jlengrand 6y ago
    No, I think he really tried his best, working more than 55 hours / week towards the end. He had been an architect for more than 10 years in a large corp, and joined this small company I joined too. I think the expectations, lack of coding practice for a few years, architecture jargon that made him seem like an expert was a recipe for disaster. Not completely his fault.
    - carlmr 6y ago
      >He had been an architect for more than 10 years in a large corp, and joined this small company I joined too. A lot of "software architects" are just people that have been at a big corporation long enough that they found someone willing to promote them, but they don't have social skills to put them in a managerial role. In my experience it often also means they stop coding and only do presentations and (bad) UML Diagrams. Leading to the complete deterioration of their skills.
      - jlengrand 6y ago
        Well, I'm not the one who said it. I have met some architects that were a crucial part of the company. I have also met exactly those that you mention. And arguably they get promoted because they do less harm there than in a team of developers. . .
shusson 6y ago
Anyone else find the quotation marks in the original picture suspicious? Especially around: if ("true" === "true") { return false; }
al2o3cr 6y ago
TBH if this is the "worst code you've ever seen", bless you sweet summer child.
- runawaybottle 6y ago
  Right? I suppose he/she is focusing on the naive authentication piece, but I honestly thought most of the code was clean and sensible (the dev is just inexperienced). I’d rather teach this person the technology than someone who constructs labyrinth-like mental models, but knows the technology. They are code terror incarnate. There are many people that could take that simple function, split it across three files, with various events being dispatched, with “elegant” switch cases, and over abstracted utility functions all over the place. These are the minds I fear, these monsters will eat your sanity with their contraptions. You know nothing Jon Snow, this code is good. I know what the person was trying to do, and it can mostly be solved. Please purchase a ticket to a few React apps, or a home grown php framework. Then we’ll talk. You have not met a real mind eater yet.
_ZeD_ 6y ago
I can only suggest you to spend some time on thedailywtf.com ... it can be frustating as much as hilarious
deleted 6y ago
[deleted]
esgwpl 6y ago
I like how everyone's answering questions about someone's personal experience with even more personal experience, I'm not being sarcastic, keep them coming, please.
deleted 6y ago
[deleted]
nerder92 6y ago
Honest question, is code a very important part of our job as a developers, seniors or junriors? I truly think about code as an implementation details, I don't really care how the code looks as soon as is quality tested (and cross-functionally testes, ie: security). I agree with the sense of the article in general, this code lacks basic engineering/programming concepts, but I generally disagree on the fact that code as a piece of text that execute a software have any meaning of "beauty" standard as such. Code is just the way we have in this specific moment in time to describe the solution of a problem, very far from being the only one ore the "final" one. Therefore I would not invest much time in putting lipstick on cows.
- kingdomcome50 6y ago
  In short. Yes. Most code needs to be maintained and understood(!), so having some semblance of standardization (in its many forms) usually provides benefits to the stakeholders. And to address your last sentence, I think it’s important to understand that for a skilled engineer it _does not necessarily_ take more time to write better code!
  - nerder92 6y ago
    I totally agree with you on the fact that for a skilled engineer good code does not comes with a bigger cognitive load (nor time spent). But I would like to challenge your point on writing maintainable and understandable code. One can argue that writing maintainable code following the best practices it's just a set of conventions that are useful just because we have no better solution right now. It's a solution that works but can be very well be improved for instance by removing the needs of writing code in the first place (ie: with some very advanced level of code generation). About the understanding bit I usually look at tests for understanding code, it's easier for me to read an English statement that tells me what a things does rather then trying to understand it starting from how the problem has been solved in details. Code is for machines, "it should do this" is for humans.
    - lolc 6y ago
      > it's just a set of conventions that are useful just because we have no better solution right now. Every profession has conventions on how their solutions are expressed. Go against the convention, and you'll make us work harder to understand you. This is not appreciated. If you want to establish a new convention, show us how yours is better.
8lall0 6y ago
This is genuinely stupid code from a first-week-junior dev who is still learning a lot. The worst code i've ever seen was a PHP function that printed a menu (with relative permissions) from a database. Problem: it was O(n^3), 200 lines long, completely unmantainable and with random fails. Problem n° 2: it was written by our "best" programmer. It took me one hour to rewrite that into a 10 lines O(nlogn) function that my PM didn't want to use inside the framework because it was my first week.
- techslave 6y ago
  i hope you left before the 2nd week!
Seb-C 6y ago
I have seen way worse than this. In a "corporate [bull]shit" setting, I could easily imagine scenarios where the synchronous client-side query could make sense and be the best available option. Thankfully I have escaped this kind of workplace long ago. The worse codebases IMHO are the ones that have random associative arrays that are exchanged and mutated randomly in any part of the code. Give it a few years of maintenance by bad developers, and you end up with a mess of conditional workarounds and variables that can contain up to a dozen of different data structures with inconsistent properties.
aljgz 6y ago
This is a piece of jewelry, compared to what I've seen. Our team was called in to help on a strange case. An important organization had corruption in the management and they had contracted a mission-critical service, one that created income, to someone. The code was written in asp (the project has started in 2016, 16 years after the last release). Database tables where named table_01 to table_36, columns named Column_01 to whatever. All values stored in database were "encrypted" using base64 most files where 10,000+ lines of repeated code, no modularity, no naming convention, not DRY. All the income from the orders went to the programmer's personal bank account. The money would then be wired to the right companies. Well, this was just the beginning, the code was much worse, I cannot bring any examples as I don't have the code and don't remember, but suffice to say that even though we had made our minds that we're going to handle this mess, but faced much much worse problem than we expected. We came up with a smooth plan to transition to a good state. Meanwhile, the corrupt management had been lobbying, got reinstated, made the version 2 contract with the same guy and the team I consulted was asked to just keep the version 1 running while the genius is coming up with more brilliant ways to write version 2 of pure shit.
ConcernedCoder 6y ago
This can't be real, it seems to be especially crafted to break most if not all common-sense programming rules...
rawfan 6y ago
I just saw an atrocious piece of Perl code that controls the flow of shipping containers for several large corporations. It’s completely unmaintainable and only the original author has the complete picture. He was originally a trucker on office duty because of health issues. In the office he saw lots of inefficient thing so he bought a copy of „Perl for Dummies“ and started fixing. The product itself is actually a huge success. No developer is willing to stay, though, so the guy just picks other people from the office and teaches them what he thinks is Perl.

143 points by micheleriva 6y ago

I'm sure most of us have seen far worse and more nonsensical code, it's a nicely designed snippet to teach basic security issues and show silly mistakes though.

banana_giraffe 6y ago

According to a reddit thread [1] from a few years ago, this is from an internal application, though on a public facing server. Still, I agree with one of the posts on reddit, this is a decent ice breaker interview question to see how many problems the candidate can find. [1] https://www.reddit.com/r/programminghorror/comments/66klvc/this_javascript_code_powers_a_1500_user_intranet/ https://www.reddit.com/r/programminghorror/comments/66klvc/t...

ExcavateGrandMa 6y ago

some ppl are epic! particularly ppl who can't explain what they doing... :)

deleted 6y ago

[deleted]

29athrowaway 6y ago

1. SQL injection as a service. 2. It should implement this logic server-side. 3. No hashing + salting of passwords. 4. It should retrieve one user at a time, rather than all of them. 5. Not understanding basic control flow. ... There was probably a presentation about this project, where the author received a round of applause. This person was likely promoted for finishing this project in record-time. The developer that spent time reporting the issue lowered their own performance metrics in exchange for a bug report that was given a low priority. When the developer objected to the prioritization, the project leadership got pissed off and punished the developer evaluation citing reasons such as "does not align with business needs", "has poor communication skills" (talks abstract things that nobody understands or cares about). By the time this defect was found and fixed, the author likely pushed 10 different other defects just like this. Also, other engineers copied and pasted this code multiple times because it's working and has unit tests.

Ar-Curunir 6y ago
Your conclusions are entirely unsubstantiated...
unsignedchar 6y ago
Were you going for a comic effect or tragic?
- 29athrowaway 6y ago
  Is Office Space a comedy or a documentary?
  - eckza 6y ago
    ... yes.

Sniffnoy 6y ago

One bit of awfulness the article missed: Rather than scanning for the username and then checking whether the password is correct, it scans for a match on both username and password. So if you have the right username but the wrong password, it still has to check every single username to conclude this; it won't stop when it gets to the username you entered. I mean, not that it should be scanning through a list to find the right username, but still... (And yeah, I suppose that would allow for some sort of username enumeration via a timing attack, but somehow I don't think that's the reason they didn't do that here...)

champtar 6y ago
You assume that username are unique in this DB, maybe they support multiple password :)
- onemiketwelve 6y ago
  What a neat feature ;)
  - Viliam1234 6y ago
    If you forget a new password, you can create a new one, and then if you remember the old password later, you can use both. Is this so different in principle from supporting multiple methods of authentication? :D
CodesInChaos 6y ago
I don't think it matters. With an index on username you don't need to scan. Without an index (like in the example), you have to scan on average half the table for existing names and the full table for missing ones. Adding a check for the password won't affect performance in a way that matters. The main reason why you can't do this in a correctly written application is that it's incompatible with salted hashes, since you need to get the salt from the database to verify the hash. (You should calculate the hash in the app server, since it's expensive and scaling app servers is usually easier and cheaper than scaling db servers)

mrbonner 6y ago

Not the worst, though. It maybe insecure because the app was intended for internal use. I did, however, see some shitty stuff in my job. Like, someone iterates a hashtable to find a key. I didn’t even know what to explain to that person when I reviewed the code.

deleted 6y ago

[deleted]

anamexis 6y ago

Hilarious, and a great breakdown of what's wrong with the code! But also, in an article calling out programming falsehoods, this snippet is a bit odd: > Even if apiService.sql returns a value synchronously (which I doubt), internally it have to open a connection to a database, make a query and send back the response, which (as you may have guessed) can’t be synchronous. Sure it can, it's dead simple to write an API that accepts a database query and returns the result synchronously.

ErikAugust 6y ago
But how would you call this API from the browser synchronously? I guess in theory there is a synchronous XHR call but I’ve never seen it used. That could be something you add to the problems.
- Izkata 6y ago
  In the early days of ajax proliferation, I remember reading blog posts where people recommended synchronous XHR to keep your code linear, instead of trying to understand callbacks, which would make the code too complicated.
  - bradleybuda 6y ago
    Obviously crazy advice that the current generation of Javascript programmers was wise to disregard. /s
    - idreyn 6y ago
      Well, yeah. You have one thread to work with in the browser, and if you tie it up with a synchronous XHR your application freezes until the request finishes.
- cortesoft 6y ago
  Synchronous XHR calls were not that uncommon back in the day.

jl2718 6y ago

But the dev made 15 commits this week and knocked out 5 Trello tasks. All tests pass. Top performer.

kaetemi 6y ago

"Just throw more hardware at it."

Izkata 6y ago

Code formatting is indeed minor here compared to the rest, but you missed the one that popped out at me: The opening curly braces are sometimes on the same line as the condition, sometimes on the following line. That said, > I’m absolutely sure that the code above is fake. > That’s the first time that I see a synchronous SQL query: var accounts = apiService.sql( "SELECT * FROM users" ); Pretty sure you meant synchronous ajax/web request rather than sql query. This could be old code, because it totally used the be a thing: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Synchronous_and_Asynchronous_Requests#Synchronous_request https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequ... Edit: Actually hell, it still works, at least in Firefox. It just also logs the deprecation warning mentioned on that page. I thought this was already removed.

tom_ 6y ago
You'd have to see all of the code to be sure, but the braces here are on the same line as the statement that opened the scope, except one case where the opening line was split. You don't have to approve, but there may at least be a reason.
lolc 6y ago
Funny I almost commented that you must be mistaken because it would be positively insane to support synchronous requests. Wow, that is insane. In the end, synchronization can be achieved by busy waiting. So the implementation of apiService.sql() could just rely on that even if the browser API didn't support it. The code still looks fake to me. Because you have to know quite a bit to create a combination of fails like that. Natural bad code I've seen is less purposefully structured. But maybe I just haven't been exposed to this type.
- Izkata 6y ago
  > In the end, synchronization can be achieved by busy waiting. So the implementation of apiService.sql() could just rely on that even if the browser API didn't support it. I haven't tested this, but I don't think it can. The result callback doesn't have a chance to run if there's a busy loop - javascript is single-threaded, and callbacks only fire when idle (think cooperative multitasking, not preemptive multitasking). Webworkers may get around this, but I've not used them and am not sure of the details on how they work. A quick look at a guide indicates they still rely on event callbacks in the main thread, so if my thought above is right, it looks like they wouldn't work either.

deleted 6y ago

[deleted]

jarym 6y ago

But the employer wanted a full-stack developer, everything else is just details! I’ve seen worse code than this. How about a hard coded backdoor check that looked like: || pw == “debug0”

mercer 6y ago
Hmm. I have hard-coded superusers for an app that are based on email address (where email is the login username). I'd love to hear from the HN experts if that is a very bad idea and how to do it better. (initially I had a 'superuser' permission that would sort of short-circuit the permission system, but I felt really uncomfortably about how much that affected my code. I figured having compile-time hard-coded superusers per-app-instance would make more sense).
- rtlfe 6y ago
  > I'd love to hear from the HN experts if that is a very bad idea Yes > and how to do it better. Put these users/passwords in the database like all the other users so that you can shut them down when the passwords leak to the public.
  - bzb3 6y ago
    You can also shut the user down by removing the backdoor in the code.
    - rtlfe 6y ago
      To do it via code, you have to write the code change, run it to make sure you didn't break anything, get the code reviewed, wait for it to compile, and then deploy. In database, you run one simple query.
  - mercer 6y ago
    They are in the database and need to be added as users the regular way. It's just 'marking' them as superadmins that is done through a config file.

seanwilson 6y ago

I've seen something like this before that allowed login with any password as long as you gave a valid user name: > if (hash(password == hashedPassword)) { login(username); } "Don't write your own hash function" is common advice, but I would go much further and say don't write anything to do with passwords, logins, roles or sessions etc. if you can because one small mistake is all it takes to create a huge security hole.

debarshri 6y ago
It really depends what kind of product you are building. Sometimes, especially an enterprise product, monolithic in nature, you have to building user management, session management, role management into the product. It does take a seasoned engineer to do to so. But lot of these concepts are well established. I would say dont write your own hash function, dont create your own concepts in user, role and session management.
- samus 6y ago
  Enterprises with a significant number of IT services are well-adviced to think about consolidation of authentication and authentication mechanisms. There is a huge zoo of architectures to choose from (LDAP/AD, Kerberos, OAuth, Radius, OpenID) and most off-the-shelf software has integrations for these. Every password that your users have to remember increases the chance that they use unsafe ones or even write them down on sticky notes on their desktop. Yes, middlewares have bugs too, but most of the trivial ones likely have been found long ago.
jedimastert 6y ago
Does anyone have any experience with password-less login? Like were you just do the other half of a 2fa like an email? And I mean both experience as a consumer or developer. I feel like if I were to have a service that required log in that's how I would do it, but with all the talking about it I've never actually seen it in the wild.
- bobbylarrybobby 6y ago

moltar 6y ago

I call bs on the code being real, because of triple equal operator. Doubt this developer would know such thing.

jschwartzi 6y ago
Every JS IDE warns you about that so I could see a developer correcting the warning.
- wonderlg 6y ago
  Do you think this dev uses an IDE?
deleted 6y ago
[deleted]

rl3 6y ago

>... why they’re not hashing passwords inside of their database? Sending passwords plaintext over the wire isn't good either, even with TLS. You don't want your server to have any knowledge of the plaintext password. For that reason, it's a good idea to salt and hash passwords client-side as well. An even better idea is to not roll your own authentication if you can help it.

eat_veggies 6y ago
What's the threat model you have in mind for salting and hashing on the client? TLS is quite strong, and if your adversary has broken your TLS then they can also intercept cookies or inject code to capture the plain text.
- thaumasiotes 6y ago
  The only threat model this would actually address is a threat from the user himself. Hashing on the client side means the user probably doesn't know his own password. But he still knows how to log in, so the password he accidentally discloses to someone else is trivially convertible to his actual password. (The threat model has to involve an accident, because if the user wants to know his password, he can just look at it when he logs in.) The strategy is called "passing the hash", and it will be flagged as a low-priority problem if you get a security review. It doesn't introduce a weakness, but it's a sign that you don't know what you're doing. (The more classical form is that you _only_ do the hashing on the client side, in which case you've also introduced the issue that your database stores everyone's password in plaintext.)
  - rl3 6y ago
    >The only threat model this would actually address is a threat from the user himself. Incorrect. Please refer to other comments in reply to you.[0][1] >It doesn't introduce a weakness, but it's a sign that you don't know what you're doing. How's the old proverb go? "Those who live in glass houses should not throw stones" [0] https://news.ycombinator.com/item?id=24026585 https://news.ycombinator.com/item?id=24026585 [1] https://news.ycombinator.com/item?id=24026954 https://news.ycombinator.com/item?id=24026954

bane 6y ago

Ha, this is nothing. We once fired a programmer and had her turn over her code after she failed to turn any work in after a few months. Her code was atrocious and also none of it worked...or even was runnable. The biggest sin was that she wrote all of her code in MS-Word, smart-quotes and all on all the literals. Never once ever tried to run or test any code, just thousands of lines of useless stuff that "looked" like code. Oh yeah, she's also an adjunct CS professor at a local college.

colmvp 6y ago
> We once fired a programmer and had her turn over her code after she failed to turn any work in after a few months How did that go un-noticed for months?
- watwut 6y ago
  We had something similar with a dude. Went "unnoticed" for months, then moved around teams doing nothing in multiple for months.
- mercer 6y ago
  We had a guy who, after a month of sitting smack-dab in the center of the IT department being busy on his new MacBook, got caught with his pants down because he finally asked for help, and me and a bunch of other devs stood behind him watched in horror as we discovered that not only did he know nothing about programming, he didn't know /any/ keyboard shortcuts. When we asked him to copy some code from one file to another, he'd click on 'edit' and 'copy', struggle to get to the other file, and then click on 'edit' and 'paste'. How it went unnoticed is that we were all busy with our own stuff, he was put on some small project that none of us were working on, and we just assumed that nobody would be stupid enough to hire someone without at least /some/ vetting. He slipped through the cracks because he got hired into another department, and when they found out that he'd worked as a 'developer' at Microsoft they figured he'd be more useful in our department. In hindsight I feel I should've known something wasn't quite right when every time I walked past his brand-new MacBook, he seemed to /really/ be struggling with something and it never looked like it was code. I imagine he spent the entire month figuring out how to deal with not having a 'start' button. EDIT: I'll add that while this was in my top 10 of worst situations, there are many others that, for me, provided a convincing argument against the idea that corporations are somehow more efficient or whatever than the government. My experience with both the public and private sector, beyond a certain size at least, is that they're largely similar in wastefulness/inefficiency/idiocy/etc.

riffraff 6y ago

I swear, I conducted an interview once where the candidate actually did this (tho it was server side). They could write fizzbuzz tho.

fao_ 6y ago

Hold up. The expectation of junior developers seems pretty low. I think I've been job seeking below my mark...

alephu5 6y ago
Yes, this is beyond incompetent. Even someone without any programming knowledge would probably know that you can't have an API for checking the username and password combinations of all users.

toomanybeersies 6y ago

> Here we can see the use of the double quotes for writing strings ... Here we can see single quotes ... This may not look important, but it’s actually telling us that the developer has probably copied some code from StackOverflow without even rewriting it following a common style guide for the whole codebase. I have a habit of mixing double and single quotes, depending on what side of bed I woke up on. Doesn't mean I copied my code from Stackoverflow.

rtlfe 6y ago
I work in a language that only allows double quotes, but I still occasionally type a string with single quotes because that was the accepted style at my last job.
ziml77 6y ago
Same. It's very easy to mix them, sometimes on lines entered minutes apart. I'm used to double quotes in C#, C++, Rust, etc. so it's just my natural fallback when coding in Python. I find myself doing it with SQL occasionally too, but there it's quite obvious right after entering the double quote since the string syntax highlighting doesn't kick in.
- mark-r 6y ago
  I don't seem to have a problem with context switching. I use C++ 90% of the time, but in Python I use single quotes just because I'm too lazy to use the Shift key.
linkdd 6y ago
When my linter screams at me for not using the correct quotes, I usually think "who cares... it's a string and you know it". I don't really understand why some languages allow multiple syntax for the same thing. But it's good to know that in some languages (most shell scripts, Groovy, ...) double-quotes and single-quotes are not exactly the same. String substitution does not exist with single quotes, and I always take far too long to notice that this is why my code is not working.

jedimastert 6y ago

Does anyone have any experience with password-less login? Like were you just do the other half of a 2fa like an email? And I mean both experience as a consumer or developer. I feel like if I were to make a service that required log in that's how I would do it, but with all the talking about it I've never actually seen it in the wild.

jamil7 6y ago
I believe notion used to do something similar with one time passwords but they seemed to have dropped that.
mercer 6y ago
Do you mean stuff like Medium's 'magic link' approach? enter email address, get a link via email, click on link: magically logged in! If so, that's exactly what I've discussed with various clients to implement for their projects, because I agree that in many cases it's a really nice and, as far as I can tell, safe approach. Personally I really don't like it, because I have a password manager and I rarely have my email open in my browser. But for many, if not most 'casual' computer users it does seem ideal to me.
flak48 6y ago
Firebase offers email login (passwordless) as an auth method. Where you are emailed a new URL that logs you in, each time you wish to. It also offers SMS based login in a similar manner where no passwords are involved and you just copy a 6 digit or so number from a SMS you receive upon clicking 'Login' In India this SMS based login method without passwords is the norm for almost all locally developed apps I've used (despite SMS as the sole authentication factor being not really secure due to the possibility of sim swapping, etc). Disney Plus (Hotstar) in India has recently started mandating users to switch to such SMS based single factor auth (from password based login), presumably to add friction for account sharers.
Macha 6y ago
It feels like a great way to lose users in the login process. They try to log in, go to their email, get distracted by a facebook/whatever login. Even as a user, it's more friction than a social login or password manager. This is effectively what I needed to do when I used vatsim prior to using a password manager as they made you use a password set by the app that was 8 characters alphanumeric so I used to end up having to reset it a lot.

jlengrand 6y ago

A colleague of mine once turned in A FULL JAVA APPLICATION all written in the static void main method. Using booleans in databases polled at 25k/s to check for completion of async tasks. Worst is, it was working. I spent 6 months rewriting it, using it as a black blox to recreate the same system, including bugs because we were interfacing with other systems. Infuriating given that we were a team people team, but also a great deal of fun from the engineering's side. EDIT: Forgot to mention I was the junior dev and he had in excess of 15 years of experience :D

ur-whale 6y ago
> he had in excess of 15 years of experience From your description, it looks like the code was optimized to irritate while remaining functional. Might he have been pissed with the institution he worked for?
- jlengrand 6y ago
  No, I think he really tried his best, working more than 55 hours / week towards the end. He had been an architect for more than 10 years in a large corp, and joined this small company I joined too. I think the expectations, lack of coding practice for a few years, architecture jargon that made him seem like an expert was a recipe for disaster. Not completely his fault.
  - carlmr 6y ago
    >He had been an architect for more than 10 years in a large corp, and joined this small company I joined too. A lot of "software architects" are just people that have been at a big corporation long enough that they found someone willing to promote them, but they don't have social skills to put them in a managerial role. In my experience it often also means they stop coding and only do presentations and (bad) UML Diagrams. Leading to the complete deterioration of their skills.
    - jlengrand 6y ago
      Well, I'm not the one who said it. I have met some architects that were a crucial part of the company. I have also met exactly those that you mention. And arguably they get promoted because they do less harm there than in a team of developers. . .

shusson 6y ago

Anyone else find the quotation marks in the original picture suspicious? Especially around: if ("true" === "true") { return false; }

al2o3cr 6y ago

TBH if this is the "worst code you've ever seen", bless you sweet summer child.

runawaybottle 6y ago
Right? I suppose he/she is focusing on the naive authentication piece, but I honestly thought most of the code was clean and sensible (the dev is just inexperienced). I’d rather teach this person the technology than someone who constructs labyrinth-like mental models, but knows the technology. They are code terror incarnate. There are many people that could take that simple function, split it across three files, with various events being dispatched, with “elegant” switch cases, and over abstracted utility functions all over the place. These are the minds I fear, these monsters will eat your sanity with their contraptions. You know nothing Jon Snow, this code is good. I know what the person was trying to do, and it can mostly be solved. Please purchase a ticket to a few React apps, or a home grown php framework. Then we’ll talk. You have not met a real mind eater yet.

_ZeD_ 6y ago

I can only suggest you to spend some time on thedailywtf.com ... it can be frustating as much as hilarious

deleted 6y ago

[deleted]

esgwpl 6y ago

I like how everyone's answering questions about someone's personal experience with even more personal experience, I'm not being sarcastic, keep them coming, please.

deleted 6y ago

[deleted]

nerder92 6y ago

Honest question, is code a very important part of our job as a developers, seniors or junriors? I truly think about code as an implementation details, I don't really care how the code looks as soon as is quality tested (and cross-functionally testes, ie: security). I agree with the sense of the article in general, this code lacks basic engineering/programming concepts, but I generally disagree on the fact that code as a piece of text that execute a software have any meaning of "beauty" standard as such. Code is just the way we have in this specific moment in time to describe the solution of a problem, very far from being the only one ore the "final" one. Therefore I would not invest much time in putting lipstick on cows.

kingdomcome50 6y ago
In short. Yes. Most code needs to be maintained and understood(!), so having some semblance of standardization (in its many forms) usually provides benefits to the stakeholders. And to address your last sentence, I think it’s important to understand that for a skilled engineer it _does not necessarily_ take more time to write better code!
- nerder92 6y ago
  I totally agree with you on the fact that for a skilled engineer good code does not comes with a bigger cognitive load (nor time spent). But I would like to challenge your point on writing maintainable and understandable code. One can argue that writing maintainable code following the best practices it's just a set of conventions that are useful just because we have no better solution right now. It's a solution that works but can be very well be improved for instance by removing the needs of writing code in the first place (ie: with some very advanced level of code generation). About the understanding bit I usually look at tests for understanding code, it's easier for me to read an English statement that tells me what a things does rather then trying to understand it starting from how the problem has been solved in details. Code is for machines, "it should do this" is for humans.
  - lolc 6y ago
    > it's just a set of conventions that are useful just because we have no better solution right now. Every profession has conventions on how their solutions are expressed. Go against the convention, and you'll make us work harder to understand you. This is not appreciated. If you want to establish a new convention, show us how yours is better.

8lall0 6y ago

This is genuinely stupid code from a first-week-junior dev who is still learning a lot. The worst code i've ever seen was a PHP function that printed a menu (with relative permissions) from a database. Problem: it was O(n^3), 200 lines long, completely unmantainable and with random fails. Problem n° 2: it was written by our "best" programmer. It took me one hour to rewrite that into a 10 lines O(nlogn) function that my PM didn't want to use inside the framework because it was my first week.

techslave 6y ago
i hope you left before the 2nd week!

Seb-C 6y ago

I have seen way worse than this. In a "corporate [bull]shit" setting, I could easily imagine scenarios where the synchronous client-side query could make sense and be the best available option. Thankfully I have escaped this kind of workplace long ago. The worse codebases IMHO are the ones that have random associative arrays that are exchanged and mutated randomly in any part of the code. Give it a few years of maintenance by bad developers, and you end up with a mess of conditional workarounds and variables that can contain up to a dozen of different data structures with inconsistent properties.

aljgz 6y ago

This is a piece of jewelry, compared to what I've seen. Our team was called in to help on a strange case. An important organization had corruption in the management and they had contracted a mission-critical service, one that created income, to someone. The code was written in asp (the project has started in 2016, 16 years after the last release). Database tables where named table_01 to table_36, columns named Column_01 to whatever. All values stored in database were "encrypted" using base64 most files where 10,000+ lines of repeated code, no modularity, no naming convention, not DRY. All the income from the orders went to the programmer's personal bank account. The money would then be wired to the right companies. Well, this was just the beginning, the code was much worse, I cannot bring any examples as I don't have the code and don't remember, but suffice to say that even though we had made our minds that we're going to handle this mess, but faced much much worse problem than we expected. We came up with a smooth plan to transition to a good state. Meanwhile, the corrupt management had been lobbying, got reinstated, made the version 2 contract with the same guy and the team I consulted was asked to just keep the version 1 running while the genius is coming up with more brilliant ways to write version 2 of pure shit.

ConcernedCoder 6y ago

This can't be real, it seems to be especially crafted to break most if not all common-sense programming rules...

rawfan 6y ago

I just saw an atrocious piece of Perl code that controls the flow of shipping containers for several large corporations. It’s completely unmaintainable and only the original author has the complete picture. He was originally a trucker on office duty because of health issues. In the office he saw lots of inefficient thing so he bought a copy of „Perl for Dummies“ and started fixing. The product itself is actually a huge success. No developer is willing to stay, though, so the guy just picks other people from the office and teaches them what he thinks is Perl.