From dcada4522d4e4979868190c0f42a56a84c93a89d Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Thu, 18 Apr 2024 16:14:07 +0530 Subject: [PATCH] fix: Input validations on page update API (#32692) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [Slack thread with details](https://theappsmith.slack.com/archives/C03RPDB936Z/p1713161688607509). Not including details here. Sanity passes on EE. /ok-to-test tags="@tag.Sanity" > [!WARNING] > Workflow run: > Commit: fb1724a0e93f21e25dd09f821b101438a8bee52f > Cypress dashboard url: Click here! > It seems like **no tests ran** 😔. We are not able to recognize it, please check workflow here. ## Summary by CodeRabbit - **New Features** - Enhanced page update capabilities with new fields for page name, icon, and custom slug. - **Improvements** - Improved text handling support for non-Latin characters and punctuation in page names. - **Tests** - Added test cases for new page creation scenarios and page updates. --- .../controllers/ce/PageControllerCE.java | 3 +- .../appsmith/server/dtos/PageUpdateDTO.java | 18 ++++++ .../appsmith/server/helpers/TextUtils.java | 17 +++--- .../newpages/base/NewPageServiceCE.java | 3 +- .../newpages/base/NewPageServiceCEImpl.java | 6 +- .../controllers/PageControllerTest.java | 61 +++++++++++++++++++ 6 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/PageControllerTest.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java index a16c31cb0b..a40f17d7b3 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java @@ -8,6 +8,7 @@ import com.appsmith.server.dtos.ApplicationPagesDTO; import com.appsmith.server.dtos.CRUDPageResourceDTO; import com.appsmith.server.dtos.CRUDPageResponseDTO; import com.appsmith.server.dtos.PageDTO; +import com.appsmith.server.dtos.PageUpdateDTO; import com.appsmith.server.dtos.ResponseDTO; import com.appsmith.server.newpages.base.NewPageService; import com.appsmith.server.services.ApplicationPageService; @@ -166,7 +167,7 @@ public class PageControllerCE { @PutMapping("/{defaultPageId}") public Mono> updatePage( @PathVariable String defaultPageId, - @RequestBody PageDTO resource, + @RequestBody @Valid PageUpdateDTO resource, @RequestHeader(name = FieldName.BRANCH_NAME, required = false) String branchName) { log.debug("Going to update page with id: {}, branchName: {}", defaultPageId, branchName); return newPageService 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 new file mode 100644 index 0000000000..120aeac683 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java @@ -0,0 +1,18 @@ +package com.appsmith.server.dtos; + +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, + String icon, + @Pattern(regexp = "[-\\w]+") String customSlug) { + + public PageDTO toPageDTO() { + final PageDTO page = new PageDTO(); + page.setName(name); + page.setIcon(icon); + page.setCustomSlug(customSlug); + return page; + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/TextUtils.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/TextUtils.java index ae8b5105f6..b749eb1ee1 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/TextUtils.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/TextUtils.java @@ -15,14 +15,14 @@ public class TextUtils { * NON_LATIN regex matches any letter that is not a ASCII i.e. A-Za-z0-9, `-` and `_` * It'll match the unicode letters. * */ - private static final Pattern NON_LATIN = Pattern.compile("[^\\w_-]"); + private static final Pattern NON_LATIN = Pattern.compile("[^\\w-]"); - /* - * The SEPARATORS pattern matches those characters which cane be replaced with `-` - * This includes Punctuation characters, `&`, space and not `-` itself. Details on `Punct` - * http://www.gnu.org/software/grep/manual/html_node/Character-Classes-and-Bracket-Expressions.html - * */ - private static final Pattern SEPARATORS = Pattern.compile("[\\s\\p{Punct}&&[^-]]"); + /** + * The SEPARATORS pattern matches those characters which will be replaced with `-`. This includes spaces and + * punctuation characters. + * Details on {@code Punct}. + */ + private static final Pattern SEPARATORS = Pattern.compile("[\\s\\p{Punct}]+"); /** * Creates URL safe text aka slug from the input text. It supports english locale only. @@ -36,7 +36,8 @@ public class TextUtils { if (inputText == null) { return ""; } - // remove all the separators with a `-` + + // Replace all spaces and punctuation with a `-`. String noseparators = SEPARATORS.matcher(inputText).replaceAll("-"); String normalized = Normalizer.normalize(noseparators, Normalizer.Form.NFD); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java index 5e8ab93fa5..9dd7ec150f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java @@ -6,6 +6,7 @@ import com.appsmith.server.domains.Layout; import com.appsmith.server.domains.NewPage; import com.appsmith.server.dtos.ApplicationPagesDTO; import com.appsmith.server.dtos.PageDTO; +import com.appsmith.server.dtos.PageUpdateDTO; import com.appsmith.server.services.CrudService; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -62,7 +63,7 @@ public interface NewPageServiceCE extends CrudService { Mono updatePage(String pageId, PageDTO page); - Mono updatePageByDefaultPageIdAndBranch(String defaultPageId, PageDTO page, String branchName); + Mono updatePageByDefaultPageIdAndBranch(String defaultPageId, PageUpdateDTO page, String branchName); Mono save(NewPage page); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java index 369b20a251..98006d9bea 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java @@ -13,6 +13,7 @@ import com.appsmith.server.domains.NewPage; import com.appsmith.server.dtos.ApplicationPagesDTO; import com.appsmith.server.dtos.PageDTO; import com.appsmith.server.dtos.PageNameIdDTO; +import com.appsmith.server.dtos.PageUpdateDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.ResponseUtils; @@ -527,10 +528,11 @@ public class NewPageServiceCEImpl extends BaseService updatePageByDefaultPageIdAndBranch(String defaultPageId, PageDTO page, String branchName) { + public Mono updatePageByDefaultPageIdAndBranch( + String defaultPageId, PageUpdateDTO page, String branchName) { return repository .findPageByBranchNameAndDefaultPageId(branchName, defaultPageId, pagePermission.getEditPermission()) - .flatMap(newPage -> updatePage(newPage.getId(), page)) + .flatMap(newPage -> updatePage(newPage.getId(), page.toPageDTO())) .map(responseUtils::updatePageDTOWithDefaultResources); } 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 new file mode 100644 index 0000000000..2ad066bc2a --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/PageControllerTest.java @@ -0,0 +1,61 @@ +package com.appsmith.server.controllers; + +import com.appsmith.server.configurations.SecurityTestConfig; +import com.appsmith.server.newpages.base.NewPageService; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.reactive.AutoConfigureWebTestClient; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.context.annotation.Import; +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 java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verifyNoInteractions; + +@SpringBootTest +@AutoConfigureWebTestClient +@Import({SecurityTestConfig.class}) +public class PageControllerTest { + + @Autowired + private WebTestClient client; + + @MockBean + private NewPageService newPageService; + + @Test + @WithMockUser + void noBody() { + client.put().uri("/api/v1/pages/abcdef").exchange().expectStatus().isBadRequest(); + } + + @ParameterizedTest + @ValueSource( + strings = { + "../mal", "..\\mal", "/mal", "C:\\mal", "mal/", "/mal/", "\\mal", "mal\\", "\\mal\\", ":mal", ":mal/", + ":mal\\" + }) + @WithMockUser + void invalidName(String name) { + client.put() + .uri("/api/v1/pages/abcdef") + .contentType(MediaType.APPLICATION_JSON) + .body(BodyInserters.fromValue(Map.of("name", name))) + .exchange() + .expectStatus() + .isBadRequest() + .expectBody() + .jsonPath("$.errorDisplay") + .value(s -> assertThat((String) s).contains("Validation Failure")); + + verifyNoInteractions(newPageService); + } +}