Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a bug where JSON content is not always validated #804

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Fix a bug where JSON content is not always validated #804

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec74064
Fix a bug where an JSON content is not validated
ikhoon Feb 14, 2023
2fe5aef
Fix test
ikhoon Feb 14, 2023
15893eb
Fix test errors
ikhoon Feb 15, 2023
2173a7e
Update server/src/test/java/com/linecorp/centraldogma/server/internal…
ikhoon Mar 8, 2023
71d7817
Update server/src/test/java/com/linecorp/centraldogma/server/internal…
ikhoon Mar 8, 2023
0310e1e
Update server/src/test/java/com/linecorp/centraldogma/server/internal…
ikhoon Mar 9, 2023
52938e9
Merge branch 'main' of https://github.com/line/centraldogma into vali…
jrhee17 Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions ...java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,28 +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) {
return Change.ofJsonUpsert(path, node.get("content"));
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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Can we apply this approach that checks if it's textual in the Change.ofJsonUpsert method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"string" is a valid JSON type. So I don't think we can generally apply it to Change.ofJsonUpsert().
This is a special case where a web application sends a stringfied JSON content after editing to the server.

} 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.
Expand Down
84 changes: 84 additions & 0 deletions server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -44,6 +45,8 @@
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.Entry;
import com.linecorp.centraldogma.common.InvalidPushException;
import com.linecorp.centraldogma.common.RedundantChangeException;
import com.linecorp.centraldogma.internal.Jackson;
Expand Down Expand Up @@ -154,6 +157,87 @@ void pushFileToMetaRepositoryShouldFail() {
assertThat(res.contentUtf8()).contains(InvalidPushException.class.getName());
}

@Test
void pushEmbeddedJsonString() throws JsonParseException {
final WebClient client = dogma.httpClient();

// An invalid JSON containing a trailing comma.
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
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();
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
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.
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
final String body =
'{' +
" \"path\" : \"/string.json\"," +
" \"type\" : \"UPSERT_JSON\"," +
" \"content\" : \"json string\"," +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for reviewers

Suggested change
" \"content\" : \"json string\"," +
" \"content\" : \"json string\"," + /* This string is not a JSON string but a plain text. */

" \"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.
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);
// Rejected by ChangesRequestConverter
assertThat(res.contentUtf8()).contains(ChangeFormatException.class.getName());
}

@Nested
class FilesTest {

Expand Down