Skip to content

Commit cdd0b3a

Browse files
authored
fix: return 400 for malformed JSON Patch pointers instead of 500 (#28316)
Client patches with paths missing the leading '/' (e.g., "displayName" instead of "/displayName") triggered jakarta.json.JsonException from JsonPointerImpl, which fell through the exception mapper and surfaced as an unhandled 500 (and Sentry alert) on PATCH endpoints such as ClassificationResource. - JsonUtils.applyPatch now validates each operation's 'path' and 'from' upfront, throwing IllegalArgumentException with a clear RFC 6901 message before the cryptic library exception fires. - CatalogGenericExceptionMapper maps jakarta.json.JsonException to 400 as defense in depth, covering other RFC 6902 violations (e.g., out-of-range array index, replace on missing path) that were also returning 500. - Added JsonUtilsTest cases for malformed 'path' and 'from' pointers.
1 parent 9921dc1 commit cdd0b3a

3 files changed

Lines changed: 67 additions & 0 deletions

File tree

openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogGenericExceptionMapper.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import static jakarta.ws.rs.core.Response.Status.UNAUTHORIZED;
2323

2424
import io.dropwizard.jersey.errors.ErrorMessage;
25+
import jakarta.json.JsonException;
2526
import jakarta.ws.rs.BadRequestException;
2627
import jakarta.ws.rs.Path;
2728
import jakarta.ws.rs.ProcessingException;
@@ -49,6 +50,8 @@ public Response toResponse(Throwable ex) {
4950
return getRuleViolationResponse(ex);
5051
} else if (ex instanceof BadRequestException || ex instanceof IllegalArgumentException) {
5152
return getResponse(BAD_REQUEST, ex.getMessage());
53+
} else if (ex instanceof JsonException) {
54+
return getResponse(BAD_REQUEST, ex.getMessage());
5255
} else if (ex instanceof ProcessingException) {
5356
return getResponse(BAD_REQUEST, "Invalid request parameter");
5457
} else if (ex instanceof UnableToExecuteStatementException) {

openmetadata-service/src/test/java/org/openmetadata/service/util/JsonUtilsTest.java

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,12 @@
2121
import com.fasterxml.jackson.core.type.TypeReference;
2222
import com.fasterxml.jackson.databind.JsonNode;
2323
import jakarta.json.Json;
24+
import jakarta.json.JsonArray;
2425
import jakarta.json.JsonArrayBuilder;
2526
import jakarta.json.JsonException;
2627
import jakarta.json.JsonObject;
2728
import jakarta.json.JsonObjectBuilder;
29+
import jakarta.json.JsonPatch;
2830
import jakarta.json.JsonPatchBuilder;
2931
import java.net.URI;
3032
import java.net.URISyntaxException;
@@ -117,6 +119,54 @@ void applyPatch() {
117119
assertTrue(jsonException.getMessage().contains("An array item index is out of range"));
118120
}
119121

122+
@Test
123+
void applyPatchRejectsMalformedJsonPointerPath() {
124+
JsonObjectBuilder teamJson = Json.createObjectBuilder();
125+
teamJson.add("id", UUID.randomUUID().toString()).add("name", "finance");
126+
Team original = JsonUtils.readValue(teamJson.build().toString(), Team.class);
127+
128+
JsonArray malformedPatch =
129+
Json.createArrayBuilder()
130+
.add(
131+
Json.createObjectBuilder()
132+
.add("op", "replace")
133+
.add("path", "displayName")
134+
.add("value", "Finance Team"))
135+
.build();
136+
JsonPatch patch = Json.createPatch(malformedPatch);
137+
138+
IllegalArgumentException ex =
139+
assertThrows(
140+
IllegalArgumentException.class,
141+
() -> JsonUtils.applyPatch(original, patch, Team.class));
142+
assertTrue(ex.getMessage().contains("displayName"));
143+
assertTrue(ex.getMessage().contains("must begin with '/'"));
144+
}
145+
146+
@Test
147+
void applyPatchRejectsMalformedFromPointer() {
148+
JsonObjectBuilder teamJson = Json.createObjectBuilder();
149+
teamJson.add("id", UUID.randomUUID().toString()).add("name", "finance");
150+
Team original = JsonUtils.readValue(teamJson.build().toString(), Team.class);
151+
152+
JsonArray malformedPatch =
153+
Json.createArrayBuilder()
154+
.add(
155+
Json.createObjectBuilder()
156+
.add("op", "move")
157+
.add("from", "name")
158+
.add("path", "/displayName"))
159+
.build();
160+
JsonPatch patch = Json.createPatch(malformedPatch);
161+
162+
IllegalArgumentException ex =
163+
assertThrows(
164+
IllegalArgumentException.class,
165+
() -> JsonUtils.applyPatch(original, patch, Team.class));
166+
assertTrue(ex.getMessage().contains("from"));
167+
assertTrue(ex.getMessage().contains("must begin with '/'"));
168+
}
169+
120170
@Test
121171
void testReadValuePassingTypeReference() {
122172
Map<String, String> expectedMap = Map.of("key1", "value1", "key2", "value2");

openmetadata-spec/src/main/java/org/openmetadata/schema/utils/JsonUtils.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,8 @@ public static JsonValue applyPatch(Object original, JsonPatch patch) {
335335
continue;
336336
}
337337

338+
validateJsonPointer(path, "path");
339+
338340
// Skip operations on read-only auto-generated fields
339341
if (isReadOnlyPatchPath(path)) {
340342
continue;
@@ -343,6 +345,9 @@ public static JsonValue applyPatch(Object original, JsonPatch patch) {
343345
// For copy/move operations, also check the 'from' field if present
344346
if (jsonObject.containsKey("from")) {
345347
String from = jsonObject.getString("from", null);
348+
if (from != null) {
349+
validateJsonPointer(from, "from");
350+
}
346351
if (isReadOnlyPatchPath(from)) {
347352
continue;
348353
}
@@ -366,6 +371,15 @@ public static JsonValue applyPatch(Object original, JsonPatch patch) {
366371
return currentJson;
367372
}
368373

374+
private static void validateJsonPointer(String pointer, String fieldName) {
375+
if (!pointer.isEmpty() && pointer.charAt(0) != '/') {
376+
throw new IllegalArgumentException(
377+
String.format(
378+
"Invalid JSON Patch '%s' value '%s' - non-empty JSON Pointer must begin with '/' (RFC 6901)",
379+
fieldName, pointer));
380+
}
381+
}
382+
369383
private static boolean isReadOnlyPatchPath(String path) {
370384
if (path == null || path.isBlank()) {
371385
return false;

0 commit comments

Comments
 (0)