fix: add checks for preventing HSL colors from entering React state (#9893)

Parkreiner · web-flow · commit dcad8fdc2fed · 2023-09-27T19:27:11.000Z
* fix: remove hsl color from initial form state

* chore: add extra color helpers

* chore: add extra runtime validation for colors

* chore: clean up comments for clarity

* chore: add tests for colors
diff --git a/site/src/components/Dashboard/DashboardProvider.tsx b/site/src/components/Dashboard/DashboardProvider.tsx
@@ -10,13 +10,16 @@ import {
 } from "api/typesGenerated";
 import { FullScreenLoader } from "components/Loader/FullScreenLoader";
 import {
+  type FC,
+  type PropsWithChildren,
   createContext,
-  FC,
-  PropsWithChildren,
+  useCallback,
   useContext,
   useState,
 } from "react";
 import { appearance } from "api/queries/appearance";
+import { hslToHex, isHexColor, isHslColor } from "utils/colors";
+import { displayError } from "components/GlobalSnackbar/utils";
 
 interface Appearance {
   config: AppearanceConfig;
@@ -45,8 +48,35 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ children }) => {
     !entitlementsQuery.data ||
     !appearanceQuery.data ||
     !experimentsQuery.data;
+
   const [configPreview, setConfigPreview] = useState<AppearanceConfig>();
 
+  // Centralizing the logic for catching malformed configs in one spot, just to
+  // be on the safe side; don't want to expose raw setConfigPreview outside
+  // the provider
+  const setPreview = useCallback((newConfig: AppearanceConfig) => {
+    // Have runtime safety nets in place, just because so much of the codebase
+    // relies on HSL for formatting, but server expects hex values. Can't catch
+    // color format mismatches at the type level
+    const incomingBg = newConfig.service_banner.background_color;
+    let configForDispatch = newConfig;
+
+    if (typeof incomingBg === "string" && isHslColor(incomingBg)) {
+      configForDispatch = {
+        ...newConfig,
+        service_banner: {
+          ...newConfig.service_banner,
+          background_color: hslToHex(incomingBg),
+        },
+      };
+    } else if (typeof incomingBg === "string" && !isHexColor(incomingBg)) {
+      displayError(`The value ${incomingBg} is not a valid hex string`);
+      return;
+    }
+
+    setConfigPreview(configForDispatch);
+  }, []);
+
   if (isLoading) {
     return <FullScreenLoader />;
   }
@@ -59,7 +89,7 @@ export const DashboardProvider: FC<PropsWithChildren> = ({ children }) => {
         experiments: experimentsQuery.data,
         appearance: {
           config: configPreview ?? appearanceQuery.data,
-          setPreview: setConfigPreview,
+          setPreview: setPreview,
           isPreview: configPreview !== undefined,
         },
       }}
diff --git a/site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPage.tsx b/site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPage.tsx
@@ -17,21 +17,17 @@ const AppearanceSettingsPage: FC = () => {
   const { appearance, entitlements } = useDashboard();
   const queryClient = useQueryClient();
   const updateAppearanceMutation = useMutation(updateAppearance(queryClient));
-  const isEntitled =
-    entitlements.features["appearance"].entitlement !== "not_entitled";
 
   const onSaveAppearance = async (
     newConfig: Partial<UpdateAppearanceConfig>,
     preview: boolean,
   ) => {
-    const newAppearance = {
-      ...appearance.config,
-      ...newConfig,
-    };
+    const newAppearance = { ...appearance.config, ...newConfig };
     if (preview) {
       appearance.setPreview(newAppearance);
       return;
     }
+
     try {
       await updateAppearanceMutation.mutateAsync(newAppearance);
       displaySuccess("Successfully updated appearance settings!");
@@ -50,8 +46,10 @@ const AppearanceSettingsPage: FC = () => {
 
       <AppearanceSettingsPageView
         appearance={appearance.config}
-        isEntitled={isEntitled}
         onSaveAppearance={onSaveAppearance}
+        isEntitled={
+          entitlements.features.appearance.entitlement !== "not_entitled"
+        }
       />
     </>
   );
diff --git a/site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx b/site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx
@@ -21,6 +21,7 @@ import { useFormik } from "formik";
 import { useTheme } from "@mui/styles";
 import Link from "@mui/material/Link";
 import { colors } from "theme/colors";
+import { hslToHex } from "utils/colors";
 
 export type AppearanceSettingsPageViewProps = {
   appearance: UpdateAppearanceConfig;
@@ -30,13 +31,17 @@ export type AppearanceSettingsPageViewProps = {
     preview: boolean,
   ) => void;
 };
+
+const fallbackBgColor = hslToHex(colors.blue[7]);
+
 export const AppearanceSettingsPageView = ({
   appearance,
   isEntitled,
   onSaveAppearance,
 }: AppearanceSettingsPageViewProps): JSX.Element => {
   const styles = useStyles();
   const theme = useTheme();
+
   const logoForm = useFormik<{
     logo_url: string;
   }>({
@@ -53,7 +58,7 @@ export const AppearanceSettingsPageView = ({
         message: appearance.service_banner.message,
         enabled: appearance.service_banner.enabled,
         background_color:
-          appearance.service_banner.background_color ?? colors.blue[7],
+          appearance.service_banner.background_color ?? fallbackBgColor,
       },
       onSubmit: (values) =>
         onSaveAppearance(
@@ -65,9 +70,11 @@ export const AppearanceSettingsPageView = ({
     },
   );
   const serviceBannerFieldHelpers = getFormHelpers(serviceBannerForm);
+
   const [backgroundColor, setBackgroundColor] = useState(
     serviceBannerForm.values.background_color,
   );
+
   return (
     <>
       <Header
diff --git a/site/src/utils/colors.test.ts b/site/src/utils/colors.test.ts
@@ -0,0 +1,81 @@
+/**
+ * Not testing hslToHex, because it's code directly copied from a reliable
+ * source
+ */
+import { isHslColor, isHexColor } from "./colors";
+
+describe(`${isHslColor.name}`, () => {
+  it("Should reject obviously wrong or malformed inputs", () => {
+    const wrongInputs = [
+      "",
+      "Hi :)",
+      "hsl",
+      "#333",
+      "#444666",
+      "rgb(255, 300, 299)",
+      "hsl(255deg, 10%, 20%",
+      "hsv(255deg, 10%, 20%)",
+      "hsb(255deg, 10%, 20%)",
+      "hsl(0%, 10deg, 20)",
+    ];
+
+    for (const str of wrongInputs) {
+      expect(isHslColor(str)).toBe(false);
+    }
+  });
+
+  it("Should allow strings with or without deg unit", () => {
+    const base = "hsl(200deg, 100%, 37%)";
+    expect(isHslColor(base)).toBe(true);
+
+    const withoutDeg = base.replace("deg", "");
+    expect(isHslColor(withoutDeg)).toBe(true);
+  });
+
+  it("Should not care about whether there are spaces separating the inner values", () => {
+    const inputs = [
+      "hsl(180deg,20%,97%)",
+      "hsl(180deg, 20%, 97%)",
+      "hsl(180deg,           20%,         97%)",
+    ];
+
+    for (const str of inputs) {
+      expect(isHslColor(str)).toBe(true);
+    }
+  });
+
+  it("Should reject HSL strings that don't have percents for saturation/luminosity values", () => {
+    expect(isHslColor("hsl(20%, 45, 92)")).toBe(false);
+  });
+
+  it("Should catch HSL strings that follow the correct pattern, but have impossible values", () => {
+    const inputs = [
+      "hsl(360deg, 120%, 240%)", // Impossible hue
+      "hsl(100deg, 120%, 84%)", //  Impossible saturation
+      "hsl(257, 12%, 110%)", //     Impossible luminosity
+      "hsl(360deg, 120%, 240%)", // Impossible everything
+    ];
+
+    for (const str of inputs) {
+      expect(isHslColor(str)).toBe(false);
+    }
+  });
+});
+
+describe(`${isHexColor.name}`, () => {
+  it("Should reject obviously wrong or malformed inputs", () => {
+    const inputs = ["", "#", "#bananas", "#ZZZZZZ", "AB179C", "#55555"];
+
+    for (const str of inputs) {
+      expect(isHexColor(str)).toBe(false);
+    }
+  });
+
+  it("Should be fully case-insensitive", () => {
+    const inputs = ["#aaaaaa", "#BBBBBB", "#CcCcCc"];
+
+    for (const str of inputs) {
+      expect(isHexColor(str)).toBe(true);
+    }
+  });
+});
diff --git a/site/src/utils/colors.ts b/site/src/utils/colors.ts
@@ -1,3 +1,65 @@
+/**
+ * Does not support shorthand hex strings (e.g., #fff), just to maximize
+ * compatibility with server, which also doesn't support shorthand
+ */
+const hexMatcher = /^#[0-9A-Fa-f]{6}$/;
+
+/**
+ * Determines whether a string is a hex color string. Returns false for
+ * shorthand hex strings.
+ *
+ * Mainly here to validate input before sending it to the server.
+ */
+export function isHexColor(input: string): boolean {
+  // Length check isn't necessary; it's just an fast way to validate before
+  // kicking things off to the slower regex check
+  return input.length === 7 && hexMatcher.test(input);
+}
+
+/**
+ * Regex written and tested via Regex101. This doesn't catch every invalid HSL
+ * string and still requires some other checks, but it can do a lot by itself.
+ *
+ * Setup:
+ * - Supports capture groups for all three numeric values. Regex tries to fail
+ *   the input as quickly as possible.
+ * - Regex is all-or-nothing – there is some tolerance for extra spaces, but
+ *   this regex will fail any string that is missing any part of the format.
+ * - String is case-insensitive
+ * - String must start with HSL and have both parentheses
+ * - All three numeric values must be comma-delimited
+ * - Hue can be 1-3 digits. Rightmost digit (if it exists) can only be 1-3;
+ *   other digits have no constraints. The degree unit ("deg") is optional
+ * - Both saturation and luminosity can be 1-3 digits. Rightmost digit (if it
+ *   exists) can only ever be 1. Other digits have no constraints.
+ */
+const hslMatcher =
+  /^hsl\(((?:[1-3]?\d)?\d)(?:deg)?, *((?:1?\d)?\d)%, *((?:1?\d)?\d)%\)$/i;
+
+export function isHslColor(input: string): boolean {
+  const [, hue, sat, lum] = hslMatcher.exec(input) ?? [];
+  if (hue === undefined || sat === undefined || lum === undefined) {
+    return false;
+  }
+
+  const hueN = Number(hue);
+  if (!Number.isInteger(hueN) || hueN < 0 || hueN > 359) {
+    return false;
+  }
+
+  const satN = Number(sat);
+  if (!Number.isInteger(satN) || satN < 0 || satN > 100) {
+    return false;
+  }
+
+  const lumN = Number(lum);
+  if (!Number.isInteger(lumN) || lumN < 0 || lumN > 100) {
+    return false;
+  }
+
+  return true;
+}
+
 // Used to convert our theme colors to Hex since monaco theme only support hex colors
 // From https://www.jameslmilner.com/posts/converting-rgb-hex-hsl-colors/
 export function hslToHex(hsl: string): string {
