Class types in containers: help option, known subclasses and set parsing; harden import path policy

CHANGELOG.rst

@@ -23,7 +23,12 @@ Added
   set of standard library paths that give code execution, e.g. ``os``,
   ``subprocess`` and ``pickle``, is denied by default, see
   :ref:`untrusted-configs` (`#959
-  <https://github.com/mauvilsa/jsonargparse/pull/959>`__).
+  <https://github.com/mauvilsa/jsonargparse/pull/959>`__, `#960
+  <https://github.com/mauvilsa/jsonargparse/pull/960>`__).
+- Class types nested in a ``tuple``, ``set``, ``frozenset`` or mapping, e.g.
+  ``dict[str, SomeBaseClass]``, now have a ``--*.help`` option, and subclasses
+  in any container are now included in the known subclasses shown in the help
+  (`#960 <https://github.com/mauvilsa/jsonargparse/pull/960>`__).
 
 Fixed
 ^^^^^
@@ -35,6 +40,16 @@ Fixed
 - Instance factory protocols with a ``__call__`` that takes no parameters
   instantiated the class instead of giving a factory (`#959
   <https://github.com/mauvilsa/jsonargparse/pull/959>`__).
+- ``Literal`` with arguments that are attributes of the class in whose body the
+  method is defined failed to resolve when annotations are postponed (`#960
+  <https://github.com/mauvilsa/jsonargparse/pull/960>`__).
+- ``set`` and ``frozenset`` of a class type failed to parse because subclass
+  specs are not hashable (`#960
+  <https://github.com/mauvilsa/jsonargparse/pull/960>`__).
+- Shell completion of a ``--*.help`` option gave wrong choices when the class
+  type is nested in a container or optional, e.g. ``builtins.NoneType`` for
+  ``Optional[list[SomeBaseClass]]`` (`#960
+  <https://github.com/mauvilsa/jsonargparse/pull/960>`__).
 
 Deprecated
 ^^^^^^^^^^

DOCUMENTATION.rst

@@ -561,7 +561,9 @@ Some notes about this support are:
   element position can have its own type and will be validated as such.
   ``tuple`` with ellipsis (``tuple[type, ...]``) is also supported. In command
   line arguments, config files and environment variables, tuples and sets are
-  represented as an array.
+  represented as an array. A ``set`` or ``frozenset`` of a class type is kept as
+  a list when parsing, since subclass specs are not hashable, and becomes a set
+  on :meth:`instantiate <.ArgumentParser.instantiate>`.
 
 - To set a value to ``None`` it is required to use ``null`` since this is how
   JSON/YAML defines it. To avoid confusion in the help, ``NoneType`` is
@@ -2399,14 +2401,20 @@ An entry denies or allows a dot import path and everything under it, so ``os``
 also denies ``os.system``. The most specific entry decides, which is why
 ``functools.partial`` above is allowed even though ``functools`` is denied by
 default. An entry given in both lists is allowed, so naming a default entry in
-``import_path_allowlist`` is how to stop denying it.
+``import_path_allowlist`` is how to stop denying it. The one exception is
+``jsonargparse`` itself, which is denied by default and not accepted in
+``import_path_allowlist``, since a value that names it would be able to call
+:func:`.set_parsing_settings` and thus change the policy that is checking it.
 
 An object is denied by where it is defined, not only by the path used to reach
 it. Modules commonly import others, e.g. ``import os``, so without this
 ``some.module.os.system`` would give the same object as the denied
 ``os.system``. This second check can only happen once the object is resolved, so
 it prevents the object from being used, unlike the check on the given path,
-which prevents the import from happening at all.
+which prevents the import from happening at all. An object that has no defining
+path of its own is denied by the callable it reaches, i.e. the bound function
+for a ``functools.partial`` and the defining class for an instance, e.g.
+``builtins.help`` is an instance of the ``_sitebuiltins._Helper`` class.
 
 Entries given are added to the ones denied by default, they don't replace them.
 For configs that are entirely untrusted, prefer denying everything and allowing
@@ -2425,6 +2433,16 @@ only what the application expects. The ``*`` entry is only accepted in
     _common.parsing_settings.clear()
     _common.parsing_settings.update(saved_import_path_settings)
 
+The denylist is not the only thing that limits what a config can reach. Type
+hints do as well, since a ``class_path`` is only accepted where the annotation
+allows one, and must name a subclass of the annotated type. The exceptions are
+``Any`` and ``object``, which accept a subclass spec of any class, see
+:ref:`sub-classes`. Setting ``instantiate_subclass_spec_in_any=False``, which is
+the default from v5.0.0, keeps these values as plain dicts, so nothing is
+imported or instantiated and the code that receives the dict decides what to do
+with it. The denylist still applies when ``validate_subclass_spec_in_any=True``,
+since validating a spec requires importing the class it names.
+
 .. note::
 
     A denylist is a mitigation, not a sandbox. A large enough set of installed
@@ -2433,6 +2451,15 @@ only what the application expects. The ``*`` entry is only accepted in
     Only ``*`` plus a narrow allowlist gives a bound on what a config can
     import.
 
+.. note::
+
+    The ``omegaconf`` parser modes, see :ref:`omegaconf-interpolation`, give a
+    config access to OmegaConf's resolvers, which the import path denylist does
+    not check. The built-in ``oc.env`` resolver reads environment variables, so
+    a value of ``${oc.env:AWS_SECRET_ACCESS_KEY}`` puts that variable's value
+    into the config, and the resolvers that the application registers are
+    equally reachable. Avoid these parser modes for untrusted configs.
+
 .. note::
 
     Until v5.0.0 a denied import path only gives a deprecation warning and the
@@ -3050,6 +3077,8 @@ instantiates ``Data`` first, then use the ``num_classes`` attribute to
 instantiate ``Model``.
 
 
+.. _omegaconf-interpolation:
+
 OmegaConf variable interpolation
 ================================
 

jsonargparse/_actions.py

@@ -570,7 +570,7 @@ def __init__(
             ValueError: If the parser parameter is invalid.
         """
         self._parser = parser
-        if not isinstance(self._parser, import_object("jsonargparse.ArgumentParser", check_path=False)):
+        if not isinstance(self._parser, __import__("jsonargparse").ArgumentParser):
             raise ValueError("Expected parser keyword argument to be an ArgumentParser.")
 
     @staticmethod

jsonargparse/_common.py

@@ -135,6 +135,10 @@ class ImportDenied(ImportError, ValueError):
 # that exposes them and both are needed, since a pure Python implementation is
 # used when the C one is unavailable.
 default_import_path_denylist = (
+    # Policy self-modification. Naming jsonargparse itself would give a value a
+    # way to change the settings that decide what values are allowed, which is
+    # why it is also not accepted in import_path_allowlist.
+    "jsonargparse",
     # Command and code execution. Instantiation is the execution, so a class
     # path plus init_args suffices to run arbitrary code or shell commands.
     "builtins.eval",
@@ -158,7 +162,11 @@ class ImportDenied(ImportError, ValueError):
     "timeit",
     "pdb",
     "bdb",
+    "builtins.breakpoint",
     "trace",
+    "cProfile",
+    "profile",
+    "doctest",
     # Untrusted deserialization. Loading attacker influenced bytes is execution,
     # and only a loader plus a file path is needed, not the payload itself.
     "pickle",
@@ -169,16 +177,22 @@ class ImportDenied(ImportError, ValueError):
     # Import machinery and package installation. Resolve or install code by name
     # at runtime, which would reopen everything the other groups deny.
     "importlib",
+    "_frozen_importlib",
+    "_frozen_importlib_external",
     "_imp",
     "pkgutil",
     "zipimport",
     "pydoc",
+    "builtins.help",
+    "unittest",
     "sys",
     "site",
     "pip",
+    "pkg_resources",
     "setuptools",
     "distutils",
     "venv",
+    "ensurepip",
     "sysconfig",
     # Callable adapters and reflection. Wrap or synthesize a callable so that the
     # call happens later in code that receives it, where no type check applies.
@@ -191,6 +205,11 @@ class ImportDenied(ImportError, ValueError):
     "ast",
     "py_compile",
     "compileall",
+    "builtins.getattr",
+    "builtins.setattr",
+    "builtins.delattr",
+    "builtins.vars",
+    "builtins.globals",
     # Filesystem and process lifetime. Destructive or disruptive instead of
     # executing, e.g. deleting trees, signals or deferring a call to exit.
     "shutil",
@@ -200,11 +219,17 @@ class ImportDenied(ImportError, ValueError):
     "_signal",
     "atexit",
     "gc",
+    "resource",
+    "faulthandler",
+    "builtins.exit",
+    "builtins.quit",
+    "_sitebuiltins",
     # File access. Opening a path for writing truncates or creates it, and the
     # archive and database openers do the same, so only a path is needed to
     # destroy or plant a file, not any payload. builtins.open and the io classes
     # are the primitives, the rest wrap them.
     "builtins.open",
+    "codecs.open",
     "io",
     "_io",
     "fileinput",
@@ -220,13 +245,15 @@ class ImportDenied(ImportError, ValueError):
     "logging.config",
     "logging.handlers",
     "logging.FileHandler",
+    "winreg",
     # Network and external launch. Exfiltration primitives and launching an
     # external program with an argument that the config decides. The client and
     # server modules connect or listen with a destination the config decides.
     "socket",
     "_socket",
     "ssl",
     "webbrowser",
+    "antigravity",
     "asyncio",
     "urllib",
     "http",
@@ -237,6 +264,7 @@ class ImportDenied(ImportError, ValueError):
     "nntplib",
     "telnetlib",
     "socketserver",
+    "xml",
     "xmlrpc",
     "wsgiref",
 )
@@ -272,6 +300,11 @@ def set_import_path_verdicts(denylist: list[str] | None, allowlist: list[str] |
                 raise ValueError("'*' is only accepted in import_path_denylist, to deny all not allowed paths.")
             if not isinstance(entry, str) or (entry != "*" and not all(p.isidentifier() for p in entry.split("."))):
                 raise ValueError(f"Expected import path entries to be dot import paths or '*', but got {entry!r}.")
+            if allowed and (entry == "jsonargparse" or entry.startswith("jsonargparse.")):
+                raise ValueError(
+                    "Import paths under 'jsonargparse' can't be allowed, since a value that names them "
+                    "would be able to change the import path policy itself."
+                )
             previous = verdicts.get(entry)
             verdicts[entry] = allowed
             if previous is None:
@@ -409,7 +442,9 @@ class when a value for a type that accepts any value, i.e. ``Any``,
         import_path_allowlist: Import paths that a value is allowed to name,
             taking precedence over the denylist for the same entry. The most
             specific entry decides, so ``functools.partial`` here allows only
-            that path out of a denied ``functools``.
+            that path out of a denied ``functools``. Paths under ``jsonargparse``
+            are not accepted, since a value that names them would be able to
+            change these settings.
     """
     # validate_defaults
     if isinstance(validate_defaults, bool):

jsonargparse/_completions.py

@@ -1,5 +1,4 @@
 import argparse
-import inspect
 import locale
 import os
 import re
@@ -27,6 +26,7 @@
     callable_origin_types,
     get_all_subclass_paths,
     get_callable_return_type,
+    get_help_types,
     get_typed_dict_key_type,
     get_typehint_origin,
     is_single_subclass_or_closed_type,
@@ -439,14 +439,10 @@ def add_subactions_and_get_subclass_choices(
 
 
 def get_help_class_choices(typehint) -> list[str]:
-    choices = []
-    if get_typehint_origin(typehint) == Union:
-        for subtype in typehint.__args__:
-            # a subscripted generic typed dict is a generic alias instead of a class
-            if inspect.isclass(subtype) or is_typed_dict(subtype):
-                choices.extend(get_help_class_choices(subtype))
-    elif is_typed_dict(typehint):
-        choices = [typehint.__name__]  # typed dicts don't accept a class path, only their name
-    else:
-        choices = get_all_subclass_paths(typehint)
+    choices: list[str] = []
+    for help_type in get_help_types(typehint) or []:
+        if is_typed_dict(help_type):
+            choices.append(help_type.__name__)  # typed dicts don't accept a class path, only their name
+        else:
+            choices += [p for p in get_all_subclass_paths(help_type) if p not in choices]
     return choices

jsonargparse/_postponed_annotations.py

@@ -4,11 +4,12 @@
 import sys
 import textwrap
 from dataclasses import is_dataclass
+from enum import Enum
 from importlib import import_module
 from types import UnionType
 from typing import Any, ForwardRef, TypeAlias, TypeVar, Union, get_type_hints
 
-from ._typehints import mapping_origin_types, sequence_origin_types, tuple_set_origin_types
+from ._typehints import literal_types, mapping_origin_types, sequence_origin_types, tuple_set_origin_types
 from ._util import get_typehint_origin
 
 _TRIGGER_MODULE_CACHE_MAXSIZE = 1024
@@ -304,11 +305,49 @@ class that defines it, so names defined there, e.g. a nested class, must be
     return {key: value for key, value in vars(owner).items() if is_type_like(value)}
 
 
+literal_value_types = (bool, bytes, int, str, Enum)
+
+
+def get_local_literal_values(owner: Any) -> dict:
+    """Returns the names defined in the body of a class whose value is valid as a Literal argument."""
+    if not inspect.isclass(owner):  # None when the method has no owner, i.e. a plain function
+        return {}
+    return {key: value for key, value in vars(owner).items() if isinstance(value, literal_value_types)}
+
+
+def is_literal_ast(node: ast.AST, aliases: dict) -> bool:
+    """Whether an ast node in the position of the value of a subscript is a Literal."""
+    value = None
+    if isinstance(node, ast.Name):
+        value = aliases.get(node.id)
+    elif isinstance(node, ast.Attribute) and isinstance(node.value, ast.Name):
+        value = getattr(aliases.get(node.value.id), node.attr, None)
+    return any(value is literal_type for literal_type in literal_types)
+
+
+def add_literal_value_aliases(arg_ast: ast.AST, aliases: dict, literal_values: dict) -> dict:
+    """Adds to the aliases the class attributes used as arguments of a Literal.
+
+    Strictly a Literal is invalid when its arguments are variables. Still, it is written, e.g.
+    ``Literal[TRAIN_SET, VALIDATION_SET]`` referencing attributes of the class in whose body the
+    method is defined. Since these are values and not types, they are only added for the names in
+    the subscript of a Literal, so that a class attribute never shadows a global in a type position.
+    """
+    names: set[str] = set()
+    if literal_values:
+        for node in ast.walk(arg_ast):
+            if isinstance(node, ast.Subscript) and is_literal_ast(node.value, aliases):
+                names.update(NamesVisitor().find(node.slice))
+    values = {name: literal_values[name] for name in names if name in literal_values}
+    return {**aliases, **values} if values else aliases
+
+
 def get_types(obj: Any, logger: logging.Logger | None = None, parent: Any = None) -> dict:
     global_vars = get_global_vars(obj, logger)
     # Locals are only needed for methods. For a class get_type_hints already uses as locals
     # the namespace of each of the bases that the annotations come from.
-    local_vars = None if inspect.isclass(obj) else get_local_vars(get_owner_class(obj) or parent)
+    owner = None if inspect.isclass(obj) else get_owner_class(obj) or parent
+    local_vars = None if inspect.isclass(obj) else get_local_vars(owner)
     try:
         types = get_type_hints(obj, global_vars, local_vars)
     except Exception as ex1:
@@ -337,11 +376,14 @@ def get_types(obj: Any, logger: logging.Logger | None = None, parent: Any = None
 
     arg_asts = [(a.arg, a.annotation) for a in node.args.args + node.args.kwonlyargs]  # type: ignore[union-attr]
 
+    literal_values = get_local_literal_values(owner)
+
     for name, annotation in arg_asts:
         if annotation and (name not in types or type_requires_eval(types[name])):
+            arg_aliases = add_literal_value_aliases(annotation, aliases, literal_values)
             try:
-                arg_type = get_arg_type(annotation, aliases)
-                types[name] = resolve_forward_refs(arg_type, aliases, logger)
+                arg_type = get_arg_type(annotation, arg_aliases)
+                types[name] = resolve_forward_refs(arg_type, arg_aliases, logger)
             except Exception as ex3:
                 types[name] = ex3
 

jsonargparse/_stubs_resolver.py

@@ -272,5 +272,5 @@ def get_stub_types(params, component, parent, logger) -> dict[str, Any] | None:
                     f"Failed to parse type stub for {component.__qualname__!r} parameter {name!r}", exc_info=ex
                 )
                 if name not in known_params:
-                    types[name] = inspect._empty  # pragma: no cover
+                    types[name] = inspect._empty
     return types