Re: [PATCH proxmox-backup] fix #5247: relative paths in exclude patterns.

[Christian Ebner <c.ebner@proxmox.com>]

Hi, thanks for the patch! A few high level comments, no full in detail 
review yet.

On 3/23/26 4:31 PM, Manuel Federanko wrote:
> Patterns which start with ./ or for that matter contain /../ or similar
> constructs will never match, since they get compared to sanitized paths
> from directory traversal, sanitize those patterns too.

May also state that the user is now warned if such patterns are encountered.

> Tested by creating a .pxarexclude file and via the --exclude command
> line option.

Would not include explicitly that you tested this in the commit message, 
it should rather state that this has effect for both cases. Don't get me 
wrong, it can make sense to include such information as well, e.g. as 
future references for more complex command invocations, just a bit to 
much for this trivial test case IMHO.

> 

your comment from here ...

> I'm unsure if we should accept those silently, currently I added a
> warning for patterns supplied via the cli option, since that feels most
> susceptible to wrong usage.
> 
> It might also make sense to base the patterns for the cli arg on the
> cwd, but that would be a breaking change.

... to here should not be part of the commit message (which will be 
applied), but rather be additional notes which go below the `---` 
separator (where it does not get applied). See marking below.

> 
> Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5247
> ---

-> additional notes and comments here

>   pbs-client/src/pxar/create.rs     |  7 ++++++-
>   pbs-client/src/pxar/tools.rs      | 32 +++++++++++++++++++++++++++++++
>   proxmox-backup-client/src/main.rs | 16 ++++++++++++++--
>   3 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index e42bcc87..1c878cb7 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -560,7 +560,12 @@ impl Archiver {
>                   (line, MatchType::Exclude, false)
>               };
>   
> -            match MatchEntry::parse_pattern(line, PatternFlag::PATH_NAME, mode) {
> +            let line_normalized = crate::pxar::tools::normalize_lexically(OsStr::from_bytes(line));
> +            match MatchEntry::parse_pattern(
> +                line_normalized.as_path().as_os_str().as_bytes(),
> +                PatternFlag::PATH_NAME,
> +                mode,
> +            ) {
>                   Ok(pattern) => {
>                       if anchored {
>                           self.patterns.push(pattern.add_flags(MatchFlag::ANCHORED));
> diff --git a/pbs-client/src/pxar/tools.rs b/pbs-client/src/pxar/tools.rs
> index 475c08ad..71a38b29 100644
> --- a/pbs-client/src/pxar/tools.rs
> +++ b/pbs-client/src/pxar/tools.rs
> @@ -76,6 +76,38 @@ fn assert_single_path_component_do(path: &Path) -> Result<(), Error> {
>       Ok(())
>   }
>   
> +pub fn normalize_lexically<S: AsRef<OsStr> + ?Sized>(path: &S) -> PathBuf {
> +    normalize_lexically_do(Path::new(path))

comment: `path` could be wrapped here and the function in-lined:

     let path = Path::new(path);
     [...]


> +}
> +fn normalize_lexically_do(path: &Path) -> PathBuf {
> +    // Once std::path::normalize_lexically is stabilized we can switch
> +    // to that

comment: This comment warrants a FIXME: prefix.

> +    use std::path::Component;
> +    let has_trailing_slash = path
> +        .as_os_str()
> +        .as_encoded_bytes()
> +        .last()
> +        .copied()
> +        .is_some_and(|c: u8| c == std::path::MAIN_SEPARATOR_STR.bytes().last().unwrap());
> +    let mut new = PathBuf::new();
> +    let iter = path.components();
> +    for component in iter {
> +        match component {
> +            Component::RootDir => new.push(Component::RootDir),
> +            Component::Prefix(p) => new.push(Component::Prefix(p)),
> +            Component::CurDir => continue,
> +            Component::ParentDir => {
> +                new.pop();
> +            }
> +            Component::Normal(n) => new.push(n),
> +        };
> +    }
> +    if has_trailing_slash {
> +        new.push("");
> +    }
> +    new
> +}
> +
>   #[rustfmt::skip]
>   fn symbolic_mode(c: u64, special: bool, special_x: u8, special_no_x: u8) -> [u8; 3] {
>       [
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 2ada87bd..4ed23d6e 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -826,9 +826,21 @@ async fn create_backup(
>           let entry = entry
>               .as_str()
>               .ok_or_else(|| format_err!("Invalid pattern string slice"))?;
> +        let entry_normalized = pbs_client::pxar::tools::normalize_lexically(entry);
> +        if entry_normalized.as_os_str() != entry {
> +            log::info!(

nit: better escalate this to a log::warn!
After all it has proven to behave unexpectedly to some users.

> +                "Sanitized exclude pattern. Exclude patterns are relative to backup root, \
> +                not the current working directory and should not contain '.' or '..' as path \
> +                segments"
> +            );
> +        }
>           pattern_list.push(
> -            MatchEntry::parse_pattern(entry, PatternFlag::PATH_NAME, MatchType::Exclude)
> -                .map_err(|err| format_err!("invalid exclude pattern entry: {}", err))?,
> +            MatchEntry::parse_pattern(
> +                entry_normalized.as_os_str().as_encoded_bytes(),
> +                PatternFlag::PATH_NAME,
> +                MatchType::Exclude,
> +            )
> +            .map_err(|err| format_err!("invalid exclude pattern entry: {}", err))?,
>           );
>       }
>