Conversation
3fb341a to
ce201ca
Compare
ce201ca to
9b29aca
Compare
|
I have rewritten and rebased this branch. It's basically waiting on me to finish |
|
Last commit fixes that. |
|
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. |
|
@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 |
|
The first two are both straight-up non-sense in the vein of The third is "merely" dangerous and should be prevented. |
|
@xsawyerx, ,what is missing here? Probably update the todo list above? |
27be625 to
3f293c6
Compare
|
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 The problem with the third case is a security issue, but not because of what I've resolved the third issue but I want to write some tests before I commit and push these too. |
|
I added the fix and the tests. Seems be be good. I originally also patched |
|
You replaced the one place we were using This is good stuff. 👍 from me. |
77ef8a0 to
c0fe438
Compare
|
@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. |
|
Some local test failures, but Travis is busy with maintenance atm. Lots of interesting appveyor failures too. I'll start looking tomorrow. |
| $location->is_dir | ||
| or Carp::croak("Caller $script is not an existing file"); |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Hmm..... no memory at all.
I want to get back to this, but I can't promise when. Maybe Saturday?
There was a problem hiding this comment.
@xsawyerx Saturday would be cool, but no sweat. With luck I'll have time to look at the Windows test failures this weekend.
|
Removing from milestone since this still needs a huge amount of work. |
"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!
c0fe438 to
4dd2da1
Compare
cromedome
left a comment
There was a problem hiding this comment.
This is a lot of churn, but I find the result to be a codebase that's easier to read and maintain. I realize that may be personal preference, but it's a huge win IMO.
A couple of things:
- I realize that you've eradicated
File::Specfrom most tests. It is still being used int/strict_config.t- might as well be consistent here - I have not retested on Windows, and I am not in a position to, so the test failures identified by @SysPete might still apply.
This is great!
We do need to have this tested on Windows. @wchristian is also an expert on the matter and uncovered much of the mistakes in the previous attempt. (And those mistakes might not have been fixed...) |
|
Great piece of work @xsawyerx , many thanks! Looking forward to seeing this merged. 👍 from me |
|
I'll have a look next week. :) |
|
Several Windows test failures to resolve: I can try and tackle them, but I won't have an opportunity until at least Tuesday. |
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.
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.
Integrate Path::Tiny even further into the core, using path semantics like child/exists/is_file, and fewer conversions between string paths and Path::Tiny objects. The public path/config APIs are still strings. * Now using Path::Tiny attrs in config locations, app paths, views, public dir, sessions, logger, and template resolution. * Public settings still return strings. * File/static handling, session file paths, and logger file paths now use Path::Tiny for joins/existence checks, with stringification at boundaries. * Error handling uses Path::Tiny for static error pages and backtrace file reads. * CLI generator now emits string paths derived from Path::Tiny. * Tests were updated to replace File::Spec with Path::Tiny, and the runtime dependency on File::Spec was removed from cpanfile.
71cff85 to
adc3493
Compare
|
Rebased and applied a fix in |
|
All tests pass :) Thanks @xsawyerx! |
|
i ran the test suite based on your branch and found 3 things that don't look like path issues and raised those as their own tickets otherwise, the tests relating to path stuff seem to pass, however, the following smells like the time between file handles being closed and the test program ended is too little, resulting in a race condition when file::temp tries to clean up |
|
Rebased and merged. Thanks everyone! |
|
??? the request_upload.t errors still occur |
|
@wchristian Created #1777 to track this so it doesn't get lost. |
It was easier to merge the branch and deal with the problems later than to keep rebasing the branch as we prepare to release this feature. Sorry for not saying as much when I merged this, and thanks @xsawyerx for doing the work I should have done to start with ;-) |
[ BUG FIXES ]
* GH #686: Fix: to_json is double encoding UTF8 (Sawyer X)
* GH #863: Fix case insensitive system confusion (Sawyer X)
* GH #1124: Fix: charset config option is mostly ignored (Sawyer X)
* GH #1143: Fix utf8 in URL (Sawyer X, Sorin Pop)
* GH #1449, 1630: Make plugin DSL keyword app-specific (Sawyer X)
* GH #1772: t/dsl/send_file.t fails with content_type-related errors
(Sawyer X)
* GH #1773: t/dsl/send_as.t throws json-related warnings (Jason
A. Crome)
* GH #1774: t/hooks.t throws void warnings (Jason A. Crome)
* GH #1777: Properly unlink file uploads on Windows (Sawyer X)
[ ENHANCEMENTS ]
* GH #763: Strict config mode; warn on unknown config keys with
opt-out (Sawyer X)
* GH #763: Default strict config to off, but scaffold new apps with
stict config enabled (Jason A. Crome)
* GH #1073: Get multiple session cookie values at once with clear
method (Sawyer X)
* GH #1264: Move to Path::Tiny (Sawyer X)
* GH #1323: Allow fully qualified namespaces for all engines (Russell
@veryrusty Jenkins)
* GH #1594: Use Unicode::UTF8 if available (Sawyer X)
* GH #1664: Stop sending double server headers (Sawyer X)
* GH #1709: `send_as` should use the full serializer, including hooks
(Sawyer X)
* PR #1757: Remove api_version, improve dispatching loop (Sawyer X)
* PR #1758: Move MIME ownership to app (Sawyer X)
* PR #1767: Turn off strict_config for noisy tests (Jason A. Crome)
* PR #1780: Add package name capability to logger output (Mikko
Koivunalho)
[ DOCUMENTATION ]
* GH #1431: Better document behavior of views setting (Jason A. Crome)
* PR #1749, #1750: Fix broken manual and tutorial links (Gil Magno,
Jason A. Crome)
* PR #1753: Fix structure of config docs (Mikko Koivunalho)
* PR #1762: Remove keyword logger from DSL document (Mikko Koivunalho)
[ DEPRECATED ]
* None
[ MISC ]
* PR #1776: Remove "Powered by..." from the error page (Jason A. Crome)
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::Tinyand maybe remove it at some point in favor.This is a work in progress. I have more work to commit once the tests pass.
FileUtilsinternals.File::Spec.File::*modules.File::Basename->path($path)->basename.(
basediris not available.)-d->path($path)->is_dir.-f->path($path)->is_file.-e->path($path)->exists.