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

[Manuel Federanko <m.federanko@proxmox.com>]

Hi!

thanks for your input, I will incorporate the changes in v2.

On 2026-03-25 6:04 PM, Christian Ebner wrote:
> 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))?,
>>           );
>>       }
>>   
>