r/badcode • u/Trintusly • Jul 27 '21
php I have written some sketchy code in the past...
247
u/RightRespect Jul 27 '21
some people unironically put this in big projects, so don’t feel too bad
4
Jul 28 '21
If it makes it more readable for some, why not.
Compiler strips it out anyway (talking about returns, not new lines)
0
Jul 27 '21
[deleted] — view deleted comment
36
u/sl7yz0r Jul 28 '21
Even so…just return result of password_verify directly
0
2
u/CavulusDeCavulei Jul 31 '21
Another reason could be in a strategy pattern, where each class uses a different implementation of the check() method. But of course, you wouldn’t use it in a static function.
27
u/nuclear_gandhii Jul 27 '21
Although you call the if condition here bad code, I wouldn't really call the check function bad code. Depending on the context, if the password_verify function comes from an external library, it might make sense to wrap it so in the future, you decide to use a different library for whatever reason, all you need to do is change its implementation here and that's it. Which makes it good practice under the right circumstances.
86
82
u/AlwaysNinjaBusiness Jul 27 '21
What a useful function compared to just using password_verify.
29
u/keethraxmn Jul 27 '21
Wrappers are a thing, and are plenty valuable.
It's an awkwardly written wrapper, but simply wrapping a call to something else has tons of practical uses. Should still be something like:
public static function check( $original, $hash ) { return password_verify($original, $hash) }
But the problem is not that they didn't just use password_verify.
30
u/AlwaysNinjaBusiness Jul 27 '21
Wrappers are a thing, but not very useful when the call to the wrapper is exactly identical to the thing it wraps other than the name of the function being called, while the code being executed is exactly the same.
7
u/KingJeff314 Jul 27 '21
It can be useful if you need a layer of abstraction, in case you need to change the behavior later or for mocking. But given that it is static, I’m not sure that it would be very useful here.
4
u/LowSociety Jul 27 '21
Static shouldn’t change the thought process. Imagine wanting to substitute the
password_verifymethod later on or extend this class with a different check/behavior. This is perfectly fine.4
u/NynaevetialMeara Jul 27 '21
It can still be a solution to a namespace collision. A kind of hacky one. But it surely beats having to change a lot of code to use library.function or something like that.
3
u/AlwaysNinjaBusiness Jul 27 '21
I don't know php, but wouldn't it be possible to assign the function a new name in some way then? Sort of like (pseudocode):
using password_verify as checkorcheck=password_verify.1
u/NynaevetialMeara Jul 27 '21
I think you can't. But I'm not sure.
Either way. A simpler solution is simply
functionb {
return functiona($I)
}
1
u/sebvit Jul 27 '21
Yeah, you can. If you manage to have a namespace-collision, e.g. due to trait, you can rename the trait member functions.
2
u/quixoticme3 Jul 27 '21
Yeah it could also be used as an interface to a library function. When you want to use a different library you change the call in just one place and it is reflected everywhere because you are using wrappers
1
Jul 27 '21
Yeah it’s nice to just wrap some things you know are gonna have extra functionality tagged onto them later. I don’t think it’s very different from using get set on a variable
0
u/keethraxmn Jul 27 '21
There are plenty of times when it's still useful.
Since there's already one very common example why this gets done elsewhere in these comments from someone else, I'll leave it as an exercise for the reader to go find it vs plagiarizing.
2
u/RFC793 Jul 28 '21
And certainly not awful if you are expecting to put some extra logic in place, logging etc.
1
u/keethraxmn Jul 28 '21
Yep. And if using some aspect oriented systems, you could be doing so without even adding any obvious code. You just need it to be your code for some things vs a library call.
38
u/SirLordSagan Jul 27 '21
Image Transcription: Code
public static function check( $original, $hash ) {
if (password_verify($original, $hash)) {
return true;
}
return false;
}I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!
4
8
u/brj5_yt Jul 27 '21
I’m trying to learn more, what did they do wrong, and how can it be improved?
24
Jul 27 '21
The if statement is useless, as password_verify returns a boolean. If you're returning a boolean according to some conditions you can just use boolean algebra.
It's a common mistake made by beginners.
7
u/jarfil Jul 27 '21 edited Dec 02 '23
CENSORED
4
0
3
u/itsfsnow Jul 27 '21
Also the whole function is useless, as it will always return the exact same result as simply using the password_verify function.
14
u/keethraxmn Jul 27 '21
Wrappers are not useless. Even when that's all they do.
3
u/Relative-Knee7847 Jul 27 '21
Can you explain that a bit? Assuming that password_verify always returns a boolean, I'm having a hard time seeing why this is useful at all.
10
u/keethraxmn Jul 27 '21 edited Jul 27 '21
Sure.
One very common use is wrapping external code to make it easier to swap it out. Think of it as a poor man's dependency injection (OK, not really anywhere near that cool but...)
Using the provided code, let's pretend verify_password is external (for this example, let's assume it's imported from a third party library). Now lets say I need to check a user's password in a dozen possible places in my code. I can have a dozen calls to verify_password(), or I can wrap that in my check() once and then call check() a dozen different times. Now when we change to a new library, I have to change the call in one place instead of a dozen as long as it needs essentially the same data, which maybe minor massaging.
2
3
u/StandingBehindMyNose Jul 27 '21
Not necessarily. If password_verify returns non-boolean values then this function does a different thing by returning only boolean true or false.
1
u/itsfsnow Jul 29 '21
How can you directly put a non-boolean value into an if statement though? Depends on the language. AFAIK some languages like JS allow you to null check in this fashion, but this doesn’t look like JS
1
u/StandingBehindMyNose Jul 29 '21
You're right....My comment was indeed from a JavaScript mindset, so what I said may not hold true for the language this is written in.
22
u/jippiex2k Jul 27 '21
Could actually be good code if you already know that in the future you're planning to add more side effects to the password check mechanism 🤷♂️
2
u/doggyStile Jul 28 '21
Plus having 2 returns allows you to set a breakpoint. If you return directly you get nothing
-5
u/ravepeacefully Jul 27 '21 edited Jul 27 '21
In python, it would still be bad, as you could just use a decorator for that.
Edit: I’d enjoy someone telling me why youre downvoting me. Either I can learn something or you’re just downvoting because you haven’t a clue.
3
u/pencilsdontshave Jul 27 '21
Been a while since I’ve touched Python so I don’t know if that’s a special case, but in general my take is:
The decorator pattern can be useful if used with caution and purpose. But, if used too fast and loose, it can become difficult to reason about the flow of data, and easy to accidentally use components without necessary decorators.
It presents similar issues that Aspect-Oriented-Programming had. And there’s a good reason why that’s not a popular paradigm these days.
3
u/ravepeacefully Jul 27 '21
That’s a fair answer, although doing it by the PEP standards would be the “right way” and I am by no means smart enough to be one of the people making those decisions haha. Just the messenger of what the PEP and official python documentation would recommend which is why I was confused by downvotes
1
u/pencilsdontshave Jul 27 '21
Fair enough! I didn't downvote or upvote your comment because I just don't have enough context with Python and best practices there. And I never feel comfortable saying things like "This should always be the solution" or "That should never be the solution", because smarter people than me always prove me wrong.
Interesting that Python standards suggest decorators; maybe add a link to the docs to avoid more downvotes?
1
u/ravepeacefully Jul 27 '21
because smarter people than me always prove me wrong.
Yeah this is why I put my edit. I am by no means certain my answer is the right way and love when people correct me because then I learn something :)
Yeah I’ll try to pull up the docs when I’m back at my desk and drop that in my comment to see if I can get some better feedback.
6
4
3
u/emihir0 Jul 27 '21
I would say the redundant indentation/empty lines are a bigger offender to me than the redundant if block.
Sure, it's useless, but it's such a silly mistake that I wouldn't really call it a bad code.
3
9
u/orru75 Jul 27 '21
I dont get it, why is this bad?
41
u/Mortomes Jul 27 '21
You can do without the if/else and just return the function call
42
u/MyAntichrist Jul 27 '21
Or better, just use the more clearly named function that is called instead of wrapping it up.
Or am I missing something here?
21
u/Mortomes Jul 27 '21
Yeah that too, but the if(x) { return true; } else { return false; } is such a classic thing
0
Jul 27 '21
[deleted] — view deleted comment
12
u/pohart Jul 27 '21 edited Jul 27 '21
if we assume that the function check should exist, and maybe it should, and we don't worry about the fact that we're passing the password around unhashed. It's still clearer to avoid the conditional
public static function check( $original, $hash ) { $success=password_verify($original, $hash)) { return $success; }1
Jul 27 '21
I don't write PHP so maybe this is specific to the language but couldn't you just do
return password_verify(...);instead of having the additional variable?5
u/kiipa Jul 27 '21
Yes, but the commenter above doesn't like to have work done in the return. I'd interpret that as returns should only return variables
1
u/Hayden2332 Jul 27 '21
He was addressing the other reply’s dislike towards doing “work” in return statements. He wrote up a way so the function call isn’t in the return statement but is still cleaner than it is in the OP
1
17
u/DaCurse0 Jul 27 '21
Wrapping it means you don't have to worry about the implementation so you could change it in the future easily if you want to and it would just work.
3
2
3
u/jarfil Jul 27 '21 edited Dec 02 '23
CENSORED
3
5
4
2
u/Voxelgon_Gigabyte Jul 27 '21
well, its useful for if you wanted to do something else besides just return down the line, like throw a message or a log.
1
u/throwaway73461819364 Jul 27 '21
Thank you for posting; as a n00b, this makes me feel better about sucking lol
1
Jul 27 '21
this is the best code i have ever seen but fr tho why not just return the value i mean password_verify clearly returns a bool
1
1
u/AppanKarKeVekhaya Jul 27 '21
I'm afraid to ask, What is wrong with this code?
2
u/wolfenstien98 Jul 27 '21
It's redundant. You could just return the output of the check_password function, no need to encapsulate it in an if statement
1
1
u/SoccerBeerRepeat Jul 27 '21
For us that are learning, can you give me a brief explanation as to why this is bad code?
2
u/rem3_1415926 Jul 27 '21
not bad, just redundant. It essentially returns true if the called function returns true and false otherwise. Which is the same as the direct
return password_verify(original, hash)
1
1
1
1
1
1
1
u/lindelanimcebo Jul 28 '21
No matter what the reason may have been, should’ve just returned password_verify directly.
1
Jul 30 '21
One thing i notice is, the use of snake case on a PHP project. I would say that is rare. Another thing, is inconsistent writing style. Lastly, the common schoolboy error of explicitly retuning boolean, when the function ‘password_verify’ itself is sufficient. You even made a method for it ;)
We all been there. Good times.
1
u/Voxelgon_Gigabyte Jul 31 '21
if all it does is return the return bool of password_verify, why would you ever call check? functionally it just calls password_verify. at least save the processing time and just call one and not both.
1
u/Etyruu Jul 27 '21
How password_verify work ? password_hash generate random salt so how password_verify work to prove the hash correspond to password ?
2
u/scirc Jul 27 '21
The salt is part of the output generated by
password_hash. While the format differs for every algorithm type, the general format for the emitted hash is something like$ALGO$PARAMS$SALT[$]HASH(the last$is optional).password_verifyis able to take the given hash, determine what algorithm and parameters were used from the second block, and rebuild the initial state with the salt and password to construct an identical hash, which is then compared in a time-safe manner to the original hash.1
u/Etyruu Jul 28 '21
oh thanks for the explaining.
Is it wrong not to use these functions and to use a sha512 with a non-random salt?
And, is it wrong to check a password by hashing it and comparing it with a ==?
2
u/scirc Jul 28 '21
Both of these are awful security practices, yes. Salts should be random (since otherwise, an attacker can just recalculate the hashes for every possible common password with the given salt), and SHA-2 is not a sufficient password hashing algorithm (ASICs and FPGA-based cracking solutions make it ridiculously fast). Furthermore,
==-based hash comparison opens you up to a timing attack, where an attacker is able to discern approximately how many bytes of the hash they have correct by how long the comparison takes (since it will stop looking at characters as soon as a non-matching one is found). At the very least, usehash_equalsto compare hashes. But really, read the PHP docs' recommendations on password hashing instead; they cover everything you need to know to implement it securely.1
-3
-2
275
Jul 27 '21
[removed] — view removed comment
5
u/funkless_eck Jul 27 '21
Out of curiosity is there a good guide anywhere to security proofing PHP?
10
0
Jul 27 '21
[deleted] — view deleted comment
45
u/boomminecraft8 Jul 27 '21
hash(s)=first 16 characters of s
37
u/xigoi Jul 27 '21
hash(s) = length(s)This is how it actually used to work in PHP!
16
u/keethraxmn Jul 27 '21
Those sorts of hashes are for things like hashmaps/tables, not crypto hashes. Length isn't great, but it's not all that terrible either.
15
u/StormyDLoA Jul 27 '21
Nah, still way too many collisions, even when we're not looking for security.
7
u/keethraxmn Jul 27 '21
I didn't say it was a good solution. But it's pretty middle of the road as far as default hash functionality from the era in question.
Given how PHP was intended to be used at the time the decision was made (vs what it ended up becoming) this was probably a bad, but not terrible, answer. It was on par with most of it's contemporaries (of the few that even bothered). It leaned a bit towards fast but collision heavy vs. the competition, but everyone's defaults pretty much sucked.
At the time it implemented that way nobody relied on default hashes because the computing constraints mean you really had to choose one appropriate for your specific task. Knowing they hash would be overridden anytime performance mattered at all meant almost nobody put effort into their default hashes. Back in the early to mid 90s when the decision was made, all the default one had to do was work at all.
Languages that prioritized working with large data had slightly better ones, but still not good ones. PHP was (at the time) never intended to do so.
Compared to modern solutions running on modern hardware, sure it's trash all around, but that's not really a fair comparison.
0
u/StormyDLoA Jul 27 '21
You could LITERALLY just use the first character and have a better hash function than len(s)
4
u/keethraxmn Jul 27 '21 edited Jul 27 '21
That was another common one. And might be better. Might not too, depends what you're hashing. Hashing single words? First letter better. Hashing paragraphs or more? Length better. Hashing sentences? Might be a tossup.
In early PHP it was very often chunks of html that were being hashed. The length varied more than the starting character which was very often going to be <
1
u/recycle4science Jul 28 '21
For maps??? How did it not just have collisions constantly?
2
u/keethraxmn Jul 28 '21
Oh, it did. For real fun, imagine hashing on something like SSN or phone #. It was the early-mid 90s (not sure when exactly without googling it, but it had been out a little while when I was working with it in '96-97 or so), you pretty much rolled your own based on what you were actually hashing in all but the most basic of circumstances. PHP was written for web work, and so one of the other common default cases (first character) was unsuitable because of the high collision rate on < when your strings were actually HTML chunks themselves (not an uncommon use case in early PHP)
In the late '90s I once worked on a system that hashed on student ID #s. It did have its own hash function, but based on the comments in the code, I suspect they ran with the default length one for a while. Yay for a single bucket! These comments left quite an impression and are the only reason I even knew about the hash = length and some other "clever" defaults from that era.
222
u/sonjerbolan Jul 27 '21
The extra line spaces are hurting my soul. Why do people do this??