Skip to content

Security hardening, UX improvements, and monitoring#10

Closed
darox wants to merge 21 commits intomainfrom
claude/review-project-improvements-eUgOu
Closed

Security hardening, UX improvements, and monitoring#10
darox wants to merge 21 commits intomainfrom
claude/review-project-improvements-eUgOu

Conversation

@darox
Copy link
Copy Markdown
Owner

@darox darox commented Apr 16, 2026

Summary

  • Security hardening: TOFU cert pinning (reject on mismatch), bounded arrays, sanitized API key in logs, auth failure stops polling
  • UX improvements: scrollable menu, redesigned preferences with HIG-compliant Form layout, credential editing UX
  • Monitoring: alerts, security, traffic, events, DDNS, port forwards, nearby APs, speed test, VPN sections
  • Robustness: error handling, observability, wake-from-sleep recovery, network path change handling, backoff

Test plan

  • Verify Preferences window shows configured/editing states correctly
  • Confirm cert pin mismatch shows error and "Reset Certificate Pin" button
  • Check that 401/403 stops polling and shows "Check API Key" message
  • Verify nearby APs show correct dBm values (not 0)
  • Confirm legacy API failures silently hide sections
  • Verify wake-from-sleep and network change recover polling

🤖 Generated with Claude Code

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 16, 2026

Failed to generate code suggestions for PR

@darox darox force-pushed the claude/review-project-improvements-eUgOu branch 2 times, most recently from 24ef677 to a46c6a5 Compare April 16, 2026 06:21
@darox
Copy link
Copy Markdown
Owner Author

darox commented Apr 16, 2026

/pr-agent

@darox
Copy link
Copy Markdown
Owner Author

darox commented Apr 16, 2026

/review

@darox
Copy link
Copy Markdown
Owner Author

darox commented Apr 16, 2026

/pr-agent review

@darox
Copy link
Copy Markdown
Owner Author

darox commented Apr 16, 2026

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 16, 2026

Preparing review...

@darox
Copy link
Copy Markdown
Owner Author

darox commented Apr 16, 2026

/review

claude and others added 9 commits April 16, 2026 09:19
@claude @darox
Improve error handling, observability, and robustness
f287d1f 
- Add structured logging (os.Logger) to UniFiClient and StatusBarController
  so network failures and state transitions are no longer silent
- Add exponential backoff to polling (30s → 5min) on consecutive errors
  to avoid hammering the controller during outages
- Fix observer/monitor leak: add deinit to StatusBarController that
  removes the wake observer and cancels the NWPathMonitor
- Surface WiFi tx retries percentage from V2ClientDTO (was available
  but never displayed); remove dead txRetriesPct field from APStats
- Improve SetupView error messages: distinguish auth failures, HTTP
  errors, unreachable controller, and missing sites
- Make build scripts resilient: default version.env values, graceful
  handling of missing icon assets

https://claude.ai/code/session_01Pmx6pGDgBmZ6aCxwQeHPPz
@claude @darox
Add comprehensive monitoring: alerts, security, traffic, events, DDNS…
0874fc1 
…, port forwards, nearby APs, speed test

New features:
- Alerts section with active alarm count and badge in status bar
- Security section (IDS/IPS events, anomalies) with threat summary
- Traffic section (DPI) showing top bandwidth categories
- Recent events section with subsystem-specific icons
- Dynamic DNS status section
- Port forwards section showing active rules
- Nearby APs section with signal strength
- Speed test results in Internet section (download/upload/ping)
- Collapsible sections with persistent expand/collapse state
- Section visibility preferences (toggle sections on/off)
- Smart data fetching: only queries enabled sections' APIs

Architecture:
- All new fields are optional (nil default) for backward compatibility
- New API calls are parallel, independent, and fail silently
- Section visibility persisted in UserDefaults with sensible defaults
- Core sections (Internet, VPN, WiFi, Network, Session History) enabled by default
- Monitoring sections (Security, Traffic, Events, DDNS, Port Forwards, Nearby APs) opt-in
- Alerts enabled by default as they're high-signal
- ViewBuilder 10-component limit respected via view decomposition

https://claude.ai/code/session_01Pmx6pGDgBmZ6aCxwQeHPPz
@claude @darox
Harden new monitoring code: bound arrays, sanitize API key, clean up …
e71d5f1 
…on reset

Security fixes:
- Cap fetchDDNSStatus and fetchPortForwards response arrays to prevent
  memory exhaustion from malicious controller responses
- Strip control characters from API key to prevent HTTP header injection
- Clear certificate pin hash and section visibility on resetAll()
  to avoid orphaned sensitive data in UserDefaults

https://claude.ai/code/session_01Pmx6pGDgBmZ6aCxwQeHPPz
@claude @darox
Address all security audit findings
f1673e2 
Certificate pinning (HIGH):
- Reject connections on cert mismatch instead of silently re-pinning
  (the silent re-pin completely defeated TOFU protection against MITM)
- Introduce PinState enum: unpinned → pinned → mismatch (terminal)
- Move certificate pin hash from UserDefaults to Keychain
  (UserDefaults is a plaintext plist readable by same-user processes)
- Add deletePinFromKeychain() for clean reset flow
- User must reset via Preferences to re-pin after cert rotation

Input validation (MEDIUM):
- Truncate all displayMessage properties to 200 chars max
- Truncate AP names, port forward names/summaries to 64 chars
- Clamp RSSI values to physically plausible range [-120, 0] dBm
- Clamp timestamps to reasonable epoch range (2000-2100) before
  passing to RelativeDateTimeFormatter

Logging (MEDIUM):
- Replace all `\(error)` in log statements with safeErrorDescription()
  that only emits error domain/code — never full URLs or hostnames
- Remove device/site identifiers from warning log messages

Denial of service (LOW):
- Add 5-second debounce to refreshNow() to prevent request flooding
  from rapid manual refresh clicks

Concurrency (LOW):
- Replace deinit with explicit tearDown() method called from
  applicationWillTerminate — deinit is nonisolated in Swift 6 and
  cannot safely access @MainActor-isolated properties

https://claude.ai/code/session_01Pmx6pGDgBmZ6aCxwQeHPPz
@claude @darox
Fix remaining security findings from re-audit (5 medium, 6 low)
07301af 
Certificate pinning (MEDIUM):
- M1: Reject connection when public key extraction fails instead of
  trusting blindly — prevents bypass via malformed certificates
- M2: Make TOFU pin state read-check-write atomic in single withLock
  block — prevents race between concurrent TLS challenges
- M4: Use delete+add instead of SecItemUpdate for Keychain cert pin
  to ensure kSecAttrAccessible is always set correctly

Transport security (MEDIUM):
- M3: Enforce HTTPS-only for controller URL in both SetupView and
  PreferencesView — prevents API key transmission in cleartext

Error handling (MEDIUM+LOW):
- M5: Replace error.localizedDescription with generic message in
  SetupView catch-all to avoid leaking controller URL
- L1: Fix last remaining raw \(error) interpolation in os_log

UI hardening (LOW):
- L3: Add lineLimit(1) + truncationMode to MetricRow value text
- L4: Wrap DDNSStatusDTO.displayStatus default case in truncated()
- L5: Truncate DDNS hostname to 128 chars at view layer
- L6: Truncate VPN tunnel name to 128 chars at view layer

https://claude.ai/code/session_01Pmx6pGDgBmZ6aCxwQeHPPz
@claude @darox
Fix final low-severity findings from third audit
6cfbad1 
- Eliminate redundant Keychain write on every TLS handshake: capture
  shouldPersist flag from atomic withLock block, only write on first pin
- Reject controller URLs containing query parameters or fragments to
  prevent malformed API requests

https://claude.ai/code/session_01Pmx6pGDgBmZ6aCxwQeHPPz
@claude @darox
Bound remaining unbounded arrays and cap error counter
8af6b3a 
- Cap decoded v2 active clients array at 5,000 entries to prevent
  memory exhaustion from compromised controller responses
- Cap session history response at 1,000 entries
- Cap consecutiveErrors at 10 to prevent integer overflow in
  poll interval bit-shift calculation (1 << 10 = 1024, safely < Int.max)

https://claude.ai/code/session_01Pmx6pGDgBmZ6aCxwQeHPPz
@darox
Security hardening, UX improvements, and CI setup
09c3b8c 
Security:
- Fix cert pin MITM vulnerability: reject mismatch instead of auto-repin
- Add cert validity checks (expiration, hostname) to PinnedCertDelegate
- Surface certChanged error state with reset button in menu & preferences
- Stop polling on auth failure (401/403) instead of infinite retry loop
- Cap consecutive error backoff at 4 (max ~8min) instead of 10
- Clear stale data (satisfaction, signal) on error states
- Remove DDNS login credentials from UI display
- Cap unbounded device array at 500 entries
- Fix nearby APs showing 0 dBm (use signal field, convert rssi correctly)

UX:
- Scrollable menu setting (on by default)
- Redesign Preferences with grouped Form, glass effect, edit/configured states
- No Keychain prompt on cancel, settings save immediately
- Version number shown in Preferences
- Distinct status bar icons per error type (unreachable, auth, cert changed)

CI:
- Release workflow triggered from GitHub UI (tag-only, no binaries)
- PR-Agent workflow with GLM 5.1:cloud
- Actions pinned to SHAs, SHA pinning enforced
- Branch protection: CODEOWNERS review for .github/ changes
- Dependabot for weekly action updates

AI assisted to create this change
@darox
Fix PR-Agent CI config and address code review findings
bd1565b 
- Use openai/ prefix for LiteLLM model routing
- Add custom_reasoning_model for glm-5.1:cloud reasoning output
- Expand comment trigger to match standard PR-Agent commands
- Fix integer overflow in rogue AP sort (Int.min - 95)
- Use stable identity for RogueAPDTO instead of UUID()
- Make DDNSStatusDTO Identifiable with composite key
- Replace fragile offset-based ForEach with stable identity

AI assisted to create this change
@darox darox force-pushed the claude/review-project-improvements-eUgOu branch from 687d978 to bd1565b Compare April 16, 2026 07:22
Repository owner deleted a comment from github-actions Bot Apr 16, 2026
@darox
Copy link
Copy Markdown
Owner Author

darox commented Apr 16, 2026

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 16, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Certificate validation bypass:
validateCertificate cannot reliably distinguish "self-signed only" from "self-signed AND expired" because SecTrustEvaluateWithError may return errSecNotTrusted for both cases, allowing expired certificates to be accepted. The DDNSStatusDTO.login field is decoded from API responses but is not displayed or logged, so no credential leak is present there. API key control-character stripping and HTTPS-only URL validation are good defensive additions.

⚡ Recommended focus areas for review

Security Bug

validateCertificate(_:for:) claims to reject expired self-signed certs, but SecTrustEvaluateWithError typically returns only one failure reason. For a certificate that is both self-signed AND expired, it may return errSecNotTrusted (untrusted root) as the primary error, which the code explicitly allows through. This means an expired self-signed cert can be accepted and pinned on first connection, or continue to be accepted on subsequent connections if the pin matches. To reliably reject expired certs regardless of chain trust status, extract the leaf certificate from the SecTrust object and check its validity dates directly via SecCertificateCopyValues.

 
private static func validateCertificate(_ trust: SecTrust, for host: String) -> Bool {
    let policy = SecPolicyCreateSSL(true, host as CFString)
    SecTrustSetPolicies(trust, policy)
    // Evaluate — this checks expiration, hostname, etc.
    // Self-signed certs will fail standard evaluation, which is expected;
    // the pin check (done separately) is what authorizes them.
    var error: CFError?
    let valid = SecTrustEvaluateWithError(trust, &error)
    // If evaluation fails, check if it's solely due to a self-signed root (which we allow).
    // A valid pin already confirmed the cert identity, so we only need to reject expired leafs.
    if !valid, let error, CFErrorGetCode(error) != errSecNotTrusted {
        return false
    }
    return true

darox added 7 commits April 16, 2026 09:37
@darox
Fix cert validation: reject expired self-signed certificates
af30d83 
SecTrustEvaluateWithError returns errSecNotTrusted for both untrusted-root
and expired certificates, allowing expired self-signed certs through.
Now extracts the leaf cert and checks validity dates directly via
SecCertificateCopyValues before the trust evaluation step.

AI assisted to create this change
@darox
Add diagnostics, error detail, and update checker
f05ca5f 
Addresses issue #6 — users had no way to diagnose "Controller Unreachable"
or report bugs. Changes:

- DiagnosticsLog: in-memory ring buffer (200 events) with export to clipboard
- ErrorState now carries reason strings (e.g. "DNS lookup failed",
  "Connection timed out") and HTTP codes for API key errors
- StatusBarController records all errors/info to DiagnosticsLog,
  exposes consecutiveErrorCount and currentPollInterval
- Error view shows reason, retry interval, and "Copy Diagnostics" button
- Diagnostics footer section with recent events, copy/clear actions
- UpdateChecker fetches GitHub Releases API, shows new version in footer
- Version displayed in menu footer timestamp area

AI assisted to create this change
@darox
Rename scrollable to compact mode, fix expanded window height
2a12c0f 
- Rename scrollableMenu to compactMode (default off = expanded)
- Compact on: auto-sized window, content determines height
- Compact off: window expands to bottom of screen, scrollable
- Footer buttons are all icon-only (refresh, diagnostics, prefs, quit)
- Revert section collapse logic tied to compact mode

AI assisted to create this change
@darox
Switch DPI to v2 traffic API, move diagnostics to Preferences
cdb3377 
- Replace legacy stat/sitedpi endpoint with v2 traffic API
  (client_usage_by_app) which works on Network 9.1+
- Add V2TrafficResponse, ClientAppUsage, AppUsage DTOs
- Pass siteId to fetchMonitoringData and fetchDPIStats
- Remove monitoring nil-result warning spam from diagnostics log
- Move diagnostics section from menu to Preferences window
- Move version/update indicator from menu footer to Preferences
- Simplify footer to 3 icon-only buttons (refresh, prefs, quit)
- Add debug logging when v2 traffic API returns empty categories

AI assisted to create this change
@darox
Remove broken monitoring endpoints and dead code
966ebda 
- Remove DPI/Traffic section (v2 API returns 404 on UCG Fiber)
- Remove Anomalies (decode fails on current API response format)
- Remove all related DTOs: V2TrafficResponse, ClientAppUsage,
  AppUsage, DPICategoryDTO, DPIStatsResponse, AnomalyDTO
- Remove fetchDPIStats, fetchAnomalies methods from UniFiClient
- Remove .traffic from MenuSection enum
- Remove anomalies from SecuritySection and WiFiStatus
- Make monitoring fetches return error details for diagnostics
- Add flexible array decoding (handles both wrapped and bare formats)
- Enable security section by default
- Clean up probe script (remove broken endpoints)

AI assisted to create this change
@darox
Add configurable poll interval and enhance diagnostics
0f3532f 
- Add poll interval picker in Preferences (10s-300s, default 30s)
- Persist poll interval in UserDefaults, clamp 10-300s
- Error backoff now uses configured base interval
- Enhance diagnostics report with structured sections:
  System, Connection, WiFi, WAN, Gateway, VPN, Network
- Show WiFi details (AP, experience, signal, IP) in
  Preferences Diagnostics section
- Show event detail on second line in monospaced font

AI assisted to create this change
@darox
Remove live status fields from diagnostics, keep only logs
d084ca9 
Diagnostics section now shows: version + update indicator,
consecutive errors, poll interval, and event log with detail.
Live WiFi status (AP, signal, experience, IP) removed — that
info belongs in the menu popover, not in debug logs.

AI assisted to create this change
darox added 5 commits April 16, 2026 15:01
@darox
Remove WAN IP from diagnostics report
94f87aa 
AI assisted to create this change
@darox
Show update available in menu footer below last updated timestamp
ccce434 
Blue 'v1.x.x available' link with download arrow icon, clickable
to open the GitHub release page. Only shown when an update exists.

AI assisted to create this change
@darox
Add 5-tap debug mode on version to toggle fake update indicator
d2fb3e9 
Tap the version number 5 times in Preferences → Diagnostics to
toggle a fake update (v99.99.99). This shows the update indicator
in the menu footer for UI testing. Tap 5 times again to dismiss.

AI assisted to create this change
@darox
Add Fake Update toggle in Diagnostics debug mode
093d5b9 
Visible toggle switch in Preferences → Diagnostics → Debug Mode.
Also still accessible via 5-tap on version number.

AI assisted to create this change
@darox
Remove label from debug mode toggle
5a176aa 
AI assisted to create this change
@darox darox closed this Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 4/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@darox @claude