Skip to content

Audit and fix: security, bugs, and code quality#10

Draft
Copilot wants to merge 1 commit intomainfrom
copilot/audit-and-fix-code-issues
Draft

Audit and fix: security, bugs, and code quality#10
Copilot wants to merge 1 commit intomainfrom
copilot/audit-and-fix-code-issues

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 6, 2026

Summary

Full audit of all three platform implementations (Python, macOS Bash, Windows PowerShell) ahead of public release. All issues found are fixed below.


Security Fixes

HTTP → HTTPS for public IP/ISP lookups (macos/lib/network.sh, windows/ConnectivityDropMonitor.ps1)

All three versions fetch the user's public IP and ISP name from ip-api.com. The macOS and Windows versions were using plain HTTP, which means the response could be intercepted and modified by anyone on the network path (a real concern on public Wi-Fi or hostile networks). Changed to HTTPS.

HTML injection in generated reports (python/connectivity_monitor/html_report.py)

ISP name, public IP, target hostnames, and drop event data were interpolated directly into the HTML report string without escaping. A crafted API response (or an ISP name containing <script> or & characters) would break the report or execute code. Added html.escape() for all external/user-supplied values before HTML injection.


Bug Fixes

headless_config falsy check (python/connectivity_monitor/config.py)

if args.poll: is False when --poll 0 is passed. The check should be if args.poll is not None: to honour any explicit value. Same fix applied to args.threshold.

No input validation in interactive setup (python/connectivity_monitor/config.py)

The interactive prompts used bare int() casts. Typing a non-numeric value (e.g. "two") would raise an unhandled ValueError and crash the program. Replaced with a _prompt_int() helper that re-prompts until a valid integer is entered.

Confusing variable names in duration calculation (macos/lib/logging.sh)

log_drop and log_threshold_breach had variables named now_s (holding the end timestamp) and end_s (holding the start timestamp) — exactly reversed from what the names imply. The arithmetic happened to be correct because both variables were swapped, but this was a maintenance hazard. Renamed to start_s and end_s.


Code Quality

File Change
python/connectivity_monitor/metrics.py Moved import datetime from inside three functions to module level
python/connectivity_monitor/network.py Used try/finally for socket in get_local_ip so it always closes even on exception
python/connectivity_monitor/metrics.py Renamed single-letter variables l, a, jloss_pct, avg_lat, jitter_ms in get_health_score

Validation

  • Python: py_compile passes for all 10 modules; functional tests pass including HTML-escape assertions
  • macOS: bash -n syntax check passes for all 8 shell files
  • CodeQL security scan: 0 alerts
  • Code review: no comments

- Fix HTTP → HTTPS for ip-api.com in macOS and Windows to prevent
  man-in-the-middle interception of public IP/ISP lookups
- Fix falsy-check bug in headless_config: use `is not None` for poll
  and threshold so explicit value 0 is honoured
- Add input validation in interactive_setup: replace bare int() casts
  with _prompt_int() which re-prompts on invalid input
- Add HTML escaping (html.escape) for ISP name, public IP, target
  names, and drop data injected into HTML reports
- Move import datetime from inside functions to module level in metrics.py
- Use try/finally for socket cleanup in network.py get_local_ip
- Rename single-letter variables (l, a, j) in get_health_score to
  loss_pct, avg_lat, jitter_ms for readability
- Fix confusingly-named variables now_s/end_s → start_s/end_s in
  macos/lib/logging.sh log_drop and log_threshold_breach

Agent-Logs-Url: https://github.com/nexuspcs/ConnectivityMonitor/sessions/36342ce1-c124-412a-9d81-904bbde9071a

Co-authored-by: nexuspcs <69493073+nexuspcs@users.noreply.github.com>
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.

2 participants