diff --git a/api/src/org/labkey/api/data/PropertyManager.java b/api/src/org/labkey/api/data/PropertyManager.java index 2827a6d1f2c..3fc9d7d2eaf 100644 --- a/api/src/org/labkey/api/data/PropertyManager.java +++ b/api/src/org/labkey/api/data/PropertyManager.java @@ -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() diff --git a/api/src/org/labkey/api/data/SQLFragment.java b/api/src/org/labkey/api/data/SQLFragment.java index c944f3f6c7c..2d1af8d0982 100644 --- a/api/src/org/labkey/api/data/SQLFragment.java +++ b/api/src/org/labkey/api/data/SQLFragment.java @@ -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; @@ -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; @@ -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; @@ -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); @@ -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); @@ -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) { @@ -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; } @@ -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("'"); @@ -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); @@ -915,7 +972,7 @@ public String getFilterText() param = param.replaceAll("\\$", "\\\\\\$"); sql = sql.replaceFirst("\\?", param); } - return sql.replaceAll("\"", ""); + return sql.replace("\"", ""); } @@ -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; @@ -1052,7 +1109,7 @@ public static SQLFragment prettyPrint(SQLFragment from) if (t.startsWith("-- 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 diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index da8a3382203..4f1a8e4a6e8 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java @@ -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; @@ -858,7 +854,7 @@ public Map 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 @@ -866,7 +862,7 @@ public String buildProcedureCall(String procSchema, String procName, int paramCo 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++) { diff --git a/api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java b/api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java index 73337dd3feb..281442d69be 100644 --- a/api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java @@ -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; @@ -102,12 +101,6 @@ public boolean supportsGroupConcat() return false; } - @Override - public boolean supportsSelectConcat() - { - return false; - } - @Override public boolean canShowExecutionPlan(ExecutionPlanType type) { @@ -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 @@ -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; } @@ -513,7 +501,7 @@ public Map 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(); } diff --git a/api/src/org/labkey/api/data/dialect/SqlDialect.java b/api/src/org/labkey/api/data/dialect/SqlDialect.java index 45952b986dc..54b647d1ef9 100644 --- a/api/src/org/labkey/api/data/dialect/SqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/SqlDialect.java @@ -95,6 +95,8 @@ import java.util.regex.Pattern; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; // Isolate the big SQL differences between database servers. A new SqlDialect instance is created for each DbScope; the // dialect holds state specific to each database, for example, reserved words, user defined type information, etc. @@ -607,7 +609,7 @@ public SQLFragment appendCaseInsensitiveEndsWith(SQLFragment sql, @NotNull Strin /** * Composes the fragments into a SQL query that will be limited by rowCount * starting at the given 0-based offset. - * + * * @param select must not be null * @param from must not be null * @param filter may be null @@ -623,9 +625,40 @@ public SQLFragment appendCaseInsensitiveEndsWith(SQLFragment sql, @NotNull Strin public abstract boolean supportsComments(); - public abstract String execute(DbSchema schema, String procedureName, String parameters); + /** + * Build a SQL statement that invokes a stored procedure. The schema and procedure names are safely + * quoted and escaped via {@link #getProcedureSelectName} before delegating to the dialect-specific + * {@link #doExecute}, so it is safe to pass less-trusted names. Names with special characters or + * reserved words are quoted; plain legal identifiers are emitted bare. Throws + * {@link IllegalArgumentException} if a name contains a control character or is otherwise malformed. + */ + public final SQLFragment execute(DbSchema schema, String procedureName, SQLFragment parameters) + { + return doExecute(getProcedureSelectName(schema.getName(), procedureName), parameters); + } - public abstract SQLFragment execute(DbSchema schema, String procedureName, SQLFragment parameters); + /** + * Dialect-specific worker for {@link #execute} that builds the procedure invocation SQL around the + * already-quoted, schema-qualified {@code qualifiedProcName}. Always invoke {@code execute()}, never this method. + */ + protected abstract SQLFragment doExecute(SQLFragment qualifiedProcName, SQLFragment parameters); + + /** + * Build a safely quoted, schema-qualified name for invoking a stored procedure/function in a CALL/EXEC/SELECT statement. + */ + protected SQLFragment getProcedureSelectName(String schemaName, String procName) + { + return new SQLFragment().appendDottedIdentifiers(quoteProcedureIdentifier(schemaName), quoteProcedureIdentifier(procName)); + } + + private String quoteProcedureIdentifier(String id) + { + // The schema and procedure names are separate parameters, so a single component may not itself + // contain a '.' -- a dotted name is an ambiguous (possibly smuggled) schema-qualified reference. + if (id.indexOf('.') >= 0) + throw new IllegalArgumentException("Procedure schema/name may not contain '.': " + id); + return shouldQuoteIdentifier(id) ? quoteIdentifier(id) : id; + } public abstract String concatenate(String... args); @@ -712,8 +745,6 @@ public final SQLFragment getGroupConcat(SQLFragment sql, boolean distinct, boole */ public abstract SQLFragment getGroupConcat(SQLFragment sql, boolean distinct, boolean sorted, @NotNull SQLFragment delimiterSQL, boolean includeNulls); - public abstract boolean supportsSelectConcat(); - public boolean supportsGroupConcatSubSelect() { return true; @@ -902,7 +933,7 @@ private boolean validateIdentifier(DatabaseIdentifier id) if (LOG.isTraceEnabled()) { final var me = this; - return _validatedIds.computeIfAbsent(id.getId(), key -> + return _validatedIds.computeIfAbsent(id.getId(), _ -> { var optScope = DbScope.getDbScopesToTest().stream().filter(s -> s.getSqlDialect().getClass() == me.getClass()).findFirst(); if (optScope.isPresent()) @@ -996,6 +1027,17 @@ public String makeLegalIdentifierName(String id) // Escape quotes and quote the identifier public String quoteIdentifier(String id) { + // Be defensive: NUL/CR/LF, DEL, and other non-printable control characters in an identifier + // would either be silently truncated by the driver (SQL Server on NUL) or surface as an + // opaque JDBC error. Reject them here with a clear message before any bytes leave the JVM. + // Character.isISOControl covers the C0 (0x00-0x1F) and C1 (0x7F-0x9F) control ranges; tab is + // the one control character we tolerate. + for (int i = 0, len = id.length(); i < len; i++) + { + char c = id.charAt(i); + if (Character.isISOControl(c) && c != '\t') + throw new IllegalArgumentException("Identifier contains illegal control character (0x" + Integer.toHexString(c) + "): " + id); + } return "\"" + Strings.CS.replace(id, "\"", "\"\"") + "\""; } @@ -1318,19 +1360,19 @@ public String getJDBCArrayType(Object[] array) } else if (Integer.class.equals(componentType)) { - typeName = getJDBCArrayType(Integer.valueOf(0)); + typeName = getJDBCArrayType(0); } else if (Long.class.equals(componentType)) { - typeName = getJDBCArrayType(Long.valueOf(0L)); + typeName = getJDBCArrayType(0L); } else if (Double.class.equals(componentType)) { - typeName = getJDBCArrayType(Double.valueOf(0.0d)); + typeName = getJDBCArrayType(0.0d); } else if (Float.class.equals(componentType)) { - typeName = getJDBCArrayType(Float.valueOf(0.0f)); + typeName = getJDBCArrayType(0.0f); } else if (Boolean.class.equals(componentType)) { @@ -1452,7 +1494,7 @@ public boolean updateStatistics(TableInfo table) public abstract String getBooleanDataType(); - + public String getBooleanTRUE() { return "CAST(1 AS " + getBooleanDataType() + ")"; @@ -1501,12 +1543,12 @@ public String getDatabaseName(String url) throws ServletException /** * Drop a schema if it exists. - * Throws an exception if schema exists, and could not be dropped. + * Throws an exception if schema exists and could not be dropped. */ public void dropSchema(DbSchema schema, String schemaName) { - String sql = schema.getSqlDialect().execute(CoreSchema.getInstance().getSchema(), "fn_dropifexists", "?, ?, ?, ?"); - new SqlExecutor(schema).execute(sql, "*", schemaName, "SCHEMA", null); + SQLFragment sql = schema.getSqlDialect().execute(CoreSchema.getInstance().getSchema(), "fn_dropifexists", new SQLFragment("?, ?, ?, ?", "*", schemaName, "SCHEMA", null)); + new SqlExecutor(schema).execute(sql); } /** @@ -1519,14 +1561,14 @@ public void dropSchema(DbSchema schema, String schemaName) */ public void dropIfExists(DbSchema schema, String objectName, String objectType, @Nullable String subObjectName) { - String sql = schema.getSqlDialect().execute(CoreSchema.getInstance().getSchema(), "fn_dropifexists", "?, ?, ?, ?"); String schemaName = schema.getName(); if (Strings.CS.contains(objectName,".") && Strings.CS.startsWith(objectName,getGlobalTempTablePrefix())) { schemaName = objectName.substring(0,objectName.indexOf(".")); objectName = objectName.substring(objectName.indexOf(".")+1); } - new SqlExecutor(schema).execute(sql, objectName, schemaName, objectType, subObjectName); + SQLFragment sql = schema.getSqlDialect().execute(CoreSchema.getInstance().getSchema(), "fn_dropifexists", new SQLFragment("?, ?, ?, ?", objectName, schemaName, objectType, subObjectName)); + new SqlExecutor(schema).execute(sql); } /** @@ -1829,7 +1871,7 @@ private int getTomcatJdbcVersion() if (serverInfo.startsWith("Apache Tomcat/")) { String[] versionParts = serverInfo.substring(14).split("\\."); - int majorVersion = Integer.valueOf(versionParts[0]); + int majorVersion = Integer.parseInt(versionParts[0]); if (majorVersion >= 6) version = 4; @@ -2065,11 +2107,23 @@ public void setParamValue(Object value) public abstract Map getParametersFromDbMetadata(DbScope scope, String procSchema, String procName) throws SQLException; /** - * Build the dialect-specific string to call the procedure, with the correct number and placement of parameter placeholders + * Build the string to call the procedure, with the correct number and placement of parameter placeholders. + * The schema and procedure names are safely quoted and escaped via {@link #getProcedureSelectName} before + * delegating to the dialect-specific {@link #doBuildProcedureCall}, so it is safe to pass less-trusted names. + * Throws {@link IllegalArgumentException} if a name contains a control character or is otherwise malformed. * * @param assignResult true if the call string should include an assignment (e.g., "? = CALL...) Some dialects always need this; for others it is dependent on return type */ - public abstract String buildProcedureCall(String procSchema, String procName, int paramCount, boolean hasReturn, boolean assignResult, DbScope procScope); + public final String buildProcedureCall(String procSchema, String procName, int paramCount, boolean hasReturn, boolean assignResult, DbScope procScope) + { + return doBuildProcedureCall(getProcedureSelectName(procSchema, procName).getSQL(), paramCount, hasReturn, assignResult, procScope); + } + + /** + * Dialect-specific worker for {@link #buildProcedureCall} that builds the call syntax around the already-quoted, + * schema-qualified {@code qualifiedProcName}. Always invoke {@code buildProcedureCall()}, never this method. + */ + protected abstract String doBuildProcedureCall(String qualifiedProcName, int paramCount, boolean hasReturn, boolean assignResult, DbScope procScope); /** * Register and set the input value for each INPUT or INPUT/OUTPUT parameter from the parameters map into the CallableStatement, and register @@ -2327,5 +2381,33 @@ public void testAutoIncrementQuery() Sequence seq = sequences.stream().findFirst().orElseThrow(); assertEquals("rowid", seq.columnName().toLowerCase()); } + + @Test + public void testProcedureIdentifierQuoting() + { + DbScope scope = DbScope.getLabKeyScope(); + SqlDialect dialect = scope.getSqlDialect(); + DbSchema core = CoreSchema.getInstance().getSchema(); + + // Plain legal names are emitted bare (no behavior change), through both execute() and buildProcedureCall() + assertTrue(dialect.execute(core, "fn_dropifexists", new SQLFragment("?, ?, ?, ?")).getSQL().contains("core.fn_dropifexists")); + assertTrue(dialect.buildProcedureCall(core.getName(), "fn_dropifexists", 0, false, false, scope).contains("core.fn_dropifexists")); + + // Names with special characters are quoted and embedded quotes are doubled (cannot break out of the quoted identifier) + assertTrue(dialect.execute(core, "a\"b", new SQLFragment("?")).getSQL().contains("\"a\"\"b\"")); + assertTrue(dialect.buildProcedureCall(core.getName(), "weird name", 0, false, false, scope).contains("\"weird name\"")); + + // A reserved word is quoted + assertTrue(dialect.buildProcedureCall(core.getName(), "select", 0, false, false, scope).contains("\"select\"")); + + // A special-character schema name is quoted + assertTrue(dialect.buildProcedureCall("weird schema", "proc", 0, false, false, scope).contains("\"weird schema\"")); + + // Control characters are hard failures + assertThrows(IllegalArgumentException.class, () -> dialect.execute(core, "a\0b", new SQLFragment("?"))); + assertThrows(IllegalArgumentException.class, () -> dialect.buildProcedureCall(core.getName(), "a\0b", 0, false, false, scope)); + // A dotted name is rejected: schema and procedure are separate parameters, so a '.' in a single component is an ambiguous schema-qualified reference + assertThrows(IllegalArgumentException.class, () -> dialect.buildProcedureCall(core.getName(), "a.b", 0, false, false, scope)); + } } } diff --git a/api/src/org/labkey/api/exp/OntologyManager.java b/api/src/org/labkey/api/exp/OntologyManager.java index b284960bccf..c30734dfbf3 100644 --- a/api/src/org/labkey/api/exp/OntologyManager.java +++ b/api/src/org/labkey/api/exp/OntologyManager.java @@ -30,8 +30,39 @@ import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveMapWrapper; import org.labkey.api.collections.IntHashMap; -import org.labkey.api.data.*; +import org.labkey.api.data.BeanObjectFactory; +import org.labkey.api.data.ColumnInfo; +import org.labkey.api.data.ColumnRenderPropertiesImpl; +import org.labkey.api.data.CompareType; +import org.labkey.api.data.ConditionalFormat; +import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerFilter; +import org.labkey.api.data.ContainerManager; +import org.labkey.api.data.ConvertHelper; +import org.labkey.api.data.DatabaseCache; +import org.labkey.api.data.DbSchema; +import org.labkey.api.data.DbSchemaType; +import org.labkey.api.data.DbScope; import org.labkey.api.data.DbScope.Transaction; +import org.labkey.api.data.ExceptionFramework; +import org.labkey.api.data.JdbcType; +import org.labkey.api.data.MultiChoice; +import org.labkey.api.data.MvUtil; +import org.labkey.api.data.ObjectFactory; +import org.labkey.api.data.Parameter; +import org.labkey.api.data.ParameterMapStatement; +import org.labkey.api.data.RemapCache; +import org.labkey.api.data.RuntimeSQLException; +import org.labkey.api.data.SQLFragment; +import org.labkey.api.data.SimpleFilter; +import org.labkey.api.data.Sort; +import org.labkey.api.data.SqlExecutor; +import org.labkey.api.data.SqlSelector; +import org.labkey.api.data.StatementUtils; +import org.labkey.api.data.Table; +import org.labkey.api.data.TableInfo; +import org.labkey.api.data.TableSelector; +import org.labkey.api.data.UpdateableTableInfo; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.dataiterator.DataIterator; import org.labkey.api.dataiterator.DataIteratorContext; @@ -64,8 +95,8 @@ import org.labkey.api.util.HtmlString; import org.labkey.api.util.HtmlStringBuilder; import org.labkey.api.util.Pair; -import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.ResultSetUtil; +import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.TestContext; import org.labkey.api.view.HttpView; @@ -1012,12 +1043,12 @@ public static void deleteOntologyObjects(Container c, String... uris) try { DbSchema schema = getExpSchema(); - String sql = getSqlDialect().execute(getExpSchema(), "deleteObject", "?, ?"); SqlExecutor executor = new SqlExecutor(schema); for (String uri : uris) { - executor.execute(sql, c.getId(), uri); + SQLFragment sql = getSqlDialect().execute(getExpSchema(), "deleteObject", new SQLFragment("?, ?", c.getId(), uri)); + executor.execute(sql); } } finally @@ -1749,15 +1780,11 @@ private static PropertyDescriptor ensurePropertyDescriptor(PropertyDescriptor pd // you are allowed to update if you are coming from the project root, or if you are in the container // in which the descriptor was created - boolean fUpdateIfExists = false; - if (pdIn.getContainer().getId().equals(pd.getContainer().getId()) - || pdIn.getContainer().getId().equals(pdIn.getProject().getId())) - fUpdateIfExists = true; + boolean fUpdateIfExists = pdIn.getContainer().getId().equals(pd.getContainer().getId()) + || pdIn.getContainer().getId().equals(pdIn.getProject().getId()); - boolean fMajorDifference = false; - if (colDiffs.toString().contains("RangeURI") || colDiffs.toString().contains("PropertyType")) - fMajorDifference = true; + boolean fMajorDifference = colDiffs.toString().contains("RangeURI") || colDiffs.toString().contains("PropertyType"); String errmsg = "ensurePropertyDescriptor: descriptor In different from Found for " + colDiffs + "\n\t Descriptor In: " + pdIn + diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index b6cf8154f87..d36771395a3 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -124,6 +124,7 @@ import org.labkey.api.reports.LabKeyScriptEngineManager; import org.labkey.api.resource.Resource; import org.labkey.api.search.SearchService; +import org.labkey.api.secrets.SecretService; import org.labkey.api.security.AuthenticationManager; import org.labkey.api.security.AuthenticationManager.Priority; import org.labkey.api.security.AuthenticationSettingsAuditTypeProvider; @@ -158,7 +159,6 @@ import org.labkey.api.settings.OptionalFeatureFlag; import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.settings.OptionalFeatureService.FeatureType; -import org.labkey.api.secrets.SecretService; import org.labkey.api.settings.ProductConfiguration; import org.labkey.api.settings.StandardStartupPropertyHandler; import org.labkey.api.settings.StartupPropertyEntry; @@ -254,8 +254,8 @@ import org.labkey.core.dialect.PostgreSqlVersion; import org.labkey.core.junit.JunitController; import org.labkey.core.login.DbLoginAuthenticationProvider; -import org.labkey.core.login.LoginAttemptDisableLoginProvider; import org.labkey.core.login.DbLoginManager; +import org.labkey.core.login.LoginAttemptDisableLoginProvider; import org.labkey.core.login.LoginController; import org.labkey.core.metrics.SimpleMetricsServiceImpl; import org.labkey.core.metrics.WebSocketConnectionManager; @@ -284,6 +284,7 @@ import org.labkey.core.reports.DocumentConversionServiceImpl; import org.labkey.core.reports.ScriptEngineManagerImpl; import org.labkey.core.script.RhinoService; +import org.labkey.core.secrets.SecretServiceImpl; import org.labkey.core.security.AllowedExternalResourceHosts; import org.labkey.core.security.ApiKeyViewProvider; import org.labkey.core.security.SecurityApiActions; @@ -296,8 +297,6 @@ import org.labkey.core.thumbnail.ThumbnailServiceImpl; import org.labkey.core.user.LimitActiveUsersSettings; import org.labkey.core.user.UserController; -import org.labkey.core.secrets.SecretServiceImpl; - import org.labkey.core.vcs.VcsServiceImpl; import org.labkey.core.view.ShortURLServiceImpl; import org.labkey.core.view.TableViewFormTestCase; @@ -540,8 +539,8 @@ public QuerySchema createSchema(DefaultSchema schema, Module module) "This feature will switch the query based select inputs on the row insert/update form to use the React QuerySelect" + "component. This will allow for a user to view the first 100 options in the select but then use type ahead" + "search to find the other select values.", false, true); - OptionalFeatureService.get().addExperimentalFeatureFlag(SQLFragment.FEATUREFLAG_DISABLE_STRICT_CHECKS, "Disable SQLFragment strict checks", - "SQLFragment now has very strict usage validation, these checks may cause errors in code that has not been updated. Turn on this feature to disable checks.", false, true); + OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag(SQLFragment.FEATUREFLAG_DISABLE_STRICT_CHECKS, "Disable SQLFragment strict checks", + "Disables strict SQL generation safeguards in SQLFragment.appendIdentifier and QueryPivot value emission", false, true, FeatureType.Deprecated)); OptionalFeatureService.get().addExperimentalFeatureFlag(LoginController.FEATUREFLAG_DISABLE_LOGIN_XFRAME, "Disable Login X-FRAME-OPTIONS=DENY", "By default LabKey disables all framing of login related actions. Disabling this feature will revert to using the standard site settings.", false, true); OptionalFeatureService.get().addExperimentalFeatureFlag(PageTemplate.EXPERIMENTAL_SHORT_CIRCUIT_ROBOTS, @@ -1672,7 +1671,7 @@ public void handle(Map map) { SiteResourceHandler handler = getResourceHandler(entry.getKey()); if (handler != null) - incrementRevision |= setSiteResource(handler, entry.getValue(), User.guest); + incrementRevision |= setSiteResource(handler, entry.getValue()); } // Bump the look & feel revision so browsers retrieve the new logo, custom stylesheet, etc. @@ -1701,7 +1700,7 @@ private void populateSiteSettingsWithStartupProps() } StartupPropertyEntry resetPermissionsEntry = props.get(homeProjectResetPermissions); - if (null != resetPermissionsEntry && Boolean.valueOf(resetPermissionsEntry.getValue())) + if (null != resetPermissionsEntry && Boolean.parseBoolean(resetPermissionsEntry.getValue())) { // reset the home project permissions to remove the default assignments given at server install MutableSecurityPolicy homePolicy = new MutableSecurityPolicy(ContainerManager.getHomeContainer()); @@ -1743,14 +1742,14 @@ private void populateSiteSettingsWithStartupProps() return LookAndFeelPropertiesManager.get().getResourceHandler(type); } - private boolean setSiteResource(SiteResourceHandler resourceHandler, StartupPropertyEntry prop, User user) + private boolean setSiteResource(SiteResourceHandler resourceHandler, StartupPropertyEntry prop) { Resource resource = getModuleResourceFromPropValue(prop.getValue()); if (resource != null) { try { - resourceHandler.accept(resource, ContainerManager.getRoot(), user); + resourceHandler.accept(resource, ContainerManager.getRoot(), User.guest); return true; } catch(Exception e) diff --git a/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java b/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java index 6e9b51ddbe0..6a8c97f97dd 100644 --- a/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java +++ b/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java @@ -259,12 +259,6 @@ public boolean supportsGroupConcat() return getServerType().supportsGroupConcat(); } - @Override - public boolean supportsSelectConcat() - { - return true; - } - @Override public SQLFragment getSelectConcat(SQLFragment selectSql, String delimiter) { diff --git a/query/src/org/labkey/query/QueryModule.java b/query/src/org/labkey/query/QueryModule.java index 08afcc10ad3..9a0184bbebd 100644 --- a/query/src/org/labkey/query/QueryModule.java +++ b/query/src/org/labkey/query/QueryModule.java @@ -133,6 +133,7 @@ import org.labkey.query.reports.view.ReportUIProvider; import org.labkey.query.sql.Method; import org.labkey.query.sql.QNode; +import org.labkey.query.sql.QNumber; import org.labkey.query.sql.Query; import org.labkey.query.sql.SqlParser; import org.labkey.query.view.InheritedQueryDataViewProvider; @@ -432,6 +433,7 @@ public Set getSchemaNames() Method.TestCase.class, ExpressionAssistantAgentAction.TestCase.class, QNode.TestCase.class, + QNumber.TestCase.class, Query.TestCase.class, ReportsController.SerializationTest.class, SqlParser.SqlParserTestCase.class, diff --git a/query/src/org/labkey/query/sql/QNumber.java b/query/src/org/labkey/query/sql/QNumber.java index 712526779c0..e81297064bd 100644 --- a/query/src/org/labkey/query/sql/QNumber.java +++ b/query/src/org/labkey/query/sql/QNumber.java @@ -16,10 +16,18 @@ package org.labkey.query.sql; +import org.antlr.runtime.CommonToken; import org.antlr.runtime.tree.CommonTree; import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.data.JdbcType; +import org.labkey.api.data.SQLFragment; +import org.labkey.api.query.QueryParseException; +import org.labkey.api.settings.AppProps; +import org.labkey.api.util.logging.LogHelper; import org.labkey.query.sql.antlr.SqlBaseParser; import java.math.BigDecimal; @@ -27,25 +35,12 @@ public class QNumber extends QExpr implements IConstant { + private static final Logger LOG = LogHelper.getLogger(QNumber.class, "Numeric literal parse diagnostics"); + Number _value = null; JdbcType _sqlType = JdbcType.DOUBLE; - - - public QNumber(String s) - { - String substring = s; - if (s.startsWith("0x")) - setValue(convertInteger(s)); - else if (s.startsWith("+") || s.startsWith("-")) - substring = s.substring(1); - if (StringUtils.containsOnly(substring,"0123456789")) - setValue(convertInteger(s)); - else - setValue(convertDouble(s)); - } - public QNumber(CommonTree n) { super(false); @@ -69,11 +64,29 @@ public QNumber(CommonTree n) } catch (NumberFormatException x) { - // + // Lexer produced a numeric token Java couldn't parse. Strict mode (default) treats this + // as a parse error rather than silently emitting the raw lexeme (a SQL-injection + // surface). Gated by FEATUREFLAG_DISABLE_STRICT_CHECKS while we collect telemetry; + // when the flag is set, fall back to the previous silent behavior so existing + // deployments that hit this path keep working. + if (AppProps.getInstance().isOptionalFeatureEnabled(SQLFragment.FEATUREFLAG_DISABLE_STRICT_CHECKS)) + { + LOG.warn("Unparseable numeric literal in LabKey SQL (flag-on, falling back to raw lexeme): {}", getTokenText()); + // leave _value null; getValueString() falls back to getTokenText() + } + else + { + LOG.warn("Unparseable numeric literal in LabKey SQL (flag-off, throwing parse error): {}", getTokenText()); + // Throw QueryParseException (not IllegalArgumentException) so SqlParser.wrapParseException returns it + // as-is: the user sees a precise, located parse error instead of a generic "Unexpected exception" at + // line 0:0, and it isn't logged at ERROR/reported to mothership. cause is left null so the + // QueryParseException constructor sets SkipMothershipLogging (this is malformed user input, not a fault). + throw new QueryParseException("Invalid numeric literal: " + getTokenText(), null, getLine(), getColumn()); + } } } - + public QNumber(Number value) { setValue(value); @@ -144,7 +157,7 @@ Number convertInteger(String s) return new BigInteger(s, base); } } - + Number convertDouble(String s) { @@ -193,4 +206,98 @@ public boolean isConstant() { return true; } + + + public static class TestCase extends Assert + { + private static CommonTree token(int type, String text) + { + return new CommonTree(new CommonToken(type, text)); + } + + @Test + public void testValidIntegerToken() + { + QNumber n = new QNumber(token(SqlBaseParser.NUM_INT, "42")); + assertEquals(42L, n.getValue()); + assertEquals("42", n.getValueString()); + assertEquals(JdbcType.INTEGER, n.getJdbcType()); + } + + @Test + public void testValidLongToken() + { + QNumber n = new QNumber(token(SqlBaseParser.NUM_LONG, "9999999999")); + assertEquals(9999999999L, n.getValue()); + } + + @Test + public void testValidDoubleToken() + { + QNumber n = new QNumber(token(SqlBaseParser.NUM_DOUBLE, "1.5")); + assertEquals(new BigDecimal("1.5"), n.getValue()); + assertEquals(JdbcType.DECIMAL, n.getJdbcType()); + } + + @Test + public void testValidFloatScientificToken() + { + // 'e' in the token text triggers the floatish branch -> Double.parseDouble + QNumber n = new QNumber(token(SqlBaseParser.NUM_FLOAT, "1.5e2")); + assertEquals(150.0, ((Number) n.getValue()).doubleValue(), 0.0); + } + + /** + * Strict mode (default — flag unset): an unparseable numeric token must surface as a + * parse-time QueryParseException so SqlParser routes it into _parseErrors as a + * user-facing error rather than silently emitting the raw lexeme. + */ + @Test + public void testInvalidIntegerStrictThrows() + { + if (AppProps.getInstance().isOptionalFeatureEnabled(SQLFragment.FEATUREFLAG_DISABLE_STRICT_CHECKS)) + return; // flag set in this environment; strict-mode assertion does not apply + try + { + new QNumber(token(SqlBaseParser.NUM_INT, "1.2.3")); + fail("Expected QueryParseException for unparseable NUM_INT token"); + } + catch (QueryParseException expected) + { + assertTrue("error message should include the bad token: " + expected.getMessage(), + expected.getMessage().contains("1.2.3")); + } + } + + @Test + public void testInvalidDoubleStrictThrows() + { + if (AppProps.getInstance().isOptionalFeatureEnabled(SQLFragment.FEATUREFLAG_DISABLE_STRICT_CHECKS)) + return; + try + { + new QNumber(token(SqlBaseParser.NUM_DOUBLE, "1.2.3.4")); + fail("Expected QueryParseException for unparseable NUM_DOUBLE token"); + } + catch (QueryParseException expected) + { + assertTrue(expected.getMessage().contains("1.2.3.4")); + } + } + + /** + * Lenient mode (flag set): the previous silent-fallback behavior is preserved so existing + * deployments that somehow reach this path keep working. _value stays null and + * getValueString() returns the raw token text. + */ + @Test + public void testInvalidIntegerLenientFallback() + { + if (!AppProps.getInstance().isOptionalFeatureEnabled(SQLFragment.FEATUREFLAG_DISABLE_STRICT_CHECKS)) + return; // flag not set; lenient-mode assertion does not apply + QNumber n = new QNumber(token(SqlBaseParser.NUM_INT, "1.2.3")); + assertNull(n.getValue()); + assertEquals("1.2.3", n.getValueString()); + } + } } diff --git a/query/src/org/labkey/query/sql/QueryLookupWrapper.java b/query/src/org/labkey/query/sql/QueryLookupWrapper.java index 88de8ac7cb5..c0c9bed230d 100644 --- a/query/src/org/labkey/query/sql/QueryLookupWrapper.java +++ b/query/src/org/labkey/query/sql/QueryLookupWrapper.java @@ -17,8 +17,8 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.labkey.api.collections.ArrayListMap; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveHashSet; @@ -148,7 +148,6 @@ protected void setHasLookup() _source.setAlias(getAlias() + "Wrapped"); } - @Override public void declareFields() { @@ -310,7 +309,6 @@ public int getSelectedColumnCount() { return _selectedColumns.size(); } - @Override public QLWColumn getColumn(@NotNull String name) @@ -457,7 +455,7 @@ public SQLFragment getSql() else sql.append(f); sql.append(" AS "); - sql.append(col.getAlias()); + sql.appendIdentifier(col.getAlias()); comma = ", "; } sql.append("\nFROM "); @@ -521,7 +519,7 @@ private abstract class QLWColumn extends RelationColumn final String _alias; ColumnLogging _columnLogging = null; PHI _phi; - + QLWColumn(AbstractQueryRelation table, FieldKey key, String alias) { _table = table; diff --git a/query/src/org/labkey/query/sql/QueryPivot.java b/query/src/org/labkey/query/sql/QueryPivot.java index 49f6eaea1db..394aae3ef79 100644 --- a/query/src/org/labkey/query/sql/QueryPivot.java +++ b/query/src/org/labkey/query/sql/QueryPivot.java @@ -16,11 +16,33 @@ package org.labkey.query.sql; import org.apache.commons.beanutils.ConvertUtils; -import org.jetbrains.annotations.Nullable; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.labkey.api.collections.CaseInsensitiveMapWrapper; import org.labkey.api.collections.NamedObjectList; -import org.labkey.api.data.*; +import org.labkey.api.data.AbstractTableInfo; +import org.labkey.api.data.AggregateColumnInfo; +import org.labkey.api.data.BaseColumnInfo; +import org.labkey.api.data.ColumnInfo; +import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerFilter; +import org.labkey.api.data.CrosstabDimension; +import org.labkey.api.data.CrosstabMeasure; +import org.labkey.api.data.CrosstabMember; +import org.labkey.api.data.CrosstabSettings; +import org.labkey.api.data.CrosstabTableInfo; +import org.labkey.api.data.Filter; +import org.labkey.api.data.ForeignKey; +import org.labkey.api.data.JdbcType; +import org.labkey.api.data.NullColumnInfo; +import org.labkey.api.data.PHI; +import org.labkey.api.data.QueryLogging; +import org.labkey.api.data.RenderContext; +import org.labkey.api.data.SQLFragment; +import org.labkey.api.data.Sort; +import org.labkey.api.data.SqlSelector; +import org.labkey.api.data.TableInfo; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.query.AliasManager; import org.labkey.api.query.FieldKey; @@ -29,7 +51,9 @@ import org.labkey.api.query.QueryService; import org.labkey.api.query.SchemaKey; import org.labkey.api.query.UserSchema; +import org.labkey.api.settings.AppProps; import org.labkey.api.util.StringExpression; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.UnauthorizedException; import org.labkey.data.xml.ColumnType; import org.springframework.dao.DataAccessException; @@ -56,6 +80,8 @@ */ public class QueryPivot extends AbstractQueryRelation { + private static final Logger LOG = LogHelper.getLogger(QueryPivot.class, "Pivot value emission diagnostics"); + final QuerySelect _from; final AliasManager _manager; final Map pivotColumnAliases = new HashMap<>(); @@ -573,7 +599,6 @@ String makePivotAggName(String aggName, String pivotValue) { return pivotValue + PIVOT_SEPARATOR + aggName; } - @Override public RelationColumn getColumn(@NotNull String name) @@ -726,7 +751,6 @@ public RelationColumn getLookupColumn(@NotNull RelationColumn parent, @NotNull C return null; } - @Override public SQLFragment getSql() { @@ -822,9 +846,28 @@ public SQLFragment getSql() String alias = makePivotColumnAlias(col.getAlias(), pivotValue.getKey()); sql.append(comma).append("MAX(CASE WHEN (").append(_pivotColumn.getValueSql()); if (value instanceof QNull) + { sql.append(" IS NULL"); + } + else if (value instanceof QString qs) + { + // Route QString pivot values through the dialect's string handler so + // dialect-specific escape rules (e.g. PG non-conforming strings) are honored. + sql.append("="); + if (AppProps.getInstance().isOptionalFeatureEnabled(SQLFragment.FEATUREFLAG_DISABLE_STRICT_CHECKS)) + { + LOG.debug("Pivot QString value emitted via legacy LabKey-SQL escape (flag-on, dialect string handler bypassed)"); + sql.append(value.getSourceText()); + } + else + { + sql.appendStringLiteral(qs.getValue(), getSqlDialect()); + } + } else + { sql.append("=").append(value.getSourceText()); + } sql.append(") THEN (").append(col.getValueSql()).append(") ELSE NULL END) AS ").appendIdentifier(alias); comma = ",\n"; } @@ -1084,7 +1127,7 @@ private CrosstabMember createCrosstabMember(Object value, String caption) SqlDialect _dialect; - + SqlDialect getSqlDialect() { if (null == _dialect) @@ -1093,7 +1136,7 @@ SqlDialect getSqlDialect() } - + // We could use aliasManager and remember these // but its easier if we can generate names we expect to be unique String makePivotColumnAlias(String aggAlias, String pivotValueName) @@ -1121,7 +1164,7 @@ class PivotForeignKey implements ForeignKey, ExpandoForeignKey { _agg = agg; } - + @Override public ColumnInfo createLookupColumn(ColumnInfo parent, String displayField) { diff --git a/query/src/org/labkey/query/sql/SqlParser.java b/query/src/org/labkey/query/sql/SqlParser.java index 80ae4653c9c..530c6a8328e 100644 --- a/query/src/org/labkey/query/sql/SqlParser.java +++ b/query/src/org/labkey/query/sql/SqlParser.java @@ -43,7 +43,6 @@ import org.labkey.api.query.AliasManager; import org.labkey.api.query.DefaultSchema; import org.labkey.api.query.FieldKey; -import org.labkey.api.query.QueryKey; import org.labkey.api.query.QueryParseException; import org.labkey.api.query.QueryParseWarning; import org.labkey.api.query.QuerySchema; @@ -80,8 +79,126 @@ import java.util.Map; import java.util.Set; -import static org.labkey.query.sql.QNode.*; -import static org.labkey.query.sql.antlr.SqlBaseParser.*; +import static org.labkey.query.sql.antlr.SqlBaseParser.AGGREGATE; +import static org.labkey.query.sql.antlr.SqlBaseParser.ALIAS; +import static org.labkey.query.sql.antlr.SqlBaseParser.ALL; +import static org.labkey.query.sql.antlr.SqlBaseParser.AND; +import static org.labkey.query.sql.antlr.SqlBaseParser.ANY; +import static org.labkey.query.sql.antlr.SqlBaseParser.AS; +import static org.labkey.query.sql.antlr.SqlBaseParser.ASCENDING; +import static org.labkey.query.sql.antlr.SqlBaseParser.AVG; +import static org.labkey.query.sql.antlr.SqlBaseParser.BETWEEN; +import static org.labkey.query.sql.antlr.SqlBaseParser.BIT_AND; +import static org.labkey.query.sql.antlr.SqlBaseParser.BIT_OR; +import static org.labkey.query.sql.antlr.SqlBaseParser.BIT_XOR; +import static org.labkey.query.sql.antlr.SqlBaseParser.CASE; +import static org.labkey.query.sql.antlr.SqlBaseParser.CASE2; +import static org.labkey.query.sql.antlr.SqlBaseParser.CAST; +import static org.labkey.query.sql.antlr.SqlBaseParser.CLOSE; +import static org.labkey.query.sql.antlr.SqlBaseParser.COLON; +import static org.labkey.query.sql.antlr.SqlBaseParser.COMMA; +import static org.labkey.query.sql.antlr.SqlBaseParser.COMMENT; +import static org.labkey.query.sql.antlr.SqlBaseParser.CONCAT; +import static org.labkey.query.sql.antlr.SqlBaseParser.COUNT; +import static org.labkey.query.sql.antlr.SqlBaseParser.CROSS; +import static org.labkey.query.sql.antlr.SqlBaseParser.DATATYPE; +import static org.labkey.query.sql.antlr.SqlBaseParser.DECLARATION; +import static org.labkey.query.sql.antlr.SqlBaseParser.DELETE; +import static org.labkey.query.sql.antlr.SqlBaseParser.DESCENDING; +import static org.labkey.query.sql.antlr.SqlBaseParser.DISTINCT; +import static org.labkey.query.sql.antlr.SqlBaseParser.DIV; +import static org.labkey.query.sql.antlr.SqlBaseParser.DOT; +import static org.labkey.query.sql.antlr.SqlBaseParser.ELSE; +import static org.labkey.query.sql.antlr.SqlBaseParser.END; +import static org.labkey.query.sql.antlr.SqlBaseParser.EOF; +import static org.labkey.query.sql.antlr.SqlBaseParser.EQ; +import static org.labkey.query.sql.antlr.SqlBaseParser.ESCAPE; +import static org.labkey.query.sql.antlr.SqlBaseParser.EXCEPT; +import static org.labkey.query.sql.antlr.SqlBaseParser.EXISTS; +import static org.labkey.query.sql.antlr.SqlBaseParser.EXPANCESTORSOF; +import static org.labkey.query.sql.antlr.SqlBaseParser.EXPDESCENDANTSOF; +import static org.labkey.query.sql.antlr.SqlBaseParser.EXPLINEAGEOF; +import static org.labkey.query.sql.antlr.SqlBaseParser.EXPONENT; +import static org.labkey.query.sql.antlr.SqlBaseParser.EXPR_LIST; +import static org.labkey.query.sql.antlr.SqlBaseParser.FALSE; +import static org.labkey.query.sql.antlr.SqlBaseParser.FLOAT_SUFFIX; +import static org.labkey.query.sql.antlr.SqlBaseParser.FROM; +import static org.labkey.query.sql.antlr.SqlBaseParser.FULL; +import static org.labkey.query.sql.antlr.SqlBaseParser.GE; +import static org.labkey.query.sql.antlr.SqlBaseParser.GROUP; +import static org.labkey.query.sql.antlr.SqlBaseParser.GROUP_CONCAT; +import static org.labkey.query.sql.antlr.SqlBaseParser.GT; +import static org.labkey.query.sql.antlr.SqlBaseParser.HAVING; +import static org.labkey.query.sql.antlr.SqlBaseParser.HEX_DIGIT; +import static org.labkey.query.sql.antlr.SqlBaseParser.IDENT; +import static org.labkey.query.sql.antlr.SqlBaseParser.ID_LETTER; +import static org.labkey.query.sql.antlr.SqlBaseParser.ID_START_LETTER; +import static org.labkey.query.sql.antlr.SqlBaseParser.IFDEFINED; +import static org.labkey.query.sql.antlr.SqlBaseParser.IN; +import static org.labkey.query.sql.antlr.SqlBaseParser.INNER; +import static org.labkey.query.sql.antlr.SqlBaseParser.INSERT; +import static org.labkey.query.sql.antlr.SqlBaseParser.INTERSECT; +import static org.labkey.query.sql.antlr.SqlBaseParser.INTO; +import static org.labkey.query.sql.antlr.SqlBaseParser.IN_LIST; +import static org.labkey.query.sql.antlr.SqlBaseParser.IS; +import static org.labkey.query.sql.antlr.SqlBaseParser.IS_NOT; +import static org.labkey.query.sql.antlr.SqlBaseParser.JOIN; +import static org.labkey.query.sql.antlr.SqlBaseParser.LE; +import static org.labkey.query.sql.antlr.SqlBaseParser.LEFT; +import static org.labkey.query.sql.antlr.SqlBaseParser.LIKE; +import static org.labkey.query.sql.antlr.SqlBaseParser.LIMIT; +import static org.labkey.query.sql.antlr.SqlBaseParser.LINE_COMMENT; +import static org.labkey.query.sql.antlr.SqlBaseParser.LT; +import static org.labkey.query.sql.antlr.SqlBaseParser.MAX; +import static org.labkey.query.sql.antlr.SqlBaseParser.METHOD_CALL; +import static org.labkey.query.sql.antlr.SqlBaseParser.MIN; +import static org.labkey.query.sql.antlr.SqlBaseParser.MINUS; +import static org.labkey.query.sql.antlr.SqlBaseParser.MODULO; +import static org.labkey.query.sql.antlr.SqlBaseParser.NE; +import static org.labkey.query.sql.antlr.SqlBaseParser.NOT; +import static org.labkey.query.sql.antlr.SqlBaseParser.NOT_BETWEEN; +import static org.labkey.query.sql.antlr.SqlBaseParser.NOT_IN; +import static org.labkey.query.sql.antlr.SqlBaseParser.NOT_LIKE; +import static org.labkey.query.sql.antlr.SqlBaseParser.NULL; +import static org.labkey.query.sql.antlr.SqlBaseParser.NUM_DOUBLE; +import static org.labkey.query.sql.antlr.SqlBaseParser.NUM_FLOAT; +import static org.labkey.query.sql.antlr.SqlBaseParser.NUM_INT; +import static org.labkey.query.sql.antlr.SqlBaseParser.NUM_LONG; +import static org.labkey.query.sql.antlr.SqlBaseParser.ON; +import static org.labkey.query.sql.antlr.SqlBaseParser.OPEN; +import static org.labkey.query.sql.antlr.SqlBaseParser.OR; +import static org.labkey.query.sql.antlr.SqlBaseParser.ORDER; +import static org.labkey.query.sql.antlr.SqlBaseParser.OUTER; +import static org.labkey.query.sql.antlr.SqlBaseParser.PARAM; +import static org.labkey.query.sql.antlr.SqlBaseParser.PIVOT; +import static org.labkey.query.sql.antlr.SqlBaseParser.PLUS; +import static org.labkey.query.sql.antlr.SqlBaseParser.QUERY; +import static org.labkey.query.sql.antlr.SqlBaseParser.QUOTED_IDENTIFIER; +import static org.labkey.query.sql.antlr.SqlBaseParser.QUOTED_STRING; +import static org.labkey.query.sql.antlr.SqlBaseParser.RANGE; +import static org.labkey.query.sql.antlr.SqlBaseParser.RIGHT; +import static org.labkey.query.sql.antlr.SqlBaseParser.ROW_STAR; +import static org.labkey.query.sql.antlr.SqlBaseParser.SELECT; +import static org.labkey.query.sql.antlr.SqlBaseParser.SELECT_FROM; +import static org.labkey.query.sql.antlr.SqlBaseParser.SET; +import static org.labkey.query.sql.antlr.SqlBaseParser.SOME; +import static org.labkey.query.sql.antlr.SqlBaseParser.SQL_NE; +import static org.labkey.query.sql.antlr.SqlBaseParser.STAR; +import static org.labkey.query.sql.antlr.SqlBaseParser.STATEMENT; +import static org.labkey.query.sql.antlr.SqlBaseParser.STDDEV; +import static org.labkey.query.sql.antlr.SqlBaseParser.SUM; +import static org.labkey.query.sql.antlr.SqlBaseParser.THEN; +import static org.labkey.query.sql.antlr.SqlBaseParser.TRUE; +import static org.labkey.query.sql.antlr.SqlBaseParser.UNARY_MINUS; +import static org.labkey.query.sql.antlr.SqlBaseParser.UNARY_PLUS; +import static org.labkey.query.sql.antlr.SqlBaseParser.UNION; +import static org.labkey.query.sql.antlr.SqlBaseParser.UNION_ALL; +import static org.labkey.query.sql.antlr.SqlBaseParser.UPDATE; +import static org.labkey.query.sql.antlr.SqlBaseParser.VALUES; +import static org.labkey.query.sql.antlr.SqlBaseParser.WHEN; +import static org.labkey.query.sql.antlr.SqlBaseParser.WHERE; +import static org.labkey.query.sql.antlr.SqlBaseParser.WITH; +import static org.labkey.query.sql.antlr.SqlBaseParser.WS; /** @@ -413,7 +530,7 @@ public QExpr parseExpr(String str, boolean constExpression, List(); try (var parser = getAntlrParser()) @@ -658,7 +775,7 @@ static String formatRecognitionException(RecognitionException re) if (re instanceof MissingTokenException mte) { if (null != mte.inserted) - missing = tokenName(((CommonToken)mte.inserted).getType()); + missing = tokenName(((CommonToken)mte.inserted).getType()); } if (null != near) @@ -873,33 +990,34 @@ private QNode convertParseTree(CommonTree node, boolean constExpr) private QNode convertNode(CommonTree node, LinkedList children, boolean constExpr) { + label: switch (node.getType()) { - case ALIAS: - case AS: + case SqlBaseParser.ALIAS: + case SqlBaseParser.AS: { // CONSIDER: check type // if (children.size() == 1) // return first(children); - node.getToken().setType(AS); + node.getToken().setType(SqlBaseParser.AS); break; } - case DIV: + case SqlBaseParser.DIV: { var usesNullIf = false; var nonZeroConstant = false; var divisorType = children.size() > 1 ? children.get(1).getTokenType() : 0; - if (divisorType==METHOD_CALL) + if (divisorType== SqlBaseParser.METHOD_CALL) { var method = children.get(1).childList().getFirst(); if ("NULLIF".equalsIgnoreCase(method.getTokenText())) usesNullIf = true; } - else if (divisorType==NUM_DOUBLE || divisorType==NUM_FLOAT || divisorType==NUM_INT || divisorType==NUM_LONG) + else if (divisorType== SqlBaseParser.NUM_DOUBLE || divisorType== SqlBaseParser.NUM_FLOAT || divisorType== SqlBaseParser.NUM_INT || divisorType== SqlBaseParser.NUM_LONG) { try { - nonZeroConstant = 0.0 != (Double)JdbcType.DOUBLE.convert(children.get(1).getTokenText()); + nonZeroConstant = 0.0 != (Double) JdbcType.DOUBLE.convert(children.get(1).getTokenText()); } catch(ConversionException e) { @@ -910,25 +1028,25 @@ else if (divisorType==NUM_DOUBLE || divisorType==NUM_FLOAT || divisorType==NUM_I _parseWarnings.add(new QueryParseWarning("Consider using NULLIF() to prevent division by zero. e.g. dividend / NULLIF(divisor,0))", null, node.getLine(), node.getCharPositionInLine())); break; } - case ESCAPE: + case SqlBaseParser.ESCAPE: { if (children.size() != 1) { _parseErrors.add(new QueryParseException("ESCAPE expects simple string specification", null, node.getLine(), node.getCharPositionInLine())); break; } - return first(children); + return QNode.first(children); } - case IN: - case NOT_IN: + case SqlBaseParser.IN: + case SqlBaseParser.NOT_IN: { - var lhs = firstOrThrow(children); - var rhs = secondOrThrow(children); - if (rhs.getTokenType() == METHOD_CALL) + var lhs = QNode.firstOrThrow(children); + var rhs = QNode.secondOrThrow(children); + if (rhs.getTokenType() == SqlBaseParser.METHOD_CALL) { // rewrite "IN EXPANCESTORS" "IN EXPDESCENDANTS" var method = rhs.getFirstChild(); - if (method.getTokenType() != EXPANCESTORSOF && method.getTokenType() != EXPDESCENDANTSOF && method.getTokenType() != EXPLINEAGEOF) + if (method.getTokenType() != SqlBaseParser.EXPANCESTORSOF && method.getTokenType() != SqlBaseParser.EXPDESCENDANTSOF && method.getTokenType() != SqlBaseParser.EXPLINEAGEOF) { _parseErrors.add(new QueryParseException("Illegal syntax near 'IN'", null, node.getLine(), node.getCharPositionInLine())); return null; @@ -941,21 +1059,21 @@ else if (divisorType==NUM_DOUBLE || divisorType==NUM_FLOAT || divisorType==NUM_I return null; } - var qInLineage = new QInLineage(node.getType() == IN, method.getTokenType()); + var qInLineage = new QInLineage(node.getType() == SqlBaseParser.IN, method.getTokenType()); var qInLineageChildren = new LinkedList(); qInLineageChildren.add(lhs); - qInLineageChildren.add(secondOrThrow(rhsChildren)); + qInLineageChildren.add(QNode.secondOrThrow(rhsChildren)); if (rhsChildren.size() > 2) - qInLineageChildren.add(childOrThrow(rhsChildren, 2)); + qInLineageChildren.add(QNode.childOrThrow(rhsChildren, 2)); qInLineage._replaceChildren(qInLineageChildren); return qInLineage; } } - case METHOD_CALL: + case SqlBaseParser.METHOD_CALL: { - @NotNull QNode id = firstOrThrow(children); - @NotNull QNode exprList = secondOrThrow(children); + @NotNull QNode id = QNode.firstOrThrow(children); + @NotNull QNode exprList = QNode.secondOrThrow(children); // check for special case table method "findColumn", this isn't a real method so it's easier if it has its own node type @@ -974,57 +1092,60 @@ else if (divisorType==NUM_DOUBLE || divisorType==NUM_FLOAT || divisorType==NUM_I break; String name = ((QIdentifier)id).getIdentifier().toLowerCase(); - if (name.equals("convert") || name.equals("cast")) + switch (name) { - if (!(exprList instanceof QExprList) || exprList.childList().size() != 2) + case "convert", "cast" -> { - _parseErrors.add(new QueryParseException(name.toUpperCase() + " function expects 2 arguments", null, node.getLine(), node.getCharPositionInLine())); - break; + if (!(exprList instanceof QExprList) || exprList.childList().size() != 2) + { + _parseErrors.add(new QueryParseException(name.toUpperCase() + " function expects 2 arguments", null, node.getLine(), node.getCharPositionInLine())); + break label; + } + var valueExpression = exprList.childList().get(0); + QNode type = createType(exprList.childList().get(1)); + if (null == type) + { + assert !_parseErrors.isEmpty(); + return null; + } + exprList._replaceChildren(new LinkedList<>(List.of(valueExpression, type))); } - var valueExpression = exprList.childList().get(0); - QNode type = createType(exprList.childList().get(1)); - if (null == type) + case "timestampadd", "timestampdiff" -> { - assert !_parseErrors.isEmpty(); - return null; - } - exprList._replaceChildren(new LinkedList<>(List.of(valueExpression, type))); - } - else if (name.equals("timestampadd") || name.equals("timestampdiff")) - { - if (!(exprList instanceof QExprList) || exprList.childList().size() != 3) - { - _parseErrors.add(new QueryParseException(name.toUpperCase() + " function expects 3 arguments", null, node.getLine(), node.getCharPositionInLine())); - break; + if (!(exprList instanceof QExprList) || exprList.childList().size() != 3) + { + _parseErrors.add(new QueryParseException(name.toUpperCase() + " function expects 3 arguments", null, node.getLine(), node.getCharPositionInLine())); + break label; + } + assert exprList.childList().size() == 3; + LinkedList args = new LinkedList<>(); + args.add(constantToStringNode(exprList.childList().get(0))); + args.add(exprList.childList().get(1)); + args.add(exprList.childList().get(2)); + exprList._replaceChildren(args); + validateTimestampConstant(args.getFirst()); } - assert exprList.childList().size() == 3; - LinkedList args = new LinkedList<>(); - args.add(constantToStringNode(exprList.childList().get(0))); - args.add(exprList.childList().get(1)); - args.add(exprList.childList().get(2)); - exprList._replaceChildren(args); - validateTimestampConstant(args.getFirst()); - } - else if (name.equals("age")) - { - if (!(exprList instanceof QExprList) || exprList.childList().size() < 2 || exprList.childList().size() > 3) + case "age" -> { - _parseErrors.add(new QueryParseException(name.toUpperCase() + " function expects 2 or 3 arguments", null, node.getLine(), node.getCharPositionInLine())); - break; + if (!(exprList instanceof QExprList) || exprList.childList().size() < 2 || exprList.childList().size() > 3) + { + _parseErrors.add(new QueryParseException(name.toUpperCase() + " function expects 2 or 3 arguments", null, node.getLine(), node.getCharPositionInLine())); + break label; + } + assert exprList.childList().size() == 2 || exprList.childList().size() == 3; + LinkedList args = new LinkedList<>(); + args.add(exprList.childList().get(0)); + args.add(exprList.childList().get(1)); + if (exprList.childList().size() == 3) + args.add(constantToStringNode(exprList.childList().get(2))); + exprList._replaceChildren(args); + if (args.size() == 3) + validateTimestampConstant(args.get(2)); } - assert exprList.childList().size() == 2 || exprList.childList().size() == 3; - LinkedList args = new LinkedList<>(); - args.add(exprList.childList().get(0)); - args.add(exprList.childList().get(1)); - if (exprList.childList().size() == 3) - args.add(constantToStringNode(exprList.childList().get(2))); - exprList._replaceChildren(args); - if (args.size() == 3) - validateTimestampConstant(args.get(2)); } // special case for table returning method - var isTableResultMethod = id.getTokenType() == EXPANCESTORSOF || id.getTokenType() == EXPDESCENDANTSOF || id.getTokenType() == EXPLINEAGEOF; + var isTableResultMethod = id.getTokenType() == SqlBaseParser.EXPANCESTORSOF || id.getTokenType() == SqlBaseParser.EXPDESCENDANTSOF || id.getTokenType() == SqlBaseParser.EXPLINEAGEOF; if (!isTableResultMethod) { try @@ -1043,7 +1164,7 @@ else if (name.equals("age")) } break; } - case AGGREGATE: + case SqlBaseParser.AGGREGATE: { if (constExpr) return constError(node); @@ -1058,7 +1179,7 @@ else if (name.equals("age")) { boolean distinct = false; - if (children.size() > 1 && first(children) instanceof QDistinct) + if (children.size() > 1 && QNode.first(children) instanceof QDistinct) { children.removeFirst(); distinct = true; @@ -1069,13 +1190,13 @@ else if (name.equals("age")) } return qAggregate; } - case TIMESTAMP_LITERAL: - case DATE_LITERAL: + case SqlBaseParser.TIMESTAMP_LITERAL: + case SqlBaseParser.DATE_LITERAL: { - String s = LabKeySql.unquoteString(firstOrThrow(children).getTokenText()); + String s = LabKeySql.unquoteString(QNode.firstOrThrow(children).getTokenText()); try { - if (node.getType() == TIMESTAMP_LITERAL) + if (node.getType() == SqlBaseParser.TIMESTAMP_LITERAL) return new QTimestamp(node,new Timestamp(DateUtil.parseDateTime(s))); else return new QDate(node,new java.sql.Date(DateUtil.parseDate(s))); @@ -1086,7 +1207,7 @@ else if (name.equals("age")) return null; } } - case TABLE_PATH_SUBSTITUTION: + case SqlBaseParser.TABLE_PATH_SUBSTITUTION: { if (constExpr) return constError(node); if (children.size() != 3) @@ -1115,7 +1236,7 @@ else if (name.equals("age")) } return substituteModuleProperty(((QString) args.get(0)).getValue(), ((QString)args.get(1)).getValue()); } - case QUERY: + case SqlBaseParser.QUERY: { if (constExpr) return constError(node); QQuery query = (QQuery)qnode(node, children, false); @@ -1125,7 +1246,7 @@ else if (name.equals("age")) } return query; } - case RANGE: + case SqlBaseParser.RANGE: { if (constExpr) return constError(node); @@ -1654,7 +1775,7 @@ public void reportError(RecognitionException ex) } @Override - public void close() throws Exception + public void close() { _errors = null; setTokenStream(null);