• (nodebb)

    And as for how useful this method actually is, well… Christian turned it into a no-op, and nothing about the application's behavior changed. It has since been removed entirely.

    I wouldn't be surprised to hear that it was called only for names generated by the application itself.

  • my name (unregistered)

    Stay sane inside insanity (Rocky Horror Picture Show)

  • TheCPUWizard (unregistered)

    Presume that the eventual "worker" (what uses the file path) will throw an error with illegal paths... This approach keeps the same error path, but mitigates certain error conditions.

  • (nodebb)
    Christian turned it into a no-op, and nothing about the application's behavior changed. It has since been removed entirely.

    This is TRWTF. Define "nothing". Was a complete regression testing suite executed? Were business and or technical requirements consulted?

    I mean, I'm sure they lucked on this one, but no, that's not how you go about removing things - let's make it a no-op and see what happens. No no no

  • Michael R (unregistered) in reply to Mr. TA

    You must be new here.

  • (author) in reply to Mr. TA

    Oddly, our submitters don't usually include all the testing steps they did to confirm their fixes, but we can assume they did something. Presumably, if you convert a function into a no-op, the first thing you do is run your test suite, assuming there is one. And if there isn't, then you write a test for at least that much.

  • (nodebb) in reply to Remy Porter

    You must be new here.

    (Riffing off Michael R's entirely realistic reply.)

  • Naomi (unregistered) in reply to Mr. TA

    You know, the problem - well, one of many - with the "condescend first, ask questions never" approach exemplified in this question is that there's usually a reasonable explanation, and ignoring it just makes you look like a no-nothing know-it-all. In this case, the reasonable explanation is that they analyzed and tested to make sure it wasn't necessary, but couldn't rule out a corner case and left it in as a no-op to simplify rollback.

  • (nodebb) in reply to Mr. TA

    let's make it a no-op and see what happens.

    That's called a "scream test" and is a perfectly cromulent way of checking whether $thing_to_be_decommissioned is in use.

  • (nodebb) in reply to The Beast in Black

    My team has insufficient tests (especially integration tests), and is doing some heavy security upgrades. We're doing a LOT of scream tests lately.

    Send help.

  • DQ (unregistered) in reply to The Beast in Black

    Now I finally know how our testing strategy is called. Thanks.

  • (nodebb)

    My process for eliminating a seemingly useless method would be:

    1. Make sure I have a full understanding of what the code to be removed does;

    2. I establish that that action is no longer necessary, by mapping it on to the requirements and or technical notes, as appropriate;

    3. The code is removed (not "no op"ed) and tested (as needed) and released.

    I've never had to resort to the no op approach, never even thought about doing something like that. It's like the perfect recipe to introduce an evil but which happens in 1% of the cases.

    Regarding scream testing, if I understand correctly that it means "wait for somebody to scream", it can be done in certain situations. For example, you have a large organization and some people may be reading from a table, and said table needs to be removed. An email chain is started asking the users of that table to speak up, nobody responds, and you can rename the table temporarily to see if something fails. It's easy to tell because the chance of failure is 100% - a missing table is much more apparent than some characters being removed in some cases.

    This situation in the article doesn't bode well for scream testing.

  • Charles (unregistered)

    The code may be bad, but the change may have been much worse - it might have introduced a security vulnerability. If we are given a potentially hostile input. the exception from FileInfo() tells us that using that path will not work. There may be other paths which work but do bad things - for example, escaping from a directory by containing '..' and the tests for invalid chars might be protecting against that. By taking out the code, non-functional paths still don't work, but now hostile input can refer to paths which should not be accessible.

    Scream testing is like disabling the fire alarm and seeing if anything goes wrong: if you don't understand why it is there, it may be dangerous to remove.

  • Loren Pechtel (unregistered)

    If it's being fed garbage it can't clean it up. I do agree it shouldn't be a silent failure.

    And I strongly suspect this is to sanitize names for some more restrictive system. Since it didn't change anything when removed that more restrictive system likely no longer exists.

  • (nodebb)

    We can't make too much comment without seeing what those FileUtilities methods do. I assume Christian took a look. That fact that it takes a single char as an input makes me wonder if this method is being called from a loop, looking for naughty characters.

    But the biggest issue I have is the lack of comments describing why it's calling those methods, and anything else it's doing. That would be helpful in knowing whether or not removing the method is an option. E.g. if there's a comment saying "removing punctuation characters that system X can't handle", and you know you no longer use system X, then that would be a good start, although it is possible that the new system Y has other restrictions that are being checked here, and no-one has updated the comments.

  • (nodebb)

    @Mr.TA ref

    I establish that that action is no longer necessary, by mapping it on to the requirements and or technical notes, as appropriate;

    An entertaining notion to be sure. In most cases, the running collection of apps and IT infrastructure is the only definition document of that system that actually exists. It's certainly the only one that is up to date. The system does whatever it does however it does it and that is the only definition of "correct" there is. Warts, bugs, and unexplained crashes and all.

    It'd be really neat to live in a world where some spec defines correctness and everything is built to the spec and only the spec. Where the only way to change the app or infrastructure is to first change the specs, debug those, get end-to-end user and management buy-in, then begin altering code and the rest.

    I've never seen this promised land. Perhaps you already live there, but if so you're in a very odd corner of the commercial / government IT world.

  • (nodebb) in reply to prueg

    That fact that it takes a single char as an input ...

    which it doesn't.

    It takes either a directory name or a filename (presumably a string) and a character. My uneducated guess is that it scans the string passed in replacing any invalid characters with the replacement character and returns the new string. If the set of invalid characters is the same as the set of invalid characters that will cause FileInfo() to throw an exception, then the funciotn is, indeed a no-op.

    If it were me, I would check what the list of invalid characters is and only replace the function with a no-op if it was a subset of the characters that cause FileInfo() to throw.

  • (nodebb) in reply to jeremypnet

    If it is a security check (which we cannot tell, so it might not be), it could be deliberately replacing certain characters with those that make FileInfo() throw so that if the caller does not reject the input in the UI, it is not harmful to the rest of the system when it is used, so replacing it with a no-op would create a security vulnerability.

    There is no substitute for understanding what the code does before changing it.

  • A Nonny Moose (unregistered)

    We accept an input path, and attempt to open it using FileInfo. Now, the fun thing about this is that if FileInfo is vulnerable to any sort of malicious input, we just passed it that input without any “sanitisation” as our first step in “sanitising” it.

Leave a comment on “Sanitary Paths”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article