Copilot commented on code in PR #11785: URL: https://github.com/apache/gravitino/pull/11785#discussion_r3503167573
########## common/src/main/java/org/apache/gravitino/utils/RemoteFileDownloader.java: ########## @@ -0,0 +1,396 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.utils; + +import java.io.BufferedInputStream; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.Socket; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.Optional; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLSocketFactory; + +/** + * Downloads an {@code http} or {@code https} file while pinning the TCP connection to a + * pre-validated address. + * + * <p>This closes the DNS-rebinding TOCTOU window between SSRF validation and the actual fetch: the + * host is resolved and validated once by {@link RemoteUriValidator#resolveAndValidate}, and the + * resulting {@link InetAddress} is connected to directly here, so the hostname is never + * re-resolved. A minimal HTTP/1.1 client is used so the connection can be pinned at the socket + * level (the JDK exposes no per-connection address override for plain {@code HttpURLConnection}, + * and silently drops a {@code Host} header set via {@code setRequestProperty}). The original + * hostname is still used for the HTTP {@code Host} header and, for {@code https}, for TLS SNI and + * certificate-hostname verification, so virtual hosting and certificate validation keep working. + * Redirects are not followed, because a redirect target would re-resolve to an unvalidated address. + * + * <p>{@code ftp} is intentionally not handled here: the FTP data channel is opened to an address + * the server chooses in its {@code PASV}/{@code EPSV} reply, which cannot be pinned, so {@link + * FileFetcher} rejects {@code ftp} on the SSRF-blocking path instead. + * + * <p>Only responses framed by {@code Content-Length} or chunked transfer-encoding are accepted; a + * connection-close-delimited body is rejected because a premature close cannot be told apart from a + * complete one. Combined with streaming to a sibling temporary file that is atomically moved into + * place only on success, this means a connection that drops mid-transfer never leaves a truncated + * destination behind. Response and header sizes are bounded to prevent a malicious pinned host from + * exhausting disk or memory. + */ +final class RemoteFileDownloader { + + private static final int DEFAULT_HTTP_PORT = 80; + private static final int DEFAULT_HTTPS_PORT = 443; + private static final int MAX_HEADER_BYTES = 64 * 1024; + private static final long MAX_BODY_BYTES = 2L * 1024 * 1024 * 1024; + private static final int COPY_BUFFER_BYTES = 8192; + + private RemoteFileDownloader() {} + + /** + * Downloads {@code uri} into {@code destFile}, connecting only to {@code pinnedAddress}. + * + * @param uri the source URI; must use the {@code http} or {@code https} scheme + * @param pinnedAddress the validated address to connect to + * @param destFile the local destination file + * @param timeoutMs the connect/read timeout in milliseconds + * @throws IOException if the file cannot be fetched or the response is not successful + */ + static void download(URI uri, InetAddress pinnedAddress, File destFile, int timeoutMs) + throws IOException { + String scheme = Optional.ofNullable(uri.getScheme()).orElse("").toLowerCase(Locale.ROOT); + if (!scheme.equals("http") && !scheme.equals("https")) { + throw new IllegalArgumentException( + String.format("Pinned download does not support scheme '%s'", scheme)); + } + + Path destPath = destFile.toPath().toAbsolutePath(); + Path tempPath = + Files.createTempFile(destPath.getParent(), destPath.getFileName() + ".", ".tmp"); + try { + httpDownload(uri, pinnedAddress, tempPath, timeoutMs, scheme.equals("https")); + Files.move(tempPath, destPath, StandardCopyOption.REPLACE_EXISTING); + } catch (Throwable e) { + // Catch every throwable (incl. Error/OutOfMemoryError) so the temp file is never orphaned in + // the staging directory. Precise rethrow keeps the declared `throws IOException`. + try { + Files.deleteIfExists(tempPath); + } catch (IOException suppressed) { + e.addSuppressed(suppressed); + } + throw e; Review Comment: `catch (Throwable e)` rethrows `e` directly (`throw e;`) even though `download(...)` only declares `throws IOException`. This will not compile because `Throwable` is a broader checked type. Convert the rethrow into (a) `IOException` passthrough, (b) unchecked rethrow for `RuntimeException`/`Error`, and (c) wrap any other `Throwable` in an `IOException` after cleaning up the temp file. ########## common/src/main/java/org/apache/gravitino/utils/FileFetcher.java: ########## @@ -142,7 +168,19 @@ private synchronized void linkLocalFile(URI uri, File destFile) throws IOExcepti Path tmpPath = destPath.resolveSibling(destPath.getFileName() + ".symlink.tmp"); Files.deleteIfExists(tmpPath); Files.createSymbolicLink(tmpPath, srcPath); - Files.move(tmpPath, destPath, StandardCopyOption.REPLACE_EXISTING); + try { + Files.move(tmpPath, destPath, StandardCopyOption.REPLACE_EXISTING); + } catch (Throwable e) { + // Do not orphan the temporary symlink if the rename fails (e.g. read-only or cross-device + // destination). This method is synchronized on the singleton, so the fixed temp name cannot + // race with a concurrent local fetch. + try { + Files.deleteIfExists(tmpPath); + } catch (IOException suppressed) { + e.addSuppressed(suppressed); + } + throw e; Review Comment: Same issue as in `RemoteFileDownloader`: this `catch (Throwable e)` ends with `throw e;` inside a method that only declares `throws IOException`. That will not compile because `Throwable` includes checked types. Preserve the cleanup logic but rethrow as `IOException`/unchecked appropriately. ########## common/src/main/java/org/apache/gravitino/utils/RemoteFileDownloader.java: ########## @@ -0,0 +1,396 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.utils; + +import java.io.BufferedInputStream; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.Socket; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.Optional; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLSocketFactory; + +/** + * Downloads an {@code http} or {@code https} file while pinning the TCP connection to a + * pre-validated address. + * + * <p>This closes the DNS-rebinding TOCTOU window between SSRF validation and the actual fetch: the + * host is resolved and validated once by {@link RemoteUriValidator#resolveAndValidate}, and the + * resulting {@link InetAddress} is connected to directly here, so the hostname is never + * re-resolved. A minimal HTTP/1.1 client is used so the connection can be pinned at the socket + * level (the JDK exposes no per-connection address override for plain {@code HttpURLConnection}, + * and silently drops a {@code Host} header set via {@code setRequestProperty}). The original + * hostname is still used for the HTTP {@code Host} header and, for {@code https}, for TLS SNI and + * certificate-hostname verification, so virtual hosting and certificate validation keep working. + * Redirects are not followed, because a redirect target would re-resolve to an unvalidated address. + * + * <p>{@code ftp} is intentionally not handled here: the FTP data channel is opened to an address + * the server chooses in its {@code PASV}/{@code EPSV} reply, which cannot be pinned, so {@link + * FileFetcher} rejects {@code ftp} on the SSRF-blocking path instead. + * + * <p>Only responses framed by {@code Content-Length} or chunked transfer-encoding are accepted; a + * connection-close-delimited body is rejected because a premature close cannot be told apart from a + * complete one. Combined with streaming to a sibling temporary file that is atomically moved into + * place only on success, this means a connection that drops mid-transfer never leaves a truncated + * destination behind. Response and header sizes are bounded to prevent a malicious pinned host from + * exhausting disk or memory. + */ +final class RemoteFileDownloader { + + private static final int DEFAULT_HTTP_PORT = 80; + private static final int DEFAULT_HTTPS_PORT = 443; + private static final int MAX_HEADER_BYTES = 64 * 1024; + private static final long MAX_BODY_BYTES = 2L * 1024 * 1024 * 1024; + private static final int COPY_BUFFER_BYTES = 8192; + + private RemoteFileDownloader() {} + + /** + * Downloads {@code uri} into {@code destFile}, connecting only to {@code pinnedAddress}. + * + * @param uri the source URI; must use the {@code http} or {@code https} scheme + * @param pinnedAddress the validated address to connect to + * @param destFile the local destination file + * @param timeoutMs the connect/read timeout in milliseconds + * @throws IOException if the file cannot be fetched or the response is not successful + */ + static void download(URI uri, InetAddress pinnedAddress, File destFile, int timeoutMs) + throws IOException { + String scheme = Optional.ofNullable(uri.getScheme()).orElse("").toLowerCase(Locale.ROOT); + if (!scheme.equals("http") && !scheme.equals("https")) { + throw new IllegalArgumentException( + String.format("Pinned download does not support scheme '%s'", scheme)); + } + + Path destPath = destFile.toPath().toAbsolutePath(); + Path tempPath = + Files.createTempFile(destPath.getParent(), destPath.getFileName() + ".", ".tmp"); Review Comment: `Files.createTempFile(dir, prefix, suffix)` requires `prefix` to be at least 3 characters. Here the prefix is derived from `destPath.getFileName() + "."`, which will throw `IllegalArgumentException` for 1-character destination filenames (e.g. `destFile = new File("a")`). Please ensure the computed prefix is always >= 3 chars to avoid unexpected runtime failures. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
