From ec74064ce77c0e12083614fd9e6eca5477fb881c Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 14 Feb 2023 20:22:36 +0900 Subject: [PATCH 1/6] Fix a bug where an JSON content is not validated Motivation: If an invalid JSON content is submitted to Central Dogma Server using REST API, the JSON is not validated and it is saved as is. The file content is formatted as a text so `node.get("content")` returns `TextNode` and it is regarded as a string although it is a JSON object. https://github.com/line/centraldogma/blob/9030505a928006d32f3897dc08823dac0d5fd6c2/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java#L91-L95 Modifications: - Pass a raw text value to `Change.ofJsonUpsert()` and make an invalidid JSON validated. Result: Central Dogma server now correctly rejects an invalid JSON content. --- .../converter/ChangesRequestConverter.java | 4 ++- .../internal/api/ContentServiceV1Test.java | 25 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java index f8095da53..41d5d0c13 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java @@ -89,7 +89,9 @@ private static Change readChange(JsonNode node) { return Change.ofTextUpsert(path, node.get("content").textValue()); } if (changeType == ChangeType.UPSERT_JSON) { - return Change.ofJsonUpsert(path, node.get("content")); + // ofJsonUpsert() parses the text value of the context as JSON. + // An invalid JSON will be rejected with JsonParseException. + return Change.ofJsonUpsert(path, node.get("content").textValue()); } if (changeType == ChangeType.REMOVE) { return Change.ofRemoval(path); diff --git a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java index 05a153141..3b762a3c6 100644 --- a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java +++ b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java @@ -32,6 +32,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.databind.JsonNode; import com.linecorp.armeria.client.WebClient; @@ -154,6 +155,30 @@ void pushFileToMetaRepositoryShouldFail() { assertThat(res.contentUtf8()).contains(InvalidPushException.class.getName()); } + @Test + void pushInvalidJson() { + final WebClient client = dogma.httpClient(); + + // An invalid JSON containing a trailing comma. + final String body = + '{' + + " \"path\" : \"/invalid.json\"," + + " \"type\" : \"UPSERT_JSON\"," + + " \"content\" : {\"trailing\": \"comma\", }," + + " \"commitMessage\" : {" + + " \"summary\" : \"Add invalid.json\"," + + " \"detail\": \"An invalid JSON must be rejected.\"," + + " \"markup\": \"PLAINTEXT\"" + + " }" + + '}'; + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/api/v1/projects/myPro/repos/myRepo/contents", + HttpHeaderNames.CONTENT_TYPE, MediaType.JSON); + final AggregatedHttpResponse res = client.execute(headers, body).aggregate().join(); + assertThat(res.status()).isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(res.contentUtf8()).contains(JsonParseException.class.getName()); + } + @Nested class FilesTest { From 2fe5aef90089e3ca6928cdb77403d6be3d4e1079 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 14 Feb 2023 21:24:27 +0900 Subject: [PATCH 2/6] Fix test --- .../server/internal/api/ContentServiceV1Test.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java index 3b762a3c6..3e416e44c 100644 --- a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java +++ b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java @@ -45,6 +45,7 @@ import com.linecorp.armeria.common.RequestHeaders; import com.linecorp.armeria.common.ResponseHeaders; import com.linecorp.centraldogma.common.ChangeConflictException; +import com.linecorp.centraldogma.common.ChangeFormatException; import com.linecorp.centraldogma.common.InvalidPushException; import com.linecorp.centraldogma.common.RedundantChangeException; import com.linecorp.centraldogma.internal.Jackson; @@ -164,7 +165,7 @@ void pushInvalidJson() { '{' + " \"path\" : \"/invalid.json\"," + " \"type\" : \"UPSERT_JSON\"," + - " \"content\" : {\"trailing\": \"comma\", }," + + " \"content\" : \"{\\\"trailing\\\": \\\"comma\\\", }\"," + " \"commitMessage\" : {" + " \"summary\" : \"Add invalid.json\"," + " \"detail\": \"An invalid JSON must be rejected.\"," + @@ -176,7 +177,7 @@ void pushInvalidJson() { HttpHeaderNames.CONTENT_TYPE, MediaType.JSON); final AggregatedHttpResponse res = client.execute(headers, body).aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.BAD_REQUEST); - assertThat(res.contentUtf8()).contains(JsonParseException.class.getName()); + assertThat(res.contentUtf8()).contains(ChangeFormatException.class.getName()); } @Nested From 15893eb300acc63893c4a2507ec865a0dfbe7293 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Wed, 15 Feb 2023 13:47:53 +0900 Subject: [PATCH 3/6] Fix test errors --- .../converter/ChangesRequestConverter.java | 20 ++++--- .../internal/api/ContentServiceV1Test.java | 60 ++++++++++++++++++- 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java index 41d5d0c13..554120099 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java @@ -80,30 +80,34 @@ private static Change readChange(JsonNode node) { checkArgument(node.get("path") != null && node.get("type") != null, "a change should have a path and a type"); final ChangeType changeType = ChangeType.parse(node.get("type").textValue()); + final JsonNode content = node.get("content"); if (changeType != ChangeType.REMOVE) { - checkArgument(node.get("content") != null, "a change should have a content."); + checkArgument(content != null, "a change should have a content."); } final String path = node.get("path").textValue(); if (changeType == ChangeType.UPSERT_TEXT) { - return Change.ofTextUpsert(path, node.get("content").textValue()); + return Change.ofTextUpsert(path, content.textValue()); } if (changeType == ChangeType.UPSERT_JSON) { - // ofJsonUpsert() parses the text value of the context as JSON. - // An invalid JSON will be rejected with JsonParseException. - return Change.ofJsonUpsert(path, node.get("content").textValue()); + if (content.isTextual()) { + // A content can be a serialized JSON so the text value needs to be parsed as JSON. + return Change.ofJsonUpsert(path, content.textValue()); + } else { + return Change.ofJsonUpsert(path, content); + } } if (changeType == ChangeType.REMOVE) { return Change.ofRemoval(path); } if (changeType == ChangeType.RENAME) { - return Change.ofRename(path, node.get("content").textValue()); + return Change.ofRename(path, content.textValue()); } if (changeType == ChangeType.APPLY_TEXT_PATCH) { - return Change.ofTextPatch(path, node.get("content").textValue()); + return Change.ofTextPatch(path, content.textValue()); } if (changeType == ChangeType.APPLY_JSON_PATCH) { - return Change.ofJsonPatch(path, node.get("content")); + return Change.ofJsonPatch(path, content); } // Should never reach here. diff --git a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java index 3e416e44c..356c9ec79 100644 --- a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java +++ b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java @@ -46,6 +46,7 @@ import com.linecorp.armeria.common.ResponseHeaders; import com.linecorp.centraldogma.common.ChangeConflictException; import com.linecorp.centraldogma.common.ChangeFormatException; +import com.linecorp.centraldogma.common.Entry; import com.linecorp.centraldogma.common.InvalidPushException; import com.linecorp.centraldogma.common.RedundantChangeException; import com.linecorp.centraldogma.internal.Jackson; @@ -157,7 +158,63 @@ void pushFileToMetaRepositoryShouldFail() { } @Test - void pushInvalidJson() { + void pushEmbeddedJsonString() throws JsonParseException { + final WebClient client = dogma.httpClient(); + + // An invalid JSON containing a trailing comma. + final String body = + '{' + + " \"path\" : \"/embedded-string.json\"," + + " \"type\" : \"UPSERT_JSON\"," + + " \"content\" : \"\\\"json string\\\"\"," + + " \"commitMessage\" : {" + + " \"summary\" : \"Add embedded string.json\"," + + " \"detail\": \"An embedded JSON string.\"," + + " \"markup\": \"PLAINTEXT\"" + + " }" + + '}'; + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/api/v1/projects/myPro/repos/myRepo/contents", + HttpHeaderNames.CONTENT_TYPE, MediaType.JSON); + final AggregatedHttpResponse res = client.execute(headers, body).aggregate().join(); + assertThat(res.status()).isEqualTo(HttpStatus.OK); + + final Entry entry = dogma.client().forRepo("myPro", "myRepo") + .file("/embedded-string.json") + .get().join(); + assertThat(entry.contentAsText()).isEqualTo("\"json string\""); + final JsonNode content = entry.contentAsJson(); + assertThat(content.isTextual()).isTrue(); + assertThat(content.asText()).isEqualTo("json string"); + } + + @Test + void pushUnquoteJsonString() throws JsonParseException { + final WebClient client = dogma.httpClient(); + + // An invalid JSON containing a trailing comma. + final String body = + '{' + + " \"path\" : \"/string.json\"," + + " \"type\" : \"UPSERT_JSON\"," + + " \"content\" : \"json string\"," + + " \"commitMessage\" : {" + + " \"summary\" : \"Add string.json\"," + + " \"detail\": \"An JSON string.\"," + + " \"markup\": \"PLAINTEXT\"" + + " }" + + '}'; + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/api/v1/projects/myPro/repos/myRepo/contents", + HttpHeaderNames.CONTENT_TYPE, MediaType.JSON); + final AggregatedHttpResponse res = client.execute(headers, body).aggregate().join(); + assertThat(res.status()).isEqualTo(HttpStatus.BAD_REQUEST); + // Rejected by ChangesRequestConverter + assertThat(res.contentUtf8()).contains(ChangeFormatException.class.getName()); + } + + @Test + void pushInvalidEmbeddedJson() { final WebClient client = dogma.httpClient(); // An invalid JSON containing a trailing comma. @@ -177,6 +234,7 @@ void pushInvalidJson() { HttpHeaderNames.CONTENT_TYPE, MediaType.JSON); final AggregatedHttpResponse res = client.execute(headers, body).aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.BAD_REQUEST); + // Rejected by ChangesRequestConverter assertThat(res.contentUtf8()).contains(ChangeFormatException.class.getName()); } From 2173a7e2a673c991679d6c460c3f368355cfec0e Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Wed, 8 Mar 2023 20:03:09 +0900 Subject: [PATCH 4/6] Update server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java --- .../centraldogma/server/internal/api/ContentServiceV1Test.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java index 356c9ec79..164f5c270 100644 --- a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java +++ b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java @@ -161,7 +161,6 @@ void pushFileToMetaRepositoryShouldFail() { void pushEmbeddedJsonString() throws JsonParseException { final WebClient client = dogma.httpClient(); - // An invalid JSON containing a trailing comma. final String body = '{' + " \"path\" : \"/embedded-string.json\"," + From 71d7817067d414629e6384b916e72858cb7692f6 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Wed, 8 Mar 2023 20:03:16 +0900 Subject: [PATCH 5/6] Update server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java --- .../centraldogma/server/internal/api/ContentServiceV1Test.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java index 164f5c270..b77af1b41 100644 --- a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java +++ b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java @@ -191,7 +191,6 @@ void pushEmbeddedJsonString() throws JsonParseException { void pushUnquoteJsonString() throws JsonParseException { final WebClient client = dogma.httpClient(); - // An invalid JSON containing a trailing comma. final String body = '{' + " \"path\" : \"/string.json\"," + From 0310e1e4b4b6d192a1e064a5cf3f682211e39aa6 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Fri, 10 Mar 2023 00:37:24 +0900 Subject: [PATCH 6/6] Update server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java Co-authored-by: minux --- .../server/internal/api/ContentServiceV1Test.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java index b77af1b41..e9ee4efe7 100644 --- a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java +++ b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java @@ -179,8 +179,8 @@ void pushEmbeddedJsonString() throws JsonParseException { assertThat(res.status()).isEqualTo(HttpStatus.OK); final Entry entry = dogma.client().forRepo("myPro", "myRepo") - .file("/embedded-string.json") - .get().join(); + .file("/embedded-string.json") + .get().join(); assertThat(entry.contentAsText()).isEqualTo("\"json string\""); final JsonNode content = entry.contentAsJson(); assertThat(content.isTextual()).isTrue();