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.

Sign up for GitHub

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

[3.x] Restrict tag usage and ensure proper escaping of element name, caption, and description fields #15936

Open
wants to merge 9 commits into
base: 3.x
Choose a base branch
from
Open

[3.x] Restrict tag usage and ensure proper escaping of element name, caption, and description fields #15936

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3a5b726
Two minor lexicon fixes
Dec 8, 2021
1fdac97
Processor updates
Dec 8, 2021
f696056
modExt updates
Dec 8, 2021
2730a34
Loosen restrictions on TV captions and descriptions
Jan 20, 2022
e4ccd3d
Fix incorrect object reference
Apr 2, 2022
d291929
Reincorporate changes lost in rebase
May 6, 2022
bcd2f11
Minor consistency tweaks
May 6, 2022
f611c3b
Update core/lexicon/en/setting.inc.php
Jan 21, 2023
c6812e4
Use fully-qualified classname in conditional
Jan 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Processor updates 
Prevents saving of tags in fields that will be displayed in the interface (name, caption, description)
  • Loading branch information
Jim Graham committed Jan 21, 2023
commit 1fdac97739d913d35b41bc4c0addf251c8d18bde
65 changes: 43 additions & 22 deletions core/src/Revolution/Processors/Element/Create.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ abstract class Create extends CreateProcessor
/** @var modElement $object */
public $object;

protected $elementNameField = 'name';

public function initialize()
{
if ($this->classKey === modTemplate::class) {
$this->elementNameField = 'templatename';
}
return parent::initialize();
}

/**
* Cleanup the process and send back the response
*
Expand All @@ -37,9 +47,7 @@ abstract class Create extends CreateProcessor
public function cleanup()
{
$this->clearCache();
$fields = ['id', 'description', 'locked', 'category'];
array_push($fields, ($this->classKey == modTemplate::class ? 'templatename' : 'name'));

$fields = ['id', $this->elementNameField, 'description', 'locked', 'category'];
return $this->success('', $this->object->get($fields));
}

Expand All @@ -50,14 +58,34 @@ public function cleanup()
*/
public function beforeSave()
{
$nameField = $this->classKey === modTemplate::class ? 'templatename' : 'name';
$name = $this->getProperty($nameField, '');

/* verify element with that name does not already exist */
if ($this->alreadyExists($name)) {
$this->addFieldError($nameField, $this->modx->lexicon($this->objectType . '_err_ae', [
'name' => $name,
]));
$locked = (bool)$this->getProperty('locked', false);
$this->object->set('locked', $locked);

$elementClassName = array_pop(explode('\\', $this->classKey));

if ($elementClassName === 'modTemplateVar') {
smg6511 marked this conversation as resolved.
Show resolved Hide resolved
if ($caption = $this->getProperty('caption', '')) {
$this->object->set('caption', strip_tags($caption));
}
smg6511 marked this conversation as resolved.
Show resolved Hide resolved
}

if ($description = $this->getProperty('description', '')) {
$this->object->set('description', strip_tags($description));
}

$name = $this->getProperty($this->elementNameField, '');

/* verify element has a name and that name does not already exist */

if (empty($name)) {
$this->addFieldError($this->elementNameField, $this->modx->lexicon($this->objectType . '_err_ns_name'));
} else {
if ($this->alreadyExists($name)) {
$this->modx->error->addField(
$this->elementNameField,
$this->modx->lexicon($this->objectType . '_err_ae', ['name' => $name])
);
}
}

$category = $this->getProperty('category', 0);
Expand All @@ -72,9 +100,6 @@ public function beforeSave()
}
}

$locked = (bool)$this->getProperty('locked', false);
$this->object->set('locked', $locked);

$this->setElementProperties();
$this->validateElement();

Expand All @@ -90,21 +115,17 @@ public function beforeSave()
}

/**
* Check to see if a Chunk already exists with specified name
* Check to see if an Element with the specified name already exists
*
* @param string $name
*
* @return bool
*/
public function alreadyExists($name)
{
if ($this->classKey == modTemplate::class) {
$c = ['templatename' => $name];
} else {
$c = ['name' => $name];
}

return $this->modx->getCount($this->classKey, $c) > 0;
return $this->modx->getCount($this->classKey, [
$this->elementNameField => $name,
]) > 0;
}

/**
Expand Down
48 changes: 35 additions & 13 deletions core/src/Revolution/Processors/Element/Update.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ abstract class Update extends UpdateProcessor
/** @var modElement $object */
public $object;

protected $elementNameField = 'name';

public function initialize()
{
if ($this->classKey === modTemplate::class) {
$this->elementNameField = 'templatename';
}
return parent::initialize();
}

public function beforeSet()
{
// Make sure the element isn't locked
Expand All @@ -46,13 +56,25 @@ public function beforeSave()
$this->object->set('locked', (bool)$locked);
}

/* make sure a name was specified */
$nameField = $this->classKey === modTemplate::class ? 'templatename' : 'name';
$name = $this->getProperty($nameField, '');
$elementClassName = array_pop(explode('\\', $this->classKey));

if ($elementClassName === 'modTemplateVar') {
if ($caption = $this->getProperty('caption', '')) {
$this->object->set('caption', strip_tags($caption));
}
}

if ($description = $this->getProperty('description', '')) {
$this->object->set('description', strip_tags($description));
}

/* verify element has a name and that name does not already exist */

$name = $this->getProperty($this->elementNameField, '');

if (empty($name)) {
$this->addFieldError($nameField, $this->modx->lexicon($this->objectType . '_err_ns_name'));
} elseif ($this->alreadyExists($name)) {
} else if ($this->alreadyExists($name)) {
/* if changing name, but new one already exists */
$this->modx->error->addField(
$nameField,
Expand All @@ -74,7 +96,7 @@ public function beforeSave()
if ($this->object->staticContentChanged()) {
if (!$this->object->isStaticSourceMutable()) {
$this->addFieldError('static_file', $this->modx->lexicon('element_static_source_immutable'));
} elseif (!$this->object->isStaticSourceValidPath()) {
} else if (!$this->object->isStaticSourceValidPath()) {
$this->addFieldError('static_file', $this->modx->lexicon('element_static_source_protected_invalid'));
}
}
Expand All @@ -84,12 +106,10 @@ public function beforeSave()

public function alreadyExists($name)
{
$nameField = $this->classKey === modTemplate::class ? 'templatename' : 'name';

return $this->modx->getCount($this->classKey, [
'id:!=' => $this->object->get('id'),
$nameField => $name,
]) > 0;
'id:!=' => $this->object->get('id'),
$this->elementNameField => $name,
]) > 0;
}

public function afterSave()
Expand All @@ -101,11 +121,13 @@ public function afterSave()

public function cleanup()
{
$fields = array('id', 'description', 'locked', 'category', 'content');
array_push($fields, ($this->classKey == modTemplate::class ? 'templatename' : 'name'));
$fields = ['id', $this->elementNameField, 'description', 'locked', 'category', 'content'];
return $this->success(
'',
array_merge($this->object->get($fields), ['previous_category' => $this->previousCategory])
array_merge(
$this->object->get($fields),
['previous_category' => $this->previousCategory]
)
);
}
}
3 changes: 3 additions & 0 deletions core/src/Revolution/Processors/Security/Forms/Set/Update.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ public function setFieldRules()
$this->newRules[] = $rule;
}
if (!empty($field['label'])) {
$field['label'] = strip_tags($field['label']);
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
Expand Down Expand Up @@ -224,6 +225,7 @@ public function setTabRules()
$this->newRules[] = $rule;
}
if (!empty($tab['label'])) {
$tab['label'] = strip_tags($tab['label']);
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
Expand Down Expand Up @@ -285,6 +287,7 @@ public function setTVRules()
$this->newRules[] = $rule;
}
if (!empty($tvData['label'])) {
$tvData['label'] = strip_tags($tvData['label']);
$rule = $this->modx->newObject(modActionDom::class);
$rule->set('set', $this->object->get('id'));
$rule->set('action', $this->object->get('action'));
Expand Down
2 changes: 1 addition & 1 deletion core/src/Revolution/modTemplateVar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ public function renderInput($resource = null, $options = [])
$this->set('default_text', $this->processBindings($this->get('default_text'), $resourceId));

/* strip tags from description */
$this->set('description', strip_tags($this->get('description')));
// $this->set('description', strip_tags($this->get('description')));

$params = [];
if ($paramstring = $this->get('display_params')) {
Expand Down