src/lib/kcSanitize/HtmlPolicyBuilder.ts (1)

198-206: Document the rationale for the hardcoded tag list.

The change correctly allows <br> tags by restricting removal checks to a specific set of tags. However, the hardcoded list ["A", "FONT", "IMG", "INPUT", "SPAN"] lacks explanation for why these particular tags require special handling when empty.

Consider adding a comment explaining why these five tags specifically need to be removed when they have no attributes and no children, while other tags (like <br>) should be preserved. This will help future maintainers understand the business logic.

             if (
                 currentNode.attributes.length == 0 &&
                 currentNode.childNodes.length == 0
             ) {
+                // Only remove specific tags when empty and not explicitly allowed.
+                // A, IMG: meaningless without href/src attributes
+                // FONT, SPAN: styling tags with no effect when empty
+                // INPUT: form element that requires attributes to be functional
                 if (["A", "FONT", "IMG", "INPUT", "SPAN"].includes(currentNode.tagName)) {
                     if (
                         !this.tagsAllowedWithNoAttribute.has(
                             currentNode.tagName.toLowerCase()
                         )
                     ) {
                         currentNode.remove();
                     }
                 }
             }

test/kcSanitize/KcSanitizer.spec.ts (1)

121-122: Good test coverage for the basic case.

The test correctly verifies that <br> tags are preserved through sanitization along with adjacent text.

For more comprehensive coverage, consider adding tests for:

• <br> tag alone: assertResult("<br>", "<br>");
• Self-closing syntax: assertResult("<br>", "<br/>");
• Multiple breaks: assertResult("Line1<br><br>Line2", "Line1<br><br>Line2");

However, the current test is sufficient to validate the PR objective.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/lib/kcSanitize/HtmlPolicyBuilder.ts (2)

200-200: Consider extracting the hardcoded list as a constant.

The array ["a", "font", "img", "input", "span"] represents tags requiring explicit whitelisting per OWASP. Extracting this as a named constant (e.g., TAGS_REQUIRING_EXPLICIT_WHITELIST) would improve maintainability and make the intent clearer.

Apply this diff:

+    // Tags that require explicit whitelisting via allowWithoutAttributes() per OWASP
+    private static readonly TAGS_REQUIRING_EXPLICIT_WHITELIST = ["a", "font", "img", "input", "span"];
+
     private globalAttributesAllowed: Set<AttributeType> = new Set();

Then update line 200:

-                if (
-                    ["a", "font", "img", "input", "span"].includes(tag) &&
-                    !this.tagsAllowedWithNoAttribute.has(tag)
-                ) {
+                if (
+                    HtmlPolicyBuilder.TAGS_REQUIRING_EXPLICIT_WHITELIST.includes(tag) &&
+                    !this.tagsAllowedWithNoAttribute.has(tag)
+                ) {

---

198-202: Good fix; recommend refactoring tag comparisons for consistency.

The logic at lines 198-202 correctly restricts removal to the five tags requiring explicit whitelisting per OWASP documentation, allowing <br> and other tags to pass through. The code properly converts tag names to lowercase before comparison, which aligns with the pattern at line 137 where tagName.toLowerCase() is used to query the tagsAllowed map.

However, lines 207 and 221 use uppercase comparisons (currentNode.tagName === "A"), creating inconsistency. For consistency across the hook, refactor lines 207 and 221 to also use lowercase comparisons:

if (currentNode.tagName.toLowerCase() === "a" || currentNode.tagName.toLowerCase() === "img") {

and

if (currentNode.tagName.toLowerCase() === "a") {

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/lib/kcSanitize/HtmlPolicyBuilder.ts (2)

198-203: Good fix—consider extracting the tag list to a constant.

The logic correctly restricts removal to the specific tags mentioned in the OWASP documentation, allowing <br> and other tags to pass through. Lowercasing the tag name is essential since currentNode.tagName returns uppercase values in the DOM.

Consider extracting the hardcoded array to a named constant for better maintainability:

+    // Tags that require explicit whitelisting per OWASP Java HTML Sanitizer
+    private static readonly TAGS_REQUIRING_EXPLICIT_WHITELIST = ["a", "font", "img", "input", "span"];
+
     private setupHooks(): void {

Then update line 201:

-                ["a", "font", "img", "input", "span"].includes(tag) &&
+                HtmlPolicyBuilder.TAGS_REQUIRING_EXPLICIT_WHITELIST.includes(tag) &&

---

207-220: Consider standardizing tag name comparisons for consistency.

Lines 208 and 222 use uppercase comparisons (currentNode.tagName === "A" and currentNode.tagName === "IMG"), while line 198 now uses lowercase. This inconsistency can make the code harder to maintain.

Consider standardizing on lowercase comparisons throughout:

             } else {
                 //in case of <a> or <img> if we have no attribute we need to remove them even if they have child
-                if (currentNode.tagName === "A" || currentNode.tagName === "IMG") {
+                const tag = currentNode.tagName.toLowerCase();
+                if (tag === "a" || tag === "img") {
                     if (currentNode.attributes.length == 0) {
                         //add currentNode children to parent node
                         while (currentNode.firstChild) {
                             currentNode?.parentNode?.insertBefore(
                                 currentNode.firstChild,
                                 currentNode
                             );
                         }
                         // Remove the currentNode itself
                         currentNode.remove();
                     }
                 }
                 //
-                if (currentNode.tagName === "A") {
+                if (tag === "a") {
                     if (this.enforceRelNofollow) {

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/lib/kcSanitize/HtmlPolicyBuilder.ts (1)

36-39: Use toLowerCase() for consistency.

Line 37 uses toLocaleLowerCase(), but line 198 uses toLowerCase(). For HTML tag names (ASCII-only), both produce identical results, but toLowerCase() is more commonly used and creates consistency with the rest of the file.

Apply this diff:

 allowWithoutAttributes(tag: string): this {
-    this.tagsAllowedWithNoAttribute.add(tag.toLocaleLowerCase());
+    this.tagsAllowedWithNoAttribute.add(tag.toLowerCase());
     return this;
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro