search

wttech / AEM-Rules-for-SonarQube

SonarQube plugin with set of rules detecting possible bugs and bad smells specific for AEM development.
Apache License 2.0
112 stars 51 forks source link

Question related to HTL rules #235

Open anrayt opened 1 year ago

anrayt commented 1 year ago

Hey, folks! I'd wish to discuss some rules related to HTL which is not clear to me " HTL-6 HTL automatically recognises the context for HTML output

HTL uses uri display context as default for src, poster, manifest, href, formaction, data, cite, action attributes HTL-7 Style and script tags display context definition is mandatory " Let me share an example: <script type="text/javascript" src="${'{0}/some-js-file.js' @ format = [inheritedPageProperties.someProperty]}"></script> There is a major issue with the rule HTL-7 - no context is provided with a script tag. Okay, I'll fix it by adding context='uri' because it's actually an uri. But, after that, one more issue will be here: HTL-6 - HTL uses uri display context as default for src . To fix that, I changed the context to - context='attribute'. It also looks ok to me, but maybe you could clarify a little bit about how it's expected to be used correctly. Thanks in advance, Anton

toniedzwiedz commented 1 year ago

Hi @anrayt, l believe HTL-7 was intended as a check for literals, i.e. verbatim JavaScript and CSS code being output. The VIOLATION_MESSAGE seems to confirm that.

Looking at the implementation https://github.com/wttech/AEM-Rules-for-SonarQube/blob/master/src/main/java/com/cognifide/aemrules/htl/checks/ScriptsAndStyleMandatoryDisplayContextCheck.java#L69, the rule applies to all expressions within a <script> tag, even when it's inside an src attribute.

In your situation, I'd keep the code as-is, with the implicit, default display context on the src attribute, and mark the violation of HTL-7 as a false positive/won't fix.

The rule could use an improvement.

anrayt commented 1 year ago

thanks!