Serguey Asael Shinder: Reject bad input; do not try to repair it
Logback 1.6.5 shipped this week with a fix for a fix. In 1.6.3 the project dealt with MDC values that end up in log file names by removing slashes from them. That turned out to be insufficient: a value could still carry .., variable references or other characters with special meaning downstream. The second fix does something different. It no longer edits the value; it refuses it, and uses a default instead.
That change of approach is worth more than either patch.
Repair assumes you know every bad character. Stripping, escaping out or "cleaning" input means keeping a list of what is dangerous and deleting it. The list is always written against the attacks someone has already thought of, and the input will be read by more than one interpreter - a file system, a pattern language, a mail header - each with its own special characters. The first logback fix knew about slashes. The file-name pattern knew about ${.
Repair also changes meaning. Deleting characters turns one value into another. Two different inputs can become the same output, and a value can become dangerous only after the deletion, as when removing a character joins two harmless pieces into something that is not. The program then acts on a value nobody sent.
Rejection makes the rule small and visible. Logback's new rule says what is acceptable: not empty, at most 64 characters, no .., none of a short list of characters. Anything else is not used. That is easy to test in both directions, a value that must pass and one that must not, and it fails loudly instead of quietly producing something odd.
The test I use. If the code that handles input has to be clever about what it removes, the interface is accepting too much. Decide what valid input looks like, accept exactly that, and send everything else to a default or an error, with a log line that says why.