Use dot-separated name resolution for relationship target

The string argument accepted as the first positional argument by the
:func:`.relationship` function when using the Declarative API is no longer
interpreted using the Python ``eval()`` function; instead, the name is dot
separated and the names are looked up directly in the name resolution
dictionary without treating the value as a Python expression.  However,
passing a string argument to the other :func:`.relationship` parameters
that necessarily must accept Python expressions will still use ``eval()``;
the documentation has been clarified to ensure that there is no ambiguity
that this is in use.

Fixes: #5238
Change-Id: Id802f403190adfab0ca034afe2214ba10fd9cfbb

7 files changed, 281 insertions, 41 deletions

diff --git a/doc/build/changelog/unreleased_13/5238.rst b/doc/build/changelog/unreleased_13/5238.rst
new file mode 100644
index 000000000..1fb54ebe7
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/5238.rst
@@ -0,0 +1,17 @@
+.. change::
+    :tags: bug, orm, declarative
+    :tickets: 5238
+
+    The string argument accepted as the first positional argument by the
+    :func:`.relationship` function when using the Declarative API is no longer
+    interpreted using the Python ``eval()`` function; instead, the name is dot
+    separated and the names are looked up directly in the name resolution
+    dictionary without treating the value as a Python expression.  However,
+    passing a string argument to the other :func:`.relationship` parameters
+    that necessarily must accept Python expressions will still use ``eval()``;
+    the documentation has been clarified to ensure that there is no ambiguity
+    that this is in use.
+
+    .. seealso::
+
+        :ref:`declarative_relationship_eval` - details on string evaluation
\ No newline at end of file
diff --git a/doc/build/orm/basic_relationships.rst b/doc/build/orm/basic_relationships.rst
index ace6f8d7e..0f098d4cd 100644
--- a/doc/build/orm/basic_relationships.rst
+++ b/doc/build/orm/basic_relationships.rst
@@ -240,6 +240,13 @@ is accepted as well, matching the name of the table as stored in ``Base.metadata
                         secondary="association",
                         backref="parents")
 
+.. warning:: When passed as a Python-evaluable string, the
+    :paramref:`.relationship.secondary` argument is interpreted using Python's
+    ``eval()`` function. **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. See
+    :ref:`declarative_relationship_eval` for details on declarative
+    evaluation of :func:`.relationship` arguments.
+
+
 .. _relationships_many_to_many_deletion:
 
 Deleting Rows from the Many to Many Table
diff --git a/doc/build/orm/extensions/declarative/relationships.rst b/doc/build/orm/extensions/declarative/relationships.rst
index 1763344c7..07b6ed5bf 100644
--- a/doc/build/orm/extensions/declarative/relationships.rst
+++ b/doc/build/orm/extensions/declarative/relationships.rst
@@ -44,13 +44,21 @@ class using them::
         user_id = Column(Integer, ForeignKey('users.id'))
         user = relationship(User, primaryjoin=user_id == User.id)
 
+.. _declarative_relationship_eval:
+
+Evaluation of relationship arguments
+=====================================
+
 In addition to the main argument for :func:`~sqlalchemy.orm.relationship`,
 other arguments which depend upon the columns present on an as-yet
-undefined class may also be specified as strings.  These strings are
-evaluated as Python expressions.  The full namespace available within
-this evaluation includes all classes mapped for this declarative base,
-as well as the contents of the ``sqlalchemy`` package, including
-expression functions like :func:`~sqlalchemy.sql.expression.desc` and
+undefined class may also be specified as strings.   For most of these
+arguments except that of the main argument, these strings are
+**evaluated as Python expressions using Python's built-in eval() function.**
+
+The full namespace available within this evaluation includes all classes mapped
+for this declarative base, as well as the contents of the ``sqlalchemy``
+package, including expression functions like
+:func:`~sqlalchemy.sql.expression.desc` and
 :attr:`~sqlalchemy.sql.expression.func`::
 
     class User(Base):
@@ -59,6 +67,37 @@ expression functions like :func:`~sqlalchemy.sql.expression.desc` and
                              order_by="desc(Address.email)",
                              primaryjoin="Address.user_id==User.id")
 
+.. warning::
+
+    The strings accepted by the following parameters:
+
+        :paramref:`.relationship.order_by`
+
+        :paramref:`.relationship.primaryjoin`
+
+        :paramref:`.relationship.secondaryjoin`
+
+        :paramref:`.relationship.secondary`
+
+        :paramref:`.relationship.remote_side`
+
+        :paramref:`.relationship.foreign_keys`
+
+        :paramref:`.relationship._user_defined_foreign_keys`
+
+    Are **evaluated as Python code expressions using eval().  DO NOT PASS
+    UNTRUSTED INPUT TO THESE ARGUMENTS.**
+
+    In addition, prior to version 1.3.16 of SQLAlchemy, the main
+    "argument" to :func:`.relationship` is also evaluated as Python
+    code.  **DO NOT PASS UNTRUSTED INPUT TO THIS ARGUMENT.**
+
+.. versionchanged:: 1.3.16
+
+    The string evaluation of the main "argument" no longer accepts an open
+    ended Python expression, instead only accepting a string class name
+    or dotted package-qualified name.
+
 For the case where more than one module contains a class of the same name,
 string class names can also be specified as module-qualified paths
 within any of these string expressions::
diff --git a/doc/build/orm/join_conditions.rst b/doc/build/orm/join_conditions.rst
index ef023feb4..a317c6ecc 100644
--- a/doc/build/orm/join_conditions.rst
+++ b/doc/build/orm/join_conditions.rst
@@ -96,6 +96,13 @@ one :class:`.Column` we need::
 
         billing_address = relationship("Address", foreign_keys="Customer.billing_address_id")
 
+.. warning:: When passed as a Python-evaluable string, the
+    :paramref:`.relationship.foreign_keys` argument is interpreted using Python's
+    ``eval()`` function. **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. See
+    :ref:`declarative_relationship_eval` for details on declarative
+    evaluation of :func:`.relationship` arguments.
+
+
 .. _relationship_primaryjoin:
 
 Specifying Alternate Join Conditions
@@ -138,12 +145,21 @@ load those ``Address`` objects which specify a city of "Boston"::
         state = Column(String)
         zip = Column(String)
 
-Within this string SQL expression, we made use of the :func:`.and_` conjunction construct to establish
-two distinct predicates for the join condition - joining both the ``User.id`` and
-``Address.user_id`` columns to each other, as well as limiting rows in ``Address``
-to just ``city='Boston'``.   When using Declarative, rudimentary SQL functions like
-:func:`.and_` are automatically available in the evaluated namespace of a string
-:func:`.relationship` argument.
+Within this string SQL expression, we made use of the :func:`.and_` conjunction
+construct to establish two distinct predicates for the join condition - joining
+both the ``User.id`` and ``Address.user_id`` columns to each other, as well as
+limiting rows in ``Address`` to just ``city='Boston'``.   When using
+Declarative, rudimentary SQL functions like :func:`.and_` are automatically
+available in the evaluated namespace of a string :func:`.relationship`
+argument.
+
+.. warning:: When passed as a Python-evaluable string, the
+    :paramref:`.relationship.primaryjoin` argument is interpreted using
+    Python's
+    ``eval()`` function. **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. See
+    :ref:`declarative_relationship_eval` for details on declarative
+    evaluation of :func:`.relationship` arguments.
+
 
 The custom criteria we use in a :paramref:`~.relationship.primaryjoin`
 is generally only significant when SQLAlchemy is rendering SQL in
@@ -557,6 +573,14 @@ use the string name of the table as it is present in the :class:`.MetaData`::
                             backref="left_nodes"
         )
 
+.. warning:: When passed as a Python-evaluable string, the
+    :paramref:`.relationship.primaryjoin` and
+    :paramref:`.relationship.secondaryjoin` arguments are interpreted using
+    Python's ``eval()`` function. **DO NOT PASS UNTRUSTED INPUT TO THESE
+    STRINGS**. See :ref:`declarative_relationship_eval` for details on
+    declarative evaluation of :func:`.relationship` arguments.
+
+
 A classical mapping situation here is similar, where ``node_to_node`` can be joined
 to ``node.c.id``::
 
diff --git a/lib/sqlalchemy/ext/declarative/clsregistry.py b/lib/sqlalchemy/ext/declarative/clsregistry.py
index 71594aae7..219c4ba2e 100644
--- a/lib/sqlalchemy/ext/declarative/clsregistry.py
+++ b/lib/sqlalchemy/ext/declarative/clsregistry.py
@@ -289,6 +289,38 @@ class _class_resolver(object):
 
         return self.fallback[key]
 
+    def _raise_for_name(self, name, err):
+        util.raise_(
+            exc.InvalidRequestError(
+                "When initializing mapper %s, expression %r failed to "
+                "locate a name (%r). If this is a class name, consider "
+                "adding this relationship() to the %r class after "
+                "both dependent classes have been defined."
+                % (self.prop.parent, self.arg, name, self.cls)
+            ),
+            from_=err,
+        )
+
+    def _resolve_name(self):
+        name = self.arg
+        d = self._dict
+        rval = None
+        try:
+            for token in name.split("."):
+                if rval is None:
+                    rval = d[token]
+                else:
+                    rval = getattr(rval, token)
+        except KeyError as err:
+            self._raise_for_name(name, err)
+        except NameError as n:
+            self._raise_for_name(n.args[0], n)
+        else:
+            if isinstance(rval, _GetColumns):
+                return rval.cls
+            else:
+                return rval
+
     def __call__(self):
         try:
             x = eval(self.arg, globals(), self._dict)
@@ -298,16 +330,7 @@ class _class_resolver(object):
             else:
                 return x
         except NameError as n:
-            util.raise_(
-                exc.InvalidRequestError(
-                    "When initializing mapper %s, expression %r failed to "
-                    "locate a name (%r). If this is a class name, consider "
-                    "adding this relationship() to the %r class after "
-                    "both dependent classes have been defined."
-                    % (self.prop.parent, self.arg, n.args[0], self.cls)
-                ),
-                from_=n,
-            )
+            self._raise_for_name(n.args[0], n)
 
 
 def _resolver(cls, prop):
@@ -320,16 +343,18 @@ def _resolver(cls, prop):
     def resolve_arg(arg):
         return _class_resolver(cls, prop, fallback, arg)
 
-    return resolve_arg
+    def resolve_name(arg):
+        return _class_resolver(cls, prop, fallback, arg)._resolve_name
+
+    return resolve_name, resolve_arg
 
 
 def _deferred_relationship(cls, prop):
 
     if isinstance(prop, RelationshipProperty):
-        resolve_arg = _resolver(cls, prop)
+        resolve_name, resolve_arg = _resolver(cls, prop)
 
         for attr in (
-            "argument",
             "order_by",
             "primaryjoin",
             "secondaryjoin",
@@ -341,6 +366,11 @@ def _deferred_relationship(cls, prop):
             if isinstance(v, util.string_types):
                 setattr(prop, attr, resolve_arg(v))
 
+        for attr in ("argument",):
+            v = getattr(prop, attr)
+            if isinstance(v, util.string_types):
+                setattr(prop, attr, resolve_name(v))
+
         if prop.backref and isinstance(prop.backref, tuple):
             key, kwargs = prop.backref
             for attr in (
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 8b7a4b549..c6f3bc30a 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -217,7 +217,19 @@ class RelationshipProperty(StrategizedProperty):
 
           :paramref:`~.relationship.argument` may also be passed as a callable
           function which is evaluated at mapper initialization time, and may
-          be passed as a Python-evaluable string when using Declarative.
+          be passed as a string name when using Declarative.
+
+          .. warning:: Prior to SQLAlchemy 1.3.16, this value is interpreted
+             using Python's ``eval()`` function.
+             **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**.
+             See :ref:`declarative_relationship_eval` for details on
+             declarative evaluation of :func:`.relationship` arguments.
+
+          .. versionchanged 1.3.16::
+
+             The string evaluation of the main "argument" no longer accepts an
+             open ended Python expression, instead only accepting a string
+             class name or dotted package-qualified name.
 
           .. seealso::
 
@@ -237,6 +249,12 @@ class RelationshipProperty(StrategizedProperty):
           present in the :class:`.MetaData` collection associated with the
           parent-mapped :class:`.Table`.
 
+          .. warning:: When passed as a Python-evaluable string, the
+             argument is interpreted using Python's ``eval()`` function.
+             **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**.
+             See :ref:`declarative_relationship_eval` for details on
+             declarative evaluation of :func:`.relationship` arguments.
+
           The :paramref:`~.relationship.secondary` keyword argument is
           typically applied in the case where the intermediary :class:`.Table`
           is not otherwise expressed in any direct class mapping. If the
@@ -482,6 +500,12 @@ class RelationshipProperty(StrategizedProperty):
           and may be passed as a Python-evaluable string when using
           Declarative.
 
+          .. warning:: When passed as a Python-evaluable string, the
+             argument is interpreted using Python's ``eval()`` function.
+             **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**.
+             See :ref:`declarative_relationship_eval` for details on
+             declarative evaluation of :func:`.relationship` arguments.
+
           .. seealso::
 
             :ref:`relationship_foreign_keys`
@@ -641,6 +665,12 @@ class RelationshipProperty(StrategizedProperty):
           function which is evaluated at mapper initialization time, and may
           be passed as a Python-evaluable string when using Declarative.
 
+          .. warning:: When passed as a Python-evaluable string, the
+             argument is interpreted using Python's ``eval()`` function.
+             **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**.
+             See :ref:`declarative_relationship_eval` for details on
+             declarative evaluation of :func:`.relationship` arguments.
+
         :param passive_deletes=False:
            Indicates loading behavior during delete operations.
 
@@ -733,6 +763,12 @@ class RelationshipProperty(StrategizedProperty):
           and may be passed as a Python-evaluable string when using
           Declarative.
 
+          .. warning:: When passed as a Python-evaluable string, the
+             argument is interpreted using Python's ``eval()`` function.
+             **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**.
+             See :ref:`declarative_relationship_eval` for details on
+             declarative evaluation of :func:`.relationship` arguments.
+
           .. seealso::
 
               :ref:`relationship_primaryjoin`
@@ -746,6 +782,12 @@ class RelationshipProperty(StrategizedProperty):
           and may be passed as a Python-evaluable string when using
           Declarative.
 
+          .. warning:: When passed as a Python-evaluable string, the
+             argument is interpreted using Python's ``eval()`` function.
+             **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**.
+             See :ref:`declarative_relationship_eval` for details on
+             declarative evaluation of :func:`.relationship` arguments.
+
           .. seealso::
 
             :ref:`self_referential` - in-depth explanation of how
@@ -780,6 +822,12 @@ class RelationshipProperty(StrategizedProperty):
           and may be passed as a Python-evaluable string when using
           Declarative.
 
+          .. warning:: When passed as a Python-evaluable string, the
+             argument is interpreted using Python's ``eval()`` function.
+             **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**.
+             See :ref:`declarative_relationship_eval` for details on
+             declarative evaluation of :func:`.relationship` arguments.
+
           .. seealso::
 
               :ref:`relationship_primaryjoin`
diff --git a/test/ext/declarative/test_clsregistry.py b/test/ext/declarative/test_clsregistry.py
index d61cc81c8..fbde544e4 100644
--- a/test/ext/declarative/test_clsregistry.py
+++ b/test/ext/declarative/test_clsregistry.py
@@ -49,13 +49,16 @@ class ClsRegistryTest(fixtures.TestBase):
         f2 = MockClass(base, "foo.alt.Foo")
         clsregistry.add_class("Foo", f1)
         clsregistry.add_class("Foo", f2)
-        resolver = clsregistry._resolver(f1, MockProp())
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
 
         gc_collect()
 
         is_(resolver("foo.bar.Foo")(), f1)
         is_(resolver("foo.alt.Foo")(), f2)
 
+        is_(name_resolver("foo.bar.Foo")(), f1)
+        is_(name_resolver("foo.alt.Foo")(), f2)
+
     def test_fragment_resolve(self):
         base = weakref.WeakValueDictionary()
         f1 = MockClass(base, "foo.bar.Foo")
@@ -64,13 +67,16 @@ class ClsRegistryTest(fixtures.TestBase):
         clsregistry.add_class("Foo", f1)
         clsregistry.add_class("Foo", f2)
         clsregistry.add_class("HoHo", f3)
-        resolver = clsregistry._resolver(f1, MockProp())
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
 
         gc_collect()
 
         is_(resolver("bar.Foo")(), f1)
         is_(resolver("alt.Foo")(), f2)
 
+        is_(name_resolver("bar.Foo")(), f1)
+        is_(name_resolver("alt.Foo")(), f2)
+
     def test_fragment_ambiguous(self):
         base = weakref.WeakValueDictionary()
         f1 = MockClass(base, "foo.bar.Foo")
@@ -79,7 +85,7 @@ class ClsRegistryTest(fixtures.TestBase):
         clsregistry.add_class("Foo", f1)
         clsregistry.add_class("Foo", f2)
         clsregistry.add_class("Foo", f3)
-        resolver = clsregistry._resolver(f1, MockProp())
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
 
         gc_collect()
 
@@ -91,6 +97,39 @@ class ClsRegistryTest(fixtures.TestBase):
             resolver("alt.Foo"),
         )
 
+        assert_raises_message(
+            exc.InvalidRequestError,
+            'Multiple classes found for path "alt.Foo" in the registry '
+            "of this declarative base. Please use a fully "
+            "module-qualified path.",
+            name_resolver("alt.Foo"),
+        )
+
+    def test_no_fns_in_name_resolve(self):
+        base = weakref.WeakValueDictionary()
+        f1 = MockClass(base, "foo.bar.Foo")
+        f2 = MockClass(base, "foo.alt.Foo")
+        clsregistry.add_class("Foo", f1)
+        clsregistry.add_class("Foo", f2)
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
+
+        gc_collect()
+
+        import sqlalchemy
+
+        is_(
+            resolver("__import__('sqlalchemy.util').util.EMPTY_SET")(),
+            sqlalchemy.util.EMPTY_SET,
+        )
+
+        assert_raises_message(
+            exc.InvalidRequestError,
+            r"When initializing mapper some_parent, expression "
+            r"\"__import__\('sqlalchemy.util'\).util.EMPTY_SET\" "
+            "failed to locate a name",
+            name_resolver("__import__('sqlalchemy.util').util.EMPTY_SET"),
+        )
+
     def test_resolve_dupe_by_name(self):
         base = weakref.WeakValueDictionary()
         f1 = MockClass(base, "foo.bar.Foo")
@@ -100,7 +139,7 @@ class ClsRegistryTest(fixtures.TestBase):
 
         gc_collect()
 
-        resolver = clsregistry._resolver(f1, MockProp())
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
         resolver = resolver("Foo")
         assert_raises_message(
             exc.InvalidRequestError,
@@ -110,6 +149,15 @@ class ClsRegistryTest(fixtures.TestBase):
             resolver,
         )
 
+        resolver = name_resolver("Foo")
+        assert_raises_message(
+            exc.InvalidRequestError,
+            'Multiple classes found for path "Foo" in the '
+            "registry of this declarative base. Please use a "
+            "fully module-qualified path.",
+            resolver,
+        )
+
     def test_dupe_classes_back_to_one(self):
         base = weakref.WeakValueDictionary()
         f1 = MockClass(base, "foo.bar.Foo")
@@ -121,9 +169,12 @@ class ClsRegistryTest(fixtures.TestBase):
         gc_collect()
 
         # registry restores itself to just the one class
-        resolver = clsregistry._resolver(f1, MockProp())
-        resolver = resolver("Foo")
-        is_(resolver(), f1)
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
+        f_resolver = resolver("Foo")
+        is_(f_resolver(), f1)
+
+        f_resolver = name_resolver("Foo")
+        is_(f_resolver(), f1)
 
     def test_dupe_classes_cleanout(self):
         # force this to maintain isolation between tests
@@ -156,13 +207,21 @@ class ClsRegistryTest(fixtures.TestBase):
 
         dupe_reg = base["Foo"]
         dupe_reg.contents = [lambda: None]
-        resolver = clsregistry._resolver(f1, MockProp())
-        resolver = resolver("Foo")
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
+        f_resolver = resolver("Foo")
         assert_raises_message(
             exc.InvalidRequestError,
             r"When initializing mapper some_parent, expression "
             r"'Foo' failed to locate a name \('Foo'\).",
-            resolver,
+            f_resolver,
+        )
+
+        f_resolver = name_resolver("Foo")
+        assert_raises_message(
+            exc.InvalidRequestError,
+            r"When initializing mapper some_parent, expression "
+            r"'Foo' failed to locate a name \('Foo'\).",
+            f_resolver,
         )
 
     def test_module_reg_cleanout_race(self):
@@ -175,14 +234,22 @@ class ClsRegistryTest(fixtures.TestBase):
         reg = base["_sa_module_registry"]
 
         mod_entry = reg["foo"]["bar"]
-        resolver = clsregistry._resolver(f1, MockProp())
-        resolver = resolver("foo")
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
+        f_resolver = resolver("foo")
         del mod_entry.contents["Foo"]
         assert_raises_message(
             AttributeError,
             "Module 'bar' has no mapped classes registered "
             "under the name 'Foo'",
-            lambda: resolver().bar.Foo,
+            lambda: f_resolver().bar.Foo,
+        )
+
+        f_resolver = name_resolver("foo")
+        assert_raises_message(
+            AttributeError,
+            "Module 'bar' has no mapped classes registered "
+            "under the name 'Foo'",
+            lambda: f_resolver().bar.Foo,
         )
 
     def test_module_reg_no_class(self):
@@ -191,13 +258,21 @@ class ClsRegistryTest(fixtures.TestBase):
         clsregistry.add_class("Foo", f1)
         reg = base["_sa_module_registry"]
         mod_entry = reg["foo"]["bar"]  # noqa
-        resolver = clsregistry._resolver(f1, MockProp())
-        resolver = resolver("foo")
+        name_resolver, resolver = clsregistry._resolver(f1, MockProp())
+        f_resolver = resolver("foo")
+        assert_raises_message(
+            AttributeError,
+            "Module 'bar' has no mapped classes registered "
+            "under the name 'Bat'",
+            lambda: f_resolver().bar.Bat,
+        )
+
+        f_resolver = name_resolver("foo")
         assert_raises_message(
             AttributeError,
             "Module 'bar' has no mapped classes registered "
             "under the name 'Bat'",
-            lambda: resolver().bar.Bat,
+            lambda: f_resolver().bar.Bat,
         )
 
     def test_module_reg_cleanout_two_sub(self):