Skip to content

Conversation

@Philzen
Copy link

@Philzen Philzen commented Jan 4, 2024

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.

@elharo
Copy link
Contributor

elharo commented Jan 5, 2024

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.

@Philzen
Copy link
Author

Philzen commented Apr 24, 2024

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?

@govind-gupta-tfs
Copy link

`

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
Copy link

@elharo pls see the above message

@ppkarwasz
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants