From 0205f29285f547e3f87e0ad2036e1e2712284dcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Wed, 15 Apr 2026 23:21:19 +0800 Subject: [PATCH 1/2] fix: validate project name to reject path separators in apm init apm init passed the project_name argument directly into Path(), causing '/' and '\' to be silently interpreted as filesystem path separators. On Windows this produced a low-level WinError instead of a clear message. Add _validate_project_name() in _helpers.py that rejects names containing '/' or '\', call it in init() before any Path usage, and re-prompt in interactive mode when the user enters an invalid name. Fixes #723 Closes #718 (discussion) --- src/apm_cli/commands/_helpers.py | 11 +++++ src/apm_cli/commands/init.py | 31 ++++++++++-- tests/unit/test_init_command.py | 84 ++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 3 deletions(-) diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 6f220455f..c7c1f03dc 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -414,6 +414,17 @@ def _validate_plugin_name(name): return bool(re.match(r"^[a-z][a-z0-9-]{0,63}$", name)) +def _validate_project_name(name): + """Validate that a project name does not contain path separators. + + Project names are used directly as directory names and must not contain + '/' or '\\' to prevent unintended filesystem path traversal. + + Returns True if valid, False otherwise. + """ + return "/" not in name and "\\" not in name + + def _create_plugin_json(config): """Create plugin.json file with package metadata. diff --git a/src/apm_cli/commands/init.py b/src/apm_cli/commands/init.py index 5d032d12f..d68c6a07e 100644 --- a/src/apm_cli/commands/init.py +++ b/src/apm_cli/commands/init.py @@ -22,6 +22,7 @@ _lazy_confirm, _rich_blank_line, _validate_plugin_name, + _validate_project_name, ) @@ -47,6 +48,14 @@ def init(ctx, project_name, yes, plugin, verbose): if project_name == ".": project_name = None + # Reject names containing path separators before any filesystem use + if project_name and not _validate_project_name(project_name): + logger.error( + f"Invalid project name '{project_name}': " + "project names must not contain path separators ('/' or '\\\\')." + ) + sys.exit(1) + # Determine project directory and name if project_name: project_dir = Path(project_name) @@ -163,7 +172,7 @@ def init(ctx, project_name, yes, plugin, verbose): def _interactive_project_setup(default_name, logger): """Interactive setup for new APM projects with auto-detection.""" - from ._helpers import _auto_detect_author, _auto_detect_description + from ._helpers import _auto_detect_author, _auto_detect_description, _validate_project_name # Get auto-detected defaults auto_author = _auto_detect_author() @@ -179,7 +188,15 @@ def _interactive_project_setup(default_name, logger): console.print("\n[info]Setting up your APM project...[/info]") console.print("[muted]Press ^C at any time to quit.[/muted]\n") - name = Prompt.ask("Project name", default=default_name).strip() + while True: + name = Prompt.ask("Project name", default=default_name).strip() + if _validate_project_name(name): + break + console.print( + f"[error]Invalid project name '{name}': " + "project names must not contain path separators ('/' or '\\\\').[/error]" + ) + version = Prompt.ask("Version", default="1.0.0").strip() description = Prompt.ask("Description", default=auto_description).strip() author = Prompt.ask("Author", default=auto_author).strip() @@ -201,7 +218,15 @@ def _interactive_project_setup(default_name, logger): logger.progress("Setting up your APM project...") logger.progress("Press ^C at any time to quit.") - name = click.prompt("Project name", default=default_name).strip() + while True: + name = click.prompt("Project name", default=default_name).strip() + if _validate_project_name(name): + break + click.echo( + f"{ERROR}Invalid project name '{name}': " + "project names must not contain path separators ('/' or '\\\\').{RESET}" + ) + version = click.prompt("Version", default="1.0.0").strip() description = click.prompt("Description", default=auto_description).strip() author = click.prompt("Author", default=auto_author).strip() diff --git a/tests/unit/test_init_command.py b/tests/unit/test_init_command.py index 6da5accb6..9362ffd6e 100644 --- a/tests/unit/test_init_command.py +++ b/tests/unit/test_init_command.py @@ -319,3 +319,87 @@ def test_invalid_names(self): assert _validate_plugin_name("-plugin") is False assert _validate_plugin_name("a" * 65) is False assert _validate_plugin_name("My-Plugin") is False + + +class TestProjectNameValidation: + """Unit tests for _validate_project_name helper.""" + + def test_valid_names(self): + from apm_cli.commands._helpers import _validate_project_name + + assert _validate_project_name("myproject") is True + assert _validate_project_name("my-project") is True + assert _validate_project_name("my_project") is True + assert _validate_project_name("Project123") is True + assert _validate_project_name("4") is True + assert _validate_project_name(".") is True + + def test_invalid_forward_slash(self): + from apm_cli.commands._helpers import _validate_project_name + + assert _validate_project_name("4/15") is False + assert _validate_project_name("a/b") is False + assert _validate_project_name("/leading") is False + assert _validate_project_name("trailing/") is False + + def test_invalid_backslash(self): + from apm_cli.commands._helpers import _validate_project_name + + assert _validate_project_name("a\\b") is False + assert _validate_project_name("\\leading") is False + assert _validate_project_name("trailing\\") is False + + +class TestInitProjectNameValidation: + """Integration tests: apm init rejects project names with path separators.""" + + def setup_method(self): + self.runner = CliRunner() + try: + self.original_dir = os.getcwd() + except FileNotFoundError: + self.original_dir = str(Path(__file__).parent.parent.parent) + os.chdir(self.original_dir) + + def teardown_method(self): + try: + os.chdir(self.original_dir) + except (FileNotFoundError, OSError): + os.chdir(str(Path(__file__).parent.parent.parent)) + + def test_init_rejects_forward_slash_in_name(self): + """apm init 4/15 must fail with a clear error, not a WinError.""" + with tempfile.TemporaryDirectory() as tmp_dir: + os.chdir(tmp_dir) + try: + result = self.runner.invoke(cli, ["init", "4/15", "--yes"]) + assert result.exit_code != 0 + assert "Invalid project name" in result.output + assert "4/15" in result.output + # No directory should be created + assert not Path("4").exists() + finally: + os.chdir(self.original_dir) + + def test_init_rejects_backslash_in_name(self): + """apm init with a backslash in the name must fail with a clear error.""" + with tempfile.TemporaryDirectory() as tmp_dir: + os.chdir(tmp_dir) + try: + result = self.runner.invoke(cli, ["init", "a\\b", "--yes"]) + assert result.exit_code != 0 + assert "Invalid project name" in result.output + assert "a\\b" in result.output + finally: + os.chdir(self.original_dir) + + def test_init_accepts_plain_name(self): + """apm init with a simple name still works normally.""" + with tempfile.TemporaryDirectory() as tmp_dir: + os.chdir(tmp_dir) + try: + result = self.runner.invoke(cli, ["init", "my-project", "--yes"]) + assert result.exit_code == 0 + assert (Path(tmp_dir) / "my-project" / "apm.yml").exists() + finally: + os.chdir(self.original_dir) From a50725c3317b0369783b33a6e8c336aac2758019 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Thu, 16 Apr 2026 12:51:56 +0800 Subject: [PATCH 2/2] fix: address review feedback on project name validation - Fix f-string bug: second string in click.echo was not an f-string so {RESET} was printed literally instead of applying the ANSI reset code - Extend _validate_project_name to also reject '..', which would resolve to the parent directory via Path(); update all three error messages to mention '..' alongside the separator check - Update docstring to accurately describe what the guard does (interprets as a filesystem path vs. traversal wording) - Refactor TestInitProjectNameValidation to use CliRunner.isolated_filesystem() removing the manual os.chdir / try-finally bookkeeping - Add test_invalid_dotdot and test_init_rejects_dotdot covering 'apm init ..' - Add test_init_interactive_reprompts_on_invalid_name_click and test_init_interactive_reprompts_on_dotdot_click covering the while-True re-prompt loop in interactive mode - Fix test_invalid_backslash to use chr(92) to construct backslash strings unambiguously, avoiding Python escape sequence confusion --- src/apm_cli/commands/_helpers.py | 12 ++-- src/apm_cli/commands/init.py | 6 +- tests/unit/test_init_command.py | 110 +++++++++++++++++++------------ 3 files changed, 78 insertions(+), 50 deletions(-) diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index c7c1f03dc..895a9984d 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -415,15 +415,19 @@ def _validate_plugin_name(name): def _validate_project_name(name): - """Validate that a project name does not contain path separators. + """Validate that a project name is safe to use as a directory name. Project names are used directly as directory names and must not contain - '/' or '\\' to prevent unintended filesystem path traversal. + '/' or '\' so the name is not interpreted as a filesystem path, + and must not be '..' to prevent directory traversal. Returns True if valid, False otherwise. """ - return "/" not in name and "\\" not in name - + if "/" in name or "\\" in name: + return False + if name == "..": + return False + return True def _create_plugin_json(config): """Create plugin.json file with package metadata. diff --git a/src/apm_cli/commands/init.py b/src/apm_cli/commands/init.py index d68c6a07e..f97ff46df 100644 --- a/src/apm_cli/commands/init.py +++ b/src/apm_cli/commands/init.py @@ -52,7 +52,7 @@ def init(ctx, project_name, yes, plugin, verbose): if project_name and not _validate_project_name(project_name): logger.error( f"Invalid project name '{project_name}': " - "project names must not contain path separators ('/' or '\\\\')." + "project names must not contain path separators ('/' or '\\\\') or be '..'." ) sys.exit(1) @@ -194,7 +194,7 @@ def _interactive_project_setup(default_name, logger): break console.print( f"[error]Invalid project name '{name}': " - "project names must not contain path separators ('/' or '\\\\').[/error]" + "project names must not contain path separators ('/' or '\\\\') or be '..'.[/error]" ) version = Prompt.ask("Version", default="1.0.0").strip() @@ -224,7 +224,7 @@ def _interactive_project_setup(default_name, logger): break click.echo( f"{ERROR}Invalid project name '{name}': " - "project names must not contain path separators ('/' or '\\\\').{RESET}" + f"project names must not contain path separators ('/' or '\\\\') or be '..'.{RESET}" ) version = click.prompt("Version", default="1.0.0").strip() diff --git a/tests/unit/test_init_command.py b/tests/unit/test_init_command.py index 9362ffd6e..135d91f02 100644 --- a/tests/unit/test_init_command.py +++ b/tests/unit/test_init_command.py @@ -345,61 +345,85 @@ def test_invalid_forward_slash(self): def test_invalid_backslash(self): from apm_cli.commands._helpers import _validate_project_name - assert _validate_project_name("a\\b") is False - assert _validate_project_name("\\leading") is False - assert _validate_project_name("trailing\\") is False + bs = chr(92) # one backslash character + assert _validate_project_name("a" + bs + "b") is False + assert _validate_project_name(bs + "leading") is False + assert _validate_project_name("trailing" + bs) is False + + def test_invalid_dotdot(self): + from apm_cli.commands._helpers import _validate_project_name + + assert _validate_project_name("..") is False + + def test_dotdot_in_slash_path_caught_by_slash_check(self): + """Names like a/../b are caught by the slash check, not the dotdot check.""" + from apm_cli.commands._helpers import _validate_project_name + + assert _validate_project_name("a/../b") is False # slash catches it class TestInitProjectNameValidation: - """Integration tests: apm init rejects project names with path separators.""" + """Integration tests: apm init rejects project names with path separators or '..'.""" def setup_method(self): self.runner = CliRunner() - try: - self.original_dir = os.getcwd() - except FileNotFoundError: - self.original_dir = str(Path(__file__).parent.parent.parent) - os.chdir(self.original_dir) - - def teardown_method(self): - try: - os.chdir(self.original_dir) - except (FileNotFoundError, OSError): - os.chdir(str(Path(__file__).parent.parent.parent)) def test_init_rejects_forward_slash_in_name(self): """apm init 4/15 must fail with a clear error, not a WinError.""" - with tempfile.TemporaryDirectory() as tmp_dir: - os.chdir(tmp_dir) - try: - result = self.runner.invoke(cli, ["init", "4/15", "--yes"]) - assert result.exit_code != 0 - assert "Invalid project name" in result.output - assert "4/15" in result.output - # No directory should be created - assert not Path("4").exists() - finally: - os.chdir(self.original_dir) + with self.runner.isolated_filesystem(): + result = self.runner.invoke(cli, ["init", "4/15", "--yes"]) + assert result.exit_code != 0 + assert "Invalid project name" in result.output + assert "4/15" in result.output + assert not Path("4").exists() def test_init_rejects_backslash_in_name(self): """apm init with a backslash in the name must fail with a clear error.""" - with tempfile.TemporaryDirectory() as tmp_dir: - os.chdir(tmp_dir) - try: - result = self.runner.invoke(cli, ["init", "a\\b", "--yes"]) - assert result.exit_code != 0 - assert "Invalid project name" in result.output - assert "a\\b" in result.output - finally: - os.chdir(self.original_dir) + bs = chr(92) + with self.runner.isolated_filesystem(): + result = self.runner.invoke(cli, ["init", "a" + bs + "b", "--yes"]) + assert result.exit_code != 0 + assert "Invalid project name" in result.output + assert bs in result.output + + def test_init_rejects_dotdot(self): + """apm init .. must fail -- '..' would create a project in the parent directory.""" + with self.runner.isolated_filesystem(): + result = self.runner.invoke(cli, ["init", "..", "--yes"]) + assert result.exit_code != 0 + assert "Invalid project name" in result.output + assert ".." in result.output def test_init_accepts_plain_name(self): """apm init with a simple name still works normally.""" - with tempfile.TemporaryDirectory() as tmp_dir: - os.chdir(tmp_dir) - try: - result = self.runner.invoke(cli, ["init", "my-project", "--yes"]) - assert result.exit_code == 0 - assert (Path(tmp_dir) / "my-project" / "apm.yml").exists() - finally: - os.chdir(self.original_dir) + with self.runner.isolated_filesystem() as tmp_dir: + result = self.runner.invoke(cli, ["init", "my-project", "--yes"]) + assert result.exit_code == 0 + assert (Path(tmp_dir) / "my-project" / "apm.yml").exists() + + def test_init_interactive_reprompts_on_invalid_name_click(self): + """In interactive mode, an invalid name triggers a re-prompt.""" + with self.runner.isolated_filesystem() as tmp_dir: + # First input is invalid (contains '/'), second is valid. + # In no-argument interactive mode, the prompted name goes into apm.yml + # but does not create a subdirectory; apm.yml lands in the CWD. + result = self.runner.invoke( + cli, + ["init"], + input="bad/name\nmy-project\n1.0.0\n\n\ny\n", + catch_exceptions=False, + ) + assert "Invalid project name" in result.output + assert (Path(tmp_dir) / "apm.yml").exists() + + def test_init_interactive_reprompts_on_dotdot_click(self): + """In interactive mode, '..' triggers re-prompt.""" + with self.runner.isolated_filesystem() as tmp_dir: + result = self.runner.invoke( + cli, + ["init"], + input="..\nmy-project\n1.0.0\n\n\ny\n", + catch_exceptions=False, + ) + assert "Invalid project name" in result.output + assert (Path(tmp_dir) / "apm.yml").exists()