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

com_mediamanger fix for failure to traverse/create/delete directories #24924

Merged
merged 6 commits into from Jun 5, 2019

Conversation

Projects
None yet
10 participants
@nonickch
Copy link
Contributor

commented May 16, 2019

Pull Request for Issue ##24539 and #24723 .

Summary of Changes

Fix directory and file traversal/creation/deletion in com_media for a symlinked "images" folder.
See original bug submission at #24539, PR #24723 by @smehrbrodt and extra analysis by @HLeithner .
Please note that the #24723 changes are included in this PR although not via a merge that shows history (simply copy/paste of the diffed line).

Testing Instructions

Have the /images directory as a symlink that points outside the J installation folder. In my test case it points to ../images
1.Through the back-end mediamanager (/administrator/index.php?option=com_media&folder=), try to navigate to a subdirectory by clicking on the subdirectory icon.
2. Through the back-end mediamanager (/administrator/index.php?option=com_media&folder=), try to upload an image by clicking the "+Upload" button in the toolbar.
3. Through the back-end mediamanager (/administrator/index.php?option=com_media&folder=), try to delete an image by clicking the "X" icon at the top-left of a listed image.
4. Through the back-end mediamanager (/administrator/index.php?option=com_media&folder=), try to create a new directory by clicking the "Create New Folder" button in the toolbar.
5. Through the back-end mediamanager (/administrator/index.php?option=com_media&folder=), try to delete a directory by clicking the "X" icon at the top-right of the folder icon.
6. While in the back-end editing an article with TinyMCE (/administrator/index.php?option=com_content&view=article&layout=edit), drag&drop an image from your OS file manager in the content editing area

Expected result

  1. UI directory traversal should display the contents of the selected directory.
  2. Image uploads take place
  3. Image deletions take place
  4. Directory creations take place
  5. Directory deletions take place
  6. Image upload via the drag&drop takes place and the image is inserted in the content area

Actual result

  1. The pageload proceeds without any apparent errors yet it still shows the contents of the root /images directory.
  2. The file upload fails. No image upload takes place and a red Error box with the message "Invalid folder provided." appears
  3. The file deletion fails. No image deletion takes place and a red Error box with the message "Unable to delete: [uploaded-filename]. File name must only have alphanumeric characters and no spaces." appears
  4. The directory creation fails. No directory gets created and a green Info box appears with the message "Unable to create folder. Folder name must only have alphanumeric characters and no spaces.".
  5. The directory deletion fails. No directory deletion takes place and a red Error box with the message "Invalid folder provided." appears
  6. An upload begins and fails with the error message: "Unable to upload file.: [realpathed-base-media-dir]undefined". No file gets uploaded.

Documentation Changes Required

None

com_mediamanger fix for failure to traverse/create/delete directories
and files when the images directory is a symlink.

As per the discussion in #24539 and #24723
@wnkkm

This comment has been minimized.

Copy link

commented May 18, 2019

Can the fix also work for a symlinked sub-folder in Joomla installation's 'images' folder?
( J-installation/images/media --symlink--> /media )
This case is a bit different from the test scenario.

Thanks.

@zero-24 zero-24 requested a review from SniperSister May 18, 2019

@nonickch

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

No, unfortunately this is only going to work for the upper images (e.g COM_MEDIA base) dir.
As discussed in #24539 that'd need something different than realpath() to perform the path checks, and the proposed alternative/code was met with some resistance over "over-engineering".

I'd be happy to go down that path, but I fear that PR would be in danger getting rejected.

@okonomiyaki3000

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

I've tested this and I'm fine with it as it solves my immediate problem. I understand that there remain edge cases which are not solved but, in my opinion, those can be solved by a later patch (if at all).

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@okonomiyaki3000 can you please mark your test as successfully at Issue Tracker?

@okonomiyaki3000

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@franz-wohlkoenig is there any link to issue tracker around here?

@franz-wohlkoenig

This comment has been minimized.

@okonomiyaki3000

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

I have tested this item successfully on 5ae54c7

👍


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24924.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

I have tested this item successfully on 5ae54c7


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24924.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC label May 23, 2019

nonickch and others added some commits May 23, 2019

@HLeithner HLeithner merged commit ee6911d into joomla:staging Jun 5, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Hound No violations found. Woof!
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@HLeithner

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

thx

@joomla-cms-bot joomla-cms-bot removed the RTC label Jun 5, 2019

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.