Skip to content

SAK-52333 portal Remove PortalRenderTest and supporting files#14361

Open
adrianfish wants to merge 1 commit intosakaiproject:masterfrom
adrianfish:SAK-52333
Open

SAK-52333 portal Remove PortalRenderTest and supporting files#14361
adrianfish wants to merge 1 commit intosakaiproject:masterfrom
adrianfish:SAK-52333

Conversation

@adrianfish
Copy link
Contributor

@adrianfish adrianfish commented Feb 5, 2026

https://sakaiproject.atlassian.net/browse/SAK-52333

This commit removes the PortalRenderTest and MockCharonPortal because the latter was using an old and defunct version of JTidy and had to include workarounds for the lack of HTML5 awareness in the library. There is a new version of JTidy available which this commit upgrades portal to, however the PortalRenderTest was dumping errors into the output files which were effectively useless as they were based on the fully rendered outputs of our composed Velocity templates. Telling us that a quote is missing on line 311 of a 20 line template is not much use. Another issue is that JTidy doesn't recognise svg path tags or html fragments in single comma attributes.

My opinion is that we should be using a mixture of static analysis of the velocity template together with our current Cypress tests.

Summary by CodeRabbit

  • Chores

    • Updated HTML validation library dependency versions and sources across build configuration files.
    • Removed legacy test utilities and configurations related to HTML validation.
  • Tests

    • Removed outdated test classes and test configuration files.

https://sakaiproject.atlassian.net/browse/SAK-52333

This commit removes the PortalRenderTest and MockCharonPortal because
the latter was using an old and defunct version of JTidy and had to
include workarounds for the lack of HTML5 awareness in the library.
There is a new version of JTidy available which this commit upgrades
portal to, however the PortalRenderTest was dumping errors into the
output files which were effectively useless as they were based on the
fully rendered outputs of our composed Velocity templates. Telling us
that a quote is missing on line 311 of a 20 line template is not much
use. Another issue is that JTidy doesn't recognise svg path tags or
html fragments in single comma attributes.

My opinion is that we should be using a mixture of static analysis
of the velocity template together with our current Cypress tests.
@adrianfish adrianfish marked this pull request as draft February 5, 2026 18:32
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 5, 2026

Walkthrough

The PR updates jtidy dependencies from the old groupId/version (jtidy/r938) to com.github.jtidy/1.0.5, removes test-scoped jtidy dependencies, eliminates test utilities that relied on jtidy HTML validation, and removes associated test configuration files.

Changes

Cohort / File(s) Summary
Dependency Updates
portal/pom.xml, portal/portal-impl/impl/pom.xml, portal/portal-render-engine-impl/impl/pom.xml, portal/portal-render-impl/impl/pom.xml
Updated jtidy groupId from jtidy to com.github.jtidy and version from r938 to 1.0.5 in dependencyManagement; removed test-scoped jtidy dependencies from build configs.
Test Utilities Removal
portal/portal-render-engine-impl/impl/src/test/org/sakaiproject/portal/charon/test/MockCharonPortal.java, portal/portal-render-engine-impl/impl/src/test/org/sakaiproject/portal/charon/test/PortalRenderTest.java
Removed MockCharonPortal test helper class (533 lines) that orchestrated portal rendering with HTML validation via JTidy, and removed PortalRenderTest unit test class (176 lines) that exercised rendering scenarios.
Test Configuration
portal/portal-render-engine-impl/impl/src/testBundle/testportalvelocity.config
Removed Velocity test configuration file entries including resource loader, encoding, runtime logging, and velocimacro permissions settings (27 lines).

Suggested reviewers

  • ottenhoff
  • ern
  • csev
🚥 Pre-merge checks | ✅ 3
✅ 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 clearly and specifically identifies the main change: removal of PortalRenderTest and supporting files from the portal module.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@portal/pom.xml`:
- Around line 53-55: The pom declares JTidy as com.github.jtidy:jtidy:1.0.5
which is vulnerable to CVE-2023-34623; update the dependency strategy by either
replacing JTidy with a maintained HTML sanitizer/parser (e.g., jsoup) or
removing/isolating JTidy usage, or if you must keep it, add a patched
fork/version or input validation to reject untrusted/ deeply nested/cyclic HTML
before parsing; locate usages of com.github.jtidy classes (JTidy, Tidy, etc.)
and either refactor those call sites to the new library or wrap them with strict
pre-validation/size/recursion guards and update the pom to the chosen safe
dependency or patch artifact.

@adrianfish adrianfish marked this pull request as ready for review February 5, 2026 19:34
@adrianfish
Copy link
Contributor Author

The cypress check is failing on Samigo stats. There's no way this change can have caused that. This change mostly touches portal tests, although PortletToolRenderService does use jtidy the version of which is updated in this ticket.This change patches a vulnerability in the old jtidy library, too.

@ottenhoff
Copy link
Contributor

hi @adrianfish sorry about the samigo cypress error. please ignore it. it is waiting on #14353 to be merged

@adrianfish
Copy link
Contributor Author

@ottenhoff No worries, cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants