Repository: radiocosmology/alpenhorn

Issue:
Vetting acq and file names
With the CLI commands "acq create" and "file create" taking arbitrary strings that end up being used for acq or file names, there's going to need to be some vetting of the names.

The daemon implicitly sets some limits on the names to make them sanitary, by extracting them from a path to a file on a Node, but the CLI doesn't have that, so it should at least prohibit:

* starting or ending with `/`
* two or more `/` in a row (`/////`)
* `/./` or `/../` path elements

Anything else?  We definitely need to prohibit acq and file names from escaping from `node.root`.  Though I'm more fine with names which simply produce unaccessible paths (since those are fairly harmless, outside of the acq/file they're associated with being unaccessible to the daemon).

If the combination `acqname/filename` formed a concrete path we could use standard path normalisation, but they don't (becasue the CLI can be run somewhere without any data present), so we need to do something else.

The daemon always forms a path like: `node_root/acq_name/file_name` so leading/trailing slashes can't force it out-of-tree, but internal `/../` might.

We _do_ need to protect against having both an acq named `"acq"` and one named, say, `"acq/"`, since those would end up referring to the same files on disk.

Discussion:
ketiltrout: I've relaised there's a problem with this that I need to fix...
ketiltrout: OK, that wasn't as difficult as I thought it would be.  The issue in my original PR was there needs to be different checks for non-recursive and recursive `ArchiveFileImportRequest`s, which I have now implemented.

Patch:
diff --git a/alpenhorn/cli/acq/create.py b/alpenhorn/cli/acq/create.py
index 7e49b77..87efb92 100644
--- a/alpenhorn/cli/acq/create.py
+++ b/alpenhorn/cli/acq/create.py
@@ -3,6 +3,7 @@
 import click
 import peewee as pw
 
+from ...common.util import invalid_import_path
 from ...db import database_proxy, ArchiveAcq
 from ..options import cli_option
 from ..cli import echo
@@ -17,6 +18,11 @@ def create(name):
     another existing Acquisition.
     """
 
+    # Validate
+    rejection_reason = invalid_import_path(name)
+    if rejection_reason:
+        raise click.ClickException(f"invalid name: {rejection_reason}")
+
     with database_proxy.atomic():
         try:
             ArchiveAcq.get(name=name)
diff --git a/alpenhorn/cli/file/create.py b/alpenhorn/cli/file/create.py
index d091c76..a54ee64 100644
--- a/alpenhorn/cli/file/create.py
+++ b/alpenhorn/cli/file/create.py
@@ -4,7 +4,7 @@ import click
 import pathlib
 import peewee as pw
 
-from ...common.util import md5sum_file
+from ...common.util import md5sum_file, invalid_import_path
 from ...db import (
     ArchiveAcq,
     ArchiveFile,
@@ -91,6 +91,11 @@ def create(name, acq_name, from_file, md5, prefix, size):
     if size is not None and size < 0:
         raise click.ClickException(f"negative file size.")
 
+    # Validate
+    rejection_reason = invalid_import_path(name)
+    if rejection_reason:
+        raise click.ClickException(f"invalid name: {rejection_reason}")
+
     validate_md5(md5)
 
     # Scan a file, if requested
diff --git a/alpenhorn/common/util.py b/alpenhorn/common/util.py
index 905b752..119e9f5 100644
--- a/alpenhorn/common/util.py
+++ b/alpenhorn/common/util.py
@@ -313,3 +313,47 @@ def pretty_deltat(seconds: float) -> str:
 
     # For short durations, include tenths of a second
     return f"{seconds:.1f}s"
+
+
+def invalid_import_path(name: str) -> str | None:
+    """Is `name` invalid as an import path?
+
+    i.e., can `name` be used as an ArchiveAcq or
+    ArchiveFile name (or both, combined).
+
+    Returns
+    -------
+    rejection_reason: str or None
+        A string describing why `name` was rejected,
+        or None, if the name was valid.
+    """
+
+    # Can't be the null string
+    if name == "":
+        return "empty path"
+
+    # Can't simply be "." or ".."
+    if name == "." or name == "..":
+        return "invalid path"
+
+    # Can't start with "/" or "./" or "../"
+    if name.startswith("/") or name.startswith("./") or name.startswith("../"):
+        return "invalid start"
+
+    # Can't end with "/" or "/." or "/.."
+    if name.endswith("/") or name.endswith("/.") or name.endswith("/.."):
+        return "invalid end"
+
+    # Can't have multiple "/" in a row
+    if "//" in name:
+        return "repeated /"
+
+    # Can't have internal "/./"
+    if "/./" in name:
+        return 'invalid path element "."'
+
+    # Can't have internal "/../"
+    if "/../" in name:
+        return 'invalid path element ".."'
+
+    return None
diff --git a/alpenhorn/daemon/auto_import.py b/alpenhorn/daemon/auto_import.py
index a25852d..2241324 100644
--- a/alpenhorn/daemon/auto_import.py
+++ b/alpenhorn/daemon/auto_import.py
@@ -15,6 +15,7 @@ from watchdog.events import FileSystemEventHandler
 
 from .. import db
 from ..common import config, extensions
+from ..common.util import invalid_import_path
 from ..db import (
     ArchiveAcq,
     ArchiveFile,
@@ -173,6 +174,15 @@ def _import_file(
             req.complete()
         return
 
+    # Vet acq_name from extension
+    rejection_reason = invalid_import_path(str(acq_name))
+    if rejection_reason:
+        log.warning(f'Rejecting invalid acq path "{acq_name}": {rejection_reason}')
+        if req:
+            log.info(f"Completed import request #{req.id}.")
+            req.complete()
+        return
+
     file_name = path.relative_to(acq_name)
 
     # If a copy already exists, we're done
diff --git a/alpenhorn/daemon/update.py b/alpenhorn/daemon/update.py
index 08c2aed..9d71d6b 100644
--- a/alpenhorn/daemon/update.py
+++ b/alpenhorn/daemon/update.py
@@ -491,7 +491,29 @@ class UpdateableNode(updateable_base):
                 continue
 
             if req.recurse:
-                # If recursion was requested, run a scan on the path
+                # Ensure the base directory we want to scan is in-tree
+                fullpath = pathlib.Path(self.db.root, path)
+                try:
+                    fullpath = fullpath.resolve(strict=True)
+                except OSError as e:
+                    log.warning(
+                        f"Ignoring import request of unresolvable scan path: {fullpath}"
+                    )
+                    req.complete()
+                    continue
+
+                # Recompute the relative path after resolution, or skip scan if we're
+                # now out-of-tree
+                try:
+                    path = fullpath.relative_to(self.db.root)
+                except ValueError:
+                    log.warning(
+                        f"Ignoring import request of out-of-tree scan path: {fullpath}"
+                    )
+                    req.complete()
+                    continue
+
+                # Run scan
                 Task(
                     func=auto_import.scan,
                     queue=self._queue,
@@ -500,7 +522,17 @@ class UpdateableNode(updateable_base):
                     name=f'Scan "{path}" on {self.name}',
                 )
             else:
-                # Otherwise, try to directly import the path
+                # Check that the import path is valid
+                rejection_reason = util.invalid_import_path(req.path)
+                if rejection_reason:
+                    log.warning(
+                        f'Ignoring request for import of invalid path "{req.path}": '
+                        + rejection_reason
+                    )
+                    req.complete()
+                    continue
+
+                # Try to directly import the path
                 auto_import.import_file(self, self._queue, req.path, req.register, req)
 
     def update(self) -> None:


Test patch:
diff --git a/tests/cli/acq/test_create.py b/tests/cli/acq/test_create.py
index 3794d59..ad50a09 100644
--- a/tests/cli/acq/test_create.py
+++ b/tests/cli/acq/test_create.py
@@ -14,6 +14,15 @@ def test_create(clidb, cli):
     ArchiveAcq.get(name="TEST")
 
 
+def test_bad_name(clidb, cli):
+    """Test rejection of invalid name."""
+
+    cli(1, ["acq", "create", "/test/"])
+
+    # No acq was created
+    assert ArchiveAcq.select().count() == 0
+
+
 def test_create_existing(clidb, cli):
     """Test creating an acq that already exists."""
 
diff --git a/tests/cli/file/test_create.py b/tests/cli/file/test_create.py
index cc179bf..e625764 100644
--- a/tests/cli/file/test_create.py
+++ b/tests/cli/file/test_create.py
@@ -16,6 +16,24 @@ def test_no_data(clidb, cli):
     cli(2, ["file", "create", "name", "Acq", "--size=3"])
 
 
+def test_bad_name(clidb, cli):
+    """Test an invalid file name."""
+
+    ArchiveAcq.create(name="Acq")
+
+    cli(
+        1,
+        [
+            "file",
+            "create",
+            "name/../name",
+            "Acq",
+            "--md5=0123456789ABCDEF0123456789ABCDEF",
+            "--size=3",
+        ],
+    )
+
+
 def test_negative_size(clidb, cli):
     """Test a negative size."""
 
diff --git a/tests/common/test_util.py b/tests/common/test_util.py
index 77f3817..697ad62 100644
--- a/tests/common/test_util.py
+++ b/tests/common/test_util.py
@@ -127,3 +127,33 @@ def test_pretty_deltat():
     assert util.pretty_deltat(0.01) == "0.0s"
     assert util.pretty_deltat(0) == "0.0s"
     assert util.pretty_deltat(-1) == "-1.0s"
+
+
+def test_invalid_import_path():
+    """Test invalid_import_path"""
+
+    # Explicitly forbidden names
+    assert util.invalid_import_path("") is not None
+    assert util.invalid_import_path(".") is not None
+    assert util.invalid_import_path("..") is not None
+
+    # Forbidden starts
+    assert util.invalid_import_path("/name") is not None
+    assert util.invalid_import_path("./name") is not None
+    assert util.invalid_import_path("../name") is not None
+
+    # Forbidden ends
+    assert util.invalid_import_path("name/") is not None
+    assert util.invalid_import_path("name/.") is not None
+    assert util.invalid_import_path("name/..") is not None
+
+    # Forbidden middles
+    assert util.invalid_import_path("name//name") is not None
+    assert util.invalid_import_path("name///name") is not None
+    assert util.invalid_import_path("name/./name") is not None
+    assert util.invalid_import_path("name/../name") is not None
+
+    # These are fine
+    assert util.invalid_import_path("name") is None
+    assert util.invalid_import_path("name/name") is None
+    assert util.invalid_import_path("name/.../name") is None
diff --git a/tests/daemon/test_auto_import.py b/tests/daemon/test_auto_import.py
index 9e2a158..676e9d0 100644
--- a/tests/daemon/test_auto_import.py
+++ b/tests/daemon/test_auto_import.py
@@ -104,6 +104,24 @@ def test_import_file_no_detect(dbtables, unode):
         ArchiveAcq.get(name="acq")
 
 
+def test_import_file_invalid_acqname(dbtables, unode):
+    """Test invalid acq_name from import_detect."""
+
+    with patch(
+        "alpenhorn.common.extensions._id_ext", [lambda path, node: ("acq/", None)]
+    ):
+        with pytest.raises(StopIteration):
+            next(
+                auto_import._import_file(
+                    None, unode, pathlib.PurePath("acq/file"), True, None
+                )
+            )
+
+    # No acq has been added
+    with pytest.raises(pw.DoesNotExist):
+        ArchiveAcq.get(name="acq")
+
+
 def test_import_file_locked(xfs, dbtables, unode):
     """Test bad file in _import_file()"""
 
diff --git a/tests/daemon/test_update_node.py b/tests/daemon/test_update_node.py
index 7c0704d..2cd3db1 100644
--- a/tests/daemon/test_update_node.py
+++ b/tests/daemon/test_update_node.py
@@ -514,6 +514,52 @@ def test_update_import_absolute(unode, queue, archivefileimportrequest):
     assert ArchiveFileCopy.select().count() == 0
 
 
+def test_update_import_invalid_path(unode, queue, archivefileimportrequest):
+    """update_import() should ignore invalid paths."""
+
+    afir = archivefileimportrequest(path="./path/..", node=unode.db)
+
+    unode.update_import()
+
+    # Nothing queued
+    assert queue.qsize == 0
+
+    # Request is completed
+    assert ArchiveFileImportRequest.get(id=afir.id).completed == 1
+
+
+def test_update_import_outtree_scan(unode, xfs, queue, archivefileimportrequest):
+    """Test rejection of out-of-tree scan paths from import requests"""
+
+    xfs.create_dir("/node/import")
+
+    afir = archivefileimportrequest(path="..", node=unode.db, recurse=True)
+
+    unode.update_import()
+
+    # Nothing queued
+    assert queue.qsize == 0
+
+    # Request is completed
+    assert ArchiveFileImportRequest.get(id=afir.id).completed == 1
+
+
+def test_update_import_unresolve_scan(unode, xfs, queue, archivefileimportrequest):
+    """Test rejection of unresolvable scan paths from import requests"""
+
+    xfs.create_dir("/node/import")
+
+    afir = archivefileimportrequest(path="path/subpath/..", node=unode.db, recurse=True)
+
+    unode.update_import()
+
+    # Nothing queued
+    assert queue.qsize == 0
+
+    # Request is completed
+    assert ArchiveFileImportRequest.get(id=afir.id).completed == 1
+
+
 def test_update_import_no_recurse(unode, queue, archivefileimportrequest):
     """Test update_import() with a non-recursive import request."""
 
@@ -532,9 +578,11 @@ def test_update_import_no_recurse(unode, queue, archivefileimportrequest):
     mock.assert_called_with(unode, queue, "import/path", True, afir)
 
 
-def test_update_import_scan(unode, queue, archivefileimportrequest):
+def test_update_import_scan(unode, xfs, queue, archivefileimportrequest):
     """Test update_import() with a recursive import request."""
 
+    xfs.create_dir("/node/import/path")
+
     afir = archivefileimportrequest(
         path="import/path", node=unode.db, recurse=True, register=True
     )