From 71867f9bcd4bac1a33580a220f9b18ba3cdb3161 Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Mon, 22 Apr 2024 19:22:41 +0530 Subject: [PATCH] chore: Page name validation closer to file name validation (#32830) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Introducing a custom validation annotation, `@FileName` to be used on fields that are used as file/folder names as part of the product. 2. Updated the validation logic to only accept valid file names on Linux, macOS as well as on Windows. /ok-to-test tags="@tag.Settings, @tag.IDE" > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: a13f700d490845a9d3c53d0da9c1a763fde6be9b > Cypress dashboard url: Click here! ## Summary by CodeRabbit - **New Features** - Enhanced page name validation to include more special characters and reserved names, ensuring compatibility and preventing errors. - **Bug Fixes** - Updated regular expression for replacing disallowed characters in page names to improve system stability and user experience. - **Tests** - Expanded testing for invalid page names to cover a broader range of scenarios, enhancing reliability. --- app/client/src/utils/helpers.tsx | 2 +- .../appsmith/server/dtos/PageUpdateDTO.java | 3 +- .../server/meta/validations/FileName.java | 28 ++++++ .../meta/validations/FileNameValidator.java | 43 ++++++++ .../controllers/PageControllerTest.java | 99 ++++++++++++++++++- 5 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/FileName.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/FileNameValidator.java diff --git a/app/client/src/utils/helpers.tsx b/app/client/src/utils/helpers.tsx index 9241e8b6fd..419ef131b6 100644 --- a/app/client/src/utils/helpers.tsx +++ b/app/client/src/utils/helpers.tsx @@ -354,7 +354,7 @@ function getWidgetElementToScroll( export const toValidPageName = (value: string) => { // Ensure that `/`, `\` and `:` are not allowed in page names, aligning with server-side validation. - return value.replaceAll(/[/\\:]+/g, "").slice(0, 30); + return value.replaceAll(/[\\/:<>"|?*\x00-\x1f]+/g, "").slice(0, 30); }; export const PLATFORM_OS = { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java index 575ceecb74..45f089e1cf 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java @@ -1,10 +1,11 @@ package com.appsmith.server.dtos; +import com.appsmith.server.meta.validations.FileName; import jakarta.validation.constraints.Pattern; import jakarta.validation.constraints.Size; public record PageUpdateDTO( - @Pattern(regexp = "[^/\\\\:]+", message = "/, \\, : not allowed in page names") @Size(max = 30) String name, + @FileName(message = "Page names must be valid file names") @Size(max = 30) String name, @Pattern(regexp = "[-a-z]+") @Size(max = 20) String icon, @Pattern(regexp = "[-\\w]*") String customSlug, Boolean isHidden) { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/FileName.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/FileName.java new file mode 100644 index 0000000000..232c4ca3f7 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/FileName.java @@ -0,0 +1,28 @@ +package com.appsmith.server.meta.validations; + +import jakarta.validation.Constraint; +import jakarta.validation.Payload; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Validate field as a valid file name. Nulls are treated as valid by default, but can be changed with + * {@link FileName#isNullValid()}. + */ +@Documented +@Constraint(validatedBy = {FileNameValidator.class}) +@Target({ElementType.FIELD, ElementType.METHOD}) +@Retention(RetentionPolicy.RUNTIME) +public @interface FileName { + String message() default "Invalid file name"; + + Class[] groups() default {}; + + Class[] payload() default {}; + + boolean isNullValid() default true; +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/FileNameValidator.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/FileNameValidator.java new file mode 100644 index 0000000000..2262607af5 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/FileNameValidator.java @@ -0,0 +1,43 @@ +package com.appsmith.server.meta.validations; + +import jakarta.validation.ConstraintValidator; +import jakarta.validation.ConstraintValidatorContext; + +import java.util.Set; +import java.util.regex.Pattern; + +public class FileNameValidator implements ConstraintValidator { + + // Checks built to exclude characters listed in https://stackoverflow.com/a/31976060/151048. + private static final Pattern DISALLOWED_CHARS = Pattern.compile("[\\\\/:<>\"|?*\\x00-\\x1f]"); + + // These are not allowed on Windows, even with an extension. + private static final Set DISALLOWED_NAMES = Set.of( + "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "LPT1", + "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9"); + + private boolean isNullValid; + + @Override + public void initialize(FileName constraintAnnotation) { + ConstraintValidator.super.initialize(constraintAnnotation); + isNullValid = constraintAnnotation.isNullValid(); + } + + @Override + public boolean isValid(String value, ConstraintValidatorContext context) { + if (value == null) { + return isNullValid; + } + + if (value.isBlank() || value.startsWith(".") || value.endsWith(".") || value.length() > 255) { + return false; + } + + if (DISALLOWED_CHARS.matcher(value).find()) { + return false; + } + + return !DISALLOWED_NAMES.contains(value.split("\\.")[0].toUpperCase()); + } +} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/PageControllerTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/PageControllerTest.java index 365d84bedc..d502af963d 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/PageControllerTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/PageControllerTest.java @@ -48,8 +48,103 @@ public class PageControllerTest { @ParameterizedTest @ValueSource( strings = { - "../mal", "..\\mal", "/mal", "C:\\mal", "mal/", "/mal/", "\\mal", "mal\\", "\\mal\\", ":mal", ":mal/", - ":mal\\", "", + "../mal", + "..\\mal", + "/mal", + "C:\\mal", + "mal/", + "/mal/", + "\\mal", + "mal\\", + "\\mal\\", + ":mal", + ":mal/", + ":mal\\", + "", + " ", + "\t", + "<", + ">", + "\"", + "|", + "?", + "*", + "\0", + "\1", + "\2", + "\3", + "\4", + "\5", + "\6", + "\7", + "\10", + "\11", + "\12", + "\13", + "\14", + "\15", + "\16", + "\17", + "\20", + "\21", + "\22", + "\23", + "\24", + "\25", + "\26", + "\27", + "\30", + "\31", + "\32", + "\33", + "\34", + "\35", + "\36", + "\37", + "CON", + "PRN", + "AUX", + "NUL", + "COM1", + "COM2", + "COM3", + "COM4", + "COM5", + "COM6", + "COM7", + "COM8", + "COM9", + "LPT1", + "LPT2", + "LPT3", + "LPT4", + "LPT5", + "LPT6", + "LPT7", + "LPT8", + "LPT9", + "CON.txt", + "PRN.txt", + "AUX.txt", + "NUL.txt", + "COM1.txt", + "COM2.txt", + "COM3.txt", + "COM4.txt", + "COM5.txt", + "COM6.txt", + "COM7.txt", + "COM8.txt", + "COM9.txt", + "LPT1.txt", + "LPT2.txt", + "LPT3.txt", + "LPT4.txt", + "LPT5.txt", + "LPT6.txt", + "LPT7.txt", + "LPT8.txt", + "LPT9.txt", }) @WithMockUser void invalidName(String name) {