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:
- Only one override would be possible. So it would be pretty much the same as XCLASS the specific renderer
- 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.
