Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tokens in File Include Insert Tag don't get filled #330

Closed
Shoekrates opened this issue May 7, 2024 · 13 comments
Closed

Tokens in File Include Insert Tag don't get filled #330

Shoekrates opened this issue May 7, 2024 · 13 comments

Comments

@Shoekrates
Copy link

Hi,
current setup: NC 2, Contao 4.13.43
We use a file include insert tag {{file::filename.php}} to use a more complex html layout with conditional sections dependant on user input. These conditional statements ({if form_field=="valueA"}{endif}) would get stripped out by tinyMCE on save, so we did it via file include.
With NC1 it worked like a charm and tokens got filled, after upgrading to NC2 (Pro) it doesn't work anymore.
If I put the HTML directly into the HTML field in mail settings tokens get filled but then our conditions are stripped out.
Is this a bug in NC2 or desired behaviour? And how can I add conditional output to HTML templates?
Cheers, Christian

@Shoekrates Shoekrates changed the title Tokens in File Include Inser Tag don't get filled Tokens in File Include Insert Tag don't get filled May 7, 2024
@Toflar
Copy link
Member

Toflar commented May 7, 2024

Sorry but I cannot follow your issue. Can you post some concrete examples of what you did and what you expect? Maybe with screenshots? Why would tinyMCE strip any {if statements?

@Shoekrates
Copy link
Author

No problem. I guess I wasn't precise with „get stripped out“.
They aren't stripped out but TinyMCE cuts the if statements out and puts them above the table into a paragraph:

This code

<table>
	<tr colspan="2">
		<td>Headline</td>
	</tr>
{if form_field1=="valueA"}
<tr>
	<td>Label Field A1</td>
	<td>Value Field A1</td>
</tr>
<tr>
	<td>Label Field A2</td>
	<td>Value Field A2</td>
</tr>
{endif}
{if form_field1=="valueB"}
<tr>
	<td>Label Field B1</td>
	<td>Value Field B1</td>
</tr>
<tr>
	<td>Label Field B2</td>
	<td>Value Field B2</td>
</tr>
{endif}
</table>

leads to:

<p>{if form_field1=="valueA"}{endif}{if form_field1=="valueB"}{endif}</p>
<table>
	<tr colspan="2">
		<td>Headline</td>
	</tr>

<tr>
	<td>Label Field A1</td>
	<td>Value Field A1</td>
</tr>
<tr>
	<td>Label Field A2</td>
	<td>Value Field A2</td>
</tr>

<tr>
	<td>Label Field B1</td>
	<td>Value Field B1</td>
</tr>
<tr>
	<td>Label Field B2</td>
	<td>Value Field B2</td>
</tr>

</table>

@Toflar
Copy link
Member

Toflar commented May 7, 2024

Okay, that's kind of expected because tinyMCE tries to generate valid HTML, right. But that's always been the case and nothing we can do about that. So what exactly changed in NC2?

@Shoekrates
Copy link
Author

To avoid this tinyMCE behaviour we added a file include insert tag to the HTML mail text like:
{{file::filename.php}}
In this file we could build the markup with conditions.
But with NC2 the tokens in the included file don't get processed. The token names remain unprocessed in the mail body like ##form_field1##.
This worked in NC1.

So, if this won't work anymore what do you suggest to add conditions to html mail templates?
Would it be a prefilled custom mail_default.html5?
Or maybe something with custom tokens? Haven't experimented with that yet but if mails are long with many fields I guess this gets a little crowdy and confusing.

@Toflar
Copy link
Member

Toflar commented May 7, 2024

I've never used the {{file::*}} insert tag in 17 years of Contao that's why I'm probably having a hard time finding out what you are trying to do 😉 I might need some more time than usual to understand - bear with me, please 😊
So what did your filename.php do? Return tokens again? I mean, your filename.php contains

<table>
	<tr colspan="2">
		<td>Headline</td>
	</tr>
{if form_field1=="valueA"}
<tr>
	<td>Label Field A1</td>
	<td>Value Field A1</td>
</tr>
<tr>
	<td>Label Field A2</td>
	<td>Value Field A2</td>
</tr>
{endif}
{if form_field1=="valueB"}
<tr>
	<td>Label Field B1</td>
	<td>Value Field B1</td>
</tr>
<tr>
	<td>Label Field B2</td>
	<td>Value Field B2</td>
</tr>
{endif}
</table>

this?

@fritzmg
Copy link
Sponsor Collaborator

fritzmg commented May 7, 2024

I think the issue is that NC 2.0 does not replace insert tags and tokens recursively anymore.

@Shoekrates
Copy link
Author

Shoekrates commented May 7, 2024

@Toflar That's right. We just copied the markup over to an external file to avoid TinyMCE.

<table>
	<tr colspan="2">
		<td>Headline</td>
	</tr>
{if form_field1=="valueA"}
<tr>
	<td>Label Field A1</td>
	<td>##form_fielda1##</td>
</tr>
<tr>
	<td>Label Field A2</td>
	<td>##form_fielda2##</td>
</tr>
{endif}
{if form_field1=="valueB"}
<tr>
	<td>Label Field B1</td>
	<td>##form_fieldb1##</td>
</tr>
<tr>
	<td>Label Field B2</td>
	<td>##form_fieldb2##</td>
</tr>
{endif}
</table>

Would it be an option to deactivate the TinyMCE for the HTML part of the mail via DCA update?
(And maybe this could get an optional feature for configuration as well in the future?)

@fritzmg
Copy link
Sponsor Collaborator

fritzmg commented May 7, 2024

To restore the original behavior a replaceTokensAndInsertTagsRecursively functionality (as was used in NC 1.x via Contao Haste) would need to be re-introduced.

@Toflar
Copy link
Member

Toflar commented May 7, 2024

I know. I just wanted to understand the use case. Which I'm not quite sure about here. Probably needs a bit of time to think about.

@Shoekrates
Copy link
Author

Hi @Toflar, are there any new insights since last week?
Thinking loudly:
Another idea would be to use a separate notification mail for every option the user has chosen. With the new feature in NC Pro this should work. But this means also redundant general mail content several times. Maybe something we could handle with the custom tokens ... mmmh.

@Toflar
Copy link
Member

Toflar commented May 22, 2024

I was on vacation and will check this soon.

@Toflar
Copy link
Member

Toflar commented May 23, 2024

TLDR: we assessed the risk of re-introducing the behavior of NC v1 and decided against it.

So I looked into the issue and discussed with @aschempp for quite a long time and we don't want to change this behavior for security reasons. Recursive replacing tokens could be dangerous as it might lead to unwanted information leakage.

Imagine a user entering ##simple_token_that_is_not_meant_for_my_eyes## in a form field for example. If we apply recursive replacement, this value would potentially get sent to an attacker (if it exists).
We are very well aware that the attack vector is limited to the array of tokens the developers provide for a given notification and that this is also the case in NC v1.
But we cannot really control what extension developers come up with and just because it's an acceptable risk for v1 does not mean it also is for v2. It's not acceptable anymore.

You have multiple options:

  • You can disable tinyMCE in your user settings, configure the HTML, save and remember you have to do this in case you need to modify it. That's a super simple workaround.
  • You can develop your very own little extension that adds a ##my_table## token, that you can then use. All you need is one listener using the CreateParcelEvent event.
  • In case you don't want to develop yourself we also offer a Pro Version which is exactly meant to achieve such more complex use cases. It allows you to create your very own tokens based on Twig templates so you can create your ##my_table## token with a nice UI and use that as a placeholder in the notification just like you would when developing it yourself. I know you already know about that but I'm leaving this here for future readers.

@Toflar Toflar closed this as not planned Won't fix, can't repro, duplicate, stale May 23, 2024
@Shoekrates
Copy link
Author

Hey Yanick, thanks for clarification and assessment from the developer's perspective.
And yes, we already use the pro version ;-)

For our case we think about another pragmatic solution. TinyMCE replaces the if statements only if we put them around table rows inside the table tag. If statements should not be replaced if we put them around a separate table with complete markup.
We'll try it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants