closes #16211 Table: breaks after one call to updateStyleElement()

[ymg2006]

closes #16211 Table: breaks after one call to updateStyleElement()
Added security bypass and html sanitizer for user inputs without breaking components!
This will stop injections to happen with angular built-in DomSanitizer.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
primeng	⬜️ Ignored (Inspect)	Visit Preview		Nov 18, 2024 5:51pm
primeng-v18	⬜️ Ignored (Inspect)	Visit Preview		Nov 18, 2024 5:51pm

[aaronshim]

If the original PR with textContent was not working because the assignment value contained markup that wasn’t just the CSS code, how can we be sure that we can trust this assignment value as a style?

[ymg2006]

bypassSecurityTrustStyle will Bypass security and trust the given value to be safe style value (CSS).
abstract bypassSecurityTrustStyle(value: string): SafeStyle
since the type is assigned to responsiveStyleElement we cannot set innerHTML as SafeStyle.
so the suggestion here is to make the responsiveStyleElement as any and then we do not need the as string cast.
I already merged this suggestion into this pull request.

On the other hand, these styles are created internally by primeng, so they are trusted and there is no need for sanitization.
Moreover, all the inputs from user are SANITIZED then bypassed by SafeHtml pipe.

   constructor(@Inject(PLATFORM_ID) private readonly platformId: any, private readonly domSanitizer: DomSanitizer) { }

    public transform(value: string): SafeHtml {
        if (!value || !isPlatformBrowser(this.platformId)) {
            return value;
        }

+         const sanitizedValue = this.domSanitizer.sanitize(SecurityContext.HTML ,value);
+         return this.domSanitizer.bypassSecurityTrustHtml(sanitizedValue);

    }

[rosenthalj]

I took a quick cursory look at the proposed changes and a few comments:

1. A lot of file code changes. Has the all the changes been tested?
2. Maybe I'm missing something, there are no code changes to table.ts?

Note: I would recommend modifying the text in issue #16211 and remove the following (since you took a different approach in the PR):

 To sanitize html my suggestion is to use [DOMPurify](https://github.com/cure53/DOMPurify) package and replace all methods that uses inner html with:
import DOMPurify from 'dompurify';
el.innerHTML = DOMPurify.sanitize(innerHtml, {RETURN_TRUSTED_TYPE: true});

It's good that this PR doesn't add new OSS dependency to PrimeNG.

[ymg2006]

@rosenthalj, to response your first question, I have NOT tested all changes, I will really appreciate it if @aaronshim do this on my behalf and see if they are safe as explained in #16153.

For the second question, I have merged the needed changes, thanks for the review.

Finally, I read angular documentation and saw their standard approach for this so implemented as suggested.

PR #16210 has nothing to do with this PR.
issue #16211 Table: breaks after one call to updateStyleElement() is modified.

[rosenthalj]

@rosenthalj, to response your first question, I have NOT tested all changes, I will really appreciate it if @aaronshim do this on my behalf and see if they are safe as explained in #16153.

For the second question, I have merged the needed changes, thanks for the review.

Finally, I read angular documentation and saw their standard approach for this so implemented as suggested.

PR #16210 has nothing to do with this PR. issue #16211 Table: breaks after one call to updateStyleElement() is modified.

@ymg2006

You are correct, in my previous comment I incorrectly referenced issue #16210 instead of issue #16211. I saw that you acted on my note and updated the the text in issue #16211. I'm sorry for any confusion based on my incorrect reference. I have also updated my previous comment in this PR to point to issue #16211

[cetincakiroglu]

Hi,

Thanks for all the effort guys, you're awesome! since 20 files have been changed, it'll take time to review this PR. During this time, we'll fix the issue for the table beforehand.

[cetincakiroglu]

Also, could you please remove table fix (we've just fixed that), and create a separate issue for the rest of the components so that we can include it in next week's milestone?

[ymg2006]

Also, could you please remove table fix (we've just fixed that), and create a separate issue for the rest of the components so that we can include it in next week's milestone?

@cetincakiroglu I really appreciate if you bear with me up to 29th of August that I test all the changes exhaustively and then make the aforementioned corrections.
Thanks for fixing the table issue.

[ymg2006]

@cetincakiroglu merged with changes from table.ts

[cetincakiroglu]

Hi @ymg2006,

Thanks a lot for the effort and support! Could you please resolve the conflicts and test again, files are updated and it might resolved already, if not, please tag me after the conflict fix, so I can merge it safely.

[ymg2006]

@cetincakiroglu merge is done and build issue is solved too.

[mertsincan]

Hi,

Could you please update your PR and create a new issue? Then, I'll review asap. Thank you!

[vercel]

@ymg2006 is attempting to deploy a commit to the primetek Team on Vercel.

A member of the Team first needs to authorize it.

[ymg2006]

@mertsincan merge is done.

[mertsincan]

Thanks a lot for your contribution! Unfortunately, this PR has conflicts, so I cannot merge it. Could you please create a new PR and open a corresponding issue for this PR? Then, I'll merge it asap.