Final decision on the flash message rendering


(Alexander Schnitzler) #1

Discussion Topic

The rendering of flash messages has changed again and again from time to time and as I am quite frustrated about the latest changes (https://forge.typo3.org/issues/78477), I’d like to have a general discussion and decision on how we will handle flash messages in the future.

When it comes to rendering these massages there are two key elements:

  1. Security
  2. Separation of concerns

When it comes to security we need to know that we are mostly dealing with static text, set by developers or integrators in typoscript or xlf files. Yes, it’s also possible that user generated content will end up in flash messages but it’s more of an edge case.

Still, having sane defaults, like escaping all output do make sense, if they can be overridden in case I need to and of course on my own risk. With the latest changes we have sane but unconfigurable defaults.

Regarding the separation of concerns, we already have a quite good solution by letting renderers do the rendering instead of putting that logic into the view helper. The current approach would be a brilliant solution if it would be configurable. Unfortunately the core decides for me, which renderer should render my flash messages and the default changed from the Bootstrap Styling in 7.6 to the List-Styling in 8.7 (in frontend context).

I have the feeling that the latest changes are 1 step forward and 3 steps back. Imagine what a non-core-developer does when he/she realises these burdens.

  • People could write their own view helper and avoid using the renderers
  • People could write partials, using a custom rendering with using the as-argument of the view helper, avoid the renderers and (worst case) doing a format.raw on all flash message texts.
  • People XClass the renderers and disable the escaping of messages.

No matter what solution people will choose, they will come up with either of these or worse and therefore the latest changes to the rendering of flash messages may even have a quite bad impact on security.

While I understand the good intentions of the mentioned patch, I think it’s not a good and stable solution (yet). IMHO there are just 2 things missing that would make that patch perfect.

  1. Let the user register and configure the flash message renderers globally or give him/her the chance to override the renderer in single cases (e.g. using a new argument in the flash message view helper)
  2. Let the user decide whether the texts of the messages should be escaped, having sane defaults like we do have now.

What do you think about these ideas?

Organizational

Topic Initiator: Alexander Schnitzler
Topic Mentor: -


(Markus Klein) #2

I basically agree with the renderer arguments. I had a look recently into these as well and realized that I can’t adjust the renderers on my own.

So I think that making those configurable would be beneficial.
I haven’t thought about how/where this should be configured though.


(Jo Hasenau) #3

I second that, since we already had to do a kind of dirty workaround for the L10nmgr to still provide the link to the newly generated XML files for the exporter within the messages by using a marker that we can replace later on.


(Alexander Schnitzler) #4

What about introducing yet another global configuration array where people can register renderers?
Pseudo code:

$GLOBALS['TYPO3_CONF_VARS']['...']['Bootstrap'] => BootstrapRenderer::class.

In the view:

<f:flashmessages renderer="Bootstrap" />

I don’t see any better approach at this very moment to both let the user take control and keep the rendering out of the view helper.

Edit:
A little bit more sophisticated solution would be a one similar to the property mapping configuration, defining RendererResolvers with a priority and a method that evaluates whether the renderer should be used or not. I guess the current approach could easily be adopted. However I am not sure if such a solution makes sense or would be a bit over engineered then because I cannot come up with a case where I would let a renderer resolver decide when to use the bootstrap rendering or the list rendering.


(Markus Klein) #5

On top of all that we need to line out the conceptual difference between FlashMessages and CallOuts (at least for BE).


(Jigal Van Hemert) #6

For me security is the most important argument here. Flash messages are in my mind meant for short notifications about the result of an action where the rest of the screen doesn’t reflect the changes. I personally don’t see a need for markup in them.
On the other hand we all know how many developers handle their data. Some might just insert the name of a page or other data from the system in the flash message content.

Anything more complicated that a simple notification deserves a more sophisticated solution and that would be up to the extension author to write that.


(Alexander Schnitzler) #7

Hi Jigal,
I respect your view but I cannot understand such expressions. Did you never have the request of a client to make a single word bold in a sentence you use for a flash message? Client’s don’t ask for iframes, but they often want to emphasize words or use links.

Example:
We are sorry, but the login credentials you entered aren’t correct, do you maybe want to reset your password?

Example:
We are sorry, but your password must only contain letters, numbers and these special characters &, %, $.

That sounds a bit like:

Well, we know how badly some people code. They simply can’t see the simple way around here…

No matter if that’s your intention or not, I see a problem with user translated content. We don’t want them to use html because it’s not safe, but we do not offer a better solution either. btw: I actually don’t know any project that is offering a good solution here, not even big players like symfony. So, should we use bb code or markdown for simple markup in such sentences? I really don’t know but my clients are asking for these things and they can’t believe there are no solutions or that I have to write them from scratch.

Just to get it right. You think that security should be our biggest concern here and you willingly accept dozens of self-made flash message solutions out there that might contradict with your idea of security even more than allowing the user to decide whether to use html on their own?

It’s really not about offending you, it’s just that I don’t see a clear statement from you because you didn’t even refer to my possible solutions, so I have to guess that you are against it and would rather like to keep things completely safe and not configurable at all? Or did I get you wrong here?


(Jigal Van Hemert) #8

Hi Alexander,

Not that I can remember. Perhaps the issue is that we both use flash messages in a different way.
There are often action for which the result is hardly visible or can easily be missed. E.g. an item is deleted, a file has been uploaded. Or there is a simple interface that doesn’t allow for much interaction. E.g. a form is shown and after submitting the same form is shown again.
In those cases a flash message is an easy way to attract the attention of the ‘user’ and communicate the result.

In case more information must be communicated a result screen would be a better option. I can make lists, display images, use complex styling, create links, etcetera.

We have many places where the core provides tools to safely process data. Think about the quote/escape functions in the old database class, the markers option the CONTENT cObject (available since 4.3), … I still come across recent TS snippets that insert external data in a query. Hours are spent to fix SQL injections in code where devs simply didn’t use the core functions.
An option to disable escaping is just an invitation to set the option always because “one can use markup now”.

That’s a rather dramatic statement. On the one hand there is not much what anyone can do against someone creating a different solution. Your statement could also be interpreted that the core should implement everything to prevent extensions from providing “bad” solutions.
An alternative would be that someone creates an extension for extra-de-luxe flash messages and all the dozens of other extensions that need extravagant flashy messages can use that one.

I also don’t want to offend you, but we’re talking here about something to create simple notifications. It’s not a database layer, image processing or a templating engine. Registration of renderers is in my opinion a total overkill. But if others think it’s important to spend time on, fine.

You wanted to see opinions here, so I gave mine. Some people will see a use case, others won’t. In the end you can start a voting on a proposal based on the opinions and ideas from the replies. We don’t have to agree on everything :slight_smile:


(Alexander Schnitzler) #9

Jigal, I am fine with your opinion and I respect it. I just have the feeling that you (quite often) argue against something just to be against something. To be honest, I don’t buy it. Your statement, that you never had the need for a bold/italic word or a link in a flash message and that you can always use result screens in your projects.

But, whatever… It doesn’t matter. From what you write I understand that you wouldn’t have a problem if flash messages were secure by default but left the possibility to also output simple html if needed. That’s fine and something I can work with. :slightly_smiling_face:


(Riccardo De Contardi) #10

I don’t have a peculiar opinion on this topic.


(Jigal Van Hemert) #11

Alexander, you’re taking this discussion to a personal level that is neither necessary nor wanted in our community. Even stranger is the suggestion that you know more about the projects I’ve been working on than I know.
I’ll refrain from further comments in this discussion.


(Kevin Ditscheid) #12

I totally agree with jigal here. I don’t see the need to have a simple flash message rendering markup or more complicated stuff.


(Moritz) #13

I sometimes had the need to have markup inside flash messages, for example to make one word bold or to insert links into flash messages. Examples:

  1. you have a huge list view where you press a button to create a new record. You enter create action and create the record. After creation, you don’t want to show a confirmation page but rather take the user back to the list view context and you want to show a flash message to tell the user his action was successful. In that message, you might want to show a link to the user to do something else with the newly created record. As you have the user’s attention in the flash message, that would be the ideal place.

  2. you have an error message and you want to link an FAQ page to that specific error

Unless I am missing other core features to achieve those goals, flash message would be my first guess. Of course you can do those things using custom methods, by using modals, ajax, build your own viewhelper or whatever. But why so complicated? TYPO3 is already complicated enough :wink:

I agree that security is important. But we also need to ease development and usability. In the end, Developers simply need to know what they are doing. If they don’t, we won’t solve that problem by limiting functionalities. I don’t think anyone would blame TYPO3 for a too generous flash message API. People will always find ways to do it wrong - outputting unsanitized content can happen virtually everywhere.

To be on the safe side, we can escape things by default. But I’d suggest to at least include a switch in the flash message API or the viewhelper to allow raw output.

I also think that overriding/registering renderers would be very handy.

BTW, wouldn’t it be nice to have those renderers based on a fluid template instead of a PHP class? Maybe I am missing something, but I think it would be more straightforward for frontend developers.


(Simon Schaufelberger) #14

I made an issue some time ago about a specific flashmessage after an extension is installed to sponsor it with a paypal link: https://forge.typo3.org/issues/71247
This would be a need for me. Apart from that, I rarely see flash messages on other web applications with special markup but links are the number one.

A general suggestion: Would it for example be helpful to use markdown for markup and output it in plain text in cli mode and as html in frontend for example?

Even Google maps has flash messages with links :wink:

grafik

grafik


(Alexander Schnitzler) #15

I don’t and to say it in your own words: I’ll refrain from further accusations like these.

Don’t make me quote how often I said “No offense, but…”, “I respect your opinion…” or “in my opinion…”.

In your first response you have been very unclear. You said you are against something but didn’t give any reasons or arguments. That’s why I took your words, put a little drama in and played back the ball to you to get a real statement. Look, you can’t join a discussion that started with arguments and just refer to your feelings. Well, you can, but you don’t add anything of value.

And me, not buying your further explanation, is not a personal attack. I have the impression that you tend to be against things just to be against it. This is not an insult, it’s my impression from what I read from you, nothing more. So, please keep calm and don’t run around and accuse people of dragging conversations somewhere. If I call you names, it’s an insult, if I talk bad about you behind your back, it’s an attack. All the rest is an imagination in your head, maybe just like my impression of you. Probably totally wrong and 100% not personal.


(Helmut Hummel) #16

Before we’re continuing to discuss technical solutions, we should start with user stories and define from that what flash messages are and what they are not, leading to a conclusion where they can be used and where they shouldn’t

The mistake we made in the past is not clearly communicating where flash messages should be used leading to a situation that they were used for “everything”, even inside the core.

Here is what happened roughly:

1. Flash Message API was introduced

  • The Flash Message class had a render method, with hard coded markup and message and title not being encoded
  • The view helper had two modes to render messages, one called “div” and one “ul”. The first called the render method, the second generated a ul>li list with title and messages properly encoded. render mode “div” was default

1.1 Consequences of this API

  • Flash messages were used to render permanent “info boxes” in arbitrary places in the TYPO3 backend, sometimes with sophisticated additional HTML
  • We had to fix multiple XSS security issues in the core, which boiled down to user input being put into flash messages.
  • Default extension builder built code was vulnerable, as the view helper used vulnerable render mode div.
  • Basically every thrid party extension was vulenrable when using TYPO3 defaults

2. Mitigations to XSS issues

As one example exception messages were passed to flash messages, where it is totally upredictable whether the exception message contains some user input. We fixed this by calling htmlspecialchars() on the exception message in varous places, which obviously is tedious and error prone.

Additionally we started encoding (htmlspechialchars) everything passed to Flash Messages in core code, including labels. This lead to html tags being shown in some message, which we accepted as side effect. We prioritized security over minor functional/ optical issues.

3. Introduction of InfoBoxViewHelper

To overcome the issues introduced to secure TYPO3 and to remove most obvious abuse of flash messages, InfoBoxViewHelper was introduced to output so called callouts to show permanent information or warning messages to the user.

Unfortunately only a view helper was introduced, which makes it tedious to use/render such callouts from PHP code.

But at least we achieved a great cleanup, leaving flash messages
only for messages displayed for user actions

4. Improvement of FlashMessageViewHelper

The FlashMessageViewHelper was improved to give developers and integrators full control over the rendering of flash messages.
Instead of rendering fixed markup, it allows adding your own customized markup within Fluid templates.

This was the first step to allow moving rendering from PHP to the view layer, which in our case is Fluid, where the title and text was properly HTML encoded.

As a benefit, we could remove all HTML encoding from our exception messages, errors, … which was just wrong to do, as those exceptions weren’t just thrown for HTML rendering.

I explicitly accepted that some flash message output was broken (because e.g. some labels still contained HTML, which was then encoded), because we achieved a secure state with this change. I also accepted the limitation (no markup was now possible), with the idea to check for concrete use cases for flash messages containing more than plain text (one of them rendering a link) and implementing these cases without sacrificing security.

5. Further separating concerns in FlashMessages

Architectually it was a bad decision that message objects contained code for rendering.
This was tackled in another change.

The render method in FlashMessage was deprecated and the rendering of flash messages was unified to our central module template, which renders every backend module.
Also the different queue identifiers were unified to only have one.

What was still missing here, was deprecating further rendering related code from flash messages and flash message code, which we needed in some places for backend ui, where rendering was still done in PHP instead of Fluid.

All remaining code for rendering flash messages was changed to HTML encode title and messages.

6. Re-introduction of render code in FlashMessages

From my point of view, this change was indeed a step backwards. Instead of further removing rendering related code from our messaging API and moving further with putting all rendering into our view layer, flash messages got a rendering method again. This time everything (title, message) was encoded correctly by default, but still this re-introduced a technical dept to satisfy the needs of legacy code instead of refactoring such code.

7. Centralise code to render flash messages

As a compromise to have a central place to render flash messages renderer were introduced.
That new code is only useful to be used in our own legacy code, that still renders flash messages in PHP instead of rendering them in the view layer. It is code that mixes business logic with controller logic with view logic.

This means we introduced new API to support legacy code instead of refactoring legacy code.

It is fine, but it still is a techical dept that we should remove at some point.

Good thing is, that we deprecated all view related code from FlashMessage now and the only remaining view realated method that remained (again only to support legacy code), if in FlashMessageQueue, which still has a render method (which calls the new API).

And here we are with the current state.

Summary

Regarding what is complained here, which is FlashMessages can’t contain plain HTML, there was no back and forth. Since 5. flash messages are secure and HTML encoded. There is still work to do to further refactor legacy code and remove all rendering code from our messaging API and there is some work to do to evaluate what content flash messages need to support except plain text and implement a solution for that.

To get to the point of this discussion.

When it comes to security we need to know that we are mostly dealing with static text, set by developers or integrators in typoscript or xlf files. Yes, it’s also possible that user generated content will end up in flash messages but it’s more of an edge case.

That is a wrong assumption. It wasn’t an edge case. Look at where we had htmlspecialchars() before, just to ensure to prevent XSS.

Security was the main reason that drove these changes.

With the latest changes we have sane but unconfigurable defaults

That is not true. If your plugin renders flash messages in the view layer (Fluid), you have full control to introduce your own XSS, if you dare so.

People could write partials,

Please explain what is wrong with that. You have full control over the markup. End of story.

using a custom rendering with using the as-argument of the view helper, avoid the renderers and (worst case) doing a format.raw on all flash message texts.

You want full control over the markup, which you have, want to allow HTML in flash messages which you can, argue that security is not a concern, as flash messages contain mostly static strings in developer’s control, but then complain that it might not be secure to not encode the messages in custom markup. It does not matter which layer renders flash messages. They can be either securely encoded or not.

There is something where you don’t have full control over the markup and that is when writing a backend module. Flash message of your backend module will always have the same markup and the same feature set as core modules. Flash messages are ensured to look and work exactly the same as for core modules. To me this sounds like a consistency feature.

Let the user register and configure the flash message renderers globally

I have a significant concern with this suggetion. Assuming it would be possible to override a render type:

  1. Only one override would be possible. So it would be pretty much the same as XCLASS the specific renderer
  2. Overriding the renderer and not encoding messages any more (what is what I understood a desired feature), would render (pun intended) the whole effort of securing the backend flash messages against XSS useless. One extension installed could make the backend vulnerable.

give him/her the chance to override the renderer in single cases

That is possible, isn’t it? Not in backend sure, but when using the view helper you have full control.

Let the user decide whether the texts of the messages should be escaped

To achieve what exactly?

I really want to understand the use case. My suggetion therefore is: Let’s find user stories of things that are required before starting a discussion about technical implementation.

User stories for backend usage

We should look at backend only, because frontend plugins are free to do anything with the current flash message view helper implementation. (If you think I’m wrong here, help me understand what is missing, or why putting HTML rendering into PHP is superiour over having that in Fluid temlpates using the view helper)

As an editor I want visual feedback whether a triggered action was successful or has failed

That should be the main user story for flash messages. It would mean flash messages should only be used in context of user initiated actions.

Currently we use flash messages for a different case as well.

As a site administrator I want to be informed about errors or deprecations when working in the system.

Currently the error handler emits flash messages when an error occurs. Such errors may not be connected to a certain action, but can be caused by a certain system state. We imho should discuss whether flash messages are a good channel to communicate state errors for administrators, mainly but not only because also editors might see such errors without being able to cope with them.

As an editor when I get visual feedback in a flash message, I sometime want to be able to have access to actionables related to this message.

Example: “Item FOO was deleted.” -> quick action: "Undo deletion"
Example: “Action failed.” -> quick action: “Try again”

This would IMHO cover everything we need, but I’m open to hear from your use case/ evaluate your user story.


(Helmut Hummel) #17

Ok, I have started writing my comment before some other comments popped up. Two of them acknowledge the user story to have actionables (links) in flash messages, which I pretty much agree on.

Another one was having the possibility to highlight certain words in a message to make it better for reading.

This sounds legit for me as well. Both could be done without sacrificing security and consistency (with allowing full HTML we would)

Anything else besides that you think is required to improve flash messages (action notifications) for users?


(Helmut Hummel) #18

My main point is: Anything we add to flash messages, should semantically extend the domain. If we know the semantics of what is rendered, we can ensure security and even render it for contexts other than HTML (XML, Json, command line, HTTP transport to other systems, persistence, …)

If we just allow HTML markup in message texts, we allow for having unknown, potentially harmful blob of data without having a chance to handle that properly.


(Alexander Schnitzler) #19

You are right.
Let’s close this topic.


(Helmut Hummel) #20

uhm, anything wrong with what I wrote? I didn’t mean to shut down the discussion but find out what we really need before discussing implementations