Closed schwern closed 13 years ago
The realpath check was added in 34b4e73ddfd71b932b6bc66c0a5178e283c07b36 only to "get rid of '..' fragments" and has no test. Enforces the conclusion this is a bug.
This is why Dancer::FileUtils::path_no_verify() was added...
I don't think the correct choice is to leave path
as is and write a new function.
path_no_verify
isn't documented.path_no_verify
still requires the path exists and it fails in an odd manner
print path_no_verify("path", "does", "not", "exist");
Those can be fixed, but more importantly...
path
is not documented to only work on files which existpath
was changed (what looks like) accidentallypath
was changed incompatiblypath
is the convenient name; it's what people will reach for thinking it just concatenates file pathsIt's easy to verify that a file exists. The harder problem is concatenating and normalizing paths. It's far more useful to have a function which does the later than a function which does both. If you have a function which just concatenates paths, you can turn it into a function which also verifies. You can't do the reverse.
path
should do no verification and verify_path
should.
I agree about the confusion of the path() issue. It had become an issue at some point not long before I started looking at that code. After I added the path_no_verify() function to provide a path() which only verifies the directories (and sukria explicitly didn't want it added to the DSL, i.e. by not copying it into Dancer as well as Dancer::FileUtils), I posted the following comment somewhere (I can't remember where). I'll just copy-paste it below from my own copy (I don't know if some of the functions I referred to have changed since then though...):
...one last nitpick-question this raises from me (which I wouldn't want to be the one to decide about) is that at present: Dancer::FileUtils::path() just constructs a path, doesn't fully resolve or check it against FS Dancer::path() does same as above and also resolves/checks it against FS Dancer::FileUtils::path_no_verify() constructs path, resolves/checks the directories against FS, and leaves final component as is Dancer::path_no_verify() doesn't exist (by request from sukria) ...the problem is that Dancer::FileUtils::path_no_verify() actually verifies more than Dancer::FileUtils::path() does. This means the names are becoming a bit counter-intuitive. The check against FS can't be moved into Dancer::path_no_verify() because that would involve calling Cwd::realpath() directly from Dancer.pm (sukria said he wants to avoid such practise). I would suggest names like this: Dancer::FileUtils::path() keeps the same name Dancer::FileUtils::path_no_verify() becomes Dancer::FileUtils::path_verify_dirs() Dancer::path() becomes Dancer::path_verify_all(), or stays the same if backwards compatibility must be maintained ...but this might involve lots of renaming within existing code, so maybe it is preferred to let sleeping dogs lie and go with the less intuitive naming...?
I would add that when I last looked at it, Dancer::FileUtils::path() was the function for doing the totally non-verifying that you want, as opposed to Dancer::path() (but of course that means it is not in the DSL). Also, as for "common usage", as a user I would personally expect the standard path() would probably emulate the Unix realpath, which requires an existing path.
Afterthought: if Dancer::FileUtils::path_no_verify() could become Dancer::FileUtils::path_verify_dirs(), maybe Dancer::path_no_verify() could be added as a direct goto-fu to Dancer::FileUtils::path() - which would put the functionality you want (completely non-verifying path) into the DSL...?
I would like to see this ticket going somewhere. Or decinding things are OK as they are, or defining the way they should be, so one can help and fix it. To be just here, as it is, is not relevant :)
So, dear architect @sukria, can you comment on this issue?
@schwern our troubles started because of the behavior of realpath() in GNU/Linux/BSD and Windows being incompatible. On Windows it will crash, on GNU/Linux/BSD it will just return undef. @rowanthrope worked really hard on making it still work, but it involved a lot of messy hacks (and a lot of heavy debugging which Rowan did).
Rowan and I spoke about this, and I've expressed my desire to fix this into a uniform, understandable behavior. What you're suggesting sounds just like it (and could probably close 2-3 more related tickets which still haunt us).
So, @schwern, will you be able to fix it? Please? :)
There's been a lot of twists and turns on this issue. So let's be clear what's been asked for.
I propose to make path() just concatenate and normalize. I would also remove path_no_verify() and friends. This makes path() simple and reliable and well defined and generally useful.
In https://github.com/sukria/Dancer/issues/384#issuecomment-883865 I proposed that 1) path
just concatenate and normalize with no filesystem check. 2) verify_path
would be path
plus the filesystem check.
1 is what is needed and simplifies the path handling interface.
As to 2, the whole verification thing isn't very useful, greatly complicates the interface, came in accidentally and is trivial to do with -e
.
It's possible there is a use case for path check with verification, but it would be something which ties in with Dancer and not just a simple file existence check. Perhaps something which issues a 404, I don't know. But that's another ticket.
@schwern Back when I was making fixups for the realpath problems I felt that something similar to what you are proposing is what would be a more sensible "solution". There were issues though, but time has passed and I haven't looked at Dancer's code for quite a while so I can't remember exactly why. I'll take a few stabs in the dark:
Those points pretty much boxed out any possible proposal I had in mind (well, at least anything I had the free time for), so I just did the hackish fixes to at least minimise the confusion. If @sukria has since warmed up to ideas like changing path()'s core behaviour then maybe those goalposts have shifted :-)
Anyway, I just throw those semi-reliable recollections into the mix, to help you avoid re-treading similar paths [bad pun, I know] while implementing what you have in mind. I know path-verification was essential for a few things (at least at the time) because for example whether Dancer ran in scaffolded mode or not relied on testing the existence of directories, etc.
PS: This is why I am becoming a bit of a Common Lisp fan these days, their FS libs are amazing and ridiculously straightforward, for every platform they run on, by contrast to perl's arguably hackish and sometimes less-than-intuitive FS handling. Aside from that I still love perl though. Maybe if/when Perl6 becomes stable I will kiss-and-make-up with perl. Maybe :-D
@schwern, in one word: agreed.
The new code by @xsawyerx seems to fix this. Please reopen if it doesn't.
path("does", "not", "exist")
will return undef. This is not documented behavior.It is because
realpath
will return undef if it's given a file which does not exist andpath
usesrealpath
.I find it makes path() far less useful to tie it to a physical file check. I noticed this writing a handler that sent a file and being unable to use path() to concatenate something which didn't exist so I could put the name in the logs and 404 message.
I say it's a bug and I'm happy to fix it.