launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28277
[Merge] ~lgp171188/lpcraft:contain-config-file-path-under-project-dir into lpcraft:main
Guruprasad has proposed merging ~lgp171188/lpcraft:contain-config-file-path-under-project-dir into lpcraft:main.
Commit message:
Require the configuration file to be under the project directory
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lgp171188/lpcraft/+git/lpcraft/+merge/418035
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/lpcraft:contain-config-file-path-under-project-dir into lpcraft:main.
diff --git a/NEWS.rst b/NEWS.rst
index f4d3db9..769a6b1 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -12,6 +12,8 @@ Version history
- pre-commit: Add the ``pydocstyle`` hook to lint the docstrings.
+- Require the configuration file to be present under the project directory.
+
0.0.5 (2022-03-30)
====================
diff --git a/lpcraft/commands/clean.py b/lpcraft/commands/clean.py
index 5d67579..7464df1 100644
--- a/lpcraft/commands/clean.py
+++ b/lpcraft/commands/clean.py
@@ -17,18 +17,20 @@ def clean(args: Namespace) -> int:
config_path = getattr(args, "config", Path(".launchpad.yaml"))
Config.load(config_path)
- cwd = Path.cwd()
+ project_dir = Path.cwd()
emit.progress(
- f"Deleting the managed environments for the {cwd.name!r} project."
+ "Deleting the managed environments for the "
+ f"{project_dir.name!r} project."
)
provider = get_provider()
provider.ensure_provider_is_available()
provider.clean_project_environments(
- project_name=cwd.name, project_path=cwd
+ project_name=project_dir.name, project_path=project_dir
)
emit.message(
- f"Deleted the managed environments for the {cwd.name!r} project."
+ "Deleted the managed environments for the "
+ f"{project_dir.name!r} project."
)
return 0
diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index 23e9a04..389517e 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -21,23 +21,7 @@ from lpcraft.config import Config, Job, Output
from lpcraft.errors import CommandError
from lpcraft.plugin.manager import get_plugin_manager
from lpcraft.providers import Provider, get_provider
-from lpcraft.utils import get_host_architecture
-
-
-def _check_relative_path(path: PurePath, container: PurePath) -> PurePath:
- """Check that `path` does not escape `container`.
-
- Any symlinks in `path` must already have been resolved within the
- context of the container.
-
- :raises CommandError: if `path` is outside `container` when fully
- resolved.
- :return: A version of `path` relative to `container`.
- """
- try:
- return path.relative_to(container)
- except ValueError as e:
- raise CommandError(str(e), retcode=1)
+from lpcraft.utils import check_relative_path, get_host_architecture
def _list_files(instance: Executor, path: Path) -> List[PurePath]:
@@ -97,7 +81,7 @@ def _copy_output_paths(
# pattern as a whole first produces clearer error messages. We have
# to use os.path for this, as pathlib doesn't expose any equivalent
# of normpath.
- _check_relative_path(
+ check_relative_path(
PurePath(os.path.normpath(remote_cwd / path_pattern)),
remote_cwd,
)
@@ -122,7 +106,7 @@ def _copy_output_paths(
)
for path in sorted(resolved_paths):
- relative_path = _check_relative_path(path, remote_cwd)
+ relative_path = check_relative_path(path, remote_cwd)
destination = output_files / relative_path
destination.parent.mkdir(parents=True, exist_ok=True)
try:
@@ -144,7 +128,7 @@ def _copy_output_properties(
instance,
[remote_cwd / output.dynamic_properties],
)
- _check_relative_path(path, remote_cwd)
+ check_relative_path(path, remote_cwd)
dynamic_properties = dotenv_values(
stream=io.StringIO(
instance.execute_run(
@@ -346,7 +330,8 @@ def run_one(args: Namespace) -> int:
(This command is for use by Launchpad, and is subject to change.)
"""
- config = Config.load(args.config)
+ config_path = getattr(args, "config", Path(".launchpad.yaml"))
+ config = Config.load(config_path)
jobs = config.jobs.get(args.job, [])
if not jobs:
diff --git a/lpcraft/commands/tests/test_clean.py b/lpcraft/commands/tests/test_clean.py
index 5a11555..150676a 100644
--- a/lpcraft/commands/tests/test_clean.py
+++ b/lpcraft/commands/tests/test_clean.py
@@ -24,6 +24,31 @@ class TestClean(CommandBaseTestCase):
os.chdir(self.tmp_project_path)
self.addCleanup(os.chdir, cwd)
+ def test_config_file_not_under_project_directory(self):
+ paths = [
+ "/",
+ "/etc/init.d",
+ "../../foo",
+ "a/b/c/../../../../d",
+ ]
+
+ for path in paths:
+ config_file = f"{path}/config.yaml"
+ result = self.run_command(
+ "clean",
+ "-c",
+ config_file,
+ )
+ config_file_path = Path(config_file).resolve()
+ # The exception message differs between Python 3.8 and 3.9+.
+ # That is why the `result` object is manually tested below.
+ self.assertEqual(1, result.exit_code)
+ self.assertEqual(1, len(result.errors))
+ self.assertTrue(isinstance(result.errors[0], CommandError))
+ message = result.errors[0].args[0]
+ self.assertIn(str(config_file_path), message)
+ self.assertIn(str(self.tmp_project_path), message)
+
def test_missing_config_file(self):
result = self.run_command("clean")
diff --git a/lpcraft/commands/tests/test_run.py b/lpcraft/commands/tests/test_run.py
index cda2aaa..f9e5670 100644
--- a/lpcraft/commands/tests/test_run.py
+++ b/lpcraft/commands/tests/test_run.py
@@ -40,7 +40,7 @@ class LocalExecuteRun:
*,
cwd: Optional[Path] = None,
env: Optional[Dict[str, Optional[str]]] = None,
- **kwargs: Any
+ **kwargs: Any,
) -> "subprocess.CompletedProcess[AnyStr]":
run_kwargs = kwargs.copy()
run_kwargs["cwd"] = self.override_cwd
@@ -83,6 +83,32 @@ class RunBaseTestCase(CommandBaseTestCase):
class TestRun(RunBaseTestCase):
+ def test_config_file_not_under_project_directory(self):
+ paths = [
+ "/",
+ "/etc/init.d",
+ "../../foo",
+ "a/b/c/../../../../d",
+ ]
+
+ for path in paths:
+ config_file = f"{path}/config.yaml"
+ result = self.run_command(
+ "run",
+ "-c",
+ config_file,
+ )
+ config_file_path = Path(config_file).resolve()
+ config_file_path = Path(config_file).resolve()
+ # The exception message differs between Python 3.8 and 3.9+.
+ # That is why the `result` object is manually tested below.
+ self.assertEqual(1, result.exit_code)
+ self.assertEqual(1, len(result.errors))
+ self.assertTrue(isinstance(result.errors[0], CommandError))
+ message = result.errors[0].args[0]
+ self.assertIn(str(config_file_path), message)
+ self.assertIn(str(self.tmp_project_path), message)
+
def test_missing_config_file(self):
result = self.run_command("run")
@@ -1630,6 +1656,33 @@ class TestRun(RunBaseTestCase):
class TestRunOne(RunBaseTestCase):
+ def test_config_file_not_under_project_directory(self):
+ paths = [
+ "/",
+ "/etc/init.d",
+ "../../foo",
+ "a/b/c/../../../../d",
+ ]
+
+ for path in paths:
+ config_file = f"{path}/config.yaml"
+ result = self.run_command(
+ "run-one",
+ "-c",
+ config_file,
+ "test",
+ "0",
+ )
+ config_file_path = Path(config_file).resolve()
+ # The exception message differs between Python 3.8 and 3.9+.
+ # That is why the `result` object is manually tested below.
+ self.assertEqual(1, result.exit_code)
+ self.assertEqual(1, len(result.errors))
+ self.assertTrue(isinstance(result.errors[0], CommandError))
+ message = result.errors[0].args[0]
+ self.assertIn(str(config_file_path), message)
+ self.assertIn(str(self.tmp_project_path), message)
+
def test_missing_config_file(self):
result = self.run_command("run-one", "test", "0")
diff --git a/lpcraft/config.py b/lpcraft/config.py
index 0777dd3..6b9286b 100644
--- a/lpcraft/config.py
+++ b/lpcraft/config.py
@@ -10,7 +10,7 @@ from typing import Any, Dict, List, Optional, Union
import pydantic
from pydantic import StrictStr
-from lpcraft.utils import load_yaml
+from lpcraft.utils import check_relative_path, load_yaml
class _Identifier(pydantic.ConstrainedStr):
@@ -127,5 +127,11 @@ class Config(ModelConfigDefaults):
@classmethod
def load(cls, path: Path) -> "Config":
"""Load config from the indicated file name."""
+ # XXX lgp171188 2022-03-31: it is not a good idea to evaluate
+ # `Path.cwd()` in multiple places to determine the project directory.
+ # This could be made available as an attribute on the `Config` class
+ # instead.
+ project_dir = Path.cwd()
+ check_relative_path(path.resolve(), project_dir)
content = load_yaml(path)
return cls.parse_obj(content)
diff --git a/lpcraft/tests/test_config.py b/lpcraft/tests/test_config.py
index 96025b6..6867024 100644
--- a/lpcraft/tests/test_config.py
+++ b/lpcraft/tests/test_config.py
@@ -1,6 +1,7 @@
# Copyright 2021-2022 Canonical Ltd. This software is licensed under the
# GNU General Public License version 3 (see the file LICENSE).
+import os
from datetime import timedelta
from pathlib import Path
from textwrap import dedent
@@ -16,18 +17,37 @@ from testtools.matchers import (
)
from lpcraft.config import Config, OutputDistributeEnum
+from lpcraft.errors import CommandError
class TestConfig(TestCase):
def setUp(self):
super().setUp()
self.tempdir = Path(self.useFixture(TempDir()).path)
+ # `Path.cwd()` is assumed as the project directory.
+ # So switch to the created project directory.
+ os.chdir(self.tempdir)
def create_config(self, text):
path = self.tempdir / ".launchpad.yaml"
path.write_text(text)
return path
+ def test_load_config_not_under_project_dir(self):
+ paths_outside_project_dir = [
+ "/",
+ "/etc/init.d",
+ "../../foo",
+ "a/b/c/../../../../d",
+ ]
+ for path in paths_outside_project_dir:
+ config_file = Path(path) / "config.yaml"
+ self.assertRaises(
+ CommandError,
+ Config.load,
+ config_file,
+ )
+
def test_load(self):
path = self.create_config(
dedent(
diff --git a/lpcraft/tests/test_utils.py b/lpcraft/tests/test_utils.py
index 52b42e3..71ed1b2 100644
--- a/lpcraft/tests/test_utils.py
+++ b/lpcraft/tests/test_utils.py
@@ -10,8 +10,13 @@ from fixtures import TempDir
from systemfixtures import FakeProcesses
from testtools import TestCase
-from lpcraft.errors import ConfigurationError
-from lpcraft.utils import ask_user, get_host_architecture, load_yaml
+from lpcraft.errors import CommandError, ConfigurationError
+from lpcraft.utils import (
+ ask_user,
+ check_relative_path,
+ get_host_architecture,
+ load_yaml,
+)
class TestLoadYAML(TestCase):
@@ -145,3 +150,21 @@ class TestAskUser(TestCase):
mock_input.assert_called_once_with("prompt [y/N]: ")
mock_input.reset_mock()
+
+
+class TestCheckRelativePath(TestCase):
+ def test_relative_path_uncomputable(self):
+ paths = [
+ "/",
+ "/etc/init.d" "../../foo",
+ "a/b/c/../../../../d",
+ ]
+ container = Path(self.useFixture(TempDir()).path)
+
+ for path in paths:
+ self.assertRaises(
+ CommandError,
+ check_relative_path,
+ Path(path),
+ container,
+ )
diff --git a/lpcraft/utils.py b/lpcraft/utils.py
index a369cbd..2e907e1 100644
--- a/lpcraft/utils.py
+++ b/lpcraft/utils.py
@@ -1,4 +1,4 @@
-# Copyright 2021 Canonical Ltd. This software is licensed under the
+# Copyright 2021-22 Canonical Ltd. This software is licensed under the
# GNU General Public License version 3 (see the file LICENSE).
__all__ = [
@@ -10,12 +10,12 @@ __all__ = [
import subprocess
import sys
from functools import lru_cache
-from pathlib import Path
+from pathlib import Path, PurePath
from typing import Any, Dict
import yaml
-from lpcraft.errors import ConfigurationError
+from lpcraft.errors import CommandError, ConfigurationError
def load_yaml(path: Path) -> Dict[Any, Any]:
@@ -69,3 +69,19 @@ def ask_user(prompt: str, default: bool = False) -> bool:
elif reply[0] == "n":
return False
return default
+
+
+def check_relative_path(path: PurePath, container: PurePath) -> PurePath:
+ """Check that `path` does not escape `container`.
+
+ Any symlinks in `path` must already have been resolved within the
+ context of the container.
+
+ :raises CommandError: if `path` is outside `container` when fully
+ resolved.
+ :return: A version of `path` relative to `container`.
+ """
+ try:
+ return path.relative_to(container)
+ except ValueError as e:
+ raise CommandError(str(e))
Follow ups
