#12572 introduced mvnlog - #12716
Conversation
gnodet
left a comment
There was a problem hiding this comment.
Review of #12716 — mvnlog build report system
This PR adds a build report generation system and an mvnlog viewer tool. Several issues were identified that should be addressed before merging.
Critical Issues
1. Breaking interface change in BuildEventListener
projectFinished(String projectId) is replaced with projectFinished(String projectId, String status) and mojoFinished(ExecutionEvent event, String status) is added — neither has a default method implementation. Any third-party code implementing BuildEventListener (e.g., Maven extensions, custom build listeners) will fail to compile. At minimum, preserve the old signature as a default method delegating to the new one, and add an empty default body for mojoFinished.
2. Path traversal vulnerability in /api/report endpoint (LogInvoker.java)
The id query parameter is passed directly to Paths.get(".mvn", "reports", reqId) without sanitization. A request with id=../../../etc/passwd resolves to a path outside the reports directory and Files.readAllBytes() serves the file. Combined with the server binding to 0.0.0.0 (all network interfaces), this is exploitable from the network. The id value must be validated to be a simple filename (no path separators, no ..), and the server should bind to InetAddress.getLoopbackAddress() instead.
3. System.exit(0) in inactivity shutdown timer (LogInvoker.java)
If mvnlog runs in embedded mode (via the MavenLogCling 5-arg main entry point), System.exit(0) terminates the host JVM instead of just stopping the server. Other Maven tools use graceful shutdown patterns — stop the server and let the thread return.
4. No opt-out for report generation (LookupInvoker.java)
BuildReportEventListener is unconditionally enabled for all mvn commands. Every build will write a JSON report and accumulate all log messages in memory. Users should be able to disable this (e.g., -Dmaven.build.report.skip=true).
5. CopyOnWriteArrayList for log entries — O(n²) overhead (BuildReportEventListener.java)
All log entries are stored in a CopyOnWriteArrayList, which copies the entire backing array on every add(). For a large multi-module build producing tens of thousands of log lines, this results in quadratic memory copies and potential OOM. A ConcurrentLinkedQueue or synchronized ArrayList would be far more appropriate.
Medium Issues
6. Static lastRequestTime field (LogInvoker.java) — shared across all instances via private static volatile; should be an instance field.
7. External CDN dependency (report.html) — loads Google Fonts from fonts.googleapis.com, which won't load in air-gapped/corporate environments. Consider system fonts or bundled fonts.
8. Fragile JSON parsing via regex (LogInvoker.java) — regex patterns like "id"\\s*:\\s*"([^"]+)" assume exact field ordering and break on escaped quotes. A minimal JSON parser would be more robust.
Additional Observations
- This PR overlaps with the existing PR chain (#12694–#12699) that decomposes the same issue (#12572) into independently reviewable pieces. Coordination with that work would be beneficial.
- 2470 lines of new production code with zero test files. Unit tests for
BuildReportEventListener, CLI options parsing, report file discovery, and JSON serialization are needed. com.sun.net.httpserver.HttpServeris not part of the Java SE specification and may not be available in all JDK distributions.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
This plan outlines the implementation of mvnlog, a CLI tool to explore Maven build reports. When run with --web, it serves an interactive web-based report viewer from a lightweight embedded Java HTTP server.
User Review Required
IMPORTANT
The implementation leverages the built-in JDK com.sun.net.httpserver.HttpServer which has zero external dependencies and runs out of the box in standard JRE/JDK configurations (since Java 6).
TIP
To capture detailed performance timings ( mojo starts/ends, warnings/errors, and logs per mojo), the core Maven execution listener will be updated to intercept these details during the build and serialize them into .mvn/reports/build-report-.json upon session completion.
Open Questions
None. The requirements and architecture are fully specified.
Proposed Changes
Maven API CLI Layer
[MODIFY]
Tools.java
Add definitions for MVNLOG_CMD = "mvnlog" and MVNLOG_NAME = "Maven Log Viewer Tool".
[MODIFY]
ParserRequest.java
Add static factory builder methods: mvnlog(String[] args, MessageBuilderFactory messageBuilderFactory) and mvnlog(List args, MessageBuilderFactory messageBuilderFactory).
[NEW]
LogOptions.java
Define options interface for mvnlog (methods web(), port(), and file()).
Core Logging Layer
[MODIFY]
BuildEventListener.java
Add void mojoFinished(ExecutionEvent event, String status) to track mojo execution endings.
Update void projectFinished(String projectId) to void projectFinished(String projectId, String status) to record module build status.
[MODIFY]
LoggingExecutionListener.java
Invoke buildEventListener.mojoFinished(...) in mojoSucceeded, mojoFailed, and mojoSkipped.
Update calls to buildEventListener.projectFinished(...) to include status ("SUCCESS", "FAILED", or "SKIPPED").
[MODIFY]
SimpleBuildEventListener.java
Implement updated interface methods as no-ops.
[NEW]
BuildReportEventListener.java
Implements BuildEventListener. Intercepts all session events and log messages, tracks start/end times of projects and mojos, groups log messages per active mojo, and formats warnings/errors as structured problems.
Writes the structured report to .mvn/reports/build-report-.json upon session success or failure.
CLI & Launcher Layer
[MODIFY]
LookupInvoker.java
Update doDetermineBuildEventListener to wrap the default listener in BuildReportEventListener when running Maven builds.
[NEW]
MavenLogCling.java
Entry point for the new mvnlog command, extending ClingSupport.
[NEW]
LogContext.java
Context holder for mvnlog execution.
[NEW]
LogParser.java
Parses the arguments for mvnlog command.
[NEW]
CommonsCliLogOptions.java
Command line option parser implementing LogOptions.
[NEW]
LogInvoker.java
Renders build report as formatted console output by default.
If --web is provided, spins up a com.sun.net.httpserver.HttpServer, opens the default web browser via java.awt.Desktop.browse(URI), and terminates after 30 minutes of inactivity or Ctrl+C.
[NEW]
report.html
Interactive web report viewer. Bundled in classpath. Features dark/light themes, module gantt charts, collapsible logs per mojo, filterable problems list, syntax highlighting for failures, and virtual scrolling build log viewer.
Scripts & Packaging
[MODIFY]
mvn
Add --log script argument handling to run org.apache.maven.cling.MavenLogCling.
[MODIFY]
mvn.cmd
Add --log batch argument handling to run org.apache.maven.cling.MavenLogCling.
[NEW]
mvnlog
Script launcher that routes arguments to mvn --log "$@".
[NEW]
mvnlog.cmd
Windows command script that routes arguments to mvn.cmd --log %*.
[MODIFY]
component.xml
Include mvnlog in the unix shell script zip/tar.gz packaging group.
Verification Plan
Automated Tests
Build the project using Maven: mvn clean install -DskipTests (to confirm compile success)
Run CLI parsing and execution tests.
Verify JSON formatting output.
Manual Verification
Execute a sample multi-module build to generate the .mvn/reports/build-report-.json report.
Run mvnlog --web to launch the web-based report viewer on a custom port and check timeline rendering, problem sorting, and log search details.