Full-stack: Make "Server url" validation conditions consistent across Fleet, update Web Address form validation and submission logic per Fleet best practices (frontend/docs/patterns.md)

changes/27454-validation

@@ -0,0 +1 @@
+- Accept any "http://" or "https://" prefixed Fleet web URL

frontend/components/forms/RegistrationForm/FleetDetails/FleetDetails.tests.jsx

@@ -5,6 +5,8 @@ import { renderWithSetup } from "test/test-utils";
 
 import FleetDetails from "components/forms/RegistrationForm/FleetDetails";
 
+import INVALID_SERVER_URL_MESSAGE from "utilities/error_messages";
+
 describe("FleetDetails - form", () => {
   const handleSubmitSpy = jest.fn();
   it("renders", () => {
@@ -29,24 +31,30 @@ describe("FleetDetails - form", () => {
     ).toBeInTheDocument();
   });
 
-  it("validates the fleet web address field starts with https://", async () => {
+  it("validates the Fleet server URL field starts with 'https://' or 'http://'", async () => {
     const { user } = renderWithSetup(
       <FleetDetails handleSubmit={handleSubmitSpy} currentPage />
     );
 
-    await user.type(
-      screen.getByRole("textbox", { name: "Fleet web address" }),
-      "http://gnar.Fleet.co"
-    );
-    await user.click(screen.getByRole("button", { name: "Next" }));
+    const inputField = screen.getByRole("textbox", {
+      name: "Fleet web address",
+    });
+    const nextButton = screen.getByRole("button", { name: "Next" });
+
+    await user.type(inputField, "gnar.Fleet.co");
+    await user.click(nextButton);
 
     expect(handleSubmitSpy).not.toHaveBeenCalled();
-    expect(
-      screen.getByText("Fleet web address must start with https://")
-    ).toBeInTheDocument();
+    expect(screen.getByText(INVALID_SERVER_URL_MESSAGE)).toBeInTheDocument();
+
+    await user.type(inputField, "localhost:8080");
+    await user.click(nextButton);
+
+    expect(handleSubmitSpy).not.toHaveBeenCalled();
+    expect(screen.getByText(INVALID_SERVER_URL_MESSAGE)).toBeInTheDocument();
   });
 
-  it("submits the form when valid", async () => {
+  it("submits the form with valid https link", async () => {
     const { user } = renderWithSetup(
       <FleetDetails handleSubmit={handleSubmitSpy} currentPage />
     );
@@ -61,4 +69,19 @@ describe("FleetDetails - form", () => {
       server_url: "https://gnar.Fleet.co",
     });
   });
+  it("submits the form with valid http link", async () => {
+    const { user } = renderWithSetup(
+      <FleetDetails handleSubmit={handleSubmitSpy} currentPage />
+    );
+    // when
+    await user.type(
+      screen.getByRole("textbox", { name: "Fleet web address" }),
+      "http://localhost:8080"
+    );
+    await user.click(screen.getByRole("button", { name: "Next" }));
+    // then
+    expect(handleSubmitSpy).toHaveBeenCalledWith({
+      server_url: "http://localhost:8080",
+    });
+  });
 });

frontend/components/forms/RegistrationForm/FleetDetails/helpers.js

@@ -1,15 +1,23 @@
-import { size, startsWith } from "lodash";
+import { size } from "lodash";
+
+import validUrl from "components/forms/validators/valid_url";
+
+import INVALID_SERVER_URL_MESSAGE from "utilities/error_messages";
 
 const validate = (formData) => {
   const errors = {};
   const { server_url: fleetWebAddress } = formData;
 
   if (!fleetWebAddress) {
     errors.server_url = "Fleet web address must be completed";
-  }
-
-  if (fleetWebAddress && !startsWith(fleetWebAddress, "https://")) {
-    errors.server_url = "Fleet web address must start with https://";
+  } else if (
+    !validUrl({
+      url: fleetWebAddress,
+      protocols: ["http", "https"],
+      allowLocalHost: true,
+    })
+  ) {
+    errors.server_url = INVALID_SERVER_URL_MESSAGE;
   }
 
   const valid = !size(errors);

frontend/components/forms/validators/valid_url/valid_url.ts

@@ -6,8 +6,13 @@ interface IValidUrl {
   url: string;
   /**  Validate protocols specified */
   protocols?: ("http" | "https")[];
+  allowLocalHost?: boolean;
 }
 
-export default ({ url, protocols }: IValidUrl): boolean => {
-  return isURL(url, { protocols });
+export default ({ url, protocols, allowLocalHost = false }: IValidUrl) => {
+  return isURL(url, {
+    protocols,
+    require_protocol: !!protocols?.length,
+    require_tld: !allowLocalHost,
+  });
 };

frontend/docs/patterns.md

@@ -208,9 +208,10 @@ When building a React-controlled form:
 - Use the native HTML `form` element to wrap the form.
 - Use a `Button` component with `type="submit"` for its submit button.
 - Write a submit handler, e.g. `handleSubmit`, that accepts an `evt:
-React.FormEvent<HTMLFormElement>` argument and, critically, calls `evt.preventDefault()` in its
-body. This prevents the HTML `form`'s default submit behavior from interfering with our custom
+React.FormEvent<HTMLFormElement>` argument and, critically:
+  - calls `evt.preventDefault()` in its body. This prevents the HTML `form`'s default submit behavior from interfering with our custom
 handler's logic.
+  - does nothing (e.g., returns `null`) if the form is in an invalid state, preventing submission by any means.
 - Assign that handler to the `form`'s `onSubmit` property (*not* the submit button's `onClick`)
 
 ### Data validation
@@ -248,7 +249,9 @@ const onInputChange = ({ name, value }: IFormField) => {
   // new errors are only set onBlur
   const errsToSet: Record<string, string> = {};
   Object.keys(formErrors).forEach((k) => {
+    // @ts-ignore
     if (newErrs[k]) {
+      // @ts-ignore
       errsToSet[k] = newErrs[k];
     }
   });
@@ -270,7 +273,6 @@ const onInputBlur = () => {
 ```tsx
 const onFormSubmit = (evt: React.MouseEvent<HTMLFormElement>) => {
   evt.preventDefault();
-
   // return null if there are errors
   const errs = validateFormData(formData);
   if (Object.keys(errs).length > 0) {

frontend/pages/admin/OrgSettingsPage/cards/WebAddress/WebAddress.tsx

@@ -1,4 +1,5 @@
 import React, { useState } from "react";
+import { size } from "lodash";
 
 import Button from "components/buttons/Button";
 // @ts-ignore
@@ -7,6 +8,8 @@ import validUrl from "components/forms/validators/valid_url";
 import SectionHeader from "components/SectionHeader";
 import GitOpsModeTooltipWrapper from "components/GitOpsModeTooltipWrapper";
 
+import INVALID_SERVER_URL_MESSAGE from "utilities/error_messages";
+
 import { IAppConfigFormProps, IFormField } from "../constants";
 
 interface IWebAddressFormData {
@@ -16,9 +19,25 @@ interface IWebAddressFormData {
 interface IWebAddressFormErrors {
   server_url?: string | null;
 }
-
 const baseClass = "app-config-form";
 
+const validateFormData = ({ serverURL }: IWebAddressFormData) => {
+  const errors: IWebAddressFormErrors = {};
+  if (!serverURL) {
+    errors.server_url = "Fleet server URL must be present";
+  } else if (
+    !validUrl({
+      url: serverURL,
+      protocols: ["http", "https"],
+      allowLocalHost: true,
+    })
+  ) {
+    errors.server_url = INVALID_SERVER_URL_MESSAGE;
+  }
+
+  return errors;
+};
+
 const WebAddress = ({
   appConfig,
   handleSubmit,
@@ -35,23 +54,34 @@ const WebAddress = ({
   const [formErrors, setFormErrors] = useState<IWebAddressFormErrors>({});
 
   const onInputChange = ({ name, value }: IFormField) => {
-    setFormData({ ...formData, [name]: value });
-    setFormErrors({});
+    const newFormData = { ...formData, [name]: value };
+    setFormData(newFormData);
+    const newErrs = validateFormData(newFormData);
+    // only set errors that are updates of existing errors
+    // new errors are only set onBlur
+    const errsToSet: Record<string, string> = {};
+    Object.keys(formErrors).forEach((k) => {
+      // @ts-ignore
+      if (newErrs[k]) {
+        // @ts-ignore
+        errsToSet[k] = newErrs[k];
+      }
+    });
+    setFormErrors(errsToSet);
   };
 
-  const validateForm = () => {
-    const errors: IWebAddressFormErrors = {};
-    if (!serverURL) {
-      errors.server_url = "Fleet server URL must be present";
-    } else if (!validUrl({ url: serverURL, protocols: ["http", "https"] })) {
-      errors.server_url = `${serverURL} is not a valid URL`;
-    }
-
-    setFormErrors(errors);
+  const onInputBlur = () => {
+    setFormErrors(validateFormData(formData));
   };
 
-  const onFormSubmit = (evt: React.MouseEvent<HTMLFormElement>) => {
+  const onFormSubmit = (evt: React.FormEvent<HTMLFormElement>) => {
     evt.preventDefault();
+    // return null if there are errors
+    const errs = validateFormData(formData);
+    if (size(errs)) {
+      setFormErrors(errs);
+      return;
+    }
 
     // Formatting of API not UI
     const formDataToSubmit = {
@@ -79,7 +109,7 @@ const WebAddress = ({
             name="serverURL"
             value={serverURL}
             parseTarget
-            onBlur={validateForm}
+            onBlur={onInputBlur}
             error={formErrors.server_url}
             tooltip="The base URL of this instance for use in Fleet links."
             disabled={gitOpsModeEnabled}
@@ -90,7 +120,7 @@ const WebAddress = ({
               <Button
                 type="submit"
                 variant="brand"
-                disabled={Object.keys(formErrors).length > 0 || disableChildren}
+                disabled={!!size(formErrors) || disableChildren}
                 className="button-wrap"
                 isLoading={isUpdatingSettings}
               >

frontend/utilities/error_messages.ts

@@ -0,0 +1,3 @@
+const INVALID_SERVER_URL_MESSAGE = `Fleet server address must be a valid “https” or “http” URL.`;
+
+export default INVALID_SERVER_URL_MESSAGE;

server/fleet/errors.go

@@ -627,6 +627,9 @@ const (
 
 	// Labels
 	InvalidLabelSpecifiedErrMsg = "Invalid label name(s):"
+
+	// Config
+	InvalidServerURLMsg = `Fleet server URL must use “https” or “http”.`
 )
 
 // ConflictError is used to indicate a conflict, such as a UUID conflict in the DB.

server/service/appconfig.go

@@ -417,6 +417,10 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle
 	}
 	if appConfig.ServerSettings.ServerURL == "" {
 		invalid.Append("server_url", "Fleet server URL must be present")
+	} else {
+		if err := ValidateServerURL(appConfig.ServerSettings.ServerURL); err != nil {
+			invalid.Append("server_url", "Couldn't update settings: "+err.Error())
+		}
 	}
 
 	if appConfig.ActivityExpirySettings.ActivityExpiryEnabled && appConfig.ActivityExpirySettings.ActivityExpiryWindow < 1 {
@@ -924,8 +928,8 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle
 }
 
 func (svc *Service) processAppConfigCAs(ctx context.Context, newAppConfig *fleet.AppConfig, oldAppConfig *fleet.AppConfig,
-	appConfig *fleet.AppConfig, invalid *fleet.InvalidArgumentError) (appConfigCAStatus, error) {
-
+	appConfig *fleet.AppConfig, invalid *fleet.InvalidArgumentError,
+) (appConfigCAStatus, error) {
 	var invalidLicense bool
 	fleetLicense, _ := license.FromContext(ctx)
 	if newAppConfig.Integrations.NDESSCEPProxy.Set && newAppConfig.Integrations.NDESSCEPProxy.Valid && !fleetLicense.IsPremium() {
@@ -1249,7 +1253,8 @@ func (svc *Service) populateCustomSCEPChallenges(ctx context.Context, remainingO
 // filterDeletedDigiCertCAs identifies deleted DigiCert integrations in the provided configs.
 // It mutates the provided result to set a deleted status where applicable and returns a list of the remaining (non-deleted) integrations.
 func filterDeletedDigiCertCAs(oldAppConfig *fleet.AppConfig, newAppConfig *fleet.AppConfig,
-	result *appConfigCAStatus) []fleet.DigiCertIntegration {
+	result *appConfigCAStatus,
+) []fleet.DigiCertIntegration {
 	remainingOldCAs := make([]fleet.DigiCertIntegration, 0, len(oldAppConfig.Integrations.DigiCert.Value))
 	for _, oldCA := range oldAppConfig.Integrations.DigiCert.Value {
 		var found bool
@@ -1271,7 +1276,8 @@ func filterDeletedDigiCertCAs(oldAppConfig *fleet.AppConfig, newAppConfig *fleet
 // filterDeletedCustomSCEPCAs identifies deleted custom SCEP integrations in the provided configs.
 // It mutates the provided result to set a deleted status where applicable and returns a list of the remaining (non-deleted) integrations.
 func filterDeletedCustomSCEPCAs(oldAppConfig *fleet.AppConfig, newAppConfig *fleet.AppConfig,
-	result *appConfigCAStatus) []fleet.CustomSCEPProxyIntegration {
+	result *appConfigCAStatus,
+) []fleet.CustomSCEPProxyIntegration {
 	remainingOldCAs := make([]fleet.CustomSCEPProxyIntegration, 0, len(oldAppConfig.Integrations.CustomSCEPProxy.Value))
 	for _, oldCA := range oldAppConfig.Integrations.CustomSCEPProxy.Value {
 		var found bool

server/service/validation_setup.go

@@ -18,7 +18,7 @@ func (mw validationMiddleware) NewAppConfig(ctx context.Context, payload fleet.A
 	} else {
 		serverURLString = cleanupURL(payload.ServerSettings.ServerURL)
 	}
-	if err := validateServerURL(serverURLString); err != nil {
+	if err := ValidateServerURL(serverURLString); err != nil {
 		invalid.Append("server_url", err.Error())
 	}
 	if invalid.HasErrors() {
@@ -27,14 +27,21 @@ func (mw validationMiddleware) NewAppConfig(ctx context.Context, payload fleet.A
 	return mw.Service.NewAppConfig(ctx, payload)
 }
 
-func validateServerURL(urlString string) error {
-	serverURL, err := url.Parse(urlString)
+func ValidateServerURL(urlString string) error {
+	// TODO - implement more robust URL validation here
+
+	// no valid scheme provided
+	if !(strings.HasPrefix(urlString, "http://") || strings.HasPrefix(urlString, "https://")) {
+		return errors.New(fleet.InvalidServerURLMsg)
+	}
+
+	// valid scheme provided - require host
+	parsed, err := url.Parse(urlString)
 	if err != nil {
 		return err
 	}
-
-	if serverURL.Scheme != "https" && !strings.Contains(serverURL.Host, "localhost") {
-		return errors.New("url scheme must be https")
+	if parsed.Host == "" {
+		return errors.New(fleet.InvalidServerURLMsg)
 	}
 
 	return nil