9 ms·
It's funny, I wonder if hearing about that github bug made them check if they had committed the same mistake... only to find that they did :-)
by jahvo 8y ago
It's funny, I wonder if hearing about that github bug made them check if they had committed the same mistake... only to find that they did :-)
- badloginagain 8y agoI think I, and everyone here, should check as well. If capable, security-minded companies can make such a mistake, so can you.
- milesokeefe 8y agoYep, glad I read this thread. We were making the same simple mistake.
- bigiain 8y agoWe aren't. Now. (We caught ourselves doing it 4-5 months back, and went through _everything_ checking... Only random accident that brought it to the attention of anyone who bothered to question it too... Two separate instances by different devs of 'if (DEBUG_LEVEL = 3){ }' instead of == 3 - both missed by code reviews too...)
- jcoffland 8y agoThis is why you should turn on compiler warnings and heed them. It would have caught this.
- bch 8y agoAnd consider “Yoda Notation”[0], which some people find annoying, but I found an easy hurdle to clear: if ( 3 = DEBUGLEVEL ) wouldn’t pass the the parser because you can’t assign to an rvalue. [0] https://en.wikipedia.org/wiki/Yoda_conditions https://en.wikipedia.org/wiki/Yoda_conditions
- bigiain 8y agoYep - I pointed out that I used to do this in Perl back in '95 or so. At least one of the devs wasn't born then, none of them had ever used Perl. (I'm not even sure how they'd ended up with a Grails configuration that'd let them do this anyway...)
- extralego 8y agoThanks for sharing. I am a less experienced programmer and have never seen this before. The name is so wonderful.
- marssaxman 8y agoYes! I've been doing this in C for years. It's a little weird to read at first, but every now and then it really saves you.
- lawl 8y agoIn this specific case DEBUGLEVEL should be a constant anyways, and thus assignment should fail, no? Also kind of denoted by being all caps.
- stan_rogers 8y agoConventions cause assumptions.
- anyzen 8y agoThere are always assumptions being made, no matter what you do. But "uppercase -> constant" is such a generic and cross-platform convention that it should always be followed. This code should never have passed code review for this glitch alone.
- onion2k 8y agoWhich language would stop/warn you assigning the value of a constant to a variable? Doesn't "var = const" just work in most languages?
- astura 8y agoYeah, exactly. This error shouldn't ever happen, period. All modern development tools give big fat warnings when you do this.
- mkagenius 8y agoPeople (atleast me) ignore warnings quite often, they aren’t safe haven if you ask me.
- _sh 8y agoHey no problem, just add -Werror to your compiler flags (C/C++/Java) or '<TreatWarningsAsErrors>true</TreatWarningsAsErrors>' to your csproj (C#).
- kiallmacinnes 8y agoThis! Treat every warning as a failure, ideally in your CI system so people can't forget, and this problem (ignoring warnings..) goes away. You will have a better, more reliable, and safer codebase once you clean up the legacy mess and turn this on..
- acomjean 8y agoI agree. Having worked in a project with warnings as errors on (c++) I found it annoying at first but it made me a better coder in the long run. Plus you get out of the habit of not reading output from the compiler because there are so many warnings...
- maljx 8y agoUnless you follow a zero warning policy they are almost useless. If you have a warning that should be ignored add a pragma disable to that file. Or disable that type of warning if it's too spammy for your project.
- FPGAhacker 8y ago“Should” is a bad word. If you are basing a conclusion off of a “should,” you are skating on thin ice.
- bognition 8y agoseems like a bug in your platform
- inferiorhuman 8y agoI'm curious, how often do you actually need to print out the password in a development context?
- zimpenfish 8y agoI've been working on authentication stuff for the last two weeks and the answer is "more than you'd like". But luckily it's something we cover in code reviews and the logging mechanism has a "sensitive data filter" that defaults to on (and has an alert on it being off in production.)
- deleted 8y ago[deleted]
- __jal 8y agoWe schedule log reviews just like we schedule backup tests. (Similar stuff gets caught during normal troubleshooting, but reviews are more comprehensive.) It only takes one debug statement leaking to prod - it has to be a process, not an event.
- Rapzid 8y agoLog review is an awesome idea. Do you mind divulging your workplace?
- fixitnow 8y agoLog review is done for every single project at my workplace too (Walmart Labs). So I don't think this is a novel idea. And it does not stop there. Our workplace has a security risk and compliance review process which includes reviewing configuration files, data on disk, data flowing between nodes, log files, GitHub repositories, and many other artifacts to ensure that no sensitive data is being leaked anywhere. Any company that deals with credit card data has to be very very sure that no sensitive data is written in clear anywhere. Even while in memory, the data needs to be hashed and the cleartext data erased as soon as possible. Per what I have heard from friends and colleagues, the other popular companies like Amazon, Twitter, Netflix, etc. also have similar processes.
- Rapzid 8y agoIt's novel to me; never worked anywhere that required high level PCI compliance or that scheduled log reviews. Adhoc log review, sure. I think it's a fantastic idea regardless of PCI compliance obligations.
- baud147258 8y agoWe just realised the software I'm working on has written RSA private keys in the logs for years. Granted, it was at debug level and only when using a rarely-used functionnality, but still.
- kdbg 8y ago
- OldSchoolJohnny 8y agoWhat developer in their right mind would ever log a password in the first place? Are we devolving as a profession?
- Michielvv 8y agoCan be more accidental. e.g. dumping full POST data in a more generic way (e.g. on exceptions) that happens to also be applied on the login page.