Overly strict FromSegments implementation for PathBuf

[Noughmad]

In the interest of safety, the implementation of FromSegments for PathBuf (code) rejects all segments that start with a dot (.). I believe this is overly strict, since

1. The dot-dot case (a filename of exactly ..) is already handled.
2. A single dot segment (exactly .) could also be valid, and just skipped.
3. Lots of hidden files start with ., such as .gitignore and other config files.

In my use case I only require the third case, as I am trying to create a file browser, where the path is specified as <path..> and path: PathBuf. However, such a route cannot browse a file with a name starting with a dot, and I get a Failed to parse 'path': BadStart('.') error message.

I can reproduce this behavior on version 0.3.6 on ArchLinux, but looking at the code I can see it is present in master as well.

[zxvfxwing]

Hi,

I have also encountered this behavior when I wanted to accept dotfiles (as you say) in my routes.
@SergioBenitez answered me on matrix channel / IRC bridge after exposing my problem.
You can read the entire conversation here.

So I made a workaround (used in one of my personal project) which is certainly not perfect but works.

Use case:

#[get("/<unsafe_p..>")]
pub fn dotfiles(unsafe_p: UnsafePBuf) -> ... {
    let dotpath = unsafe_p.path();
    /* Do wathever you want with your path */
}

Hopefully, my answer met your expectations.

[Noughmad]

Thank you @zxvfxwing , I was about to make a similar workaround myself, and you just saved me the effort.

That said, it would still be great if such a change could be made in Rocket itself. It looks like there is interest for it, and I can submit a PR. The question is if @SergioBenitez would accept it, and which cases may be allowed. I'm not really a web dev, so I'm not familiar with all the forms of path traversal attacks.

[SergioBenitez]

The danger is that users will unknowingly leak hidden files if this change is made. I'd like to keep the safe-by-default behavior while also allowing users to disable this on a per-case basis.

In #239 (comment) I sketch out an API for mounting a "StaticFiles" handler. To resolve this issue, that handler could accept options on creation to allow this behavior:

rocket::ignite()
    .mount("/path_to_serve_on", StaticFiles::from("/path_to_serve_from").allow_hidden());

I've already laid out all of the ground work to enable the implementation of something like StaticFiles, and I've implemented a very basic version of StaticFiles. Once my time frees up a bit, I'll polish the implementation and merge it in.

If this sounds like a good plan moving forward @Noughmad, let's close this and track progress in #239. (Edit: Whoops. Didn't mean to close it myself!)

P.S: Complementary to this, we could have the StaticFiles handler deny hidden files by default but have PathBuf allow them. We'd then advocate for only using StaticFiles to serve static files with the safe-by-default behavior. PathBuf would be relegated to special cases, protecting against path traversal attacks but nothing else.

[Noughmad]

Serving static files is not good for me, as I want to show other things in addition to the file content.

However, as I said I the workaround meantioned above works for me, and is not overly complicated. So unless you wish to change the current implementation, feel free to close the issue.

[SergioBenitez]

@Noughmad The plan in my previous comment covers your use-case as well by relaxing the constraints on PathBuf. I'll close this out and track in #239.

[Scripter17]

P.S: Complementary to this, we could have the StaticFiles handler deny hidden files by default but have PathBuf allow them. We'd then advocate for only using StaticFiles to serve static files with the safe-by-default behavior. PathBuf would be relegated to special cases, protecting against path traversal attacks but nothing else.

Has any progress been made on this in the last 2 and a half years? For reasons that aren't relevant using FileServer simply will not work for me.

I would quite like to be able to decide myself whether or not I want to handle a provided path. Yeah security by default is nice but I'd like the option to turn it off.

Also, @zxvfxwing, your workaround seems to not work in rocket 0.5.0-rc

[SergioBenitez]

P.S: Complementary to this, we could have the StaticFiles handler deny hidden files by default but have PathBuf allow them. We'd then advocate for only using StaticFiles to serve static files with the safe-by-default behavior. PathBuf would be relegated to special cases, protecting against path traversal attacks but nothing else.

Has any progress been made on this in the last 2 and a half years? For reasons that aren't relevant using FileServer simply will not work for me.

I would quite like to be able to decide myself whether or not I want to handle a provided path. Yeah security by default is nice but I'd like the option to turn it off.

Also, @zxvfxwing, your workaround seems to not work in rocket 0.5.0-rc

Yes. Rocket doesn't prohibit you from doing anything in this space, and never did. You can take a raw Segments and convert it to a path yourself.

[Scripter17]

I probably should've clarified my issue is that routes refuse to accept any path with a segment starting in .

As far as I can tell doing this

#[macro_use] extern crate rocket;
use std::path::PathBuf;

#[get("/<path..>")]
fn route(path: PathBuf) -> _ {
    //
}

#[launch]
fn rocket() -> _ {
    rocket::build()
        .mount("/route", routes![route])
}

will never let you access /route/.fileINeedToAccess, at least in 0.5.0-rc.2. And yes. I need to handle directories too. I know what I need and this is it. Alternative design suggestions will be both ignored and not appreciated as they are irrelevant to the problem at hand

If there's a way to use raw Segmentss as you said then I'll do it but as far as I'm concerned I shouldn't have to

[SergioBenitez]

I probably should've clarified my issue is that routes refuse to accept any path with a segment starting in .

As far as I can tell doing this

#[macro_use] extern crate rocket;

use std::path::PathBuf;



#[get("/<path..>")]

fn route(path: PathBuf) -> _ {

    //

}



#[launch]

fn rocket() -> _ {

    rocket::build()

        .mount("/route", routes![route])

}

will never let you access /route/.fileINeedToAccess, at least in 0.5.0-rc.2. And yes. I need to handle directories too. I know what I need and this is it. Alternative design suggestions will be both ignored and not appreciated as they are irrelevant to the problem at hand

If there's a way to use raw Segmentss as you said then I'll do it but as far as I'm concerned I shouldn't have to

As I said in my response, you simply need take a Segments and convert it to a path yourself. Here's the code allowing dotfiles, in case the docs are out of reach:

#[get("/<path..>")]
fn route(path: Segments<'_, Path>) -> _ {
    let path = path.to_path_buf(true)?;

}

Furthermore, you can create your own type implementing FromSegments that implements whatever security policy you want, entirely removing the need to use Segments in a route directly and centralizing said policy.

[Scripter17]

Oh I didn't realize that was even possible

Sorry 'bout that

Edit for anyone who stumbles upon this:

#[macro_use] extern crate rocket;

#[get("/<path..>")]
fn route(path: rocket::http::uri::Segments<'_, rocket::http::uri::fmt::Path>) -> _ {
    let path = path.to_path_buf(true).unwrap();
    // Stuff
}

rocket::http::uri::Segments, rocket::http::uri::fmt::Path

Also the validation code currently exists at https://github.com/SergioBenitez/Rocket/blob/master/core/http/src/uri/segments.rs#L201