From 8f269e28fe8038a6c60f31a1c36cfda04795ab45 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Fri, 8 Sep 2017 11:25:44 +0200 Subject: [PATCH] DATAREST-1127 - Patch operations now always verify paths before operation application. Previously the SpEL expresssion created from JSON Patch path expressions were executed without double checking whether these paths actually exist on the target object in the first place. This is now in place. --- .../rest/webmvc/json/patch/AddOperation.java | 16 +------ .../webmvc/json/patch/PatchOperation.java | 42 ++++++++++++++++++- .../webmvc/json/patch/JsonPatchTests.java | 5 +++ .../webmvc/json/patch/patch-invalid-path.json | 3 ++ 4 files changed, 49 insertions(+), 17 deletions(-) create mode 100644 spring-data-rest-webmvc/src/test/resources/org/springframework/data/rest/webmvc/json/patch/patch-invalid-path.json diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java index 36fa09a14..a303125b3 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java @@ -15,11 +15,6 @@ */ package org.springframework.data.rest.webmvc.json.patch; -import java.util.Arrays; -import java.util.stream.Collectors; - -import org.springframework.data.mapping.PropertyPath; - /** * Operation to add a new value to the given "path". Will throw a {@link PatchException} if the path is invalid or if * the given value is not assignable to the given path. @@ -59,15 +54,6 @@ protected Object evaluateValueFromTarget(Object targetObject, Class entit return super.evaluateValueFromTarget(targetObject, entityType); } - String pathSource = Arrays.stream(path.split("/"))// - .filter(it -> !it.matches("\\d")) // no digits - .filter(it -> !it.equals("-")) // no "last element"s - .filter(it -> !it.isEmpty()) // - .collect(Collectors.joining(".")); - - PropertyPath propertyPath = PropertyPath.from(pathSource, entityType); - - return value instanceof LateObjectEvaluator ? ((LateObjectEvaluator) value).evaluate(propertyPath.getType()) - : value; + return evaluate(verifyPath(entityType).> map(it -> it.getType()).orElse(entityType)); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java index dcf404e0e..811734faa 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java @@ -17,11 +17,16 @@ import static org.springframework.data.rest.webmvc.json.patch.PathToSpEL.*; +import java.util.Arrays; import java.util.Collection; import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; import org.springframework.core.CollectionFactory; import org.springframework.core.convert.TypeDescriptor; +import org.springframework.data.mapping.PropertyPath; +import org.springframework.data.mapping.PropertyReferenceException; import org.springframework.expression.Expression; import org.springframework.expression.ExpressionException; import org.springframework.expression.spel.SpelEvaluationException; @@ -35,6 +40,8 @@ */ public abstract class PatchOperation { + private static final String INVALID_PATH_REFERENCE = "Invalid path reference %s on type %s (from source %s)!"; + protected final String op; protected final String path; protected final Object value; @@ -191,8 +198,39 @@ protected Object getValueFromTarget(Object target) { */ protected Object evaluateValueFromTarget(Object targetObject, Class entityType) { - return value instanceof LateObjectEvaluator - ? ((LateObjectEvaluator) value).evaluate(spelExpression.getValueType(targetObject)) : value; + verifyPath(entityType); + + return evaluate(spelExpression.getValueType(targetObject)); + } + + protected final Object evaluate(Class type) { + return value instanceof LateObjectEvaluator ? ((LateObjectEvaluator) value).evaluate(type) : value; + } + + /** + * Verifies that the current path is available on the given type. + * + * @param type must not be {@literal null}. + * @return the {@link PropertyPath} representing the path. Empty if the path only consists of index lookups or append + * characters. + */ + protected final Optional verifyPath(Class type) { + + String pathSource = Arrays.stream(path.split("/"))// + .filter(it -> !it.matches("\\d")) // no digits + .filter(it -> !it.equals("-")) // no "last element"s + .filter(it -> !it.isEmpty()) // + .collect(Collectors.joining(".")); + + if (pathSource.isEmpty()) { + return Optional.empty(); + } + + try { + return Optional.of(PropertyPath.from(pathSource, type)); + } catch (PropertyReferenceException o_O) { + throw new PatchException(String.format(INVALID_PATH_REFERENCE, pathSource, type, path), o_O); + } } /** diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java index 3b178e69a..55d9e2af7 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java @@ -162,6 +162,11 @@ public void failureWithInvalidPatchContent() throws Exception { patch.apply(todo, Todo.class); } + @Test(expected = PatchException.class) // DATAREST-1127 + public void rejectsInvalidPaths() throws Exception { + readJsonPatch("patch-invalid-path.json").apply(new Todo(), Todo.class); + } + private Patch readJsonPatch(String jsonPatchFile) throws IOException, JsonParseException, JsonMappingException { ClassPathResource resource = new ClassPathResource(jsonPatchFile, getClass()); diff --git a/spring-data-rest-webmvc/src/test/resources/org/springframework/data/rest/webmvc/json/patch/patch-invalid-path.json b/spring-data-rest-webmvc/src/test/resources/org/springframework/data/rest/webmvc/json/patch/patch-invalid-path.json new file mode 100644 index 000000000..98c32f6e2 --- /dev/null +++ b/spring-data-rest-webmvc/src/test/resources/org/springframework/data/rest/webmvc/json/patch/patch-invalid-path.json @@ -0,0 +1,3 @@ +[ + {"op":"replace", "path":"/somethingInvalid", "value": "bar" } +]