fix: bind empty message string as plain text to avoid xss

[chintankavathia]

BREAKING CHANGE: emptyMessage no longer allow passing html to prevent XSS attacks. use slot based content projection empty-content for displaying html rich empty content message.

What kind of change does this PR introduce? (check one with "x")

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

• Yes
• No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

[spike-rabbit]

Makes sense. Can you just please enhance you breaking change note: (but remove the )

BREAKING CHANGE: `emptyMessage` is no longer interpreted as HTML to prevent XSS attacks.
Use content projection for displaying an HTML empty content message:

\```
<ngx-datatable>
   <div empty-content>
      My rich <i>html</i> content.
   </div>
</ngx-datatable>
\```

[spike-rabbit]

After thinking a little longer about: this could actually be very annoying for apps, that used to provide some HTML as config on a global level. Since I agree, that we should not have this, I thought about maybe adding support to provide a component on a global level for empty content.
This would also help us internally and would be a replacement for HTML configs.
WDYT @chintankavathia @fh1ch @timowolf @kfenner ?

[chintankavathia]

After thinking a little longer about: this could actually be very annoying for apps, that used to provide some HTML as config on a global level. Since I agree, that we should not have this, I thought about maybe adding support to provide a component on a global level for empty content. This would also help us internally and would be a replacement for HTML configs. WDYT @chintankavathia @fh1ch @timowolf @kfenner ?

I am bit in doubt here if applications would be using a generic html as empty content. It might not make any sense to have such thing in large applications having different feature based tables. e.g application might have a page which list all the users where it says no users and there might be other page which displays list of devices and there it says no devices and so on.

[fh1ch]

After thinking a little longer about: this could actually be very annoying for apps, that used to provide some HTML as config on a global level. Since I agree, that we should not have this, I thought about maybe adding support to provide a component on a global level for empty content. This would also help us internally and would be a replacement for HTML configs. WDYT @chintankavathia @fh1ch @timowolf @kfenner ?

@spike-rabbit as we also quickly talked offline about this: I think this change is for our internal applications not that problematic as they pretty much exclusively rely on empty-content/content-projection which we've introduced here in our fork. However, I also consider the change rather annoying for applications out in the wild that rely on global HTML injection. There are frankly many applications/design systems that use an identical empty-table placeholder, hence I would consider this a more-than-fair-enough use-case. Hence in terms of good open-source stewardship and also the adoption of our fork for existing applications that use ngx-datatable, I see this change as somewhat problematic. I think the benefits can also be somewhat challenged: While XSS is a real problem for areas where data is loaded dynamically, this particular value is set during module initialization (see

ngx-datatable/src/main.ts

Lines 18 to 24 in f0fb0fb

	NgxDatatableModule.forRoot({
	messages: {
	emptyMessage: 'No data to display', // Message to show when array is presented, but contains no values
	totalMessage: 'total', // Footer total message
	selectedMessage: 'selected' // Footer selected message
	}
	})

) where dynamic injection is not really a concern.

I'm therefore also more in favor of finding a longer-term solution that would probably help us too in reducing duplication and fostering alignment.

[Sirius-A]

Sorry to pop in here unannounced 😅. Two inputs from my side:

• Would it be possible to use Angular's built-in sanitize method to ensure no dangerous code is rendered?

  • My understanding was that [innerHTML] is "patched" by Angular, so it gets sanitized automatically. I'm not sure why Angular fails to do that in this case 🤷.

• Would you considered creating a security advisory once this is resolved and a patch is available? That way GitHub might inform users automatically about the potential vulnerability.

[chintankavathia]

Sorry to pop in here unannounced 😅. Two inputs from my side:

• Would it be possible to use Angular's built-in sanitize method to ensure no dangerous code is rendered?

  • My understanding was that [innerHTML] is "patched" by Angular, so it gets sanitized automatically. I'm not sure why Angular fails to do that in this case 🤷.

• Would you considered creating a security advisory once this is resolved and a patch is available? That way GitHub might inform users automatically about the potential vulnerability.

regarding using sanitize - problem with sanitize is that it is very aggressive and trims most of the things into plain text and mostly become same as having plain text except few exceptions like support for header tags etc.

[innerHTML] is patched by angular however here we get the value from apps and they may do something like sanitizer.bypassSecurityTrustHtml and then angular won't patch

security advisory I wasn't aware of such feature 😄

[Sirius-A]

Just my opinion: If one uses sanitizer.bypassSecurityTrustHtml, they should know that their input is safe and do not allow it to come from end user inputs 🤷. We can't safe everyone.

I did not test the sanitze method, but I can see that it would remove html pretty aggressively.

---

Security advisories are great! They also allow you to create a private fork of a repo, while you are still working on a fix. That way you can keep the security fixes and discussion private. A bit late for this issue, but good to know if something like this comes up again 😇.

[fh1ch]

Sorry to pop in here unannounced 😅. Two inputs from my side:

• Would it be possible to use Angular's built-in sanitize method to ensure no dangerous code is rendered?

  • My understanding was that [innerHTML] is "patched" by Angular, so it gets sanitized automatically. I'm not sure why Angular fails to do that in this case 🤷.

• Would you considered creating a security advisory once this is resolved and a patch is available? That way GitHub might inform users automatically about the potential vulnerability.

@Sirius-A thanks a lot for chiming in 🙇 The PR title might be a bit misleading since Angular's innerHTML already does sanitize the input and it was never possible to inject XSS (without doing so on purpose with bypassSecurityTrustHtml). However, internal tests found HTML injections, which could be problematic if used for bad-link injection, hence we patched it in a few places (e.g. #126). But as discussed in #216 (comment), the problem is less of a problem in this case, since the configuration is done during initialization which doesn't allow 3rd parties to inject custom content that easily.

Due to that, I frankly don't see a need to create a GitHub security advisory for this case. Both from an Angular and project's point of view, things work as expected: XSS is not possible without bypassSecurityTrustHtml, inputs that support HTML can be used to inject safe HTML.

@spike-rabbit WDYT?

[Sirius-A]

Ah I see. The title was indeed a bit confusing to me.

I totally agree that there is no need for a security advisory in this case.