-
Notifications
You must be signed in to change notification settings - Fork 50
Implement production-ready update system with security and reliability features #1180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR upgrades the software update subsystem from prototype to production-ready status by implementing comprehensive security features, reliability mechanisms, and extensive test coverage. The changes transform a basic update checker into a robust, enterprise-ready update system with SHA256 verification, automatic retry with exponential backoff, resume capabilities, and thorough validation.
Key Changes:
- Security enhancements: SHA256 checksum verification, PE header validation, URL whitelisting, and file permission hardening
- Reliability features: Download resume with HTTP Range headers, exponential backoff with jitter, automatic cleanup of corrupted files
- Comprehensive test suite: 2,067 lines of new tests covering unit, integration, BDD, and acceptance scenarios
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| ardupilot_methodic_configurator/backend_internet.py | Major refactoring: adds checksum verification, retry logic with exponential backoff, download resume, and comprehensive validation helpers |
| ardupilot_methodic_configurator/data_model_software_updates.py | Integrates SHA256 verification into download workflow, fixes logging import bug, restructures error handling |
| tests/unit_frontend_tkinter_software_update.py | New comprehensive UI test suite (406 lines) covering dialog initialization, button behavior, progress updates, and window protocols |
| tests/unit_download_resume.py | New tests for download resume and retry mechanisms with mock responses |
| tests/unit_checksum_parsing.py | New tests for SHA256 checksum parsing from various release asset formats |
| tests/integration_software_update_github_api.py | New integration tests making real GitHub API calls (marked with @pytest.mark.integration) |
| tests/bdd_software_update.py | New BDD tests focusing on user workflows and business requirements |
| tests/acceptance_software_update.py | New acceptance tests validating complete end-to-end scenarios |
| tests/unit_data_model_software_updates.py | Updated to include expected_sha256 parameter in mock calls |
| tests/unit_backend_internet.py | Updated to include allow_resume parameter in download calls |
| ARCHITECTURE_1_software_update.md | Comprehensive documentation update marking 14 items as implemented, adds Known Limitations section |
| def test_update_process_is_intuitive_and_requires_minimal_intervention(self) -> None: | ||
| """ | ||
| Update process is intuitive with minimal user intervention. | ||
| GIVEN: User encounters an available update | ||
| WHEN: User goes through update process | ||
| THEN: Process is self-explanatory | ||
| AND: Requires minimal clicks/decisions | ||
| AND: Provides clear guidance at each step | ||
| AND: Error messages are actionable when they occur | ||
| AND: Overall experience is positive | ||
| """ | ||
| # This test validates UX through structure of UpdateDialog | ||
| # Real validation would require user testing | ||
| formatted = format_version_info("1.0.0", "2.0.0", "- Improvement") | ||
|
|
||
| # Assert - Information is clear | ||
| assert "1.0.0" in formatted | ||
| assert "2.0.0" in formatted | ||
| assert "Improvement" in formatted |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test function test_update_process_is_intuitive_and_requires_minimal_intervention has minimal implementation (lines 577-596). The docstring describes testing the intuitiveness of the update process, but the actual test only validates that version strings appear in formatted output. This doesn't test usability aspects like minimal clicks, clear guidance, or actionable error messages as described in the docstring. Consider either implementing proper usability tests or revising the docstring to match what the test actually validates.
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Test Results 3 files ± 0 3 suites ±0 34m 10s ⏱️ + 2m 28s Results for commit b052396. ± Comparison against base commit 19c7516. This pull request removes 99 and adds 150 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
bfdfe5b to
370ab46
Compare
|
On windows I still get: |
338c9d3 to
d475c80
Compare
… and reliability features Upgrade the software update subsystem from prototype to production-ready with comprehensive integrity verification, automatic retry, and resume capabilities. Security Enhancements: - SHA256 checksum verification for all downloaded files - File format validation (PE headers for Windows .exe) - Source URL validation (GitHub-only) - File permission hardening (chmod 0o600) - Document security trade-offs (DOS header vs full PE validation) Reliability Features: - Resume interrupted downloads using HTTP Range headers - Exponential backoff with jitter (3 retries, configurable) - Automatic retry on network/filesystem errors - Proper error classification and logging New Capabilities: - Automatic checksum discovery from multiple asset formats (SHA256SUMS, *.sha256, checksums.txt) - Fallback to release notes for checksums - Corporate proxy support via environment variables Code Quality: - Extract magic values to named constants (DOWNLOAD_BLOCK_SIZE, PE_MAGIC_BYTES) - Replace generic exceptions with specific types - Use contextlib.suppress() for optional operations - Document non-cryptographic use of random.uniform() for timing jitter Testing: - Add checksum parsing tests for various release asset formats - Update download tests to use allow_resume parameter - All existing tests remain passing (70+ tests) Documentation: - Update ARCHITECTURE_1_software_update.md marking features as implemented - Change 14 items from ❌ TODO to ✅ IMPLEMENTED - Add Known Limitations section documenting security trade-offs Closes: Security requirements for production deployment Ref: ARCHITECTURE_1_software_update.md
d475c80 to
c877abb
Compare
|
@copilot when reviewing the PR, completely ignore the second git commit. |
|
@amilcarlucas I've opened a new pull request, #1181, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 16 out of 22 changed files in this pull request and generated no new comments.
Upgrade the software update subsystem from prototype to production-ready with comprehensive integrity verification, automatic retry, and resume capabilities.
Security Enhancements:
Reliability Features:
New Capabilities:
Code Quality:
Testing:
Documentation:
Closes: Security requirements for production deployment
Ref: ARCHITECTURE_1_software_update.md