Martin Peřina has posted comments on this change. Change subject: tools: Validate WebSocketProxy config value ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/37498/1/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/WebSocketProxyLocationValueHelper.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/WebSocketProxyLocationValueHelper.java: Line 23: } Line 24: } Line 25: Line 26: private boolean matchesHostColonPort(String s) { Line 27: return (s == null ? false : s.matches("\\S+:\\d+")); //$NON-NLS-1$ Well, this allows too many false positives. If you really want to validate input then the validation should contain these steps: 1. Is blank? 2. Equals to "Off"? 3. Split using ':' for 2 parts (hostname and port) 4. Is port part a number in range <1, 65535>? (or <1024, 65535> if websocket proxy doesn't run as root)? 5. Does hostname part contains or "Engine" or "Host"? 6. Does hostname part contains only characters valid for FQDN or IP address? Line 28: } http://gerrit.ovirt.org/#/c/37498/1/backend/manager/tools/src/test/java/org/ovirt/engine/core/config/entity/helper/WebSocketProxyLocationValueHelperTest.java File backend/manager/tools/src/test/java/org/ovirt/engine/core/config/entity/helper/WebSocketProxyLocationValueHelperTest.java: If you write a test, then please test all valid inputs and also at least some invalid. Line 1: package org.ovirt.engine.core.config.entity.helper; Line 2: Line 3: import org.junit.Test; Line 4: import org.omg.CORBA.WStringValueHelper; Line 1: package org.ovirt.engine.core.config.entity.helper; Line 2: Line 3: import org.junit.Test; Line 4: import org.omg.CORBA.WStringValueHelper; Please remove this import Line 5: Line 6: Line 7: import static org.junit.Assert.*; Line 8: Line 3: import org.junit.Test; Line 4: import org.omg.CORBA.WStringValueHelper; Line 5: Line 6: Line 7: import static org.junit.Assert.*; 1. According to our conventions static imports should be placed before normal imports 2. Please don't use wildcard imports Line 8: Line 9: public class WebSocketProxyLocationValueHelperTest { Line 10: Line 11: @Test Line 22: Line 23: @Test Line 24: public void testValidateIncomplete() throws Exception { Line 25: WebSocketProxyLocationValueHelper helper = new WebSocketProxyLocationValueHelper(); Line 26: assertFalse(helper.validate(null, "myengine:").isOk()); If user uses /etc/hosts on all machines, then "myengine" is a valid hostname ... Line 27: } Line 28: Line 29: @Test Line 30: public void testValidateHostPort() throws Exception { Line 31: WebSocketProxyLocationValueHelper helper = new WebSocketProxyLocationValueHelper(); Line 32: assertTrue(helper.validate(null, "myengine.com:6100").isOk()); Line 33: } Line 34: Line 35: } According to Checkstyle, you don't have a newline at the end of file. Please fix. -- To view, visit http://gerrit.ovirt.org/37498 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84b02d25a15762ba24544bd678328e6532d360ac Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
