Skip to content

Continue enhancing SQL protections #7707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
labkey-jeckels wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Continue enhancing SQL protections #7707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1420eac
Continue enhancing SQL protections
labkey-jeckels May 30, 2026
25f7ecb
Formatting
labkey-jeckels May 30, 2026
f8a1fae
Formatting
labkey-jeckels May 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions api/src/org/labkey/api/data/PropertyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,9 +488,9 @@ private void saveValue(String name, String value)
if (null == name)
return;

String sql = SCHEMA.getSqlDialect().execute(SCHEMA.getSchema(), "property_setValue", "?, ?, ?");

new SqlExecutor(SCHEMA.getSchema()).execute(sql, getSet(), name, _store.getSaveValue(this, value));
new SqlExecutor(SCHEMA.getSchema()).execute(
SCHEMA.getSqlDialect().execute(SCHEMA.getSchema(), "property_setValue",
new SQLFragment("?, ?, ?", getSet(), name, _store.getSaveValue(this, value))));
}

public void save()
Expand Down
111 changes: 91 additions & 20 deletions api/src/org/labkey/api/data/SQLFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.Strings;
import org.apache.logging.log4j.Logger;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.Assert;
Expand All @@ -31,6 +32,7 @@
import org.labkey.api.util.JdbcUtil;
import org.labkey.api.util.Pair;
import org.labkey.api.util.StringUtilsLabKey;
import org.labkey.api.util.logging.LogHelper;

import java.math.BigDecimal;
import java.math.BigInteger;
Expand Down Expand Up @@ -68,7 +70,9 @@
/// semicolons in appended text.
public class SQLFragment implements Appendable, CharSequence
{
public static final String FEATUREFLAG_DISABLE_STRICT_CHECKS = "sqlfragment-disable-strict-checks";
private static final Logger LOG = LogHelper.getLogger(SQLFragment.class, "SQL injection safety net diagnostics");

public static final String FEATUREFLAG_DISABLE_STRICT_CHECKS = "SQLFragmentDisableStrictChecks";

private String sql;
private StringBuilder sb = null;
Expand Down Expand Up @@ -135,11 +139,9 @@ public SQLFragment(CharSequence charseq, @Nullable List<?> params)
(StringUtils.countMatches(charseq, '\"') % 2) != 0 ||
StringUtils.contains(charseq, ';'))
{
if (!AppProps.getInstance().isOptionalFeatureEnabled(FEATUREFLAG_DISABLE_STRICT_CHECKS))
throw new IllegalArgumentException("SQLFragment.append(String) does not allow semicolons or unmatched quotes");
throw new IllegalArgumentException("SQLFragment.append(String) does not allow semicolons or unmatched quotes");
}

// allow statement separators
this.sql = charseq.toString();
if (null != params)
this.params = new ArrayList<>(params);
Expand Down Expand Up @@ -419,8 +421,7 @@ public SQLFragment append(CharSequence charseq)
(StringUtils.countMatches(charseq, '\"') % 2) != 0 ||
StringUtils.contains(charseq, ';'))
{
if (!AppProps.getInstance().isOptionalFeatureEnabled(FEATUREFLAG_DISABLE_STRICT_CHECKS))
throw new IllegalArgumentException("SQLFragment.append(String) does not allow semicolons or unmatched quotes");
throw new IllegalArgumentException("SQLFragment.append(String) does not allow semicolons or unmatched quotes");
}

getStringBuilder().append(charseq);
Expand Down Expand Up @@ -453,15 +454,36 @@ public SQLFragment appendIdentifier(CharSequence charseq)
}

boolean malformed;
if (identifier.length() >= 2 && identifier.startsWith("\"") && identifier.endsWith("\""))
boolean quoteWrapped = identifier.length() >= 2 && identifier.startsWith("\"") && identifier.endsWith("\"");
if (quoteWrapped)
malformed = (StringUtils.countMatches(identifier, '\"') % 2) != 0;
else if (identifier.length() >= 2 && identifier.startsWith("`") && identifier.endsWith("`"))
malformed = (StringUtils.countMatches(identifier, '`') % 2) != 0;
else
malformed = StringUtils.containsAny(identifier, "*/\\'\"`?;- \t\n");
if (malformed && !AppProps.getInstance().isOptionalFeatureEnabled(FEATUREFLAG_DISABLE_STRICT_CHECKS))
if (malformed)
throw new IllegalArgumentException("SQLFragment.appendIdentifier(String) value appears to be incorrectly formatted: " + identifier);

// Tighten pre-quoted path. For any input wrapped in "...", require every interior `"`
// to appear as `""` and forbid `.` (dotted refs must use appendDottedIdentifiers).
if (quoteWrapped)
{
String interior = identifier.substring(1, identifier.length() - 1);
// Strip valid escaped quotes (""). Any remaining `"` is an unbalanced/breakout quote.
String stripped = Strings.CS.replace(interior, "\"\"", "");
boolean strictMalformed = stripped.indexOf('"') >= 0 || stripped.indexOf('.') >= 0;
if (strictMalformed)
{
if (AppProps.getInstance().isOptionalFeatureEnabled(FEATUREFLAG_DISABLE_STRICT_CHECKS))
LOG.warn("appendIdentifier strict pre-quoted check would have rejected (flag-on, allowed): {}", identifier);
else
{
LOG.warn("appendIdentifier strict pre-quoted check rejected (flag-off): {}", identifier);
throw new IllegalArgumentException("SQLFragment.appendIdentifier(String) pre-quoted value has unescaped interior quote or dotted form: " + identifier);
}
}
}

getStringBuilder().append(charseq);
return this;
}
Expand Down Expand Up @@ -680,9 +702,10 @@ public SQLFragment appendValue(Enum<?> e)
if (null == e)
return appendNull();
String name = e.name();
// Enum.name() usually returns a simple string (a legal java identifier), this is a paranoia check.
if (name.contains("'"))
throw new IllegalStateException();
// Enum.name() returns a legal Java identifier per JLS, so none of these characters can appear
// in practice. Defense in depth: reject anything SQL-active rather than only the apostrophe.
if (StringUtils.containsAny(name, "'\"\\;\r\n"))
throw new IllegalStateException("Unexpected character in Enum.name(): " + name);
getStringBuilder().append("'").append(name).append("'");
return this;
}
Expand Down Expand Up @@ -774,6 +797,9 @@ public boolean appendComment(String comment, SqlDialect dialect)
boolean truncated = comment.length() > 1000;
if (truncated)
comment = StringUtilsLabKey.leftSurrogatePairFriendly(comment, 1000);
// Strip CR/LF so an embedded newline can't terminate the `--` comment and turn the rest
// of the payload into live SQL.
comment = comment.replace('\r', ' ').replace('\n', ' ');
sb.append(comment);
if (StringUtils.countMatches(comment, "'")%2==1)
sb.append("'");
Expand Down Expand Up @@ -879,8 +905,7 @@ public void insert(int index, String str)
(StringUtils.countMatches(str, '\"') % 2) != 0 ||
StringUtils.contains(str, ';'))
{
if (!AppProps.getInstance().isOptionalFeatureEnabled(FEATUREFLAG_DISABLE_STRICT_CHECKS))
throw new IllegalArgumentException("SQLFragment.insert(int,String) does not allow semicolons or unmatched quotes");
throw new IllegalArgumentException("SQLFragment.insert(int,String) does not allow semicolons or unmatched quotes");
}

getStringBuilder().insert(index, str);
Expand Down Expand Up @@ -915,7 +940,7 @@ public String getFilterText()
param = param.replaceAll("\\$", "\\\\\\$");
sql = sql.replaceFirst("\\?", param);
}
return sql.replaceAll("\"", "");
return sql.replace("\"", "");
}


Expand Down Expand Up @@ -1052,7 +1077,7 @@ public static SQLFragment prettyPrint(SQLFragment from)
if (t.startsWith("-- </"))
indent = Math.max(0,indent-1);

sb.append("\t".repeat(Math.max(0, indent)));
sb.repeat("\t", Math.max(0, indent));

sb.append(line);
sb.append('\n');
Expand Down Expand Up @@ -1284,13 +1309,11 @@ private void shouldFail(Runnable r)
try
{
r.run();
if (!AppProps.getInstance().isOptionalFeatureEnabled(FEATUREFLAG_DISABLE_STRICT_CHECKS))
fail("Expected IllegalArgumentException");
fail("Expected IllegalArgumentException");
}
catch (IllegalArgumentException e)
{
if (AppProps.getInstance().isOptionalFeatureEnabled(FEATUREFLAG_DISABLE_STRICT_CHECKS))
fail("Did not expect IllegalArgumentException");
// expected
}
}

Expand All @@ -1313,7 +1336,7 @@ public void testIllegalArgument()

String mysqlQuoteIdentifier(String id)
{
return "`" + id.replaceAll("`", "``") + "`";
return "`" + id.replace("`", "``") + "`";
}

@Test
Expand All @@ -1328,6 +1351,54 @@ public void testMysql()
shouldFail(() -> new SQLFragment().appendIdentifier("`"));
shouldFail(() -> new SQLFragment().appendIdentifier("`a`a`"));
}

@Test
public void testAppendCommentStripsNewlines()
{
// PR1-P3: an embedded newline in a comment payload must not terminate the `-- ` comment
// line and expose the trailing text as live SQL.
if (!dialect.supportsComments())
return;

SQLFragment sqlf = new SQLFragment();
sqlf.appendComment("hello\nDROP TABLE x;--", dialect);
String sql = sqlf.getSQL();

assertFalse("appendComment leaked an embedded newline into emitted SQL: " + sql, sql.contains("hello\nDROP"));
assertTrue("appendComment should keep the payload on one comment line: " + sql, sql.contains("hello DROP TABLE x;--"));

// CR is stripped too
SQLFragment sqlf2 = new SQLFragment();
sqlf2.appendComment("hi\rDROP TABLE x;--", dialect);
assertFalse("appendComment leaked an embedded CR: " + sqlf2.getSQL(), sqlf2.getSQL().contains("hi\rDROP"));
}

@Test
public void testAppendIdentifierPreQuotedDottedRejected()
{
// PR2-G1: "schema"."table" has an even quote count so the legacy check accepted it.
// The strict check rejects the embedded `.` so a single appendIdentifier call can't
// smuggle a cross-schema reference.
shouldFail(() -> new SQLFragment().appendIdentifier("\"schema\".\"table\""));
}

@Test
public void testAppendIdentifierPreQuotedInteriorQuoteRejected()
{
// PR2-G1: an interior `"` that isn't part of the standard `""` doubling is a breakout.
// Even quote count alone is insufficient.
shouldFail(() -> new SQLFragment().appendIdentifier("\"foo\"bar\"baz\""));
}

@Test
public void testAppendIdentifierPreQuotedValidCases()
{
// PR2-G1: legitimate pre-quoted identifiers still pass.
new SQLFragment().appendIdentifier("\"my_table\""); // simple quoted
new SQLFragment().appendIdentifier("\"with a space\""); // whitespace inside quotes is fine
new SQLFragment().appendIdentifier("\"foo\"\"bar\""); // embedded literal " via "" doubling
new SQLFragment().appendIdentifier("\"contains\"\"more\"\"doubles\"");
}
}

@Override
Expand Down
16 changes: 6 additions & 10 deletions api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,15 +292,11 @@ public boolean supportsComments()
}

@Override
public String execute(DbSchema schema, String procedureName, String parameters)
protected SQLFragment doExecute(SQLFragment qualifiedProcName, SQLFragment parameters)
{
return "SELECT " + schema.getName() + "." + procedureName + "(" + parameters + ")";
}

@Override
public SQLFragment execute(DbSchema schema, String procedureName, SQLFragment parameters)
{
SQLFragment select = new SQLFragment("SELECT " + schema.getName() + "." + procedureName + "(");
SQLFragment select = new SQLFragment("SELECT ");
select.append(qualifiedProcName);
select.append("(");
select.append(parameters);
select.append(")");
return select;
Expand Down Expand Up @@ -858,15 +854,15 @@ public Map<String, MetadataParameterInfo> getParametersFromDbMetadata(DbScope sc
}

@Override
public String buildProcedureCall(String procSchema, String procName, int paramCount, boolean hasReturn, boolean assignResult, DbScope procScope)
protected String doBuildProcedureCall(String qualifiedProcName, int paramCount, boolean hasReturn, boolean assignResult, DbScope procScope)
{
if (hasReturn || assignResult)
paramCount--; // this param isn't included in the argument list of the CALL statement
StringBuilder sb = new StringBuilder();
sb.append("{");
if (assignResult)
sb.append("? = ");
sb.append("CALL ").append(procSchema).append(".").append(procName).append("(");
sb.append("CALL ").append(qualifiedProcName).append("(");
String comma = "";
for (int i = 0; i < paramCount; i++)
{
Expand Down
16 changes: 2 additions & 14 deletions api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.labkey.api.data.ColumnInfo;
import org.labkey.api.data.DbSchema;
import org.labkey.api.data.DbScope;
import org.labkey.api.data.PropertyStorageSpec;
import org.labkey.api.data.SQLFragment;
Expand Down Expand Up @@ -102,12 +101,6 @@ public boolean supportsGroupConcat()
return false;
}

@Override
public boolean supportsSelectConcat()
{
return false;
}

@Override
public boolean canShowExecutionPlan(ExecutionPlanType type)
{
Expand Down Expand Up @@ -154,11 +147,6 @@ public String addReselect(SQLFragment sql, ColumnInfo column, @Nullable String p
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

@Override
public String execute(DbSchema schema, String procedureName, String parameters)
{
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

@NotNull
@Override
Expand Down Expand Up @@ -190,7 +178,7 @@ public SQLFragment limitRows(SQLFragment select, SQLFragment from, SQLFragment f
}

@Override
public SQLFragment execute(DbSchema schema, String procedureName, SQLFragment parameters)
protected SQLFragment doExecute(SQLFragment qualifiedProcName, SQLFragment parameters)
{
return null;
}
Expand Down Expand Up @@ -513,7 +501,7 @@ public Map<String, MetadataParameterInfo> getParametersFromDbMetadata(DbScope sc
}

@Override
public String buildProcedureCall(String procSchema, String procName, int paramCount, boolean hasReturn, boolean assignResult, DbScope procScope)
protected String doBuildProcedureCall(String qualifiedProcName, int paramCount, boolean hasReturn, boolean assignResult, DbScope procScope)
{
throw new UnsupportedOperationException();
}
Expand Down
Loading