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

Path::Class::Dir->contains doesn't handle ".." #43

Closed
nrdvana opened this issue Dec 6, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@nrdvana
Copy link
Contributor

commented Dec 6, 2015

The documentation for contains says that it checks the filesystem to see if one directory is contained within another, however

perl -e 'use Path::Class "dir"; print dir("lib")->contains(dir("lib","..."))."\n"'

shows that it didn't actually resolve the directories or even logically deal with the "..".

@kenahoo

This comment has been minimized.

Copy link
Owner

commented Dec 7, 2015

I'm assuming you meant two dots instead of three dots in the example?

@nrdvana

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2015

yes :-)

Also to reproduce the test there needs to be a subdirectory named "lib".

kenahoo added a commit that referenced this issue Dec 8, 2015

@kenahoo

This comment has been minimized.

Copy link
Owner

commented Dec 8, 2015

I added a fix & tests for this, want to take a look?

@vanstyn

This comment has been minimized.

Copy link

commented Aug 13, 2016

@kenahoo, @nrdvana --

The changes made in 935eeed introduces a regression (confirmed with git bisect)

Starting with that commit, the following cmdline prints FAIL!!:

mkdir -p deleteme/test/path
touch deleteme/test/path/somefile
perl -Ilib -e '
  use Path::Class "dir", "file";
  dir("deleteme/test/../test/path/")
    ->contains(file("deleteme/test/../test/path/somefile"))
    or print "FAIL!!\n"
'

This change is breaking some of the auto-generated dev scripts for RapidApp users, which are already out in the wild, so the sooner this can be fixed (broken in v0.36 on CPAN) the better.

Thanks

nrdvana added a commit to nrdvana/Path-Class that referenced this issue Aug 13, 2016

Fix "contains" when $self is a relative path - kenahoo#43
Need to resolve both $self and $other before comparing them with
subsumes.

nrdvana added a commit to nrdvana/Path-Class that referenced this issue Aug 13, 2016

Fix "contains" when $self is a relative path - kenahoo#43
Need to resolve both $self and $other before comparing them with
subsumes.

nrdvana added a commit to nrdvana/Path-Class that referenced this issue Aug 13, 2016

Fix "contains" when $self is a relative path - kenahoo#43
Need to resolve both $self and $other before comparing them with
subsumes.
@nrdvana

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2016

Sorry for not following up sooner. My issue was resolved, but I found the problem affecting vanstyn, and went ahead and fixed them in pull request #48.

@kenahoo

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2016

Now that #48 is merged, is this issue resolved to everyone's satisfaction?

@vanstyn

This comment has been minimized.

Copy link

commented Aug 14, 2016

Yes! Can we ship to CPAN quickly??

@kenahoo

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2016

Sure. Will do now.

@vanstyn

This comment has been minimized.

Copy link

commented Aug 14, 2016

Thank you!!!

@kenahoo

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2016

No problem, thanks for reaching out to get it fixed.

@nrdvana

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2016

No prob, thanks for authoring a great module like this!

BTW, my original use case was to check web URLs vs. a path of statically-served files

if (dir($static_path)->contains(file($static_path, @path)) {
...
}

and I wanted to make sure that @path had not escaped this directory via ".." or a symlink.

@nrdvana nrdvana closed this Aug 14, 2016

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