DATAJPA-965 - Fix potential blind SQL injection in Sort when used in …

…combination with @query. We now decline sort expressions that contain functions such as ORDER BY LENGTH(name) when used with repository having a String query defined via the @query annotation. Think of a query method as follows: @query("select p from Person p where LOWER(p.lastname) = LOWER(:lastname)") List<Person> findByLastname(@param("lastname") String lastname, Sort sort); Calls to findByLastname("lannister", new Sort("LENGTH(firstname)")) from now on throw an Exception indicating function calls are not allowed within the _ORDER BY_ clause. However you still can use JpaSort.unsafe("LENGTH(firstname)") to restore the behavior. Kudos to Niklas Särökaari, Joona Immonen, Arto Santala, Antti Virtanen, Michael Holopainen and Antti Ahola who brought this to our attention.
spring-projects · Sep 20, 2016 · b8e7fec · b8e7fec
1 parent c227c67
commit b8e7fec
Show file tree

Hide file tree

Showing 5 changed files with 491 additions and 6 deletions.
diff --git a/src/main/asciidoc/jpa.adoc b/src/main/asciidoc/jpa.adoc
@@ -303,6 +303,42 @@ public interface UserRepository extends JpaRepository<User, Long> {
 
 This also works with named native queries by adding the suffix `.count` to a copy of your query. Be aware that you probably must register a result set mapping for your count query, though.
 
+[[jpa.query-methods.sorting]]
+=== Using Sort
+
+Sorting can be done be either providing a `PageRequest` or using `Sort` directly. The properties actually used within the `Order` instances of `Sort` need to match to your domain model, which means they need to resolve to either a property or an alias used within the query. The JPQL defines this as a _state_field_path_expression_.
+
+[NOTE]
+====
+Using any non referenceable path expression leads to an Exception.
+====
+
+Using `Sort` together with <<jpa.query-methods.at-query, @Query>> however allows you to sneak in non path checked `Order` instances containing _functions_ within the `ORDER BY` clause. This is possible because the `Order` is just appended to the given query string. By default we will reject any `Order` instance containing function calls, but you can use `JpaSort.unsafe` to add potentially unsafe ordering.
+
+.Using Sort and JpaSort
+====
+[source, java]
+----
+public interface UserRepository extends JpaRepository<User, Long> {
+
+  @Query("select u from User u where u.lastname like ?1%")
+  List<User> findByAndSort(String lastname, Sort sort);
+
+  @Query("select u.id, LENGTH(u.firstname) as fn_len from User u where u.lastname like ?1%")
+  List<Object[]> findByAsArrayAndSort(String lastname, Sort sort);
+}
+
+repo.findByAndSort("lannister", new Sort("firstname"));               <1>
+repo.findByAndSort("stark", new Sort("LENGTH(firstname)"));           <2>
+repo.findByAndSort("targaryen", JpaSort.unsafe("LENGTH(firstname)")); <3>
+repo.findByAsArrayAndSort("bolton", new Sort("fn_len"));              <4>
+----
+<1> Valid `Sort` expression pointing to property in domain model.
+<2> Invalid `Sort` containing function call. Thows Exception.
+<3> Valid `Sort` containing explicitly _unsafe_ `Order`.
+<4> Valid `Sort` expression pointing to aliased function.
+====
+
 [[jpa.named-parameters]]
 === Using named parameters
 

diff --git a/src/main/java/org/springframework/data/jpa/domain/JpaSort.java b/src/main/java/org/springframework/data/jpa/domain/JpaSort.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2015 the original author or authors.
+ * Copyright 2013-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -32,6 +32,7 @@
  * 
  * @author Thomas Darimont
  * @author Oliver Gierke
+ * @author Christoph Strobl
  */
 public class JpaSort extends Sort {
 
@@ -83,6 +84,10 @@ private JpaSort(List<Order> orders, Direction direction, List<Path<?, ?>> paths)
 		super(combine(orders, direction, paths));
 	}
 
+	private JpaSort(List<Order> orders) {
+		super(orders);
+	}
+
 	/**
 	 * Returns a new {@link JpaSort} with the given sorting criteria added to the current one.
 	 * 
@@ -117,6 +122,30 @@ public JpaSort and(Direction direction, Path<?, ?>... paths) {
 		return new JpaSort(existing, direction, Arrays.asList(paths));
 	}
 
+	/**
+	 * Returns a new {@link JpaSort} with the given sorting criteria added to the current one.
+	 *
+	 * @param direction can be {@literal null}.
+	 * @param properties must not be {@literal null} or empty.
+	 * @return
+	 */
+	public JpaSort andUnsafe(Direction direction, String... properties) {
+
+		Assert.notEmpty(properties, "Properties must not be null!");
+
+		List<Order> orders = new ArrayList<Order>();
+
+		for (Order order : this) {
+			orders.add(order);
+		}
+
+		for (String property : properties) {
+			orders.add(new JpaOrder(direction, property));
+		}
+
+		return new JpaSort(orders, direction, Collections.<Path<?, ?>> emptyList());
+	}
+
 	/**
 	 * Turns the given {@link Attribute}s into {@link Path}s.
 	 * 
@@ -174,6 +203,51 @@ public static <A extends Attribute<T, S>, T, S> Path<T, S> path(A attribute) {
 		return new Path<T, S>(Arrays.asList(attribute));
 	}
 
+	/**
+	 * Creates new unsafe {@link JpaSort} based on given properties.
+	 *
+	 * @param properties must not be {@literal null} or empty.
+	 * @return
+	 */
+	public static JpaSort unsafe(String... properties) {
+		return unsafe(Sort.DEFAULT_DIRECTION, properties);
+	}
+
+	/**
+	 * Creates new unsafe {@link JpaSort} based on given {@link Direction} and properties.
+	 *
+	 * @param direction must not be {@literal null}.
+	 * @param properties must not be {@literal null} or empty.
+	 * @return
+	 */
+	public static JpaSort unsafe(Direction direction, String... properties) {
+
+		Assert.notNull(direction, "Direction must not be null!");
+		Assert.notEmpty(properties, "Properties must not be empty!");
+		Assert.noNullElements(properties, "Properties must not contain null values!");
+
+		return unsafe(direction, Arrays.asList(properties));
+	}
+
+	/**
+	 * Creates new unsafe {@link JpaSort} based on given {@link Direction} and properties.
+	 *
+	 * @param direction must not be {@literal null}.
+	 * @param properties must not be {@literal null} or empty.
+	 * @return
+	 */
+	public static JpaSort unsafe(Direction direction, List<String> properties) {
+
+		Assert.notEmpty(properties, "Properties must not be empty!");
+
+		List<Order> orders = new ArrayList<Order>();
+		for (String property : properties) {
+			orders.add(new JpaOrder(direction, property));
+		}
+
+		return new JpaSort(orders);
+	}
+
 	/**
 	 * Value object to abstract a collection of {@link Attribute}s.
 	 * 
@@ -233,4 +307,131 @@ public String toString() {
 			return builder.length() == 0 ? "" : builder.substring(0, builder.lastIndexOf("."));
 		}
 	}
+
+	/**
+	 * @author Christoph Strobl
+	 */
+	public static class JpaOrder extends Order {
+
+		private final boolean unsafe;
+		private final boolean ignoreCase;
+
+		/**
+		 * Creates a new {@link JpaOrder} instance. if order is {@literal null} then order defaults to
+		 * {@link Sort#DEFAULT_DIRECTION}
+		 *
+		 * @param direction can be {@literal null}, will default to {@link Sort#DEFAULT_DIRECTION}.
+		 * @param property must not be {@literal null}.
+		 */
+		private JpaOrder(Direction direction, String property) {
+			this(direction, property, NullHandling.NATIVE);
+		}
+
+		/**
+		 * Creates a new {@link Order} instance. if order is {@literal null} then order defaults to
+		 * {@link Sort#DEFAULT_DIRECTION}.
+		 *
+		 * @param direction can be {@literal null}, will default to {@link Sort#DEFAULT_DIRECTION}.
+		 * @param property must not be {@literal null}.
+		 * @param nullHandlingHint can be {@literal null}, will default to {@link NullHandling#NATIVE}.
+		 */
+		private JpaOrder(Direction direction, String property, NullHandling nullHandlingHint) {
+			this(direction, property, nullHandlingHint, false, true);
+		}
+
+		private JpaOrder(Direction direction, String property, NullHandling nullHandling, boolean ignoreCase,
+				boolean unsafe) {
+
+			super(direction, property, nullHandling);
+			this.ignoreCase = ignoreCase;
+			this.unsafe = unsafe;
+		}
+
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.domain.Sort.Order#with(org.springframework.data.domain.Sort.Direction)
+		 */
+		@Override
+		public JpaOrder with(Direction order) {
+			return new JpaOrder(order, getProperty(), getNullHandling(), isIgnoreCase(), this.unsafe);
+		}
+
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.domain.Sort.Order#with(org.springframework.data.domain.Sort.NullHandling)
+		 */
+		@Override
+		public JpaOrder with(NullHandling nullHandling) {
+			return new JpaOrder(getDirection(), getProperty(), nullHandling, isIgnoreCase(), this.unsafe);
+		}
+
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.domain.Sort.Order#nullsFirst()
+		 */
+		@Override
+		public JpaOrder nullsFirst() {
+			return with(NullHandling.NULLS_FIRST);
+		}
+
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.domain.Sort.Order#nullsLast()
+		 */
+		@Override
+		public JpaOrder nullsLast() {
+			return with(NullHandling.NULLS_LAST);
+		}
+
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.domain.Sort.Order#nullsNative()
+		 */
+		public JpaOrder nullsNative() {
+			return with(NullHandling.NATIVE);
+		}
+
+		/**
+		 * Creates new {@link Sort} with potentially unsafe {@link Order} instances.
+		 *
+		 * @param properties must not be {@literal null}.
+		 * @return
+		 */
+		public Sort withUnsafe(String... properties) {
+
+			Assert.notEmpty(properties, "Properties must not be empty!");
+			Assert.noNullElements(properties, "Properties must not contain null values!");
+
+			List<Order> orders = new ArrayList<Order>();
+			for (String property : properties) {
+				orders.add(new JpaOrder(getDirection(), property, getNullHandling(), isIgnoreCase(), this.unsafe));
+			}
+			return new Sort(orders);
+		}
+
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.domain.Sort.Order#ignoreCase()
+		 */
+		@Override
+		public JpaOrder ignoreCase() {
+			return new JpaOrder(getDirection(), getProperty(), getNullHandling(), true, this.unsafe);
+		}
+
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.domain.Sort.Order#isIgnoreCase()
+		 */
+		@Override
+		public boolean isIgnoreCase() {
+			return super.isIgnoreCase() || ignoreCase;
+		}
+
+		/**
+		 * @return true if {@link JpaOrder} created {@link #withUnsafe(String...)}.
+		 */
+		public boolean isUnsafe() {
+			return unsafe;
+		}
+	}
 }