weiqingy commented on code in PR #1005:
URL: https://github.com/apache/flink-agents/pull/1005#discussion_r3781343774
##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java:
##########
@@ -254,14 +254,37 @@ 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");
Path tmpZip = Files.createTempFile(TEMP_DIR_PREFIX, ".zip");
- try (InputStream in = conn.getInputStream()) {
- Files.copy(in, tmpZip, StandardCopyOption.REPLACE_EXISTING);
+ try {
+ int responseCode = conn.getResponseCode();
+ if (responseCode >= 300 && responseCode < 400) {
Review Comment:
This catches a redirect that changes protocol, but a same-protocol redirect
is followed silently and the final URL is never checked. Java only looks at the
protocol of the configured URL (`:267-272`), never at `conn.getURL()`; Python
compares just the scheme of `resp.geturl()` (`_materialize.py:189-196`), not
the host.
So for an unpinned HTTPS source, an open redirect at the configured host can
pull the archive from somewhere else, and nothing records it — `SkillOrigin`
(`SkillSourceRegistry.java:56`) still reports the URL that was configured.
Since the archive becomes agent instructions and possibly scripts, where the
bytes actually came from seems worth surfacing.
Pinning a `sha256` does catch this. Is that the intended answer, or would
logging the final URL when it differs be worth adding?
One related note: the protection against a scheme change here is real but
implicit, coming from the JDK declining cross-protocol redirects rather than
from this code. A later move to `java.net.http.HttpClient` would reverse it,
since its default policy follows those. Might be worth a comment naming that.
##########
python/flink_agents/runtime/skill/repository/_materialize.py:
##########
@@ -159,8 +167,31 @@ def download_to_tempfile(url: str, timeout: int = 90) ->
Path:
tmp_path = Path(tmp_path_str)
try:
with urlopen(req, timeout=timeout) as resp, tmp_path.open("wb") as out:
Review Comment:
`urlopen` here uses the default opener, and its redirect handler allows
`http`, `https` and `ftp` targets. So when an HTTPS source redirects to
`http://…`, Python performs the plaintext GET and only rejects it afterwards at
`:191-193`. An `ftp://` location gets dialled too, since `FTPHandler` is
installed by default.
No archive bytes are written, so the content is never trusted. But the
request itself, URL and headers included, does travel over the downgraded
connection. Java never gets that far — `HttpURLConnection` refuses the
cross-protocol redirect, so the second request is never made.
The test covering this (`test_materialize.py:164`) swaps `urlopen` for a
stub that reports a different `geturl()`, so it checks the string comparison
rather than urllib's redirect handling. This case would slip past it.
What do you think about declining the redirect in a custom opener, so the
request is never issued? A real `HTTPServer` returning a 302 to an `http://`
location, the way `SkillMaterializerTest.java:153` does on the Java side, would
cover it end to end.
##########
docs/content/docs/development/yaml.md:
##########
@@ -208,13 +208,14 @@ agents:
key: tenant_id
```
-**Skills** — bundles of agent skill assets loaded from one or more sources. At
least one of `paths` / `urls` / `classpath` / `package` must be non-empty;
multiple sources can coexist.
+**Skills** — bundles of agent skill assets loaded from one or more sources. At
least one of `paths` / `urls` / `url_sources` / `classpath` / `package` must be
non-empty; multiple sources can coexist.
| Field | Required | Description |
|-------|----------|-------------|
| `name` | yes | Skills resource name. |
| `paths` | one-of | `local` scheme: list of directories or `.zip` files. |
-| `urls` | one-of | `url` scheme: list of `http(s)` URLs pointing to `.zip`
archives. |
+| `urls` | one-of | `url` scheme: list of HTTPS URLs pointing to `.zip`
archives. |
Review Comment:
`urls: [http://…]` worked before this PR and now fails, and neither doc says
what to switch to. The answer also differs by surface: in code there's
`from_url_unsafe` / `fromUrlUnsafe` (`skills.md:145`), but `urls` has no opt-in
at all — the entry has to move into `url_sources` with `allow_insecure_http:
true`. That's a different shape rather than a flag, so it's the harder one to
guess from the error alone.
Something like this after the row, if it helps: "`urls` entries must be
HTTPS. To keep an existing plain-HTTP source working, move it to `url_sources`
with `allow_insecure_http: true`."
Does that feel like doc material, or more of a release-note thing?
##########
runtime/src/test/java/org/apache/flink/agents/runtime/skill/URLSkillRepositoryTest.java:
##########
@@ -97,19 +100,57 @@ 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 {
Review Comment:
The name says `BeforeExtraction`, but the body only checks the exception and
its message. If the digest check moved to after `extractZipSafely`, the archive
would extract, the same `IllegalArgumentException` with the same `"SHA-256
mismatch"` text would still be thrown, and this test would stay green. Python's
`test_sha256_mismatch_rejected` (`test_url_repository.py:103`) has the same
gap, just without the name promising more.
Checking before extraction is the property #1003 actually asks for, so a
test that turns red when it changes seems worth having. One option: serve an
archive that extraction itself would reject — the zip-slip fixture at
`SkillMaterializerTest.java:72` — and assert the failure is the digest error
rather than `"Unsafe zip entry"`. Flip the order and you would get the zip-slip
error instead.
Any reason that wouldn't work here?
##########
api/src/main/java/org/apache/flink/agents/api/yaml/YamlLoader.java:
##########
@@ -173,8 +174,19 @@ public static Skills buildSkills(SkillsSpec spec) {
for (String p : spec.getPaths()) {
sources.add(new SkillSourceSpec("local", Map.of("path", p)));
}
- for (String u : spec.getUrls()) {
- sources.add(new SkillSourceSpec("url", Map.of("url", u)));
+ for (String url : spec.getUrls()) {
+ sources.add(new SkillSourceSpec("url", Map.of("url", url)));
Review Comment:
This builds the source without running the URL check, and
`loader.py:208-210` does the same. So `urls: [http://x/s.zip]` parses cleanly
and only fails later on the TaskManager, while `Skills.fromUrl("http://…";)`
fails right away. A malformed `sha256` under `url_sources` behaves the same way.
Both languages do this identically, so it isn't a parity issue. It's that
the same mistake surfaces at very different moments depending on how you
configure it, and the YAML user finds out last. Nothing on either side tests
that a YAML `http://` entry is rejected, either.
Was that deliberate, keeping the loaders out of transport policy, or did it
just work out that way?
--
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]
