Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not leak unknown asset paths into HTML #3061

Merged
merged 3 commits into from Jul 7, 2023
Merged

Do not leak unknown asset paths into HTML #3061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0debce0
Do not leak unknown asset paths into HTML
IgnatBeresnev Jul 6, 2023
460d124
Make test assertions more robust
IgnatBeresnev Jul 7, 2023
02fec83
Add an explanation comment for the unknown resource test
IgnatBeresnev Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt
View file
Open in desktop
Expand Up @@ -92,7 +92,7 @@ class DefaultTemplateModelFactory(val context: DokkaContext) : TemplateModelFact

private fun Appendable.resourcesForPage(pathToRoot: String, resources: List<String>): Unit =
resources.forEach {
append(with(createHTML()) {
val resourceHtml = with(createHTML()) {
when {
it.URIExtension == "css" ->
link(
Expand All @@ -112,7 +112,11 @@ class DefaultTemplateModelFactory(val context: DokkaContext) : TemplateModelFact
it.isImage() -> link(href = if (it.isAbsolute) it else "$pathToRoot$it")
else -> null
}
} ?: it)
Copy link
Member Author

Choose a reason for hiding this comment

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

The bug was with ?: it

}

if (resourceHtml != null) {
append(resourceHtml)
}
}
}

Expand Down
89 changes: 89 additions & 0 deletions plugins/base/src/test/kotlin/resourceLinks/ResourceLinksTest.kt
View file
Open in desktop
Expand Up @@ -13,13 +13,16 @@ 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
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<String>) : DokkaPlugin() {
Expand Down Expand Up @@ -205,4 +208,90 @@ class ResourceLinksTest : BaseAbstractTest() {
}
}
}

@Test // see #3040; plain text added to <head> can be rendered by engines inside <body> as well
fun `should not add unknown resources as text to the head or body section`() {
val configuration = dokkaConfiguration {
sourceSets {
sourceSet {
sourceRoots = listOf("src/main/kotlin")
}
}

pluginsConfigurations = mutableListOf(
PluginConfigurationImpl(
DokkaBase::class.java.canonicalName,
DokkaConfiguration.SerializationFormat.JSON,
toJsonString(
DokkaBaseConfiguration(
customAssets = listOf(File("test/unknown-file.ext"))
)
)
)
)
}

val writerPlugin = TestOutputWriterPlugin()
testInline(
"""
|/src/main/kotlin/test/Test.kt
|package test
|
|class Test
""".trimMargin(),
configuration,
pluginOverrides = listOf(writerPlugin)
) {
renderingStage = { _, _ ->
val testClassPage = writerPlugin.writer.contents
.getValue("root/test/-test/-test.html")
.let { Jsoup.parse(it) }

val headChildNodes = testClassPage.head().childNodes()
assertTrue("<head> section should not contain non-blank text nodes") {
headChildNodes.all { it !is TextNode || it.isBlank }
}

val bodyChildNodes = testClassPage.body().childNodes()
assertTrue("<body> section should not contain non-blank text nodes. Something leaked from head?") {
bodyChildNodes.all { it !is TextNode || it.isBlank }
}
}
}
}

@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")
}
}
}
}
34 changes: 0 additions & 34 deletions plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt
View file
Open in desktop
Expand Up @@ -177,40 +177,6 @@ class PageTransformerBuilderTest : BaseAbstractTest() {
}
}

@Test
fun `should load script as defer if name ending in _deferred`() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote this test a while ago, and at the time I couldn't find a good place for it, but I think having it in ResourceLinksTest is better now, so I moved it there.

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 <T> Collection<T>.assertCount(n: Int, prefix: String = "") =
assert(count() == n) { "${prefix}Expected $n, got ${count()}" }

Expand Down