[#24723] - Fix directory traversal for symlinked folders
- Closed
- 16 May 2019
- Medium
- Build: staging
- # 24723
- Diff
- smehrbrodt:patch-9
User tests: Successful: Unsuccessful:
Pull Request for Issue #24539 .
Summary of Changes
Fix directory traversal in com_media for symlinked "images" folder
Testing Instructions
- Set up Joomla with the "images" folder being a symlink
- Create a new article
- Try to set an intro image
- Navigate to some folder in the media selector popup
Expected result
You can traverse through the folders and select an image
Actual result
Clicking on a folder does nothing
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
| Category | ⇒ | Administration com_media |
If found several other places with similar usages:
If found several other places with similar usages:
Great, but please let's chase them one by one so that each patch has clear and simple testing instructions.
We have to fix all or none.
And I want to merge this in to 3.9.6 so tests would be really appreciated.
@smehrbrodt are you planing to update your PR?
@smehrbrodt are you planing to update your PR?
I have no idea how to test the other places, so I'll keep this patch just for the thing I tested.
@HLeithner if you are fine with blind fixes, I can update the other places too, but as said, no idea how to test them (or even give test instructions).
@HLeithner any comment?
I don't like to merge a pr that only fix the same problem on one position.
I don't like to merge a pr that only fix the same problem on one position.
And people don't like blindly patching code. If someone wants to test that this same fix works for all lines of code that you pointed out, then sure, it can be included in a pull request. Expecting a contributor to blindly change a pull request with additions that they have not tested is a little unfair, then again most pull requests end up with some form of scope creep around here.
I note the complete absence of any tests or comments by any of the people who demanded this change
I don't like to merge a pr that only fix the same problem on one position.
And people don't like blindly patching code. If someone wants to test that this same fix works for all lines of code that you pointed out, then sure, it can be included in a pull request. Expecting a contributor to blindly change a pull request with additions that they have not tested is a little unfair, then again most pull requests end up with some form of scope creep around here.
It was not my intention to get a blindly patched pr, till now I didn't found time to find out what is the use case is for each line that have to be patched. But yeah let fix one case and ignore all other is much better.
But yeah let fix one case and ignore all other is much better.
If that's all that has been tested so far then yes, that actually is better. Again, if someone wants to test the same fix is valid for the other lines you pointed out then include it, otherwise without testing and validation it's blindly patching files. I didn't when I spent 15 minutes coming up with this one-liner originally because the bug report was about traversing directories in the UI, there wasn't a bug report about any of the other write actions (probably because you couldn't execute them in the right context without this patch in the first place if I'm going to put a bet down on things).
I never said that we don't need correct test instructions.
I have tested this item
- Tested with media manager: administrator/index.php?option=com_media
- Tested with article Intro Image
LAMP Server. PHP7.3.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24723.
But image upload doesn't work yet. But we know, as discussed above, that's not part of this pr.
Error Invalid folder provided.
@brianteeman I'm here, testing the PR now and I'll also look into all of the files @HLeithner mentioned. I'm pretty sure I can walk back from the files and find most (if not all) cases where they're being used.
I just come up with a PR, right?
Confirming that this PR solves the original report of Issue #24539 .
Also I went through all of the 3.9.5 diffs and I agree/confirm with the list of files @HLeithner listed as needing the same kind of fix so we can address the entire set of issues.
Working on a PR for that.
Please note that I haven't checked for any pre-3.9.5 "realpath" usages, but given that the whole issue was due to an addition to a security fix 3.9.5 and that I haven't noticed any issues on our platform before, I feel relatively comfortable not going through the entire codebase
@nonickch please mark your test as successfully > https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results
Not yet, sorry for the delay everyone, I'm a bit swamped these days.
I'll have a go at it tomorrow while I mark my test complete as well (never used the test plugin before).
| Labels |
Added:
?
|
||
Made my PR at #24924 that references all locations @HLeithner spotted.
This implies that the contents of this PR is also present there.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-05-16 13:57:09 |
| Closed_By | ⇒ | joomla-cms-bot |
| Closed_By | joomla-cms-bot | ⇒ | Quy |
| Status | Closed | ⇒ | Pending |
Set to "closed" on behalf of @Quy by The JTracker Application at issues.joomla.org/joomla-cms/24723
Closing in favor for PR #24924.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24723.
Closing in favor for PR #24924.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24723.
| Closed_By | Quy | ⇒ | joomla-cms-bot |
| Status | Pending | ⇒ | Closed |
| Closed_Date | 2019-05-16 13:57:09 | ⇒ | 2019-05-16 13:57:45 |
Set to "closed" on behalf of @Quy by The JTracker Application at issues.joomla.org/joomla-cms/24723
@nonickch @joel-lupfer please test > https://docs.joomla.org/Testing_Joomla!_patches