Fix a bug where JSON content is not always validated by ikhoon · Pull Request #804 · line/centraldogma

ikhoon · 2023-02-14T11:30:36Z

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.

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java

Lines 91 to 95 in 9030505

    
           if (changeType == ChangeType.UPSERT_JSON) { 
        
               return Change.ofJsonUpsert(path, node.get("content")); 
        
           } 
        
           if (changeType == ChangeType.REMOVE) { 
        
               return Change.ofRemoval(path);

Modifications:

Pass a raw text value to Change.ofJsonUpsert() and make the JSON validated.

Result:

Central Dogma server now correctly rejects an invalid JSON content.

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.

ikhoon · 2023-02-14T12:26:07Z

/cc @rainy789 This will fix the bug where the invalid JSON content is pushed that we talked about before.

codecov · 2023-02-15T04:58:06Z

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.68%. Comparing base (975bdda) to head (52938e9).
Report is 238 commits behind head on main.

Files with missing lines	Patch %	Lines
...nternal/api/converter/ChangesRequestConverter.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #804      +/-   ##
============================================
+ Coverage     65.60%   65.68%   +0.08%     
- Complexity     3317     3323       +6     
============================================
  Files           355      355              
  Lines         13875    13878       +3     
  Branches       1498     1499       +1     
============================================
+ Hits           9102     9116      +14     
+ Misses         3918     3911       -7     
+ Partials        855      851       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

trustin

👍

server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java

…/api/ContentServiceV1Test.java

minwoox

Thanks! Left nits and a question. 😄

minwoox · 2023-03-09T14:32:53Z

server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java

+                '{' +
+                "   \"path\" : \"/string.json\"," +
+                "   \"type\" : \"UPSERT_JSON\"," +
+                "   \"content\" : \"json string\"," +


nit: for reviewers

Suggested change

" \"content\" : \"json string\"," +

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

minwoox · 2023-03-09T14:39:15Z

...in/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java

-            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());


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

"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.

server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java

…/api/ContentServiceV1Test.java Co-authored-by: minux <songmw725@gmail.com>

…date-json-content

ikhoon · 2023-04-10T09:28:59Z

This PR is still WIP. Changed the milestone to the next one.

ikhoon added the defect label Feb 14, 2023

ikhoon added this to the 0.60.2 milestone Feb 14, 2023

ikhoon requested review from jrhee17, minwoox and trustin as code owners February 14, 2023 11:30

Fix test

2fe5aef

Fix test errors

15893eb

trustin approved these changes Mar 8, 2023

View reviewed changes

ikhoon commented Mar 8, 2023

View reviewed changes

server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java Outdated Show resolved Hide resolved

server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java Outdated Show resolved Hide resolved

ikhoon added 2 commits March 8, 2023 20:03

Update server/src/test/java/com/linecorp/centraldogma/server/internal…

2173a7e

…/api/ContentServiceV1Test.java

Update server/src/test/java/com/linecorp/centraldogma/server/internal…

71d7817

…/api/ContentServiceV1Test.java

trustin changed the title ~~Fix a bug where an JSON content is not validated~~ Fix a bug where JSON content is not always validated Mar 8, 2023

minwoox reviewed Mar 9, 2023

View reviewed changes

ikhoon and others added 2 commits March 10, 2023 00:37

Update server/src/test/java/com/linecorp/centraldogma/server/internal…

0310e1e

…/api/ContentServiceV1Test.java Co-authored-by: minux <songmw725@gmail.com>

Merge branch 'main' of https://github.com/line/centraldogma into vali…

52938e9

…date-json-content

ikhoon marked this pull request as draft April 10, 2023 09:26

ikhoon modified the milestones: 0.61.0, 0.61.1 Apr 10, 2023

minwoox modified the milestones: 0.61.1, 0.61.2 May 22, 2023

ikhoon modified the milestones: 0.61.2, 0.61.3 Jun 16, 2023

jrhee17 modified the milestones: 0.61.3, 0.61.4, 0.61.5 Jun 21, 2023

minwoox modified the milestones: 0.61.5, 0.61.6 Jul 26, 2023

ikhoon removed this from the 0.62.0 milestone Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Fix a bug where JSON content is not always validated#804
ikhoon wants to merge 7 commits intoline:mainfrom
ikhoon:validate-json-content

ikhoon commented Feb 14, 2023 •

edited

Loading

Uh oh!

ikhoon commented Feb 14, 2023

Uh oh!

codecov bot commented Feb 15, 2023 •

edited

Loading

Uh oh!

trustin left a comment

Uh oh!

Uh oh!

Uh oh!

minwoox left a comment

Uh oh!

minwoox Mar 9, 2023

Uh oh!

minwoox Mar 9, 2023

Uh oh!

ikhoon Mar 9, 2023

Uh oh!

Uh oh!

ikhoon commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (changeType == ChangeType.UPSERT_JSON) {
	return Change.ofJsonUpsert(path, node.get("content"));
	}
	if (changeType == ChangeType.REMOVE) {
	return Change.ofRemoval(path);

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

Conversation

ikhoon commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikhoon commented Feb 14, 2023

Uh oh!

codecov bot commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

trustin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

minwoox Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

minwoox Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

ikhoon Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ikhoon commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ikhoon commented Feb 14, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading