Can we have some unit tests added for this? If they could include attempts at abusing this new logic to somehow perform some sort of malicious action (if you can think of any) that would be great. The proposed fix could open the door for that type of potential attack vector.

@LukeTowers the only specific opening this would provide would be if one were to be accepting any form of user input into a content block - they could put in the replacement signature (ie. .=VAR=[some variable]=.) into their input, and as long as the variable is actually passed into the rendering pipeline, it would be replaced.

I figured the likelihood that people would be accepting input into content blocks was extremely low, and at best, they would simply get access to a variable that's already made available to the page anyway (and can only be provided by the backend, so it would need to be a privileged user).

I will set up a test case or two for it though.

@bennothommo this is a horrible idea, but we could prevent it from happening by pre-replacing the replace target with something else and then changing it back afterwards.

So:

• replace .=VAR=. with .=NOTVAR=.
• replace {} with .=VAR=.
• markdown parse
• replace .=VAR=. with {}
• replace .=NOTVAR=. with .=VAR=.

I could see some potential attack vectors around the use of template fields and safe mode, so we should at least cover this case to avoid introducing any vectors unnecessarily.

@LukeTowers slightly less horrible, but the other idea is we could add that sequence to the escaping functionality within Twig, so if it's fed through a Twig variable, it's escaped, but should be fine if it's directly in the template.

Maybe a stupid question, but why can't we expand the content variables to their values BEFORE parsing the markup with Markdown::parse ?

@mjauvin because the content blocks are cached on render. Putting in the variables before rendering would lock that set of variables in, and you'd lose the ability to re-use the content block with different values without clearing and re-rendering each time.

Right, that couldn't be so simple... It was worth a shot!

@bennothommo what would that look like? Cause it sounds pretty horrible 😂

Also is there any flexibility to just changing the markdown parser to not cause us this issue in the first place?

@LukeTowers it basically requires us to completely replace one of the core Commonmark extensions with our own copy of the same extension (plus any overrides to make the links work with the variables).

While I'm not adverse to that, it does mean that we then have to ensure that we're monitoring the Commonmark library for changes and implementing any fixes into our own.

Argh, how annoying. Let's give the twig escaping a try then I guess? Would that actually stop the theoretical attacks I was proposing?

It should. I'll have to circle around to this in a couple of weeks though unfortunately. Currently knee-deep in a very time-critical project.

@LukeTowers just circling back to this - I'm wondering if we even need Twig escaping? The particular issue here is with content blocks that are Markdown-formatted. Content blocks aren't put through the Twig parser at all, as far as I'm aware.

@LukeTowers just circling back to this - I'm wondering if we even need Twig escaping? The particular issue here is with content blocks that are Markdown-formatted. Content blocks aren't put through the Twig parser at all, as far as I'm aware.

@bennothommo didn't you say that they are?

@mjauvin because the content blocks are cached on render. Putting in the variables before rendering would lock that set of variables in, and you'd lose the ability to re-use the content block with different values without clearing and re-rendering each time.

@bennothommo what's the status on this?

I really don't like this fix, TBH. I'm more inclined to can it and think of something else.