Raise error if there is a relative path in `global.cylc[install]sourc…

…e dirs` (#4887)

* globalcfg: make clear when dense config loaded
* Raise err if relative path in `global.cylc[install]source dirs`
* Fix premature expansion of config

CHANGES.md

@@ -33,6 +33,11 @@ ones in. -->
 
 Fourth Release Candidate for Cylc 8 suitable for acceptance testing.
 
+### Enhancements
+
+[#4887](https://github.com/cylc/cylc-flow/pull/4887) - Disallow relative paths
+in `global.cylc[install]source dirs`.
+
 ### Fixes
 
 [#4881](https://github.com/cylc/cylc-flow/pull/4881) - Fix bug where commands

cylc/flow/cfgspec/globalcfg.py

@@ -16,8 +16,9 @@
 """Cylc site and user configuration file spec."""
 
 import os
+from pathlib import Path
 from sys import stderr
-from typing import List, Optional, Tuple, Any
+from typing import List, Optional, Tuple, Any, Union
 
 from contextlib import suppress
 from pkg_resources import parse_version
@@ -31,7 +32,11 @@
     ConfigNode as Conf,
     ParsecConfig,
 )
-from cylc.flow.parsec.exceptions import ParsecError, ItemNotFoundError
+from cylc.flow.parsec.exceptions import (
+    ParsecError,
+    ItemNotFoundError,
+    ValidationError,
+)
 from cylc.flow.parsec.upgrade import upgrader
 from cylc.flow.parsec.util import printcfg, expand_many_section
 from cylc.flow.parsec.validate import (
@@ -1450,7 +1455,7 @@ def __init__(self, *args, **kwargs) -> None:
         super().__init__(*args, **kwargs)
 
     @classmethod
-    def get_inst(cls, cached=True):
+    def get_inst(cls, cached: bool = True) -> 'GlobalConfig':
         """Return a GlobalConfig instance.
 
         Args:
@@ -1469,11 +1474,17 @@ def get_inst(cls, cached=True):
             cls._DEFAULT.load()
         return cls._DEFAULT
 
-    def _load(self, fname, conf_type):
-        if os.access(fname, os.F_OK | os.R_OK):
+    def _load(self, fname: Union[Path, str], conf_type: str) -> None:
+        if not os.access(fname, os.F_OK | os.R_OK):
+            return
+        try:
             self.loadcfg(fname, conf_type)
+            self._validate_source_dirs()
+        except ParsecError:
+            LOG.error(f'bad {conf_type} {fname}')
+            raise
 
-    def load(self):
+    def load(self) -> None:
         """Load or reload configuration from files."""
         self.sparse.clear()
         self.dense.clear()
@@ -1487,22 +1498,39 @@ def load(self):
             # Use default locations.
             for conf_type, conf_dir in self.conf_dir_hierarchy:
                 fname = os.path.join(conf_dir, self.CONF_BASENAME)
-                try:
-                    self._load(fname, conf_type)
-                except ParsecError:
-                    LOG.error(f'bad {conf_type} {fname}')
-                    raise
+                self._load(fname, conf_type)
 
         # Expand platforms needs to be performed first because it
         # manipulates the sparse config.
         self._expand_platforms()
+
+        # Flesh out with defaults
+        self.expand()
+
         self._set_default_editors()
         self._no_platform_group_name_overlap()
 
+    def _validate_source_dirs(self) -> None:
+        """Check source dirs are absolute paths."""
+        keys = ['install', 'source dirs']
+        try:
+            src_dirs: List[str] = self.get(keys, sparse=True)
+        except ItemNotFoundError:
+            return
+        for item in src_dirs:
+            path = Path(item)
+            # Do not expand user/env vars - it is ok if they don't exist
+            if not (
+                path.is_absolute() or path.parts[0].startswith(('~', '$'))
+            ):
+                raise ValidationError(
+                    keys, value=item, msg="must be an absolute path"
+                )
+
     def _set_default_editors(self):
         # default to $[G]EDITOR unless an editor is defined in the config
         # NOTE: use `or` to handle cases where an env var is set to ''
-        cfg = self.get()
+        cfg = self.get(sparse=False)
         if not cfg['editors']['terminal']:
             cfg['editors']['terminal'] = os.environ.get('EDITOR') or 'vi'
         if not cfg['editors']['gui']:

cylc/flow/context_node.py

@@ -15,6 +15,9 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 
+from typing import Optional
+
+
 class ContextNode():
     """A class for defining nested objects.
 
@@ -84,7 +87,7 @@ class ContextNode():
 
     DATA: dict = {}
 
-    def __init__(self, name):
+    def __init__(self, name: str):
         self.name = name
         self._parent = None
         self._children = None
@@ -123,15 +126,30 @@ def __iter__(self):
             return iter(self._children.values())
         return iter([])
 
-    def __contains__(self, name):
-        return name in self._children
+    def __contains__(self, name: str) -> bool:
+        return name in self._children  # type: ignore[operator]  # TODO
 
-    def __getitem__(self, name):
+    def __getitem__(self, name: str):
         if self._children:
             return self._children.__getitem__(name)
         raise TypeError('This is not a leaf node')
 
-    def __str__(self):
+    def get(self, *names: str):
+        """Retrieve the node given by the list of names.
+
+        Example:
+            >>> with ContextNode('a') as a:
+            ...     with ContextNode('b') as b:
+            ...          c = ContextNode('c')
+            >>> a.get('b', 'c')
+            a/b/c
+        """
+        node = self
+        for name in names:
+            node = node[name]
+        return node
+
+    def __str__(self) -> str:
         if self.is_root():
             fmt = self.ROOT_NAME_FMT
         elif not self.is_leaf():
@@ -143,7 +161,7 @@ def __str__(self):
             for key in self.__slots__
         })
 
-    def __repr__(self):
+    def __repr__(self) -> str:
         return self.SEP.join(
             [
                 str(node)
@@ -153,22 +171,12 @@ def __repr__(self):
             ]
         )
 
-    def is_root(self):
-        """Return True if this is a root node.
-
-        Returns:
-            bool
-
-        """
+    def is_root(self) -> bool:
+        """Return True if this is a root node."""
         return self._parent is None
 
-    def is_leaf(self):
-        """Return True if this is a leaf node.
-
-        Returns:
-            bool
-
-        """
+    def is_leaf(self) -> bool:
+        """Return True if this is a leaf node."""
         return self._children is None
 
     def parents(self):
@@ -192,14 +200,14 @@ def parents(self):
             yield pointer
             pointer = pointer._parent
 
-    def walk(self, depth=None, level=0):
+    def walk(self, depth: Optional[int] = None, _level: int = 0):
         """Walk the context tree starting at this node.
 
         Args:
-            depth (int):
+            depth:
                 The max depth below the current node to yield.
-            level (int):
-                For recursive use, do not specify.
+            _level:
+                (For recursive use, do not specify.)
 
         Yields:
             tuple - (level, node)
@@ -219,13 +227,13 @@ def walk(self, depth=None, level=0):
             [(0, a/b/c)]
 
         """
-        if depth is not None and level >= depth:
+        if depth is not None and _level >= depth:
             return
-        if level == 0:
+        if _level == 0:
             yield (0, self)
         for child in self:
-            yield (level + 1, child)
-            yield from child.walk(depth=depth, level=level + 1)
+            yield (_level + 1, child)
+            yield from child.walk(depth=depth, _level=_level + 1)
 
     def tree(self):
         """Return a string representation of the tree starting at this node.