From f339f9d4f201d25ab5a63a835ede39e794df2d42 Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Fri, 19 Apr 2024 11:23:14 +0530 Subject: [PATCH] chore: Page rename API client validation (#32788) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. The page names can accept any character except for `/`, backslash and `:`. This is the validation that the server does now. This PR fixes the regex that checks the page name to align with this. 2. When typing a character that's not allowed, we end up with a space at the beginning or end of the page name, that doesn't really make sense, and is confusing. Then we end up with trailing or leading spaces in the page name, making the page name display look misaligned. This PR will make it so that the disallowed characters just can't be typed, and just don't make a difference in the page name. 3. We're also adding server-side validation for the icon slug. [Slack conversation](https://theappsmith.slack.com/archives/C03RPDB936Z/p1713161688607509). /ok-to-test tags="@tag.Sanity" > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 21e8d3a0060b9b4721b49149f307354b073332d7 > Cypress dashboard url: Click here! ## Summary by CodeRabbit - **New Features** - Enhanced page name validation to improve naming consistency across the application. - Updated validation for page icons and custom slugs to allow more flexibility and ensure inputs meet new standards. - **Bug Fixes** - Fixed issues in page naming functions to prevent the use of certain special characters and limit the length, enhancing data integrity and user experience. - **Tests** - Added new tests to verify the handling of invalid and empty custom slugs, ensuring robustness in page management functionalities. --- .../AppSettings/PageSettings.tsx | 4 +- .../IDE/EditorPane/components/PageElement.tsx | 4 +- app/client/src/utils/helpers.tsx | 15 +---- .../appsmith/server/dtos/PageUpdateDTO.java | 4 +- .../controllers/PageControllerTest.java | 61 ++++++++++++++++++- 5 files changed, 69 insertions(+), 19 deletions(-) diff --git a/app/client/src/pages/Editor/AppSettingsPane/AppSettings/PageSettings.tsx b/app/client/src/pages/Editor/AppSettingsPane/AppSettings/PageSettings.tsx index f17c5bc473..cb1bfcad44 100644 --- a/app/client/src/pages/Editor/AppSettingsPane/AppSettings/PageSettings.tsx +++ b/app/client/src/pages/Editor/AppSettingsPane/AppSettings/PageSettings.tsx @@ -33,7 +33,7 @@ import TextLoaderIcon from "../Components/TextLoaderIcon"; import { filterAccentedAndSpecialCharacters, getUrlPreview } from "../Utils"; import type { AppState } from "@appsmith/reducers"; import { getUsedActionNames } from "selectors/actionSelectors"; -import { isNameValid, resolveAsSpaceChar } from "utils/helpers"; +import { isNameValid, toValidPageName } from "utils/helpers"; import { useFeatureFlag } from "utils/hooks/useFeatureFlag"; import { FEATURE_FLAG } from "@appsmith/entities/FeatureFlag"; import { getHasManagePagePermission } from "@appsmith/utils/BusinessFeatures/permissionPageHelpers"; @@ -169,7 +169,7 @@ function PageSettings(props: { page: Page }) { } setIsPageNameValid(isValid); - setPageName(resolveAsSpaceChar(value, 30)); + setPageName(toValidPageName(value)); }; const onPageSlugChange = (value: string) => { diff --git a/app/client/src/pages/Editor/IDE/EditorPane/components/PageElement.tsx b/app/client/src/pages/Editor/IDE/EditorPane/components/PageElement.tsx index b2c7e5bdaf..4e0bec6a99 100644 --- a/app/client/src/pages/Editor/IDE/EditorPane/components/PageElement.tsx +++ b/app/client/src/pages/Editor/IDE/EditorPane/components/PageElement.tsx @@ -18,7 +18,7 @@ import { import { getCurrentApplication } from "@appsmith/selectors/applicationSelectors"; import type { AppState } from "@appsmith/reducers"; import { StyledEntity } from "pages/Editor/Explorer/Common/components"; -import { resolveAsSpaceChar } from "utils/helpers"; +import { toValidPageName } from "utils/helpers"; import { updatePage } from "actions/pageActions"; import { useGetPageFocusUrl } from "pages/Editor/IDE/hooks"; import AnalyticsUtil from "utils/AnalyticsUtil"; @@ -113,7 +113,7 @@ const PageElement = ({ isDefaultExpanded={isCurrentPage} key={page.pageId} name={page.pageName} - onNameEdit={resolveAsSpaceChar} + onNameEdit={toValidPageName} ref={ref} searchKeyword={""} step={0} diff --git a/app/client/src/utils/helpers.tsx b/app/client/src/utils/helpers.tsx index b07139d724..9241e8b6fd 100644 --- a/app/client/src/utils/helpers.tsx +++ b/app/client/src/utils/helpers.tsx @@ -352,18 +352,9 @@ function getWidgetElementToScroll( } } -export const resolveAsSpaceChar = (value: string, limit?: number) => { - // ensures that all special characters are disallowed - // while allowing all utf-8 characters - const removeSpecialCharsRegex = - /`|\~|\!|\@|\#|\$|\%|\^|\&|\*|\(|\)|\+|\=|\[|\{|\]|\}|\||\\|\'|\<|\,|\.|\>|\?|\/|\""|\;|\:|\s/; - const duplicateSpaceRegex = /\s+/; - return value - .split(removeSpecialCharsRegex) - .join(" ") - .split(duplicateSpaceRegex) - .join(" ") - .slice(0, limit || 30); +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); }; 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 120aeac683..2b9c00d4c7 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 @@ -5,8 +5,8 @@ import jakarta.validation.constraints.Size; public record PageUpdateDTO( @Pattern(regexp = "[^/\\\\:]+", message = "/, \\, : not allowed in page names") @Size(max = 30) String name, - String icon, - @Pattern(regexp = "[-\\w]+") String customSlug) { + @Pattern(regexp = "[-a-z]+") @Size(max = 20) String icon, + @Pattern(regexp = "[-\\w]*") String customSlug) { public PageDTO toPageDTO() { final PageDTO page = new PageDTO(); 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 2ad066bc2a..365d84bedc 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 @@ -1,6 +1,7 @@ package com.appsmith.server.controllers; import com.appsmith.server.configurations.SecurityTestConfig; +import com.appsmith.server.dtos.PageDTO; import com.appsmith.server.newpages.base.NewPageService; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -14,10 +15,17 @@ import org.springframework.http.MediaType; import org.springframework.security.test.context.support.WithMockUser; import org.springframework.test.web.reactive.server.WebTestClient; import org.springframework.web.reactive.function.BodyInserters; +import reactor.core.publisher.Mono; import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @SpringBootTest @@ -41,7 +49,7 @@ public class PageControllerTest { @ValueSource( strings = { "../mal", "..\\mal", "/mal", "C:\\mal", "mal/", "/mal/", "\\mal", "mal\\", "\\mal\\", ":mal", ":mal/", - ":mal\\" + ":mal\\", "", }) @WithMockUser void invalidName(String name) { @@ -58,4 +66,55 @@ public class PageControllerTest { verifyNoInteractions(newPageService); } + + @ParameterizedTest + @ValueSource( + strings = { + "../mal", + "..\\mal", + "/mal", + "C:\\mal", + "mal/", + "/mal/", + "\\mal", + "mal\\", + "\\mal\\", + ":mal", + ":mal/", + ":mal\\", + "spaced content", + "newline\ncontent", + " untrimmed ", + }) + @WithMockUser + void invalidCustomSlug(String slug) { + client.put() + .uri("/api/v1/pages/abcdef") + .contentType(MediaType.APPLICATION_JSON) + .body(BodyInserters.fromValue(Map.of("customSlug", slug))) + .exchange() + .expectStatus() + .isBadRequest() + .expectBody() + .jsonPath("$.errorDisplay") + .value(s -> assertThat((String) s).contains("Validation Failure")); + + verifyNoInteractions(newPageService); + } + + @Test + @WithMockUser + void emptyCustomSlugShouldBeOkay() { + doReturn(Mono.just(new PageDTO())) + .when(newPageService) + .updatePageByDefaultPageIdAndBranch(anyString(), any(), anyString()); + + client.put() + .uri("/api/v1/pages/abcdef") + .contentType(MediaType.APPLICATION_JSON) + .body(BodyInserters.fromValue(Map.of("customSlug", ""))) + .exchange(); + + verify(newPageService, times(1)).updatePageByDefaultPageIdAndBranch(eq("abcdef"), any(), eq(null)); + } }