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 5 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
5 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
0d6e595
Back off the check a bit
labkey-jeckels Jun 1, 2026
c9f8996
Back off the check a bit
labkey-jeckels Jun 1, 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
168 changes: 147 additions & 21 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,19 +454,72 @@ 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);

// A quote-wrapped value must be a well-formed quoted identifier, or dotted sequence of them.
if (quoteWrapped && !isQuotedIdentifierSequence(identifier))
{
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 is not a well-formed quoted identifier (or dotted sequence of them): " + identifier);
}
}

getStringBuilder().append(charseq);
return this;
}

// True iff the value is one or more double-quote-delimited identifiers joined by single dots,
// e.g. "a", "a"."b", "schema"."table". A literal quote within a segment must be escaped as "".
// A '.' inside quotes is part of the name; a '.' between segments is a separator. Anything else
// outside the quotes -- stray text, a lone (breakout) quote, an unterminated segment -- is rejected.
private static boolean isQuotedIdentifierSequence(String s)
{
int i = 0;
int n = s.length();
while (i < n)
{
if (s.charAt(i) != '"') // each segment must open with a quote
return false;
i++;
boolean closed = false;
while (i < n)
{
if (s.charAt(i) == '"')
{
if (i + 1 < n && s.charAt(i + 1) == '"') // escaped "" -- part of the name
{
i += 2;
continue;
}
i++; // closing quote
closed = true;
break;
}
i++; // any other char is legal inside quotes
}
if (!closed) // ran off the end without a closing quote
return false;
if (i == n) // end of a valid final segment
return true;
if (s.charAt(i) != '.') // segments must be separated by a single dot
return false;
i++; // consume the separator and require another segment
}
return false; // trailing dot with no following segment
}

// just to save some typing
public SQLFragment appendDottedIdentifiers(CharSequence table, DatabaseIdentifier col)
{
Expand Down Expand Up @@ -680,9 +734,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 +829,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 +937,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 +972,7 @@ public String getFilterText()
param = param.replaceAll("\\$", "\\\\\\$");
sql = sql.replaceFirst("\\?", param);
}
return sql.replaceAll("\"", "");
return sql.replace("\"", "");
}


Expand Down Expand Up @@ -997,7 +1054,7 @@ public void setCommonTableExpressionSql(Object key, SQLFragment sqlf, boolean re
sqlf = newSql;
}

CTE cte = commonTableExpressionsMap.get(key);
CTE cte = commonTableExpressionsMap == null ? null : commonTableExpressionsMap.get(key);
if (null == cte)
throw new IllegalStateException("CTE not found.");
cte.sqlf = sqlf;
Expand Down Expand Up @@ -1052,7 +1109,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 +1341,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 +1368,7 @@ public void testIllegalArgument()

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

@Test
Expand All @@ -1328,6 +1383,77 @@ 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 testAppendIdentifierPreQuotedDottedAccepted()
{
// PR2-G1: a dotted sequence of individually well-formed quoted identifiers is legitimate --
// e.g. a fully-qualified "schema"."table" returned by TableInfo.getSelectName(). The strict
// check validates each quoted segment rather than blanket-rejecting any interior dot.
new SQLFragment().appendIdentifier("\"schema\".\"table\"");
new SQLFragment().appendIdentifier("\"a\".\"b\".\"c\"");
}

@Test
public void testAppendIdentifierPreQuotedDotInsideQuotesAccepted()
{
// PR2-G1: a `.` *inside* the quotes is part of a single identifier's name, not a separator.
// This is the metadata-name case (e.g. a calculated column's generated class name) that the
// earlier strict check wrongly rejected.
new SQLFragment().appendIdentifier("\"name.with.dots\"");
new SQLFragment().appendIdentifier("\"org.labkey.query.sql.CalculatedExpressionColumn6b8f\"");
}

@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\""));
// A classic breakout: close the identifier early, then inject. The lone interior quote
// (after `foo`) is followed by non-separator text, so it must be rejected.
shouldFail(() -> new SQLFragment().appendIdentifier("\"foo\"; DROP TABLE x; --\""));
// Stray text after a valid segment (not a `.` separator).
shouldFail(() -> new SQLFragment().appendIdentifier("\"a\" \"b\""));
// Trailing separator with no following segment.
shouldFail(() -> new SQLFragment().appendIdentifier("\"a\".\""));
// Empty segment between separators.
shouldFail(() -> new SQLFragment().appendIdentifier("\"a\"..\"b\""));
}

@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\"");
// Doubled interior quotes around a dot decode to a single identifier named weird"."name --
// distinct from the two-segment "weird"."name" -- and cannot break out, so it's accepted.
new SQLFragment().appendIdentifier("\"weird\"\".\"\"name\"");
}
}

@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)
Copy link
Copy Markdown
Contributor

@labkey-matthewb labkey-matthewb Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be (String schema, String proc) OR (SQLFragment qualifiedProcName)

{
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