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

Protect important directories #14374

Merged
merged 7 commits into from
Mar 1, 2019
Merged

Conversation

mvoevodskiy
Copy link
Contributor

What does it do?

Added list of protected directories, that can not be renamed or removed from manager:

  • MODX_ASSETS_PATH
  • MODX_BASE_PATH
  • MODX_CONNECTORS_PATH
  • MODX_CORE_PATH
  • MODX_MANAGER_PATH
  • MODX_PROCESSORS_PATH
  • XPDO_CORE_PATH

Renaming or removing this directories will cause error with message "Access Denied".

Also, this PR contain 2 small fixes in processor browser/directory/rename

Why is it needed?

Description from issue:
Right click on any of these folders, rename or delete them and you are in trouble. Could also reduce impact of malicious actions by people with low morals or a short temper.

Related issue(s)/PR(s)

#14114

@JoshuaLuckers JoshuaLuckers changed the title Feature 14114 Protect important directories Feb 13, 2019
@JoshuaLuckers JoshuaLuckers added area-core pr/review-needed Pull request requires review and testing. labels Feb 13, 2019
@JoshuaLuckers JoshuaLuckers added this to the v3.0.0-alpha milestone Feb 13, 2019
@JoshuaLuckers
Copy link
Contributor

Thanks for taking time to create this PR and help make MODX better. It seems that you are a first-time contributor. To prevent us from merging this PR it's important that you sign the Contributor License Agreement (CLA).

If you have signed the CLA already please let us know. If you have any questions feel free to post them.

@mvoevodskiy
Copy link
Contributor Author

@JoshuaLuckers thanks for link to CLA. I have already signed today.

Copy link
Collaborator

@alroniks alroniks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see a more descriptive error message with an explanation of why user can to do such action.

@mvoevodskiy
Copy link
Contributor Author

I would love to see a more descriptive error message with an explanation of why user can to do such action.

Added.

Copy link
Contributor

@JoshuaLuckers JoshuaLuckers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please only include the English lexicon entries. Lexicons in other languages are managed via CrowdIn and they might get overwritten when translations are imported.

core/lexicon/ru/file.inc.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ruslan-Aleev Ruslan-Aleev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything works as described

*
* @return array
*/
public function getProtectedPathDirectories() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to make it static as it returns constants? Not a request, but just a question for discussion.

Copy link
Collaborator

@alroniks alroniks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code seems OK for me and works as expected.

@Mark-H Mark-H added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Feb 28, 2019
alroniks pushed a commit to alroniks/revolution that referenced this pull request Mar 1, 2019
* upstream/pr/14374:
  Fixed comment
  Fixed comment
  Removed ru lexicons  for rename and delete actions
  Added lexicons (en, ru) for rename and delete actions
  Fix new directory name check in processor browser/directory/rename
  Fix processor description browser/directory/rename
  Fixed issue modxcms#14114
@alroniks alroniks merged commit 78df2bb into modxcms:3.x Mar 1, 2019
@mvoevodskiy mvoevodskiy deleted the feature-14114 branch March 15, 2019 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core pr/ready-for-merging Pull request reviewed and tested and ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants