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

Make “root” a reserved (restricted) key for Contexts #16475

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Sep 20, 2023

What does it do?

  1. Prevents "root" from being specified as a Context key.
  2. Slightly refactors the Create->beforeSave() error testing conditional to use a switch statement so only relevant code will execute in that block.
  3. Additionally, in the second commit, leverages the newly-added RESERVED_KEYS constant (in the modContext base class) in the Context processor GetList class.

Why is it needed?

The keyword "root" is used as the default id for all trees in the core. As such, attempting to create a Context with that key results in errors in the rendering of the Resources tree.

How to test

Attempt to create a new Context with the key "root." You should receive an error message indicating the key is reserved.

Related issue(s)/PR(s)

Resolves #16457

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (8d97447) 21.57% compared to head (4f58af1) 21.57%.

Files Patch % Lines
core/src/Revolution/Processors/Context/Create.php 45.45% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##                3.x   #16475   +/-   ##
=========================================
  Coverage     21.57%   21.57%           
- Complexity    10564    10566    +2     
=========================================
  Files           561      561           
  Lines         31932    31938    +6     
=========================================
+ Hits           6890     6892    +2     
- Misses        25042    25046    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

theboxer
theboxer previously approved these changes Oct 10, 2023
@Mark-H
Copy link
Collaborator

Mark-H commented Feb 10, 2024

While root gets blocked correctly, ROOT or roOt bypasses the check. Not 100% sure, but we probably want to guard against this regardless of casing?

@smg6511
Copy link
Collaborator Author

smg6511 commented Feb 10, 2024

While root gets blocked correctly, ROOT or roOt bypasses the check. Not 100% sure, but we probably want to guard against this regardless of casing?

Yeah, while it may or may not be needed technically, it'd be a good idea. I'll make a small update to account for casing later today...

@Mark-H
Copy link
Collaborator

Mark-H commented Feb 13, 2024

Thank you @smg6511.

@Ibochkarev Ibochkarev added the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Feb 14, 2024
Adds "root" to already protected set of keys ("mgr" and "web")
Use new base class constant in canRemove conditional instead of hard-coded inline array
Ensure reserved key comparison is not case sensitive
@opengeek opengeek merged commit f5645d8 into modxcms:3.x Feb 16, 2024
7 checks passed
opengeek pushed a commit that referenced this pull request Feb 16, 2024
### What does it do?

1. Prevents "root" from being specified as a Context key.
2. Slightly refactors the `Create->beforeSave() `error testing
conditional to use a `switch` statement so only relevant code will
execute in that block.
3. Additionally, in the second commit, leverages the newly-added
`RESERVED_KEYS` constant (in the `modContext` base class) in the Context
processor `GetList` class.

### Why is it needed?
The keyword "root" is used as the default id for all trees in the core.
As such, attempting to create a Context with that key results in errors
in the rendering of the Resources tree.

### How to test
Attempt to create a new Context with the key "root." You should receive
an error message indicating the key is reserved.

### Related issue(s)/PR(s)
Resolves #16457
@smg6511 smg6511 deleted the 3.x-issue-16457 branch March 30, 2024 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Naming a context root crashes the manager resource tree
6 participants