[api][runtime][java][python] Require HTTPS and add digest pinning for URL skill sources - #1005
[api][runtime][java][python] Require HTTPS and add digest pinning for URL skill sources#1005rob-9 wants to merge 29 commits into
Conversation
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for taking this on. A few questions inline.
| @@ -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: | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
agreed here - added a handler that rejects protocol changes before the target request is sent, including redirects to HTTP or FTP. also replaced the stubbed test with a real HTTPServer test.
| Files.copy(in, tmpZip, StandardCopyOption.REPLACE_EXISTING); | ||
| try { | ||
| int responseCode = conn.getResponseCode(); | ||
| if (responseCode >= 300 && responseCode < 400) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
pinning remains the integrity control, but surfacing redirects is useful too. added URL logging in both runtimes and documented Java's cross-protocol behavior,
| } | ||
|
|
||
| @Test | ||
| void sha256MismatchRejectedBeforeExtraction(@TempDir Path tempDir) throws IOException { |
There was a problem hiding this 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?
There was a problem hiding this comment.
no reason, that works well. updated tests so they now fail if extraction happens first.
| 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))); |
There was a problem hiding this 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?
There was a problem hiding this comment.
it wasn't deliberate. both YAML loaders now use the validated URL factories, so plain HTTP and malformed digests fail during loading while explicit HTTP opt-in remains supported.
| | `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. | |
There was a problem hiding this 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?
There was a problem hiding this comment.
probably both. the migration path belongs in the YAML docs, while the compatibility change belongs in release notes. I added the YAML migration example here.
| } | ||
|
|
||
| @Test | ||
| void pinnedUrlRoundTripsThroughJackson() throws Exception { |
There was a problem hiding this comment.
nit: this round trip stops at sha256, and test_skills.py::test_serialize_roundtrip does the same, so allow_insecure_http isn't covered on either side.
I compared what the two languages emit and they match exactly — both write the flag as the string "true" and omit it otherwise, and both read it back the same way — so there's no bug here today. The only gap is that nothing would catch it if they drifted. Raising it because #1003 mentions alignment "including plan serialization".
Would adding the flag to this test and its Python counterpart be enough?
There was a problem hiding this comment.
agreed. extended both round-trip tests to cover allow_insecure_http and verify it remains the string "true" thru serialization and deserialization.
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. While going back through the updated code I spotted a few smaller things, all inline.
| 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); |
There was a problem hiding this comment.
The comment is accurate, the JDK really does decline a scheme-changing redirect: followRedirect() compares the two protocols and bails before taking it.
The call itself isn't quite a no-op though. instanceFollowRedirects is initialised from the static followRedirects, so if anything in the JVM has called HttpURLConnection.setFollowRedirects(false), which some deployments do as a hardening step, this line overrides that and re-enables redirect following for skill downloads specifically. I checked on temurin-11: after setFollowRedirects(false) the instance default reads back false, and this line flips it to true. Without it the 3xx would surface and :284 would fail closed with a clear message.
Was pinning it to true deliberate, or is the comment doing the real work here?
There was a problem hiding this comment.
good catch. this wasn't intentional, removed the override and added a test to confirm.
| } | ||
|
|
||
| @Test | ||
| void rejectsPlainHttpSkillUrlDuringLoading(@TempDir Path tmp) throws Exception { |
There was a problem hiding this comment.
nit: this covers the urls: list, and the new four-way test in YamlLoaderBuildersTest covers the url_sources shapes that succeed. The case #1003 is actually about, url_sources: [{url: http://…}] with allow_insecure_http absent, isn't asserted on either side, and the issue asks for "focused Java and Python tests for HTTP rejection or opt-in behavior".
The positive cases do pin the factory choice indirectly, so this is coverage rather than a hole. Worth one line per language?
There was a problem hiding this comment.
added focused Java and Python tests for plain HTTP in url_sources without the opt-in.
| if not isinstance(url, str): | ||
| msg = "skill URL must be a string" | ||
| raise TypeError(msg) | ||
| parsed = urlparse(url) |
There was a problem hiding this comment.
urlparse doesn't reject a malformed URL, so https://exa mple.com/x.zip is accepted here and only fails later at download time, while Skills.java:163-168 runs the same string through URI.create and rethrows it as Invalid skill URL: at call time.
Before these commits neither YAML loader reached either validator. Now both do, so the split is exercised at load time on both runtimes rather than only through the code API.
Is the later failure good enough here, or would you rather Python rejected it at the same point Java does?
There was a problem hiding this comment.
aligned Python with Java so malformed URLs fail immediately. also added coverage for whitespace and invalid percent escapes.
|
|
||
| private static Skills urlSource(String url, String sha256, boolean allowInsecureHttp) { | ||
| requireUrl(url, allowInsecureHttp); | ||
| if (sha256 == null || !sha256.matches("[0-9a-fA-F]{64}")) { |
There was a problem hiding this comment.
nit: this accepts either case and URLSkillRepository.java:90 lowercases before comparing, which matches what skills.md:144 promises ("lowercase or uppercase SHA-256 digest"). Every digest literal in the suite is lowercase though ("a".repeat(64) and friends), so nothing walks the uppercase path.
Would flipping one existing digest literal to uppercase cover it?
|
|
||
| @pytest.mark.parametrize( | ||
| "url", | ||
| ["https://exa mple.com/x.zip", "https://example.com/%invalid"], |
There was a problem hiding this comment.
nit: both of these cases hit the two new regexes at skills.py:189, so the except ValueError just above at skills.py:186-188 still has nothing exercising it. That branch is the only thing handling a URL urlparse itself refuses: urlparse("https://[::1/x.zip") raises Invalid IPv6 URL and neither regex matches that string, while URI.create rejects the same string as Expected closing bracket for IPv6 address.
Adding it to this list would cover it. Nothing asserts Invalid skill URL: on the Java side either (Skills.java:165-167), if you want one per language like last time.
|
thanks for looking this over, addressed the nit. |
|
fixed conflicts, should be good to go |
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for working on this security hardening.
| if (!actual.equals(normalizedSha256)) { | ||
| throw new IllegalArgumentException( | ||
| "SHA-256 mismatch for skill archive " | ||
| + url |
There was a problem hiding this comment.
This error includes the full configured URL, so signed URLs may expose query credentials through logs or exception reports. Python has the same behavior, and both SkillManager implementations also include the raw spec.params. Could we sanitize or omit URLs in these error paths?
| + " insecure HTTP for this source: " | ||
| + url); | ||
| } | ||
| if (uri.getRawAuthority() == null || uri.getRawAuthority().isEmpty()) { |
There was a problem hiding this comment.
getRawAuthority() only checks that the authority string is non-empty, so URLs like https://:443/x.zip and https://example.com:bad/x.zip are accepted. Python's parsed.netloc check has the same issue. Could we validate the actual hostname and port in both implementations?
44b0373 to
e98e0f7
Compare
|
addressed comments and made another review pass. |
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. Everything I raised in the earlier rounds is closed at this head, so the notes below are all on code added since.
| } | ||
|
|
||
| @Test | ||
| void rawPercentEscapeIsRejectedByDownloader() { |
There was a problem hiding this 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?
There was a problem hiding this comment.
Fixed by rejecting this URL in the shared Java validator before networking and asserting its exact exception message.
| except AddressValueError: | ||
| return False | ||
| return True | ||
| if not hostname.isascii(): |
There was a problem hiding this 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?
There was a problem hiding this comment.
Aligned both languages to reject raw Unicode authorities and raw % IPv6 zones while accepting encoded %25 zones.
| } | ||
| return uri.getScheme() + "://" + authority + uri.getRawPath(); | ||
| } catch (IllegalArgumentException ignored) { | ||
| return "<redacted>"; |
There was a problem hiding this 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?
There was a problem hiding this comment.
Matched Python. Preserves safe host/path context while stripping secrets and retaining <redacted> for unsafe inputs.
| "https://example.com:/x.zip", | ||
| "https://999/x.zip", | ||
| "https://1bar/x.zip", | ||
| "https://999./x.zip")) { |
There was a problem hiding this 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?
There was a problem hiding this comment.
added 65535 as a valid port boundary test in both Java and Python.
Generated-by: Codex (GPT-5)
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. All four notes from last round are closed at this head. One small thing on the Python side, inline.
| netloc = parts.netloc.rsplit("@", 1)[-1] | ||
| if not netloc: | ||
| return "<redacted>" | ||
| return urlunsplit((parts.scheme, netloc, parts.path, "", "")) |
There was a problem hiding this comment.
nit: redact_skill_url still lets a raw control character through, while the new Java guard now blocks it.
https://u:[email protected]/a\x1b[31mred?token=SECRET:
- Java:
Invalid skill URL: <redacted> - Python:
Invalid skill URL: https://example.com/a\x1b[31mred
Small either way. Neither side leaks the password or the token, and CR/LF cannot get through on Python because urlsplit strips them first. So it is an ANSI escape in an exception message, not log line forging.
The Java half is containsUnsafeLogCharacter (SkillUrlUtils.java:133), pinned by SkillsResourceTest.java:144. Worth mirroring here, or is the Java-only guard deliberate?
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for the update and for addressing the previous feedback.
| if (authority.isEmpty()) { | ||
| return REDACTED; | ||
| } | ||
| return scheme + "://" + authority + (path == null ? "" : path); |
There was a problem hiding this 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?
There was a problem hiding this comment.
Invalid-port authorities now fail closed to <redacted> in both languages, including through SkillManager, with coverage for the reported credential-shaped case.
| URL effectiveUrl = u; | ||
| int redirects = 0; | ||
| while (true) { | ||
| conn = (HttpURLConnection) effectiveUrl.openConnection(); |
There was a problem hiding this 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?
There was a problem hiding this comment.
Going to reject scoped IPv6 URLs consistently.
Purpose of change
Closes #1003.
Remote skill archives are trusted instructions and code: their
SKILL.mdcontent is consumed by the agent, and bundled scripts can become executable through the built-in tools. Until now,Skills.fromUrl(...)/Skills.from_url(...)accepted plain HTTP and extracted whatever arrived, without verifying an operator-provided integrity expectation.This change makes URL skill sources secure by default and adds optional integrity verification across the Java, Python, and YAML surfaces.
allow_insecure_httpYAML option.url_sourceslist while preserving the existingurlsshorthand for unpinned HTTPS sources.Existing direct and same-protocol-redirecting HTTPS sources continue to download successfully. When a same-protocol redirect changes the effective URL, both runtimes now emit a warning with sensitive URL components removed. Existing plain HTTP sources must opt in explicitly.
Tests
./tools/ut.sh -j) passed under JDK 17 across all 32 reactor modules.git diff --checkpassed.API
Yes. The Java and Python skill factories reject plain HTTP by default and provide aligned methods for SHA-256 pinning and explicit HTTP opt-in:
fromUrlWithSha256,fromUrlUnsafe, andfromUrlUnsafeWithSha256.from_url_with_sha256,from_url_unsafe, andfrom_url_unsafe_with_sha256.YAML provides a structured
url_sourceslist whose entries accepturl, optionalsha256, and optionalallow_insecure_http. The existingurlslist remains available for ordinary HTTPS sources. Invalid URL and digest configuration is rejected during YAML loading, while runtime validation remains as defense in depth.Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5)