From f5bfe5358864de1a15566110de7ad7e5ffb48e99 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Fri, 8 Sep 2017 12:00:30 +0200 Subject: [PATCH] DATAREST-1127 - Patch operations now always verify paths before operation application. Previously, the SpEL expression 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 check is now in place. --- .../rest/webmvc/json/patch/AddOperation.java | 17 +------- .../webmvc/json/patch/PatchOperation.java | 43 ++++++++++++++++++- .../webmvc/json/patch/JsonPatchTests.java | 5 +++ .../webmvc/json/patch/patch-invalid-path.json | 3 ++ 4 files changed, 51 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 124941117..4238782b0 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,7 @@ */ package org.springframework.data.rest.webmvc.json.patch; -import java.util.ArrayList; -import java.util.List; - import org.springframework.data.mapping.PropertyPath; -import org.springframework.util.StringUtils; /** * Operation to add a new value to the given "path". Will throw a {@link PatchException} if the path is invalid or if @@ -60,17 +56,8 @@ class AddOperation extends PatchOperation { return super.evaluateValueFromTarget(targetObject, entityType); } - List segments = new ArrayList(); + PropertyPath path = verifyPath(entityType); - for (String segment : path.split("/")) { - if (!(segment.matches("\\d+") || segment.equals("-") || segment.isEmpty())) { - segments.add(segment); - } - } - - PropertyPath propertyPath = PropertyPath.from(StringUtils.collectionToDelimitedString(segments, "."), entityType); - - return value instanceof LateObjectEvaluator ? ((LateObjectEvaluator) value).evaluate(propertyPath.getType()) - : value; + return evaluate(path == null ? entityType : path.getType()); } } 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 aa35ea499..3d06c772d 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,14 +17,18 @@ package org.springframework.data.rest.webmvc.json.patch; import static org.springframework.data.rest.webmvc.json.patch.PathToSpEL.*; +import java.util.ArrayList; import java.util.Collection; import java.util.List; 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; +import org.springframework.util.StringUtils; /** * Abstract base class representing and providing support methods for patch operations. @@ -35,6 +39,8 @@ import org.springframework.expression.spel.SpelEvaluationException; */ 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; @@ -193,8 +199,13 @@ public abstract class PatchOperation { */ 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 targetType) { + return value instanceof LateObjectEvaluator ? ((LateObjectEvaluator) value).evaluate(targetType) : value; } /** @@ -219,4 +230,32 @@ public abstract class PatchOperation { return null; } } + + /** + * Verifies, that the path for the current operation is a valid property path on the given target type. + * + * @param type must not be {@literal null}. + */ + protected PropertyPath verifyPath(Class type) { + + List segments = new ArrayList(); + + for (String segment : path.split("/")) { + if (!(segment.matches("\\d+") || segment.equals("-") || segment.isEmpty())) { + segments.add(segment); + } + } + + if (segments.isEmpty()) { + return null; + } + + String pathSource = StringUtils.collectionToDelimitedString(segments, "."); + + try { + return 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 cf899a1e5..9dc15395a 100644 --- 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 @@ -171,6 +171,11 @@ public class JsonPatchTests { 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" } +]