SAK-47679 Update Antisamy 1.7.8

[axxter99]

Summary by CodeRabbit

• Chores
  • Upgraded a third-party sanitization library to a newer version to improve compatibility and reliability.
  • Excluded an unnecessary logging dependency to streamline the dependency tree and reduce potential conflicts.
  • No functional changes expected; application behavior remains the same for end users.
  • These updates help keep the app lightweight and maintainable while aligning with current dependency best practices.

[bjones86]

This is still failing some FormattedText test(s)

[axxter99]

[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] FormattedTextTest.testDataAttributes:337 expected:<> but was:<>
[ERROR] FormattedTextTest.testKNL_1487:1083 expected:<...e530d7221ee512cb429"[]>> but was:<...e530d7221ee512cb429"[ /]>>
[INFO]
[ERROR] Tests run: 373, Failures: 2, Errors: 0, Skipped: 0

[axxter99]

break SAK-47712 Passay: #10809

[jonespm]

Passay was updated passed this in #12967

[jonespm]

I changed the version to the latest 1.7.7 and removed the already upgraded passay. If this fails the test I will look at it this week.

[jonespm]

The error that was there seems minor and could just be fixed in the test. However it looks like it's related to this open issue. nahsra/antisamy#484

There is a new test failure

Error: [ERROR]   FormattedTextTest.testDataAttributes:341 expected:<<span class="[]two"></span>> but was:<<span class="[one" class="]two"></span>>

Which looks like it's checking for duplicate attributes, which are invalid HTML but not really a security issue. It looks like it does have a check in the code for duplicate attributes. So not sure why this one is failing now.

https://github.com/nahsra/antisamy/blob/f94866b98c909cf470d0a15b59f7b28dcb9ab4bf/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java#L1555

I tried also to use AntiSamy.DOM as used in the test to scan (the default is AntiSamy.SAX) but that just resulted in more errors. This test still in error but it was a different result.

Using Antisamy.DOM

[ERROR]   FormattedTextTest.testDataAttributes:341 expected:<[<span class="two"></span>]> but was:<[]>
[ERROR]   FormattedTextTest.testKNL_1407:831 The source tag was empty, and therefore we could not process it. The rest of the message is intact, and its removal should not have any side effects.<br/>The track tag was empty, and therefore we could not process it. The rest of the message is intact, and its removal should not have any side effects.<br/>
[ERROR]   FormattedTextTest.testKNL_1487:1087 expected:<...e530d7221ee512cb429"[]/>> but was:<...e530d7221ee512cb429"[ ]/>>
[ERROR]   FormattedTextTest.testKNL_1530:1165 expected:<.../www.sakailms.org/" [rel="noopener" ]target="_blank" rel=...> but was:<.../www.sakailms.org/" []target="_blank" rel=...>

[davewichers]

@jonespm - Are you still working on this? Or should we just close it?

[jonespm]

Hi @davewichers, not sure what you mean by "close it". This issue here was just meant to upgrade antisamy to 1.7.7 or later. But it was blocked by a test failing from the latest release of antisamy so we can't upgrade to a later version. We still want to upgrade when/if we're able and our tests pass.

I've done nothing to try to fix the issue in the library. I also hadn't looked at that issue for 8 months. From the conversation it looks like @spassarop was working on a fix in this branch. I haven't reviewed or tested, and looks like they never made a PR.

nahsra/antisamy#484 (comment)
nahsra/antisamy@main...closing-tags-behavior

I tried to make a PR from it but I cannot because it says I'm not a collaborator, but someone on the project should be able to hit the button and see how it looks.

[davewichers]

@jonespm - Sorry. This PR is in YOUR project, not mine, so ignore my comment about the PR. However, I just merged a fix into antisamy main related to this issue. Can you install/test to see if it helps?

[coderabbitai]

Walkthrough

Updated Maven dependency in kernel/kernel-impl/pom.xml: bumped org.owasp.antisamy from 1.6.8 to 1.7.8 and added an exclusions block to exclude commons-logging.

Changes

Cohort / File(s)	Summary of changes
Dependency Updates kernel/kernel-impl/pom.xml	Updated org.owasp.antisamy:antisamy version from 1.6.8 to 1.7.8; added <exclusions> to exclude commons-logging:commons-logging.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "SAK-47679 Update Antisamy 1.7.8" is concise, includes the issue reference, and directly describes the primary change in the diff (the Antisamy dependency bump in kernel/kernel-impl/pom.xml from 1.6.8 to 1.7.8), so it accurately summarizes the main intent of the PR.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

kernel/kernel-impl/pom.xml (1)

315-320: Excluding commons-logging is correct — a JCL→SLF4J bridge exists; add an Enforcer rule to prevent regressions.

• jcl-over-slf4j is declared (master/pom.xml, deploy/pom.xml) and many modules already exclude commons-logging.
• Confirm a single SLF4J binding on the classpath (site-manage/site-manage-impl/impl/pom.xml uses slf4j-simple; master references slf4j-log4j12).
• Recommend adding a root-level maven-enforcer bannedDependencies rule to block commons-logging:commons-logging (and commons-logging:commons-logging-api) so it cannot be reintroduced.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6fcdc9f and bed0387.

📒 Files selected for processing (1)

• kernel/kernel-impl/pom.xml (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/pom.xml

📄 CodeRabbit inference engine (AGENTS.md)

Configure Maven to compile with Java 17 (maven-compiler-plugin/source/target)

Files:

• kernel/kernel-impl/pom.xml

⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: maven-build
• GitHub Check: sakai-deploy

🔇 Additional comments (2)

kernel/kernel-impl/pom.xml (2)

25-27: Confirm inherited maven-compiler-plugin sets Java 17

master/pom.xml declares the maven-compiler-plugin (v3.11.0) with true (≈lines 2898–2902); verify it configures Java 17 (e.g. 17, or 17 and 17, or maven.compiler.release=17). If yes, no change needed in kernel/kernel-impl/pom.xml; if not, add the plugin or set 17 in this module.

---

312-321: Align AntiSamy version and verify jsoup compatibility

• PR title says 1.7.7 but kernel/kernel-impl/pom.xml declares AntiSamy 1.7.8 — the duplicate/split-attribute fix referenced landed in 1.7.7, so 1.7.8 already contains it; confirm the intended target and make the PR title or POM consistent.
• Verify jsoup resolution for the selected AntiSamy to avoid transitive conflicts — master/pom.xml declares jsoup as ${sakai.jsoup.version}; confirm that property's value and that no transitive or module override will pull an incompatible jsoup.

[jonespm]

So as of the latest version there's these two remaining

[ERROR] FormattedTextTest.testKNL_1487:1087 expected:<...e530d7221ee512cb429"[]/>> but was:<...e530d7221ee512cb429"[ ]/>>

This is just saying there's it's removing whitespace at the end of a tag. I feel like this is fine and preservation of whitespace in a tag isn't something we care about and we can just change this test to accommodate.

This is the other one
[ERROR] FormattedTextTest.testDataAttributes:341 expected:<<span class="[]two"></span>> but was:<<span class="[one" class="]two"></span>>

Previous AntiSamy removed duplicate attributes. Current is leaving both attributes as is.

String repeatKV = "<span class=\"one\" class=\"two\"></span>";

Previously was

String resultRepeatKV = "<span class=\"two\"></span>";
But now is returning the same string of
<span class=\"one\" class=\"two\"></span>";

[ottenhoff]

both seem fine to me!

[jonespm]

I was trying to figure out where the change came in, I think it's even further upstream in the change to htmlunit-neko? I don't think it's worth spending more time and I'd probably just update both tests to match the current output. I feel like someone having duplicate attributes is unlikely and even if so, browsers will only use the last value.

[github-actions]

This PR has been automatically marked as stale due to 10 days of inactivity. It will be closed in 4 days unless there is new activity.

[github-actions]

This PR has been automatically closed due to inactivity after being marked as stale. You may simply reopen it and continue working on it.