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

Move to Path::Tiny #1264

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Move to Path::Tiny #1264

wants to merge 22 commits into from

Conversation

xsawyerx
Copy link
Member

@xsawyerx xsawyerx commented Oct 6, 2016

Path::Tiny provides a clean, correct, and consistent interface to various path-related modules. We have our own module, which could be improved.

I want to at least replace the core of it with Path::Tiny and maybe remove it at some point in favor.

This is a work in progress. I have more work to commit once the tests pass.

  • Clean up FileUtils internals.
  • Clean up usages of File::Spec.
  • Clean up usages of other File::* modules.
  • File::Basename -> path($path)->basename.
    (basedir is not available.)
  • Clean up usages of -d -> path($path)->is_dir.
  • Clean up usages of -f -> path($path)->is_file.
  • Clean up usages of -e -> path($path)->exists.

@xsawyerx xsawyerx changed the title Move to Path::Tin Move to Path::Tiny Oct 6, 2016
@xsawyerx
Copy link
Member Author

I have rewritten and rebased this branch. It's basically waiting on me to finish merge_path from the File Handler, but I need to first figure out what it's doing. :/

@xsawyerx
Copy link
Member Author

xsawyerx commented Mar 1, 2017

Last commit fixes that.
My next part is cleaning up the explicit calls to Path::Tiny::path() and then I'll start removing the stringify that exists everywhere.

@wchristian
Copy link
Contributor

wchristian commented Mar 1, 2017

That merge_paths commit is no good in that form, i'm afraid.

All of these should throw errors instead of "succeeding", otherwise they present huge security problems and/or break tests on systems with more than one file system tree. @haarg probably has other ideas of how this could break as well.

C:\Users\Mithaldu>perl -e "use Path::Tiny; print Path::Tiny::path( 'c:/meep', 'c:/marp' )->stringify"
C:/meep/c:/marp
C:\Users\Mithaldu>perl -e "use Path::Tiny; print Path::Tiny::path( 'c:/meep', 'd:/marp' )->stringify"
C:/meep/d:/marp
C:\Users\Mithaldu>perl -e "use Path::Tiny; print Path::Tiny::path( 'c:/meep', '../marp' )->stringify"
C:/marp

@xsawyerx
Copy link
Member Author

xsawyerx commented Mar 2, 2017

@wchristian, Thank you for testing them out. I'm not sure what these should have been doing to begin with. What does it mean to ask for "c:/meep", "d:/marp"? These are different volumes.

@wchristian
Copy link
Contributor

The first two are both straight-up non-sense in the vein of merge_path( "~/.ssh", "ftp://some.server/whatever" ) so they should error out, nothing else.

The third is "merely" dangerous and should be prevented.

@ambs
Copy link
Member

ambs commented Oct 8, 2017

@xsawyerx, ,what is missing here? Probably update the todo list above?

@xsawyerx
Copy link
Member Author

xsawyerx commented Sep 8, 2019

I just rebased this branch (had a few conflicts) and I wrote a solution to the issues raised by @Mithaldu.

The short answer is: 2 out of 3 cases get it wrong with both merge_paths (our code that I'm removing) and with Path::Tiny, but it's okay because our code will catch it.

The problem with the third case is a security issue, but not because of what Path::Tiny does. It does the right thing - but we assume the user is correct in doing this, which is a possible security issue.

I've resolved the third issue but I want to write some tests before I commit and push these too.

@xsawyerx
Copy link
Member Author

xsawyerx commented Sep 8, 2019

I added the fix and the tests. Seems be be good. I originally also patched Dancer2::Handler::File but coudn't find any test for it. Then I couldn't find anything using it. I made a syntax error in i and ran the entire testing suite - successful. Are we even using this module anymore?

@cromedome
Copy link
Contributor

You replaced the one place we were using Dancer2::Handler::File, so I think at this point we can effectively retire it.

This is good stuff. 👍 from me.

xsawyerx added 16 commits April 11, 2020 18:02
1. It's core.
2. We're already loading it elsewhere.
This both clears a call to read_glob_content and is faster and
more memory efficient.
[First step in moving the core to Path::Tiny]

This change helps us test the interaction between the core,
FileUtils, and Path::Tiny. This assures we do not deviate.
[This is just a step and is not complete]

Dancer2::FileUtils has moved to mostly use Path::Tiny, but this
commit starts moving th core code itself to it, in the hopes of
rendering most of FileUtils unnecessary.

There are some cases that are more complicated, and they are
addressed separately one on one.
This logic was trickier. The problem is that we both shift() and
also not account for PATH_INFO being empty, which trips Path::Tiny.

So, we're cautiously checking whether it is defined has length.
Dancer2::Template::Simple will render files *and* strings. This
is why we cannot just use Path::Tiny.
We're using "realpath" because it converts to absolute.
There are two important changes here.

The first defends against empty paths from the environment. We can
see that there was a situation in which we would have returned
an empty string (in case none of the three options worked) and
that's bad. We should fix it in the future.

The second is what might be a security issue. We use FileUtils'
path() which returns path based on root (/) if it's empty. There
is a situation in which the environments_location is empty (see
paragraph above) and in that case, we will accidentally use the
root directory. This does nothing if the file does not exist, but
this is now officially fixed with Path::Tiny and checking whether
it's empty or not.
The majority of this is rather simple, but annoying to type. We're
not making full use of Path::Tiny here, which we could, once we
clear it up.
This was a bit hard to figure out, but with the help of @wchristian,
it seems as though this merges path from argument 1 onto argument 2.

I have this replaced it with a single path() call. All tests pass,
so I hope this is okay. We should run these tests under Windows to
make sure it works there as well.
We're already using File::Basename. Why are we trying to lazily
load it now?
Instead of moving from Path::Tiny back to string as early as possible,
we're keeping it for a bit longer.

The defined() and -f checks can be done with Path::Tiny too. Then we
can just call openr_raw() instead of creating another Path::Tiny object.

Once we're done with that, we can finally stringify and go back to string
form.

Eventually, we would want to make all of our internals assume on Path::Tiny.
If someone were to send a file to send_file() that includes '../',
then we would allow them to reach outside the directory we choose.
This is a possible security issue. (One can argue the user should
sanitize their input, but I think we simply shouldn't allow it.)

The problem is that Path::Tiny does the right thing and allows us to
reach there. To prevent that, we're resolving paths using
Path::Tiny's realpath() method and then subsumes() to see that the
file is within the original directory.

Otherwise, we send a 403 forbidden. There is also a test that verifies
this is done correctly.
@SysPete SysPete force-pushed the cleanup/path-tiny branch from 77ef8a0 to c0fe438 Compare April 11, 2020 16:29
@SysPete
Copy link
Member

SysPete commented Apr 11, 2020

@xsawyerx I just rebased this against current master as I'd really like to see it merged. I'm going to go back over all of the comments to check that all issues are addressed, and then will add a further comment.

@SysPete
Copy link
Member

SysPete commented Apr 11, 2020

Some local test failures, but Travis is busy with maintenance atm. Lots of interesting appveyor failures too. I'll start looking tomorrow.

Comment on lines +38 to +39
$location->is_dir
or Carp::croak("Caller $script is not an existing file");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xsawyerx this is causing the only current (non-Windows) test failure, and was the only failure before I rebased. I know it is a long time ago, but can you remember why this was added?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm..... no memory at all.

I want to get back to this, but I can't promise when. Maybe Saturday?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xsawyerx Saturday would be cool, but no sweat. With luck I'll have time to look at the Windows test failures this weekend.

@cromedome cromedome added this to the April 2020 Release milestone Apr 13, 2020
@SysPete
Copy link
Member

SysPete commented May 30, 2020

Removing from milestone since this still needs a huge amount of work.

@SysPete SysPete removed this from the April 2020 Release milestone May 30, 2020
cromedome added a commit that referenced this pull request Mar 23, 2021
"The journey of a thousand miles begins with a single step." We have
discussed migrating to Path::Tiny (and started, see #1264 and #1544 for
details), and this is the first concrete, discrete step in doing so. It
takes a problem of small scope (app scaffolding and File::Find) to set
the stage for the rest of the migration.

There are a few TODO items here, and if there are better ways to handle
them, I am all ears. If not, I will remove my comments when I merge.

Feedback welcome/needed!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants