diff --git a/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java b/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java index 2d173afe..24fc649c 100644 --- a/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java +++ b/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java @@ -96,6 +96,12 @@ public String getRepoUrl() throws IOException { return ""; } + // Only web URLs may be embedded in generated reports as source links; + // anything else (javascript:, data:, file:, ...) is rejected outright. + if (!originUrl.startsWith("https://") && !originUrl.startsWith("http://")) { + return ""; + } + repoUrl = originUrl.replace(".git", ""); if (repoUrl.contains("gitlab")) { @@ -105,7 +111,11 @@ public String getRepoUrl() throws IOException { } else { repoUrl = repoUrl + "/blob/" + getCurrentCommitHash() + "/"; } - return repoUrl; + + // Keep only RFC 3986 URL characters: drops every char that is markup- or + // JS-significant outside a URL (space, ", ', <, >, `, {, }, \), so the value + // cannot break out of an attribute or a ")) + .call(); + + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + assertEquals("", repoUrl, "data: scheme should be rejected"); + } + } + + @Test + void testGetRepoUrl_returnsEmpty_whenOriginUrlIsFileScheme() throws Exception { + git.remoteAdd() + .setName("origin") + .setUri(new URIish("file:///etc/passwd")) + .call(); + + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + assertEquals("", repoUrl, "file: scheme should be rejected"); + } + } + + @Test + void testGetRepoUrl_returnsEmpty_whenOriginUrlIsNotHttpOrHttps() throws Exception { + git.remoteAdd() + .setName("origin") + .setUri(new URIish("ssh://git@example.com/repo.git")) + .call(); + + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + assertEquals("", repoUrl, "ssh: scheme should be rejected"); + } + } + + @Test + void testGetRepoUrl_sanitizesUrl_removingMarkupCharacters() throws Exception { + git.getRepository().getConfig().setString("remote", "origin", "url", "https://example.com/repo\".git"); + git.getRepository().getConfig().save(); + + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + assertTrue(repoUrl.startsWith("https://example.com/repounsafe")); + assertFalse(repoUrl.contains("\""), "URL must not retain quotes: " + repoUrl); + assertFalse(repoUrl.contains("<"), "URL must not retain angle brackets: " + repoUrl); + } + } + + @Test + void testGetRepoUrl_allowsValidHttpsUrl() throws Exception { + git.remoteAdd() + .setName("origin") + .setUri(new URIish("https://github.com/user/repo.git")) + .call(); + + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + assertTrue( + repoUrl.startsWith("https://github.com/user/repo/blob/"), + "Valid HTTPS URL should be allowed: " + repoUrl); + } + } + + @Test + void testGetRepoUrl_allowsValidHttpUrl() throws Exception { + git.remoteAdd() + .setName("origin") + .setUri(new URIish("http://example.com/repo.git")) + .call(); + + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + assertTrue( + repoUrl.startsWith("http://example.com/repo/blob/"), + "Valid HTTP URL should be allowed: " + repoUrl); + } + } + + @Test + void testGetRepoUrl_stripsGitSuffix() throws Exception { + git.remoteAdd() + .setName("origin") + .setUri(new URIish("https://github.com/user/repo.git")) + .call(); + + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + assertTrue(!repoUrl.contains(".git"), "URL should not contain .git suffix"); + } + } + + @Test + void testGetRepoUrl_appendsBlobPath_forNonGithubHosts() throws Exception { + git.remoteAdd() + .setName("origin") + .setUri(new URIish("https://example.com/user/repo.git")) + .call(); + + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + String commitHash = git.log().call().iterator().next().getName(); + assertTrue( + repoUrl.endsWith("/blob/" + commitHash + "/"), + "Non-GitHub hosts should get /blob/ path: " + repoUrl); + } + } } diff --git a/cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java b/cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java index 71a7cc4d..2d64b6b1 100644 --- a/cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java +++ b/cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java @@ -11,6 +11,7 @@ import org.apache.maven.project.MavenProject; import org.hjug.refactorfirst.report.CsvReport; import org.hjug.refactorfirst.report.HtmlReport; +import org.hjug.refactorfirst.report.ReportWriter; import org.hjug.refactorfirst.report.SimpleHtmlReport; import org.hjug.refactorfirst.report.json.JsonReportExecutor; import picocli.CommandLine.Command; @@ -92,46 +93,52 @@ public Integer call() { // TODO: add support for inferring arguments from gradle properties inferArgumentsFromMavenProject(); populateDefaultArguments(); - switch (reportType) { - case SIMPLE_HTML: - SimpleHtmlReport simpleHtmlReport = new SimpleHtmlReport(); - simpleHtmlReport.execute( - backEdgeAnalysisCount, - analyzeCycles, - showDetails, - minifiyHtml, - excludeTests, - testSourceDirectory, - projectName, - projectVersion, - baseDir, - outputDirectory); - return 0; - case HTML: - HtmlReport htmlReport = new HtmlReport(); - htmlReport.execute( - backEdgeAnalysisCount, - analyzeCycles, - showDetails, - minifiyHtml, - excludeTests, - testSourceDirectory, - projectName, - projectVersion, - baseDir, - outputDirectory); - return 0; - case JSON: - JsonReportExecutor jsonReportExecutor = new JsonReportExecutor(); - jsonReportExecutor.execute(baseDir, outputDirectory); - return 0; - case CSV: - CsvReport csvReport = new CsvReport(); - csvReport.execute(showDetails, projectName, projectVersion, outputDirectory, baseDir); - return 0; + try { + outputDirectory = ReportWriter.containReportDirectory(baseDir, outputDirectory); + switch (reportType) { + case SIMPLE_HTML: + SimpleHtmlReport simpleHtmlReport = new SimpleHtmlReport(); + simpleHtmlReport.execute( + backEdgeAnalysisCount, + analyzeCycles, + showDetails, + minifiyHtml, + excludeTests, + testSourceDirectory, + projectName, + projectVersion, + baseDir, + outputDirectory); + return 0; + case HTML: + HtmlReport htmlReport = new HtmlReport(); + htmlReport.execute( + backEdgeAnalysisCount, + analyzeCycles, + showDetails, + minifiyHtml, + excludeTests, + testSourceDirectory, + projectName, + projectVersion, + baseDir, + outputDirectory); + return 0; + case JSON: + JsonReportExecutor jsonReportExecutor = new JsonReportExecutor(); + jsonReportExecutor.execute(baseDir, outputDirectory); + return 0; + case CSV: + CsvReport csvReport = new CsvReport(); + csvReport.execute(showDetails, projectName, projectVersion, outputDirectory, baseDir); + return 0; + } + } catch (IllegalArgumentException | ReportWriter.ReportWriteException e) { + log.error("Report generation failed: {}", e.getMessage()); + return 1; } - return 0; + return 1; } private void populateDefaultArguments() { diff --git a/cli/src/test/java/org/hjug/refactorfirst/ReportCommandTest.java b/cli/src/test/java/org/hjug/refactorfirst/ReportCommandTest.java new file mode 100644 index 00000000..9e980457 --- /dev/null +++ b/cli/src/test/java/org/hjug/refactorfirst/ReportCommandTest.java @@ -0,0 +1,50 @@ +package org.hjug.refactorfirst; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import java.lang.reflect.Field; +import java.nio.file.Files; +import java.nio.file.Path; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class ReportCommandTest { + + @Test + void call_returnsFailureWhenOutputPathContainsSymlink(@TempDir Path tempDir) throws Exception { + Path target = Files.createDirectory(tempDir.resolve("target")); + Path link = tempDir.resolve("link"); + try { + Files.createSymbolicLink(link, target); + } catch (UnsupportedOperationException e) { + assumeTrue(false, "Symbolic links not supported on this platform"); + } + + ReportCommand command = new ReportCommand(); + setField(command, "baseDir", tempDir.toFile()); + setField(command, "outputDirectory", "link/reports"); + setField(command, "reportType", ReportType.CSV); + + assertEquals(1, command.call()); + } + + @Test + void call_returnsFailureWhenOutputPathTraversesOutsideBase(@TempDir Path tempDir) throws Exception { + Path baseDirectory = Files.createDirectory(tempDir.resolve("project")); + ReportCommand command = new ReportCommand(); + setField(command, "baseDir", baseDirectory.toFile()); + setField(command, "outputDirectory", "../escape"); + setField(command, "reportType", ReportType.CSV); + + assertEquals(1, command.call()); + assertFalse(Files.exists(tempDir.resolve("escape"))); + } + + private static void setField(ReportCommand command, String name, Object value) throws ReflectiveOperationException { + Field field = ReportCommand.class.getDeclaredField(name); + field.setAccessible(true); + field.set(command, value); + } +} diff --git a/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java b/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java index 70e8d8d9..f5492c74 100644 --- a/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java +++ b/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java @@ -5,6 +5,8 @@ import java.io.File; import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; import java.nio.file.Files; import java.nio.file.Path; import java.util.*; @@ -139,11 +141,18 @@ public List getClassDisharmonies(CodebaseGraphDTO codebaseGr List instances = raw.stream() .map(d -> { + String filePath = classToSourceFilePathMapping.get(d.getClassName()); + if (filePath == null && d.getClassName().contains("$")) { + filePath = classToSourceFilePathMapping.get( + d.getClassName().substring(0, d.getClassName().indexOf("$"))); + } + if (filePath == null) { + log.warn("No source file mapping found for class disharmony in class: {}", d.getClassName()); + } DisharmonyInstance instance = new DisharmonyInstance( disharmonyType, d.getClassName(), - canonicaliseURIStringForRepoLookup( - d.getMetrics().getSourceFilePath().replace("\\", "/")), + filePath, d.getMetrics().getPackageName(), null, new ArrayList<>(d.getMetricValues())); @@ -460,9 +469,27 @@ private String getFileName(RuleViolation violation) { } String canonicaliseURIStringForRepoLookup(String uriString) { - if (repositoryPath.startsWith("/") || repositoryPath.startsWith("\\")) { - return uriString.replace("file://" + repositoryPath.replace("\\", "/") + "/", ""); + return canonicaliseURIStringForRepoLookup(repositoryPath, uriString); + } + + static String canonicaliseURIStringForRepoLookup(String repositoryPath, String uriString) { + try { + URI fileUri = new URI(uriString); + if (!"file".equalsIgnoreCase(fileUri.getScheme())) { + return uriString; + } + if (fileUri.isOpaque()) { + return fileUri.getSchemeSpecificPart().replace("\\", "/"); + } + + Path repository = Path.of(repositoryPath).toAbsolutePath().normalize(); + Path file = Path.of(fileUri).toAbsolutePath().normalize(); + if (file.startsWith(repository)) { + return repository.relativize(file).toString().replace("\\", "/"); + } + } catch (IllegalArgumentException | URISyntaxException e) { + log.debug("Unable to canonicalize file URI {}", uriString, e); } - return uriString.replace("file:///" + repositoryPath.replace("\\", "/") + "/", ""); + return uriString; } } diff --git a/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java b/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java index 870ea4f8..80abbbd6 100644 --- a/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java +++ b/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java @@ -43,6 +43,13 @@ public void tearDown() { repository.close(); } + @Test + void canonicaliseURIStringForRepoLookup_relativizesAbsoluteUnixPath() { + Assertions.assertEquals( + "src/Foo.java", + CostBenefitCalculator.canonicaliseURIStringForRepoLookup("/tmp/repo", "file:///tmp/repo/src/Foo.java")); + } + @Test void testCBOViolation() throws IOException, GitAPIException, InterruptedException { // Has CBO violation diff --git a/graph-data-generator/src/main/java/org/hjug/gdg/GraphDataGenerator.java b/graph-data-generator/src/main/java/org/hjug/gdg/GraphDataGenerator.java index 27616dd4..298d093f 100644 --- a/graph-data-generator/src/main/java/org/hjug/gdg/GraphDataGenerator.java +++ b/graph-data-generator/src/main/java/org/hjug/gdg/GraphDataGenerator.java @@ -42,7 +42,7 @@ public String generateBubbleChartData( RankedDisharmony rankedDisharmony = rankedDisharmonies.get(i); chartData.append("["); chartData.append("'"); - chartData.append(rankedDisharmony.getFileName()); + chartData.append(escapeJavaScriptString(rankedDisharmony.getFileName())); chartData.append("',"); chartData.append(rankedDisharmony.getEffortRank()); chartData.append(","); @@ -58,4 +58,16 @@ public String generateBubbleChartData( } return chartData.toString(); } + + static String escapeJavaScriptString(String value) { + if (value == null) { + return ""; + } + return value.replace("\\", "\\\\") + .replace("'", "\\'") + .replace("\n", "\\n") + .replace("\r", "\\r") + .replace("<", "\\u003C") + .replace(">", "\\u003E"); + } } diff --git a/graph-data-generator/src/test/java/org/hjug/gdg/GraphDataGeneratorTest.java b/graph-data-generator/src/test/java/org/hjug/gdg/GraphDataGeneratorTest.java index 296d1e07..c86ebc44 100644 --- a/graph-data-generator/src/test/java/org/hjug/gdg/GraphDataGeneratorTest.java +++ b/graph-data-generator/src/test/java/org/hjug/gdg/GraphDataGeneratorTest.java @@ -79,6 +79,23 @@ void generateBubbleChartDataForTwoPoints() { assertTrue(data.startsWith("[ 'ID', 'Effort', 'Change Proneness', 'Priority', 'Priority (Visual)']")); } + @Test + void generateBubbleChartData_escapesScriptClosingSequence() { + RankedDisharmony disharmony = makeRankedDisharmony(1); + disharmony.setFileName(""); + + String data = gen.generateBubbleChartData(List.of(disharmony), 1, "Effort"); + + assertFalse(data.contains("")); + assertTrue(data.contains("\\u003C/script\\u003E")); + } + + @Test + void escapeJavaScriptString_escapesQuotesBackslashesAndLineBreaks() { + assertEquals("a\\\\b\\'c\\nd\\re", GraphDataGenerator.escapeJavaScriptString("a\\b'c\nd\re")); + assertEquals("", GraphDataGenerator.escapeJavaScriptString(null)); + } + // ── helper ───────────────────────────────────────────────────────────────── private RankedDisharmony makeRankedDisharmony(int priority) { diff --git a/plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026-implementation-plan.md b/plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026-implementation-plan.md new file mode 100644 index 00000000..356ab95e --- /dev/null +++ b/plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026-implementation-plan.md @@ -0,0 +1,598 @@ +# OpenVuln RefactorFirst — Implementation Plan for All 13 Findings + +This plan addresses all 13 security findings from the OpenVuln report using Test-Driven Development (TDD). Each finding is addressed with failing unit tests written first, then production code to make them pass. + +--- + +## Finding 1: BUG-R2-S2-A1-H1 — HTML Report Origin-URL Embedding (XSS via `remote.origin.url`) + +**Severity:** High (CVSS 7.4) +**Root Cause:** `GitLogReader.getRepoUrl()` returns raw `.git/config` remote URL without validation/escaping; used in unquoted `href`, `javascript:` scheme, and DOT-in-script sinks. + +### Files to Modify +- `../change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java` +- `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` +- `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` + +### TDD Tasks + +#### 1.1 Add tests for `GitLogReader.getRepoUrl()` validation +```java +// change-proneness-ranker/src/test/java/org/hjug/git/GitLogReaderTest.java + +@Test void getRepoUrl_returnsEmpty_whenOriginUrlIsNull() throws IOException +@Test void getRepoUrl_returnsEmpty_whenOriginUrlIsNotHttpOrHttps() throws IOException +@Test void getRepoUrl_returnsEmpty_whenOriginUrlIsJavascriptScheme() throws IOException +@Test void getRepoUrl_returnsEmpty_whenOriginUrlIsDataScheme() throws IOException +@Test void getRepoUrl_returnsEmpty_whenOriginUrlIsFileScheme() throws IOException +@Test void getRepoUrl_sanitizesUrl_removingMarkupCharacters() throws IOException +@Test void getRepoUrl_allowsValidHttpsUrl() throws IOException +@Test void getRepoUrl_allowsValidHttpUrl() throws IOException +@Test void getRepoUrl_stripsGitSuffix() throws IOException +@Test void getRepoUrl_appendsBlobPath_forNonGithubHosts() throws IOException +``` + +#### 1.2 Add tests for HTML report URL escaping +```java +// report/src/test/java/org/hjug/refactorfirst/report/SimpleHtmlReportTest.java + +@Test void printProjectHeader_escapesRepoUrlInHref() throws IOException +@Test void hyperlinkClass_escapesRepoUrlInHref() throws IOException +@Test void renderDisharmonyInfo_escapesRepoUrlInHref() throws IOException +``` + +```java +// report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java + +@Test void hyperlinkClassForDot_escapesRepoUrlInDotUrlAttribute() throws IOException +@Test void generateGraphButtons_escapesRepoUrlInScriptTemplateLiteral() throws IOException +``` + +#### 1.3 Implement fix in `GitLogReader.getRepoUrl()` +- Add scheme allow-list (only `http://` and `https://`) +- Sanitize URL to RFC 3986 characters only +- Return empty string for invalid URLs + +#### 1.4 Implement fix in report renderers +- Quote all `href` attributes +- Apply `escapeHtmlLabel` to all user-controlled strings in HTML context +- For DOT-in-script: escape `$`, `{`, `}`, backtick, `` + +--- + +## Finding 2: BUG-R2-S2-A1-H2 — POM Name/Version XSS in HTML Reports + +**Severity:** High (CVSS 7.9) +**Root Cause:** `projectName` and `projectVersion` from POM interpolated unescaped into `

`, ``, and no-disharmony `<div>`. + +### Files to Modify +- `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` +- `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` + +### TDD Tasks + +#### 2.1 Add tests for POM value escaping +```java +// report/src/test/java/org/hjug/refactorfirst/report/SimpleHtmlReportTest.java + +@Test void printProjectHeader_escapesProjectNameAndVersion() throws IOException +@Test void generateReport_escapesProjectNameInNoDisharmonyDiv() throws IOException +@Test void printTitle_escapesProjectNameAndVersion() throws IOException +``` + +```java +// report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java + +@Test void printTitle_escapesProjectNameAndVersionInTitleTag() throws IOException +``` + +#### 2.2 Implement fix +- Apply `escapeHtmlLabel(projectName)` and `escapeHtmlLabel(projectVersion)` in: + - `SimpleHtmlReport.printProjectHeader()` (line ~926) + - `SimpleHtmlReport.generateReport()` no-disharmony branch (line ~364) + - `HtmlReport.printTitle()` (line ~423) +- Enhance `escapeHtmlLabel` to also escape `"` and `'` + +--- + +## Finding 3: BUG-R2-S2-A1-H3 — File Name/Path XSS in Class-Disharmony Tables + +**Severity:** High (CVSS 7.4) +**Root Cause:** File names and absolute paths from analyzed repo flow unescaped into `<a>` element body and `href` attribute, plus "Full Path" column. + +### Files to Modify +- `../cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java` +- `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` + +### TDD Tasks + +#### 3.1 Add tests for file name/path escaping +```java +// cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java + +@Test void getClassDisharmonies_usesUriPathForSourceFilePath() throws IOException +@Test void canonicaliseURIStringForRepoLookup_handlesRawAbsolutePath() throws IOException +``` + +```java +// report/src/test/java/org/hjug/refactorfirst/report/SimpleHtmlReportTest.java + +@Test void renderDisharmonyInfo_escapesFileNameInAnchorBody() throws IOException +@Test void renderDisharmonyInfo_quotesAndEscapesHrefAttribute() throws IOException +@Test void renderDisharmonyInfo_escapesFullPathInShowDetailsMode() throws IOException +``` + +#### 3.2 Implement fix +- In `CostBenefitCalculator.getClassDisharmonies()`: use `Path.toUri().toString()` instead of raw path +- In `SimpleHtmlReport.renderDisharmonyInfo()`: + - Quote `href` attribute: `href="..."` + - Apply `escapeHtmlLabel()` to file name in anchor body + - Apply `escapeHtmlLabel()` to full path in showDetails mode +- Enhance `escapeHtmlLabel` to escape `"` and `'` + +--- + +## Finding 4: BUG-R2-S2-A1-H4 — Kotlin Class/Cycle/Method XSS in HTML Reports + +**Severity:** High (CVSS 7.4) +**Root Cause:** Kotlin backtick identifiers allow arbitrary characters; cycle names, method signatures, duplicate partners, package names rendered unescaped. + +### Files to Modify +- `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` +- `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` + +### TDD Tasks + +#### 4.1 Add tests for Kotlin identifier escaping +```java +// report/src/test/java/org/hjug/refactorfirst/report/SimpleHtmlReportTest.java + +@Test void getRankedCycleSummaryData_escapesCycleName() throws IOException +@Test void renderSingleCycle_escapesCycleNameInH2() throws IOException +@Test void renderDisharmonyInfo_escapesMethodSignature() throws IOException +@Test void renderDisharmonyInfo_escapesDuplicationPartners() throws IOException +``` + +```java +// report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java + +@Test void renderPackageEdge_escapesPackageNames() throws IOException +@Test void renderPackageVertices_escapesPackageNames() throws IOException +@Test void renderClassVertices_escapesAnonymousLabels() throws IOException +``` + +#### 4.2 Implement fix +- Apply `escapeHtmlLabel()` to: + - Cycle name in `getRankedCycleSummaryData()` (line ~776) + - Cycle name in `renderSingleCycle()` (line ~793) + - Method signature in `renderDisharmonyInfo()` (line ~1045) + - Duplication partners in `renderDisharmonyInfo()` (line ~1068) + - Package vertex names in `renderPackageEdge()` and `renderPackageVertices()` +- Enhance `escapeHtmlLabel` to escape `"` and `'` + +--- + +## Finding 5: BUG-R2-S2-A2-H2 — DOT-in-Script XSS via `remote.origin.url` + +**Severity:** High (CVSS 7.4) +**Root Cause:** `remote.origin.url` embedded raw in JavaScript template literal inside `<script>` block for graph maps. + +### Files to Modify +- `../change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java` (shared with Finding 1) +- `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` + +### TDD Tasks + +#### 5.1 Add tests for DOT-in-script escaping +```java +// report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java + +@Test void hyperlinkClassForDot_escapesRepoUrlForTemplateLiteral() throws IOException +@Test void buildClassGraphDot_escapesUrlInTemplateLiteral() throws IOException +@Test void buildClassCycleDot_escapesUrlInTemplateLiteral() throws IOException +@Test void buildPackageGraphDot_noUrlAttributeWhenNoSourceMapping() throws IOException +@Test void generateGraphButtons_scriptBlockExecutionSafe() throws IOException +``` + +#### 5.2 Implement fix +- Reuse `GitLogReader.getRepoUrl()` fix from Finding 1 (scheme allow-list + RFC 3986 sanitization) +- In `HtmlReport.hyperlinkClassForDot()`: escape `$`, `{`, `}`, backtick for template literal context +- In `buildClassGraphDot()`/`buildClassCycleDot()`: ensure template literal content is safe + +--- + +## Finding 6: BUG-R2-S2-A2-H5 — Cycle Map Visuals XSS via Kotlin Class Names + +**Severity:** High (CVSS 7.4) +**Root Cause:** Cycle name (from Kotlin backtick class names) interpolated into unquoted HTML attributes, popup button bodies, and script blocks. + +### Files to Modify +- `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` + +### TDD Tasks + +#### 6.1 Add tests for cycle map visual escaping +```java +// report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java + +@Test void renderClassCycleVisuals_sanitizesCycleNameForJsIdentifier() throws IOException +@Test void renderClassCycleVisuals_sanitizesCycleNameForHtmlAttribute() throws IOException +@Test void renderClassCycleVisuals_sanitizesCycleNameForElementBody() throws IOException +@Test void generateGraphButtons_escapesCycleNameInConstDeclaration() throws IOException +@Test void generateDotImage_escapesCycleNameInDivIdAndModuleScript() throws IOException +@Test void generate2DPopup_escapesCycleNameInOnclickAndElementBody() throws IOException +@Test void generateForce3DPopup_escapesCycleNameInOnclickAndElementBody() throws IOException +@Test void generateHidePopup_escapesCycleNameInDivIds() throws IOException +@Test void renderClassCycleVisuals_usesFallbackIdentifierForEmptyName() throws IOException +@Test void renderClassCycleVisuals_prefixesIdentifierStartingWithDigit() throws IOException +``` + +#### 6.2 Implement fix +- In `renderClassCycleVisuals()`, derive a separate, non-empty graph identifier using the identifier-safe charset `[A-Za-z0-9_]`, prefixing values that could start with a digit +- Keep the original cycle name separate and HTML-escape it for visible popup/button text +- Apply the graph identifier to identifier-bearing downstream positions: + - JS identifier in `const <name>_dot` + - HTML `id` attributes + - JS string literals in `onclick` +- Include a stable suffix so distinct names that sanitize to the same text do not collide + +--- + +## Finding 7: BUG-R2-S2-A5-H3 — Symlink Following in ReportWriter (Arbitrary File Overwrite) + +**Severity:** High (CVSS 8.1) +**Root Cause:** `ReportWriter.writeReportToDisk()` follows symlinks in output directory and output file path. + +### Files to Modify +- `../report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java` + +### TDD Tasks + +#### 7.1 Add tests for symlink protection +```java +// report/src/test/java/org/hjug/refactorfirst/report/ReportWriterTest.java + +@Test void writeReportToDisk_throwsWhenOutputDirIsSymlink() throws IOException +@Test void writeReportToDisk_throwsWhenOutputFileIsSymlink() throws IOException +@Test void writeReportToDisk_throwsWhenOutputDirIsDanglingSymlink() throws IOException +@Test void writeReportToDisk_throwsWhenOutputFileIsDanglingSymlink() throws IOException +@Test void writeReportToDisk_throwsWhenIntermediateDirectoryIsSymlink() throws IOException +@Test void writeReportToDisk_throwsWhenNestedAncestorIsSymlink() throws IOException +@Test void writeReportToDisk_failsIfPathComponentIsReplacedDuringWrite() throws IOException +@Test void writeReportToDisk_atomicallyReplacesExistingReport() throws IOException +@Test void writeReportToDisk_writesNormallyWhenNoSymlinks() throws IOException +@Test void writeReportToDisk_createsParentDirectories() throws IOException +@Test void reportCommand_returnsFailureWhenReportWriteIsBlocked() throws IOException +``` + +#### 7.2 Implement fix +- Validate or create each path component without following symbolic links, rejecting symlinks in every ancestor as well as the final directory and report file +- Use descriptor-relative `SecureDirectoryStream` operations where supported so a path-component replacement cannot redirect the report write +- Write to a securely created temporary file and atomically rename it into place with no-follow checks +- Log errors early and propagate a controlled write failure so CLI commands return nonzero and Maven mojos fail instead of reporting success + +--- + +## Finding 8: BUG-R2-S2-A2-H1 — Bubble Chart XSS via File Names in JS String Literals + +**Severity:** Medium (CVSS 6.1) +**Root Cause:** File names embedded unescaped in single-quoted JavaScript string literals in Google Charts data table. + +### Files to Modify +- `../graph-data-generator/src/main/java/org/hjug/gdg/GraphDataGenerator.java` + +### TDD Tasks + +#### 8.1 Add tests for JS string escaping +```java +// graph-data-generator/src/test/java/org/hjug/gdg/GraphDataGeneratorTest.java + +@Test void generateBubbleChartData_escapesSingleQuotesInFileName() throws IOException +@Test void generateBubbleChartData_escapesBackslashesInFileName() throws IOException +@Test void generateBubbleChartData_escapesNewlinesInFileName() throws IOException +@Test void generateBubbleChartData_escapesCarriageReturnsInFileName() throws IOException +@Test void generateBubbleChartData_escapesScriptClosingSequenceInFileName() throws IOException +@Test void escapeJavaScriptString_handlesNull() throws IOException +@Test void escapeJavaScriptString_handlesEmpty() throws IOException +``` + +#### 8.2 Implement fix +- Add `escapeJavaScriptString(String value)` method: + - Escape `\` → `\\` + - Escape `'` → `\'` + - Escape `\n` → `\n` + - Escape `\r` → `\r` + - Encode `<` → `\u003C` and `>` → `\u003E` so a file name cannot terminate the enclosing script +- Apply to `rankedDisharmony.getFileName()` in `generateBubbleChartData()` (line ~44-46) + +--- + +## Finding 9: BUG-R2-S2-A2-H4 — Package Map XSS via Kotlin Package Names in Template Literals + +**Severity:** Medium (CVSS 6.9) +**Root Cause:** Kotlin package names (from backtick identifiers) embedded in DOT inside JS template literal with only `.` → `_` replacement. + +### Files to Modify +- `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` + +### TDD Tasks + +#### 9.1 Add tests for package map escaping +```java +// report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java + +@Test void renderPackageVertices_escapesPackageNameForNodeId() throws IOException +@Test void renderPackageVertices_escapesPackageNameForLabel() throws IOException +@Test void renderPackageGraphEdge_escapesPackageNameForEdgeEndpoints() throws IOException +@Test void renderSafePackageNodeId_handlesDollarSign() throws IOException +@Test void renderSafePackageNodeId_handlesQuotes() throws IOException +@Test void renderSafePackageNodeId_handlesBraces() throws IOException +@Test void escapeDotQuoted_escapesBackslash() throws IOException +@Test void escapeDotQuoted_escapesDoubleQuote() throws IOException +@Test void packageGraph_escapesScriptClosingSequence() throws IOException +@Test void packageGraph_escapesBackslashes() throws IOException +@Test void renderSafePackageNodeId_handlesLeadingDigit() throws IOException +@Test void renderSafePackageNodeId_avoidsSanitizationCollisions() throws IOException +``` + +#### 9.2 Implement fix +- Use separate encoders for each output context rather than reusing one partial sanitizer +- Add a stable, collision-resistant DOT node-ID encoder with a non-digit prefix for package edge endpoints and vertices +- Add complete DOT quoted-string escaping for package and class labels, including quotes, backslashes, and control characters +- Encode the completed DOT value for the JavaScript/raw-script template context, including backticks, interpolation markers, backslashes, and `<`/`>` +- Apply these encoders consistently in `buildPackageGraphDot()`, `renderPackageGraphEdge()`, `renderPackageVertices()`, and `renderClassVertices()` + +--- + +## Finding 10: BUG-R2-S2-A3-H1 — CSV Formula Injection via POM Name/Version + +**Severity:** Medium (CVSS 6.1) +**Root Cause:** POM `<name>`/`<version>` written unescaped as cell #1 of every CSV data row. + +### Files to Modify +- `../report/src/main/java/org/hjug/refactorfirst/report/CsvReport.java` + +### TDD Tasks + +#### 10.1 Add tests for CSV formula neutralization +```java +// report/src/test/java/org/hjug/refactorfirst/report/CsvReportTest.java + +@Test void execute_sanitizesProjectVersionInDataRows() throws IOException +@Test void execute_sanitizesProjectNameInNoGitFallback() throws IOException +@Test void execute_sanitizesProjectVersionInNoGitFallback() throws IOException +@Test void execute_sanitizesProjectNameInNoGodClassesFallback() throws IOException +@Test void execute_sanitizesProjectVersionInNoGodClassesFallback() throws IOException +@Test void sanitizeCsvCell_prefixesFormulaTriggersWithApostrophe() throws IOException +@Test void sanitizeCsvCell_handlesEqualsPrefix() throws IOException +@Test void sanitizeCsvCell_handlesPlusPrefix() throws IOException +@Test void sanitizeCsvCell_handlesMinusPrefix() throws IOException +@Test void sanitizeCsvCell_handlesAtPrefix() throws IOException +@Test void sanitizeCsvCell_handlesTabPrefix() throws IOException +@Test void sanitizeCsvCell_handlesCarriageReturnPrefix() throws IOException +@Test void sanitizeCsvCell_handlesEmbeddedNewlineWithFormula() throws IOException +@Test void sanitizeCsvCell_quotesAllValues() throws IOException +@Test void sanitizeCsvCell_escapesEmbeddedQuotes() throws IOException +``` + +#### 10.2 Implement fix +- Define one canonical `sanitizeCsvCell(String value)` method: + - Escape embedded `"` → `""` + - Prefix with `'` if value starts with `=`, `+`, `-`, `@`, `\t`, `\r` (or after `\n`/`\r`) + - Wrap entire value in `"` +- Apply to: + - `projectVersion` in data row loop (line ~121) + - `projectName`/`projectVersion` in no-git fallback (line ~63-70) + - `projectName`/`projectVersion` in no-god-classes fallback (line ~99-100) + - Every cell emitted by `addsRow()` (line ~208), including headers and fallback rows + +--- + +## Finding 11: BUG-R2-S2-A3-H2 — CSV Formula Injection via File Names/Paths + +**Severity:** Medium (CVSS 6.1) +**Root Cause:** File names and paths written unquoted/unneutralized into CSV cells. + +### Files to Modify +- `../report/src/main/java/org/hjug/refactorfirst/report/CsvReport.java` + +### TDD Tasks + +#### 11.1 Add tests for file name/path CSV escaping +```java +// report/src/test/java/org/hjug/refactorfirst/report/CsvReportTest.java + +@Test void getDataList_escapesFileNameInClassCell() throws IOException +@Test void getDataList_escapesFullPathInDetailedMode() throws IOException +@Test void addsRow_escapesAllCellsWithSanitizeCsvCell() throws IOException +@Test void sanitizeCsvCell_quotesAndEscapesFormulaTriggers() throws IOException +@Test void sanitizeCsvCell_escapesEmbeddedQuotes() throws IOException +@Test void sanitizeCsvCell_handlesCommaInValue() throws IOException +@Test void addsRow_sanitizesEachCellExactlyOnce() throws IOException +``` + +#### 11.2 Implement fix +- Reuse the canonical `sanitizeCsvCell(String value)` from Finding 10; do not add a second encoder +- Route file names, paths, project metadata, fallback rows, and every other value through `addsRow()` so each cell is sanitized exactly once + +--- + +## Finding 12: BUG-R2-S2-A5-H1 — Maven Plugin Output Directory Path Traversal + +**Severity:** Medium (CVSS 4.4) +**Root Cause:** Maven mojos use attacker-controlled `<reporting><outputDirectory>` without containment check. + +### Files to Modify +- `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstHtmlReport.java` +- `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstSimpleHtmlReport.java` +- `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstMavenJsonReport.java` +- `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstMavenCsvReport.java` +- `../cli/src/main/java/org/hjug/cli/ReportCommand.java` +- `../report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java` + +### TDD Tasks + +#### 12.1 Add tests for output directory containment +```java +// report/src/test/java/org/hjug/refactorfirst/report/ReportWriterTest.java + +@Test void containReportDirectory_allowsNormalTargetSite() throws IOException +@Test void containReportDirectory_rejectsAbsolutePath() throws IOException +@Test void containReportDirectory_rejectsTraversalPath() throws IOException +@Test void containReportDirectory_rejectsSymlinkTraversal() throws IOException +@Test void containReportDirectory_rejectsEmptyValue_usesDefault() throws IOException +@Test void containReportDirectory_usesBaseDirAsRoot() throws IOException +``` + +```java +// refactor-first-maven-plugin/src/test/java/org/hjug/mavenreport/RefactorFirstHtmlReportTest.java + +@Test void execute_usesContainedOutputDirectory() throws IOException +@Test void resolveOutputDirectory_defaultsWhenReportingIsAbsent() throws IOException +@Test void resolveOutputDirectory_defaultsWhenReportingOutputIsAbsent() throws IOException +@Test void reportCommand_rejectsTraversalBeforeDispatch() throws IOException +@Test void reportCommand_rejectsSymlinkEscapeBeforeDispatch() throws IOException +``` + +#### 12.2 Implement fix +- Add `ReportWriter.containReportDirectory(File baseDir, String configuredDir)`: + - Resolve configured dir against baseDir + - Normalize and verify it starts with baseDir + - Default to `target/site` if empty/null + - Reject existing symbolic links in every path component + - Throw `IllegalArgumentException` if escapes baseDir +- Resolve Maven's effective reporting output directory without dereferencing absent reporting configuration; when reporting or its output directory is absent, use `${project.build.directory}/site` +- Update all 4 mojos to contain the effective directory against `project.getBasedir()` +- Normalize and contain the CLI output directory against its project base before dispatching to any of the four report executors + +--- + +## Finding 13: BUG-R2-S2-A5-H2 — CSV Filename Path Traversal via POM Name/Version + +**Severity:** Medium (CVSS 4.3) +**Root Cause:** CSV filename composed from unsanitized POM `<name>`/`<version>` with path traversal. + +### Files to Modify +- `../report/src/main/java/org/hjug/refactorfirst/report/CsvReport.java` + +### TDD Tasks + +#### 13.1 Add tests for filename sanitization +```java +// report/src/test/java/org/hjug/refactorfirst/report/CsvReportTest.java + +@Test void execute_sanitizesProjectNameInFilename() throws IOException +@Test void execute_sanitizesProjectVersionInFilename() throws IOException +@Test void sanitizeFilenameSegment_replacesPathSeparators() throws IOException +@Test void sanitizeFilenameSegment_replacesTraversalSegments() throws IOException +@Test void sanitizeFilenameSegment_replacesControlCharacters() throws IOException +@Test void sanitizeFilenameSegment_handlesNullOrBlank() throws IOException +``` + +#### 13.2 Implement fix +- Add `sanitizeFilenameSegment(String value)`: + - Replace non-word, non-dot, non-hyphen with `_` + - Replace `..` sequences with `_` + - Return `"unknown"` for null/blank +- Apply to `projectName` and `projectVersion` in filename composition (line ~24-33) + +--- + +## Execution Order (Dependencies) + +### Phase 1: Core Infrastructure (No Dependencies) +1. **Finding 7** (ReportWriter symlink) - Foundation for all file writes +2. **Finding 12** (Maven output directory containment) - Uses ReportWriter helper + +### Phase 2: Input Validation (Shared by Multiple Findings) +3. **Finding 1** (GitLogReader URL validation) - Used by Findings 1, 5 +4. **Finding 2** (POM name/version escaping) - Used by Findings 2, 10, 13 + +### Phase 3: HTML Report XSS Fixes (Depend on Phase 1-2) +5. **Finding 3** (File name/path escaping in tables) +6. **Finding 4** (Kotlin identifiers in cycle/method/package tables) +7. **Finding 5** (DOT-in-script URL escaping) - Reuses Finding 1 fix +8. **Finding 6** (Cycle map visuals Kotlin name sanitization) +9. **Finding 8** (Bubble chart JS string escaping) +10. **Finding 9** (Package map template literal escaping) + +### Phase 4: CSV Fixes (Depend on Phase 2) +11. **Finding 10** (CSV formula injection via POM values) +12. **Finding 11** (CSV formula injection via file names/paths) +13. **Finding 13** (CSV filename path traversal) + +--- + +## Test Infrastructure Requirements + +### Test Dependencies (verify in pom.xml) +- JUnit 5 (Jupiter) +- AssertJ or Hamcrest for assertions +- Temporary directory support (`@TempDir`) +- Mockito for mocking GitLogReader, MavenProject, etc. + +### Test Fixtures Needed +- Malicious `.git/config` with XSS payloads in `remote.origin.url` +- Malicious `../pom.xml` with XSS/formula payloads in `<name>`/`<version>` +- Test Kotlin files with backtick identifiers containing payloads +- Test Java files with malicious file names +- Symlink test fixtures (requires Linux/macOS or Git Bash on Windows) + +### Test Commands +```bash +# Run all tests +mvn clean test + +# Run specific module tests +mvn clean test -pl change-proneness-ranker +mvn clean test -pl report +mvn clean test -pl graph-data-generator +mvn clean test -pl cost-benefit-calculator +mvn clean test -pl refactor-first-maven-plugin + +# Run single test class +mvn clean test -pl report -Dtest=SimpleHtmlReportTest +mvn clean test -pl change-proneness-ranker -Dtest=GitLogReaderTest +``` + +--- + +## Verification Checklist Per Finding + +For each finding, verify: +- [ ] Failing unit tests written first (red) +- [ ] Production code implemented (green) +- [ ] All tests pass (refactor if needed) +- [ ] Integration test with malicious fixture passes +- [ ] `mvn spotless:check` passes +- [ ] `mvn clean install -DskipTests` succeeds + +--- + +## Integration Test Fixtures + +Create test fixtures in `test-resources/src/test/resources/` for each finding: +- `finding-1-git-config-xss/` - `.git/config` with malicious remote URL +- `finding-2-pom-xss/` - `../pom.xml` with `<name><script>...</script></name>` +- `finding-3-filename-xss/` - Java file named `Pwn<img src=x onerror=alert(1)>.java` +- `finding-4-kotlin-xss/` - Kotlin file with backtick class names +- `finding-5-dot-script-xss/` - Combined with finding-1 +- `finding-6-cycle-map-xss/` - Kotlin cycle with payload class names +- `finding-7-symlink/` - Repo with symlink in target/site +- `finding-8-bubble-chart-xss/` - Java file with `'` in name +- `finding-9-package-map-xss/` - Kotlin package with `${...}` payload +- `finding-10-csv-formula-pom/` - POM with `=WEBSERVICE(...)` version +- `finding-11-csv-formula-filename/` - Java file with formula name +- `finding-12-maven-output-traversal/` - POM with `<outputDirectory>../../../tmp</outputDirectory>` +- `finding-13-csv-filename-traversal/` - POM with `<name>evil/../../../tmp/planted</name>` + +--- + +## Final Validation + +After all 13 findings implemented: +1. Run full build: `mvn clean install` +2. Run OWASP dependency check: `mvn clean install -Plocal` +3. Verify all tests pass: `mvn clean test` +4. Verify formatting: `mvn spotless:check` +5. Manual verification with malicious fixtures against CLI and Maven plugin diff --git a/plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026.md b/plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026.md new file mode 100644 index 00000000..34c9aac2 --- /dev/null +++ b/plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026.md @@ -0,0 +1,3952 @@ +# OpenVuln report — refactorfirst/RefactorFirst + +## [high] HTML report origin-URL embedding (GitLogReader.getRepoUrl → unquoted href / DOT-in-script sinks) unvalidated .git/config remote URL leading to stored XSS in the report viewer's browser, with session-class impact when the report is published to an authenticated web origin + +- key: `BUG-R2-S2-A1-H1` +- disclosure: owner_only +- cwe: CWE-79 +- file: `../change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java` + +# HTML report origin-URL embedding (GitLogReader.getRepoUrl → unquoted href / DOT-in-script sinks) unvalidated .git/config remote URL leading to stored XSS in the report viewer's browser, with session-class impact when the report is published to an authenticated web origin + +- **Project:** refactorfirst/RefactorFirst +- **Finding key:** BUG-R2-S2-A1-H1 +- **CWE:** CWE-79 +- **CVSS:** 7.4 (`CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:C/C:H/I:H/A:N`) +- **EV priority:** P1 +- **EV score:** 7 +- **PoC status:** reproduced +- **EXP status:** downgraded +- **Affected versions:** 0.9.0 (latest release, verified: 19 injected sinks per report) through the audited commit + 65d3bef1 (0.10.0-SNAPSHOT); 0.8.0 is not affected (the origin-URL link feature is absent — the raw remote URL appears + nowhere in its report). CLI jars are not a realistic entry point for any tested version (0.8.0/0.9.0 fail to start + with a pre-existing picocli bug, as does the audited commit); the Maven plugin goals are the working entry points + +## Exploitability rationale + +Reachability R:L — the attacker needs control over the analyzed repository's files, specifically its .git/config (its +remote.origin.url). Delivery is verified cheap on all realistic channels: an archive with a pre-built .git (the +.git/config is a plain attacker-authored file), a clone from an attacker-operated git remote (verified over git://: the +markup-bearing URL, spaces included, is stored verbatim into the victim's .git/config), or an attacker superproject's +.gitmodules URL (verified: after a recursive clone the submodule's .git/modules config carries it verbatim and the +report generated inside the submodule injects and executes). Not network-reachable: +the victim must analyze attacker-provided code with the tool — the tool's advertised use case. Exposure E:D — the sink +renders on the default path of every working entry point: the project-header link is emitted unconditionally for both +report types (generateReport:146 -> printProjectHeader:926), even for a repository with zero findings, and the README's +documented flow is the htmlReport/simpleHtmlReport Maven goals. The CLI entry is excluded from exposure: every released +CLI jar (0.8.0/0.9.0 verified) and the audited commit fail to start (pre-existing picocli duplicate --output option), so +the CLI is not a realistic vector. Certainty C:D — pure deterministic string concatenation; a hostile remote.origin.url +reproduces the injected markup every time (re-confirmed on both delivery-path reports: 20 live +<img onerror> elements, 20 dialogs on open). Impact I:S — the realistically achievable top impact is session hijack, not +code execution: on the local file:// view (the documented default) the payload achieves attacker-controlled rendering in +a trusted context (phishing/redirect, proven) plus beacon exfiltration of the report content, while browsers block +local-file reads (proven) and there is no cookie surface; on the GitHub step-summary flow (documented CI flow) GitHub's +sanitizer strips every script shape (verified by simulating the documented allowlist) leaving at most an attacker-chosen +link and a camo-proxied beacon; session-class compromise (reading the hosting origin's data as every viewer, HttpOnly +notwithstanding) is real but requires the victim environment to publish the report into an authenticated web origin — a +standard CI practice for Maven HTML reports that the tool itself does not perform. + +## Code anchors + +| File | Line | Function | +|-----------------------------------------------------------------------------------------------|-----:|------------------------| +| `../change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java` | 85 | `getOriginUrl` | +| `../change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java` | 88 | `getRepoUrl` | +| `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` | 146 | `generateReport` | +| `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` | 926 | `printProjectHeader` | +| `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` | 756 | `hyperlinkClass` | +| `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` | 1037 | `renderDisharmonyInfo` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 631 | `hyperlinkClassForDot` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 516 | `generateGraphButtons` | +| `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstHtmlReport.java` | 60 | `execute` | +| `../cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java` | 96 | `call` | + +## Background + +RefactorFirst is a developer/CI-side static-analysis tool (Maven plugin, Gradle plugin, CLI) that mines a project's git +history with JGit, ranks design "disharmonies" +(God Classes, Cycles, ...) by change-proneness vs. effort, and emits an HTML report +(`target/site/refactor-first-report.html`). The report is meant to be opened in a browser and shared; the README +documents viewing it locally and appending it to +`$GITHUB_STEP_SUMMARY` in GitHub Actions. To let readers jump from a finding to the offending source file, the report +generator builds source links from the analyzed repository's git remote: `remote.origin.url` is read from the +repository's `.git/config` +and used as the URL prefix for every class link, disharmony row, and graph node in the report. That remote URL is a +free-form config string that is not part of the repository content checked into a hosting platform, but it *is* part of +what an attacker controls whenever the analyzed project originates from the attacker (a repository archive including +`.git`, a clone from an attacker-operated remote, or a submodule whose URL comes from the attacker-authored +`.gitmodules`). Analyzing third-party code is the tool's advertised primary use case, which puts this input on the +tool's main untrusted input channel. + +## Description + +`GitLogReader.getOriginUrl()` returns `gitRepository.getConfig().getString("remote", +"origin", "url")` — the raw INI value from the analyzed repository's `.git/config`, returned verbatim by JGit. +`GitLogReader.getRepoUrl()` then applies only three cosmetic transforms: a `git@`→`https://` rewrite (only when the +string starts with +`git@`), a global `.git` substring removal, and a `/blob|/-/blob|/src/<commit-hash>/` +suffix append. There is no scheme allow-list, no character-set validation, and no HTML/JS escaping anywhere on the path. +The resulting string is concatenated raw into markup-significant positions of both HTML report types: + +1. **Unquoted `href` attribute** — `printProjectHeader` (SimpleHtmlReport.java:926) + emits `<a href=` + repoUrl + ` target="_blank">` as the report header, unconditionally for both report types + (`generateReport` calls it at :146 before any analysis outcome is known, so it renders even for a repo with zero + findings). The same unquoted shape is used by `hyperlinkClass` (:756) for the class/package/cycle relationship tables + and by `renderDisharmonyInfo` (:1037-1038) for every row of all disharmony tables. In the HTML tokenizer's + unquoted-attribute-value state the value terminates at the first whitespace or `>`: a space starts a new attribute + (event-handler injection, e.g. `onclick=`/`onfocus=… autofocus`), and a `>` closes the start tag so following markup + such as `<img src=x onerror=alert(1)>` is parsed as a live element that fires on page load with no click. +2. **`javascript:` scheme injection** — a `remote.origin.url` of `javascript:alert(1)//` + survives all three transforms (no `git@` prefix, no `.git` substring) and the appended `/blob/<hash>/` falls behind + the JS `//` line comment, yielding a header link `href=javascript:alert(1)//blob/<hash>/` that executes on click. No + scheme allow-list exists. +3. **Quoted DOT `URL` attribute inside a `<script>` template literal** — + `HtmlReport.hyperlinkClassForDot` (:631) emits `URL="<repoUrl><path>"` into the DOT graph string that + `generateGraphButtons` (:516-520) embeds into + `<script>const X_dot = \`strict digraph G {…}\`;</script>` for the class map, every + cycle map, and the package map of the full report. A `</script>` sequence in the URL + terminates the script element at HTML parse time (the script-data state ignores JS + string context), so the rest of the payload is parsed as live HTML — again + zero-interaction script execution. A backtick or `${` additionally breaks out of the JS template literal itself. + +No defense exists downstream: the only escaping helper in the module, +`escapeHtmlLabel` (SimpleHtmlReport.java:765-767, escapes `& < >`), is applied solely to class-name labels and never to +the URL or any attribute value; `drawTableCell` +(:875-880) does no escaping; the report has no Content-Security-Policy meta tag (`printHead` returns "") while loading ~ +8 CDN scripts, so script execution is fully enabled; `ReportWriter.writeReportToDisk` writes the HTML verbatim. The +optional +`minifyHtml` post-pass is off by default in both entry points (Maven mojo field initializer and CLI +`defaultValue="false"`) and is an HTML-parser-based minifier, not an escaper — it re-serializes injected +attributes/elements as markup rather than neutralizing them. GitHub's step-summary surface does sanitize pasted HTML, +but the primary documented flow (opening the local report file) and any web-hosted report (raw-content hosting, internal +quality dashboards, CI artifacts served over HTTP) are unsanitized. + +Impact is surface-dependent (all surfaces measured dynamically): on the local file:// view (the README's default flow) +the injected script executes with zero interaction but browsers block file:// subresource reads (verified: fetch ( +'file:///…') fails with TypeError) and file:// origins carry no cookies, so the realized impact is attacker-controlled +rendering in a trusted context (phishing/redirect — verified) +plus beacon exfiltration of the report's own content to attacker infrastructure (verified); on the README-documented $ +GITHUB_STEP_SUMMARY flow GitHub's user-content sanitizer strips every script shape (verified by simulating the +documented allowlist: +event handlers, <script> blocks and javascript: hrefs all removed), leaving at most an attacker-chosen link and a +camo-proxied image beacon; session-class compromise — the payload reading the hosting origin's data as the signed-in +viewer, HttpOnly notwithstanding, and propagating to every viewer of the shared artifact — is real on web origins that +host the report with sessions (verified on a simulated authenticated report-hosting origin), the standard CI pattern for +publishing Maven HTML reports, but it requires a victim-side publishing step the tool does not perform. + +CVSS v3.1 derivation — AV:L: the victim must process attacker-provided local files (the malicious repository/archive) +with a local tool; AC:H: the session-class worst case depends on a condition beyond the attacker's control (the victim +environment must publish the generated report into an authenticated web origin; the documented default flows do not); +PR:N: no privileges on the victim system; UI:R: the victim must run a report goal on the attacker-influenced repository +and open (or host) the generated HTML; S:C: the payload crosses from the tool's file output into the browser's security +domain (XSS); C:H/I:H: on hosting origins the payload acts as every viewing user and reads everything that origin +exposes (verified); A:N: no availability impact beyond the report page itself. → 7.4 (high, conditional). + +## Attack + +The attacker is the provider of the codebase being analyzed — the tool's advertised use case includes analyzing +third-party code. Delivery paths that put a hostile +`remote.origin.url` into the victim's `.git/config` (all verified dynamically from the victim's side): (1) distribute +the project as an archive that includes a pre-built `.git` directory (vendor drop, file share, email attachment) — +`.git/config` +is a plain attacker-authored file with no git-client sanitization; (2) induce the victim to clone from an +attacker-operated remote whose URL string itself carries the payload — verified over git://: a URL like +`git://attacker/r><img src=x onerror=…>` clones successfully and git stores it verbatim, spaces included (over http (s) +curl rejects space-bearing URLs, but space-free `<>` markup — e.g. inline `<script>` shapes — still clones and stores +verbatim; percent-encoded URLs do not inject because the tool performs no decoding); (3) a superproject whose tracked +`.gitmodules` supplies the submodule origin URL — verified: after a recursive clone the submodule's +`.git/modules/<name>/config` +carries the payload verbatim and the report generated inside the submodule directory (JGit's findGitDir follows the +gitdir pointer) injects and executes it. A CI checkout of a fork PR does not deliver the payload (the checkout keeps the +base repo's origin URL). The victim then runs the tool's normal workflow — +`mvn org.hjug.refactorfirst.plugin:refactor-first-maven-plugin:htmlReport` (or +`simpleHtmlReport`) — and opens the generated report, which the tool itself advertises ("View the report at +target/site/refactor-first-report.html"). The CLI entry funnels into the same renderers but is not a realistic vector: +every released CLI jar (0.8.0/0.9.0 verified) and the audited commit fail to start with a pre-existing, unrelated +picocli bug (duplicate `--output` option); the working entry points are the Maven goals. The attacker also fully +controls the repository content, so the table and graph sinks are guaranteed non-empty; the header sink fires even for +an empty analysis. + +What the payload achieves depends on where the report is consumed (all surfaces measured): on the local file:// view +(the documented default) it renders attacker-controlled content in a trusted context — phishing/redirect works +(verified), report-content beacon exfiltration to attacker infrastructure works (verified), but local-file reads are +blocked by the browser (verified) and there is no cookie surface; on the README-documented $GITHUB_STEP_SUMMARY CI flow +GitHub's sanitizer neutralizes the XSS (no script shape survives; residual: attacker-chosen link plus a camo-proxied +image beacon); when the report is published to a web origin — the standard CI pattern for Maven HTML reports +(Jenkins-style HTML report publishing, artifact viewers, internal quality dashboards) and a pattern the project itself +demonstrates by hosting its JUnit 4 sample report on a raw-CDN — the payload runs in that origin for every viewer with +full XSS capability: verified on a simulated authenticated report-hosting origin, where the payload retrieved the +origin's protected data using each viewer's session (HttpOnly notwithstanding — the payload's same-origin fetch rides +the session), read a non-HttpOnly cookie, and exfiltrated everything to attacker infrastructure; on session-less hosting +origins the realized impact is arbitrary rendering for every viewer plus direct viewer-IP/User-Agent disclosure through +beacons. + +### Payload + +The payload is a single line in the analyzed repository's `.git/config`: +`[remote "origin"] url = <payload>`. Working shapes, all avoiding the `git@` +prefix and the `.git` substring so the cosmetic transforms leave them intact (dynamically reproduced — see poc/poc.md +and poc/evidence/): +(a) scheme injection: `javascript:alert(document.domain)//` — the appended +`/blob/<hash>/` lands behind a JS line comment, producing a click-to-execute link in the report header (and in every +table row / DOT node URL); (b) attribute/element injection via unquoted href: +`https://x/y><img src=x onerror=alert(document.domain)>` — the first `>` closes the `<a` start tag and the +`<img onerror>` fires when the report is merely opened (verified: 20 alert dialogs on load per report in Chrome); (c) +space-separated event handler: `https://x/y onclick=alert(document.domain)//` +— the space ends the unquoted href value and `onclick=…` becomes a live handler attribute on the `<a>`; the trailing +`//` keeps the tool-appended +`/blob/<hash>/` inside a JS line comment so the handler stays executable (verified: 1 alert on clicking the header +link); note that git/JGit truncate an UNQUOTED config value at the first `;`/`#`, so `;`-bearing payloads must avoid +those characters or wrap the value in INI quotes (`url = "…;//"` round-trips verbatim — reproduced with fixture F1-p7); +(d) script-block breakout for the full report's graph sections: +`</script><img src=x onerror=alert(document.domain)>` — the `</script>` sequence ends the +`<script>const X_dot = \`…\`</script>` block at HTML parse time and the +injected element executes without any click (verified: 28 dialogs on load); +a backtick (`` `+alert (…)+` ``) or `${…}` in the URL instead breaks out of the JS +template literal itself and executes during script evaluation (verified: 8 +dialogs on load). All shapes require nothing more than the victim opening the +generated `refactor-first-report.html`. Payload grammar in the unquoted-attribute +sinks: the injected handler expression must contain no spaces, no `>` and no quotes +(the unquoted attribute value ends at the first whitespace or `>`, so arrow +functions, `new X` and `return X` are unusable there); full data-theft logic remains +expressible with nested `.then (function (r){…})` chains and fetch/Image beacons — a +payload of this shape was used to steal an authenticated hosting origin's data as the +viewing victim. The DOT-in-`<script>` sinks of the full report have no such constraint. + +## Data flow + +### Step 1 — `refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstHtmlReport.java:60-77` + +Victim runs the htmlReport/simpleHtmlReport mojo on the analyzed project (baseDir = project basedir); the CLI entry +(cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java:96-125, default report type HTML) funnels into the same +renderers. + +### Step 2 — `change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java:84-85` + +getOriginUrl () returns gitRepository.getConfig ().getString ("remote", "origin", "url") — the raw, free-form +remote.origin.url value from the analyzed repository's .git/config (JGit returns it verbatim). + +### Step 3 — `change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java:88-108` + +getRepoUrl () applies only cosmetic transforms — git@→https:// rewrite (only for strings starting with git@), global +.git substring removal, /blob|/-/blob|/src/<hash>/ suffix append. No scheme allow-list, no character validation, no +escaping. + +### Step 4 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:360,417-419` + +generateReport () obtains the raw repoUrl via getRepoUrl (projectBaseDir) and passes the unmodified string into all +render helpers; printProjectHeader () re-reads it the same way at :918. + +### Step 5 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:926` + +Sink 1 — printProjectHeader () emits <a href= + repoUrl + ` target="_blank">` as an unquoted attribute value; called +unconditionally at :146 for both report types. First whitespace or > in repoUrl breaks out of the attribute or the +element (event-handler injection / element injection with onerror). + +### Step 6 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:756,1037-1038` + +Sinks 2/3 — hyperlinkClass () (class/package/cycle relationship tables) and renderDisharmonyInfo () (every disharmony +row of all 14 tables) emit the same unquoted-href concatenation; drawTableCell (:875-880) adds no escaping. + +### Step 7 — `report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java:631,516-520` + +Sink 4 — hyperlinkClassForDot () emits URL="<repoUrl><path>" into the DOT graph string; generateGraphButtons () embeds +that string inside <script>const X_dot = `…`;</script> (class map :489, cycle maps :923, package map :972). A </script> +sequence in repoUrl terminates the script element at HTML parse time and the following markup executes; javascript: URLs +also flow here. + +### Step 8 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:107-112` + +Optional minifyHtml post-pass (default false in the Maven mojo and the CLI) — an HTML minifier, not an escaper; no +escaping is applied at any stage. + +### Step 9 — `report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java:14-42` + +writeReportToDisk () writes the HTML verbatim to target/site/refactor-first-report.html; the report has no +Content-Security-Policy and loads CDN scripts, so the injected markup/JavaScript executes when the victim opens the +file. + +## Fix / patch notes + +diff --git a/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java +b/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java --- +a/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java +++ +b/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java @@ -94,6 +94,11 @@ public String getRepoUrl () +throws IOException { + + if (originUrl == null) { + return ""; + } + ++ // Only web URLs may be embedded in generated reports as source links; ++ // anything else (javascript:, data:, file:, ...) is rejected outright. ++ if (!originUrl.startsWith("https://") && !originUrl.startsWith("http://")) { ++ return ""; ++ } + + repoUrl = originUrl.replace(".git", ""); + +@@ -105,5 +110,8 @@ public String getRepoUrl () throws IOException { } else { repoUrl = repoUrl + "/blob/" + +getCurrentCommitHash () + "/"; } + +- return repoUrl; + ++ // Keep only RFC 3986 URL characters: drops every char that is markup- or ++ // JS-significant outside a URL (space, ", ', <, >, `, {, }, \), so the value ++ // cannot break out of an attribute or a <script> template literal downstream. ++ return repoUrl.replaceAll("[^A-Za-z0-9._~:/?#\[\]@!$&'()*+,;=%-]", ""); + } + +## References + +- https://cwe.mitre.org/data/definitions/79.html +- https://cwe.mitre.org/data/definitions/80.html +- https://cwe.mitre.org/data/definitions/20.html +- https://owasp.org/www-community/attacks/xss/ +- https://html.spec.whatwg.org/multipage/parsing.html#attribute-value-(unquoted)-state +- https://github.com/advisories/GHSA-8rr6-2qw5-pc7r + +--- + +_Rendered from original VulnHunter / VulnForge `report.yaml` by OpenVuln._ + +## [high] HTML report generator interpolates analyzed-repo POM project name/version unescaped into report markup (h1 header / title RCDATA / no-disharmony div of htmlReport, simpleHtmlReport and the mvn site goal), enabling stored XSS in the report viewer's browser, with session-class impact when the report is published to an authenticated web origin + +- key: `BUG-R2-S2-A1-H2` +- disclosure: owner_only +- cwe: CWE-79 +- file: `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` + +# HTML report generator interpolates analyzed-repo POM project name/version unescaped into report markup (h1 header / title RCDATA / no-disharmony div of htmlReport, simpleHtmlReport and the mvn site goal), enabling stored XSS in the report viewer's browser, with session-class impact when the report is published to an authenticated web origin + +- **Project:** refactorfirst/RefactorFirst +- **Finding key:** BUG-R2-S2-A1-H2 +- **CWE:** CWE-79 +- **CVSS:** 7.9 (`CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:C/C:H/I:H/A:N`) +- **EV priority:** P1 +- **EV score:** 7 +- **PoC status:** reproduced +- **EXP status:** downgraded +- **Affected versions:** All released versions exposing the htmlReport/simpleHtmlReport goals are affected (measured: + 0.5.0, 0.6.2, 0.8.0, 0.9.0 — the README-pinned version included; 0.4.0 predates the goals), plus the audited + 0.10.0-SNAPSHOT (commit 65d3bef; the Mojo htmlReport/simpleHtmlReport goals and the mvn-site report goal reproduce the + XSS pristine). The CLI carries the same POM-inference dataflow and is affected where it can start: cli 0.6.2 verified + affected (6 payload occurrences in the generated full report); cli 0.7.0/0.7.1/0.8.0/0.9.0 and the audited snapshot's + CLI cannot start at all (pre-existing picocli DuplicateOptionAnnotationsException: the long option --output is + registered on two picocli fields, ReportCommand.java:56/72), so the Maven goals are the realistic entry points for + current versions. Full reproduction record: findings/BUG-R2-S2-A1-H2/poc/poc.md; real-scenario impact assessment: + findings/BUG-R2-S2-A1-H2/exp/exp.md. + +## Exploitability rationale + +Reachability R:L — the attacker must be the author of the repository the victim analyzes (file-control delivery), but +this is the cheapest channel of the audit's report-XSS family: the payload is two lines of ordinary pom.xml text +(XML-entity encoded, accepted by Maven model building with BUILD SUCCESS), rides in repository CONTENT that survives +every delivery method — any git host, any mirror, a platform +"Download ZIP" (which strips .git entirely), vendor archives — and the same attacker-authored POM can additionally bind +report generation into the victim's routine `mvn verify`/`mvn site` (README "As Part of a Build" pattern; verified for +released 0.9.0 and the audited snapshot: a plain `mvn verify` on the clone generated the poisoned report with no +report-goal invocation). Not unattended-network- reachable: the victim must run the tool on attacker-provided code — the +tool's advertised use case. Exposure E:D — the h1 header sink renders on the default path of every working entry point +of every affected version: the audited snapshot's htmlReport/simpleHtmlReport/mvn-site goals (pristine) and the released +plugin 0.5.0/0.6.2/0.8.0/0.9.0 (measured: 6 payload occurrences per full report, 4 per simple report); the CLI is +excluded from exposure — released CLI 0.7.0+ and the audited commit cannot start (pre-existing picocli duplicate +--output option); only cli 0.6.2 boots, and it is affected via POM inference. Certainty C:D — deterministic string +concatenation; reproduced every time (42/42 collector beacons, 28/28 server-side session ride-alongs across 3 artifact +shapes x 2 viewers, 2/2 zero-interaction dialogs on the git://-delivered artifact). Impact I:S — the realistically +achievable top impact is session hijack, not host code execution: on the local file:// view (the documented default) the +payload achieves attacker-controlled rendering in a trusted context (phishing/redirect + report- content beacon, proven) +while browsers block local-file reads (proven) and there is no cookie surface; on the GitHub step-summary flow +(documented CI flow) GitHub's sanitizer strips every script shape (verified by simulating the documented allowlist), +leaving at most an attacker-chosen link and a camo-proxied beacon; session-class compromise (reading the hosting +origin's data as every viewer, HttpOnly notwithstanding — proven on a simulated Jenkins-HTML-Publisher-style dashboard +for all three artifact shapes, including the mvn site page, which adds the site skin's raw ${project.version} "Version:" +line as an extra sink) is real but requires the victim environment to publish the report into an authenticated web +origin — a standard CI practice for Maven HTML reports that the tool itself does not perform. + +## Code anchors + +| File | Line | Function | +|-----------------------------------------------------------------------------------------------------|-----:|----------------------------------| +| `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` | 926 | `printProjectHeader` | +| `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` | 364 | `generateReport` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 423 | `printTitle` | +| `../cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java` | 169 | `inferArgumentsFromMavenProject` | +| `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstHtmlReport.java` | 44 | `—` | +| `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstSimpleHtmlReport.java` | 44 | `—` | +| `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstMavenReport.java` | 86 | `executeReport` | + +## Background + +RefactorFirst is a static-analysis tool for Java/Kotlin codebases. A developer or CI job runs it against a codebase +(sources + .git history + build files) and it produces a single-file HTML report ranking refactor priorities (cycles, +God classes, etc.). It is consumed three ways: a standalone CLI (`org.hjug.refactorfirst.Main`), direct Maven goals +(`refactorfirst:htmlReport`, `refactorfirst:simpleHtmlReport`), and the `mvn site` report goal +(`RefactorFirstMavenReport`). The report is a persistent artifact (`target/site/refactor-first-report.html`) that the +README explicitly tells users to open in a browser after generation, that the project itself demonstrates hosting on a +script-executing web origin (a rawcdn.githack.com sample report), and that a documented CI flow appends to +`$GITHUB_STEP_SUMMARY`. The security-relevant trust boundary is therefore "analyzed repository → report → whoever views +the report": the tool's own pitch ("This command will analyze Maven and non-Maven projects") means the analyzed +repository — including its `../pom.xml` text — is routinely third-party/untrusted content, while the generated report is +opened by the analyst in a browser. The report generator (`SimpleHtmlReport`/`HtmlReport`) assembles HTML by raw +StringBuilder concatenation and does escape some fields (`escapeHtmlLabel` for class-name labels), but not all. + +## Description + +The analyzed repository's `<name>` and `<version>` POM values — free-form XML text content under the attacker's full +control in the "analyze a third-party repo" scenario — flow into HTML element bodies of the generated report with no +escaping, validation, or length restriction at any point. Sources: (1) CLI — +`ReportCommand.inferArgumentsFromMavenProject()` silently parses `baseDir/pom.xml` with `MavenXpp3Reader` and takes +`project.getName()`/`project.getVersion()` when the optional `-p`/`-v` flags are unset (the default usage); the CLI only +parses the POM as data, it does not execute the analyzed project's build. (2) Maven goals — `RefactorFirstHtmlReport`/ +`RefactorFirstSimpleHtmlReport` declare `@Parameter(defaultValue = "${project.name}")`/`${project.version}`, so running +the goal inside a cloned repo interpolates the attacker's POM values verbatim; direct goal invocation by fully-qualified +coordinates executes only RefactorFirst's own code. (3) The `mvn site` goal `RefactorFirstMavenReport` has the same +defaults and pipes `htmlReport.generateReport(...)` (which contains the same raw header) into `mainSink.rawText(...)`, +which Doxia emits unescaped — only the site goal's `<title>` is escaped, via `mainSink.text(...)`. Sinks (all raw +concatenations, reached on the default code path of every report): `SimpleHtmlReport.printProjectHeader()` builds the +`<h1>` that heads every report of both types with +`"<a href=" + repoUrl + " target=\"_blank\">" + projectName + " " + projectVersion + "</a></h1>"` — the values sit in +element bodies, so `<script>` executes with no breakout; the no-disharmony status branch appends `projectName`/ +`projectVersion` raw into a `<div>` body ("Congratulations! ... has no Cycles or Disharmonies!"); and +`HtmlReport.printTitle()` interpolates them into `<title>` (RCDATA — a `` prefix in the payload breaks out and +the following ``-shaped class name therefore never reaches any report, verified end-to-end: zero +occurrences, 'has no Cycles or Disharmonies'), '.' (getClassName ()'s last-dot split truncates the rendered name), and +'?' (the same FQN conversion strips '?' bytes verbatim — the delivered payload loses exactly its '?' characters). None +of these bound attacker capability: full exfiltration logic is expressible with bracket notation, +String.fromCharCode-built URLs and fetch chains — a payload carrying a same-origin authenticated ride-along fetch plus a +cross-origin beacon executed from the cycle-summary cell (all constraints respected). + +## Data flow + +### Step 1 — + +`codebase-graph-builder/src/main/java/org/hjug/graphbuilder/graphbuilder/KotlinSourceFileGraphBuilder.java:52-116` + +buildGraph creates rewrite-kotlin's KotlinParser (rewrite-kotlin 8.90.4, pinned via rewrite-bom 8.90.4 in +codebase-graph-builder/pom.xml:19-23), walks the whole project directory for *.kt/*.kts (only the configured test +directory is excluded; excludeTests defaults to true, testSourceDirectory defaults to src/test) and parses each file — +KotlinParser.parse runs the full Kotlin FIR frontend (buildFirFromKtFiles + AnalyseKt.runResolution) on every parse, so +type attribution is always attempted; even ParseError partial trees are subsequently visited (:92-104). + +### Step 2 — + +`dependency: org/openrewrite/kotlin/internal/KotlinTreeParserVisitor.java:3833-3851 (rewrite-kotlin 8.90.4)` + +createIdentifier takes the raw PSI token text of a declaration name — for a backticked identifier that text includes the +backticks — and, when it starts with a backtick, strips exactly the first and last character and stores the inner text +verbatim as the J.Identifier simple name (marking it Quoted for re-printing). So 'class `A ` +' yields J.ClassDeclaration.getSimpleName () == 'A '. + +### Step 3 — + +`dependency: org/jetbrains/kotlin/name/Name.java:61-63 and org/jetbrains/kotlin/psi/psiUtil/ClassIdCalculator.kt (kotlin-compiler-embeddable 2.4.10, pinned by rewrite-kotlin)` + +The FIR name chain applies no character validation: KtNamedDeclarationStub.getName () returns +KtPsiUtil.unquoteIdentifier (text) (backticks stripped); Name.identifier (String) is a bare constructor — the +isValidIdentifier check that rejects leading '<' exists but is never invoked on this path; ClassIdCalculator builds the +ClassId via FqName.fromSegments (names) = names.joinToString (".") with no validation. The class symbol for the +payload-named class is created normally by FIR resolution. + +### Step 4 — + +`dependency: org/openrewrite/kotlin/KotlinTypeSignatureBuilder.kt:712-731 and org/openrewrite/kotlin/KotlinTypeMapping.kt:396-455 (rewrite-kotlin 8.90.4)` + +convertClassIdToFqn (classId) → convertKotlinFqToJavaFq applies only '.'→'$', '/'→'.' and '?'-stripping; spaces, '<', '> +', '=' and quotes pass through untouched. KotlinTypeMapping.classType hands the resulting string to +typeFactory.computeClass (fqn, …), and JavaTypeFactory stores it verbatim: JavaType.Class.fullyQualifiedName == +'p1.A '. + +### Step 5 — `codebase-graph-builder/src/main/java/org/hjug/graphbuilder/visitor/KotlinDependencyVisitor.java:99-118` + +visitCompilationUnit registers every top-level Kotlin class as a class-graph vertex: attributed branch uses +type.getFullyQualifiedName () (payload FQN from step 4); un-attributed fallback (:106-118) builds pkg + '.' + +jcd.getSimpleName () (raw payload from step 2) — no identifier validation on either path, in contrast to the Java +visitor's fallback (UnattributedTypeFqnResolver.resolve:84) which rejects non-[A-Za-z_$][A-Za-z0-9_$]* names. +registerClassVertex and addClassDependency (GraphDependencyCollector) perform no name filtering; the payload class +survives finalizeDto's removeClassesNotInCodebase because its derived package 'p1' is a declared package. + +### Step 6 — `cost-benefit-calculator/src/main/java/org/hjug/cbc/CycleRanker.java:77-86` + +identifyRankedCycles detects cycles with JGraphT's CycleDetector and creates RankedCycle (vertex, subGraph.vertexSet +(), …) — the cycle name is the representative cycle vertex's FQN, i.e. 'p1.A ' when both +members of the 2-class cycle carry the payload name (dedup keeps one entry per cycle). Cycle edges come from the payload +FQNs themselves via DependencyVisitorLogic.handleVariableDeclarations/handleMethodDeclaration → +BaseTypeProcessor.processType → TypeDependencyExtractor (no filtering). + +### Step 7 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:773-781 and 875-881` + +getRankedCycleSummaryData returns getClassName (rankedCycle.getCycleName ()) — substring-after-last-dot only, so the +payload segment survives — and renderClassCycleSummary feeds it to drawTableCell, which emits '' + +rowData + '' with zero escaping. Rendered whenever any class cycle exists (analyzeCycles default true in CLI +ReportCommand.java:34-38 and both Mojos). HtmlReport extends SimpleHtmlReport, so every HTML entry point inherits the +sink. + +### Step 8 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:792-795` + +renderSingleCycle appends getClassName (cycle.getCycleName ()) raw into '

Largest +Class Cycle : …

' — element-body position of the top-ranked cycle's heading; the injected +is parsed as live markup and executes on page load with no interaction. No CSP exists anywhere in the generated HTML; +minifyHtml defaults to false and minify-html is a minifier, not a sanitizer. + +### Step 9 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:1042-1048 and 1067-1069` + +Secondary sinks: the method-signature cell escapes only '<'/'>' (MetricsVisitorLogic.buildMethodSignature:727 +concatenates method.getSimpleName () raw, so backtick method names carry the payload), and the Duplicate Partners cell +renders DisharmonyDetector's partner string (sigA + ' ↔ ' + simpleB + '.' + sigB, :635) raw apart from a ';'-to-
+substitution — the same signature that is partially escaped in its own cell arrives unescaped here. renderPackageEdge (: +725-748) likewise renders package vertex names raw. + +## Fix / patch notes + +diff --git a/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java +b/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java --- +a/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java +++ +b/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java @@ -772,7 +772,7 @@ private String[] +getRankedCycleSummaryData (RankedCycle rankedCycle) { return new String[] { // "Cycle Name", "Priority", "Class Count", +"Relationship Count" + +- getClassName(rankedCycle.getCycleName()), + ++ escapeHtmlLabel(getClassName(rankedCycle.getCycleName())), + rankedCycle.getPriority().toString(), + String.valueOf(rankedCycle.getCycleNodes().size()), + String.valueOf(rankedCycle.getEdgeSet().size()) + +@@ -790,7 +790,7 @@ stringBuilder .append ("

Largest Class Cycle : ") + +- .append(getClassName(cycle.getCycleName())) + ++ .append(escapeHtmlLabel(getClassName(cycle.getCycleName()))) + .append("

\n"); + +@@ -730,15 +730,15 @@ String startVertex = vertexes[0].trim (); String start; if (packagesToRemove.contains +(startVertex)) { + +- start = startVertex + "*"; + ++ start = escapeHtmlLabel(startVertex) + "*"; + } else { + +- start = startVertex; + ++ start = escapeHtmlLabel(startVertex); + } + + String endVertex = vertexes[1].trim (); String end; if (packagesToRemove.contains (endVertex)) { + +- end = endVertex + "*"; + ++ end = escapeHtmlLabel(endVertex) + "*"; + } else { + +- end = endVertex; + ++ end = escapeHtmlLabel(endVertex); + } @@ -1042,7 +1042,7 @@ } + +- sb.append(drawTableCell(sig != null ? sig.replace("<", "<").replace(">", ">") : "")); + ++ sb.append(drawTableCell(sig != null ? escapeHtmlLabel(sig) : "")); + +@@ -1065,7 +1065,8 @@ } sb.append (drawTableCell ( + +- rd.getDuplicationPartners() != null ? duplicationPartners.replace(";", "
") : "")); + ++ rd.getDuplicationPartners() != null ? escapeHtmlLabel(duplicationPartners).replace(";", "
") ++ : "")); + +@@ -765,7 +765,8 @@ static String escapeHtmlLabel (String label) { + +- return label.replace("&", "&").replace("<", "<").replace(">", ">"); + ++ return label.replace("&", "&").replace("<", "<").replace(">", ">") ++ .replace("\"", """).replace("'", "'"); + } + +## References + +- https://cwe.mitre.org/data/definitions/79.html +- https://cwe.mitre.org/data/definitions/80.html +- https://owasp.org/www-community/attacks/xss/ +- https://kotlinlang.org/docs/reference/grammar.html#escapedIdentifier + +--- + +_Rendered from original VulnHunter / VulnForge `report.yaml` by OpenVuln._ + +## [high] HtmlReport graph maps (class map + cycle maps: generateGraphButtons/buildClassGraphDot|buildClassCycleDot → hyperlinkClassForDot) embed git remote.origin.url raw inside a ` HTML breakout — all zero-interaction), with the realistically achievable top +impact measured per consumption surface (EXP-R6-E2): on an authenticated web origin that publishes the report (the +Jenkins-HTML-Publisher / CI-artifact-viewer / internal-dashboard pattern) the payload acts as every viewer and +exfiltrates the origin's protected data with each viewer's session (proven; HttpOnly does not help); on the documented +default local `file://` view the impact is bounded to redirect/phishing plus report-content beacons (local-file reads +blocked by the browser, no cookie surface — proven); on the README step-summary CI flow the sink's artifact is not even +generated (simple report) and the surface sanitizes anyway. Not code execution in the I:X sense — the ceiling is session +hijack on hosting origins, conditional on a victim-side publishing step the tool does not perform. + +## Code anchors + +| File | Line | Function | +|-----------------------------------------------------------------------------------------------|-----:|-----------------------------| +| `../change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java` | 85 | `getOriginUrl` | +| `../change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java` | 88 | `getRepoUrl` | +| `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` | 360 | `generateReport` | +| `../report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java` | 369 | `generateReport` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 512 | `generateGraphButtons` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 564 | `buildClassGraphDot` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 631 | `hyperlinkClassForDot` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 923 | `renderClassCycleVisuals` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 972 | `renderPackageGraphVisuals` | +| `../report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java` | 391 | `printHead` | +| `../report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java` | 14 | `writeReportToDisk` | +| `../refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstHtmlReport.java` | 60 | `execute` | +| `../cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java` | 96 | `call` | + +## Background + +RefactorFirst is a developer/CI-side static-analysis tool (Maven plugin, CLI) that mines a project's git history with +JGit, ranks design "disharmonies" by change-proneness vs. effort, and emits an interactive HTML report +(`target/site/refactor-first-report.html`). The HTML report's Class Map / Cycle Map / Package Map sections render the +codebase's dependency graphs client-side: the Graphviz DOT text for each graph is embedded into the page inside a +JavaScript template literal, `const _dot = ` + DOT + `;` within a plain ` + +This is a classic (non-module, dependency-free) script block: the `const` initializer executes at page load, with no +click and no CDN dependency. Three independent breakout classes make `remote.origin.url` execute attacker-controlled +code: + +1. **`${…}` interpolation** — an origin URL of `https://evil/x${}y` becomes a live substitution expression when + the template literal is evaluated during the + `const` initialization; the surrounding statement stays syntactically valid, so the whole script parses and the + payload runs (arbitrary expression: cookie read/exfil, + `fetch`, DOM manipulation). +2. **Backtick breakout** — `https://evil/x` + backtick + `++` + backtick + `y` + closes the literal and turns the payload into an operand of string concatenation — still a valid expression, executed + during initializer evaluation. +3. **`` HTML breakout** — an origin URL containing `` terminates the `` payload truncates both graph script elements at `URL="` (Chrome reports +`SyntaxError: Unexpected end of input`), leaves the consts unassigned, and turns the payload into live markup — 28 +`img[onerror]` elements firing 28 zero-interaction dialogs in real Chrome on a plain file:// open (and the same count on +a loopback-hosted http:// origin, where the payload reads `document.domain`). A benign-URL control fixture produces a +clean report with zero alerts/dialogs/injected markup. The no-disharmony branch ("Congratulations…" report) still +carries the payload in `classGraph_dot` and executes it. + +Relationship to sibling finding: this is the same root cause (unvalidated +`remote.origin.url` reaching report sinks) as BUG-R2-S2-A1-H1, whose report documents this DOT sink as its "Sink 4"; the +present finding carries the JS/DOT-sink chain (ADV-R2-S2-A2) with the full template-literal analysis. The two should +share one POC harness (one malicious repo exercises A1's `href` sinks and this DOT sink simultaneously); consolidation +is routed to decide. Real-scenario impact was assessed separately for each (exp/ of each finding): the impact ceiling is +identical (session-class on authenticated hosting origins, conditional; bounded on the local view; nothing on the +step-summary surface), so both are calibrated to CVSS 7.4 / EV 7 / P1 / I:S; the differentiators are coverage (this +sink: full report + site artifact only, the sibling: both report types incl. empty repos) and payload profile (this +sink: unconstrained JS grammar with zero DOM footprint; the sibling: unquoted- attribute grammar with injected +elements). A sink-isolated `${…}` payload proved this finding independently sufficient for the full impact class +(exp/exp.md, EXP-R6-E2). + +## Attack + +The attacker is the provider of the codebase being analyzed. Delivery paths that put a hostile `remote.origin.url` into +the victim's `.git/config`: (1) distribute the project as an archive that includes a pre-built `.git` directory (vendor +drop, file share, email attachment) — `.git/config` is a plain attacker-authored file with no git-client sanitization; +(2) induce the victim to clone from an attacker-operated remote whose URL string itself carries the payload (git stores +the clone URL verbatim; the attacker controls the server's routing so the clone succeeds); (3) a superproject whose +tracked +`.gitmodules` supplies the submodule origin URL — after `git submodule update --init` +the submodule's `.git/modules//config` carries it verbatim, and JGit's +`findGitDir` follows the `.git` pointer file when the report is run inside the submodule directory. The victim then runs +the tool's normal workflow — `mvn +org.hjug.refactorfirst.plugin:refactor-first-maven-plugin:htmlReport` (the README headline flow) or the `mvn site` +integration (both verified dynamically to emit and execute the sink; the CLI's HTML default is not a realistic vector — +no tested CLI jar can start, pre-existing picocli bug; `simpleHtmlReport` emits no graph blocks at all) — and opens the +generated report, which the tool itself advertises ("View the report at target/site/refactor-first-report.html"). The +attacker's JavaScript executes the moment the page loads, once per rendered graph vertex with a source-file mapping (8 +sites on a 10-class fixture; scales with repo size), and for the `${…}`/backtick shapes with ZERO DOM footprint — no +injected elements, no attribute changes, no extra console errors, graphs keep rendering (measured vs a benign control) — +making this the stealthiest injection channel of the report family. What the payload achieves is set by which surface +renders the artifact (all measured, EXP-R6-E2): on the default local `file://` view — redirect/phishing in the trusted +report context plus report-content beacons to attacker infrastructure, with local-file reads blocked by the browser and +no cookie surface; on an authenticated web origin that publishes the report (the Jenkins-HTML-Publisher / +CI-artifact-viewer / internal-dashboard pattern for Maven HTML reports, incl. published `mvn site` artifacts) — full +session-class compromise: the payload's same-origin fetch rides every viewer's session (HttpOnly notwithstanding), +exfiltrates the origin's protected data plus non-HttpOnly cookies, and propagates to every viewer of the shared +artifact; on session-less web origins (raw-CDN hosting like the project's own rawcdn.githack.com sample) — arbitrary +rendering/redirect for every viewer plus direct viewer-IP/UA beacons; on the README step-summary CI flow — nothing: that +flow's simpleHtmlReport artifact contains no graph blocks, and the step-summary surface sanitizes script content anyway. +The attacker also fully controls the repository content, so the two cross-referencing classes that place a +`URL="…"` attribute into the class-map DOT are guaranteed present, and the payload-carrying `const` block is +emitted regardless of graph size (the 4000-node threshold only gates the SVG image). + +### Payload + +The payload is a single line in the analyzed repository's `.git/config`: +`[remote "origin"] url = `. Working shapes (all avoid the `git@` prefix so the cosmetic rewrites leave them +intact; none contains `.git` or `gitlab`, so only the harmless `/blob//` suffix is appended behind the payload): +(a) template-literal interpolation: `https://evil/x${alert(document.domain)}y` — the substitution executes when +`const classGraph_dot = …` is evaluated at page load; (b) backtick breakout: `https://evil/x` + `` ` `` + +`+alert(document.domain)+` + `` ` `` + ++ `y` — payload becomes a concatenation operand, still executed at load; (c) script-block breakout: `https://evil/xy` — the `` sequence ends the script element at HTML parse time and the injected + `` fires without any click. Shape (a) + additionally requires nothing beyond two cross-referencing Java classes in the repo so that at least one graph vertex + renders with its `URL="…"` attribute. + +## Data flow + +### Step 1 — `refactor-first-maven-plugin/src/main/java/org/hjug/mavenreport/RefactorFirstHtmlReport.java:60-77` + +Victim runs the htmlReport mojo on the analyzed project (baseDir = project basedir) — the entry point that generates the +FULL report carrying this sink; the mvn site integration (RefactorFirstMavenReport.java:71-86, output via +mainSink.rawText — unescaped, verified dynamically) renders the same code, and the CLI's HTML default would too but no +tested CLI jar can start (pre-existing picocli bug). The simpleHtmlReport goal funnels into SimpleHtmlReport, whose +graph-render overrides return empty strings — it emits no DOT blocks and is not a carrier of this sink (verified: 0 _dot +blocks in its artifact). + +### Step 2 — `change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java:84-85` + +getOriginUrl () returns gitRepository.getConfig ().getString ("remote", "origin", "url") — the raw, free-form +remote.origin.url value from the analyzed repository's .git/config (JGit returns it verbatim; no validation). + +### Step 3 — `change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java:88-108` + +getRepoUrl () applies only cosmetic transforms — git@→https:// rewrite (only for git@-prefixed strings), global .git +substring removal, /blob|/-/blob|/src// suffix append. Backtick, ${, ", <, >, / are untouched; no scheme +allow-list, no character validation, no encoding, no escaping. + +### Step 4 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:360,417-421` + +generateReport () obtains the raw repoUrl via getRepoUrl (projectBaseDir) and passes the unmodified string into +renderClassGraphVisuals (:369 no-disharmony branch, :381), renderPackageGraphVisuals (:390) and renderCycles (:410). + +### Step 5 — `report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java:561-584,941-958,995-1016` + +buildClassGraphDot/buildClassCycleDot/buildPackageGraphDot wrap the whole DOT text as a JavaScript template literal +expression: dot.append ("`strict digraph G {\n") (:564/:947/:998) … dot.append("}`;") (:582/:957/:1015). + +### Step 6 — `report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java:623-631` + +Sink — hyperlinkClassForDot () returns URL="" target="_blank" per rendered vertex (call site :604 in +renderClassVertices): repoUrl is concatenated raw; the path half is Path.toUri () percent-encoded +(AbstractDependencyVisitor.java:78,98,107), so repoUrl is the only raw component inside the literal. + +### Step 7 — `report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java:512-521,489,923,972` + +generateGraphButtons () emits — a classic, dependency-free script +block whose const initializer (the template literal) is evaluated at page load. The block is emitted unconditionally for +the class map (:489), cycle maps (:923) and package map (:972); the dotGraphThreshold=4000 checks (:502/:928/:985) gate +only the separate vizdom SVG image, never this block (POC-verified: all fixture graphs below threshold and rendered). +The repoUrl payload lands in the class-map and cycle-map blocks (hyperlinkClassForDot URL attributes); the package-map +block carries no URL attribute (renderPackageVertices) and is not a payload carrier (POC-verified). ${…} in repoUrl +executes as a substitution; a backtick closes the literal into a concatenation; a sequence terminates the +script element at HTML parse time and the following markup executes as live HTML. + +### Step 8 — `report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:107-112` + +Optional minifyHtml post-pass (default false in the Maven mojo field initializer and the CLI) — a semantics-preserving +minifier, not an escaper; string/template-literal content survives it. + +### Step 9 — `report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java:14-42` + +writeReportToDisk () writes the HTML verbatim to target/site/refactor-first-report.html; the report has no +Content-Security-Policy (HtmlReport.printHead :391-404 emits only CDN script/link tags), so the injected JavaScript +executes when the victim opens the file. + +## Fix / patch notes + +diff --git a/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java +b/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java --- +a/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java +++ +b/change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java @@ -95,6 +95,13 @@ public class GitLogReader +implements AutoCloseable { if (originUrl == null) { return ""; } + ++ // Only well-formed web URLs may be embedded in generated reports as ++ // source-link prefixes; anything else (javascript:, data:, file:, ...) ++ // is rejected outright. ++ if (!originUrl.startsWith("https://") && !originUrl.startsWith("http://")) { ++ return ""; ++ } + + repoUrl = originUrl.replace(".git", ""); + +@@ -105,6 +112,11 @@ public class GitLogReader implements AutoCloseable { } else { repoUrl = repoUrl + "/blob/" + +getCurrentCommitHash () + "/"; } + +- return repoUrl; + ++ // Keep only RFC 3986 URL characters: drops every character that is ++ // markup- or JS-significant outside a URL (space, ", <, >, `, {, }, \), ++ // so the value cannot break out of a ' was claimed to terminate the generateGraphButtons script element at the HTML +layer, but such a name never reaches the report — rewrite-kotlin's attributed FQN construction rewrites every '/' to '.' +(and inner '.' to '$', wrapping the segment in backticks), so the payload class yields FQN `csa.` + backtick + +`x<.script>` + backtick, whose last-dot-derived package contains a backtick +and can never be a declared Kotlin package; removeClassesNotInCodebase drops the vertex, no cycle forms, nothing renders +(verified end-to-end: zero payload occurrences, no Cycle Map section). Structurally, after '/'→'.' mangling, +getClassName ()'s last-'.' split can never leave a '/' in the rendered cycle name, so the script-element breakout is +unreachable via cycle names on this snapshot (the cycle-map script block remains breakable by '' through the +git remote URL baked into the DOT vertices — that vector is BUG-R2-S2-A2-H2's scope). The pre-analysis's parse-blocking +argument was verified correct for the pure-JS positions: for quote- and markup-bearing payloads the cycle-map classic +script (const _dot = ...) and module script (parser.parse (_dot) / getElementById ("") / svgPanZoom ('# +svg')) are JS SyntaxErrors while every other inline block (including the class-map script, where the payload sits inside +a template-literal string) parses — and the onclick pure-JS variant is additionally execution-blocked because showPopup +receives garbage popupIds — yet neither property constrains the HTML tokenizer, which is where all confirmed breakouts +occur. There is no Content-Security-Policy in the generated report, the report loads all chart/graph libraries from +public CDNs (designed for online viewing), minifyHtml defaults to false, and analyzeCycles defaults to true on both CLI +and Maven — so the vulnerable section renders in the default configuration for any repository containing a crafted +Kotlin cycle. + +## Attack + +Attacker = author of a public repository or PR contributor; victim = a developer or CI pipeline that runs RefactorFirst +on the repository and anyone who views the generated report — the tool's documented workflow (delivery verified +end-to-end from the victim's side for both a direct clone and a fork-PR checkout of an OSS project). Steps: (1) attacker +commits the crafted .kt file (two payload-named classes referencing each other); (2) victim clones or checks out the PR +and runs a default analysis — CLI `refactorfirst -b .` (default -t HTML), the README's primary Maven command +`mvn org.hjug.refactorfirst.plugin:…:htmlReport`, or `mvn site` — producing target/site/refactor-first-report.html whose +Cycle Map section embeds the payload in the popup-button onclick attributes, the graph div id, the popup div ids, the +`const _dot` script block and the popup-button element bodies (19 payload occurrences in the measured fixture +report); (3) any viewer opens the report in a browser (its designed mode — all chart/graph libraries load from CDNs): +the zero-interaction variant's injected (live markup in the popup-button bodies) executes automatically at +page load with no interaction — measured in the impact assessment: first attacker beacon 61-75ms after page load, before +domContentLoaded — while the hover-gated variant A fires onmouseover on the popup buttons and the always-visible +cycle-map div. For hosted copies (CI artifact servers / Jenkins HTML Publisher / mvn-site deployments / +rawcdn-githack-style publishing — the project itself publishes a sample report this way) the payload is stored XSS +against every viewer of that origin: measured on a simulated authenticated hosting origin, two independent viewers were +each compromised at page load (cookie theft plus same-origin exfiltration of the origin's private data including their +HttpOnly session ids); for local file:// viewing the script still executes in the weaker file:// origin but with bounded +impact (no cookie surface, no same-origin data — measured). The report file is persistent, so the payload fires on every +future view. The README's $GITHUB_STEP_SUMMARY flow is NOT an execution surface for this finding: it produces the simple +report (no cycle-map visuals — measured) and GitHub's user-content sanitizer strips every script/handler shape even from +the full report (measured against the documented allowlist). + +### Payload + +A Kotlin source file in the analyzed repository declaring, in two different packages, two mutually-referencing classes +named with the same backticked payload identifier, so the two form a dependency cycle and either vertex can become the +cycle name. Variant A (attribute breakout, hover-gated): class `q" onmouseover="alert(1)" x="` with a mirror in a second +package. Zero-interaction variant (the impact-governing shape, measured end-to-end in the impact assessment): class +`x` and a mirror — quote-free so it stays inert in every attribute-context sink and renders live +markup in the popup-button element bodies; arbitrary JavaScript is deliverable despite the pipeline's character +constraints by smuggling dots/slashes/colons/question-marks/quotes as named character references (. / : +? ' = — decoded by the HTML parser in element bodies and unquoted attribute values) and +base64-encoding the payload body (eval (atob ('…')) — measured executing in Chromium). Payload identifiers +must avoid backtick/newline (Kotlin grammar), '.', +'$', '/' and '?' — rewrite-kotlin 8.90.4 rewrites '/'→'.' and inner '.'→'$', strips '?', and drops '/'-bearing vertices +entirely, and getClassName () strips everything up to the last '.'; the finding's originally documented +`x` shape violates these constraints (contains '/' and '.') and does +not survive the pipeline (dynamically verified) — the script-element breakout must not be claimed through the +cycle-name path. + +## Data flow + +### Step 1 — + +`codebase-graph-builder/src/main/java/org/hjug/graphbuilder/graphbuilder/KotlinSourceFileGraphBuilder.java:67-101` + +Source: unconditional walk of the analyzed repository for *.kt/*.kts, each parsed with rewrite-kotlin's KotlinParser +(partial trees visited even on parse errors). A Kotlin backticked class name may contain ", <, >, / and spaces (anything +but backtick/newline) — attacker (repo author) fully controls it. + +### Step 2 — + +`org/openrewrite/kotlin/internal/KotlinTreeParserVisitor.java:3826-3850 (pinned rewrite-kotlin 8.90.4 source) + org/openrewrite/kotlin/KotlinTypeSignatureBuilder.kt:712-728` + +Identifier/FQN construction: createIdentifier strips only the leading/trailing backtick and stores the inner text +verbatim as the J.Identifier simple name; the attributed JavaType FQN is built via convertClassIdToFqn which only +rewrites '.'→'$' and '/'→'.'. Quotes, angle brackets and spaces pass into jcd.getSimpleName () and +type.getFullyQualifiedName () verbatim. Dynamically probed (production parser config, poc/probe-*.log): the quote +payload attributes to FQN cqa.q" onmouseover="alert (1)" x=" and the markup payload to +cza.x — both verbatim; a '/'-bearing name attributes to csa. +`x<.script>` (backtick-wrapped), whose last-dot-derived package contains a +backtick and can never be a declared package, so the vertex is dropped in the next step — the '/'→'.' rewrite therefore +constrains cycle-name payloads to be slash-free. + +### Step 3 — + +`codebase-graph-builder/src/main/java/org/hjug/graphbuilder/visitor/KotlinDependencyVisitor.java:99-118 + GraphDependencyCollector.java:48-63,144-146` + +Vertex registration: registerClassVertex (raw FQN) / addClassDependency (raw FQN, dep FQN) add the payload string to the +JGraphT class graph with no character validation (contrast: the Java visitor's un-attributed fallback +enforces [A-Za-z_$][A-Za-z0-9_$]*). Kotlin↔Kotlin edges proven by KotlinGraphBuilderTest; the vertex survives +finalizeDto's removeClassesNotInCodebase because its package is declared. + +### Step 4 — + +`graph-algorithms/src/main/java/org/hjug/dsm/CircularReferenceChecker.java:47-84 + cost-benefit-calculator/src/main/java/org/hjug/cbc/CycleRanker.java:60-107` + +Cycle detection: getCycles keys each unique cycle by one of its vertices (2-vertex/2-edge cycles pass the vertexCount> +1 && edgeCount>1 gate); identifyRankedCycles copies that key into RankedCycle.cycleName. Naming BOTH cycle classes with +the payload (different packages) defeats the HashMap-order choice of key vertex; rawPriority=vertexSet.size () ordering +plus renderCycles' limit (1) renders the attacker's (only) cycle. + +### Step 5 — + +`report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java:483-487,783-798 + report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java:916-920` + +renderCycles (limit 1) → renderSingleCycle → renderClassCycleVisuals, where the sole sanitization is getClassName (...) +.replace ("$","_") — inert for a payload without '.' and '$'. analyzeCycles defaults to true (CLI ReportCommand.java: +38-41, Maven RefactorFirstHtmlReport.java:29-30); renderClassCycleVisuals is implemented only in the default HtmlReport. + +### Step 6 — `report/src/main/java/org/hjug/refactorfirst/report/HtmlReport.java:1070-1082` + +Sink (attribute context, primary): generate2DPopup/generateForce3DPopup emit onclick="showPopup ('popup-', +'graph-container-', _dot )" on visible \n"; } String generateForce3DPopup(String cycleName) { + return generateForce3DPopup(cycleName, cycleName); + } + + private String generateForce3DPopup(String cycleIdentifier, String displayName) { // Created by generative AI and modified - return "\n"; } diff --git a/report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java b/report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java index 3bacc6b5..881b4f5b 100644 --- a/report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java +++ b/report/src/main/java/org/hjug/refactorfirst/report/ReportWriter.java @@ -1,40 +1,191 @@ package org.hjug.refactorfirst.report; +import static java.nio.file.LinkOption.NOFOLLOW_LINKS; +import static java.nio.file.StandardOpenOption.CREATE_NEW; +import static java.nio.file.StandardOpenOption.WRITE; + import java.io.BufferedWriter; import java.io.File; import java.io.IOException; +import java.io.OutputStreamWriter; +import java.nio.channels.Channels; +import java.nio.channels.SeekableByteChannel; import java.nio.charset.Charset; +import java.nio.file.DirectoryStream; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.OpenOption; +import java.nio.file.Path; +import java.nio.file.SecureDirectoryStream; +import java.nio.file.attribute.BasicFileAttributeView; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.UUID; import lombok.extern.slf4j.Slf4j; @Slf4j public final class ReportWriter { + /** Resolves a configured report directory and rejects escapes or existing link components. */ + public static String containReportDirectory(final File baseDir, final String configuredDir) { + final Path base = (baseDir != null ? baseDir.toPath() : Path.of("")) + .toAbsolutePath() + .normalize(); + final String configured = + configuredDir == null || configuredDir.isBlank() ? "target" + File.separator + "site" : configuredDir; + final Path resolved = base.resolve(configured).normalize(); + if (!resolved.startsWith(base)) { + throw new IllegalArgumentException( + "Report output directory escapes the project base directory: " + configuredDir); + } + rejectExistingSymbolicLinkComponents(resolved); + return resolved.toString(); + } + + /** + * Writes through directory descriptors and atomically renames the completed report into place. + * Failures propagate so command and plugin callers cannot report success after a blocked write. + */ public static void writeReportToDisk( final String reportOutputDirectory, final String filename, final String string) { - final File reportOutputDir = new File(reportOutputDirectory); + Path outputDirectory = Path.of(reportOutputDirectory).toAbsolutePath().normalize(); + Path reportName = validateFilename(filename); + List> openedDirectories = new ArrayList<>(); - if (!reportOutputDir.exists()) { - reportOutputDir.mkdirs(); + try { + SecureDirectoryStream outputStream = openSecureDirectoryPath(outputDirectory, openedDirectories); + writeAtomically(outputStream, reportName, string); + log.info("Done! View the report at {}", outputDirectory.resolve(reportName)); + } catch (IOException | UnsupportedOperationException e) { + log.error("Unable to write report {}", outputDirectory.resolve(reportName), e); + throw new ReportWriteException("Unable to write report " + outputDirectory.resolve(reportName), e); + } finally { + closeDirectories(openedDirectories); } + } - final String pathname = reportOutputDirectory + File.separator + filename; + private static Path validateFilename(String filename) { + if (filename == null || filename.isBlank()) { + throw new ReportWriteException("Report filename must not be empty"); + } + Path reportName = Path.of(filename); + if (reportName.isAbsolute() + || reportName.getNameCount() != 1 + || ".".equals(filename) + || "..".equals(filename)) { + throw new ReportWriteException("Report filename must be a single path component: " + filename); + } + return reportName; + } + + private static SecureDirectoryStream openSecureDirectoryPath( + Path outputDirectory, List> openedDirectories) throws IOException { + Path root = outputDirectory.getRoot(); + if (root == null) { + throw new IOException("Report output directory has no filesystem root: " + outputDirectory); + } - final File reportFile = new File(pathname); + DirectoryStream rootStream = Files.newDirectoryStream(root); + openedDirectories.add(rootStream); + SecureDirectoryStream current = asSecureDirectoryStream(rootStream, root); + Path currentPath = root; + for (Path component : root.relativize(outputDirectory)) { + SecureDirectoryStream child; + try { + child = current.newDirectoryStream(component, NOFOLLOW_LINKS); + } catch (NoSuchFileException e) { + Path directoryToCreate = currentPath.resolve(component); + Files.createDirectory(directoryToCreate); + child = current.newDirectoryStream(component, NOFOLLOW_LINKS); + } + openedDirectories.add(child); + current = child; + currentPath = currentPath.resolve(component); + } + return current; + } + + @SuppressWarnings("unchecked") + private static SecureDirectoryStream asSecureDirectoryStream(DirectoryStream stream, Path directory) { + if (!(stream instanceof SecureDirectoryStream)) { + throw new UnsupportedOperationException( + "Secure directory operations are unavailable for report output: " + directory); + } + return (SecureDirectoryStream) stream; + } + + private static void writeAtomically(SecureDirectoryStream directory, Path reportName, String content) + throws IOException { + BasicFileAttributeView targetView = + directory.getFileAttributeView(reportName, BasicFileAttributeView.class, NOFOLLOW_LINKS); try { - reportFile.createNewFile(); - } catch (IOException e) { - log.error("Failure creating chart script file", e); + BasicFileAttributes attributes = targetView.readAttributes(); + if (attributes.isSymbolicLink() || attributes.isDirectory()) { + throw new IOException("Refusing to replace non-regular report path: " + reportName); + } + } catch (NoSuchFileException ignored) { + // The normal first-write case. + } + + Path temporaryName = Path.of("." + reportName + "." + UUID.randomUUID() + ".tmp"); + Set options = Set.of(CREATE_NEW, WRITE, NOFOLLOW_LINKS); + boolean moved = false; + try { + try (SeekableByteChannel channel = directory.newByteChannel(temporaryName, options); + BufferedWriter writer = new BufferedWriter( + new OutputStreamWriter(Channels.newOutputStream(channel), Charset.defaultCharset()))) { + writer.write(content); + } + directory.move(temporaryName, directory, reportName); + moved = true; + } finally { + if (!moved) { + try { + directory.deleteFile(temporaryName); + } catch (NoSuchFileException ignored) { + // Nothing to clean up. + } + } + } + } + + private static void rejectExistingSymbolicLinkComponents(Path path) { + Path current = path.getRoot(); + if (current == null) { + return; + } + for (Path component : current.relativize(path)) { + current = current.resolve(component); + if (Files.isSymbolicLink(current)) { + throw new IllegalArgumentException("Report output path contains a symbolic link: " + current); + } + if (!Files.exists(current, NOFOLLOW_LINKS)) { + return; + } } + } - try (BufferedWriter writer = Files.newBufferedWriter(reportFile.toPath(), Charset.defaultCharset())) { - writer.write(string); - } catch (IOException e) { - log.error("Error writing chart script file", e); + private static void closeDirectories(List> directories) { + for (int i = directories.size() - 1; i >= 0; i--) { + try { + directories.get(i).close(); + } catch (IOException e) { + log.warn("Unable to close report output directory", e); + } } + } - log.info("Done! View the report at target/site/{}", filename); + public static final class ReportWriteException extends RuntimeException { + public ReportWriteException(String message) { + super(message); + } + + public ReportWriteException(String message, Throwable cause) { + super(message, cause); + } } private ReportWriter() {} diff --git a/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java b/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java index 4420559a..ae5ce6be 100644 --- a/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java +++ b/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java @@ -1034,8 +1034,8 @@ public String renderDisharmonyInfo( sb.append("\n"); for (RankedDisharmony rd : ranked) { sb.append("\n"); - sb.append(drawTableCell( - "" + rd.getFileName() + "")); + sb.append(drawTableCell("" + + escapeHtmlLabel(rd.getFileName()) + "")); if (methodLevel) { String sig = rd.getMethodSignature(); if (!showDetails && sig != null) { diff --git a/report/src/test/java/org/hjug/refactorfirst/report/CsvReportTest.java b/report/src/test/java/org/hjug/refactorfirst/report/CsvReportTest.java new file mode 100644 index 00000000..6ea9c924 --- /dev/null +++ b/report/src/test/java/org/hjug/refactorfirst/report/CsvReportTest.java @@ -0,0 +1,27 @@ +package org.hjug.refactorfirst.report; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.Test; + +class CsvReportTest { + + @Test + void sanitizeCsvCell_quotesEscapesAndNeutralizesEveryLine() { + assertEquals("\"plain\"", CsvReport.sanitizeCsvCell("plain")); + assertEquals("\"a,b \"\"quoted\"\"\"", CsvReport.sanitizeCsvCell("a,b \"quoted\"")); + assertEquals("\"'=formula\"", CsvReport.sanitizeCsvCell("=formula")); + assertEquals("\"safe\n'+formula\"", CsvReport.sanitizeCsvCell("safe\n+formula")); + assertEquals("\"'\tformula\"", CsvReport.sanitizeCsvCell("\tformula")); + assertEquals("\"'\r'=formula\"", CsvReport.sanitizeCsvCell("\r=formula")); + } + + @Test + void addsRow_usesCanonicalEncoderOnceForEveryCell() { + StringBuilder row = new StringBuilder(); + + new CsvReport().addsRow(row, new String[] {"=project", "1,2", "a\"b"}); + + assertEquals("\"'=project\",\"1,2\",\"a\"\"b\",", row.toString()); + } +} diff --git a/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportKotlinTest.java b/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportKotlinTest.java index 41bcc1dd..a3de5456 100644 --- a/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportKotlinTest.java +++ b/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportKotlinTest.java @@ -92,9 +92,9 @@ void buildClassCycleDot_kotlinSourcePaths() { String expectedDot = """ `strict digraph G { - KotlinCycleA -> KotlinCycleB [ label = "2" weight = "2" ]; - KotlinCycleB -> KotlinCycleC [ label = "1" weight = "1" ]; - KotlinCycleC -> KotlinCycleA [ label = "1" weight = "1" ]; + KotlinCycleA -\\u003E KotlinCycleB [ label = "2" weight = "2" ]; + KotlinCycleB -\\u003E KotlinCycleC [ label = "1" weight = "1" ]; + KotlinCycleC -\\u003E KotlinCycleA [ label = "1" weight = "1" ]; KotlinCycleA [URL="https://github.com/refactorfirst/RefactorFirst/blob/src/main/kotlin/com/kotlin/cycles/KotlinCycleA.kt" target="_blank"]; KotlinCycleB [URL="https://github.com/refactorfirst/RefactorFirst/blob/src/main/kotlin/com/kotlin/cycles/KotlinCycleB.kt" target="_blank"]; KotlinCycleC [URL="https://github.com/refactorfirst/RefactorFirst/blob/src/main/kotlin/com/kotlin/cycles/KotlinCycleC.kt" target="_blank"]; diff --git a/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java b/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java index c1583d56..635384c2 100644 --- a/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java +++ b/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java @@ -63,9 +63,9 @@ void buildClassCycleDot() { String repoUrl = "https://github.com/refactorfirst/RefactorFirst/blob"; String dot = htmlReport.buildClassCycleDot(classGraph, rankedCycle, repoUrl, dto); String expectedDot = "`strict digraph G {\n" - + "A -> B [ label = \"2\" weight = \"2\" ];\n" - + "B -> C [ label = \"1\" weight = \"1\" ];\n" - + "C -> A [ label = \"1\" weight = \"1\" ];\n" + + "A -\\u003E B [ label = \"2\" weight = \"2\" ];\n" + + "B -\\u003E C [ label = \"1\" weight = \"1\" ];\n" + + "C -\\u003E A [ label = \"1\" weight = \"1\" ];\n" + "A [URL=\"https://github.com/refactorfirst/RefactorFirst/blob/src/main/java/org/hjug/refactorfirst/A.java\" target=\"_blank\"];\n" + "B [URL=\"https://github.com/refactorfirst/RefactorFirst/blob/src/main/java/org/hjug/refactorfirst/B.java\" target=\"_blank\"];\n" + "C [URL=\"https://github.com/refactorfirst/RefactorFirst/blob/src/main/java/org/hjug/refactorfirst/C.java\" target=\"_blank\"];\n" @@ -278,6 +278,56 @@ void hyperlinkClassForDot_missingPath_rendersNoUrlAttribute() { assertEquals("", result); } + @Test + void renderClassCycleVisuals_usesSafeIdentifierAndEscapedDisplayName() { + HtmlReport htmlReport = new HtmlReport(); + htmlReport.classGraph = new DefaultDirectedWeightedGraph<>(DefaultWeightedEdge.class); + RankedCycle leadingDigit = new RankedCycle("1", Set.of(), Set.of(), List.of()); + + String result = htmlReport.renderClassCycleVisuals(leadingDigit, "", null); + + assertTrue(result.contains("const graph_1_cycle_")); + assertFalse(result.contains("const 1")); + assertTrue(result.contains("Show 1<cycle> 2D Popup")); + assertFalse(result.contains("Show 1")); + } + + @Test + void renderClassCycleVisuals_usesNonEmptyIdentifierForEmptyName() { + HtmlReport htmlReport = new HtmlReport(); + htmlReport.classGraph = new DefaultDirectedWeightedGraph<>(DefaultWeightedEdge.class); + RankedCycle emptyName = new RankedCycle("", Set.of(), Set.of(), List.of()); + + String result = htmlReport.renderClassCycleVisuals(emptyName, "", null); + + assertTrue(result.contains("const graph_cycle_0_dot")); + assertFalse(result.contains("const _dot")); + } + + @Test + void buildPackageGraphDot_usesDistinctIdsAndEscapesRawScriptContext() { + Graph graph = new DefaultDirectedWeightedGraph<>(DefaultWeightedEdge.class); + String dotted = "1.a.b"; + String underscored = "1.a_b\\name"; + graph.addVertex(dotted); + graph.addVertex(underscored); + graph.addEdge(dotted, underscored); + + String dot = new HtmlReport().buildPackageGraphDot(graph, "", null); + + assertNotEquals(HtmlReport.renderSafePackageNodeId(dotted), HtmlReport.renderSafePackageNodeId(underscored)); + assertTrue(dot.contains(HtmlReport.renderSafePackageNodeId(dotted))); + assertTrue(dot.contains(HtmlReport.renderSafePackageNodeId(underscored))); + assertFalse(dot.contains("")); + assertTrue(dot.contains("\\u003C/script\\u003E")); + assertTrue(dot.contains("\\\\\\\\name"), "DOT and template-literal escaping must both preserve backslashes"); + } + + @Test + void escapeDotQuoted_escapesQuotesBackslashesAndLineBreaks() { + assertEquals("a\\\\b\\\"c\\nd", HtmlReport.escapeDotQuoted("a\\b\"c\nd")); + } + /** * Test that renderSafeNodeId with graph context uses simple name when unique. */ @@ -498,4 +548,17 @@ void renderSafeNodeId_kotlinAnonymousProducesValidNodeId() { String nodeId3 = htmlReport.renderSafeNodeId(anon2, dto); assertEquals("DeveloperWASDControl_anonymous_302290128", nodeId3); } + + @Test + void printTitle_escapesProjectNameAndVersionInTitleTag() { + HtmlReport htmlReport = new HtmlReport(); + String projectName = "Test