From: Mike Bayer Date: Mon, 4 Feb 2019 20:50:29 +0000 (-0500) Subject: Remove all remaining text() coercions and ensure identifiers are safe X-Git-Tag: rel_1_3_0b3~3^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=30307c4616ad67c01ddae2e1e8e34fabf6028414;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Remove all remaining text() coercions and ensure identifiers are safe Fully removed the behavior of strings passed directly as components of a :func:`.select` or :class:`.Query` object being coerced to :func:`.text` constructs automatically; the warning that has been emitted is now an ArgumentError or in the case of order_by() / group_by() a CompileError. This has emitted a warning since version 1.0 however its presence continues to create concerns for the potential of mis-use of this behavior. Note that public CVEs have been posted for order_by() / group_by() which are resolved by this commit: CVE-2019-7164 CVE-2019-7548 Added "SQL phrase validation" to key DDL phrases that are accepted as plain strings, including :paramref:`.ForeignKeyConstraint.on_delete`, :paramref:`.ForeignKeyConstraint.on_update`, :paramref:`.ExcludeConstraint.using`, :paramref:`.ForeignKeyConstraint.initially`, for areas where a series of SQL keywords only are expected.Any non-space characters that suggest the phrase would need to be quoted will raise a :class:`.CompileError`. This change is related to the series of changes committed as part of :ticket:`4481`. Fixed issue where using an uppercase name for an index type (e.g. GIST, BTREE, etc. ) or an EXCLUDE constraint would treat it as an identifier to be quoted, rather than rendering it as is. The new behavior converts these types to lowercase and ensures they contain only valid SQL characters. Quoting is applied to :class:`.Function` names, those which are usually but not necessarily generated from the :attr:`.sql.func` construct, at compile time if they contain illegal characters, such as spaces or punctuation. The names are as before treated as case insensitive however, meaning if the names contain uppercase or mixed case characters, that alone does not trigger quoting. The case insensitivity is currently maintained for backwards compatibility. Fixes: #4481 Fixes: #4473 Fixes: #4467 Change-Id: Ib22a27d62930e24702e2f0f7c74a0473385a08eb --- diff --git a/doc/build/changelog/migration_13.rst b/doc/build/changelog/migration_13.rst index 01fbccf22c..c133bd6620 100644 --- a/doc/build/changelog/migration_13.rst +++ b/doc/build/changelog/migration_13.rst @@ -1250,6 +1250,24 @@ considered, however this was too much verbosity). Key Changes - Core ================== +.. _change_4481: + +Coercion of string SQL fragments to text() fully removed +--------------------------------------------------------- + +The warnings that were first added in version 1.0, described at +:ref:`migration_2992`, have now been converted into exceptions. Continued +concerns have been raised regarding the automatic coercion of string fragments +passed to methods like :meth:`.Query.filter` and :meth:`.Select.order_by` being +converted to :func:`.text` constructs, even though this has emitted a warning. +In the case of :meth:`.Select.order_by`, :meth:`.Query.order_by`, +:meth:`.Select.group_by`, and :meth:`.Query.group_by`, a string label or column +name is still resolved into the corresponding expression construct, however if +the resolution fails, a :class:`.CompileError` is raised, thus preventing raw +SQL text from being rendered directly. + +:ticket:`4481` + .. _change_4393_threadlocal: "threadlocal" engine strategy deprecated diff --git a/doc/build/changelog/unreleased_13/4467.rst b/doc/build/changelog/unreleased_13/4467.rst new file mode 100644 index 0000000000..189423ab23 --- /dev/null +++ b/doc/build/changelog/unreleased_13/4467.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, sql + :tickets: 4467 + + Quoting is applied to :class:`.Function` names, those which are usually but + not necessarily generated from the :attr:`.sql.func` construct, at compile + time if they contain illegal characters, such as spaces or punctuation. The + names are as before treated as case insensitive however, meaning if the + names contain uppercase or mixed case characters, that alone does not + trigger quoting. The case insensitivity is currently maintained for + backwards compatibility. + diff --git a/doc/build/changelog/unreleased_13/4473.rst b/doc/build/changelog/unreleased_13/4473.rst new file mode 100644 index 0000000000..cdafe2b7e3 --- /dev/null +++ b/doc/build/changelog/unreleased_13/4473.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: bug, postgresql + :tickets: 4473 + + Fixed issue where using an uppercase name for an index type (e.g. GIST, + BTREE, etc. ) or an EXCLUDE constraint would treat it as an identifier to + be quoted, rather than rendering it as is. The new behavior converts these + types to lowercase and ensures they contain only valid SQL characters. diff --git a/doc/build/changelog/unreleased_13/4481.rst b/doc/build/changelog/unreleased_13/4481.rst new file mode 100644 index 0000000000..af88428c3b --- /dev/null +++ b/doc/build/changelog/unreleased_13/4481.rst @@ -0,0 +1,18 @@ +.. change:: + :tags: bug, sql + :tickets: 4481 + + Fully removed the behavior of strings passed directly as components of a + :func:`.select` or :class:`.Query` object being coerced to :func:`.text` + constructs automatically; the warning that has been emitted is now an + ArgumentError or in the case of order_by() / group_by() a CompileError. + This has emitted a warning since version 1.0 however its presence continues + to create concerns for the potential of mis-use of this behavior. + + Note that public CVEs have been posted for order_by() / group_by() which + are resolved by this commit: CVE-2019-7164 CVE-2019-7548 + + + .. seealso:: + + :ref:`change_4481` \ No newline at end of file diff --git a/doc/build/changelog/unreleased_13/ddl_validation.rst b/doc/build/changelog/unreleased_13/ddl_validation.rst new file mode 100644 index 0000000000..906710d319 --- /dev/null +++ b/doc/build/changelog/unreleased_13/ddl_validation.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, sql + :tickets: 4481 + + Added "SQL phrase validation" to key DDL phrases that are accepted as plain + strings, including :paramref:`.ForeignKeyConstraint.on_delete`, + :paramref:`.ForeignKeyConstraint.on_update`, + :paramref:`.ExcludeConstraint.using`, + :paramref:`.ForeignKeyConstraint.initially`, for areas where a series of SQL + keywords only are expected.Any non-space characters that suggest the phrase + would need to be quoted will raise a :class:`.CompileError`. This change + is related to the series of changes committed as part of :ticket:`4481`. diff --git a/lib/sqlalchemy/__init__.py b/lib/sqlalchemy/__init__.py index 5fda721fef..33a0e4af20 100644 --- a/lib/sqlalchemy/__init__.py +++ b/lib/sqlalchemy/__init__.py @@ -122,7 +122,7 @@ from .engine import create_engine # noqa nosort from .engine import engine_from_config # noqa nosort -__version__ = '1.3.0b3' +__version__ = "1.3.0b3" def __go(lcls): diff --git a/lib/sqlalchemy/dialects/postgresql/base.py b/lib/sqlalchemy/dialects/postgresql/base.py index 4004a2b9a5..4d302dabe7 100644 --- a/lib/sqlalchemy/dialects/postgresql/base.py +++ b/lib/sqlalchemy/dialects/postgresql/base.py @@ -948,6 +948,8 @@ except ImportError: _python_UUID = None +IDX_USING = re.compile(r"^(?:btree|hash|gist|gin|[\w_]+)$", re.I) + AUTOCOMMIT_REGEXP = re.compile( r"\s*(?:UPDATE|INSERT|CREATE|DELETE|DROP|ALTER|GRANT|REVOKE|" "IMPORT FOREIGN SCHEMA|REFRESH MATERIALIZED VIEW|TRUNCATE)", @@ -1908,7 +1910,10 @@ class PGDDLCompiler(compiler.DDLCompiler): using = index.dialect_options["postgresql"]["using"] if using: - text += "USING %s " % preparer.quote(using) + text += ( + "USING %s " + % self.preparer.validate_sql_phrase(using, IDX_USING).lower() + ) ops = index.dialect_options["postgresql"]["ops"] text += "(%s)" % ( @@ -1983,7 +1988,9 @@ class PGDDLCompiler(compiler.DDLCompiler): "%s WITH %s" % (self.sql_compiler.process(expr, **kw), op) ) text += "EXCLUDE USING %s (%s)" % ( - constraint.using, + self.preparer.validate_sql_phrase( + constraint.using, IDX_USING + ).lower(), ", ".join(elements), ) if constraint.where is not None: diff --git a/lib/sqlalchemy/dialects/postgresql/ext.py b/lib/sqlalchemy/dialects/postgresql/ext.py index 49b5e0ec0c..4260282397 100644 --- a/lib/sqlalchemy/dialects/postgresql/ext.py +++ b/lib/sqlalchemy/dialects/postgresql/ext.py @@ -91,6 +91,11 @@ class ExcludeConstraint(ColumnCollectionConstraint): where = None + @elements._document_text_coercion( + "where", + ":class:`.ExcludeConstraint`", + ":paramref:`.ExcludeConstraint.where`", + ) def __init__(self, *elements, **kw): r""" Create an :class:`.ExcludeConstraint` object. @@ -123,21 +128,15 @@ class ExcludeConstraint(ColumnCollectionConstraint): ) :param \*elements: + A sequence of two tuples of the form ``(column, operator)`` where "column" is a SQL expression element or a raw SQL string, most - typically a :class:`.Column` object, - and "operator" is a string containing the operator to use. - - .. note:: - - A plain string passed for the value of "column" is interpreted - as an arbitrary SQL expression; when passing a plain string, - any necessary quoting and escaping syntaxes must be applied - manually. In order to specify a column name when a - :class:`.Column` object is not available, while ensuring that - any necessary quoting rules take effect, an ad-hoc - :class:`.Column` or :func:`.sql.expression.column` object may - be used. + typically a :class:`.Column` object, and "operator" is a string + containing the operator to use. In order to specify a column name + when a :class:`.Column` object is not available, while ensuring + that any necessary quoting rules take effect, an ad-hoc + :class:`.Column` or :func:`.sql.expression.column` object should be + used. :param name: Optional, the in-database name of this constraint. @@ -159,12 +158,6 @@ class ExcludeConstraint(ColumnCollectionConstraint): If set, emit WHERE when issuing DDL for this constraint. - .. note:: - - A plain string passed here is interpreted as an arbitrary SQL - expression; when passing a plain string, any necessary quoting - and escaping syntaxes must be applied manually. - """ columns = [] render_exprs = [] @@ -184,11 +177,12 @@ class ExcludeConstraint(ColumnCollectionConstraint): # backwards compat self.operators[name] = operator - expr = expression._literal_as_text(expr) + expr = expression._literal_as_column(expr) render_exprs.append((expr, name, operator)) self._render_exprs = render_exprs + ColumnCollectionConstraint.__init__( self, *columns, @@ -199,7 +193,9 @@ class ExcludeConstraint(ColumnCollectionConstraint): self.using = kw.get("using", "gist") where = kw.get("where") if where is not None: - self.where = expression._literal_as_text(where) + self.where = expression._literal_as_text( + where, allow_coercion_to_text=True + ) def copy(self, **kw): elements = [(col, self.operators[col]) for col in self.columns.keys()] diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 9e52ef2083..6d4198a4ee 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1257,7 +1257,9 @@ class Session(_SessionClassMethods): in order to execute the statement. """ - clause = expression._literal_as_text(clause) + clause = expression._literal_as_text( + clause, allow_coercion_to_text=True + ) if bind is None: bind = self.get_bind(mapper, clause=clause, **kw) diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index b703c59f23..15ddd7d6fb 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -139,8 +139,16 @@ RESERVED_WORDS = set( ) LEGAL_CHARACTERS = re.compile(r"^[A-Z0-9_$]+$", re.I) +LEGAL_CHARACTERS_PLUS_SPACE = re.compile(r"^[A-Z0-9_ $]+$", re.I) ILLEGAL_INITIAL_CHARACTERS = {str(x) for x in range(0, 10)}.union(["$"]) +FK_ON_DELETE = re.compile( + r"^(?:RESTRICT|CASCADE|SET NULL|NO ACTION|SET DEFAULT)$", re.I +) +FK_ON_UPDATE = re.compile( + r"^(?:RESTRICT|CASCADE|SET NULL|NO ACTION|SET DEFAULT)$", re.I +) +FK_INITIALLY = re.compile(r"^(?:DEFERRED|IMMEDIATE)$", re.I) BIND_PARAMS = re.compile(r"(? instead", t.select, [const], diff --git a/test/sql/test_functions.py b/test/sql/test_functions.py index 83277ae986..5fb4bc2e4a 100644 --- a/test/sql/test_functions.py +++ b/test/sql/test_functions.py @@ -95,6 +95,43 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL): def test_underscores(self): self.assert_compile(func.if_(), "if()") + def test_underscores_packages(self): + self.assert_compile(func.foo_.bar_.if_(), "foo.bar.if()") + + def test_uppercase(self): + # for now, we need to keep case insensitivity + self.assert_compile(func.NOW(), "NOW()") + + def test_uppercase_packages(self): + # for now, we need to keep case insensitivity + self.assert_compile(func.FOO.BAR.NOW(), "FOO.BAR.NOW()") + + def test_mixed_case(self): + # for now, we need to keep case insensitivity + self.assert_compile(func.SomeFunction(), "SomeFunction()") + + def test_mixed_case_packages(self): + # for now, we need to keep case insensitivity + self.assert_compile( + func.Foo.Bar.SomeFunction(), "Foo.Bar.SomeFunction()" + ) + + def test_quote_special_chars(self): + # however we need to be quoting any other identifiers + self.assert_compile( + getattr(func, "im a function")(), '"im a function"()' + ) + + def test_quote_special_chars_packages(self): + # however we need to be quoting any other identifiers + self.assert_compile( + getattr( + getattr(getattr(func, "im foo package"), "im bar package"), + "im a function", + )(), + '"im foo package"."im bar package"."im a function"()', + ) + def test_generic_now(self): assert isinstance(func.now().type, sqltypes.DateTime) diff --git a/test/sql/test_text.py b/test/sql/test_text.py index 48302058d3..bcaf905fe6 100644 --- a/test/sql/test_text.py +++ b/test/sql/test_text.py @@ -22,10 +22,8 @@ from sqlalchemy.sql import column from sqlalchemy.sql import table from sqlalchemy.sql import util as sql_util from sqlalchemy.testing import assert_raises_message -from sqlalchemy.testing import assert_warnings from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import eq_ -from sqlalchemy.testing import expect_warnings from sqlalchemy.testing import fixtures from sqlalchemy.types import NullType @@ -574,16 +572,12 @@ class AsFromTest(fixtures.TestBase, AssertsCompiledSQL): eq_(set(t.element._bindparams), set(["bat", "foo", "bar"])) -class TextWarningsTest(fixtures.TestBase, AssertsCompiledSQL): +class TextErrorsTest(fixtures.TestBase, AssertsCompiledSQL): __dialect__ = "default" - def _test(self, fn, arg, offending_clause, expected): - with expect_warnings("Textual "): - stmt = fn(arg) - self.assert_compile(stmt, expected) - + def _test(self, fn, arg, offending_clause): assert_raises_message( - exc.SAWarning, + exc.ArgumentError, r"Textual (?:SQL|column|SQL FROM) expression %(stmt)r should be " r"explicitly declared (?:with|as) text\(%(stmt)r\)" % {"stmt": util.ellipses_string(offending_clause)}, @@ -592,45 +586,28 @@ class TextWarningsTest(fixtures.TestBase, AssertsCompiledSQL): ) def test_where(self): - self._test( - select([table1.c.myid]).where, - "myid == 5", - "myid == 5", - "SELECT mytable.myid FROM mytable WHERE myid == 5", - ) + self._test(select([table1.c.myid]).where, "myid == 5", "myid == 5") def test_column(self): - self._test(select, ["myid"], "myid", "SELECT myid") + self._test(select, ["myid"], "myid") def test_having(self): - self._test( - select([table1.c.myid]).having, - "myid == 5", - "myid == 5", - "SELECT mytable.myid FROM mytable HAVING myid == 5", - ) + self._test(select([table1.c.myid]).having, "myid == 5", "myid == 5") def test_from(self): - self._test( - select([table1.c.myid]).select_from, - "mytable", - "mytable", - "SELECT mytable.myid FROM mytable, mytable", # two FROMs - ) + self._test(select([table1.c.myid]).select_from, "mytable", "mytable") class OrderByLabelResolutionTest(fixtures.TestBase, AssertsCompiledSQL): __dialect__ = "default" - def _test_warning(self, stmt, offending_clause, expected): - with expect_warnings( - "Can't resolve label reference %r;" % offending_clause - ): - self.assert_compile(stmt, expected) + def _test_exception(self, stmt, offending_clause): assert_raises_message( - exc.SAWarning, - "Can't resolve label reference %r; converting to text" - % offending_clause, + exc.CompileError, + r"Can't resolve label reference for ORDER BY / GROUP BY. " + "Textual SQL " + "expression %r should be explicitly " + r"declared as text\(%r\)" % (offending_clause, offending_clause), stmt.compile, ) @@ -680,9 +657,7 @@ class OrderByLabelResolutionTest(fixtures.TestBase, AssertsCompiledSQL): def test_unresolvable_warning_order_by(self): stmt = select([table1.c.myid]).order_by("foobar") - self._test_warning( - stmt, "foobar", "SELECT mytable.myid FROM mytable ORDER BY foobar" - ) + self._test_exception(stmt, "foobar") def test_group_by_label(self): stmt = select([table1.c.myid.label("foo")]).group_by("foo") @@ -698,9 +673,7 @@ class OrderByLabelResolutionTest(fixtures.TestBase, AssertsCompiledSQL): def test_unresolvable_warning_group_by(self): stmt = select([table1.c.myid]).group_by("foobar") - self._test_warning( - stmt, "foobar", "SELECT mytable.myid FROM mytable GROUP BY foobar" - ) + self._test_exception(stmt, "foobar") def test_asc(self): stmt = select([table1.c.myid]).order_by(asc("name"), "description") @@ -799,23 +772,13 @@ class OrderByLabelResolutionTest(fixtures.TestBase, AssertsCompiledSQL): .order_by("myid", "t1name", "x") ) - def go(): - # the labels here are anonymized, so label naming - # can't catch these. - self.assert_compile( - s1, - "SELECT mytable_1.myid AS mytable_1_myid, " - "mytable_1.name AS name_1, foo(:foo_2) AS foo_1 " - "FROM mytable AS mytable_1 ORDER BY mytable_1.myid, t1name, x", - ) - - assert_warnings( - go, - [ - "Can't resolve label reference 't1name'", - "Can't resolve label reference 'x'", - ], - regex=True, + assert_raises_message( + exc.CompileError, + r"Can't resolve label reference for ORDER BY / GROUP BY. " + "Textual SQL " + "expression 't1name' should be explicitly " + r"declared as text\('t1name'\)", + s1.compile, ) def test_columnadapter_non_anonymized(self): diff --git a/test/test_schema.db b/test/test_schema.db new file mode 100644 index 0000000000..e69de29bb2