From 0debce0eae1df843d511569d99c46f9196c618df Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Thu, 6 Jul 2023 18:13:05 +0200 Subject: [PATCH 1/3] Do not leak unknown asset paths into HTML Fixes #3040 --- .../DefaultTemplateModelFactory.kt | 8 +- .../kotlin/resourceLinks/ResourceLinksTest.kt | 91 +++++++++++++++++++ .../PageTransformerBuilderTest.kt | 34 ------- 3 files changed, 97 insertions(+), 36 deletions(-) diff --git a/plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt b/plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt index 4fee280e60..aae2f65d24 100644 --- a/plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt +++ b/plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt @@ -92,7 +92,7 @@ class DefaultTemplateModelFactory(val context: DokkaContext) : TemplateModelFact private fun Appendable.resourcesForPage(pathToRoot: String, resources: List): Unit = resources.forEach { - append(with(createHTML()) { + val resourceHtml = with(createHTML()) { when { it.URIExtension == "css" -> link( @@ -112,7 +112,11 @@ class DefaultTemplateModelFactory(val context: DokkaContext) : TemplateModelFact it.isImage() -> link(href = if (it.isAbsolute) it else "$pathToRoot$it") else -> null } - } ?: it) + } + + if (resourceHtml != null) { + append(resourceHtml) + } } } diff --git a/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt b/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt index a751f071ef..c93cfcd247 100644 --- a/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt +++ b/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt @@ -17,9 +17,11 @@ import org.junit.jupiter.api.Test import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.ValueSource import utils.TestOutputWriterPlugin +import utils.assertContains import java.io.File import kotlin.test.assertNotNull import kotlin.test.assertNull +import kotlin.test.assertTrue class ResourceLinksTest : BaseAbstractTest() { class TestResourcesAppenderPlugin(val resources: List) : DokkaPlugin() { @@ -205,4 +207,93 @@ class ResourceLinksTest : BaseAbstractTest() { } } } + + @Test + fun `should not add unknown resources to the end of the head section`() { + val baseConfiguration = DokkaBaseConfiguration( + customAssets = listOf(File("test/relativePath.js")) + ) + + val configuration = dokkaConfiguration { + sourceSets { + sourceSet { + sourceRoots = listOf("src/main/kotlin") + } + } + + pluginsConfigurations = mutableListOf( + PluginConfigurationImpl( + fqPluginName = "org.jetbrains.dokka.base.DokkaBaseConfiguration", + serializationFormat = DokkaConfiguration.SerializationFormat.JSON, + values = toJsonString(baseConfiguration) + ) + ) + } + + val writerPlugin = TestOutputWriterPlugin() + testInline( + """ + |/src/main/kotlin/test/Test.kt + |package test + | + |class Test + """.trimMargin(), + configuration, + pluginOverrides = listOf(writerPlugin) + ) { + renderingStage = { _, _ -> + val generatedFiles = writerPlugin.writer.contents + + val testPageHeadHtml = generatedFiles + .getValue("root/test/-test/-test.html") + .let { Jsoup.parse(it) } + .select("head") + .toString() + + assertTrue { + testPageHeadHtml.endsWith( + """ + + + """.trimIndent() + ) + } + } + } + } + + @Test + fun `should load script as defer if name ending in _deferred`() { + val configuration = dokkaConfiguration { + sourceSets { + sourceSet { + sourceRoots = listOf("src/main/kotlin") + } + } + } + + val writerPlugin = TestOutputWriterPlugin() + testInline( + """ + |/src/main/kotlin/test/Test.kt + |package test + | + |class Test + """.trimMargin(), + configuration, + pluginOverrides = listOf(writerPlugin) + ) { + renderingStage = { _, _ -> + val generatedFiles = writerPlugin.writer.contents + + assertContains(generatedFiles.keys, "scripts/symbol-parameters-wrapper_deferred.js") + + val scripts = generatedFiles.getValue("root/test/-test/-test.html").let { Jsoup.parse(it) }.select("script") + val deferredScriptSources = scripts.filter { element -> element.hasAttr("defer") }.map { it.attr("src") } + + // important to check symbol-parameters-wrapper_deferred specifically since it might break some features + assertContains(deferredScriptSources, "../../../scripts/symbol-parameters-wrapper_deferred.js") + } + } + } } diff --git a/plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt b/plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt index 30fc127ff9..b2b590d3c4 100644 --- a/plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt +++ b/plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt @@ -177,40 +177,6 @@ class PageTransformerBuilderTest : BaseAbstractTest() { } } - @Test - fun `should load script as defer if name ending in _deferred`() { - val configuration = dokkaConfiguration { - sourceSets { - sourceSet { - sourceRoots = listOf("src/main/kotlin") - } - } - } - val writerPlugin = TestOutputWriterPlugin() - testInline( - """ - |/src/main/kotlin/test/Test.kt - |package test - | - |class Test - """.trimMargin(), - configuration, - pluginOverrides = listOf(writerPlugin) - ) { - renderingStage = { _, _ -> - val generatedFiles = writerPlugin.writer.contents - - assertContains(generatedFiles.keys, "scripts/symbol-parameters-wrapper_deferred.js") - - val scripts = generatedFiles.getValue("root/test/-test/-test.html").let { Jsoup.parse(it) }.select("script") - val deferredScriptSources = scripts.filter { element -> element.hasAttr("defer") }.map { it.attr("src") } - - // important to check symbol-parameters-wrapper_deferred specifically since it might break some features - assertContains(deferredScriptSources, "../../../scripts/symbol-parameters-wrapper_deferred.js") - } - } - } - private fun Collection.assertCount(n: Int, prefix: String = "") = assert(count() == n) { "${prefix}Expected $n, got ${count()}" } From 460d124274346a18cfcb2e7edbbc86329a57b903 Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Fri, 7 Jul 2023 11:08:03 +0200 Subject: [PATCH 2/3] Make test assertions more robust --- .../kotlin/resourceLinks/ResourceLinksTest.kt | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt b/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt index c93cfcd247..47735e7618 100644 --- a/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt +++ b/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt @@ -13,6 +13,7 @@ import org.jetbrains.dokka.plugability.DokkaPluginApiPreview import org.jetbrains.dokka.plugability.PluginApiPreviewAcknowledgement import org.jetbrains.dokka.transformers.pages.PageTransformer import org.jsoup.Jsoup +import org.jsoup.nodes.TextNode import org.junit.jupiter.api.Test import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.ValueSource @@ -209,11 +210,7 @@ class ResourceLinksTest : BaseAbstractTest() { } @Test - fun `should not add unknown resources to the end of the head section`() { - val baseConfiguration = DokkaBaseConfiguration( - customAssets = listOf(File("test/relativePath.js")) - ) - + fun `should not add unknown resources as text to the head section`() { val configuration = dokkaConfiguration { sourceSets { sourceSet { @@ -223,9 +220,13 @@ class ResourceLinksTest : BaseAbstractTest() { pluginsConfigurations = mutableListOf( PluginConfigurationImpl( - fqPluginName = "org.jetbrains.dokka.base.DokkaBaseConfiguration", - serializationFormat = DokkaConfiguration.SerializationFormat.JSON, - values = toJsonString(baseConfiguration) + DokkaBase::class.java.canonicalName, + DokkaConfiguration.SerializationFormat.JSON, + toJsonString( + DokkaBaseConfiguration( + customAssets = listOf(File("test/unknown-file.ext")) + ) + ) ) ) } @@ -242,21 +243,18 @@ class ResourceLinksTest : BaseAbstractTest() { pluginOverrides = listOf(writerPlugin) ) { renderingStage = { _, _ -> - val generatedFiles = writerPlugin.writer.contents - - val testPageHeadHtml = generatedFiles + val testClassPage = writerPlugin.writer.contents .getValue("root/test/-test/-test.html") .let { Jsoup.parse(it) } - .select("head") - .toString() - assertTrue { - testPageHeadHtml.endsWith( - """ - - - """.trimIndent() - ) + val headChildNodes = testClassPage.head().childNodes() + assertTrue(" section should not contain non-blank text nodes") { + headChildNodes.all { it !is TextNode || it.isBlank } + } + + val bodyChildNodes = testClassPage.body().childNodes() + assertTrue(" section should not contain non-blank text nodes. Something leaked from head?") { + bodyChildNodes.all { it !is TextNode || it.isBlank } } } } From 02fec83c6b435fb026d15c2f0ce798f4d4858d70 Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Fri, 7 Jul 2023 11:09:22 +0200 Subject: [PATCH 3/3] Add an explanation comment for the unknown resource test --- .../base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt b/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt index 47735e7618..0bbd35d1db 100644 --- a/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt +++ b/plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt @@ -209,8 +209,8 @@ class ResourceLinksTest : BaseAbstractTest() { } } - @Test - fun `should not add unknown resources as text to the head section`() { + @Test // see #3040; plain text added to can be rendered by engines inside as well + fun `should not add unknown resources as text to the head or body section`() { val configuration = dokkaConfiguration { sourceSets { sourceSet {