Skip to content

Path::force_relative #437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lolbinarycat opened this issue Sep 7, 2024 · 8 comments
Closed

Path::force_relative #437

lolbinarycat opened this issue Sep 7, 2024 · 8 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@lolbinarycat
Copy link

Proposal

Problem statement

It is frequently desirable to prevent creation of files outside a given directory.

Motivating examples or use cases

archive extractor programs (eg. tar, unzip) usually do not allow extracting to an absolute path, instead removing the leading /.

Solution sketch

impl Path {
    /// remove any leading `/`, then remove any leading `..` sections
    fn force_relative(&self) -> PathBuf;
}

Alternatives

  • proper sandboxing virtual filesystems, similar to go fs.FS. both this and the current proposal would need additonal help to prevent symlink escapes.
  • convert to string and do manual manipulations (unlikely to function properly on Windows)
  • have the function return &Path. saves an allocation and works for the basic case where it only needs to trim charachters from the start
  • have the function return Cow, saving an allocation in the trimming and passthrough cases, but slightly increasing complexity
  • delegate this to an external crate (unergonomic, requires an extension trait or function call syntax, rust already has too many trivial single-function crates, may become unmaintained, has security implications...)

Links and related work

golang fs.FS
tar(1), see the --absolute-names option.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@lolbinarycat lolbinarycat added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Sep 7, 2024
@kennytm
Copy link
Member

kennytm commented Sep 7, 2024

have the function return &Path. saves an allocation and works for the basic case where it only needs to trim charachters from the start

in what cases this function will not be just trimming characters from the start?

remove any leading /, then remove any leading .. sections

  1. what about Windows path with leading C:\ or \\?\UNC\server\share\
  2. what about .. in the middle of the path e.g. /tmp/a/b/c/d/../e/f/g or /tmp/a/../../../../b/c/../../../d

@lolbinarycat
Copy link
Author

in what cases this function will not be just trimming characters from the start?

it may be desirable to normalize away .. components to make handling easier. also i'm not at all familiar with the edge cases of windows paths

what about Windows path with leading C:\ or \?\UNC\server\share\

yes, that will also be turned into a relative path.

@kennytm
Copy link
Member

kennytm commented Sep 7, 2024

i mean could you fill in the expected results for

// this is according to doc
assert_eq!(Path::new(r"../../../../etc/passwd").force_relative(), Path::new("etc/passwd"));

#[cfg(windows)] {
    assert_eq!(Path::new(r"C:\PROGRA~1\Rust\rustc.exe").force_relative(), Path::new("???"));
    assert_eq!(Path::new(r"\\server\share\a.txt").force_relative(), Path::new("???"));
}
#[cfg(unix)] {
    // this is according to doc
    assert_eq!(Path::new(r"/tmp/swap.txt").force_relative(), Path::new("tmp/swap.txt"));

    assert_eq!(Path::new(r"a/b/../c/d").force_relative(), Path::new("???"));
    assert_eq!(Path::new(r"/a/b/../b/c/d").force_relative(), Path::new("???"));
    assert_eq!(Path::new(r"a/../b/c/d").force_relative(), Path::new("???"));
    assert_eq!(Path::new(r"/a/../b/c/d").force_relative(), Path::new("???"));
    assert_eq!(Path::new(r"a/../../c/d").force_relative(), Path::new("???"));
    assert_eq!(Path::new(r"/a/../../c/d").force_relative(), Path::new("???"));
}

@kennytm
Copy link
Member

kennytm commented Sep 7, 2024

it may be desirable to normalize away .. components to make handling easier.

this should be handled separately by .normalize_lexically() #396.

@scottmcm
Copy link
Member

scottmcm commented Sep 9, 2024

This one method feels drastically underpowered for the problem statement that proposes it.

I'll also add the "c:windows" example that has_root mentions -- is that already "relative" from the perspective of the proposed force_relative? (From my reading of is_absolute it's considered relative.)

Perhaps some form of join instead that behaves like the lhs is a chroot? In lexical and filesystem-aware forms?

@the8472
Copy link
Member

the8472 commented Sep 9, 2024

It is frequently desirable to prevent creation of files outside a given directory.

openat2 calls this RESOLVE_BENEATH

And due to symlinks it's outright impossible to do this properly without talking to the filesystem.

Prior art: pathrs

@lolbinarycat
Copy link
Author

And due to symlinks it's outright impossible to do this properly without talking to the filesystem.

what if the directory you want to isolate to is known to not contain symlinks?

@Amanieu
Copy link
Member

Amanieu commented Sep 17, 2024

We discussed this in the @rust-lang/libs-api meeting today. We think that this use case is better addressed by #396's normalize_lexically which will error on any path that tries to .. past the root. Absolute paths can be tested for separately.

@Amanieu Amanieu closed this as completed Sep 17, 2024
@dtolnay dtolnay closed this as not planned Won't fix, can't repro, duplicate, stale Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants