Issue 51749: Site validator fails on some wikis

[labkey-jeckels]

Rationale

Add some test coverage for site validation of wikis.

Related Pull Requests

• Issue 51749: Site validator fails on some wikis platform#6096

Changes

• Create a wiki with CSP problems (inline script)
• Improve WikiHelper
• Eliminate need for casting when calling getCurrentTest()

[labkey-chrisj]

Thanks for doing this

[labkey-tchad]

The wiki module isn't in the base distribution anymore.
DatabaseDiagnosticsTest gets run in a lot of suites, possibly ones where the wiki module isn't present. If not now, possibly in the future. It is also intended to be run at the end of suites to do some extra validation, without adding much extra overhead.
I would put this check into a separate test (or maybe one of the existing wiki tests) to avoid the overhead of creating an extra project and wiki in suites that aren't wiki-focused.

[labkey-tchad]

Adding the wiki webpart makes the setup longer and makes the test likely to trigger the CSP just by navigating through the project home page. Better to goToModule("Wiki") to create the wiki.

[labkey-jeckels]

@labkey-tchad I moved my new coverage into a new test, WikiCspTest. It's passing for me locally but failing on TeamCity. I eventually added enough logging to discover that there are multiple CspCheckPageLoadListener objects. I'm turning off the check for _cspCheckPageLoadListener but another one is still failing.

I don't have a good handle on the life cycle here, and can't repro locally. Any suggestions?

[labkey-tchad]

@labkey-tchad I moved my new coverage into a new test, WikiCspTest. It's passing for me locally but failing on TeamCity. I eventually added enough logging to discover that there are multiple CspCheckPageLoadListener objects. I'm turning off the check for _cspCheckPageLoadListener but another one is still failing.

I don't have a good handle on the life cycle here, and can't repro locally. Any suggestions?

Yeah, the JUnit lifecycle makes these tricky. Now that I look at it again, what you're seeing makes sense. Each @Test is run in a separate thread with a separate instance of the test class. Thus, a new CspCheckPageLoadListener instance is registered with each test that is run. Without a serious redesign, it isn't really safe to make the page load listeners stateful like this (even having the ArtifactCollector and DeferredErrorCollector baked in probably isn't a great idea).

Instead of having an off/on switch, a better option might be to call CspLogUtil.checkNewCspWarnings(getArtifactCollector()); immediately after saving the wiki and catch the CspWarningDetectedException that it should throw. This is how CrawlerTest#testCspWarning handles an expected CSP warning without triggering the CspCheckPageLoadListener.

Also, WikiCspTest should override checkLinks to no-op so that the crawler doesn't accidentally trigger the CSP.

[labkey-jeckels]

@labkey-tchad I moved my new coverage into a new test, WikiCspTest. It's passing for me locally but failing on TeamCity. I eventually added enough logging to discover that there are multiple CspCheckPageLoadListener objects. I'm turning off the check for _cspCheckPageLoadListener but another one is still failing.
I don't have a good handle on the life cycle here, and can't repro locally. Any suggestions?

Yeah, the JUnit lifecycle makes these tricky. Now that I look at it again, what you're seeing makes sense. Each @Test is run in a separate thread with a separate instance of the test class. Thus, a new CspCheckPageLoadListener instance is registered with each test that is run. Without a serious redesign, it isn't really safe to make the page load listeners stateful like this (even having the ArtifactCollector and DeferredErrorCollector baked in probably isn't a great idea).

Instead of having an off/on switch, a better option might be to call CspLogUtil.checkNewCspWarnings(getArtifactCollector()); immediately after saving the wiki and catch the CspWarningDetectedException that it should throw. This is how CrawlerTest#testCspWarning handles an expected CSP warning without triggering the CspCheckPageLoadListener.

Also, WikiCspTest should override checkLinks to no-op so that the crawler doesn't accidentally trigger the CSP.

Thanks for the guidance. The test just passed on TeamCity.

As a separate followup, should we avoid figure out how to avoid registering multiple CspCheckPageLoadListener? Maybe remove any existing ones before adding the new one? Maybe they're fast enough to not matter but we're presumably doing many redundant checks for tests that have a lot of @Test methods

[labkey-tchad]

We can deal with the duplicate CspCheckPageLoadListeners later. Hopefully the fact that the page load listeners are stored in a WeakHashMap means we don't build up too many at once.