diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 142c2dd7..4b8b4800 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,7 +55,7 @@ jobs: # in qa) run in build-and-test below, which now also covers docs-only PRs. run: | ./mvnw -B -ntp clean \ - "-Dtest=EnginePdfBoundaryTest,DocumentationCoverageTest,CanonicalSurfaceGuardTest,PackageMapGuardTest,VersionConsistencyGuardTest,CiGuardListGuardTest" \ + "-Dtest=EnginePdfBoundaryTest,DocumentationCoverageTest,CanonicalSurfaceGuardTest,PackageMapGuardTest,VersionConsistencyGuardTest,CiGuardListGuardTest,CodeQlScopeGuardTest" \ test -pl :graph-compose-core changes: diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 388a40ca..c3e763e8 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -50,15 +50,25 @@ jobs: languages: ${{ matrix.language }} queries: security-and-quality - - name: Build (engine module) - # Scoped to the engine module alone: no `-am`, so nothing but core is built - # and nothing but core reaches the extractor. Since the 2.0 split - # graph-compose-core is one of several artifacts published to Maven Central, - # which makes this narrower than both the publish train and the canonical - # verify gate in ci.yml — the render backends and templates ship unscanned. - # Widening it to the code-bearing published modules is tracked for 2.2, - # together with a triage pass on the standing alert set. - run: ./mvnw -B -ntp -DskipTests -pl :graph-compose-core package + - name: Build (published code-bearing modules) + # The extractor sees what the build compiles, so the module list is the + # analysis scope. It covers every published module that carries code, which + # is where the untrusted input actually lands: PDFBox and POI parsing, the + # SVG and image paths, font loading, and the ZIP/OPC writers all live in the + # backends rather than in core. + # + # Every deployed module is named outright, including fonts and emoji, which no + # other `-pl` list mentions. They would arrive through `-am` as somebody's + # dependency, and that is precisely the arrangement that ends quietly the day + # the dependency is dropped. `-am` stays for a future module whose upstream is + # not itself deployed. + # + # CodeQlScopeGuardTest holds this list against two inventories — what CI + # compiles and what the publish workflows deploy — so a module that ships and + # is not named here fails the build rather than shipping unscanned. + run: | + ./mvnw -B -ntp -DskipTests package -am \ + -pl :graph-compose-core,:graph-compose-render-pdf,:graph-compose-render-docx,:graph-compose-render-pptx,:graph-compose-templates,:graph-compose-testing,:graph-compose-fonts,:graph-compose-emoji - name: Perform CodeQL analysis uses: github/codeql-action/analyze@v4 diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f5090f7..11bbd6d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,15 @@ follow semantic versioning; release dates are ISO 8601. version and link the tag it names. `SECURITY.md`, `SUPPORT.md`, `ROADMAP.md` and `.github/` are scanned for the first time; historical records are skipped by path, so a new archived page is covered the day it lands. +- **Code scanning reaches the render backends.** The scan compiled the engine module + alone, and the Java extractor sees only what the build compiles — so the PDFBox and + POI parsing paths, the SVG and image handling, font loading and the ZIP/OPC writers + were not partially analysed, they were absent from the scan, with nothing in a green + result to say so. Every deployed module that carries code is now named outright, and a + guard holds that list against two inventories that fail differently: what CI compiles, + and what a release deploys. The second is what catches an artifact added to the publish + train and forgotten everywhere else — the first cannot, because a module missing from + both lists leaves them in perfect agreement. - **The package map is derived from the source tree.** A backend was findable only if someone remembered to list it, and the backend-neutral fixed-layout SPI was missing from the contributing guide — the one document a reader consults before adding an diff --git a/core/src/test/java/com/demcha/documentation/CodeQlScopeGuardTest.java b/core/src/test/java/com/demcha/documentation/CodeQlScopeGuardTest.java new file mode 100644 index 00000000..7b587b37 --- /dev/null +++ b/core/src/test/java/com/demcha/documentation/CodeQlScopeGuardTest.java @@ -0,0 +1,195 @@ +package com.demcha.documentation; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; +import java.util.regex.Pattern; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Keeps the CodeQL analysis scope from falling behind the build. + * + *

The Java extractor sees whatever the build compiles, so the {@code -pl} list in the + * scanning workflow is the analysis scope — a module absent from it is not + * partially covered, it is not scanned at all. That is invisible: the job stays green, + * the badge stays green, and the alert list simply never mentions the module. It held + * for a full release, with the PDFBox, POI, SVG, font and ZIP/OPC paths — the code most + * exposed to untrusted input — outside the scan while core was analysed. + * + *

The expectation is derived rather than listed, from two sources that fail + * differently. Against the verify gate, a module CI compiles must also be scanned — that + * catches the scan falling behind the build. Against the publish workflows, a module a + * release deploys must be scanned — that catches what the first comparison cannot, since + * a new artifact forgotten in both CI and the scan is missing from both sides of it and + * the sets agree perfectly. + */ +class CodeQlScopeGuardTest { + + private static final Path PROJECT_ROOT = RepoRoot.get(); + private static final Path CI_WORKFLOW = PROJECT_ROOT.resolve(".github/workflows/ci.yml"); + private static final Path CODEQL_WORKFLOW = PROJECT_ROOT.resolve(".github/workflows/codeql.yml"); + + /** + * A {@code -pl} value. Selectors are collected token by token rather than by one + * regex: a wrapped command joins into {@code -pl :a,:b, :c -am}, and a pattern that + * stopped at the first space would silently shrink the very set it compares against. + */ + private static final Pattern OPTION = Pattern.compile("^-{1,2}\\w.*"); + + @Test + void everyCodeBearingModuleTheVerifyGateBuildsIsAlsoScanned() throws IOException { + Set verified = modulesWithSources(selectorsFrom(CI_WORKFLOW, "clean verify")); + Set scanned = modulesWithSources(selectorsFrom(CODEQL_WORKFLOW, "package")); + + assertThat(verified) + .describedAs("no code-bearing module was found in the ci.yml verify gate — the " + + "gate's -pl list or the module layout moved, and this guard is comparing " + + "two empty sets") + .isNotEmpty(); + + Set unscanned = new TreeSet<>(verified); + unscanned.removeAll(scanned); + + assertThat(unscanned) + .describedAs("a published module that CI compiles but CodeQL does not: the " + + "extractor only sees what the build compiles, so this code is not " + + "partially analysed, it is absent from the scan entirely — and nothing " + + "about the result says so") + .isEmpty(); + } + + /** + * Everything a release deploys, and that carries code, is scanned. + * + *

The independent half. Comparing the scan against CI answers the narrower + * question of whether the two lists agree, and two lists agree perfectly when a + * module is missing from both — which is the shape a new artifact takes when it is + * added to the publish train and forgotten everywhere else. The inventory here comes + * from the publish workflows, so the question becomes the one the promise makes: + * does the scan cover what users can actually depend on.

+ */ + @Test + void everyDeployedModuleWithSourcesIsScanned() throws IOException { + Set deployed = withSources(PublishedModules.deployed(PROJECT_ROOT)); + Set scanned = modulesWithSources(selectorsFrom(CODEQL_WORKFLOW, "package")); + + assertThat(deployed) + .describedAs("no deployed module with sources was found — the publish workflows' " + + "deploy steps moved, and this guard is comparing against nothing") + .isNotEmpty(); + + Set unscanned = new TreeSet<>(deployed); + unscanned.removeAll(scanned); + + assertThat(unscanned) + .describedAs("a module a release deploys, and whose code a consumer therefore " + + "runs, is outside the scan. Naming it in the scan's -pl list is the fix; " + + "relying on it arriving as somebody's dependency is not, because that " + + "stops the day the dependency does") + .isEmpty(); + } + + /** + * The {@code -pl} selectors of the {@code mvnw} invocation carrying {@code goal}, or + * every reactor module when the command carries no {@code -pl} at all — a build + * without one compiles the whole reactor, which is more coverage, not less. + */ + private static List selectorsFrom(Path workflow, String goal) throws IOException { + List tokens = List.of(buildStep(workflow, goal).strip().split("\\s+")); + int at = tokens.indexOf("-pl"); + if (at < 0) { + return PublishedModules.of(PROJECT_ROOT).stream().map(module -> ":" + module).toList(); + } + + StringBuilder selectors = new StringBuilder(); + for (String token : tokens.subList(at + 1, tokens.size())) { + if (OPTION.matcher(token).matches()) { + break; + } + selectors.append(token); + } + assertThat(selectors.length()) + .describedAs("the -pl in the %s invocation of %s is followed by no selectors", + goal, PROJECT_ROOT.relativize(workflow)) + .isPositive(); + return List.of(selectors.toString().split(",")); + } + + /** + * The text of the {@code mvnw} invocation carrying {@code goal}, joined across the + * line continuations the longer commands are wrapped in. + */ + private static String buildStep(Path workflow, String goal) throws IOException { + List lines = Files.readAllLines(workflow); + List matches = new ArrayList<>(); + for (int i = 0; i < lines.size(); i++) { + if (!lines.get(i).contains("mvnw")) { + continue; + } + StringBuilder command = new StringBuilder(lines.get(i)); + for (int j = i + 1; j < lines.size() && command.toString().strip().endsWith("\\"); j++) { + command.append(' ').append(lines.get(j)); + } + String joined = command.toString().replace("\\", " "); + if (joined.contains(goal)) { + matches.add(joined); + } + } + + assertThat(matches) + .describedAs("no mvnw invocation carrying '%s' in %s — the step this guard reads " + + "was renamed or its goal changed", goal, PROJECT_ROOT.relativize(workflow)) + .isNotEmpty(); + + // A module selection is what this guard is after, so prefer the invocation that + // carries one. Falling back to the first match keeps a command that drops `-pl` + // entirely readable rather than throwing — building the whole reactor scans more, + // not less, and a guard that fails on the stricter setup would push toward the + // looser one. + return matches.stream().filter(command -> command.contains("-pl")).findFirst() + .orElse(matches.get(0)); + } + + /** Of the given module directories, the artifact ids of those carrying main sources. */ + private static Set withSources(java.util.List moduleDirectories) throws IOException { + Map byArtifactId = PublishedModules.byArtifactId(PROJECT_ROOT); + Set withSources = new TreeSet<>(); + byArtifactId.forEach((artifactId, directory) -> { + String name = directory.getFileName().toString(); + if (moduleDirectories.contains(name) + && Files.isDirectory(directory.resolve("src/main/java"))) { + withSources.add(artifactId); + } + }); + return withSources; + } + + /** Of the given {@code :artifact-id} selectors, those whose module carries main sources. */ + private static Set modulesWithSources(List selectors) throws IOException { + Map byArtifactId = PublishedModules.byArtifactId(PROJECT_ROOT); + + assertThat(byArtifactId) + .describedAs("no module resolved to a directory — the root pom's module list or " + + "the poms' own coordinates moved, and every selector below would be " + + "dropped as unknown") + .isNotEmpty(); + + Set withSources = new TreeSet<>(); + for (String selector : selectors) { + String artifactId = selector.strip().replaceFirst("^:", ""); + Path module = byArtifactId.get(artifactId); + if (module != null && Files.isDirectory(module.resolve("src/main/java"))) { + withSources.add(artifactId); + } + } + return withSources; + } +} diff --git a/core/src/test/java/com/demcha/documentation/PublishedModules.java b/core/src/test/java/com/demcha/documentation/PublishedModules.java new file mode 100644 index 00000000..2871fcf8 --- /dev/null +++ b/core/src/test/java/com/demcha/documentation/PublishedModules.java @@ -0,0 +1,103 @@ +package com.demcha.documentation; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * The reactor's modules, resolved from the root {@code pom.xml} for the guards that + * reason about them. + * + *

Two guards need the same answer to "what is a module, and which directory is it" — + * one checks that every backend package is documented, the other that every compiled + * module is scanned. Answering it twice is how the lists this repository keeps fixing + * came apart in the first place.

+ */ +final class PublishedModules { + + private static final Pattern MODULE = Pattern.compile("\\s*([^<]+?)\\s*"); + private static final Pattern ARTIFACT_ID = Pattern.compile("\\s*([^<]+?)\\s*"); + private static final Pattern PARENT_BLOCK = + Pattern.compile(".*?", Pattern.DOTALL); + + private PublishedModules() { + } + + /** A publish workflow's deploy step: {@code -f /pom.xml … deploy}. */ + private static final Pattern DEPLOY_STEP = + Pattern.compile("-f\\s+([\\w-]+)/pom\\.xml"); + + /** + * The modules a release actually deploys, read from the publish workflows. + * + *

The independent inventory. Comparing the scan against CI alone answers a + * narrower question than the one that matters: a module added to the publish train + * and forgotten in both CI and the scan is missing from both sides of that + * comparison, which is precisely the shape that keeps it green.

+ */ + static List deployed(Path repoRoot) throws IOException { + Path workflows = repoRoot.resolve(".github/workflows"); + List deployed = new ArrayList<>(); + try (var files = Files.list(workflows)) { + for (Path workflow : files.sorted().toList()) { + String name = workflow.getFileName().toString(); + if (!name.startsWith("publish") || !name.endsWith(".yml")) { + continue; + } + for (String line : Files.readAllLines(workflow)) { + if (!line.contains("deploy")) { + continue; + } + Matcher module = DEPLOY_STEP.matcher(line); + if (module.find() && !deployed.contains(module.group(1))) { + deployed.add(module.group(1)); + } + } + } + } + return deployed; + } + + /** The module directories the root reactor builds, in declaration order. */ + static List of(Path repoRoot) throws IOException { + String rootPom = Files.readString(repoRoot.resolve("pom.xml")); + List modules = new ArrayList<>(); + Matcher matcher = MODULE.matcher(rootPom); + while (matcher.find()) { + modules.add(matcher.group(1)); + } + return modules; + } + + /** + * Each module's own artifact id, mapped to its directory. + * + *

Read from the module's own {@code } rather than by searching the + * poms for a name: every pom that depends on a module also contains that + * module's artifact id, so a search binds {@code graph-compose-testing} to whichever + * dependent happens to come first in the reactor.

+ */ + static Map byArtifactId(Path repoRoot) throws IOException { + Map modules = new LinkedHashMap<>(); + for (String module : of(repoRoot)) { + Path pom = repoRoot.resolve(module).resolve("pom.xml"); + if (!Files.isRegularFile(pom)) { + continue; + } + // The inherited coordinate sits in above the module's own; drop it + // so the first remaining artifactId is the module speaking about itself. + String ownCoordinates = PARENT_BLOCK.matcher(Files.readString(pom)).replaceFirst(""); + Matcher artifactId = ARTIFACT_ID.matcher(ownCoordinates); + if (artifactId.find()) { + modules.put(artifactId.group(1), repoRoot.resolve(module)); + } + } + return modules; + } +}