Copilot commented on code in PR #40593:
URL: https://github.com/apache/superset/pull/40593#discussion_r3337854919
##########
superset-extensions-cli/src/superset_extensions_cli/cli.py:
##########
@@ -239,12 +239,30 @@ def copy_backend_files(cwd: Path) -> None:
# Process include patterns
for pattern in include_patterns:
+ # Include patterns are only meant to select files within the backend
+ # directory. Reject absolute patterns or ones that walk outside it via
+ # parent ("..") components before handing them to glob().
+ pattern_parts = Path(pattern).parts
+ if Path(pattern).is_absolute() or ".." in pattern_parts:
+ raise ValueError(
+ f"Invalid include pattern {pattern!r}: patterns must be "
+ "relative to the backend directory and may not contain '..'."
+ )
Review Comment:
Raising a bare ValueError from a Click-based CLI results in an unfriendly
traceback in normal CLI usage. Use click.ClickException (or UsageError) so the
command fails with a clean, user-facing error message consistent with the rest
of this file’s error handling.
##########
superset-extensions-cli/tests/test_cli_build.py:
##########
@@ -625,6 +625,109 @@ def
test_copy_backend_files_handles_various_glob_patterns(isolated_filesystem):
)
[email protected]
+def
test_copy_backend_files_supports_legitimate_nested_patterns(isolated_filesystem):
+ """Test copy_backend_files copies deeply nested files via recursive
globs."""
+ backend_dir = isolated_filesystem / "backend"
+ nested = backend_dir / "src" / "test_org" / "test_ext" / "deep" / "deeper"
+ nested.mkdir(parents=True)
+ (nested / "module.py").write_text("# nested module")
+
+ pyproject_content = """[project]
+name = "test_org-test_ext"
+version = "1.0.0"
+license = "Apache-2.0"
+
+[tool.apache_superset_extensions.build]
+include = [
+ "src/test_org/test_ext/**/*.py",
+]
+exclude = []
+"""
+ (backend_dir / "pyproject.toml").write_text(pyproject_content)
+
+ extension_data = {
+ "publisher": "test-org",
+ "name": "test-ext",
+ "displayName": "Test Extension",
+ "version": "1.0.0",
+ "permissions": [],
+ }
+ (isolated_filesystem /
"extension.json").write_text(json.dumps(extension_data))
+
+ clean_dist(isolated_filesystem)
+ copy_backend_files(isolated_filesystem)
+
+ dist_dir = isolated_filesystem / "dist"
+ assert_file_exists(
+ dist_dir
+ / "backend"
+ / "src"
+ / "test_org"
+ / "test_ext"
+ / "deep"
+ / "deeper"
+ / "module.py"
+ )
+
+
[email protected]
[email protected](
+ "bad_pattern",
+ [
+ "../../.ssh/*",
+ "../config",
+ "src/../../secret.txt",
+ "/etc/passwd",
+ ],
+)
+def test_copy_backend_files_rejects_patterns_escaping_backend_dir(
+ isolated_filesystem, bad_pattern
+):
+ """Test copy_backend_files refuses include patterns that escape
backend_dir."""
+ # Create a sensitive file outside the backend directory.
+ (isolated_filesystem / "secret.txt").write_text("SECRET")
+ (isolated_filesystem / "config").write_text("SECRET")
+
+ backend_dir = isolated_filesystem / "backend"
+ backend_src = backend_dir / "src" / "test_org" / "test_ext"
+ backend_src.mkdir(parents=True)
+ (backend_src / "__init__.py").write_text("# init")
+
+ pyproject_content = f"""[project]
+name = "test_org-test_ext"
+version = "1.0.0"
+license = "Apache-2.0"
+
+[tool.apache_superset_extensions.build]
+include = [
+ "{bad_pattern}",
+]
+exclude = []
+"""
+ (backend_dir / "pyproject.toml").write_text(pyproject_content)
+
+ extension_data = {
+ "publisher": "test-org",
+ "name": "test-ext",
+ "displayName": "Test Extension",
+ "version": "1.0.0",
+ "permissions": [],
+ }
+ (isolated_filesystem /
"extension.json").write_text(json.dumps(extension_data))
+
+ clean_dist(isolated_filesystem)
+
+ with pytest.raises(ValueError):
+ copy_backend_files(isolated_filesystem)
+
Review Comment:
If copy_backend_files is switched to raise click.ClickException for
user-friendly CLI errors, this assertion should be updated accordingly so the
test matches the intended behavior.
##########
superset-extensions-cli/src/superset_extensions_cli/cli.py:
##########
@@ -239,12 +239,30 @@ def copy_backend_files(cwd: Path) -> None:
# Process include patterns
for pattern in include_patterns:
+ # Include patterns are only meant to select files within the backend
+ # directory. Reject absolute patterns or ones that walk outside it via
+ # parent ("..") components before handing them to glob().
+ pattern_parts = Path(pattern).parts
+ if Path(pattern).is_absolute() or ".." in pattern_parts:
+ raise ValueError(
+ f"Invalid include pattern {pattern!r}: patterns must be "
+ "relative to the backend directory and may not contain '..'."
+ )
for f in backend_dir.glob(pattern):
if not f.is_file():
continue
+ # Defense in depth: confirm the matched file resolves to a location
+ # inside the backend directory before copying it into the bundle.
+ resolved = f.resolve()
+ if not resolved.is_relative_to(backend_dir):
+ raise ValueError(
+ f"Refusing to copy {f}: resolved path is outside the "
+ f"backend directory {backend_dir}."
+ )
Review Comment:
This should raise a ClickException (not ValueError) so the CLI exits with a
clean error message rather than a Python traceback when a matched file resolves
outside the backend directory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
