composer/iTop
6
0
Fork 0
mirror of https://github.com/Combodo/iTop.git synced 2026-02-13 07:24:13 +01:00
Code Issues Packages Projects Releases Wiki Activity

N°3448 - Framework field size check not correctly implemented for multibytes languages/strings (#528)

Browse Source
This commit is contained in:
Anne-Catherine
2024-01-15 16:10:23 +01:00
committed by GitHub
parent a8cd9566ac
commit cff50f8732
7 changed files with 207 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
core/dbobject.class.php
View File

@@ -756,9 +756,10 @@ abstract class DBObject implements iDisplay
{ {
$oAttDef = MetaModel::GetAttributeDef(get_class($this), $sAttCode); $oAttDef = MetaModel::GetAttributeDef(get_class($this), $sAttCode);
$iMaxSize = $oAttDef->GetMaxSize(); $iMaxSize = $oAttDef->GetMaxSize();
if ($iMaxSize && (strlen($sValue) > $iMaxSize)) $sLength = mb_strlen($sValue);
{ if ($iMaxSize && ($sLength > $iMaxSize)) {
$sValue = substr($sValue, 0, $iMaxSize); $sMessage = " -truncated ($sLength chars)";
$sValue = mb_substr($sValue, 0, $iMaxSize - mb_strlen($sMessage)).$sMessage;
} }
$this->Set($sAttCode, $sValue); $this->Set($sAttCode, $sValue);
} }
@@ -2110,33 +2111,26 @@ abstract class DBObject implements iDisplay
return true; return true;
} }
if ($toCheck instanceof ormSet) if ($toCheck instanceof ormSet) {
{
return true; return true;
} }
return "Bad type"; return "Bad type";
} } elseif ($oAtt->IsScalar()) {
elseif ($oAtt->IsScalar())
{
$aValues = $oAtt->GetAllowedValues($this->ToArgsForQuery()); $aValues = $oAtt->GetAllowedValues($this->ToArgsForQuery());
if (is_array($aValues) && (count($aValues) > 0)) if (is_array($aValues) && (count($aValues) > 0)) {
{ if (!array_key_exists($toCheck, $aValues)) {
if (!array_key_exists($toCheck, $aValues))
{
return "Value not allowed [$toCheck]"; return "Value not allowed [$toCheck]";
} }
} }
if (!is_null($iMaxSize = $oAtt->GetMaxSize())) if (!is_null($iMaxSize = $oAtt->GetMaxSize())) {
{ $iLen = mb_strlen($toCheck);
$iLen = strlen($toCheck); if ($iLen > $iMaxSize) {
if ($iLen > $iMaxSize)
{
return "String too long (found $iLen, limited to $iMaxSize)"; return "String too long (found $iLen, limited to $iMaxSize)";
} }
} }
if (!$oAtt->CheckFormat($toCheck)) if (!$oAtt->CheckFormat($toCheck)) {
{
return "Wrong format [$toCheck]"; return "Wrong format [$toCheck]";
} }
} }

4
core/displayablegraph.class.inc.php
View File

@@ -62,7 +62,7 @@ class DisplayableNode extends GraphNode
public function GetWidth() public function GetWidth()
{ {
return max(32, 5*strlen($this->GetProperty('label'))); // approximation of the text's bounding box return max(32, 5 * mb_strlen($this->GetProperty('label'))); // approximation of the text's bounding box
} }
public function GetHeight() public function GetHeight()
@@ -491,7 +491,7 @@ class DisplayableNode extends GraphNode
if ($bNoLabel) if ($bNoLabel)
{ {
// simulate a fake label with the approximate same size as the true label // simulate a fake label with the approximate same size as the true label
$sLabel = str_repeat('x',strlen($this->GetProperty('label', $this->GetId()))); $sLabel = str_repeat('x', mb_strlen($this->GetProperty('label', $this->GetId())));
$sDot = 'label="'.$sLabel.'"'; $sDot = 'label="'.$sLabel.'"';
} }
else else

41
core/event.class.inc.php
View File

@@ -242,44 +242,33 @@ class EventIssue extends Event
{ {
if (is_string($sValue)) if (is_string($sValue))
{ {
if (strlen($sValue) < 256) if (mb_strlen($sValue) < 256) {
{
$aPost[$sKey] = $sValue; $aPost[$sKey] = $sValue;
} else {
$aPost[$sKey] = "!long string: ".mb_strlen($sValue)." chars";
} }
else } else {
{
$aPost[$sKey] = "!long string: ".strlen($sValue). " chars";
}
}
else
{
// Not a string (avoid warnings in case the value cannot be easily casted into a string) // Not a string (avoid warnings in case the value cannot be easily casted into a string)
$aPost[$sKey] = @(string) $sValue; $aPost[$sKey] = @(string)$sValue;
} }
} }
$this->Set('arguments_post', $aPost); $this->Set('arguments_post', $aPost);
} } else {
else
{
$this->Set('arguments_post', array()); $this->Set('arguments_post', array());
} }
$sLength = mb_strlen($this->Get('issue'));
$sLength = strlen($this->Get('issue')); if ($sLength > 255) {
if ($sLength > 255) $this->Set('issue', mb_substr($this->Get('issue'), 0, 210)." -truncated ($sLength chars)");
{
$this->Set('issue', substr($this->Get('issue'), 0, 200)." -truncated ($sLength chars)");
} }
$sLength = strlen($this->Get('impact')); $sLength = mb_strlen($this->Get('impact'));
if ($sLength > 255) if ($sLength > 255) {
{ $this->Set('impact', mb_substr($this->Get('impact'), 0, 210)." -truncated ($sLength chars)");
$this->Set('impact', substr($this->Get('impact'), 0, 200)." -truncated ($sLength chars)");
} }
$sLength = strlen($this->Get('page')); $sLength = mb_strlen($this->Get('page'));
if ($sLength > 255) if ($sLength > 255) {
{ $this->Set('page', mb_substr($this->Get('page'), 0, 210)." -truncated ($sLength chars)");
$this->Set('page', substr($this->Get('page'), 0, 200)." -truncated ($sLength chars)");
} }
} }
} }

29
synchro/synchrodatasource.class.inc.php
View File

@@ -1930,17 +1930,13 @@ class SynchroLog extends DBObject
$oAttDef = MetaModel::GetAttributeDef(get_class($this), 'traces'); $oAttDef = MetaModel::GetAttributeDef(get_class($this), 'traces');
$iMaxSize = $oAttDef->GetMaxSize(); $iMaxSize = $oAttDef->GetMaxSize();
if (strlen($sPrevTrace) > 0) if (strlen($sPrevTrace) > 0) {
{
$sTrace = $sPrevTrace."\n".implode("\n", $this->m_aTraces); $sTrace = $sPrevTrace."\n".implode("\n", $this->m_aTraces);
} } else {
else
{
$sTrace = implode("\n", $this->m_aTraces); $sTrace = implode("\n", $this->m_aTraces);
} }
if (strlen($sTrace) >= $iMaxSize) if (mb_strlen($sTrace) >= $iMaxSize) {
{ $sTrace = mb_substr($sTrace, 0, $iMaxSize - 40)."...\nTruncated (size: ".mb_strlen($sTrace).')';
$sTrace = substr($sTrace, 0, $iMaxSize - 255)."...\nTruncated (size: ".strlen($sTrace).')';
} }
$this->Set('traces', $sTrace); $this->Set('traces', $sTrace);
@@ -2145,9 +2141,8 @@ class SynchroReplica extends DBObject implements iDisplay
break; break;
} }
if (strlen($sWarningMessage) > $MAX_WARNING_LENGTH) if (mb_strlen($sWarningMessage) > $MAX_WARNING_LENGTH) {
{ $sWarningMessage = mb_substr($sWarningMessage, 0, $MAX_WARNING_LENGTH - 3).'...';
$sWarningMessage = substr($sWarningMessage, 0, $MAX_WARNING_LENGTH - 3).'...';
} }
$this->Set('status_last_warning', $sWarningMessage); $this->Set('status_last_warning', $sWarningMessage);
@@ -2187,17 +2182,13 @@ class SynchroReplica extends DBObject implements iDisplay
public function SetLastError($sMessage, $oException = null) public function SetLastError($sMessage, $oException = null)
{ {
if ($oException) if ($oException) {
{
$sText = $sMessage.$oException->getMessage(); $sText = $sMessage.$oException->getMessage();
} } else {
else
{
$sText = $sMessage; $sText = $sMessage;
} }
if (strlen($sText) > 255) if (mb_strlen($sText) > 255) {
{ $sText = mb_substr($sText, 0, 200).'...('.mb_strlen($sText).' chars)...';
$sText = substr($sText, 0, 200).'...('.strlen($sText).' chars)...';
} }
$this->Set('status_last_error', $sText); $this->Set('status_last_error', $sText);
} }

97
tests/php-unit-tests/unitary-tests/core/DBObjectTest.php
View File

@@ -32,6 +32,7 @@ use Organization;
use Person; use Person;
use Team; use Team;
use User; use User;
use UserRequest;
use UserRights; use UserRights;
use utils; use utils;
@@ -47,6 +48,7 @@ class DBObjectTest extends ItopDataTestCase
// Counts // Counts
public $aReloadCount = []; public $aReloadCount = [];
protected function setUp(): void protected function setUp(): void
{ {
parent::setUp(); parent::setUp();
@@ -1199,7 +1201,7 @@ class DBObjectTest extends ItopDataTestCase
{ {
return $this->aReloadCount[$sClass][$sKey] ?? 0; return $this->aReloadCount[$sClass][$sKey] ?? 0;
} }
/** /**
* @since 3.1.0-3 3.1.1 3.2.0 N°6716 test creation * @since 3.1.0-3 3.1.1 3.2.0 N°6716 test creation
*/ */
@@ -1235,4 +1237,97 @@ class DBObjectTest extends ItopDataTestCase
$fTotalDuration = microtime(true) - $fStart; $fTotalDuration = microtime(true) - $fStart;
echo 'Total duration: '.sprintf('%.3f s', $fTotalDuration)."\n\n"; echo 'Total duration: '.sprintf('%.3f s', $fTotalDuration)."\n\n";
} }
public function CheckLongValueInAttributeProvider() {
return [
// UserRequest.title is an AttributeString (maxsize = 255)
'title 250 chars' => ['title', 250],
'title 254 chars' => ['title', 254],
'title 255 chars' => ['title', 255],
'title 256 chars' => ['title', 256],
'title 300 chars' => ['title', 300],
// UserRequest.solution is an AttributeText (maxsize=65535) with format=text
'solution 250 chars' => ['solution', 250],
'solution 60000 chars' => ['solution', 60000],
'solution 65534 chars' => ['solution', 65534],
'solution 65535 chars' => ['solution', 65535],
'solution 65536 chars' => ['solution', 65536],
'solution 70000 chars' => ['solution', 70000],
];
}
/**
* Test check long field with non ascii characters
*
* @covers DBObject::Set
* @covers DBObject::CheckToWrite
* @covers DBObject::SetTrim
*
* @dataProvider CheckLongValueInAttributeProvider
*
* @since 3.1.2 N°3448 - Framework field size check not correctly implemented for multi-bytes languages/strings
*/
public function testCheckLongValueInAttribute(string $sAttrCode, int $iValueLength)
{
$sPrefix = 'a'; // just a small prefix so that the emoji bytes won't have a power of 2 (we want a non even value)
$sEmojiToRepeat = '😎'; // this emoji is 4 bytes long
$sEmojiRepeats = str_repeat($sEmojiToRepeat, $iValueLength - mb_strlen($sPrefix));
$sValueToSet = $sPrefix . $sEmojiRepeats;
$oTicket = MetaModel::NewObject('UserRequest', [
'ref' => 'Test Ticket',
'title' => 'Create OK',
'description' => 'Create OK',
'caller_id' => 15,
'org_id' => 3,
]);
$oTicket->Set($sAttrCode, $sValueToSet);
$sValueInObject = $oTicket->Get($sAttrCode);
$this->assertSame($sValueToSet, $sValueInObject, 'Set should not alter the value even if the value is too long');
$oAttDef = MetaModel::GetAttributeDef(UserRequest::class, $sAttrCode);
$iAttrMaxSize = $oAttDef->GetMaxSize();
$bIsValueToSetBelowAttrMaxSize = ($iValueLength <= $iAttrMaxSize);
/** @noinspection PhpUnusedLocalVariableInspection */
[$bCheckStatus, $aCheckIssues, $bSecurityIssue] = $oTicket->CheckToWrite();
$this->assertEquals($bIsValueToSetBelowAttrMaxSize, $bCheckStatus, "CheckResult result:" . var_export($aCheckIssues, true));
$oTicket->SetTrim($sAttrCode, $sValueToSet);
$sValueInObject = $oTicket->Get($sAttrCode);
if ($bIsValueToSetBelowAttrMaxSize) {
$this->assertEquals($sValueToSet, $sValueInObject,'Should not alter string that is already shorter than attribute max length');
} else {
$this->assertEquals($iAttrMaxSize, mb_strlen($sValueInObject),'Should truncate at the same length than attribute max length');
$sLastCharsOfValueInObject = mb_substr($sValueInObject, -30);
$this->assertStringContainsString(' -truncated', $sLastCharsOfValueInObject, 'Should end with "truncated" comment');
}
}
public function SetTrimProvider()
{
return [
'short string should not be truncated' => ['name','name'],
'simple ascii string longer than 255 characters truncated' => [
str_repeat('e',300),
str_repeat('e',232) . ' -truncated (300 chars)'
],
'smiley string longer than 255 characters truncated' => [
str_repeat('😃',300),
str_repeat('😃',232) . ' -truncated (300 chars)'
],
];
}
/**
* @dataProvider SetTrimProvider
* @return void
*/
public function testSetTrim($sName, $sResult){
$oOrganisation = MetaModel::NewObject(Organization::class);
$oOrganisation->SetTrim('name', $sName);
$this->assertEquals($sResult, $oOrganisation->Get('name'), 'SetTrim must limit string to 255 characters');
}
} }

69
tests/php-unit-tests/unitary-tests/core/EventIssueTest.php Normal file
View File

@@ -0,0 +1,69 @@
<?php
use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
class EventIssueTest extends ItopDataTestCase
{
/**
* Data provider for test EventIssue Creation
*
* @return array data
* @since 3.1.2 N°3448 - Framework field size check not correctly implemented for multi-bytes languages/strings
*/
public function OnInsertProvider()
{
return [
'all with ééé' => [
str_repeat('é', 120),
str_repeat('é', 120),
str_repeat('é', 120),
str_repeat('é', 120),
],
'all with 255 smiley' => [
str_repeat('😎', 255),
str_repeat('😎', 255),
str_repeat('😎', 255),
str_repeat('😎', 255),
],
'all with 255 characters, us-ascii only' => [
str_repeat('a', 255),
str_repeat('a', 255),
str_repeat('a', 255),
str_repeat('a', 255),
],
'all with 255 é' => [
str_repeat('é', 255),
str_repeat('é', 255),
str_repeat('é', 255),
str_repeat('é', 255),
],
];
}
/**
* EventIssue has a OnInsert override that uses mb_strlen, so we need to test this specific case
*
* @covers EventIssue::OnInsert
*
* @dataProvider OnInsertProvider
*
* @since 3.1.2 N°3448 - Framework field size check not correctly implemented for multi-bytes languages/strings
*/
public function testOnInsert(string $sIssue, string $sImpact, string $sPage, string $sMessage)
{
$oEventIssue = MetaModel::NewObject('EventIssue', [
'issue' => $sIssue,
'impact' => $sImpact,
'page' => $sPage,
'message' => $sMessage,
]);
try {
$oEventIssue->DBInsert();
}
catch (CoreException $e) {
$this->fail('we should be able to persist the object though it contains long values in its attributes');
}
$this->assertTrue(true);
}
}

5
webservices/webservices.class.inc.php
View File

@@ -298,9 +298,8 @@ abstract class WebServicesBase
if (is_object($oAttDef)) if (is_object($oAttDef))
{ {
$iMaxSize = $oAttDef->GetMaxSize(); $iMaxSize = $oAttDef->GetMaxSize();
if ($iMaxSize && (strlen($sValue) > $iMaxSize)) if ($iMaxSize && (mb_strlen($sValue) > $iMaxSize)) {
{ $sValue = mb_substr($sValue, 0, $iMaxSize);
$sValue = substr($sValue, 0, $iMaxSize);
} }
$oLog->Set($sAttCode, $sValue); $oLog->Set($sAttCode, $sValue);
} }