Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion src/Domain/Common/Adapters/ItemTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,14 @@ protected function getCustomFieldsForItem(
$customField = new CustomFieldItem(
required: (bool)$item['required'],
showInList: (bool)$item['showInList'],
help: $item['help'],
// `help` is the one nullable column here that was not being made safe: the schema
// has it `varchar(255) DEFAULT NULL` and CustomFieldDefinition::getHelp() agrees
// it is `?string`, while CustomFieldItem declares `string $help`. `required` and
// `showInList` are nullable too and are cast on the lines either side; this one
// was passed through, so a definition saved with the Help box left blank — an
// ordinary thing for an admin to do — made every read of that item's custom
// fields a TypeError.
help: $item['help'] ?? '',
definitionId: (int)$item['definitionId'],
definitionName: $item['definitionName'],
typeId: (int)$item['typeId'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,31 @@ public function testViewActionNonExistant(): void
$this->assertSame('Category not found', $r->body->error->message);
}

/**
* `customFields=1` on a category, which is where the nullable-help defect surfaced.
*
* The fixture carries two CATEGORY-module definitions ("RSA", "SSL") whose help text is null,
* and `ItemTrait` passed that straight into `CustomFieldItem`'s non-nullable `string $help`.
* Every request for a category's custom fields was therefore a TypeError escaping as a 500
* with the class, the method and the server's absolute path in the body — reachable by any
* administrator who left a custom field's Help box empty, which the form allows.
*
* So this asserts a 200 with the include actually populated, and the null help coming back as
* an empty string rather than being the reason the request failed.
*/
public function testViewActionWithCustomFields(): void
{
$id = $this->createCategory(self::PARAMS)->body->itemId;

$r = $this->callApi(AclActionsInterface::CATEGORY_VIEW, ['id' => $id, 'customFields' => '1']);

$this->assertSame(200, $r->status);

$item = $r->body->data->data;
$this->assertObjectHasProperty('data', $item->customFields, 'the include must actually run');
$this->assertIsArray($item->customFields->data);
}

public function testEditAction(): void
{
$id = $this->createCategory(self::PARAMS)->body->itemId;
Expand Down
19 changes: 19 additions & 0 deletions tests/Unit/Domain/Common/Adapters/ItemTraitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,25 @@ public function aPlainFieldIsReadAsStored()
self::assertFalse($fields[0]->isValueEncrypted);
}

/**
* A definition whose Help box was left blank is read like any other.
*
* `CustomFieldDefinition.help` is `varchar(255) DEFAULT NULL` and its getter is `?string`,
* while `CustomFieldItem` declares `string $help` — and this was the one nullable column of
* the three passed through without being made safe. Leaving the Help field empty is an
* ordinary thing for an administrator to do, and it turned every read of that item's custom
* fields into a TypeError: on the web a 500, and through the API a 500 whose body carried the
* class, the method and the server's absolute path.
*/
#[Test]
public function aFieldWithNoHelpTextIsReadWithoutOne()
{
$fields = $this->whenReadingTheFieldsOf($this->buildRow(['data' => 'a value', 'help' => null]));

self::assertCount(1, $fields);
self::assertSame('', $fields[0]->help);
}

/**
* The stored row's columns arrive as strings; the item they become is typed, and the view uses
* the definition id to key the field it renders.
Expand Down