GridField escapes HTML content inappropriately

[mfendeksilverstripe]

Module version(s) affected

5.2 and 5.x-dev , but not 5.1

Description

CMS links are malformed. This looks like a regression related to CMS 5 upgrade (suspecting change of default behaviour).

• "silverstripe/recipe-cms": "5.1.x-dev" works as expected
• "silverstripe/recipe-cms": "~5.x-dev" does not work as expected

Looks like something was introduced after 5.1 that changes the behaviour.

Expected behaviour

Links are clickable (regular HTML links).

Actual behaviour

Malformed HTML (encoded).

How to reproduce

Add this GridField to a data object's edit form

$config = GridFieldConfig_RecordViewer::create();
$columns = $config->getComponentByType(GridFieldDataColumns::class);
$gridField = GridField::create('Members', 'Members', Member::get(), $config);
$summaryColumns = $columns->getDisplayFields($gridField);
$summaryColumns['Email'] = [
    'title' => 'Email',
    'callback' => function (Member $member): ?DBField {
        $render = sprintf('<a href="mailto:%s">%s</a>', $member->Email, $member->Email);
        return DBField::create_field('HTMLVarchar', $render);
    }
];
$columns->setDisplayFields($summaryColumns);

Possible Solution

I had a quick look at GridFieldDataColumns and DBHTMLVarchar but I couldn't find any obvious changes that would cause this.

I found a workaround for this (explicit cast):

$config = GridFieldConfig_RecordViewer::create();
$columns = $config->getComponentByType(GridFieldDataColumns::class);
$gridField = GridField::create('Members', 'Members', Member::get(), $config);
$columns->setFieldCasting([
    'Email' => 'HTMLVarchar->RAW',
]);
$summaryColumns = $columns->getDisplayFields($gridField);
$summaryColumns['Email'] = [
    'title' => 'Email',
    'callback' => function (Member $member): ?DBField {
        $render = sprintf('<a href="mailto:%s">%s</a>', $member->Email, $member->Email);
        return $render;
    }
];
$columns->setDisplayFields($summaryColumns);

Additional Context

Originally raised via tractorcow-farm/silverstripe-fluent#839 but confirmed that this is not related to the Fluent module.

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

OPTIONS

See #11191 (comment)

Acceptance criteria

• The offending PR is reverted.
• Open a follow up card - or repopen the original card - to discuss how to handle Nice method in CMS 6.

PRs

• FIX Revert "ENH Add Nice to DBField" #11222
• MNT Add behat test for HTML rendered in GridField silverstripe-admin#1737

[sunnysideup]

This looks similar to #11200.

silverstripe-framework/src/Forms/GridField/GridFieldDataColumns.php

Line 268 in 3d64eac

$value = $value->forTemplate();

Here is some relevant code (sorry images for speed):

[GuySartorelli]

@sunnysideup Please do not include images of code. It not going to significantly slow you down to type:

```php
// ctrl+c the code, ctrl+v the code
```

[sunnysideup]

Thank you for adding the link. I would recommend that you change the title of this ticket so it more accurately reflects the issue. I searched for GridField, summary_fields, escape (and similar words).

[GuySartorelli]

A git bisect shows that this PR introduced the bug.

Thankfully that wasn't included in the 5.2.0 release (and I have confirmed locally that a fresh installation of Silverstripe CMS 5.2.0 is not affected).

The root cause is GridField trying to be a little too helpful in sanitising values if there's a Nice() method in GridFieldDataColumns::castValue():

silverstripe-framework/src/Forms/GridField/GridFieldDataColumns.php

Lines 263 to 265 in 3d64eac

	if (method_exists($value, 'Nice')) {
	// If it has a "Nice" method, call that & make sure the result is safe
	$value = nl2br(Convert::raw2xml($value->Nice()) ?? '');

Not sure what the best course of action is yet, but I'm leaning towards just reverting that PR. We have 6 months to make the decision before this finds its way to a stable release, so no rush.

Some options are:

1. Revert the PR, and don't introduce that method until CMS 6 (we could do that PR right away for CMS 6, and make sure we update GridFieldDataColumns::castValue() while we're at it)
2. Add an additional condition to GridFieldDataColumns::castValue() so that DBHTMLText and DBHTMLVarchar aren't sanitised - though if someone has some custom DB field that should not be sanitised, they will still experience a regression
3. Remove the call to Nice() in GridFieldDataColumns::castValue() - though that may result in a new unexpected regression in some cases

@lekoala since you implemented the linked PR I'd like to know if you have any thoughts about the best course of action here?

[sunnysideup]

Great work @GuySartorelli - thank you for all your patience with me and your expertise in getting it sorted.

[lekoala]

@GuySartorelli oh i see, that is unexpected, and I was aware of that, but I had hoped the base Nice() method would actually make things simpler... not the opposite.
I think the "nice" method is supposed to be safe already, but at the same time, it seems from this line $value = nl2br(Convert::raw2xml($value->Nice()) ?? ''); that the framework isn't always think it's the case...

Personally, I don't like the "method_exists" check, so I would be in favor of removing it, but the easy course of action is simply reverting my PR. Maybe it would be simpler to introduce a new base method for "NiceAndSafe" value, and take that as the new default

[maxime-rainville]

We chatted about this in refinement. We concluded that it has too many moving parts to address specifically in CMS 5.

We'll revert the PR for now and start a discussion about what the ideal end state should be in CMS 6.

[sunnysideup]

I am still seeing issues. Is this correct?

[GuySartorelli]

The issue is still open because while we do have a plan, we haven't taken the time to enact the plan yet. If you are using the 5 branch (which is unstable, so I recommend not using it) you will still experience the bug this issue describes.

[GuySartorelli]

Reopened #11169 with new ACs for CMS 6 implementation of Nice()

[emteknetnz]

Linked PRs have been merged