CodeSOD: Sanitary Paths

When accepting user input for things like, say, accessing the filesystem, you need to do some validation. Bad or inappropriate characters could lead to surprises that no one is going to like.

So when Christian first spotted this C# method called SanitizePath, he didn’t think much of it. But then he looked at the implementation…

public static string SanitizePath(string path, char replacementchar) {
string result = “”;
try {
// Split path and filename
FileInfo fi = new FileInfo(path);
string filename = FileUtilities.ReplaceInvalidFileNameChars(fi.Name, replacementchar);
string pathname = FileUtilities.ReplaceInvalidPathChars(fi.DirectoryName, replacementchar);
result = Path.Combine(pathname, filename);
} catch (Exception e) {
Logger.Log(e);
result = path;
}
return result;
}

We accept an input path, and attempt to open it using FileInfo. Now, the fun thing about this is that if the path contains any sort of invalid characters, it throws an ArgumentException. But let’s assume there weren’t any invalid characters.

If there weren’t, and FileInfo was constructed successfully, we then split it by Name and DirectoryName, and replace invalid characters according to some rules not shared here. Then we combine them back together into a full path.

Now, this isn’t a strictly useless function- what their FileUtilities class considers “invalid” may be application specific, and completely unrelated to what the filesystem allows. They may, for example, want to prevent profanity from being in a filename, a clbuttic option. Though, since it says it replaces invalid chars, I suspect it’s avoiding certain letters- it’s possible that this wants to ensure that the files remain readable on different file systems (many a C# app needs to send text files to a mainframe, even today, and they can get real picky about what characters are in those filenames).

But let’s look at the exception path. If the filename can’t be opened because it’s invalid, we… log an error and return the input value. So SanitizePath will modify the strings if they are valid file paths, but if they’re invalid file paths, it just returns the invalid file path with no meaningful information for the caller- just a log message for an admin to check eventually.

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.

Source: Read More