Copilot commented on code in PR #44578:
URL: https://github.com/apache/superset/pull/44578#discussion_r4085944591
##########
scripts/translations/check_pot_drift.py:
##########
@@ -99,16 +99,77 @@ def _msgid_set(pot_path: Path) -> set[MsgId]:
}
-def extract_fresh(output_path: Path) -> None:
- """Run the project's extraction command into ``output_path``."""
- subprocess.run( # noqa: S603
- ["pybabel", "extract", "-F", str(BABEL_CFG), "-o", str(output_path)]
- + EXTRACT_FLAGS,
+def _archive_ref() -> str:
+ """Return a tree-ish for ``git archive`` that matches the working tree.
+
+ ``git stash create`` builds a commit object for the current index and
+ tracked-file modifications without touching the working tree, any ref,
+ or the actual stash, so it is safe to call while other processes are
+ using this checkout. It prints nothing when there is nothing to stash,
+ so fall back to ``HEAD``.
+
+ Deliberately uses ``Popen`` rather than ``run``: this module's tests
+ patch ``subprocess.run`` to fake the single "run pybabel" call, and a
+ second real ``run`` call here would be caught by that same patch.
+ """
+ proc = subprocess.Popen( # noqa: S603
+ ["git", "stash", "create"], # noqa: S607
cwd=ROOT_DIR,
- check=True,
- capture_output=True,
+ stdout=subprocess.PIPE,
text=True,
)
+ stash_sha, _ = proc.communicate()
+ if proc.returncode != 0:
+ raise subprocess.CalledProcessError(proc.returncode, proc.args)
+ return stash_sha.strip() or "HEAD"
Review Comment:
When raising `CalledProcessError`, no stderr is captured, which makes
failures hard to diagnose (especially in CI). Consider piping
`stderr=subprocess.PIPE` (and passing `stderr`/`output` into
`CalledProcessError`) so the exception includes actionable context.
##########
scripts/translations/check_pot_drift.py:
##########
@@ -99,16 +99,77 @@ def _msgid_set(pot_path: Path) -> set[MsgId]:
}
-def extract_fresh(output_path: Path) -> None:
- """Run the project's extraction command into ``output_path``."""
- subprocess.run( # noqa: S603
- ["pybabel", "extract", "-F", str(BABEL_CFG), "-o", str(output_path)]
- + EXTRACT_FLAGS,
+def _archive_ref() -> str:
+ """Return a tree-ish for ``git archive`` that matches the working tree.
+
+ ``git stash create`` builds a commit object for the current index and
+ tracked-file modifications without touching the working tree, any ref,
+ or the actual stash, so it is safe to call while other processes are
+ using this checkout. It prints nothing when there is nothing to stash,
+ so fall back to ``HEAD``.
+
+ Deliberately uses ``Popen`` rather than ``run``: this module's tests
+ patch ``subprocess.run`` to fake the single "run pybabel" call, and a
+ second real ``run`` call here would be caught by that same patch.
+ """
+ proc = subprocess.Popen( # noqa: S603
+ ["git", "stash", "create"], # noqa: S607
cwd=ROOT_DIR,
- check=True,
- capture_output=True,
+ stdout=subprocess.PIPE,
text=True,
)
+ stash_sha, _ = proc.communicate()
+ if proc.returncode != 0:
+ raise subprocess.CalledProcessError(proc.returncode, proc.args)
+ return stash_sha.strip() or "HEAD"
+
+
+def extract_fresh(output_path: Path) -> None:
+ """Run the project's extraction command into ``output_path``.
+
+ Extracts from a ``git archive`` snapshot rather than the live checkout.
+ The Python-Unit job runs ``pytest -n auto --dist loadfile``, so many
+ worker processes share this checkout while this test runs; ``pybabel
+ extract`` walks everything under ``cwd``, so scanning the working
+ directory directly makes the msgid set depend on whatever transient
+ files another worker's test happens to write into the tree at that
+ instant.
+ """
+ with tempfile.TemporaryDirectory() as snapshot_dir_str:
+ snapshot_dir = Path(snapshot_dir_str)
+ archive = subprocess.Popen( # noqa: S603
+ ["git", "archive", _archive_ref()], # noqa: S607
+ cwd=ROOT_DIR,
+ stdout=subprocess.PIPE,
+ )
+ assert archive.stdout is not None
+ with tarfile.open(fileobj=archive.stdout, mode="r|") as tar:
+ # Use the safe extraction filter (PEP 706) where available; older
+ # Python patch releases without the backport still work, just
+ # without that defense-in-depth (this is our own trusted `git
+ # archive` output, not attacker-controlled).
+ data_filter = getattr(tarfile, "data_filter", None)
+ if data_filter is not None:
+ tar.extraction_filter = data_filter
+ tar.extractall(snapshot_dir) # noqa: S202 (own trusted `git
archive` output)
+ if archive.wait() != 0:
+ raise subprocess.CalledProcessError(archive.returncode,
archive.args)
+
+ subprocess.run( # noqa: S603
+ [
+ "pybabel",
+ "extract",
+ "-F",
+ str(snapshot_dir / "superset" / "translations" / "babel.cfg"),
+ "-o",
+ str(output_path),
+ ]
+ + EXTRACT_FLAGS,
+ cwd=snapshot_dir,
+ check=True,
+ capture_output=True,
+ text=True,
+ )
Review Comment:
`cwd` is now `snapshot_dir`, but `-o str(output_path)` may be a relative
path. Previously (when `cwd=ROOT_DIR`) a relative output path would be
interpreted relative to the repo root; now it will be relative to the snapshot
directory, which can silently write the output to an unexpected location.
Consider passing an absolute path (e.g., `output_path.resolve()`) to `-o` to
make the output location independent of `cwd`.
##########
scripts/translations/check_pot_drift.py:
##########
@@ -99,16 +99,77 @@ def _msgid_set(pot_path: Path) -> set[MsgId]:
}
-def extract_fresh(output_path: Path) -> None:
- """Run the project's extraction command into ``output_path``."""
- subprocess.run( # noqa: S603
- ["pybabel", "extract", "-F", str(BABEL_CFG), "-o", str(output_path)]
- + EXTRACT_FLAGS,
+def _archive_ref() -> str:
+ """Return a tree-ish for ``git archive`` that matches the working tree.
+
+ ``git stash create`` builds a commit object for the current index and
+ tracked-file modifications without touching the working tree, any ref,
+ or the actual stash, so it is safe to call while other processes are
+ using this checkout. It prints nothing when there is nothing to stash,
+ so fall back to ``HEAD``.
+
+ Deliberately uses ``Popen`` rather than ``run``: this module's tests
+ patch ``subprocess.run`` to fake the single "run pybabel" call, and a
+ second real ``run`` call here would be caught by that same patch.
+ """
+ proc = subprocess.Popen( # noqa: S603
+ ["git", "stash", "create"], # noqa: S607
cwd=ROOT_DIR,
- check=True,
- capture_output=True,
+ stdout=subprocess.PIPE,
text=True,
)
+ stash_sha, _ = proc.communicate()
+ if proc.returncode != 0:
+ raise subprocess.CalledProcessError(proc.returncode, proc.args)
+ return stash_sha.strip() or "HEAD"
+
+
+def extract_fresh(output_path: Path) -> None:
+ """Run the project's extraction command into ``output_path``.
+
+ Extracts from a ``git archive`` snapshot rather than the live checkout.
+ The Python-Unit job runs ``pytest -n auto --dist loadfile``, so many
+ worker processes share this checkout while this test runs; ``pybabel
+ extract`` walks everything under ``cwd``, so scanning the working
+ directory directly makes the msgid set depend on whatever transient
+ files another worker's test happens to write into the tree at that
+ instant.
+ """
+ with tempfile.TemporaryDirectory() as snapshot_dir_str:
+ snapshot_dir = Path(snapshot_dir_str)
+ archive = subprocess.Popen( # noqa: S603
+ ["git", "archive", _archive_ref()], # noqa: S607
+ cwd=ROOT_DIR,
+ stdout=subprocess.PIPE,
+ )
+ assert archive.stdout is not None
+ with tarfile.open(fileobj=archive.stdout, mode="r|") as tar:
+ # Use the safe extraction filter (PEP 706) where available; older
+ # Python patch releases without the backport still work, just
+ # without that defense-in-depth (this is our own trusted `git
+ # archive` output, not attacker-controlled).
+ data_filter = getattr(tarfile, "data_filter", None)
+ if data_filter is not None:
+ tar.extraction_filter = data_filter
+ tar.extractall(snapshot_dir) # noqa: S202 (own trusted `git
archive` output)
Review Comment:
This assumes `git archive` output is trusted, but in CI this code may run on
PRs/forks where repository content is attacker-controlled. On Python versions
without PEP 706 filtering, `tar.extractall()` can be vulnerable to path
traversal and unsafe link extraction. Consider enforcing a safe extraction
policy on all supported Python versions (e.g., validate member paths stay
within `snapshot_dir` and reject symlinks/hardlinks) rather than relying on
`data_filter` only when available.
##########
scripts/translations/check_pot_drift.py:
##########
@@ -99,16 +99,77 @@ def _msgid_set(pot_path: Path) -> set[MsgId]:
}
-def extract_fresh(output_path: Path) -> None:
- """Run the project's extraction command into ``output_path``."""
- subprocess.run( # noqa: S603
- ["pybabel", "extract", "-F", str(BABEL_CFG), "-o", str(output_path)]
- + EXTRACT_FLAGS,
+def _archive_ref() -> str:
+ """Return a tree-ish for ``git archive`` that matches the working tree.
+
+ ``git stash create`` builds a commit object for the current index and
+ tracked-file modifications without touching the working tree, any ref,
+ or the actual stash, so it is safe to call while other processes are
+ using this checkout. It prints nothing when there is nothing to stash,
+ so fall back to ``HEAD``.
+
+ Deliberately uses ``Popen`` rather than ``run``: this module's tests
+ patch ``subprocess.run`` to fake the single "run pybabel" call, and a
+ second real ``run`` call here would be caught by that same patch.
+ """
+ proc = subprocess.Popen( # noqa: S603
+ ["git", "stash", "create"], # noqa: S607
cwd=ROOT_DIR,
- check=True,
- capture_output=True,
+ stdout=subprocess.PIPE,
text=True,
)
+ stash_sha, _ = proc.communicate()
+ if proc.returncode != 0:
+ raise subprocess.CalledProcessError(proc.returncode, proc.args)
+ return stash_sha.strip() or "HEAD"
+
+
+def extract_fresh(output_path: Path) -> None:
+ """Run the project's extraction command into ``output_path``.
+
+ Extracts from a ``git archive`` snapshot rather than the live checkout.
+ The Python-Unit job runs ``pytest -n auto --dist loadfile``, so many
+ worker processes share this checkout while this test runs; ``pybabel
+ extract`` walks everything under ``cwd``, so scanning the working
+ directory directly makes the msgid set depend on whatever transient
+ files another worker's test happens to write into the tree at that
+ instant.
+ """
+ with tempfile.TemporaryDirectory() as snapshot_dir_str:
+ snapshot_dir = Path(snapshot_dir_str)
+ archive = subprocess.Popen( # noqa: S603
+ ["git", "archive", _archive_ref()], # noqa: S607
+ cwd=ROOT_DIR,
+ stdout=subprocess.PIPE,
+ )
+ assert archive.stdout is not None
Review Comment:
Avoid using `assert` for runtime control flow here because it can be
disabled with Python optimizations (`-O`). Replace with an explicit check and
raise a normal exception if `archive.stdout` is unexpectedly `None`.
--
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]
