[api][runtime][java][python] Require HTTPS and add digest pinning for URL skill sources

[rob-9]

Purpose of change

Closes #1003.

Remote skill archives are trusted instructions and code: their SKILL.md content 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.

• Reject plain HTTP URL skill sources by default; HTTPS is required.
• Keep plain HTTP available only through explicitly named unsafe factory methods or the allow_insecure_http YAML option.
• Let operators pin an expected SHA-256 digest, which is verified against the downloaded archive before extraction.
• Enforce the transport and digest rules again at runtime, so sources arriving through plan serialization or YAML cannot bypass API-level validation.
• Reject cross-protocol redirects before requesting their targets.
• Log sanitized configured and effective URLs when a same-protocol redirect changes the source location.
• Clean up temporary archives on every failure path.
• Add a structured YAML url_sources list while preserving the existing urls shorthand for unpinned HTTPS sources.
• Validate URL sources during YAML loading so invalid transport or digest configuration fails before TaskManager execution.
• Keep Java and Python aligned on configuration, security behavior, tests, documentation, and generated YAML schemas.

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

• Full Java suite (./tools/ut.sh -j) passed under JDK 17 across all 32 reactor modules.
• Full Python non-integration suite passed: 844 passed, 13 skipped, 144 deselected.
• Java agent-plan cross-language tests passed: 9 passed, 1 skipped.
• Ruff passed on the changed Python files.
• git diff --check passed.
• Java Spotless passed under JDK 17.
• YAML schema consistency was verified for the original schema changes, including both generated schema copies and the updated contract blob hash.
• Apache RAT passed on the original patch.

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:

• Java: fromUrlWithSha256, fromUrlUnsafe, and fromUrlUnsafeWithSha256.
• Python: from_url_with_sha256, from_url_unsafe, and from_url_unsafe_with_sha256.

YAML provides a structured url_sources list whose entries accept url, optional sha256, and optional allow_insecure_http. The existing urls list 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-needed
• doc-not-needed
• doc-included

Was this patch authored or co-authored using generative AI tooling?

• Yes
• No

Generated-by: OpenAI Codex (GPT-5)

[weiqingy]

Thanks for taking this on. A few questions inline.

[weiqingy]

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.

[rob-9]

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.

[weiqingy]

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.

[rob-9]

pinning remains the integrity control, but surfacing redirects is useful too. added URL logging in both runtimes and documented Java's cross-protocol behavior,

[weiqingy]

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?

[rob-9]

no reason, that works well. updated tests so they now fail if extraction happens first.

[weiqingy]

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?

[rob-9]

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.

[weiqingy]

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?

[rob-9]

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.

[weiqingy]

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?

[rob-9]

agreed. extended both round-trip tests to cover allow_insecure_http and verify it remains the string "true" thru serialization and deserialization.

[weiqingy]

Thanks for addressing the comments. While going back through the updated code I spotted a few smaller things, all inline.

[weiqingy]

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?

[rob-9]

good catch. this wasn't intentional, removed the override and added a test to confirm.

[weiqingy]

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?

[rob-9]

added focused Java and Python tests for plain HTTP in url_sources without the opt-in.

[weiqingy]

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?

[rob-9]

aligned Python with Java so malformed URLs fail immediately. also added coverage for whitespace and invalid percent escapes.

[weiqingy]

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?

[rob-9]

done!

[weiqingy]

Thanks for addressing the comments. All four are closed on my side at e6176fc3. One small coverage thing inline.

[weiqingy]

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.