Introduce DirectoryBrowserSupportFilter extension point - #27216
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces an extension point for plugins to transform files served by DirectoryBrowserSupport, supporting use cases such as PR #7288’s GZIP artifact viewing.
Changes:
- Adds a mutable filtering context for streams and response metadata.
- Applies registered filters to view and normal file responses.
- Adds stream and Markdown transformation tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
DirectoryBrowserSupportFilter.java |
Defines the extension API and filtering context. |
DirectoryBrowserSupport.java |
Integrates filters into file-serving paths. |
DirectoryBrowserSupportTest.java |
Tests content and metadata transformations. |
Suppressed comments (1)
core/src/main/java/hudson/model/DirectoryBrowserSupport.java:453
- Do not continue serving after a filter failure. A filter may already have consumed the current stream or partially mutated its metadata before throwing, so catching the exception can return empty/corrupt content; after a successful decrypting filter followed by a failing sanitizer, it can also serve an unsafe intermediate representation. Fail the request, or restore a pristine stream and context before continuing.
} catch (Exception e) {
LOGGER.log(Level.WARNING, "Failed to filter stream for " + baseFile + " using " + filter, e);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (contentType != null) { | ||
| rsp.setContentType(contentType); | ||
| } |
| * Sets a new {@link InputStream} for the file content. | ||
| */ | ||
| public void setInputStream(@NonNull InputStream inputStream) { |
| } | ||
| } | ||
|
|
||
| @TestExtension("directoryBrowserSupportFilterTest") |
| * @return the transformed context (or the same context), or {@code null} if no changes are made | ||
| * @throws IOException if an I/O error occurs during filtering | ||
| */ | ||
| public abstract Context filter(@NonNull Context context) throws IOException; |
| DirectoryBrowserSupportFilter.Context context = applyFilters(req, baseFile, in, length, false); | ||
| in = context.getInputStream(); | ||
| length = context.getLength(); | ||
| String fileName = context.getFileName(); | ||
| String contentType = context.getContentType(); |
Allows plugins to filter or transform file input streams and HTTP response metadata served by DirectoryBrowserSupport (such as build artifacts and workspace files). Potential use cases for plugins include, for example: - Performing on-the-fly stream decompression (e.g., viewing GZIP artifacts). - Performing artifact decryption. Security & Resource Management: - DirectoryBrowserSupportFilter.Context implements AutoCloseable to track and close superseded streams, preventing resource leaks. - Fail-fast exception handling aborts failed filter executions cleanly.
4218aee to
3712c1f
Compare
|
@timja can you re-trigger the copilot review please? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (6)
core/src/main/java/hudson/model/DirectoryBrowserSupportFilter.java:159
close()leaves the activeinputStreamopen. This leaks the stream fromVirtualFile.open()when a filter throws beforeserveFileis called (the error path atapplyFilterscalls this method, but the current stream is not insupersededStreams). Close the current stream first, then the superseded streams so wrapper dependencies are released in the right order.
@Override
public void close() {
for (InputStream s : supersededStreams) {
IOUtils.closeQuietly(s);
}
core/src/main/java/hudson/model/DirectoryBrowserSupport.java:434
- This catch also handles failures from
rsp.serveFile; if streaming has already committed the response, the subsequentsendErroris invalid and may replace the useful client-disconnect exception with an illegal response-state failure. Limit this handling toapplyFilters, or rethrow committed-response failures.
} catch (IOException ioe) {
LOGGER.log(Level.WARNING, "Failed to serve file for " + baseFile, ioe);
rsp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
core/src/main/java/hudson/model/DirectoryBrowserSupport.java:446
- Replacing the context drops ownership of the previous context and all streams it tracks. Since the API explicitly permits returning a transformed
Context, a filter that constructs a new context leaves the prior file stream open and can leak a descriptor on every request. Make the pipeline mutate one context (for example, makefilterreturnvoid/disallow a different instance), or define and implement an ownership transfer that safely handles wrapper streams.
DirectoryBrowserSupportFilter.Context next = filter.filter(context);
if (next != null) {
context = next;
core/src/main/java/hudson/model/DirectoryBrowserSupportFilter.java:124
- Replacing the stream retains the old content length, even though transformations such as the advertised decompression normally change it. If a plugin does not also call
setLength,serveFilereceives a stale length and may emit an incorrectContent-Length, truncating the response or causing clients to wait for bytes that never arrive. Invalidate the length on replacement; filters that know the new size can set it afterward.
public void setInputStream(@NonNull InputStream inputStream) {
if (this.inputStream != inputStream) {
this.supersededStreams.add(this.inputStream);
this.inputStream = inputStream;
}
core/src/main/java/hudson/model/DirectoryBrowserSupport.java:395
- This catch also covers
rsp.serveFile, which can throw after response headers/body have been committed (for example, on a client disconnect). CallingsendErroron an already committed response is invalid and can mask the original I/O failure. Scope the catch to filter setup, or rethrow whenrsp.isCommitted()is true.
This issue also appears on line 432 of the same file.
} catch (IOException ioe) {
LOGGER.log(Level.WARNING, "Failed to serve file for " + baseFile, ioe);
rsp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java:1569
- The added tests only exercise stream and length replacement; none covers the new response-metadata path (
setFileNamedriving MIME type/headers), although the PR description claims a Markdown-to-HTML content-type test. Add a test filter that changes the filename to an HTML extension and assert the resultingContent-Typeand disposition behavior.
context.setInputStream(new java.io.ByteArrayInputStream(bytes));
context.setLength(bytes.length);
Allows plugins to filter or transform file input streams and HTTP response metadata served by DirectoryBrowserSupport (such as build artifacts and workspace files).
This extension point enables plugins to:
Implement a plugin extension point suitable for provided the desired feature of PR #7288
Testing done
DirectoryBrowserSupportTest.java:directoryBrowserSupportFilterTest: Verifies stream interception and modification on served files.directoryBrowserSupportMarkdownRenderingTest: Verifies converting Markdown content to HTML and settingContent-Type: text/html;charset=UTF-8dynamically.gzip-artifact-view-plugin):GzipDirectoryBrowserSupportFilterTestverified transparent on-the-fly decompression of.txt.gzbuild artifacts when viewed over HTTP.BUILD SUCCESS).Screenshots (UI changes only)
Before
After
Proposed changelog entries
DirectoryBrowserSupportFilterextension point to allow plugins to filter or transform file content served byDirectoryBrowserSupport.Proposed changelog category
/label developer
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@jenkinsci/core-pr-reviewers
@timja
@dwnusbaum
@NotMyFault
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.