Skip to content
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

make safe_join behave like os.path.join with *args #1730

Merged
merged 2 commits into from Jun 4, 2016

Conversation

@geusebi
Copy link
Contributor

commented Feb 17, 2016

Make safe_join behave like os.path.join, which accept multiple path components with *args.
Note that only the ``directory'' 's bound is respected and the path components are only joined together.

@untitaker

This comment has been minimized.

Copy link
Member

commented Feb 17, 2016

Is there a concrete usecase for this? I would expect this to cause confusion as it's no longer clear which components are trusted.

@geusebi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2016

I often use an another version which protect each single path components from crossing its boundary, depend on the case.

I issued a pull request with this version because it was the easier one.

e.g. (with base_dir coming from config and the other variables from the user)
safe_join(base_dir, users_dir, user_name, "some_file")
whatever the user input it cannot leave base_dir.

Checking every single path would be even better for me as it's the version I use most.

@untitaker

This comment has been minimized.

Copy link
Member

commented Feb 17, 2016

I still don't understand which usecase this has.

@geusebi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2016

If you have many path to join you can do:
safe_join(base_dir, users_dir, user_name, "some_file")
or:
paths = (base_dir, users_dir, user_name, "some_file")
safe_join(*paths)
instead of:
safe_join(base_dir, safe_join(users_dir, safe_join(user_name, "some_file")))

Also you can switch from os.path.join to safe_join without problems even if you use *args.
I've always used os.path.join this way and I missed that feature when I used safe_join for the first time.

@timster

This comment has been minimized.

Copy link

commented Feb 19, 2016

Couldn't you just do

safe_join(base_dir, os.path.join(users_dir, user_name, "some_file"))

@geusebi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2016

Sure, but it would be nice to have safe_join to accept arguments exactly as its counterpart (I'm assuming it's os.path.join(a, *p)). It's easy to implement this behavior without breaking anything and so I proposed the pull request.

If you see no value or no need to merge, no problem :)

A version which checks every path components could be something like this
https://gist.github.com/geusebi/b152f1ee17a66a9bbeee (not tested)
With this version
safe_join(base_dir, safe_join(users_dir, safe_join(user_name, "some_file")))
could be written as I did in my last comment i.e.
paths = (base_dir, users_dir, user_name, "some_file")
safe_join(*paths)

@untitaker

This comment has been minimized.

Copy link
Member

commented Feb 22, 2016

You've still not given me a concrete usecase. I'm worried that the proposed API isn't secure in situations where it's assumed to be.

On 22 February 2016 08:09:17 CET, Giampaolo Eusebi notifications@github.com wrote:

Sure, but it would be nice to have safe_join to accept arguments
exactly as its counterpart (I'm assuming it's os.path.join(a, *p)).
It's easy to implement this behavior without breaking anything and so I
proposed the pull request.

If you see no value or no need to merge, no problem :)

A version which checks every path components could be something like
this
https://gist.github.com/geusebi/b152f1ee17a66a9bbeee (not tested)
With this version
safe_join(base_dir, safe_join(users_dir, safe_join(user_name, "some_file")))
could be written as I did in my last comment i.e.
paths = (base_dir, users_dir, user_name, "some_file")
safe_join(*paths)


Reply to this email directly or view it on GitHub:
#1730 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Feb 22, 2016

I think for security-related functions it makes sense to keep them simple. Extra complexity just makes it more likely that someone uses them in an insecure fashion

@untitaker

This comment has been minimized.

Copy link
Member

commented Feb 22, 2016

To be clear, I'd rather see a behavior where only the basepath is trusted, and the other args are treated as potentially malicious.

On 22 February 2016 10:27:16 CET, Adrian notifications@github.com wrote:

I think for security-related functions it makes sense to keep them
simple. Extra complexity just makes it more likely that someone uses
them in an insecure fashion


Reply to this email directly or view it on GitHub:
#1730 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@geusebi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2016

Here only the basepath is trusted https://gist.github.com/geusebi/b152f1ee17a66a9bbeee .

Usecase: sometimes I happen to manipulate paths and I have to collect some of these paths from the user and from other untrusted sources. In the end I have a tuple or a list of paths to join, safe_join allow me to do it safely but I have to call it using reduce or with a loop to make it work with multiple paths. This puzzled me the first time as I presumed it was fully compatible with path.join.

If I am the only one which thinks it can be useful and if so many (very reasonable) doubts are coming from this simple addition it should probably be refused :)

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

I'll merge if you update the PR from the gist you posted and add a changelog.

(Also you use a generator expression and a loop in the gist, it could be only one)

@joequery

This comment has been minimized.

Copy link

commented May 4, 2016

👍 from me for this PR. With the current behavior of only allowing a single file to be joined to the basepath, we're increasing the likelihood that someone using this function has manually constructed their own insecure "basepath" (which wouldn't truly be a basepath at that point) to accommodate the function definition.

@untitaker

This comment has been minimized.

Copy link
Member

commented May 4, 2016

This PR is insecure as well. It treats *pathnames as "trusted" in the sense that it allows .. in one pathnames part to eradicate the previous one.

https://gist.github.com/geusebi/b152f1ee17a66a9bbeee is the correct approach.

@untitaker

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

@geusebi Could you rebase and add tests? Thanks!

@untitaker untitaker added waiting and removed discussion labels Jun 3, 2016

@geusebi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2016

Rebased and added some tests, let me know if you need something else.

P.S.
It would be great if somebody else could take a double look at the code before the merge.

Many thanks

@untitaker untitaker merged commit 9f9e1fd into pallets:master Jun 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@untitaker

This comment has been minimized.

Copy link
Member

commented Jun 4, 2016

Excellent, thanks!

@untitaker untitaker removed the waiting label Jun 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.