rob-9 commented on code in PR #1005:
URL: https://github.com/apache/flink-agents/pull/1005#discussion_r3892221472
##########
api/src/test/java/org/apache/flink/agents/api/skills/SkillsResourceTest.java:
##########
@@ -48,6 +53,174 @@ void fromUrlEmitsUrlScheme() {
skills.getSources());
}
+ @Test
+ void fromUrlAcceptsSharedValidHostSyntax() {
+ for (String url :
+ List.of(
+ "https://localhost/x.zip";,
+ "https://127.0.0.1/x.zip";,
+ "https://[::1]/x.zip";,
+ "https://[fe80::1%25eth0]/x.zip";,
+ "https://example.com./x.zip";,
+ "https://example.com:/x.zip";,
+ "https://999/x.zip";,
+ "https://1bar/x.zip";,
+ "https://999./x.zip";)) {
Review Comment:
added `65535` as a valid port boundary test in both Java and Python.
##########
api/src/main/java/org/apache/flink/agents/api/skills/SkillUrlUtils.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.URI;
+import java.net.URISyntaxException;
+import java.util.Locale;
+
+/**
+ * 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 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");
+ }
+ 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>";
+ }
+ String authority = uri.getRawAuthority();
+ int userInfoEnd = authority.lastIndexOf('@');
+ if (userInfoEnd >= 0) {
+ authority = authority.substring(userInfoEnd + 1);
+ }
+ if (authority.isEmpty()) {
+ return "<redacted>";
+ }
+ return uri.getScheme() + "://" + authority + uri.getRawPath();
+ } catch (IllegalArgumentException ignored) {
+ return "<redacted>";
Review Comment:
Matched Python. Preserves safe host/path context while stripping secrets and
retaining `<redacted>` for unsafe inputs.
##########
python/flink_agents/api/skills.py:
##########
@@ -57,13 +57,117 @@ def packaged_skills() -> Skills:
from __future__ import annotations
+import re
+from ipaddress import AddressValueError, IPv6Address
from typing import Dict, List, Tuple
+from urllib.parse import urlparse, urlsplit, urlunsplit
from pydantic import BaseModel, ConfigDict, Field, field_validator
from typing_extensions import override
from flink_agents.api.resource import ResourceType, SerializableResource
+_INVALID_URI_CHARACTER = re.compile(r'[\x00-\x20\x7f<>"{}|\\^`]')
+_INVALID_PERCENT_ESCAPE = re.compile(r"%(?![0-9a-fA-F]{2})")
+_HOST_LABEL = re.compile(r"[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?")
+
+
+def redact_skill_url(url: str) -> str:
+ """Return a skill URL without user info, query parameters, or a fragment.
+
+ Internal contract shared with the runtime; not a stable public API.
+ """
+ try:
+ parts = urlsplit(url)
+ if not parts.scheme or not parts.netloc:
+ return "<redacted>"
+ netloc = parts.netloc.rsplit("@", 1)[-1]
+ if not netloc:
+ return "<redacted>"
+ return urlunsplit((parts.scheme, netloc, parts.path, "", ""))
+ except ValueError:
+ return "<redacted>"
+
+
+def validate_skill_url(url: str, *, allow_insecure_http: bool) -> str:
+ """Validate a skill URL using the contract shared with the Java API.
+
+ Internal contract shared with the runtime; not a stable public API.
+ """
+ if not isinstance(url, str):
+ msg = "skill URL must be a string"
+ raise TypeError(msg)
+ try:
+ parsed = urlparse(url)
+ except ValueError:
+ msg = f"Invalid skill URL: {redact_skill_url(url)}"
+ raise ValueError(msg) from None
+ if _INVALID_URI_CHARACTER.search(url) or
_INVALID_PERCENT_ESCAPE.search(url):
+ msg = f"Invalid skill URL: {redact_skill_url(url)}"
+ raise ValueError(msg)
+ # Java's URI rejects raw brackets in the path (but not in the query or
+ # fragment); encoded %5B/%5D and IPv6 authority brackets stay valid.
+ if any(c in f"{parsed.path};{parsed.params}" for c in "[]"):
+ msg = f"Invalid skill URL: {redact_skill_url(url)}"
+ raise ValueError(msg)
+ scheme = parsed.scheme.lower()
+ if scheme not in {"http", "https"}:
+ msg = f"Only HTTP(S) skill URLs are supported: {redact_skill_url(url)}"
+ raise ValueError(msg)
+ try:
+ hostname = parsed.hostname
+ _ = parsed.port
+ except ValueError:
+ msg = (
+ "Skill URL must include a valid host and, when present, a valid
port: "
+ f"{redact_skill_url(url)}"
+ )
+ raise ValueError(msg) from None
+ if parsed.username is not None:
+ msg = f"Skill URL must not include user info: {redact_skill_url(url)}"
+ raise ValueError(msg)
+ bracketed_host = parsed.netloc.rsplit("@", 1)[-1].startswith("[")
+ if (
+ not hostname
+ or (bracketed_host and ":" not in hostname)
+ or not _is_valid_hostname(hostname)
+ ):
+ msg = f"Skill URL must include a valid host: {redact_skill_url(url)}"
+ raise ValueError(msg)
+ if scheme == "http" and not allow_insecure_http:
+ msg = (
+ "Plain HTTP skill URLs are disabled by default; use HTTPS or "
+ "explicitly allow insecure HTTP for this source: "
+ f"{redact_skill_url(url)}"
+ )
+ raise ValueError(msg)
+ return scheme
+
+
+def _is_valid_hostname(hostname: str) -> bool:
+ """Match the host syntax accepted by Java URI.parseServerAuthority()."""
+ if ":" in hostname:
+ try:
+ IPv6Address(hostname)
+ except AddressValueError:
+ return False
+ return True
+ if not hostname.isascii():
Review Comment:
Aligned both languages to reject raw Unicode authorities and raw `%` IPv6
zones while accepting encoded `%25` zones.
##########
runtime/src/test/java/org/apache/flink/agents/runtime/skill/URLSkillRepositoryTest.java:
##########
@@ -97,30 +106,166 @@ void loadFromUrl(@TempDir Path tempDir) throws
IOException {
}
}
+ @Test
+ void plainHttpRejectedByDefault() {
+ IllegalArgumentException ex =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new
URLSkillRepository("http://example.com/skills.zip";));
+ assertTrue(ex.getMessage().contains("disabled by default"));
+ }
+
+ @Test
+ void sha256MismatchRejectedBeforeExtraction(@TempDir Path tempDir) throws
IOException {
+ Path zip = tempDir.resolve("skills.zip");
+ try (ZipOutputStream zos = new
ZipOutputStream(Files.newOutputStream(zip))) {
+ zos.putNextEntry(new ZipEntry("../evil.txt"));
+ zos.write("pwn".getBytes(StandardCharsets.UTF_8));
+ zos.closeEntry();
+ }
+ HttpServer server = startZipServer(Files.readAllBytes(zip), 200);
+ try {
+ int port = server.getAddress().getPort();
+ String url = "http://127.0.0.1:"; + port +
"/skills.zip?token=top-secret#fragment";
+ IllegalArgumentException ex =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new URLSkillRepository(url, "0".repeat(64),
true));
+ assertTrue(ex.getMessage().contains("SHA-256 mismatch"));
+ assertTrue(ex.getMessage().contains("http://127.0.0.1:"; + port +
"/skills.zip"));
+ assertFalse(ex.getMessage().contains("top-secret"));
+ } finally {
+ server.stop(0);
+ }
+ }
+
+ @Test
+ void invalidHostAndPortAreRejectedBeforeDownload() {
+ IllegalArgumentException malformedPort =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ new URLSkillRepository(
+
"https://example.com:bad/skills.zip?token=top-secret";));
+ assertNull(malformedPort.getCause());
+
assertTrue(malformedPort.getMessage().contains("https://example.com:bad/skills.zip";));
+ assertFalse(malformedPort.getMessage().contains("top-secret"));
+
+ for (String url :
+ List.of("https://:443/skills.zip";,
"https://example.com:65536/skills.zip";)) {
+ assertThrows(IllegalArgumentException.class, () -> new
URLSkillRepository(url), url);
+ }
+ }
+
+ @Test
+ void invalidHostnameSyntaxIsRejectedBeforeDownload() {
+ for (String url :
+ List.of(
+ "https://exa_mple.com/skills.zip";,
+ "https://tést.com/skills.zip";,
+ "https://%65xample.com/skills.zip";,
+ "https://-example.com/skills.zip";,
+ "https://example-.com/skills.zip";,
+ "https://.example.com/skills.zip";,
+ "https://example..com/skills.zip";,
+ "https://a../skills.zip";,
+ "https://../skills.zip";,
+ "https://999.999.999.999/skills.zip";,
+ "https://127.1/skills.zip";,
+ "https://1.2.3/skills.zip";,
+ "https://foo.123/skills.zip";,
+ "https://foo.1bar/skills.zip";,
+ "https://1.2.3.4.5/skills.zip";,
+ "https://1.2.3./skills.zip";,
+ "https://1.2.3.4./skills.zip";,
+ "https://[v1.foo]/skills.zip";)) {
+ assertThrows(IllegalArgumentException.class, () -> new
URLSkillRepository(url), url);
+ }
+ }
+
+ @Test
+ void rawPercentEscapeIsRejectedByDownloader() {
Review Comment:
Fixed by rejecting this URL in the shared Java validator before networking
and asserting its exact exception message.
--
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]
