centraldogma icon indicating copy to clipboard operation
centraldogma copied to clipboard
verified publisher icon line

Disallow trailing tokens in JSON files

Open ikhoon opened this issue 2 months ago • 1 comments

Motivation:

We observed an unexpected deserialization outcome with malformed JSON. Jackson does not fail on that data but silently drops part of the data.

For example, we expected Jackson to fail to parse the data.

{
  "a" : {},
  "b" : {},
  "c" : {
    },
    "c1" : true
  }  // a malformed tailing JSON token
  "d" : {},
  "e": {}
}

But Jackson just drops d and e fields and parses the JSON into:

{
  "a":{},
  "b":{},
  "c":{},
  "c1":true
}

The maintainer of Jackson said it is an expected behavior but not a bug. https://github.com/FasterXML/jackson-core/issues/808#issuecomment-1211057640

But this is intended behavior, not a bug: Jackson only reads as much content as it needs for a valid JSON Value. It does not try to consume everything there might be available, given that the white-space-separated content streams are commonly used.

He suggested enabling DeserializationFeature.FAIL_ON_TRAILING_TOKENS to fail on the malformed data added after the end of the top-level JSON.

Modifications:

  • Enable DeserializationFeature.FAIL_ON_TRAILING_TOKENS to the default Jackson ObjectMapper in Central Dogma

Result:

Central Dogma now correctly rejects malformed JSON when parsing JSON data.

ikhoon avatar Oct 31 '25 02:10 ikhoon

Walkthrough

The PR enables Jackson's FAIL_ON_TRAILING_TOKENS deserialization feature on both compact and pretty mappers to enforce stricter JSON parsing. Supporting test cases verify the new behavior for invalid JSON with trailing content, and existing test JSON patches are corrected to contain valid syntax.

Changes

Cohort / File(s) Summary
Jackson Configuration
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java		 Adds import of DeserializationFeature and enables FAIL_ON_TRAILING_TOKENS on both compactMapper and prettyMapper to enforce stricter JSON deserialization behavior.
Test Coverage for Invalid JSON
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java
server/src/test/resources/invalid-content-after-trailing-comma.json		 New test class verifying that invalid JSON with trailing content triggers an IOError with MismatchedInputException cause when parsed via Jackson. Test resource file contains intentionally malformed JSON to exercise the stricter parser.
Test Corrections
server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java		 Fixes JSON patch bodies in test methods addUpdateAndRemoveProjectMember and addUpdateAndRemoveProjectToken by removing extra closing braces from role assignment payloads.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review Jackson feature enablement for correctness and compatibility
  • Verify test assertions align with new deserialization behavior
  • Confirm JSON patch corrections are syntactically valid

Possibly related PRs

  • line/centraldogma#1205: Modifies RevisionJsonDeserializer to stop using readValueAsTree(), a direct follow-up to handle implications of FAIL_ON_TRAILING_TOKENS strictness.

Suggested labels

breaking change

Suggested reviewers

  • trustin
  • jrhee17

Poem

🐰 JSON tokens trailing merrily no more! With stricter rules, we guard the parser's door, Extra braces trimmed, payloads cleaned, Now Jackson speaks as loudly as it's dreamed. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: enabling FAIL_ON_TRAILING_TOKENS to disallow trailing tokens in JSON deserialization.
Description check ✅ Passed The description clearly explains the motivation (silent data loss with malformed JSON), provides concrete examples, cites the Jackson maintainer's recommendation, and outlines the modification and expected result.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4bb95145378b729164cae2287866006ff7f09dd5 and f20aa9762fc1ee7de6255a4ce8851dc530f56808.

📒 Files selected for processing (1)
  • server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/jsonpatch/JsonPatch.java (1)
  • JsonPatch (125-384)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: build-ubuntu-latest-jdk-25-snapshot
  • GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
  • GitHub Check: build-ubuntu-latest-jdk-17-coverage
  • GitHub Check: build-ubuntu-latest-jdk-21
  • GitHub Check: build-macos-latest-jdk-25
  • GitHub Check: build-windows-latest-jdk-25
  • GitHub Check: build-ubuntu-latest-jdk-11
  • GitHub Check: lint
  • GitHub Check: flaky-tests
  • GitHub Check: docker-build
🔇 Additional comments (2)
server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java (2)

129-130: LGTM! JSON corrected to remove trailing tokens.

The JSON strings are now properly formed without extra closing braces. This fix is necessary since the PR enables FAIL_ON_TRAILING_TOKENS in Jackson, which would reject the previously malformed JSON.

165-166: LGTM! Consistent fix applied.

The JSON strings are correctly formed, matching the fix applied in addUpdateAndRemoveProjectMember. Both test methods now use valid JSON without trailing tokens.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 31 '25 02:10 coderabbitai[bot]

I found that DeserializationFeature.FAIL_ON_TRAILING_TOKENS throws an exception on valid JSON if @JsonDeserialize is applied to an element of a list. https://github.com/line/centraldogma/blob/3f7cac9e4543fd504b91da2d1734d3e4a49c98e3/server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaConfig.java#L278-L280

Trailing token (of type END_ARRAY) found after value (bound as `com.fasterxml.jackson.databind.JsonNode`): not allowed as per `DeserializationFeature.FAIL_ON_TRAILING_TOKENS`
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 15, column: 3] (through reference chain: com.linecorp.centraldogma.server.CentralDogmaConfig["ports"]->java.util.ArrayList[0])
com.fasterxml.jackson.databind.exc.MismatchedInputException: Trailing token (of type END_ARRAY) found after value (bound as `com.fasterxml.jackson.databind.JsonNode`): not allowed as per `DeserializationFeature.FAIL_ON_TRAILING_TOKENS`
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 15, column: 3] (through reference chain: com.linecorp.centraldogma.server.CentralDogmaConfig["ports"]->java.util.ArrayList[0])

Although it could be worked around for that case, I wasn't sure if enabling DeserializationFeature.FAIL_ON_TRAILING_TOKENS was a good idea given its false-positive behavior.

If there is consensus that applying a workaround and changing the internal deserializers is preferable, I'm happy to follow the direction. Otherwise, I will close this PR.

ikhoon avatar Dec 12 '25 02:12 ikhoon