Skip to content

Add automatic update checking with SHA256 hash comparison#53

Merged
pirate merged 1 commit intomasterfrom
claude/issue-21-20251203-2127
Dec 6, 2025
Merged

Add automatic update checking with SHA256 hash comparison#53
pirate merged 1 commit intomasterfrom
claude/issue-21-20251203-2127

Conversation

@pirate
Copy link
Owner

@pirate pirate commented Dec 6, 2025

Implements simple auto-update checking as requested in issue #21:

  • Fetches latest script version from GitHub raw
  • Compares SHA256 hashes of local vs remote versions
  • Shows alert notification when new version is detected
  • Includes direct link to releases page for downloading
  • Checks once per day to avoid excessive network calls
  • Gracefully handles network failures without false alerts

Closes #21

Generated with Claude Code

Implements simple auto-update checking as requested in issue #21:
- Fetches latest script version from GitHub raw
- Compares SHA256 hashes of local vs remote versions
- Shows alert notification when new version is detected
- Includes direct link to releases page for downloading
- Checks once per day to avoid excessive network calls
- Gracefully handles network failures without false alerts

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Nick Sweeting <pirate@users.noreply.github.com>
@pirate pirate merged commit 711e725 into master Dec 6, 2025
2 checks passed
@claude
Copy link

claude bot commented Dec 6, 2025

Pull Request Review

Thanks for implementing the auto-update checking feature! This is a well-structured implementation that addresses issue #21. Here's my detailed review:

✅ Strengths

  1. Clean Implementation: The code follows the existing codebase patterns well, with clear function separation and appropriate error handling.

  2. Sensible Rate Limiting: Checking once per day is a good balance between staying updated and avoiding excessive network calls.

  3. Graceful Failure Handling: The implementation correctly returns empty events on failures rather than alerting users of network issues.

  4. Consistent Style: Follows the codebase conventions with proper docstrings and type hints.

🔍 Code Quality Issues

1. Missing Configuration Flag (Priority: High)

The README shows that all monitors have configuration flags (e.g., MONITOR_SSH, MONITOR_ARP_SPOOF). This feature should follow the same pattern:

# In the xbar.var section (line ~31):
# <xbar.var>boolean(MONITOR_UPDATES=true): Check for updates once per day</xbar.var>

# In the config section (line ~73):
MONITOR_UPDATES = os.environ.get("MONITOR_UPDATES", "true").lower() == "true"

# In check_for_updates():
def check_for_updates(state: Dict[str, Any]) -> List[Tuple[str, str, str]]:
    """Check if a new version is available on GitHub."""
    events = []
    
    if not MONITOR_UPDATES:
        return events
    
    # ... rest of implementation

2. Hardcoded Branch Name (Priority: Medium)

The URL hardcodes master branch, which may not always be correct:

Line 171:

github_raw_url = "https://github.com/pirate/security-growler/raw/master/security-growler.30s.py"

Consider making this a constant at the top of the file or using a more flexible approach (e.g., checking the main branch first, then falling back to master).

3. Potential Race Condition with Hash Comparison (Priority: Low)

If the remote script is updated during the download, the hash comparison might detect a "new version" even when the user already has the latest. This is acceptable for the use case but worth noting.

🐛 Potential Bugs

4. State Persistence Timing (Priority: Medium)

The last_update_check timestamp is set even when an update is found. This means if a user sees the notification but doesn't update, they won't be reminded again for another day. Consider:

# Only update timestamp if no update was found
if local_hash == remote_hash:
    state["last_update_check"] = datetime.now().isoformat()
else:
    # Don't update timestamp so user gets reminded on next check
    title = "UPDATE AVAILABLE"
    body = "New version of Security Growler detected"
    download_url = "https://github.com/pirate/security-growler/releases"
    events.append(("alert", title, f"{body} - {download_url}"))

This way, users get reminded on every run (every 30s as per xbar polling) until they update.

5. Script Path Resolution (Priority: Low)

Using os.path.abspath(__file__) should work correctly, but consider that if the script is run via symlink, this will resolve to the symlink target. This is likely the desired behavior, but worth documenting.

⚡ Performance Considerations

  1. Timeout Configuration: The 15-second timeout on subprocess.run with a 10-second --max-time for curl is good. The subprocess timeout should exceed curl's timeout to allow for proper cleanup.

  2. Network Call Efficiency: The implementation is efficient - only one network call per day, and it uses capture_output=True to avoid unnecessary memory allocation.

  3. Hash Computation: Reading the file in 4096-byte chunks is efficient and handles large files well (though the script is small).

🔒 Security Concerns

6. No HTTPS Verification (Priority: High)

While curl by default verifies certificates, it would be more secure to explicitly ensure this:

result = subprocess.run(
    ["curl", "--max-time", "10", "--silent", "--location", "--fail", github_raw_url],
    capture_output=True,
    timeout=15
)

Adding --fail makes curl return non-zero for HTTP errors (404, 500, etc.), preventing false update alerts.

7. URL Injection Not Possible

The GitHub URL is hardcoded, so there's no risk of URL injection attacks.

8. Hash Comparison Security

SHA256 is cryptographically secure for this use case. The implementation correctly uses secure hashing.

🧪 Test Coverage

Issue: No tests exist for this feature (or the codebase in general based on the glob results).

Recommendation: While the codebase doesn't have tests, consider manually testing:

  • ✅ First run (no last_update_check in state)
  • ✅ Second run within 24 hours (should skip check)
  • ✅ Run after 24+ hours (should check again)
  • ✅ Network failure scenarios (curl timeout, DNS failure)
  • ✅ GitHub unavailable (should fail gracefully)
  • ✅ Same version (no alert)
  • ✅ Different version (shows alert with correct URL)

📝 Documentation

The PR description is excellent and the code is well-commented. Consider updating the README to document:

  • The new auto-update checking feature
  • How to disable it via configuration
  • The update checking frequency

Summary

Required Changes:

  1. Add MONITOR_UPDATES configuration flag for consistency with other monitors
  2. Add --fail flag to curl for better error handling

Recommended Changes:

  1. Don't update last_update_check timestamp when an update is found (so users get reminded)
  2. Make the branch name configurable or document the hardcoded master branch
  3. Update README to document the new feature

Overall Assessment: This is a solid implementation that adds valuable functionality. With the required changes above, it will be ready to merge. Great work on handling edge cases gracefully! 🎉


Review generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check for updates / auto-update

1 participant