weiqingy commented on code in PR #1005:
URL: https://github.com/apache/flink-agents/pull/1005#discussion_r3891323377
##########
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:
Two URLs where Python and Java disagree, in opposite directions. Measured on
CPython 3.11.14, against a copy of `SkillUrlUtils` on temurin-11:
| URL | Python | Java |
|---|---|---|
| `https://<U+212A KELVIN SIGN>.com/x.zip` | accepted | rejected (`must
include a valid host and, when present, a valid port`) |
| `https://[fe80::1%eth0]/x.zip` | rejected (`Invalid skill URL`) | accepted
|
Row 1: `urllib` lower-cases `parsed.hostname` at `:118`, before the
`isascii()` check on this line. U+212A is the only non-ASCII character that
lower-cases to an ASCII letter or digit, `k` (I walked U+0080 to U+10FFFF). So
the check never sees a non-ASCII host, the log shows the Kelvin character, and
`urllib` connects to `k.com`. It also gets past the rule
`test_skills.py:131-137` sets. Adding `not parsed.netloc.isascii()` at
`:130-134` catches it, with the skills suites (166 tests) still green.
Row 2: `_INVALID_PERCENT_ESCAPE` (`:71`, used at `:105`) matches `%et`, so
Python rejects the zone id before `_is_valid_hostname` runs, while Java's
`URI.create` allows a raw `%` inside an IPv6 literal. Python is the stricter
side, so nothing fails open, but the tests bake the split in:
`test_skills.py:170` calls this URL malformed, and the Java twin at
`SkillsResourceTest.java:134` leaves it out.
`AGENTS.md` asks for Java and Python to line up. Which way would you want
each of these to go?
##########
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:
The name says validation rejects this URL, but it does not. On temurin-11,
`URI.create` (`SkillUrlUtils.java:47`) and `parseServerAuthority()` (`:58`)
both accept it with `host=[fe80::1%eth0]`, and the user-info and port checks
pass. The `IOException` you catch is an `UnknownHostException` from DNS. So
this goes green because the network failed, and any unrelated network error
would do the same.
It also depends on the machine. macOS has no `eth0`, so the lookup fails
right away. Where one exists, the network decides instead, against
`REQUEST_TIMEOUT_MS = 90_000` (`URLSkillRepository.java:46`). I have not tried
a Linux runner, so treat that half as a guess.
Would asserting the message, on a URL the validator rejects by itself, be
better here?
##########
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:
nit: the port bound at `SkillUrlUtils.java:72` is only half covered. I
replayed this suite against the real class and changed the bound one line at a
time. Deleting the check turns it red, and `> 65536` turns it red, but `>=
65535` stays green, because nothing here accepts port 65535. Adding
`"https://example.com:65535/x.zip"` to this list would cover it. Worth one more
entry?
##########
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:
`redact` parses the URL again with `URI.create` at `:91`. That parse fails
for exactly the URLs `validate` just rejected at `:47-50`, so the caller gets
this constant instead of a URL. Over a 3455-URL fuzz run, 2902 of the
rejections came out that way.
| input | Java | Python |
|---|---|---|
| `https://u:[email protected]/a b.zip?token=SECRET` | `Invalid skill URL:
<redacted>` | `Invalid skill URL: https://example.com/a b.zip` |
| `https://u:[email protected]/%zz?token=SECRET` | `Invalid skill URL:
<redacted>` | `Invalid skill URL: https://example.com/%zz` |
Python does the same job with `urlsplit` and `urlunsplit`
(`skills.py:80-89`): no raise on these inputs, user info and query still
stripped, and still `<redacted>` when it cannot parse at all. Scanning both
sides over that corpus for `SECRET`, `?`, `#` and `@` gave zero hits, so the
extra detail leaks nothing.
It bites in `fromUrl(String…)`, which validates inside a stream
(`Skills.java:99-108`). Several URLs with one typo, and the message names none
of them.
I can read the constant as a deliberate fail-closed choice, and
`SkillsResourceTest.java:138` locks it in. Is that the intent, or is the Python
form worth matching here?
--
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]
