From dc27bb69da3448cb8e12d203ad14a7b235631989 Mon Sep 17 00:00:00 2001 From: Jim Bethancourt Date: Thu, 3 Sep 2026 08:11:03 -0500 Subject: [PATCH 1/3] #205 Address OpenVuln findings --- .../main/java/org/hjug/git/GitLogReader.java | 12 +- .../hjug/git/GitLogReaderGetRepoUrlTest.java | 131 + .../org/hjug/cbc/CostBenefitCalculator.java | 30 +- ...First-full-9-2-2026-implementation-plan.md | 587 +++ ...factorfirst-RefactorFirst-full-9-2-2026.md | 3952 +++++++++++++++++ .../mavenreport/RefactorFirstHtmlReport.java | 7 +- .../RefactorFirstMavenCsvReport.java | 7 +- .../RefactorFirstMavenJsonReport.java | 8 +- .../RefactorFirstSimpleHtmlReport.java | 7 +- .../refactorfirst/report/ReportWriter.java | 38 +- .../report/SimpleHtmlReport.java | 4 +- .../refactorfirst/report/HtmlReportTest.java | 13 + .../report/ReportWriterTest.java | 183 + 13 files changed, 4953 insertions(+), 26 deletions(-) create mode 100644 plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026-implementation-plan.md create mode 100644 plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026.md create mode 100644 report/src/test/java/org/hjug/refactorfirst/report/ReportWriterTest.java 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 { + // URL with various markup characters that could be used for XSS + git.remoteAdd() + .setName("origin") + .setUri(new URIish("https://example.com/repo.git")) + .call(); + + // Manually set a config with markup characters (simulating attacker-controlled config) + // We can't easily test this through the public API since getOriginUrl() reads from config + // But we can test the sanitization logic directly by checking the behavior + try (GitLogReader gitLogReader = new GitLogReader(projectBaseDir)) { + String repoUrl = gitLogReader.getRepoUrl(); + // The URL should only contain RFC 3986 allowed characters + assertTrue( + repoUrl.matches("[A-Za-z0-9._~:/?#\\[\\]@!$&'()*+,;=%-]*"), + "URL should only contain RFC 3986 characters: " + 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/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..987d0ae7 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 @@ -139,11 +139,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 +467,22 @@ private String getFileName(RuleViolation violation) { } String canonicaliseURIStringForRepoLookup(String uriString) { - if (repositoryPath.startsWith("/") || repositoryPath.startsWith("\\")) { - return uriString.replace("file://" + repositoryPath.replace("\\", "/") + "/", ""); + String normalizedRepoPath = repositoryPath.replace("\\", "/"); + // Handle both absolute (file:///C:/...) and relative (file:...) URIs + if (uriString.startsWith("file:///")) { + // Absolute path URI + return uriString.replace("file:///" + normalizedRepoPath + "/", ""); + } else if (uriString.startsWith("file://")) { + // Absolute path URI (alternative format) + return uriString.replace("file://" + normalizedRepoPath + "/", ""); + } else if (uriString.startsWith("file:")) { + // Relative path URI - make it relative to repo root + String relativePath = uriString.substring("file:".length()); + if (relativePath.startsWith("/")) { + relativePath = relativePath.substring(1); + } + return relativePath; } - return uriString.replace("file:///" + repositoryPath.replace("\\", "/") + "/", ""); + return uriString; } } 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..8bd2d526 --- /dev/null +++ b/plans/openvuln-refactorfirst-RefactorFirst-full-9-2-2026-implementation-plan.md @@ -0,0 +1,587 @@ +# 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 +``` + +#### 6.2 Implement fix +- In `renderClassCycleVisuals()`: sanitize cycle name to identifier-safe charset `[A-Za-z0-9_]` +- Apply sanitization before all 7 downstream positions: + - JS identifier in `const <name>_dot` + - HTML `id` attributes + - JS string literals in `onclick` + - Element body text in popup buttons +- Replace `$` → `_` and strip all non-identifier characters + +--- + +## 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_writesNormallyWhenNoSymlinks() throws IOException +@Test void writeReportToDisk_createsParentDirectories() throws IOException +``` + +#### 7.2 Implement fix +- Check `Files.isSymbolicLink(outputDir.toPath())` before `mkdirs()` +- Check `Files.isSymbolicLink(reportFile.toPath())` before `createNewFile()` +- Log error and return early (don't throw to avoid breaking existing callers) +- Use `Files.newBufferedWriter` with `LinkOption.NOFOLLOW_LINKS` + +--- + +## 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 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` +- 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 escapeDotQuoted_escapesDollarForTemplateLiteral() throws IOException +``` + +#### 9.2 Implement fix +- Add `renderSafePackageNodeId(String packageName)`: + - Replace `.` → `_` + - Replace `$` → `_` + - Replace `"` → `_` + - Replace `{` → `_` + - Replace `}` → `_` +- Add `escapeDotQuoted(String value)`: + - Escape `\` → `\\` + - Escape `"` → `\"` + - Escape `$` → `\$` (prevent `${...}` interpolation) +- Apply in `renderPackageVertices()` and `renderPackageGraphEdge()` +- Also apply `escapeDotQuoted` to class labels in `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 +- Add `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) + - All cells in `addsRow()` (line ~208) + +--- + +## 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 escapeCsvCell_quotesAndEscapesFormulaTriggers() throws IOException +@Test void escapeCsvCell_escapesEmbeddedQuotes() throws IOException +@Test void escapeCsvCell_handlesCommaInValue() throws IOException +``` + +#### 11.2 Implement fix +- Add `escapeCsvCell(String value)` method (RFC 4180 compliant + formula neutralization): + - Escape `"` → `""` + - Prefix `'` if starts with formula trigger + - Wrap in `"` +- Apply in `addsRow()` for all `rankedDisharmonyData` cells +- Apply in `getDataList()` for fileName and path cells + +--- + +## 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` +- `../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_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 +``` + +#### 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 + - Throw `IllegalArgumentException` if escapes baseDir +- Update all 4 mojos to call `containReportDirectory(project.getBasedir(), ...)` + +--- + +## 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 \ No newline at end of file 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 c2f595c6..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,24 +1,34 @@ 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.LinkOption; +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 output directory against the project base directory - * and refuses any value that escapes it. The value originates from the *analyzed* - * project's POM (), which is untrusted input whenever - * the tool runs against a third-party repository: absolute paths, ".." segments - * and empty values must never redirect report writes outside the analyzed project. - */ + /** 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() @@ -30,47 +40,152 @@ public static String containReportDirectory(final File baseDir, final String con 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<>(); - // CWE-59: never let a pre-existing symbolic link relocate the report write. - // Files.isSymbolicLink() examines the link itself (lstat semantics), so it also - // covers dangling links (the creation-through-link variant). - if (Files.isSymbolicLink(reportOutputDir.toPath())) { - log.error("Refusing to write report: output directory is a symbolic link: {}", reportOutputDirectory); - return; + 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); } + } - if (!reportOutputDir.exists()) { - reportOutputDir.mkdirs(); + 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; + } - final String pathname = reportOutputDirectory + File.separator + filename; + 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); - if (Files.isSymbolicLink(reportFile.toPath())) { - log.error("Refusing to write report: output path is a symbolic link: {}", pathname); - return; + 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. } - try (BufferedWriter writer = - Files.newBufferedWriter(reportFile.toPath(), Charset.defaultCharset(), LinkOption.NOFOLLOW_LINKS)) { - writer.write(string); - } catch (IOException e) { - log.error("Error writing chart script file", e); + 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. + } + } } + } - log.info("Done! View the report at target/site/{}", filename); + 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; + } + } + } + + 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); + } + } + } + + 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/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/HtmlReportTest.java b/report/src/test/java/org/hjug/refactorfirst/report/HtmlReportTest.java index 06548aa9..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. */ diff --git a/report/src/test/java/org/hjug/refactorfirst/report/ReportWriterTest.java b/report/src/test/java/org/hjug/refactorfirst/report/ReportWriterTest.java index e8d9684b..11ed41d5 100644 --- a/report/src/test/java/org/hjug/refactorfirst/report/ReportWriterTest.java +++ b/report/src/test/java/org/hjug/refactorfirst/report/ReportWriterTest.java @@ -7,20 +7,44 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardWatchEventKinds; +import java.nio.file.WatchKey; +import java.nio.file.WatchService; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; class ReportWriterTest { private static boolean supportsSymlinks() { + Path probeDirectory = null; + Path target = null; + Path link = null; try { - Path testLink = Files.createTempFile("symlink-test", ".tmp"); - Files.createSymbolicLink(testLink.resolveSibling("link"), testLink); - Files.deleteIfExists(testLink.resolveSibling("link")); - Files.deleteIfExists(testLink); + probeDirectory = Files.createTempDirectory("symlink-test"); + target = Files.createFile(probeDirectory.resolve("target")); + link = probeDirectory.resolve("link"); + Files.createSymbolicLink(link, target); return true; } catch (IOException | UnsupportedOperationException e) { return false; + } finally { + try { + if (link != null) { + Files.deleteIfExists(link); + } + if (target != null) { + Files.deleteIfExists(target); + } + if (probeDirectory != null) { + Files.deleteIfExists(probeDirectory); + } + } catch (IOException ignored) { + // The probe result remains valid even if best-effort cleanup fails. + } } } @@ -29,7 +53,7 @@ void writeReportToDisk_throwsWhenOutputDirIsSymlink(@TempDir Path tempDir) throw assumeTrue(supportsSymlinks(), "Symbolic links not supported on this platform"); // Create a target directory outside tempDir - Path outsideDir = tempDir.getParent().resolve("outside_target"); + Path outsideDir = tempDir.resolve("outside_target"); Files.createDirectories(outsideDir); // Create a symlink in tempDir pointing to outsideDir @@ -40,7 +64,9 @@ void writeReportToDisk_throwsWhenOutputDirIsSymlink(@TempDir Path tempDir) throw assertTrue(Files.isSymbolicLink(symlinkDir)); // Should refuse to write and log error - ReportWriter.writeReportToDisk(symlinkDir.toString(), "test.html", "test"); + assertThrows( + ReportWriter.ReportWriteException.class, + () -> ReportWriter.writeReportToDisk(symlinkDir.toString(), "test.html", "test")); // The file should NOT be created in the linked directory Path linkedFile = outsideDir.resolve("test.html"); @@ -52,7 +78,7 @@ void writeReportToDisk_throwsWhenOutputFileIsSymlink(@TempDir Path tempDir) thro assumeTrue(supportsSymlinks(), "Symbolic links not supported on this platform"); // Create a target file outside tempDir - Path outsideFile = tempDir.getParent().resolve("outside_file.html"); + Path outsideFile = tempDir.resolve("outside_file.html"); Files.writeString(outsideFile, "original content"); // Create a symlink in tempDir pointing to outsideFile @@ -63,7 +89,9 @@ void writeReportToDisk_throwsWhenOutputFileIsSymlink(@TempDir Path tempDir) thro assertTrue(Files.isSymbolicLink(symlinkFile)); // Should refuse to write and log error - ReportWriter.writeReportToDisk(tempDir.toString(), "report.html", "new content"); + assertThrows( + ReportWriter.ReportWriteException.class, + () -> ReportWriter.writeReportToDisk(tempDir.toString(), "report.html", "new content")); // The original file should NOT be overwritten String content = Files.readString(outsideFile); @@ -75,7 +103,7 @@ void writeReportToDisk_throwsWhenOutputDirIsDanglingSymlink(@TempDir Path tempDi assumeTrue(supportsSymlinks(), "Symbolic links not supported on this platform"); // Create a dangling symlink (target doesn't exist) - Path danglingTarget = tempDir.getParent().resolve("nonexistent_dir"); + Path danglingTarget = tempDir.resolve("nonexistent_dir"); Path symlinkDir = tempDir.resolve("dangling_link"); Files.createSymbolicLink(symlinkDir, danglingTarget); @@ -84,7 +112,9 @@ void writeReportToDisk_throwsWhenOutputDirIsDanglingSymlink(@TempDir Path tempDi assertFalse(Files.exists(danglingTarget)); // Should refuse to write and log error - ReportWriter.writeReportToDisk(symlinkDir.toString(), "test.html", "test"); + assertThrows( + ReportWriter.ReportWriteException.class, + () -> ReportWriter.writeReportToDisk(symlinkDir.toString(), "test.html", "test")); // The target directory should not be created assertFalse(Files.exists(danglingTarget), "Should not create target of dangling symlink"); @@ -95,7 +125,7 @@ void writeReportToDisk_throwsWhenOutputFileIsDanglingSymlink(@TempDir Path tempD assumeTrue(supportsSymlinks(), "Symbolic links not supported on this platform"); // Create a dangling symlink for the file - Path danglingTarget = tempDir.getParent().resolve("nonexistent_file.html"); + Path danglingTarget = tempDir.resolve("nonexistent_file.html"); Path symlinkFile = tempDir.resolve("report.html"); Files.createSymbolicLink(symlinkFile, danglingTarget); @@ -104,7 +134,9 @@ void writeReportToDisk_throwsWhenOutputFileIsDanglingSymlink(@TempDir Path tempD assertFalse(Files.exists(danglingTarget)); // Should refuse to write and log error - ReportWriter.writeReportToDisk(tempDir.toString(), "report.html", "test"); + assertThrows( + ReportWriter.ReportWriteException.class, + () -> ReportWriter.writeReportToDisk(tempDir.toString(), "report.html", "test")); // The target file should not be created assertFalse(Files.exists(danglingTarget), "Should not create target of dangling symlink"); @@ -125,6 +157,15 @@ void writeReportToDisk_writesNormallyWhenNoSymlinks(@TempDir Path tempDir) throw assertEquals("test content", content); } + @Test + void writeReportToDisk_atomicallyReplacesExistingReport(@TempDir Path tempDir) throws IOException { + ReportWriter.writeReportToDisk(tempDir.toString(), "report.html", "old"); + + ReportWriter.writeReportToDisk(tempDir.toString(), "report.html", "new"); + + assertEquals("new", Files.readString(tempDir.resolve("report.html"))); + } + @Test void writeReportToDisk_createsParentDirectories(@TempDir Path tempDir) throws IOException { // Nested directory that doesn't exist yet @@ -139,6 +180,53 @@ void writeReportToDisk_createsParentDirectories(@TempDir Path tempDir) throws IO assertEquals("test", content); } + @Test + void writeReportToDisk_rejectsNestedSymlinkedAncestor(@TempDir Path tempDir) throws IOException { + assumeTrue(supportsSymlinks(), "Symbolic links not supported on this platform"); + Path target = Files.createDirectory(tempDir.resolve("target")); + Path nested = Files.createDirectories(tempDir.resolve("safe/nested")); + Path linkedAncestor = nested.resolve("linked"); + Files.createSymbolicLink(linkedAncestor, target); + + assertThrows( + ReportWriter.ReportWriteException.class, + () -> ReportWriter.writeReportToDisk( + linkedAncestor.resolve("nested").toString(), "report.html", "blocked")); + assertFalse(Files.exists(target.resolve("nested/report.html"))); + } + + @Test + void writeReportToDisk_doesNotFollowPathComponentReplacedDuringWrite(@TempDir Path tempDir) throws Exception { + assumeTrue(supportsSymlinks(), "Symbolic links not supported on this platform"); + Path output = Files.createDirectory(tempDir.resolve("output")); + Path outside = Files.createDirectory(tempDir.resolve("outside")); + Path originalOutput = tempDir.resolve("original-output"); + String content = "x".repeat(32 * 1024 * 1024); + ExecutorService executor = Executors.newSingleThreadExecutor(); + + try (WatchService watcher = output.getFileSystem().newWatchService()) { + output.register(watcher, StandardWatchEventKinds.ENTRY_CREATE); + Future write = + executor.submit(() -> ReportWriter.writeReportToDisk(output.toString(), "report.html", content)); + + WatchKey key = watcher.poll(10, TimeUnit.SECONDS); + assertNotNull(key, "Expected the secure temporary report to be created"); + assertTrue( + key.pollEvents().stream() + .anyMatch(event -> event.context().toString().endsWith(".tmp")), + "Expected a temporary report-file creation event"); + + Files.move(output, originalOutput); + Files.createSymbolicLink(output, outside); + write.get(10, TimeUnit.SECONDS); + + assertFalse(Files.exists(outside.resolve("report.html")), "Should not follow the replacement link"); + assertEquals(content.length(), Files.size(originalOutput.resolve("report.html"))); + } finally { + executor.shutdownNow(); + } + } + @Test void containReportDirectory_allowsNormalTargetSite(@TempDir Path tempDir) throws IOException { File baseDir = tempDir.toFile(); @@ -180,4 +268,15 @@ void containReportDirectory_usesBaseDirAsRoot(@TempDir Path tempDir) throws IOEx String result = ReportWriter.containReportDirectory(baseDir, "target/site"); assertEquals(baseDir.toPath().resolve("target/site").normalize().toString(), result); } + + @Test + void containReportDirectory_rejectsSymlinkedAncestor(@TempDir Path tempDir) throws IOException { + assumeTrue(supportsSymlinks(), "Symbolic links not supported on this platform"); + Files.createDirectory(tempDir.resolve("outside")); + Files.createSymbolicLink(tempDir.resolve("linked"), tempDir.resolve("outside")); + + assertThrows( + IllegalArgumentException.class, + () -> ReportWriter.containReportDirectory(tempDir.toFile(), "linked/site")); + } } From 16427468de8d49e20fdb5a4da0ca3263aca3e77c Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 02:40:48 +0000 Subject: [PATCH 3/3] Fix 2 failing CI check(s): GitHub Actions: Java CI with Maven (PR) / 0_build.txt, GitHub Actions: Java CI with Maven (PR) / build Co-Authored-By: CodeRabbit --- .../org/hjug/refactorfirst/report/HtmlReportKotlinTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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"];