rob-9 commented on code in PR #1005:
URL: https://github.com/apache/flink-agents/pull/1005#discussion_r3837978246
##########
api/src/main/java/org/apache/flink/agents/api/skills/Skills.java:
##########
@@ -92,17 +94,94 @@ public static Skills fromLocalDir(String... paths) {
}
/**
- * Create a {@link Skills} resource from one or more http(s) URLs.
+ * Create a {@link Skills} resource from one or more HTTPS URLs.
*
* <p>Each URL must point to a {@code .zip} whose top level is the baseDir.
*/
public static Skills fromUrl(String... urls) {
return new Skills(
Arrays.stream(urls)
- .map(u -> new SkillSourceSpec("url", Map.of("url", u)))
+ .map(
+ u -> {
+ requireUrl(u, false);
+ return new SkillSourceSpec("url",
Map.of("url", u));
+ })
.collect(Collectors.toList()));
}
+ /**
+ * Create a {@link Skills} resource from an HTTPS URL pinned to a SHA-256
digest.
+ *
+ * <p>The digest is verified against the downloaded archive before
extraction.
+ */
+ public static Skills fromUrlWithSha256(String url, String sha256) {
+ return urlSource(url, sha256, false);
+ }
+
+ /**
+ * Create a {@link Skills} resource that explicitly permits plain HTTP
transport.
+ *
+ * <p>This compatibility escape hatch should be used only on trusted
networks. Prefer {@link
+ * #fromUrl(String...)} with HTTPS.
+ */
+ public static Skills fromUrlUnsafe(String... urls) {
+ return new Skills(
+ Arrays.stream(urls)
+ .map(
+ u -> {
+ requireUrl(u, true);
+ return new SkillSourceSpec(
+ "url", Map.of("url", u,
"allow_insecure_http", "true"));
+ })
+ .collect(Collectors.toList()));
+ }
+
+ /**
+ * Create a digest-pinned {@link Skills} resource that explicitly permits
plain HTTP transport.
+ */
+ public static Skills fromUrlUnsafeWithSha256(String url, String sha256) {
+ return urlSource(url, sha256, true);
+ }
+
+ private static Skills urlSource(String url, String sha256, boolean
allowInsecureHttp) {
+ requireUrl(url, allowInsecureHttp);
+ if (sha256 == null || !sha256.matches("[0-9a-fA-F]{64}")) {
Review Comment:
done!
##########
python/flink_agents/api/skills.py:
##########
@@ -107,22 +109,96 @@ def from_local_dir(cls, *paths: str) -> Skills:
a zip, its top-level entries are the skill subdirectories.
"""
return cls(
- sources=[
- SkillSourceSpec(scheme="local", params={"path": p}) for p in
paths
- ]
+ sources=[SkillSourceSpec(scheme="local", params={"path": p}) for p
in paths]
)
@classmethod
def from_url(cls, *urls: str) -> Skills:
- """Create a Skills resource from one or more http(s) URLs.
+ """Create a Skills resource from one or more HTTPS URLs.
Each URL must point to a ``.zip`` whose top level is the baseDir
(i.e. skill subdirectories sit at the top of the zip).
"""
+ for url in urls:
+ cls._require_url(url, allow_insecure_http=False)
return cls(
sources=[SkillSourceSpec(scheme="url", params={"url": u}) for u in
urls]
)
+ @classmethod
+ def from_url_with_sha256(cls, url: str, sha256: str) -> Skills:
+ """Create an HTTPS URL source pinned to a SHA-256 archive digest."""
+ cls._require_url(url, allow_insecure_http=False)
+ cls._require_sha256(sha256)
+ return cls(
+ sources=[
+ SkillSourceSpec(scheme="url", params={"url": url, "sha256":
sha256})
+ ]
+ )
+
+ @classmethod
+ def from_url_unsafe(cls, *urls: str) -> Skills:
+ """Create URL sources that explicitly permit plain HTTP transport.
+
+ This compatibility escape hatch should be used only on trusted
networks.
+ Prefer :meth:`from_url` with HTTPS.
+ """
+ for url in urls:
+ cls._require_url(url, allow_insecure_http=True)
+ return cls(
+ sources=[
+ SkillSourceSpec(
+ scheme="url",
+ params={"url": url, "allow_insecure_http": "true"},
+ )
+ for url in urls
+ ]
+ )
+
+ @classmethod
+ def from_url_unsafe_with_sha256(cls, url: str, sha256: str) -> Skills:
+ """Create a digest-pinned source that explicitly permits plain HTTP."""
+ cls._require_url(url, allow_insecure_http=True)
+ cls._require_sha256(sha256)
+ return cls(
+ sources=[
+ SkillSourceSpec(
+ scheme="url",
+ params={
+ "url": url,
+ "sha256": sha256,
+ "allow_insecure_http": "true",
+ },
+ )
+ ]
+ )
+
+ @staticmethod
+ def _require_url(url: str, *, allow_insecure_http: bool) -> None:
+ if not isinstance(url, str):
+ msg = "skill URL must be a string"
+ raise TypeError(msg)
+ parsed = urlparse(url)
Review Comment:
aligned Python with Java so malformed URLs fail immediately. also added
coverage for whitespace and invalid percent escapes.
##########
api/src/test/java/org/apache/flink/agents/api/yaml/YamlLoaderBuildAgentsTest.java:
##########
@@ -133,6 +133,34 @@ void skillsPerAgentAndShared() {
new SkillSourceSpec("local", Map.of("path",
"./more")));
}
+ @Test
+ void rejectsPlainHttpSkillUrlDuringLoading(@TempDir Path tmp) throws
Exception {
Review Comment:
added focused Java and Python tests for plain HTTP in `url_sources` without
the opt-in.
##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java:
##########
@@ -254,14 +255,47 @@ 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 {
+ 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 = new URL(url);
+ String initialProtocol = u.getProtocol();
+ if (!("https".equalsIgnoreCase(initialProtocol)
+ || (allowInsecureHttp &&
"http".equalsIgnoreCase(initialProtocol)))) {
+ throw new IOException("Skill URL uses a disallowed transport: " +
url);
+ }
HttpURLConnection conn = (HttpURLConnection) u.openConnection();
conn.setConnectTimeout(timeoutMs);
conn.setReadTimeout(timeoutMs);
conn.setRequestMethod("GET");
+ // HttpURLConnection follows same-protocol redirects but leaves
cross-protocol redirects
+ // unfollowed. Any future HTTP client must preserve that restriction.
+ conn.setInstanceFollowRedirects(true);
Review Comment:
good catch. this wasn't intentional, removed the override and added a test
to confirm.
--
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]
