Skip to content

Sanitize Input Data: Trim Whitespace on Key Backend Models for Consistent Data Storage #4956

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+50 −4
Open

Sanitize Input Data: Trim Whitespace on Key Backend Models for Consistent Data Storage #4956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
aa84fc8
Trim whitespace from all form input fields before saving to ensure cl…
addison74 Oct 1, 2025
2cb504f
Fixes php-cs-fixer errors
addison74 Oct 1, 2025
2553366
Fix formatting of username assignment
addison74 Oct 1, 2025
b40b707
Trim whitespace in tax calculation fields
addison74 Oct 1, 2025
2b89804
Clean up comments in Address.php
addison74 Oct 1, 2025
5466aaf
Merge branch 'main' into fix/trim-all-form-inputs
sreichel Oct 2, 2025
fdc5bc0
Merge branch 'main' into fix/trim-all-form-inputs
sreichel Oct 2, 2025
89ca17d
Merge branch 'OpenMage:main' into fix/trim-all-form-inputs
addison74 Oct 4, 2025
83f62c1
Merge branch 'OpenMage:main' into fix/trim-all-form-inputs
addison74 Oct 14, 2025
3a816d2
Merge branch 'OpenMage:main' into fix/trim-all-form-inputs
addison74 Oct 25, 2025
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
8 changes: 4 additions & 4 deletions app/code/core/Mage/Admin/Model/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,9 @@ protected function _construct()
protected function _beforeSave()
{
$data = [
'firstname' => $this->getFirstname(),
'lastname' => $this->getLastname(),
'email' => $this->getEmail(),
'firstname' => trim((string) $this->getFirstname()),
'lastname' => trim((string) $this->getLastname()),
'email' => trim((string) $this->getEmail()),
'modified' => $this->_getDateNow(),
'extra' => serialize($this->getExtra()),
];
Expand All @@ -145,7 +145,7 @@ protected function _beforeSave()
}

if ($this->getUsername()) {
$data['username'] = $this->getUsername();
$data['username'] = trim((string) $this->getUsername());
}

if ($this->getNewPassword()) {
Expand Down
26 changes: 26 additions & 0 deletions app/code/core/Mage/Core/Controller/Request/Http.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,4 +612,30 @@ public function getInternallyForwarded()
{
return $this->_internallyForwarded;
}

/**
* Retrieve a parameter from request (GET/POST) and trim whitespace for string and array values.
*
* @param string $key
* @param mixed $default
* @return mixed
*/
public function getParam($key, $default = null)
{
// Get the parameter value from parent (Zend_Controller_Request_Http)
$value = parent::getParam($key, $default);

// Trim whitespace for string values
if (is_string($value)) {
$value = trim($value);
}

// For array values, trim each string element
if (is_array($value)) {
$value = array_map(function ($v) {
return is_string($v) ? trim($v) : $v;
}, $value);
Comment on lines +635 to +637
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Using array_map with an anonymous function for every array parameter could impact performance for large arrays. Consider using a foreach loop or array_walk for better performance, especially since this method is called frequently for request parameters.

Suggested change
$value = array_map(function ($v) {
return is_string($v) ? trim($v) : $v;
}, $value);
foreach ($value as $k => $v) {
if (is_string($v)) {
$value[$k] = trim($v);
}
}

Copilot uses AI. Check for mistakes.
}
return $value;
}
Comment on lines +633 to +640
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

The array_map function with anonymous function creates overhead on every request parameter access. For deeply nested arrays, this could impact performance. Consider implementing recursive trimming only when necessary or adding a depth limit.

Suggested change
// For array values, trim each string element
if (is_array($value)) {
$value = array_map(function ($v) {
return is_string($v) ? trim($v) : $v;
}, $value);
}
return $value;
}
// For array values, trim each string element recursively
if (is_array($value)) {
$value = $this->_trimRecursive($value);
}
return $value;
}
/**
* Recursively trim all string values in an array, with a depth limit to prevent excessive recursion.
*
* @param mixed $value
* @param int $depth
* @return mixed
*/
private function _trimRecursive($value, $depth = 10)
{
if ($depth < 0) {
// Prevent excessive recursion
return $value;
}
if (is_array($value)) {
foreach ($value as $k => $v) {
$value[$k] = $this->_trimRecursive($v, $depth - 1);
}
return $value;
}
if (is_string($value)) {
return trim($value);
}
return $value;
}

Copilot uses AI. Check for mistakes.
}
8 changes: 8 additions & 0 deletions app/code/core/Mage/Sales/Model/Order/Address.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,19 @@ public function getOrder()

/**
* Before object save manipulations
* Trim whitespace for all string data to prevent unwanted spaces on save
*
* @return $this
*/
protected function _beforeSave()
{
// Trim all string fields before saving (for clean data storage)
foreach ($this->getData() as $key => $value) {
if (is_string($value)) {
$this->setData($key, trim($value));
Comment on lines +133 to +136
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

This approach may be inefficient as it iterates through all data fields and calls setData() for each string field, potentially triggering change detection for every field. Consider only trimming specific known string fields or implementing a more targeted approach.

Suggested change
// Trim all string fields before saving (for clean data storage)
foreach ($this->getData() as $key => $value) {
if (is_string($value)) {
$this->setData($key, trim($value));
// Trim only known string fields before saving (for clean data storage)
$fieldsToTrim = array(
'firstname', 'lastname', 'middlename', 'prefix', 'suffix',
'company', 'street', 'city', 'region', 'postcode', 'country_id',
'telephone', 'fax', 'email'
);
foreach ($fieldsToTrim as $field) {
$value = $this->getData($field);
if (is_string($value)) {
$this->setData($field, trim($value));

Copilot uses AI. Check for mistakes.
}
}

parent::_beforeSave();

if (!$this->getParentId() && $this->getOrder()) {
Expand Down
3 changes: 3 additions & 0 deletions app/code/core/Mage/Tag/Model/Resource/Tag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ public function loadByName($model, $name)
*/
protected function _beforeSave(Mage_Core_Model_Abstract $object)
{
// Trim whitespace for Tag name
$object->setName(trim($object->getName()));

if (!$object->getId() && $object->getStatus() == $object->getApprovedStatus()) {
$searchTag = new Varien_Object();
$this->loadByName($searchTag, $object->getName());
Expand Down
9 changes: 9 additions & 0 deletions app/code/core/Mage/Tax/Model/Calculation/Rate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,15 @@ protected function _construct()
*/
protected function _beforeSave()
{
// Trim whitespace for all relevant fields before validation and save
$this->setCode(trim((string) $this->getCode()));
$this->setTaxCountryId(trim((string) $this->getTaxCountryId()));
$this->setTaxRegionId(trim((string) $this->getTaxRegionId()));
$this->setTaxPostcode(trim((string) $this->getTaxPostcode()));
$this->setRate(trim((string) $this->getRate()));
$this->setZipFrom(trim((string) $this->getZipFrom()));
$this->setZipTo(trim((string) $this->getZipTo()));
Comment on lines +73 to +79
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Consider extracting the trim operation into a helper method to reduce code duplication. The pattern trim((string) $this->getField()) is repeated 7 times and could be simplified with a method like trimField($fieldName) that handles the string conversion and trimming.

Copilot uses AI. Check for mistakes.

if ($this->getCode() === '' || $this->getTaxCountryId() === '' || $this->getRate() === ''
|| $this->getZipIsRange() && ($this->getZipFrom() === '' || $this->getZipTo() === '')
) {
Expand Down
Loading