commons-io icon indicating copy to clipboard operation
commons-io copied to clipboard

Amend description to reflect empty string result for windows drive

Open Philzen opened this issue 1 year ago • 5 comments

Not sure if this is is a bug or a feature – what i can confirm is that in v2.4 it worked as it said in the description:

@Test void returnsHomeWithTrailingSlash_forHomeWithTrailingSlash() {
    assertThat(FilenameUtils.getFullPathNoEndSeparator("C:")).isEqualTo("C:");
}

After upgrading to 2.15.1, i realized my test suite failed, the behaviour is now:

@Test void returnsHomeWithTrailingSlash_forHomeWithTrailingSlash() {
    assertThat(FilenameUtils.getFullPathNoEndSeparator("C:")).isEqualTo("");
}

This PR proposes to reflect this change in the javadoc.

Philzen avatar Jan 04 '24 10:01 Philzen

You've pointed to existing bad API design. getFullPathNoEndSeparator("C:") should return "" if the argument is a Unix path and "C:" if that path is a Windows path. Of course, it's just a string, and we have no way of knowing whether that's a Windows path or a Posix path. In this case, best guess is Windows since "C:" is very common in Windows and very uncommon on Unix, but it's still a guess. Possibly we should deprecate this method and rethink the API here.

elharo avatar Jan 05 '24 13:01 elharo

Possibly we should deprecate this method and rethink the API here.

@elharo Agreed. For the time being, any objection merging this PR to bring the JavaDoc inline with the method's actual behavior?

Philzen avatar Apr 24 '24 13:04 Philzen

`

import org.apache.commons.lang3.Validate; import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.util.Locale; import java.util.Set;

public class PathValidator {

private static final Set<String> RESERVED_NAMES = Set.of(
        "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6",
        "COM7", "COM8", "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6",
        "LPT7", "LPT8", "LPT9"
);

public static void validateSafePathComponent(String component) {
    // --- 1. Basic Checks (Using Apache Commons Lang) ---
    Validate.notBlank(component, "Path component must not be blank");
    Validate.isTrue(component.length() <= 255, "Path component too long");

    // Pre-decode raw dangerous percent encodings
    String lower = component.toLowerCase(Locale.ROOT);
    if (lower.contains("%2f") || lower.contains("%5c") || lower.contains("%00")) {
        throw new IllegalArgumentException("Encoded separator/null not allowed: " + component);
    }

    // --- 2. Decoding ---
    String decoded;
    try {
        decoded = URLDecoder.decode(component, StandardCharsets.UTF_8);
    } catch (IllegalArgumentException ex) {
        throw new IllegalArgumentException("Invalid percent encoding: " + component);
    }

    // --- 3. Path Traversal/Normalization Check (Using Path API) ---
    Path p;
    try {
        p = Path.of(decoded);
    } catch (InvalidPathException e) {
        throw new IllegalArgumentException("Invalid path syntax: " + component);
    }

    // Critical Path Traversal check: must be a single name and not contain '..' or '.'
    if (p.isAbsolute() || p.getNameCount() != 1 || !p.normalize().equals(p)) {
        throw new IllegalArgumentException("Traversal/dot segments or absolute path not allowed: " + component);
    }

    String name = p.getFileName().toString();

    // --- 4. Final Name/Custom Policy Checks ---

    // Basic character / separator checks
    if (name.contains("/") || name.contains("\\") || name.indexOf('\0') >= 0 || name.contains("+")) {
        throw new IllegalArgumentException("Unsafe characters (separators/null/plus): " + component);
    }

    // Control chars
    for (int i = 0; i < name.length(); i++) {
        char c = name.charAt(i);
        if (c < 0x20 || c == 0x7F) {
            throw new IllegalArgumentException("Control character present: " + component);
        }
    }

    // Dot-only or reserved dot forms & Trailing space/dot
    if (name.equals(".") || name.equals("..") || name.matches("\\.{2,}") ||
            name.endsWith(" ") || name.endsWith(".")) {
        throw new IllegalArgumentException("Invalid dot sequence or trailing space/dot: " + component);
    }

    // Allowlist
    if (!name.matches("[A-Za-z0-9._-]+")) {
        throw new IllegalArgumentException("Disallowed characters: " + component);
    }

    // Windows reserved device names
    if (RESERVED_NAMES.contains(name.toUpperCase(Locale.ROOT))) {
        throw new IllegalArgumentException("Reserved device name: " + component);
    }
}

}`

@garydgregory can we make some code in apache commons to avoid path-injection attacks like the code above for aavoiding path injections in windows?

govind-gupta-tfs avatar Oct 08 '25 10:10 govind-gupta-tfs

@elharo pls see the above message

govind-gupta-tfs avatar Oct 08 '25 10:10 govind-gupta-tfs

Hi @govind-gupta-tfs,

Apache Commons IO already provides utilities to guard against path-traversal and invalid names:

One note: toLegalFileName(...) doesn’t currently replace Windows reserved device names (e.g., CON, NUL, PRN). If you think that would help here, we’d welcome a contribution adding that behavior.

ppkarwasz avatar Oct 08 '25 22:10 ppkarwasz