add support for inline style css functions

[garg23neha]

Custom style attributes are stripped from the output and not applied. We need to support inline style css functions for our use-case.
adding support for the var(...) css function.
Details can be found at https://jira.atlassian.com/browse/CONFCLOUD-77050
Added tests as well

[davewichers]

@spassarop - Can you look into this PR and determine if it's a safe and appropriate change?

[kwwall]

At a minimum, I think some rainy day tests for the added functionality are in order. Unfortunately, I don't have enough CSS expertise to offer any.

…

On Mon, Sep 23, 2024, 1:23 PM Dave Wichers ***@***.***> wrote: @spassarop <https://github.com/spassarop> - Can you look into this PR and determine if it's a safe and appropriate change? — Reply to this email directly, view it on GitHub <#511 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAO6PG6ACOMKRWL2G2EQAXLZYBFBNAVCNFSM6AAAAABOVZ2GQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRYHEYDQOJXGU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[garg23neha]

@spassarop Can I please get a review on this PR?

[spassarop]

FYI: Reviewing

[spassarop]

My initial comments:

Regarding functionality and tests, a plus is needed. When supporting something it was not before, we try to cover support as much as possible to avoid mid-implementations and potential issue requests. I say this because looking at the documentation just for var I saw cases that are not shown in the tests (e.g. var(--some-var,) and nested vars): https://developer.mozilla.org/en-US/docs/Web/CSS/var#syntax
Also, it is cool that there is a class for testing the validator separately, but it may be appropriate to have at least one more test on the main test class to see the full flow on AntiSamy works with this improvement.

Another aspect is the security implications of having more functions allowed in CSS. At least the current default policy should be tested, and maybe it is with the url function but as a fast review to give initial feedback I cannot check it right now. I did not find other direct issues when reading about exploits on CSS: https://book.hacktricks.xyz/pentesting-web/xs-search/css-injection

[garg23neha]

@spassarop I have modified the code to handle nested functions, fallback and also added some restrictions on url function.

[spassarop]

Thanks. I will do my best effort to check on this with more detail on my computer before Monday because I won’t have it with me for three weeks. Now I only did an overview with my phone.

I get the point of checking text values for JS URLs but that is usually trouble because of comparing text instead of other canonical representations. Anyway, maybe at the point you are comparing texts is ok, I just need to run some tests to see the actual behavior instead of just reading on GitHub.

[spassarop]

Ok I checked. Changes are fine. However, as I said before:

At least the current default policy should be tested, and maybe it is with the url function".

At the time the lexical unit processing processing is made, there is no need to check for url or javascript as a special case. That is because the CSS validator does this immediately after:

String value = lexicalValueToString(lu);

if (value == null || !validateValue(property, value)) {
  isValid = false;
  break;
}

So validateValue function checks against the selected policy and if there are invalid URLs they will be taken care of. It is no responsibility of the CssValidator class to do those checks.

To finish all this, could you just rollback the url+javascript checks? Also, if you haven't done it already, run the maven "site" action so the code gets automatically formatted with certain tab and line length settings that are common across the project.

Thanks for your submission!

[garg23neha]

@spassarop, I have updated the PR and removed url+javascript checks. Also ran maven site action.

[garg23neha]

@spassarop can you please review?