Escaping in getColumnContent (GridFieldDataColumns)

[Jakob Kristoferitsch]

Hi,
GridFieldDataColumns->getColumn content is currently broken. After
getting the value for a column either from getDataFieldValue or from a
callback method, it is imidiately escaped (see
<https://github.com/silverstripe/sapphire/blob/master/forms/gridfield/GridFieldDataColumns.php#L127>
lines 129 and 133).

This breaks the calls to castValue() and formatValue() immediately
below. It is also the root cause of
<http://open.silverstripe.org/ticket/7581> and
<http://open.silverstripe.org/ticket/7590>.

I guess the idea behind this escaping (added as a part of the redesign
in
<https://github.com/silverstripe/sapphire/commit/70d5ffefdd12eb3e69f406cf95fa90a8abb27d6d>)
was to prevent XSS problems, but I think it is not feasible to just pass
all data through raw2xml, and especially not at this very early stage.

One good example are images, where the grid field should contain an img
tag for the thumbnail. In my opinion, it would be important that
$summary_fields can contain definitions like MyImage.CMSTHumbnail or
MyTextField.FirstSentence.

What are your thoughts about this? Where should the escaping happen? How
should the code know what not to escape?

Yours
Jakob

[Uncle Cheese]

Yes! I was just going to post the same question!

[UndefinedOffset]

Ya i kinda miss the betas for that, it was so cool to be able to just add a column Image.CMSThumbnail for example and it would render the image :(.

[dospuntocero]

i was bugging the core team the other day about this same matter. lets hope they found a solution soon :)

[Jakob Kristoferitsch]

Hi,
I think #7590 (Image thumbnails broken in gridfield) is a problem that
should be fixed in 3.0.1, but it has just been delayed to 3.0.2. The fix
is easy, the hard part are the following decisions:

1. At what point should escaping happen?
2. Which code paths should be allowed to bypass escaping?
3. Should it be possible to opt out of escaping? How?

I propose a simple solution, see
<https://github.com/jakr/sapphire/commits/trac7590>. The big downside of
that solution is that the only way to bypass escaping is to return an
Object with a forTemplate method. This breaks the example in
setFieldFormatting (creating an anchor tag) and makes it impossible to
use a callback function returning a string that should not be escaped.

There are ways to work around that downside, but in my opinion the
decisions outlined above have to be made first. I am looking forward to
input on that point.

I have to say that I am disappointed that this thread has not sparked
any discussion so far. It is two weeks old, and the problem is highly
visible to anyone who actually tries to use GridField.

TL;DR: The bullet points need to be decided. What's your opinion?

Yours
Jakob

P.S.: As a reminder, the original post is below.

[edch...@gmail.com]

I would have to agree Jakob, it is kind of one of those big not show stopper issues. Kinda disappointed to see it pushed to 3.0.2, seems like one of those things that could be slipped in before the final of 3.0.1. Especially since your fix Jakob seems to be working well and is a good solid fix.

> --
>
> You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
>
> To post to this group, send email to silverst...@googlegroups.com.
>
> To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
>
> For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.
>
>
>
>

[Sigurd Magnusson]

Sorry guys as Hamish points out on the ticket comment there wasn't time - hopefully you'll agree there's been a lot of activity for 3.0.1 - look at the length of the change log - and the work on the 3.0.x line will continue to be high for a while...

http://doc.silverstripe.org/framework/en/3.0/changelogs/rc/3.0.1-rc1

Cheers,

Sigurd.

[UndefinedOffset]

Ya makes complete sense to me, its just unfortunate. Oh well hopefully 3.0.2 will be just around the corner like 3.0.1 was :)

[Sigurd Magnusson]

Yup - and keep sending in those bug reports and/or pull requests ;)

Sig

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.

To view this discussion on the web visit https://groups.google.com/d/msg/silverstripe-dev/-/rCzgdU0XpysJ.

[Jakob Kristoferitsch]

Sigurd Magnusson wrote:
> Sorry guys as Hamish points out on the ticket comment there

> wasn't time [...]

I accept that it's not part of 3.0.1 (thanks to Hamish for the detailed
explanation), my next goal is to get it into 3.0(.2) as soon as possible.

To do that, I would really like to start a discussion about where
escaping should happen and what code paths should be allowed to bypass it.

Unfortunately, this seems to be a bug where everyone agrees that it is
important, but nobody wants to make any decisions ;-). My proposed fix
is publicly available and I have explained the advantages and
disadvantages that I see. I now would like to get feedback on my
proposal as well as alternative ones.

I think the problem of where to escape applies to a lot of different
places, which is why I want to have the discussion about it in public.
I am actually quite fond of my approach - it might not be the best fit
in this situation, but I think the general direction is correct: Escape
everything, except for objects that promise to do that themselves (by
providing forTemplate(), XML() or something like that).

The dogmatic approach would be to provide an object that wraps a string,
knows what needs to be escaped and provides the methods needed to escape
it for XML output (as well as DB and any other output). The framework
would then never directly output strings, instead relying on these
wrapper objects.

Yours
Jakob

[Andrew Short]

I would expect it to operate in the same way as the rest of the framework escaping - by default escape strings but not objects. Then allow the user to use the setFieldCasting method on GridFieldDataColumns to specify if a string should not be escaped:

$columns->setFieldCasting(array('UnescapedField' => 'HTMLText'));

Andrew Short.

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.

To post to this group, send email to silverstripe-dev@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-dev+unsubscribe@googlegroups.com.

[Jakob Kristoferitsch]

Andrew Short wrote:
> I would expect it to operate in the same way as the rest of the
> framework escaping - by default escape strings but not objects.

Ok, that goes in the direction I am implementing. I still think it is
best to call forTemplate on the Object at the end, since I read the
comment to getColumnContent to mean that the return value will be
directly inserted in the output:
> @return string HTML for the column. Return NULL to skip.

> Then allow the user to use the setFieldCasting method
> on GridFieldDataColumns to specify if a string
> should not be escaped:
>
> $columns->setFieldCasting(array('UnescapedField' => 'HTMLText'))

That is a nice way to handle this problem, but to enable that kind of
behavior I think that the call to castValue() should then be after the
call to formatValue (maybe even the last one). This would make it
possible to use the example from setFieldFormatting:
> "myFieldName" => '<a href=\"custom-admin/$ID\">$ID</a>'
In combination with setFieldCasting this would then output the link
without escaping it.

Should I rearrange these calls or is there a reason for the current
order (I guess there is...)?

Thanks for your input,
Jakob
Am 25.07.2012 15:31, schrieb

[Jakob Kristoferitsch]

Hi,
just to finish this thread: The issue has been fixed in 3.0.1

Yours,
Jakob