-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: updates checker and versioning #1
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 adds version tracking and self-update capabilities to the ruleset-fetcher script, enabling users to check for and install updates directly from the command line.
Changes:
- Added version constant (1.0.1) and GitHub repository information for update checking
- Implemented version comparison, update checking, and self-update functionality with three new commands
- Updated help text and README documentation to reflect new features
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| ruleset-fetcher.sh | Added version constants, banner version display, and three new functions (get_remote_version, compare_versions, check_for_updates, self_update, show_version) with corresponding command-line argument handlers |
| README.md | Updated command reference table to document the three new update-related commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ruleset-fetcher.sh
Outdated
|
|
||
| local temp_file="/tmp/ruleset-fetcher-update.sh" | ||
|
|
||
| if ! curl -fsSL --connect-timeout 30 --max-time 120 -o "${temp_file}" "${GITHUB_RAW_URL}" 2>/dev/null; then |
Copilot
AI
Jan 15, 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 download uses curl with --connect-timeout and --max-time, but doesn't verify HTTPS certificates are being validated. While curl validates certificates by default, consider explicitly adding --cacert or ensuring the system's CA bundle is up to date. Additionally, downloading from the 'main' branch means users always get the latest commit, which could be untested. Consider using tagged releases for more stable updates.
| if ! curl -fsSL --connect-timeout 30 --max-time 120 -o "${temp_file}" "${GITHUB_RAW_URL}" 2>/dev/null; then | |
| local download_ref="${remote_version}" | |
| local download_url="https://raw.githubusercontent.com/${GITHUB_REPO}/${download_ref}/ruleset-fetcher.sh" | |
| if ! curl -fsSL --connect-timeout 30 --max-time 120 --cacert /etc/ssl/certs/ca-certificates.crt -o "${temp_file}" "${download_url}" 2>/dev/null; then |
| if ! head -1 "${temp_file}" | grep -q '^#!/bin/bash'; then | ||
| print_error "Downloaded file is not a valid script" | ||
| rm -f "${temp_file}" | ||
| return 1 | ||
| fi |
Copilot
AI
Jan 15, 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 shebang validation only checks the first line, but doesn't verify the script's integrity or authenticity. Consider adding a checksum verification or signature validation to ensure the downloaded script hasn't been tampered with. An attacker with control over the repository or network could serve malicious code that would pass this basic check.
ruleset-fetcher.sh
Outdated
| fi | ||
|
|
||
| if [[ -f "${SCRIPT_PATH}" ]]; then | ||
| cp "${SCRIPT_PATH}" "${SCRIPT_PATH}.backup" |
Copilot
AI
Jan 15, 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 backup file is created without checking if the original file exists or is readable, and without verifying the copy operation succeeded. If the copy fails, users could lose their current working version without a valid backup when the update proceeds. Add error handling to verify the backup was created successfully before proceeding with the update.
| cp "${SCRIPT_PATH}" "${SCRIPT_PATH}.backup" | |
| if [[ ! -r "${SCRIPT_PATH}" ]]; then | |
| print_error "Current script is not readable; aborting update to avoid data loss." | |
| rm -f "${temp_file}" | |
| return 1 | |
| fi | |
| if ! cp "${SCRIPT_PATH}" "${SCRIPT_PATH}.backup"; then | |
| print_error "Failed to create backup; aborting update to avoid data loss." | |
| rm -f "${temp_file}" | |
| return 1 | |
| fi |
ruleset-fetcher.sh
Outdated
| if [[ -f "${SCRIPT_PATH}" ]]; then | ||
| cp "${SCRIPT_PATH}" "${SCRIPT_PATH}.backup" | ||
| fi | ||
|
|
||
| mv "${temp_file}" "${SCRIPT_PATH}" | ||
| chmod +x "${SCRIPT_PATH}" | ||
|
|
||
| print_success "Updated successfully to v${remote_version}!" | ||
| echo "" | ||
| print_info "Backup saved to: ${SCRIPT_PATH}.backup" |
Copilot
AI
Jan 15, 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 move operation and chmod could fail silently. If the mv fails, the system would be left without the script at SCRIPT_PATH. If chmod fails, the script might not be executable. Add error checking for both operations and restore from backup if either fails.
| if [[ -f "${SCRIPT_PATH}" ]]; then | |
| cp "${SCRIPT_PATH}" "${SCRIPT_PATH}.backup" | |
| fi | |
| mv "${temp_file}" "${SCRIPT_PATH}" | |
| chmod +x "${SCRIPT_PATH}" | |
| print_success "Updated successfully to v${remote_version}!" | |
| echo "" | |
| print_info "Backup saved to: ${SCRIPT_PATH}.backup" | |
| local had_backup=false | |
| if [[ -f "${SCRIPT_PATH}" ]]; then | |
| cp "${SCRIPT_PATH}" "${SCRIPT_PATH}.backup" | |
| had_backup=true | |
| fi | |
| if ! mv "${temp_file}" "${SCRIPT_PATH}"; then | |
| print_error "Failed to install update (move operation failed)" | |
| if [[ "${had_backup}" == true ]]; then | |
| if mv "${SCRIPT_PATH}.backup" "${SCRIPT_PATH}"; then | |
| print_info "Restored previous version from backup." | |
| else | |
| print_error "Failed to restore previous version from backup at ${SCRIPT_PATH}.backup" | |
| fi | |
| fi | |
| rm -f "${temp_file}" | |
| return 1 | |
| fi | |
| if ! chmod +x "${SCRIPT_PATH}"; then | |
| print_error "Failed to make updated script executable (chmod failed)" | |
| if [[ "${had_backup}" == true ]]; then | |
| if mv "${SCRIPT_PATH}.backup" "${SCRIPT_PATH}"; then | |
| print_info "Restored previous version from backup." | |
| else | |
| print_error "Failed to restore previous version from backup at ${SCRIPT_PATH}.backup" | |
| fi | |
| fi | |
| return 1 | |
| fi | |
| print_success "Updated successfully to v${remote_version}!" | |
| echo "" | |
| if [[ -f "${SCRIPT_PATH}.backup" ]]; then | |
| print_info "Backup saved to: ${SCRIPT_PATH}.backup" | |
| fi |
ruleset-fetcher.sh
Outdated
| get_remote_version() { | ||
| local remote_script | ||
| remote_script=$(curl -fsSL --connect-timeout 10 --max-time 30 "${GITHUB_RAW_URL}" 2>/dev/null) || return 1 | ||
| local version=$(echo "$remote_script" | grep -m1 '^VERSION=' | cut -d'"' -f2) |
Copilot
AI
Jan 15, 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 version extraction assumes the VERSION variable is always on a line starting with 'VERSION=' and uses double quotes. If the remote script format changes (e.g., using single quotes, having spaces, or a different format), this will silently fail and return an empty string. Consider adding validation to ensure a valid version format was extracted.
| local version=$(echo "$remote_script" | grep -m1 '^VERSION=' | cut -d'"' -f2) | |
| # Extract VERSION= line robustly, allowing spaces and single/double quotes. | |
| # Examples handled: | |
| # VERSION="1.2.3" | |
| # VERSION = '1.2.3' | |
| # VERSION=1.2.3 | |
| local version | |
| version=$(printf '%s\n' "$remote_script" \ | |
| | sed -nE "s/^[[:space:]]*VERSION[[:space:]]*=[[:space:]]*['\"]?([^'\"[:space:]]+)['\"]?.*/\1/p" \ | |
| | head -n1) | |
| # Validate that a version was found | |
| if [[ -z "$version" ]]; then | |
| print_error "Failed to parse remote version information from ${GITHUB_RAW_URL}" | |
| return 1 | |
| fi | |
| # Validate version format (optionally leading 'v', then numeric dot-separated parts) | |
| if ! [[ "$version" =~ ^v?[0-9]+(\.[0-9]+)*$ ]]; then | |
| print_error "Invalid version format in remote script: '$version'" | |
| return 1 | |
| fi |
ruleset-fetcher.sh
Outdated
| local IFS=. | ||
| local i v1_parts=($v1) v2_parts=($v2) | ||
|
|
Copilot
AI
Jan 15, 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 IFS variable is modified without being saved and restored. While the 'local IFS' declaration limits the scope to the function, this is a common source of bugs. Consider making this more explicit by saving the original IFS value and restoring it, or using an alternative approach like string manipulation that doesn't require modifying IFS.
| local IFS=. | |
| local i v1_parts=($v1) v2_parts=($v2) | |
| local IFS_SAVE="$IFS" | |
| IFS='.' | |
| local v1_parts=() | |
| local v2_parts=() | |
| read -ra v1_parts <<< "$v1" | |
| read -ra v2_parts <<< "$v2" | |
| IFS="$IFS_SAVE" | |
| local i |
ruleset-fetcher.sh
Outdated
|
|
||
| print_info "Downloading update..." | ||
|
|
||
| local temp_file="/tmp/ruleset-fetcher-update.sh" |
Copilot
AI
Jan 15, 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.
Using a hardcoded path in /tmp for the update file could create a security vulnerability. An attacker could pre-create this file or symlink it to a sensitive location. Use mktemp to generate a secure temporary file instead.
| local temp_file="/tmp/ruleset-fetcher-update.sh" | |
| local temp_file | |
| temp_file="$(mktemp -t ruleset-fetcher-update.XXXXXX)" || { | |
| print_error "Failed to create temporary file for update" | |
| return 1 | |
| } |
|
|
||
| return 0 | ||
| } |
Copilot
AI
Jan 15, 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 comparison function returns 0 for equal versions (at line 695 also), but this creates ambiguity since return code 0 also indicates success in the equal check at line 678. While functionally correct in this context, it could be more maintainable to have the function return distinct codes for all three cases consistently throughout, avoiding the duplicate return 0 at the end that can never be reached after the loop.
|
@copilot make changes based on the comments in this thread |
|
@prettyleaf I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you. |
- Use mktemp for secure temporary file creation - Add robust version extraction with format validation - Save and restore IFS variable properly - Validate backup creation success before proceeding - Add comprehensive error handling for mv and chmod operations - Implement automatic backup restoration on failure - Download from tagged version instead of main branch Co-authored-by: prettyleaf <81024464+prettyleaf@users.noreply.github.com>
Co-authored-by: prettyleaf <81024464+prettyleaf@users.noreply.github.com>
- Check for existing backup file at start of self_update - Calculate backup file age using stat command (supports both Linux and BSD) - Delete backup if older than 86400 seconds (1 day) - Log cleanup action for audit trail Co-authored-by: prettyleaf <81024464+prettyleaf@users.noreply.github.com>
- Extract timestamp retrieval into separate variable - Use explicit calculation for one_day_seconds (24 * 60 * 60) Co-authored-by: prettyleaf <81024464+prettyleaf@users.noreply.github.com>
- Validate backup_timestamp is non-empty and numeric - Prevents arithmetic errors if stat command fails - Silently skips cleanup if timestamp cannot be determined Co-authored-by: prettyleaf <81024464+prettyleaf@users.noreply.github.com>
fix: harden self-update security and error handling
No description provided.