[#3448] - Update file.php
- Closed
- 24 Apr 2014
- Medium
- Build: staging
- # 3448
- Diff
- fcoulter:staging
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Prevents security issue where $file can be an empty string or directory path. If that happens the deletion fails but the directory is left writeable with 0777 permissions. If $file is empty then this is interpreted by chmod as the current director, so root folder of Joomla site is left writeable.
Joomla Core Tracker:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33614&start=0
Thanks @fcoulter
OK, I have made the requested style changes.
Perfect 
Would it be better to do this check after the JPath::clean() because that's the actual final file path that will try to be deleted
Would it be better to do this check after the JPath::clean() because that's the actual final file path that will try to be deleted
I agree that this would make sense.
I was going on the assumption that putting it first saves the call to JPath::clean() which doesn't actually do a whole lot that would make a big difference (except clear up possible problems with directory separator), but actually I have no objection to putting it after.that call.
Change is made.
mbabker
- | Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-04-24 21:31:39 |
For testers: http://www.spiralscripts.co.uk/Joomla-Tips/nasty-trap-lurking-within-joomla-jfile-class.html has some good writeup of this bug and also explains how to reproduce it.
In short: Use
@fcoulter Thanks for the PR. It's very appreciated! I saw some minor codestyle issues which I'm going to add notes to the lines. It would be cool if you can update the PR with that then.