wenjin272 commented on code in PR #1005: URL: https://github.com/apache/flink-agents/pull/1005#discussion_r3912223371
########## api/src/main/java/org/apache/flink/agents/api/skills/SkillUrlUtils.java: ########## @@ -0,0 +1,142 @@ +/* + * 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.flink.agents.api.skills; + +import org.apache.flink.annotation.Internal; + +import java.net.MalformedURLException; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.URL; +import java.util.Locale; +import java.util.regex.Pattern; + +/** + * Shared validation and redaction helpers for URL-backed skill sources. Internal contract shared + * with the runtime module; not a stable public API. + */ +@Internal +public final class SkillUrlUtils { + + private static final String REDACTED = "<redacted>"; + private static final Pattern INVALID_PERCENT_ESCAPE = Pattern.compile("%(?![0-9a-fA-F]{2})"); + + private SkillUrlUtils() {} + + /** + * Validate {@code url} and return its lowercase {@code http} or {@code https} scheme. + * + * @throws IllegalArgumentException if the URL is invalid or violates the transport policy. + */ + public static String validate(String url, boolean allowInsecureHttp) { + if (url == null) { + throw new IllegalArgumentException("skill URL must not be null"); + } + if (INVALID_PERCENT_ESCAPE.matcher(url).find()) { + throw new IllegalArgumentException("Invalid skill URL: " + redact(url)); + } + URI uri; + try { + uri = URI.create(url); + } catch (IllegalArgumentException ignored) { + throw new IllegalArgumentException("Invalid skill URL: " + redact(url)); + } + String scheme = uri.getScheme(); + scheme = scheme == null ? "" : scheme.toLowerCase(Locale.ROOT); + if (!(scheme.equals("http") || scheme.equals("https"))) { + throw new IllegalArgumentException( + "Only HTTP(S) skill URLs are supported: " + redact(url)); + } + try { + uri = uri.parseServerAuthority(); + } catch (URISyntaxException ignored) { + throw new IllegalArgumentException( + "Skill URL must include a valid host and, when present, a valid port: " + + redact(url)); + } + if (uri.getRawUserInfo() != null) { + throw new IllegalArgumentException( + "Skill URL must not include user info: " + redact(url)); + } + if (uri.getHost() == null || uri.getHost().isEmpty()) { + throw new IllegalArgumentException( + "Skill URL must include a valid host: " + redact(url)); + } + if (uri.getPort() > 65535) { + throw new IllegalArgumentException( + "Skill URL port must be between 0 and 65535: " + redact(url)); + } + if (scheme.equals("http") && !allowInsecureHttp) { + throw new IllegalArgumentException( + "Plain HTTP skill URLs are disabled by default; use HTTPS or explicitly allow" + + " insecure HTTP for this source: " + + redact(url)); + } + return scheme; + } + + /** Return {@code url} without user info, query parameters, or a fragment. */ + public static String redact(String url) { + if (url == null) { + return REDACTED; + } + try { + URI uri = URI.create(url); + if (uri.getScheme() == null || uri.getRawAuthority() == null) { + return REDACTED; + } + return redactParts(uri.getScheme(), uri.getRawAuthority(), uri.getRawPath()); + } catch (IllegalArgumentException ignored) { + try { + URL parsed = new URL(url); + String authority = parsed.getAuthority(); + String path = parsed.getPath(); + if (authority == null + || containsUnsafeLogCharacter(authority) + || containsUnsafeLogCharacter(path)) { + return REDACTED; + } + return redactParts(parsed.getProtocol(), authority, path); + } catch (MalformedURLException | IllegalArgumentException malformed) { + return REDACTED; + } + } + } + + private static String redactParts(String scheme, String authority, String path) { + int userInfoEnd = authority.lastIndexOf('@'); + if (userInfoEnd >= 0) { + authority = authority.substring(userInfoEnd + 1); + } + if (authority.isEmpty()) { + return REDACTED; + } + return scheme + "://" + authority + (path == null ? "" : path); Review Comment: One redaction edge case remains: `https://user:supersecret/x.zip?token=TOPSECRET` fails port validation, but the error still contains `https://user:supersecret/x.zip`. Since there is no `@`, both redactors preserve the entire malformed authority. Could invalid-port authorities return `<redacted>` or omit the raw port, with coverage in both Java and Python? ########## runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java: ########## @@ -253,23 +257,118 @@ private static void copyJarEntries(URL jarUrl, String prefix, Path extractDir) * @throws IOException on connect / read failures or HTTP error responses. */ public static Path downloadToTempFile(String url, int timeoutMs) throws IOException { - URL u = new URL(url); - HttpURLConnection conn = (HttpURLConnection) u.openConnection(); - conn.setConnectTimeout(timeoutMs); - conn.setReadTimeout(timeoutMs); - conn.setRequestMethod("GET"); + return downloadToTempFile(url, timeoutMs, false); + } + + /** + * Download {@code url}, optionally permitting plain HTTP transport. + * + * @throws IOException on connect / read failures or HTTP error responses. + */ + public static Path downloadToTempFile(String url, int timeoutMs, boolean allowInsecureHttp) + throws IOException { + URL u; + try { + u = new URL(url); + } catch (MalformedURLException ignored) { + throw new IOException("Invalid skill URL: " + SkillUrlUtils.redact(url)); + } + String initialProtocol = requireValidDownloadUrl(u, allowInsecureHttp); + boolean followRedirects = HttpURLConnection.getFollowRedirects(); Path tmpZip = Files.createTempFile(TEMP_DIR_PREFIX, ".zip"); - try (InputStream in = conn.getInputStream()) { - Files.copy(in, tmpZip, StandardCopyOption.REPLACE_EXISTING); + HttpURLConnection conn = null; + try { + URL effectiveUrl = u; + int redirects = 0; + while (true) { + conn = (HttpURLConnection) effectiveUrl.openConnection(); Review Comment: Scoped IPv6 URLs accepted by the validator do not work with the Java downloader. On Temurin 11, `%25lo0` passes validation but `HttpURLConnection` treats the scope as `25lo0` and raises `UnknownHostException`; raw `%lo0` works with `HttpURLConnection` but is rejected by the validator. Could we either normalize the zone delimiter before downloading or reject scoped IPv6 URLs in both languages, with an end-to-end test? -- 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]
