fix(security): harden tar extraction in rust_bridge against path traversal (#876)#885
fix(security): harden tar extraction in rust_bridge against path traversal (#876)#885
Conversation
…ersal Resolves #876 by: - Adding SecurityError exception class for extraction failures - Adding _validate_release_member() to reject absolute paths and .. traversal - Adding _extract_release_binary() with filter='data' on Python >=3.12 - Protecting against duplicate asset matching with download_url guard - Using tempfile in MANAGED_BIN_DIR with mode 0o700 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
⚡ Performance Report — PR #885Scope: TL;DR✅ No performance regression detected. All CLI performance budgets remain unaffected. Changed Code Path AnalysisThis PR modifies only
Import OverheadFour imports were promoted from lazy (inside
Estimated startup overhead added: ~0.5–1 ms (well within noise, budget is <100 ms). Install-Time PerformanceThe new validation logic ( The change from The temp file is now created inside CLI Performance Budgets — Status
RecommendationNo performance concerns. This PR is safe to merge from a performance perspective. The security hardening adds negligible overhead only during binary installation, which happens at most once per machine setup.
|
📏 Code Quality Report — PR #885Overall Quality: 9.2/10 ✅ This PR hardens Complexity Analysis
No function exceeds the blocking threshold of complexity > 20. ✅ Code Smells
Maintainability
Technical Debt
Test Quality (
|
| Test | Coverage Target | Status |
|---|---|---|
test_uses_data_filter_on_python_312_plus |
filter='data' path on ≥3.12 | ✅ |
test_rejects_path_traversal_member_on_older_python |
../../azlin traversal |
✅ |
test_rejects_symlink_member_on_older_python |
symlink → /etc/passwd |
✅ |
test_execvp_passthrough_is_intentional |
POSIX exec passthrough | ✅ |
- ✅ All new security helpers (
_validate_release_member,_extract_release_binary) are directly tested - ✅ Temporary files are cleaned up in
finallyblocks in tests - ✅ Full type hints on test helpers
⚠️ Missing coverage:_validate_release_memberwith an absolute path (e.g.,/etc/azlin) is not explicitly tested — only..traversal and symlink type are covered. Consider adding a test for theis_absolute()branch.⚠️ Missing coverage: The "archive did not contain an azlin binary"SecurityErrorpath in_extract_release_binaryis not tested.
Security Observation
The copy.copy(member) before renaming extracted_member.name = "azlin" is a clean pattern that prevents mutating the original TarInfo object. The use of filter='data' on Python ≥3.12 is the recommended approach per [PEP 706]((peps.python.org/redacted) and provides defense-in-depth beyond the manual pre-validation.
Recommendations
-
Optional — add two test cases to close the coverage gaps noted above:
_validate_release_memberwith an absolute-path member (e.g.,/etc/azlin) should raiseSecurityError_extract_release_binarywith an archive containing noazlinmember should raiseSecurityError
-
No blocking issues found. All new functions are within complexity limits, fully typed, and fully documented.
Verdict: ✅ Ready to merge. Quality improved vs. the pre-PR baseline — inline imports removed, security helpers are small and well-bounded, and the new test file adds meaningful regression coverage.
Generated by Code Quality Tracker for issue #885
📊 Test Coverage ReportPR: fix(security): harden tar extraction in rust_bridge against path traversal (#876) ✅ Coverage Assessment: Net PositiveThis PR introduces a new test file ( 🔍 What the New Tests Cover
🎯 Still Uncovered (Lower Priority for This PR)The following paths in the new code lack test coverage. These are good candidates for follow-up:
📈 Progress Toward 80% Target
Progress: 44% ░░░░░░░░░░░░░░░░░░░░ 80% (36pp to go) 💡 Suggested Follow-Up TestsTo maximize coverage gains from this module, consider adding to def test_rejects_absolute_path_member(self) -> None:
archive_path = self._write_archive(_make_archive("/etc/passwd"))
# ... assert raises SecurityError
def test_extract_succeeds_on_older_python_with_valid_member(self) -> None:
archive_path = self._write_archive(_make_archive())
with mock.patch.object(rust_bridge, "_PY312_PLUS", False):
# ... assert extraction succeeds without SecurityError
def test_raises_when_binary_not_in_archive(self) -> None:
archive_path = self._write_archive(_make_archive("other_file"))
# ... assert raises SecurityError("Downloaded archive did not contain an azlin binary")Great work securing the tar extraction path — this is exactly the kind of security-critical code that deserves thorough test coverage. 🎯 The tests for the traversal and filter paths are clear and well-structured. Next high-impact area: Consider adding tests for
|
Per TDD methodology (Step 7), write tests that:
- FAIL on main (SecurityError, _validate_release_member, _extract_release_binary,
and _PY312_PLUS do not yet exist in rust_bridge.py)
- PASS once the security hardening in this PR is merged
TestExtractReleaseBinary (4 tests):
test_filter_data_used_on_python_312_plus
On Python 3.12+, _extract_release_binary must call tar.extract() with
filter='data' to use CPython's new security filter (SEC-R-02).
test_path_traversal_rejected
_validate_release_member must raise SecurityError for any member whose
PurePosixPath.parts contain '..' (SEC-R-01).
test_symlink_rejected_on_pre_312
On Python < 3.12, _validate_release_member must reject SYMTYPE/LNKTYPE
members. Skipped on 3.12+ where filter='data' handles this.
test_security_error_raised_when_no_binary_in_archive
_extract_release_binary must raise SecurityError when no azlin binary
is found in the archive.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⚡ Performance Report — PR #885Scope of changes: Impact Assessment
Download / Installation Path — Changes in DetailThe security hardening adds three new helpers that run only during
Net effect on install path: Unmeasurably small overhead (microseconds per tar member validation). The dominant cost remains network I/O to GitHub's API and asset download, typically 2–5 seconds depending on network conditions. Import-Time CostTwo new top-level imports were added: import copy # stdlib — already loaded by many deps
import json # stdlib — already loaded
import tarfile # stdlib — moved from deferred to top-level
import tempfile # stdlib — moved from deferred to top-level
Measured stdlib import cost (estimate):
Since Python caches modules in Summary
No performance regressions detected. ✅ The import cost of moving
|
📊 Test Coverage ReportPR: fix(security): harden tar extraction in rust_bridge against path traversal (#876) ✅ Coverage Assessment: Net PositiveThis PR introduces a new test file ( 🔍 What the New Tests Cover
Lines newly covered in
🎯 Still Uncovered in
|
| Lines | Function | Coverage | Priority |
|---|---|---|---|
| 52–53 | _validate_release_member — backslash (\\) check |
Medium | |
| 52–53 | _validate_release_member — absolute path (e.g. /etc/passwd) |
Medium | |
| 86–102 | _platform_suffix |
❌ 0% (8 branches) | 🟡 Medium — pure function, easy to test |
| 105–116 | _is_rust_binary |
❌ 0% (subprocess call) | 🟡 Medium |
| 119–134 | _find_rust_binary |
❌ 0% | 🟡 Medium |
| 137–204 | _download_from_release |
❌ 0% (network-dependent) | 🔴 Critical path, needs mocking |
| 197–198 | _download_from_release — SecurityError handler |
❌ 0% | 🔴 Security-relevant |
| 207–234 | _build_from_source |
❌ 0% | 🟡 Medium |
| 245–247 | _exec_rust — Windows branch |
❌ 0% | 🟢 Lower (platform-specific) |
| 249 | _exec_rust — POSIX os.execvp |
❌ 0% | 🟡 Medium |
| 252–281 | entry |
❌ 0% | 🔴 Critical entry point |
📈 Progress Toward 80% Target
- Baseline: ~44% overall coverage
- This PR's contribution: Adds ~18–25 newly covered executable statements in
rust_bridge.pyout of ~112 total in that file. Givenrust_bridge.py's share of the codebase, estimated +0.5–1.5% overall coverage gain ✅ - Overall status: Net positive — no previously-covered lines are removed
Progress bar: 44% ▓░░░░░░░░░░░░░░░░░░░ 80% (est. 35–36pp remaining after merge)
💡 Suggested Follow-Up Tests (Highest Impact)
To further improve coverage of rust_bridge.py, consider adding to test_rust_bridge.py:
# 1. Absolute path member rejection (closes the is_absolute() branch gap)
def test_rejects_absolute_path_member(self) -> None:
member = tarfile.TarInfo(name="/etc/passwd")
member.type = tarfile.REGTYPE
with pytest.raises(SecurityError, match="Unsafe archive member rejected"):
_validate_release_member(member)
# 2. _platform_suffix — pure function, easy to parametrize
`@pytest`.mark.parametrize("system,machine,expected", [
("Linux", "x86_64", "linux-x86_64"),
("Linux", "aarch64", "linux-aarch64"),
("Darwin", "arm64", "macos-aarch64"),
("Windows", "AMD64", "windows-x86_64"),
("FreeBSD", "x86_64", None),
])
def test_platform_suffix(system, machine, expected, monkeypatch):
monkeypatch.setattr(platform, "system", lambda: system)
monkeypatch.setattr(platform, "machine", lambda: machine)
assert _platform_suffix() == expected
# 3. _extract_release_binary pre-3.12 happy path
def test_extract_succeeds_pre_312_with_valid_member(self, tmp_path, monkeypatch):
import azlin.rust_bridge as rb
monkeypatch.setattr(rb, "_PY312_PLUS", False)
tar_path = _make_tar_gz_file(tmp_path, [("azlin", tarfile.REGTYPE, b"binary")])
dest = tmp_path / "dest"
rb._extract_release_binary(str(tar_path), dest)
assert (dest / "azlin").exists()Next highest-impact module: After rust_bridge.py, consider adding tests for _platform_suffix() and _is_rust_binary() — both are side-effect-free (with mocking) and would meaningfully improve overall module coverage with minimal test complexity.
Verdict
✅ No coverage regression. This PR adds meaningful test coverage for the highest-risk new code paths (path traversal, symlink injection, missing-binary detection). Coverage is net positive.
The security-hardened functions — the most critical new code in this PR — are well-covered. The uncovered functions (_download_from_release, entry) existed before this PR and represent existing coverage debt, not a regression introduced here.
Great work adding security tests alongside the security fix — this is exactly the right pattern. 🎯
Generated by Test Coverage Improvement Tracker for issue #885
📏 Code Quality Report — PR #885Overall Quality: 8.4/10 ✅ This PR introduces security hardening for tar extraction in Complexity Analysis
No function exceeds the complexity-20 block threshold. ✅ Code Smells
Maintainability
Technical Debt
Test QualityThe new test file
Recommendations
|
…h loop The `download_url is None` condition in the inner asset loop was a roundabout way to take only the first matching asset. Replace it with an explicit `break`, which is both clearer in intent and terminates the inner loop immediately rather than iterating through remaining assets. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tests Improves the initial fix for #876 with additional hardening: rust_bridge.py: - Add urllib.error import for specific network exception handling (replaces bare `except Exception` in _download_from_release) - Use mode=0o700 when creating MANAGED_BIN_DIR to prevent other users reading or modifying the managed binary directory - Write temp file into MANAGED_BIN_DIR (same filesystem = atomic rename) - Catch SecurityError separately to give a clear stderr message before aborting installation - Add explicit doc note in _exec_rust() that argv passthrough is intentional for this CLI passthrough tool (documents #877 decision) - Expand _validate_release_member docstring to enumerate all checks tests/unit/test_rust_bridge_security.py (new): - 17 unit tests for _is_release_binary_member, _validate_release_member, _extract_release_binary, and _download_from_release - Covers: absolute paths, traversal, backslash paths, symlinks on Python <3.12, missing binary in archive, network errors, checksum path via SecurityError propagation Resolves #876 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| def test_temp_file_cleaned_up_on_security_error(self, tmp_path, monkeypatch): | ||
| """If SecurityError is raised during extraction, the temp file is removed.""" | ||
| import azlin.rust_bridge as rb |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, to fix this issue you should avoid importing the same module with both from module import name and import module in the same file. Choose one style and use it consistently. Since the flagged line is a late import azlin.rust_bridge as rb, the recommended approach is to remove that secondary import and instead access what you need via the existing from azlin.rust_bridge import ... import—either by importing additional names there or by binding a local alias to the module using already-imported functionality.
In this specific case, we only need a module-like object rb inside TestTempFileCleanup.test_temp_file_cleaned_up_on_security_error for monkeypatching attributes and calling _download_from_release() and _platform_suffix(). We can create such an alias without a new import by using Python’s built-in sys.modules mapping, which already contains the loaded azlin.rust_bridge module. Inside the test, replace import azlin.rust_bridge as rb with rb = sys.modules["azlin.rust_bridge"]. This uses the existing top-level import sys and avoids introducing a second import form for the same module, while preserving all current behavior. No other lines in the file need to change.
| @@ -273,7 +273,7 @@ | ||
|
|
||
| def test_temp_file_cleaned_up_on_security_error(self, tmp_path, monkeypatch): | ||
| """If SecurityError is raised during extraction, the temp file is removed.""" | ||
| import azlin.rust_bridge as rb | ||
| rb = sys.modules["azlin.rust_bridge"] | ||
|
|
||
| captured_tmp: list[Path] = [] | ||
|
|
| self, tmp_path, monkeypatch | ||
| ): | ||
| """_download_from_release() must NOT catch SecurityError.""" | ||
| import azlin.rust_bridge as rb |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, to fix this type of issue, you remove the redundant import style and stick to one approach: either use from module import name or use import module (optionally aliased), but not both for the same module in the same file. If you still need access to the module or specific attributes, you either import those attributes explicitly at the top or derive them from existing objects.
For this file, the best minimal fix is to remove the inner import azlin.rust_bridge as rb and stop using rb altogether. Instead:
- Use the already-imported
SecurityErrordirectly in the test. - Use
pytest.monkeypatch.context()to create a context manager that temporarily sets attributes on theazlin.rust_bridgemodule during the test. Inside that context, refer to_platform_suffixand_download_from_releasedirectly (they will resolve to the patched attributes on the module). - This avoids introducing a new top-level import and keeps behavior the same: we still patch
MANAGED_BIN_DIR,MANAGED_BIN,_extract_release_binary, and the network calls on the actualazlin.rust_bridgemodule, and we still call_download_from_release()as before.
Concretely, edit tests/unit/test_rust_bridge_security.py:
- In
TestSecurityErrorPropagation.test_security_error_propagates_from_download_handler, delete the lineimport azlin.rust_bridge as rb. - Replace uses of
rb:- The two
monkeypatch.setattrcalls should be moved inside awith monkeypatch.context() as mp:block, and usemp.setattr("azlin.rust_bridge.MANAGED_BIN_DIR", tmp_path)and likewise forMANAGED_BINand_extract_release_binary. - Change the f-string building the asset name to call
_platform_suffix()directly. - Change the final call to
rb._download_from_release()to_download_from_release().
- The two
No new methods or external libraries are required; we just rely on pytest’s monkeypatch.context() and the existing imported symbols from azlin.rust_bridge.
| @@ -340,37 +340,36 @@ | ||
| self, tmp_path, monkeypatch | ||
| ): | ||
| """_download_from_release() must NOT catch SecurityError.""" | ||
| import azlin.rust_bridge as rb | ||
| with monkeypatch.context() as mp: | ||
| mp.setattr("azlin.rust_bridge.MANAGED_BIN_DIR", tmp_path) | ||
| mp.setattr("azlin.rust_bridge.MANAGED_BIN", tmp_path / "azlin") | ||
|
|
||
| monkeypatch.setattr(rb, "MANAGED_BIN_DIR", tmp_path) | ||
| monkeypatch.setattr(rb, "MANAGED_BIN", tmp_path / "azlin") | ||
| def evil_extract(tmp_path, destination): | ||
| raise SecurityError("path traversal detected") | ||
|
|
||
| def evil_extract(tmp_path, destination): | ||
| raise SecurityError("path traversal detected") | ||
| mp.setattr("azlin.rust_bridge._extract_release_binary", evil_extract) | ||
|
|
||
| monkeypatch.setattr(rb, "_extract_release_binary", evil_extract) | ||
| fake_releases = [ | ||
| { | ||
| "tag_name": "v0.1.0-rust", | ||
| "assets": [ | ||
| { | ||
| "name": f"azlin-{_platform_suffix() or 'linux-x86_64'}.tar.gz", | ||
| "browser_download_url": "http://example.com/azlin.tar.gz", | ||
| } | ||
| ], | ||
| } | ||
| ] | ||
|
|
||
| fake_releases = [ | ||
| { | ||
| "tag_name": "v0.1.0-rust", | ||
| "assets": [ | ||
| { | ||
| "name": f"azlin-{rb._platform_suffix() or 'linux-x86_64'}.tar.gz", | ||
| "browser_download_url": "http://example.com/azlin.tar.gz", | ||
| } | ||
| ], | ||
| } | ||
| ] | ||
| with patch("azlin.rust_bridge.urllib.request.urlopen") as mock_urlopen: | ||
| mock_resp = MagicMock() | ||
| mock_resp.__enter__ = lambda s: s | ||
| mock_resp.__exit__ = MagicMock(return_value=False) | ||
| mock_resp.read.return_value = ( | ||
| __import__("json").dumps(fake_releases).encode() | ||
| ) | ||
| mock_urlopen.return_value = mock_resp | ||
|
|
||
| with patch("azlin.rust_bridge.urllib.request.urlopen") as mock_urlopen: | ||
| mock_resp = MagicMock() | ||
| mock_resp.__enter__ = lambda s: s | ||
| mock_resp.__exit__ = MagicMock(return_value=False) | ||
| mock_resp.read.return_value = ( | ||
| __import__("json").dumps(fake_releases).encode() | ||
| ) | ||
| mock_urlopen.return_value = mock_resp | ||
|
|
||
| with patch("azlin.rust_bridge.urllib.request.urlretrieve"): | ||
| with pytest.raises(SecurityError, match="path traversal detected"): | ||
| rb._download_from_release() | ||
| with patch("azlin.rust_bridge.urllib.request.urlretrieve"): | ||
| with pytest.raises(SecurityError, match="path traversal detected"): | ||
| _download_from_release() |
📊 Test Coverage ReportCoverage Change: 44% → ~45% (+1%) ✅ This PR adds 605 lines of new tests across two new test files ( Newly Covered
Functions with strong new test coverage:
Still Uncovered (Recommended Next Steps)These areas in
Progress Toward 80% Goal
Verdict✅ Coverage improves — no block. This PR contributes positively to the coverage trend. Thank you for adding comprehensive security tests alongside the hardening! The combination of Suggested next area: Consider adding tests for
|
⚡ Performance Report — PR #885PR: fix(security): harden tar extraction in rust_bridge against path traversal (#876) Performance Impact AssessmentThis PR modifies Startup Path Analysis
Critical Observation: Import Overhead (Negligible in Practice)The move of
Verdict: The import overhead is well within the <100ms startup budget. Command Performance Summary
Memory Usage
The additional ~200–400KB for eagerly loading Performance Regression AnalysisNo performance regressions detected. The security hardening adds:
All of these occur only during binary installation (rare, one-time event), not during normal CLI usage. Security vs. Performance Trade-offThis PR correctly prioritizes security over a negligible performance cost. The new validation in Recommendation✅ No performance concerns. Safe to merge. The import restructuring is the only measurable change (+0.5–1.5ms startup overhead), and it remains well within the <100ms startup budget. All CLI commands (
|
📏 Code Quality Report — PR #885Overall Quality: 9.0/10 ✅ Files analyzed: Complexity Analysis
Average CC: ~4.3 — Well within acceptable range. No function exceeds CC 20 ❌ threshold.
Code Smells
Maintainability
Technical Debt
Security-Specific Quality (relevant to this PR)
Test Quality
Recommendations
Verdict: ✅ PASS — No blocking quality issues. Two minor observations noted above.
|
2 participants
Summary
Hardens the tar archive extraction in
rust_bridge.pyagainst path traversal and unsafe extraction attacks, and adds a comprehensive unit test suite.Security improvements (issue #876):
SecurityErrorexception class for extraction-related failures_validate_release_member()to reject members with absolute paths,..components, backslash paths, and non-regular files (Python < 3.12 wherefilter='data'is unavailable)_extract_release_binary()usingfilter='data'on Python ≥ 3.12 for safe extractionMANAGED_BIN_DIRcreated with mode0o700to prevent other local users reading/replacing the managed binaryMANAGED_BIN_DIR(same filesystem → atomic rename/cleanup)urllib.error.URLErrorcaught specifically instead of bareexcept Exceptionin download pathSecurityErrorcaught separately to emit a clearstderrmessage before aborting_is_release_binary_member()as a pure allowlist predicateIssue #877 analysis (no code change needed):
_exec_rust()passessys.argvdirectly toos.execvp. This is intentional:azlinis a pure CLI passthrough tool where the invoking user entirely controls their own arguments — there is no untrusted input to sanitise. Documented in the_exec_rust()docstring.New test file:
tests/unit/test_rust_bridge_security.py— 21 test cases covering all security predicates, extraction logic, and error paths.Related Issues
Closes #876
Step 13: Local Testing Results
Scenario 1 — tar security predicates (basic validation)
Command:
uv run pytest tests/unit/test_rust_bridge_security.py -von branchfix/issue-876-rust-bridge-security-hardeningResult: PASS
Output:
Scenario 2 — cli_documentation quality fixes regression (complementary PR)
Command:
uv run pytest tests/unit/test_cli_documentation.py -von branchfix/issue-878-879-880-cli-documentation-qualityResult: PASS
Output:
41 passed in 0.13s— all error-swallowing, encoding, and validation scenarios verifiedTest Plan
uv run pytest tests/unit/test_rust_bridge_security.py -v— 17 pass, 4 skipped (version-gated)TestValidateReleaseMember— absolute path, traversal, symlink/hardlink/device rejectionTestIsReleaseBinaryMember— allowlist predicate correctnessTestExtractReleaseBinary— normalised extraction, traversal rejection, data filter usageTestTempFileCleanup— temp file removed on SecurityErrorTestSecurityErrorPropagation— SecurityError surfaced from download handler