Add some warnings for misusing SQL injection protection (#4)

This commit is contained in:
Bryan Terce 2018-05-09 15:35:47 -07:00
parent cef0c4c816
commit 39bb3b0b72
2 changed files with 54 additions and 9 deletions

View File

@ -34,10 +34,10 @@ import ch.njol.util.Kleenean;
/** /**
* Executes a statement on a database and optionally stores the result in a variable. Expressions * 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. * 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 * If a single variable, such as `{test}`, is passed, the variable will be set to the number of
* affected rows. * affected rows.
* * <p>
* If a list variable, such as `{test::*}`, is passed, the query result will be mapped to the list * 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>}` * variable in the form `{test::<column name>::<row number>}`
* *
@ -145,16 +145,41 @@ public class EffExecuteStatement extends Delay {
StringBuilder sb = new StringBuilder(); StringBuilder sb = new StringBuilder();
List<Object> parameters = new ArrayList<>(); List<Object> parameters = new ArrayList<>();
Object[] objects = SkriptUtil.getTemplateString(((VariableString) query)); 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) { if (o instanceof String) {
sb.append(o); sb.append(o);
} else { } else {
Expression<?> expr = SkriptUtil.getExpressionFromInfo(o); 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) { 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 { } else {
parameters.add(expr.getSingle(e)); parameters.add(expressionValue);
sb.append('?'); sb.append('?');
if (standaloneString) {
Skript.warning("Do not surround expressions with quotes!");
}
} }
} }
} }
@ -168,6 +193,20 @@ public class EffExecuteStatement extends Delay {
return stmt; 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) { private void setVariable(Event e, String name, Object obj) {
Variables.setVariable(name.toLowerCase(Locale.ENGLISH), obj, e, isLocal); Variables.setVariable(name.toLowerCase(Locale.ENGLISH), obj, e, isLocal);
} }

View File

@ -25,11 +25,16 @@ public class ExprUnsafe extends SimpleExpression<String> {
"unsafe %string%"); "unsafe %string%");
} }
private Expression<String> str; private Expression<String> stringExpression;
private String rawExpression;
public String getRawExpression() {
return rawExpression;
}
@Override @Override
protected String[] get(Event e) { protected String[] get(Event e) {
return str.getArray(e); return stringExpression.getArray(e);
} }
@Override @Override
@ -44,14 +49,15 @@ public class ExprUnsafe extends SimpleExpression<String> {
@Override @Override
public String toString(Event e, boolean debug) { public String toString(Event e, boolean debug) {
return "unsafe " + str.toString(e, debug); return "unsafe " + stringExpression.toString(e, debug);
} }
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
@Override @Override
public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed,
SkriptParser.ParseResult parseResult) { SkriptParser.ParseResult parseResult) {
str = (Expression<String>) exprs[0]; stringExpression = (Expression<String>) exprs[0];
rawExpression = parseResult.expr.substring("unsafe".length()).trim();
return true; return true;
} }
} }