Repository: actix/actix-web
File: actix-files/src/path_buf.rs (Rust)

Code before:
use crate::error::UriSegmentError;

#[derive(Debug, PartialEq, Eq)]
pub(crate) struct PathBufWrap(PathBuf);

impl FromStr for PathBufWrap {
    type Err = UriSegmentError;

    fn from_str(path: &str) -> Result<Self, Self::Err> {
        Self::parse_path(path, false)
    }
}

impl PathBufWrap {
    /// Parse a path, giving the choice of allowing hidden files to be considered valid segments.
    ///
    /// Path traversal is guarded by this method.
    pub fn parse_path(path: &str, hidden_files: bool) -> Result<Self, UriSegmentError> {
        let mut buf = PathBuf::new();

        let path = percent_encoding::percent_decode_str(path)
            .decode_utf8()
            .map_err(|_| UriSegmentError::NotValidUtf8)?;

        for segment in path.split('/') {
            if segment == ".." {
                buf.pop();
            } else if !hidden_files && segment.starts_with('.') {
                return Err(UriSegmentError::BadStart('.'));
            } else if segment.starts_with('*') {
                return Err(UriSegmentError::BadStart('*'));
            } else if segment.ends_with(':') {
                return Err(UriSegmentError::BadEnd(':'));
            } else if segment.ends_with('>') {
                return Err(UriSegmentError::BadEnd('>'));
            } else if segment.ends_with('<') {
                return Err(UriSegmentError::BadEnd('<'));
            } else if segment.is_empty() {
                continue;
            } else if cfg!(windows) && segment.contains('\\') {
                return Err(UriSegmentError::BadChar('\\'));
            } else {
                buf.push(segment)
            }
        }

        Ok(PathBufWrap(buf))
    }
}

impl AsRef<Path> for PathBufWrap {

Review comment (security):
Paths in URLs are lists of percent-encoded components, not percent-encoded lists of components. Decoding an entire path string can introduce new components by decoding forward/back-slashes.

For example: `https://example.com/music/bands/AC%2FDC`

Decode then split results in the components: `["music", "bands", "AC", "DC"]`
Split then decode results in the correct components: `["music", "bands", "AC/DC"]`

If you have a component like `foo%2F..%2F..%2F..`, decode-then-split would introduce new ".." components, which can be a security risk as it defeats any previous validation of the path.

(This is just a drive-by comment because I saw this issue linked from somewhere, and recognised this as a common misunderstanding)

Code after:
use crate::error::UriSegmentError;

#[derive(Debug, PartialEq, Eq)]
pub(crate) struct PathBufWrap(PathBuf);

impl FromStr for PathBufWrap {
    type Err = UriSegmentError;

    fn from_str(path: &str) -> Result<Self, Self::Err> {
        Self::parse_path(path, false)
    }
}

impl PathBufWrap {
    /// Parse a path, giving the choice of allowing hidden files to be considered valid segments.
    ///
    /// Path traversal is guarded by this method.
    pub fn parse_path(path: &str, hidden_files: bool) -> Result<Self, UriSegmentError> {
        let mut buf = PathBuf::new();

        // equivalent to `path.split('/').count()`
        let mut segment_count = path.matches('/').count() + 1;

        // we can decode the whole path here (instead of per-segment decoding)
        // because we will reject `%2F` in paths using `segement_count`.
        let path = percent_encoding::percent_decode_str(path)
            .decode_utf8()
            .map_err(|_| UriSegmentError::NotValidUtf8)?;

        // disallow decoding `%2F` into `/`
        if segment_count != path.matches('/').count() + 1 {
            return Err(UriSegmentError::BadChar('/'));
        }

        for segment in path.split('/') {
            if segment == ".." {
                segment_count -= 1;
                buf.pop();
            } else if !hidden_files && segment.starts_with('.') {
                return Err(UriSegmentError::BadStart('.'));
            } else if segment.starts_with('*') {
                return Err(UriSegmentError::BadStart('*'));
            } else if segment.ends_with(':') {
                return Err(UriSegmentError::BadEnd(':'));
            } else if segment.ends_with('>') {
                return Err(UriSegmentError::BadEnd('>'));
            } else if segment.ends_with('<') {
                return Err(UriSegmentError::BadEnd('<'));
            } else if segment.is_empty() {
                segment_count -= 1;
                continue;