Skip to content

Commit

Permalink
Merge commit from fork
Browse files Browse the repository at this point in the history 
* introduce wrapper class to keep unescaped variants available for templates

* escape more settings before usage
  • Loading branch information
@Flyingmana
Flyingmana authored Jul 24, 2024
1 parent eba4aa8 commit 484cf8a
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 5 deletions.
35 changes: 35 additions & 0 deletions app/code/core/Mage/Core/Model/Security/HtmlEscapedString.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php
declare(strict_types=1);

This comment has been minimized.

Sign in to view

Copy link
@pquerner

pquerner Jul 24, 2024

Contributor

complete license header is missing
/**
*

This comment has been minimized.

Sign in to view

Copy link
@pquerner

pquerner Jul 24, 2024

Contributor

empty or forgotten?
*/
class Mage_Core_Model_Security_HtmlEscapedString implements Stringable
{

This comment has been minimized.

Sign in to view

Copy link
@pquerner

pquerner Jul 24, 2024

Contributor

extra space
protected $originalValue;
protected $allowedTags;

/**
* @param string $originalValue
* @param string[]|null $allowedTags
*/
public function __construct(string $originalValue, ?array $allowedTags = null)
{
$this->originalValue = $originalValue;
$this->allowedTags = $allowedTags;
}

public function __toString(): string
{
return (string) Mage::helper('core')->escapeHtml(
$this->originalValue,
$this->allowedTags
);
}

public function getUnescapedValue(): string
{
return $this->originalValue;
}
}
16 changes: 12 additions & 4 deletions app/code/core/Mage/Page/Block/Html/Header.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ public function setLogo($logo_src, $logo_alt)
public function getLogoSrc()
{
if (empty($this->_data['logo_src'])) {
$this->_data['logo_src'] = Mage::getStoreConfig('design/header/logo_src');
$this->_data['logo_src'] = new Mage_Core_Model_Security_HtmlEscapedString(

This comment has been minimized.

Sign in to view

Copy link
@pquerner

pquerner Jul 24, 2024

Contributor

extra space
(string) Mage::getStoreConfig('design/header/logo_src')
);
}
return $this->getSkinUrl($this->_data['logo_src']);
}
Expand All @@ -68,7 +70,9 @@ public function getLogoSrc()
public function getLogoSrcSmall()
{
if (empty($this->_data['logo_src_small'])) {
$this->_data['logo_src_small'] = Mage::getStoreConfig('design/header/logo_src_small');
$this->_data['logo_src_small'] = new Mage_Core_Model_Security_HtmlEscapedString(

This comment has been minimized.

Sign in to view

Copy link
@pquerner

pquerner Jul 24, 2024

Contributor

extra space
(string) Mage::getStoreConfig('design/header/logo_src_small')
);
}
return $this->getSkinUrl($this->_data['logo_src_small']);
}
Expand All @@ -79,7 +83,9 @@ public function getLogoSrcSmall()
public function getLogoAlt()
{
if (empty($this->_data['logo_alt'])) {
$this->_data['logo_alt'] = Mage::getStoreConfig('design/header/logo_alt');
$this->_data['logo_alt'] = new Mage_Core_Model_Security_HtmlEscapedString(
(string) Mage::getStoreConfig('design/header/logo_alt')
);
}
return $this->_data['logo_alt'];
}
Expand All @@ -97,7 +103,9 @@ public function getWelcome()
if (Mage::isInstalled() && Mage::getSingleton('customer/session')->isLoggedIn()) {
$this->_data['welcome'] = $this->__('Welcome, %s!', $this->escapeHtml(Mage::getSingleton('customer/session')->getCustomer()->getName()));
} else {
$this->_data['welcome'] = Mage::getStoreConfig('design/header/welcome');
$this->_data['welcome'] = new Mage_Core_Model_Security_HtmlEscapedString(
(string) Mage::getStoreConfig('design/header/welcome')
);
}
}

Expand Down
4 changes: 3 additions & 1 deletion app/code/core/Mage/Page/Block/Html/Welcome.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ protected function _toHtml()
if (Mage::isInstalled() && $this->_getSession()->isLoggedIn()) {
$this->_data['welcome'] = $this->__('Welcome, %s!', $this->escapeHtml($this->_getSession()->getCustomer()->getName()));
} else {
$this->_data['welcome'] = Mage::getStoreConfig('design/header/welcome');
$this->_data['welcome'] = new Mage_Core_Model_Security_HtmlEscapedString(
(string) Mage::getStoreConfig('design/header/welcome')
);
}
}

Expand Down

9 comments on commit 484cf8a

@pquerner
Copy link
Contributor

Choose a reason for hiding this comment

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

Not allowing Mage::getModel here? Why?

@pquerner
Copy link
Contributor

Choose a reason for hiding this comment

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

Another xss in the backend.. That selfxss is for sure dangerous.. oô

@sreichel
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not $this->escapeHtml() at all?

@sreichel
Copy link
Contributor

@sreichel sreichel commented on 484cf8a Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It would also be possibe to add a flag to Mage::getStoreConfig($path, $storeId = null, $secureFlag = null) to escape that values.

@Flyingmana
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not allowing Mage::getModel here? Why?

no need for this modularity here. Just because we can do it, doesnt mean we have to.

Why not $this->escapeHtml() at all?

to expose a way to still get the unescaped values in templates, for Stores which might actually rely on some of the special characters.

@sreichel
Copy link
Contributor

@sreichel sreichel commented on 484cf8a Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

to expose a way to still get the unescaped values in templates, for Stores which might actually rely on some of the special characters.

Mhh, store the config-value in a variable an escape it - or not - as you need. (to not call it twice)

The unescape value isnt used - it least not in core.

@pquerner
Copy link
Contributor

Choose a reason for hiding this comment

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

Not allowing Mage::getModel here? Why?

no need for this modularity here. Just because we can do it, doesnt mean we have to.

But now I have to rewrite the full block instead. Not sure if thats worth. I remember another PR where a new Mage_XXX_Model_XXX was changed explicity to Mage::getModel - just to allowe rewrites.

But if its not worth of having, I'm fine with it. It just itched me :D

@Flyingmana
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which situation you would want to rewrite an escaping method?
And with cweagans/composer-patches its also a lot easier nowdays to patch any core file of any lib.

Anyway, if anyone sees the need to change it, they can provide a PR

@sreichel
Copy link
Contributor

Choose a reason for hiding this comment

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

And with cweagans/composer-patches its also a lot easier nowdays to patch any core file of any lib.

Am i wrong, but composer-patches do not work, when you install OM via git clone???

Please sign in to comment.