Copilot commented on code in PR #4084: URL: https://github.com/apache/hertzbeat/pull/4084#discussion_r2971461456
########## hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/IpmiProtocolTest.java: ########## @@ -0,0 +1,135 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.common.entity.job.protocol; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class IpmiProtocolTest { + + @Test + void isInvalidValidProtocol() { + IpmiProtocol protocol = IpmiProtocol.builder() + .host("192.168.1.1") + .port("623") + .username("admin") + .password("password") + .type("Chassis") + .build(); + assertFalse(protocol.isInvalid()); + } + + @Test + void isInvalidValidRawProtocol() { + IpmiProtocol protocol = IpmiProtocol.builder() + .host("192.168.1.1") + .port("623") + .username("admin") + .password("password") + .type("Raw") + .field(new IpmiProtocol.Field()) + .build(); + assertFalse(protocol.isInvalid()); + } + + @Test + void isInvalidInvalidHost() { + IpmiProtocol protocol = IpmiProtocol.builder() + .host("") + .port("623") + .username("admin") + .password("password") + .type("Chassis") + .build(); + assertTrue(protocol.isInvalid()); + } + + @Test + void isInvalidInvalidPort() { + IpmiProtocol protocol = IpmiProtocol.builder() + .host("192.168.1.1") + .port("70000") + .username("admin") + .password("password") + .type("Chassis") + .build(); + assertTrue(protocol.isInvalid()); Review Comment: Add a test case for `port="0"` being invalid for IPMI (if that’s the intended behavior). As implemented, `validPort` allows 0 and `IpmiProtocol.isInvalid()` doesn’t explicitly reject it. ########## hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/WebsocketProtocol.java: ########## @@ -47,8 +51,15 @@ public class WebsocketProtocol implements CommonRequestProtocol, Protocol { @Override public boolean isInvalid() { - - // todo: add - return true; + if (!validateIpDomain(host) || !validPort(port)) { + return true; + } + if (Integer.parseInt(port) <= 0) { + return true; + } + if (StringUtils.isBlank(path)) { Review Comment: `StringUtils.isBlank(path)` treats whitespace-only values (e.g., " ") as blank, so this code currently considers a whitespace path as valid (returns false) and skips the whitespace check below. If the intent is to allow only null/empty path as optional, switch to an empty check (null or length==0) and keep rejecting paths that contain whitespace. ```suggestion if (StringUtils.isEmpty(path)) { ``` ########## hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/RedfishProtocolTest.java: ########## @@ -0,0 +1,171 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.common.entity.job.protocol; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; +import org.junit.jupiter.api.Test; + +class RedfishProtocolTest { + + @Test + void isInvalidValidProtocol() { + RedfishProtocol protocol = RedfishProtocol.builder() + .host("127.0.0.1") + .port("443") + .username("Administrator") + .password("Password") + .timeout("5000") + .jsonPath(List.of("$.Id")) + .build(); + assertFalse(protocol.isInvalid()); + } + + @Test + void isInvalidValidProtocolWithSchemaHost() { + RedfishProtocol protocol = RedfishProtocol.builder() + .host("https://127.0.0.1";) + .port("443") + .username("Administrator") + .password("Password") + .timeout("5000") Review Comment: Add a negative test for malformed schema hosts like `host="https://"` to ensure they’re rejected; the current schema check can accept an empty host part after the scheme. ########## hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/IpmiProtocol.java: ########## @@ -72,8 +76,20 @@ static class Field { @Override public boolean isInvalid() { - - // todo: add - return true; + if (!validateIpDomain(host) || !validPort(port)) { Review Comment: This protocol disallows invalid/out-of-range ports via `validPort`, but `validPort` currently treats port 0 as valid. Other newly-updated protocols in this PR explicitly reject `port <= 0`, and `IpmiProtocol` currently would accept `port="0"`. Add an explicit `Integer.parseInt(port) <= 0` (or similar) check if port 0 should be invalid, and add a corresponding test case. ```suggestion if (!validateIpDomain(host) || !validPort(port) || Integer.parseInt(port) <= 0) { ``` ########## hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/PushProtocolTest.java: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.common.entity.job.protocol; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; +import org.apache.hertzbeat.common.entity.dto.Field; +import org.junit.jupiter.api.Test; + +class PushProtocolTest { + + @Test + void isInvalidValidProtocol() { + PushProtocol protocol = PushProtocol.builder() + .host("127.0.0.1") + .port("1157") + .uri("/api/push") + .fields(List.of(Field.builder().name("cpuUsage").type((byte) 0).build())) + .build(); + assertFalse(protocol.isInvalid()); + } + + @Test + void isInvalidValidProtocolWithSchemaHost() { + PushProtocol protocol = PushProtocol.builder() + .host("http://127.0.0.1";) + .port("1157") + .uri("/api/push") + .fields(List.of(Field.builder().name("status").type((byte) 1).build())) Review Comment: Consider adding a negative test for malformed schema hosts like `host="http://"` to ensure they are rejected. The current schema check accepts any string starting with http(s)://, even if the host part is missing. ########## hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/RedfishProtocol.java: ########## @@ -65,8 +70,30 @@ public class RedfishProtocol implements CommonRequestProtocol, Protocol { @Override public boolean isInvalid() { - - // todo: add - return true; + if ((!validateIpDomain(host) && !isHasSchema(host)) || !validPort(port)) { + return true; + } Review Comment: Same issue as PushProtocol: `isHasSchema(host)` will accept malformed values like "https://"; and make them pass validation. Consider validating schema-based hosts by parsing the URI and checking `getHost()` is non-empty (and optionally `validateIpDomain` on that host) before accepting. ########## hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/PushProtocol.java: ########## @@ -39,8 +44,26 @@ public class PushProtocol implements CommonRequestProtocol, Protocol { @Override public boolean isInvalid() { - - // todo: add - return true; + if ((!validateIpDomain(host) && !isHasSchema(host)) || !validPort(port)) { + return true; Review Comment: `isHasSchema(host)` is currently satisfied by strings like "http://"; (because the underlying regex allows an empty host after the scheme), so `host="http://"` will be treated as valid here. Add a stricter validation for schema-hosts (e.g., parse as URI/URL and require a non-empty host, optionally validate it with `validateIpDomain(uri.getHost())`) so malformed hosts don’t pass validation. ########## hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/WebsocketProtocolTest.java: ########## @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.common.entity.job.protocol; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class WebsocketProtocolTest { + + @Test + void isInvalidValidProtocol() { + WebsocketProtocol protocol = WebsocketProtocol.builder() + .host("127.0.0.1") + .port("80") + .path("/") + .build(); + assertFalse(protocol.isInvalid()); + } + + @Test + void isInvalidValidProtocolWithoutPath() { + WebsocketProtocol protocol = WebsocketProtocol.builder() + .host("127.0.0.1") + .port("80") + .path("") Review Comment: Add a test for a whitespace-only websocket path (e.g., " ") to clarify/lock in expected behavior. With the current implementation, whitespace-only paths are treated as blank and therefore valid. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
