weiqingy commented on code in PR #1005:
URL: https://github.com/apache/flink-agents/pull/1005#discussion_r3930495872
##########
api/src/test/java/org/apache/flink/agents/api/skills/SkillsResourceTest.java:
##########
@@ -48,6 +53,224 @@ 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://example.com./x.zip";,
+ "https://example.com:/x.zip";,
+ "https://example.com:65535/x.zip";,
+ "https://999/x.zip";,
+ "https://1bar/x.zip";,
+ "https://999./x.zip";)) {
+ assertEquals(url,
Skills.fromUrl(url).getSources().get(0).getParams().get("url"));
+ }
+ }
+
+ @Test
+ void fromUrlRejectsScopedIpv6() {
+ IllegalArgumentException ex =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> Skills.fromUrl("https://[fe80::1%25lo0]/x.zip";));
+ assertTrue(ex.getMessage().contains("must not include an IPv6 zone
identifier"));
+ assertNull(ex.getCause());
+ }
+
+ @Test
+ void fromUrlWithSha256EmitsIntegrityParam() {
+ String digest = "A".repeat(64);
+ Skills skills = Skills.fromUrlWithSha256("https://example.com/x.zip";,
digest);
+ assertEquals(
+ List.of(
+ new SkillSourceSpec(
+ "url",
+ Map.of("url", "https://example.com/x.zip";,
"sha256", digest))),
+ skills.getSources());
+ }
+
+ @Test
+ void fromUrlUnsafeRequiresExplicitParam() {
+ Skills skills = Skills.fromUrlUnsafe("http://example.com/x.zip";);
+ assertEquals("true",
skills.getSources().get(0).getParams().get("allow_insecure_http"));
+ }
+
+ @Test
+ void fromUrlUnsafeWithSha256EmitsBothParams() {
+ String digest = "a".repeat(64);
+ Skills skills =
Skills.fromUrlUnsafeWithSha256("http://example.com/x.zip";, digest);
+ assertEquals(
+ Map.of(
+ "url",
+ "http://example.com/x.zip";,
+ "sha256",
+ digest,
+ "allow_insecure_http",
+ "true"),
+ skills.getSources().get(0).getParams());
+ }
+
+ @Test
+ void fromUrlRejectsPlainHttpByDefault() {
+ assertThrows(
+ IllegalArgumentException.class, () ->
Skills.fromUrl("http://example.com/x.zip";));
+ }
+
+ @Test
+ void fromUrlWithSha256RejectsMalformedDigest() {
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> Skills.fromUrlWithSha256("https://example.com/x.zip";,
"invalid"));
+ }
+
+ @Test
+ void fromUrlRejectsUnsupportedSchemeClearly() {
+ IllegalArgumentException ex =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ Skills.fromUrl(
+
"ftp://user:[email protected]/x.zip?token=secret#part";));
+ assertEquals(
+ "Only HTTP(S) skill URLs are supported:
ftp://example.com/x.zip";, ex.getMessage());
+ assertFalse(ex.getMessage().contains("password"));
+ assertFalse(ex.getMessage().contains("secret"));
+ }
+
+ @Test
+ void fromUrlRejectsMalformedUrl() {
+ IllegalArgumentException unparseable =
+ assertThrows(
+ IllegalArgumentException.class, () ->
Skills.fromUrl("https://[::1/x.zip";));
+ assertEquals("Invalid skill URL: <redacted>",
unparseable.getMessage());
+
+ IllegalArgumentException unsafeForLogs =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
Skills.fromUrl("https://u:[email protected]/a\nheader?token=SECRET";));
+ assertEquals("Invalid skill URL: <redacted>",
unsafeForLogs.getMessage());
+ assertFalse(unsafeForLogs.getMessage().contains("pw"));
+ assertFalse(unsafeForLogs.getMessage().contains("SECRET"));
+
+ for (String url :
+ List.of("https://example.com/skills[1].zip";,
"https://[fe80::1%eth0]/x.zip";)) {
+ IllegalArgumentException ex =
+ assertThrows(IllegalArgumentException.class, () ->
Skills.fromUrl(url), url);
+ assertEquals("Invalid skill URL: " + url, ex.getMessage());
+ }
+ }
+
+ @Test
+ void fromUrlRedactsMalformedUrlWithoutLosingSafeContext() {
+ for (String path : List.of("a b.zip", "%zz")) {
+ IllegalArgumentException ex =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ Skills.fromUrl(
+ "https://u:[email protected]/";
+ + path
+ +
"?token=SECRET#fragment"));
+ assertEquals("Invalid skill URL: https://example.com/"; + path,
ex.getMessage());
+ assertFalse(ex.getMessage().contains("pw"));
+ assertFalse(ex.getMessage().contains("SECRET"));
+ }
+ }
+
+ @Test
+ void fromUrlAcceptsBracketsOutsideRawPath() {
+ for (String url :
+ List.of(
+ "https://example.com/skills%5B1%5D.zip";,
+ "https://example.com/x.zip?a[0]=1";,
"https://example.com/x.zip#f[1]";)) {
+ assertEquals(url,
Skills.fromUrl(url).getSources().get(0).getParams().get("url"));
+ }
+ }
+
+ @Test
+ void fromUrlRejectsInvalidHostAndPort() {
+ for (String url :
+ List.of(
+ "https://example.com:bad/x.zip?token=top-secret";,
+ "https://user:supersecret/x.zip?token=TOPSECRET";)) {
Review Comment:
The behaviour here is right, so this is only about test coverage.
`hasInvalidPort` is new hand-written parsing with several ways to return
true, and this list covers one of them. I flipped these two return values one
at a time on a private copy and ran api + plan + runtime, 1387 tests. Each flip
stays green and puts a credential-shaped authority back into a user-facing
message:
- `SkillUrlUtils.java:158`, more than one colon: `Skill URL must include a
valid host and, when present, a valid port: https://user:sup:ersecret/x.zip`
- `SkillUrlUtils.java:173`, port above 65535: `Skill URL port must be
between 0 and 65535: https://user:999999/x.zip`
Two more entries in this list catch both, since the `endsWith("<redacted>")`
assertion already does the work:
```java
List.of(
"https://example.com:bad/x.zip?token=top-secret";,
"https://user:supersecret/x.zip?token=TOPSECRET";,
"https://user:sup:ersecret/x.zip?token=TOPSECRET";,
"https://user:999999/x.zip?token=TOPSECRET";)) {
```
Green on the real source, red under each flip when I tried it. Python looks
fine here, because `_ = parts.port` is one statement and three tests already
cover it.
Worth adding, or are these covered somewhere I missed?
--
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]
