[MAGNOLIA-7625] Improve DefaultMessageBundlesLoader class Created: 05/Sep/19  Updated: 18/Aug/21

Status: Accepted
Project: Magnolia
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Neutral
Reporter: John Shankle Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: maintenance
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Problem/Incident
causality
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:

 Description   

Hi,

 

We recently ran into an issue when loading properties files from our services module. We have a properties file that has some legacy values that contain html markup. The markup has class attributes, but the DefaultMessageBundlesLoader class uses JSOUP to scrub any markup in values based on the internal Whitelist. Example of markup value is:

fund.tools.productComparison.ytdNav = YTD  <span class="pull-right">(NAV)</span>

 

We wanted to change the whitelisting behavior, but how this class is defined it has a few problems:

 

  1. It is a concrete class defined as singleton w/ no interface, so to override the default behavior you have to extend the concrete class
  2. The getMessages method is package protected, so if you do extend this class it does you no good, because your overridden method will not be used
  3. Since everything in the class is private, you basically have to cp/paste all the code and still extend the concrete class

 

What would be nice is if the class implements interface and allows you to either have the whitelist configurable from subclasses and/or allow JSOUP behavior turned off via jcr

 



 Comments   
Comment by Mikaël Geljić [ 17/Sep/19 ]

Hi John,

Are you migrating from an earlier Magnolia version where this was not an issue? Is this a translation label used in a template directly via i18n[•••]?

One could argue content shouldn't be mixed with presentation but regardless, Jsoup's basic whitelist should preserve the span here—only the class attribute goes missing, right?
Any chance we can pull that style class on the template script side (or model class if any)?

Alternatives could be reading that property in plain Java ways, or indeed as you suggest making the whitelist configurable, preferably... in magnolia properties
I don't suppose we would open up that class for plain replacement.

Hope this helps,
Mika

Comment by John Shankle [ 17/Sep/19 ]

Hi Mika,

 

No, this is not from a migration of earlier Magnolia. We are migrating off a legacy application into Magnolia. The value is read into a ftl via ${i18n['some.key.value']}

Agree that you shouldn't have markup in the properties file, but given how this app works, there isn't an alternative currently. The text from the properties is static text that is controlled via the app and not up for editorial control.

Here is an example of output in ftl:

 

<li>${i18n['keyfacts.id']}<i class="icon-info-sign" rel="tooltip"<li>${i18n['keyfacts.id']}<i class="icon-info-sign" rel="tooltip" data-original-title="${i18n['keyfacts.id.tooltip']}"></i> <span class="pull-right"> ${id!}</span></li>

 

Examples of the above occur throughout the ftls we use, so reading into java wouldn't be realistic option.

Yes, having the whitelist configurable would be very helpful.

Comment by Mikaël Geljić [ 17/Sep/19 ]

Thank you for the fast feedback, I see. Any chance you can link that to a SUPPORT ticket?

Meanwhile, I'd recommend to relax Jsoup only as a temporary measure, and to still plan for refactoring templates and translation files (which might be achievable sooner than the next Magnolia version you're on).
Consider things like template properties, model classes or custom templating functions to do the matching from a "key" to its desired alignment. And/or to combine this with splitting existing keys with a bit of Groovy perhaps.

Comment by John Shankle [ 17/Sep/19 ]

Ok. Brian mentioned to create an enhancement request. So I should now open this up as support and ref this ticket?

 

 

 

Not sure I understand the comment about this:

Consider things like template properties, model classes or custom templating functions to do the matching from a "key" to its desired alignment. And/or to combine this with splitting existing keys with a bit of Groovy perhaps.

Tech what we are doing as far as looking up from properties file w/ i18n is no diff than any web app where it can get some value from props file (while preserving i18n), but I think the combined effort where Magnolia puts all properties files and jcr into one place introduces the security risk as you can't really guarantee what is from JCR.

 

When you say relax Jsoup, just wanted to check what you mean? What we did was to create our own whitespace, which take the default basic and then says let these elements have class attributes. So it is still doing the secure route to only allow certain elements but things like span can now have class attribute. 

 

That would be very much ideal for Magnolia to allow us to specify what kind of behavior we want in our whitespace object, w/out having to extend translation service and defaultmessage bundle.

 

 

Comment by Mikaël Geljić [ 18/Sep/19 ]

Going through support ensures tickets are scored and ranked for maintenance & improvements. Support naturally prevails over community tickets.

I agree your custom whitelist looks totally fine; regarding guaranteeing what is from JCR, I'd be equally happy to leave sanity and access to resources app/workspace to the project side. On the product side, only a handful of UI elements are subject to formatting in properties files (which is where this sanitation came from originally), we could consider eliminating those as well. Then it falls back in line with typical webapp setup.

Regarding templating tricks, I'm mostly suggesting options to decouple content from presentation in those properties files, should you choose this path anyway—be it for best practices or timely constraints, by the time an improvement lands in the product.

Comment by Richard Gange [ 20/Sep/19 ]

Maybe another useful tip, the MessagesWrapper can take parameters. See MessagesWrapper.

Generated at Mon Feb 12 04:25:25 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.