DATAREST-1127 - Patch operations now always verify paths before opera…

…tion 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.

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 @@ protected <T> Object evaluateValueFromTarget(Object targetObject, Class<T> entit
 			return super.evaluateValueFromTarget(targetObject, entityType);
 		}
 
-		List<String> segments = new ArrayList<String>();
-
-		for (String segment : path.split("/")) {
-			if (!(segment.matches("\\d+") || segment.equals("-") || segment.isEmpty())) {
-				segments.add(segment);
-			}
-		}
-
-		PropertyPath propertyPath = PropertyPath.from(StringUtils.collectionToDelimitedString(segments, "."), entityType);
+		PropertyPath path = verifyPath(entityType);
 
-		return value instanceof LateObjectEvaluator ? ((LateObjectEvaluator) value).evaluate(propertyPath.getType())
-				: value;
+		return evaluate(path == null ? entityType : path.getType());
 	}
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java

@@ -17,14 +17,18 @@
 
 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 @@
  */
 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 @@ protected Object getValueFromTarget(Object target) {
 	 */
 	protected <T> Object evaluateValueFromTarget(Object targetObject, Class<T> entityType) {
 
-		return value instanceof LateObjectEvaluator
-				? ((LateObjectEvaluator) value).evaluate(spelExpression.getValueType(targetObject)) : value;
+		verifyPath(entityType);
+
+		return evaluate(spelExpression.getValueType(targetObject));
+	}
+
+	protected final <T> Object evaluate(Class<T> targetType) {
+		return value instanceof LateObjectEvaluator ? ((LateObjectEvaluator) value).evaluate(targetType) : value;
 	}
 
 	/**
@@ -219,4 +230,32 @@ private Integer targetListIndex(String path) {
 			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<String> segments = new ArrayList<String>();
+
+		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);
+		}
+	}
 }

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

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" }
+]