From c5816014a63d477d462cb409f76fdc6c65756741 Mon Sep 17 00:00:00 2001 From: Tuan Dang Date: Tue, 10 Dec 2024 11:34:08 -0800 Subject: [PATCH] Add suggested PR review improvements, better validation on ssh cert template modal --- .../v1/ssh-certificate-template-router.ts | 65 +++++++------- .../src/services/project/project-service.ts | 4 +- .../components/SshCertificateModal.tsx | 1 - .../SshCertificateTemplateModal.tsx | 86 ++++++++++++++++--- .../SshCertificateTemplatesSection.tsx | 8 +- .../SshCertificateTemplatesTable.tsx | 7 +- 6 files changed, 119 insertions(+), 52 deletions(-) diff --git a/backend/src/ee/routes/v1/ssh-certificate-template-router.ts b/backend/src/ee/routes/v1/ssh-certificate-template-router.ts index 14e1e0dc7..f94b9d50a 100644 --- a/backend/src/ee/routes/v1/ssh-certificate-template-router.ts +++ b/backend/src/ee/routes/v1/ssh-certificate-template-router.ts @@ -61,36 +61,41 @@ export const registerSshCertificateTemplateRouter = async (server: FastifyZodPro rateLimit: writeLimit }, schema: { - body: z.object({ - sshCaId: z.string().describe(SSH_CERTIFICATE_TEMPLATES.CREATE.sshCaId), - name: z - .string() - .min(1) - .max(36) - .refine((v) => slugify(v) === v, { - message: "Name must be a valid slug" - }) - .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.name), - ttl: z - .string() - .refine((val) => ms(val) > 0, "TTL must be a positive number") - .default("1h") - .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.ttl), - maxTTL: z - .string() - .refine((val) => ms(val) > 0, "Max TTL must be a positive number") - .default("30d") - .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.maxTTL), - allowedUsers: z - .array(z.string().refine(isValidUserPattern, "Invalid user pattern")) - .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowedUsers), - allowedHosts: z - .array(z.string().refine(isValidHostPattern, "Invalid host pattern")) - .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowedHosts), - allowUserCertificates: z.boolean().describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowUserCertificates), - allowHostCertificates: z.boolean().describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowHostCertificates), - allowCustomKeyIds: z.boolean().describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowCustomKeyIds) - }), + body: z + .object({ + sshCaId: z.string().describe(SSH_CERTIFICATE_TEMPLATES.CREATE.sshCaId), + name: z + .string() + .min(1) + .max(36) + .refine((v) => slugify(v) === v, { + message: "Name must be a valid slug" + }) + .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.name), + ttl: z + .string() + .refine((val) => ms(val) > 0, "TTL must be a positive number") + .default("1h") + .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.ttl), + maxTTL: z + .string() + .refine((val) => ms(val) > 0, "Max TTL must be a positive number") + .default("30d") + .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.maxTTL), + allowedUsers: z + .array(z.string().refine(isValidUserPattern, "Invalid user pattern")) + .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowedUsers), + allowedHosts: z + .array(z.string().refine(isValidHostPattern, "Invalid host pattern")) + .describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowedHosts), + allowUserCertificates: z.boolean().describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowUserCertificates), + allowHostCertificates: z.boolean().describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowHostCertificates), + allowCustomKeyIds: z.boolean().describe(SSH_CERTIFICATE_TEMPLATES.CREATE.allowCustomKeyIds) + }) + .refine((data) => ms(data.maxTTL) > ms(data.ttl), { + message: "Max TLL must be greater than TTL", + path: ["maxTTL"] + }), response: { 200: sanitizedSshCertificateTemplate } diff --git a/backend/src/services/project/project-service.ts b/backend/src/services/project/project-service.ts index 49ee43eda..c2ba53430 100644 --- a/backend/src/services/project/project-service.ts +++ b/backend/src/services/project/project-service.ts @@ -905,7 +905,7 @@ export const projectServiceFactory = ({ }; /** - * Return list of SSH certificates for organization + * Return list of SSH certificates for project */ const listProjectSshCertificates = async ({ limit = 25, @@ -945,7 +945,7 @@ export const projectServiceFactory = ({ }; /** - * Return list of SSH certificate templates for organization + * Return list of SSH certificate templates for project */ const listProjectSshCertificateTemplates = async ({ actorId, diff --git a/frontend/src/views/Project/SshCaPage/components/SshCertificateModal.tsx b/frontend/src/views/Project/SshCaPage/components/SshCertificateModal.tsx index 9495ab141..5c027316a 100644 --- a/frontend/src/views/Project/SshCaPage/components/SshCertificateModal.tsx +++ b/frontend/src/views/Project/SshCaPage/components/SshCertificateModal.tsx @@ -75,7 +75,6 @@ export const SshCertificateModal = ({ popUp, handlePopUpToggle }: Props) => { const popUpData = popUp?.sshCertificate?.data as { sshCaId: string; - templateName: string; templateId: string; }; diff --git a/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplateModal.tsx b/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplateModal.tsx index 7bdcd319c..b193ae257 100644 --- a/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplateModal.tsx +++ b/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplateModal.tsx @@ -1,6 +1,8 @@ import { useEffect } from "react"; import { Controller, useForm } from "react-hook-form"; import { zodResolver } from "@hookform/resolvers/zod"; +import slugify from "@sindresorhus/slugify"; +import ms from "ms"; import { z } from "zod"; import { createNotification } from "@app/components/notifications"; @@ -24,17 +26,79 @@ import { } from "@app/hooks/api"; import { UsePopUpState } from "@app/hooks/usePopUp"; -const schema = z.object({ - sshCaId: z.string(), - name: z.string().min(1), - ttl: z.string().trim().min(1), - maxTTL: z.string().trim().min(1), - allowedUsers: z.string(), - allowedHosts: z.string(), - allowUserCertificates: z.boolean().optional().default(false), - allowHostCertificates: z.boolean().optional().default(false), - allowCustomKeyIds: z.boolean().optional().default(false) -}); +// Validates usernames or wildcard (*) +export const isValidUserPattern = (value: string): boolean => { + // Matches valid Linux usernames or a wildcard (*) + const userRegex = /^(?:\*|[a-z_][a-z0-9_-]{0,31})$/; + return userRegex.test(value); +}; + +// Validates hostnames, wildcard domains, or IP addresses +export const isValidHostPattern = (value: string): boolean => { + // Matches FQDNs, wildcard domains (*.example.com), IPv4, and IPv6 addresses + const hostRegex = + /^(?:\*|\*\.[a-z0-9-]+(?:\.[a-z0-9-]+)*|[a-z0-9-]+(?:\.[a-z0-9-]+)*|\d{1,3}(\.\d{1,3}){3}|([a-fA-F0-9:]+:+)+[a-fA-F0-9]+(?:%[a-zA-Z0-9]+)?)$/; + return hostRegex.test(value); +}; + +const schema = z + .object({ + sshCaId: z.string(), + name: z + .string() + .trim() + .toLowerCase() + .min(1) + .max(36) + .refine((v) => slugify(v) === v, { + message: "Invalid name. Name can only contain alphanumeric characters and hyphens." + }), + ttl: z + .string() + .trim() + .refine( + (val) => ms(val) > 0, + "TTL must be a valid time string such as 2 days, 1d, 2h 1y, ..." + ) + .default("1h"), + maxTTL: z + .string() + .trim() + .refine( + (val) => ms(val) > 0, + "Max TTL must be a valid time string such as 2 days, 1d, 2h 1y, ..." + ) + .default("30d"), + allowedUsers: z.string().refine( + (val) => { + const trimmed = val.trim(); + if (trimmed === "") return true; + const users = trimmed.split(",").map((u) => u.trim()); + return users.every(isValidUserPattern); + }, + { + message: "Invalid user pattern in allowedUsers" + } + ), + allowedHosts: z.string().refine( + (val) => { + const trimmed = val.trim(); + if (trimmed === "") return true; + const users = trimmed.split(",").map((u) => u.trim()); + return users.every(isValidHostPattern); + }, + { + message: "Invalid host pattern in allowedHosts" + } + ), + allowUserCertificates: z.boolean().optional().default(false), + allowHostCertificates: z.boolean().optional().default(false), + allowCustomKeyIds: z.boolean().optional().default(false) + }) + .refine((data) => ms(data.maxTTL) > ms(data.ttl), { + message: "Max TLL must be greater than TTL", + path: ["maxTTL"] + }); export type FormData = z.infer; diff --git a/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplatesSection.tsx b/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplatesSection.tsx index b6bdde96c..f3fa88c93 100644 --- a/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplatesSection.tsx +++ b/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplatesSection.tsx @@ -54,14 +54,14 @@ export const SshCertificateTemplatesSection = ({ caId }: Props) => { }; const onUpdateSshCaStatus = async ({ - certTemplateId, + templateId, status }: { - certTemplateId: string; + templateId: string; status: SshCertTemplateStatus; }) => { try { - await updateSshCertTemplate({ id: certTemplateId, status }); + await updateSshCertTemplate({ id: templateId, status }); await createNotification({ text: `Successfully ${ @@ -144,7 +144,7 @@ export const SshCertificateTemplatesSection = ({ caId }: Props) => { onDeleteApproved={() => onUpdateSshCaStatus( popUp?.sshCertificateTemplateStatus?.data as { - certTemplateId: string; + templateId: string; status: SshCertTemplateStatus; } ) diff --git a/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplatesTable.tsx b/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplatesTable.tsx index bc8e751fc..9a41340e9 100644 --- a/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplatesTable.tsx +++ b/frontend/src/views/Project/SshCaPage/components/SshCertificateTemplatesTable.tsx @@ -47,9 +47,8 @@ type Props = { id?: string; name?: string; sshCaId?: string; - certTemplateId?: string; status?: SshCertTemplateStatus; - templateName?: string; + templateId?: string; } ) => void; }; @@ -101,7 +100,7 @@ export const SshCertificateTemplatesTable = ({ handlePopUpOpen, sshCaId }: Props onClick={(e) => { e.stopPropagation(); handlePopUpOpen("sshCertificateTemplateStatus", { - certTemplateId: certificateTemplate.id, + templateId: certificateTemplate.id, status: certificateTemplate.status === SshCertTemplateStatus.ACTIVE ? SshCertTemplateStatus.DISABLED @@ -127,7 +126,7 @@ export const SshCertificateTemplatesTable = ({ handlePopUpOpen, sshCaId }: Props onClick={() => { handlePopUpOpen("sshCertificate", { sshCaId, - templateName: certificateTemplate.name + templateId: certificateTemplate.id }); }} icon={