From 7e003552713158885b8d289ba17676717e3765cd Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 27 Sep 2023 15:40:08 +0000 Subject: [PATCH 1/5] fix: remove hsl color from initial form state --- .../AppearanceSettingsPageView.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx b/site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx index 363672e63efeb..2aa975d27e180 100644 --- 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,6 +31,9 @@ export type AppearanceSettingsPageViewProps = { preview: boolean, ) => void; }; + +const fallbackBgColor = hslToHex(colors.blue[7]); + export const AppearanceSettingsPageView = ({ appearance, isEntitled, @@ -37,6 +41,7 @@ export const AppearanceSettingsPageView = ({ }: 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 ( <>
Date: Wed, 27 Sep 2023 15:40:29 +0000 Subject: [PATCH 2/5] chore: add extra color helpers --- site/src/utils/colors.ts | 53 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/site/src/utils/colors.ts b/site/src/utils/colors.ts index 822f32bce892d..b49abd82aaed3 100644 --- a/site/src/utils/colors.ts +++ b/site/src/utils/colors.ts @@ -1,3 +1,56 @@ +/** + * Does not support shorthand hex string (e.g., #fff) + */ +const hexMatcher = /^#[0-9A-Fa-f]{6}$/; + +export function isHexColor(input: string): boolean { + return 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 >= 360) { + return false; + } + + const satN = Number(sat); + if (!Number.isInteger(satN) || satN < 0 || satN > 100) { + return false; + } + + const lumN = Number(sat); + 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 { From 115813e6f25da2259cb76b5bdf1caa3446778c47 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 27 Sep 2023 15:41:16 +0000 Subject: [PATCH 3/5] chore: add extra runtime validation for colors --- .../Dashboard/DashboardProvider.tsx | 36 +++++++++++++++++-- .../AppearanceSettingsPage.tsx | 12 +++---- 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/site/src/components/Dashboard/DashboardProvider.tsx b/site/src/components/Dashboard/DashboardProvider.tsx index e8f6485777937..e1b54165e7128 100644 --- 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 = ({ children }) => { !entitlementsQuery.data || !appearanceQuery.data || !experimentsQuery.data; + const [configPreview, setConfigPreview] = useState(); + // 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 ; } @@ -59,7 +89,7 @@ export const DashboardProvider: FC = ({ 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 index 64ecdf4f887ca..61af33aed5a19 100644 --- 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, 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 = () => { ); From 9ed3c36404901edce99083ed5eefe842859c6402 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 27 Sep 2023 15:54:19 +0000 Subject: [PATCH 4/5] chore: clean up comments for clarity --- site/src/utils/colors.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/site/src/utils/colors.ts b/site/src/utils/colors.ts index b49abd82aaed3..e8b59baedde2c 100644 --- a/site/src/utils/colors.ts +++ b/site/src/utils/colors.ts @@ -1,8 +1,15 @@ /** - * Does not support shorthand hex string (e.g., #fff) + * 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 { return hexMatcher.test(input); } From f5fd404daf2ba14c0879eb8d6e46f8c96d709ac3 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 27 Sep 2023 19:21:25 +0000 Subject: [PATCH 5/5] chore: add tests for colors --- site/src/utils/colors.test.ts | 81 +++++++++++++++++++++++++++++++++++ site/src/utils/colors.ts | 8 ++-- 2 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 site/src/utils/colors.test.ts diff --git a/site/src/utils/colors.test.ts b/site/src/utils/colors.test.ts new file mode 100644 index 0000000000000..71074f590b5b0 --- /dev/null +++ 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 index e8b59baedde2c..2f0b3708abcaf 100644 --- a/site/src/utils/colors.ts +++ b/site/src/utils/colors.ts @@ -11,7 +11,9 @@ const hexMatcher = /^#[0-9A-Fa-f]{6}$/; * Mainly here to validate input before sending it to the server. */ export function isHexColor(input: string): boolean { - return hexMatcher.test(input); + // 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); } /** @@ -41,7 +43,7 @@ export function isHslColor(input: string): boolean { } const hueN = Number(hue); - if (!Number.isInteger(hueN) || hueN < 0 || hueN >= 360) { + if (!Number.isInteger(hueN) || hueN < 0 || hueN > 359) { return false; } @@ -50,7 +52,7 @@ export function isHslColor(input: string): boolean { return false; } - const lumN = Number(sat); + const lumN = Number(lum); if (!Number.isInteger(lumN) || lumN < 0 || lumN > 100) { return false; }