On Fri, 25 Apr 2025 11:24:11 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please help review this PR which disables the unspecified but long-standing >> feature where an `FtpURLConnection` is opened as a fallback for non-local >> file URLs. >> >> Before this change, if a file URL has a non-local host component, say >> `file://remotehost/folder/data.txt`, then the implementation would attempt >> opening an FTP connection to `remotehost`. After this change, such URLs will >> be rejected with a `MalformedURLException`, unless the FTP fallback feature >> is explicitly re-enabled via a system property. >> >> This change was initially discussed here: >> https://mail.openjdk.org/pipermail/net-dev/2025-March/025988.html >> >> See the above discussion and CSR draft JDK-8354678 for the motivation for >> this change. >> >> This PR: >> >> * Changes file URL `Handler::openConnection` implementation for unix/windows >> to throw `MalformedURLException`, unless the FTP fallback feature is >> explicitly enabled by configuration. >> * Introduces a new system property `jdk.net.file.ftpenabled` which when set >> to `true` re-enables the feature. >> * Documents the new property in `net-properties.html` >> * Updates the existing test `NonLocalFtpFallback` to enable the feature via >> said system property. >> * Adds a new test `NonLocalFtpFallbackDisabled` verifying that a >> `MalformedURLException` is thrown by default for a non-local URL host >> component. >> * Moves testing of exceptional behavior in FtpConnection when using >> non-local file URLs with FTP fallback enabled from `OpenStream` to >> `NonLocalFtpFallback` >> >> I have added a Release Note as a subtask in the JBS issue, this also needs a >> review. > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 12 additional > commits since the last revision: > > - Merge branch 'master' into disable-non-local-file-urls > - Add @comment referencing testing of non-local file URLs in > NonLocalFtpFallback > - verifyNonLocalFileURLRejected is a better name for this test which expects > the FTP fallback to be disabled > - Move testing of exceptional behavior of a non-local, FTP-fallback enabled > file URL from OpenStream to NonLocalFtpFallback > - Add a test verifying that local file URLs such as > 'file://localhost/folder/data.xml' are not affected by the FTP fallback > feature being enabled > - Merge branch 'master' into disable-non-local-file-urls > - Fix repeated "should should" > - Move file URL properties out of the Proxy section, replace URI with URL > - Be explicit about explicit {@code ftp://} URIs > - Document jdk.net.file.ftpfallback as a Networking Property > - ... and 2 more: https://git.openjdk.org/jdk/compare/03e34e30...af73727f Sorry - rewording the release note got me thinking again... src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java line 55: > 53: System.getProperty("jdk.net.file.ftpfallback", "false") > 54: ); > 55: Maybe we should use `Boolean.getBoolean("jdk.net.file.ftpfallback")` here. test/jdk/sun/net/www/protocol/file/NonLocalFtpFallbackDisabled.java line 58: > 56: // We can use a fake host name here, no actual FTP request will > be made > 57: String hostname = "remotehost"; > 58: URL nonLocalURL = new URL("file", hostname, localURL.getFile()); Should we also check that `new URL("file", hostname, "").openConnection()` will throw MUE? ------------- PR Review: https://git.openjdk.org/jdk/pull/24657#pullrequestreview-2794825750 PR Review Comment: https://git.openjdk.org/jdk/pull/24657#discussion_r2060628151 PR Review Comment: https://git.openjdk.org/jdk/pull/24657#discussion_r2060634329
