From 39bb3b0b721fe43ac8ea5fc7820ca7939e7bdb9f Mon Sep 17 00:00:00 2001
From: Bryan Terce <bryan.terce@gmail.com>
Date: Wed, 9 May 2018 15:35:47 -0700
Subject: [PATCH] Add some warnings for misusing SQL injection protection (#4)

---
 .../skriptdb/skript/EffExecuteStatement.java  | 49 +++++++++++++++++--
 .../com/btk5h/skriptdb/skript/ExprUnsafe.java | 14 ++++--
 2 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/src/main/java/com/btk5h/skriptdb/skript/EffExecuteStatement.java b/src/main/java/com/btk5h/skriptdb/skript/EffExecuteStatement.java
index f3afe39..5db4c7f 100644
--- a/src/main/java/com/btk5h/skriptdb/skript/EffExecuteStatement.java
+++ b/src/main/java/com/btk5h/skriptdb/skript/EffExecuteStatement.java
@@ -34,10 +34,10 @@ import ch.njol.util.Kleenean;
 /**
  * Executes a statement on a database and optionally stores the result in a variable. Expressions
  * embedded in the query will be escaped to avoid SQL injection.
- *
+ * <p>
  * If a single variable, such as `{test}`, is passed, the variable will be set to the number of
  * affected rows.
- *
+ * <p>
  * If a list variable, such as `{test::*}`, is passed, the query result will be mapped to the list
  * variable in the form `{test::<column name>::<row number>}`
  *
@@ -145,16 +145,41 @@ public class EffExecuteStatement extends Delay {
     StringBuilder sb = new StringBuilder();
     List<Object> parameters = new ArrayList<>();
     Object[] objects = SkriptUtil.getTemplateString(((VariableString) query));
-    for (Object o : objects) {
+    for (int i = 0; i < objects.length; i++) {
+      Object o = objects[i];
       if (o instanceof String) {
         sb.append(o);
       } else {
         Expression<?> expr = SkriptUtil.getExpressionFromInfo(o);
+
+        String before = getString(objects, i - 1);
+        String after = getString(objects, i + 1);
+        boolean standaloneString = false;
+
+        if (before != null && after != null) {
+          if (before.endsWith("'") && after.endsWith("'")) {
+            standaloneString = true;
+          }
+        }
+
+        Object expressionValue = expr.getSingle(e);
+
         if (expr instanceof ExprUnsafe) {
-          sb.append(expr.getSingle(e));
+          sb.append(expressionValue);
+
+          if (standaloneString && expressionValue instanceof String) {
+            String rawExpression = ((ExprUnsafe) expr).getRawExpression();
+            Skript.warning(
+                String.format("Unsafe may have been used unnecessarily. Try replacing 'unsafe %1$s' with %1$s",
+                    rawExpression));
+          }
         } else {
-          parameters.add(expr.getSingle(e));
+          parameters.add(expressionValue);
           sb.append('?');
+
+          if (standaloneString) {
+            Skript.warning("Do not surround expressions with quotes!");
+          }
         }
       }
     }
@@ -168,6 +193,20 @@ public class EffExecuteStatement extends Delay {
     return stmt;
   }
 
+  private String getString(Object[] objects, int index) {
+    if (index < 0 || index >= objects.length) {
+      return null;
+    }
+
+    Object object = objects[index];
+
+    if (object instanceof String) {
+      return (String) object;
+    }
+
+    return null;
+  }
+
   private void setVariable(Event e, String name, Object obj) {
     Variables.setVariable(name.toLowerCase(Locale.ENGLISH), obj, e, isLocal);
   }
diff --git a/src/main/java/com/btk5h/skriptdb/skript/ExprUnsafe.java b/src/main/java/com/btk5h/skriptdb/skript/ExprUnsafe.java
index 53984f4..b557d79 100644
--- a/src/main/java/com/btk5h/skriptdb/skript/ExprUnsafe.java
+++ b/src/main/java/com/btk5h/skriptdb/skript/ExprUnsafe.java
@@ -25,11 +25,16 @@ public class ExprUnsafe extends SimpleExpression<String> {
         "unsafe %string%");
   }
 
-  private Expression<String> str;
+  private Expression<String> stringExpression;
+  private String rawExpression;
+
+  public String getRawExpression() {
+    return rawExpression;
+  }
 
   @Override
   protected String[] get(Event e) {
-    return str.getArray(e);
+    return stringExpression.getArray(e);
   }
 
   @Override
@@ -44,14 +49,15 @@ public class ExprUnsafe extends SimpleExpression<String> {
 
   @Override
   public String toString(Event e, boolean debug) {
-    return "unsafe " + str.toString(e, debug);
+    return "unsafe " + stringExpression.toString(e, debug);
   }
 
   @SuppressWarnings("unchecked")
   @Override
   public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed,
                       SkriptParser.ParseResult parseResult) {
-    str = (Expression<String>) exprs[0];
+    stringExpression = (Expression<String>) exprs[0];
+    rawExpression = parseResult.expr.substring("unsafe".length()).trim();
     return true;
   }
 }