From 87582a021b4626baf5c77a41d50f4372de0a2256 Mon Sep 17 00:00:00 2001 From: Anne-Catherine <57360138+accognet@users.noreply.github.com> Date: Mon, 15 Jan 2024 15:49:19 +0100 Subject: [PATCH 1/3] =?UTF-8?q?N=C2=B06993=20-=20Error=20message=20on=20bu?= =?UTF-8?q?lk=20transition=20on=20object=20containing=20a=20null=20blob=20?= =?UTF-8?q?(#596)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/cmdbabstract.class.inc.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/application/cmdbabstract.class.inc.php b/application/cmdbabstract.class.inc.php index ba60776c6..f712a249d 100644 --- a/application/cmdbabstract.class.inc.php +++ b/application/cmdbabstract.class.inc.php @@ -3218,13 +3218,13 @@ EOF if ($oAttDef->GetEditClass() == 'Document') { $oDocument = $this->Get($sAttCode); - if (!$oDocument->IsEmpty()) + if (is_object($oDocument) && !$oDocument->IsEmpty()) { $sDisplayValue = $this->GetAsHTML($sAttCode); $sDisplayValue .= "
".Dict::Format('UI:OpenDocumentInNewWindow_', - $oDocument->GetDisplayLink(get_class($this), $this->GetKey(), $sAttCode)).", \n"; + $oDocument->GetDisplayLink(get_class($this), $this->GetKey(), $sAttCode)).", \n"; $sDisplayValue .= "
".Dict::Format('UI:DownloadDocument_', - $oDocument->GetDownloadLink(get_class($this), $this->GetKey(), $sAttCode)).", \n"; + $oDocument->GetDownloadLink(get_class($this), $this->GetKey(), $sAttCode)).", \n"; } else { From cff50f8732beb11b35c51cd8f6b105d697bbd449 Mon Sep 17 00:00:00 2001 From: Anne-Catherine <57360138+accognet@users.noreply.github.com> Date: Mon, 15 Jan 2024 16:10:23 +0100 Subject: [PATCH 2/3] =?UTF-8?q?N=C2=B03448=20-=20Framework=20field=20size?= =?UTF-8?q?=20check=20not=20correctly=20implemented=20for=20multibytes=20l?= =?UTF-8?q?anguages/strings=20(#528)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/dbobject.class.php | 32 +++--- core/displayablegraph.class.inc.php | 4 +- core/event.class.inc.php | 41 +++----- synchro/synchrodatasource.class.inc.php | 29 ++---- .../unitary-tests/core/DBObjectTest.php | 97 ++++++++++++++++++- .../unitary-tests/core/EventIssueTest.php | 69 +++++++++++++ webservices/webservices.class.inc.php | 5 +- 7 files changed, 207 insertions(+), 70 deletions(-) create mode 100644 tests/php-unit-tests/unitary-tests/core/EventIssueTest.php diff --git a/core/dbobject.class.php b/core/dbobject.class.php index 3c9042048..fd4da584b 100644 --- a/core/dbobject.class.php +++ b/core/dbobject.class.php @@ -756,9 +756,10 @@ abstract class DBObject implements iDisplay { $oAttDef = MetaModel::GetAttributeDef(get_class($this), $sAttCode); $iMaxSize = $oAttDef->GetMaxSize(); - if ($iMaxSize && (strlen($sValue) > $iMaxSize)) - { - $sValue = substr($sValue, 0, $iMaxSize); + $sLength = mb_strlen($sValue); + if ($iMaxSize && ($sLength > $iMaxSize)) { + $sMessage = " -truncated ($sLength chars)"; + $sValue = mb_substr($sValue, 0, $iMaxSize - mb_strlen($sMessage)).$sMessage; } $this->Set($sAttCode, $sValue); } @@ -2110,33 +2111,26 @@ abstract class DBObject implements iDisplay return true; } - if ($toCheck instanceof ormSet) - { + if ($toCheck instanceof ormSet) { return true; } return "Bad type"; - } - elseif ($oAtt->IsScalar()) - { + } elseif ($oAtt->IsScalar()) { + $aValues = $oAtt->GetAllowedValues($this->ToArgsForQuery()); - if (is_array($aValues) && (count($aValues) > 0)) - { - if (!array_key_exists($toCheck, $aValues)) - { + if (is_array($aValues) && (count($aValues) > 0)) { + if (!array_key_exists($toCheck, $aValues)) { return "Value not allowed [$toCheck]"; } } - if (!is_null($iMaxSize = $oAtt->GetMaxSize())) - { - $iLen = strlen($toCheck); - if ($iLen > $iMaxSize) - { + if (!is_null($iMaxSize = $oAtt->GetMaxSize())) { + $iLen = mb_strlen($toCheck); + if ($iLen > $iMaxSize) { return "String too long (found $iLen, limited to $iMaxSize)"; } } - if (!$oAtt->CheckFormat($toCheck)) - { + if (!$oAtt->CheckFormat($toCheck)) { return "Wrong format [$toCheck]"; } } diff --git a/core/displayablegraph.class.inc.php b/core/displayablegraph.class.inc.php index f1fdd6e76..1291bebee 100644 --- a/core/displayablegraph.class.inc.php +++ b/core/displayablegraph.class.inc.php @@ -62,7 +62,7 @@ class DisplayableNode extends GraphNode 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() @@ -491,7 +491,7 @@ class DisplayableNode extends GraphNode if ($bNoLabel) { // 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.'"'; } else diff --git a/core/event.class.inc.php b/core/event.class.inc.php index da6f0522a..35f41c91f 100644 --- a/core/event.class.inc.php +++ b/core/event.class.inc.php @@ -242,44 +242,33 @@ class EventIssue extends Event { if (is_string($sValue)) { - if (strlen($sValue) < 256) - { + if (mb_strlen($sValue) < 256) { $aPost[$sKey] = $sValue; + } else { + $aPost[$sKey] = "!long string: ".mb_strlen($sValue)." chars"; } - else - { - $aPost[$sKey] = "!long string: ".strlen($sValue). " chars"; - } - } - else - { + } else { // 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); - } - else - { + } else { $this->Set('arguments_post', array()); } - - $sLength = strlen($this->Get('issue')); - if ($sLength > 255) - { - $this->Set('issue', substr($this->Get('issue'), 0, 200)." -truncated ($sLength chars)"); + $sLength = mb_strlen($this->Get('issue')); + if ($sLength > 255) { + $this->Set('issue', mb_substr($this->Get('issue'), 0, 210)." -truncated ($sLength chars)"); } - $sLength = strlen($this->Get('impact')); - if ($sLength > 255) - { - $this->Set('impact', substr($this->Get('impact'), 0, 200)." -truncated ($sLength chars)"); + $sLength = mb_strlen($this->Get('impact')); + if ($sLength > 255) { + $this->Set('impact', mb_substr($this->Get('impact'), 0, 210)." -truncated ($sLength chars)"); } - $sLength = strlen($this->Get('page')); - if ($sLength > 255) - { - $this->Set('page', substr($this->Get('page'), 0, 200)." -truncated ($sLength chars)"); + $sLength = mb_strlen($this->Get('page')); + if ($sLength > 255) { + $this->Set('page', mb_substr($this->Get('page'), 0, 210)." -truncated ($sLength chars)"); } } } diff --git a/synchro/synchrodatasource.class.inc.php b/synchro/synchrodatasource.class.inc.php index 5f4d559f9..5c887e4b0 100644 --- a/synchro/synchrodatasource.class.inc.php +++ b/synchro/synchrodatasource.class.inc.php @@ -1930,17 +1930,13 @@ class SynchroLog extends DBObject $oAttDef = MetaModel::GetAttributeDef(get_class($this), 'traces'); $iMaxSize = $oAttDef->GetMaxSize(); - if (strlen($sPrevTrace) > 0) - { + if (strlen($sPrevTrace) > 0) { $sTrace = $sPrevTrace."\n".implode("\n", $this->m_aTraces); - } - else - { + } else { $sTrace = implode("\n", $this->m_aTraces); } - if (strlen($sTrace) >= $iMaxSize) - { - $sTrace = substr($sTrace, 0, $iMaxSize - 255)."...\nTruncated (size: ".strlen($sTrace).')'; + if (mb_strlen($sTrace) >= $iMaxSize) { + $sTrace = mb_substr($sTrace, 0, $iMaxSize - 40)."...\nTruncated (size: ".mb_strlen($sTrace).')'; } $this->Set('traces', $sTrace); @@ -2145,9 +2141,8 @@ class SynchroReplica extends DBObject implements iDisplay break; } - if (strlen($sWarningMessage) > $MAX_WARNING_LENGTH) - { - $sWarningMessage = substr($sWarningMessage, 0, $MAX_WARNING_LENGTH - 3).'...'; + if (mb_strlen($sWarningMessage) > $MAX_WARNING_LENGTH) { + $sWarningMessage = mb_substr($sWarningMessage, 0, $MAX_WARNING_LENGTH - 3).'...'; } $this->Set('status_last_warning', $sWarningMessage); @@ -2187,17 +2182,13 @@ class SynchroReplica extends DBObject implements iDisplay public function SetLastError($sMessage, $oException = null) { - if ($oException) - { + if ($oException) { $sText = $sMessage.$oException->getMessage(); - } - else - { + } else { $sText = $sMessage; } - if (strlen($sText) > 255) - { - $sText = substr($sText, 0, 200).'...('.strlen($sText).' chars)...'; + if (mb_strlen($sText) > 255) { + $sText = mb_substr($sText, 0, 200).'...('.mb_strlen($sText).' chars)...'; } $this->Set('status_last_error', $sText); } diff --git a/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php b/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php index 5545c5cef..608297a5a 100644 --- a/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php +++ b/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php @@ -32,6 +32,7 @@ use Organization; use Person; use Team; use User; +use UserRequest; use UserRights; use utils; @@ -47,6 +48,7 @@ class DBObjectTest extends ItopDataTestCase // Counts public $aReloadCount = []; + protected function setUp(): void { parent::setUp(); @@ -1199,7 +1201,7 @@ class DBObjectTest extends ItopDataTestCase { return $this->aReloadCount[$sClass][$sKey] ?? 0; } - + /** * @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; 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'); + } } diff --git a/tests/php-unit-tests/unitary-tests/core/EventIssueTest.php b/tests/php-unit-tests/unitary-tests/core/EventIssueTest.php new file mode 100644 index 000000000..62faa35a4 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/core/EventIssueTest.php @@ -0,0 +1,69 @@ + [ + 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); + } +} \ No newline at end of file diff --git a/webservices/webservices.class.inc.php b/webservices/webservices.class.inc.php index f0bbc71e3..6a48646be 100644 --- a/webservices/webservices.class.inc.php +++ b/webservices/webservices.class.inc.php @@ -298,9 +298,8 @@ abstract class WebServicesBase if (is_object($oAttDef)) { $iMaxSize = $oAttDef->GetMaxSize(); - if ($iMaxSize && (strlen($sValue) > $iMaxSize)) - { - $sValue = substr($sValue, 0, $iMaxSize); + if ($iMaxSize && (mb_strlen($sValue) > $iMaxSize)) { + $sValue = mb_substr($sValue, 0, $iMaxSize); } $oLog->Set($sAttCode, $sValue); } From 3f3b0cbe55dc5e0dec339f54be9115cf0a961905 Mon Sep 17 00:00:00 2001 From: Anne-Cath Date: Mon, 15 Jan 2024 16:25:10 +0100 Subject: [PATCH 3/3] Fix merge support/2.7 to support/3.0 --- application/cmdbabstract.class.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/application/cmdbabstract.class.inc.php b/application/cmdbabstract.class.inc.php index 5756770cf..85e872447 100644 --- a/application/cmdbabstract.class.inc.php +++ b/application/cmdbabstract.class.inc.php @@ -3677,7 +3677,7 @@ HTML; if ($oAttDef->GetEditClass() == 'Document') { /** @var \ormDocument $oDocument */ $oDocument = $this->Get($sAttCode); - if (is_object($oDocument) && !$oDocument->IsEmpty()) + if (is_object($oDocument) && !$oDocument->IsEmpty()) { $sFieldAsHtml = $this->GetAsHTML($sAttCode); $sDisplayLabel = Dict::S('UI:OpenDocumentInNewWindow_');