Skip to content

Swift rewrite#927

Open
max4c wants to merge 19 commits intoSelfControlApp:masterfrom
max4c:swift-rewrite
Open

Swift rewrite#927
max4c wants to merge 19 commits intoSelfControlApp:masterfrom
max4c:swift-rewrite

Conversation

@max4c
Copy link

@max4c max4c commented Mar 17, 2026

Summary by CodeRabbit

Release Notes

  • New Features
    • Added scheduled recurring blocks with customizable weekdays and times.
    • Introduced blocklist/allowlist editor with add, remove, and drag-to-delete functionality.
    • Added ability to extend active blocks by minutes.
    • Created dedicated preferences panel for sound notifications, badge display, and advanced options.
    • Launched command-line interface for block management.

max4c and others added 19 commits March 15, 2026 22:01
Adds the ability to schedule recurring website blocks that fire
automatically on specific days and times. Schedules are managed through
a new Schedules window (accessible from the app menu) and enforced via
launchd user agents that invoke selfcontrol-cli at the configured times.

New files:
- SCSchedule: model for a recurring schedule (days, time, duration, blocklist)
- SCScheduleManager: persists schedules to NSUserDefaults, generates and
  installs/removes launchd plists in ~/Library/LaunchAgents/
- ScheduleListWindowController: programmatic Cocoa UI for CRUD on schedules

Also adds --duration flag to selfcontrol-cli as an alternative to --enddate,
allowing launchd agents to specify block length in minutes rather than
requiring a pre-computed absolute end date.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sets up the Stone project structure for the clean Swift rewrite:

Targets:
- Stone (macOS app) - GUI with AppDelegate entry point
- stonectld (privileged helper daemon) - XPC listener, SMJobBless
- stone-cli (command-line tool) - block management from terminal

Foundation layer (Common/):
- SCError: error codes matching original SelfControl domain
- StoneConstants: bundle IDs, mach service name, defaults, sentinels
- BlockEntry: hostname/IP parser with pf rule and hosts line generation
- SCSchedule: Codable recurring schedule model
- SCDaemonProtocol: @objc XPC protocol for app-daemon communication
- SCSettings: cross-process settings store (root-owned binary plist)
- SCBlockUtilities, SCBlockFileReaderWriter, SCFileWatcher, SCMiscUtilities

Daemon layer (Daemon/):
- SCDaemon: XPC listener delegate with checkup and inactivity timers
- SCDaemonXPC: protocol conformance stub
- AuditTokenBridge: ObjC shim for private NSXPCConnection.auditToken

All three targets compile successfully. Implementation is stubbed —
block enforcement, UI, and full CLI are next phases.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Block enforcement (Common/Block/):
- PacketFilter: manages pf rules via pfctl subprocess, anchor file
  generation, token persistence, append mode for running blocks
- HostFileBlocker: edits /etc/hosts with sentinel markers, supports
  IPv4 + IPv6 blocking entries
- HostFileBlockerSet: coordinates blocking across multiple hosts files
- BlockManager: orchestrates PF + hosts, DNS resolution, common
  subdomain expansion, Google domain handling

XPC communication (Common/XPC/):
- SCXPCAuthorization: wraps AuthorizationServices for daemon method
  rights validation, policy database setup
- SCXPCClient: app-side client with SMJobBless daemon installation,
  NSXPCConnection lifecycle, authorization data threading

Daemon (Daemon/):
- SCDaemonBlockMethods: serialized block logic — start, checkup (1s),
  integrity check (15s), update blocklist, extend end date
- SCDaemonXPC: full protocol implementation with auth validation
- SCDaemon: wired checkup and integrity to block methods

Utilities (Common/Utilities/):
- SCHelperToolUtilities: bridges settings to enforcement — install
  rules, remove block, clear caches, flush DNS

Schedule manager (ScheduleManager/):
- SCScheduleManager: Codable schedule CRUD with UserDefaults storage
- LaunchAgentWriter: launchd plist generation and launchctl operations

CLI (CLI/):
- Full argument parsing: --blocklist, --enddate, --duration, --settings
- Legacy positional arg fallback, UserDefaults fallback
- XPC-based block start with semaphore-based async wait

All three targets compile with zero errors and zero warnings.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AppController: central block start/stop flow — validates preconditions,
installs daemon via XPC, manages window lifecycle, observes distributed
notifications for cross-process configuration changes.

MainWindowController: programmatic main window with duration slider
(1-1440 min), start block button, blocklist/allowlist toggle, edit
blocklist and schedules buttons, entry count summary.

TimerWindowController: countdown display (HH:MM:SS), add-to-block
sheet, extend time popup (15m/30m/1h), dock badge updates, prevents
window close during active block.

DomainListWindowController: editable NSTableView for blocklist entries,
add/remove buttons, quick-add text field. Disables removal during
active blocks, reads/writes UserDefaults.

ScheduleListWindowController: 5-column table (enabled, name, days,
time, duration) with add/edit/remove. Edit sheet has day checkboxes,
time picker, duration field, blocklist text area. All CRUD via
SCScheduleManager with immediate launchd sync.

PreferencesWindowController: two-tab NSTabView. General: sound toggle,
sound picker, badge icon, max block length. Advanced: subdomain eval,
linked domains, cache clearing, local networks, error reporting.

AppDelegate: creates AppController, syncs schedules on launch, handles
dock reopen, keeps app alive during active block.

All three targets compile with zero errors.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The app was crashing on launch because Stone-Info.plist referenced
NSMainNibFile=MainMenu but no MainMenu.xib exists (all UI is
programmatic). Removed the key and set up the full menu bar in code:
Stone menu (About, Preferences, Schedules, Hide, Quit) and Window
menu (Minimize, Close).

Also wired up AppController.showDomainList/showSchedules/showPreferences
methods and connected MainWindowController buttons directly to them
instead of using NotificationCenter.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Critical fixes:
- SCXPCClient: always create fresh auth data with pre-authorized rights
  after installDaemon, fixing nil authData on subsequent launches (SelfControlApp#1)
- SCXPCClient: reconnect before invoking callback in refreshConnectionAndRun
  so proxy() has a live connection (SelfControlApp#2)
- SCDaemonBlockMethods: checkupBlock now takes the lock (tryLock) to prevent
  racing with startBlock and tearing down a block mid-install (SelfControlApp#3)

Security fixes:
- SCXPCAuthorization: remove .interactionAllowed from daemon-side
  checkAuthorization — daemon has no GUI, would block forever (SelfControlApp#9)
- SCXPCAuthorization: unknown commands now fail with INVALID right instead
  of silently falling through to startBlock right (SelfControlApp#10)

Important fixes:
- PacketFilter: addStoneConfig duplicate check now tests for anchor name
  string, consistent with containsStoneBlock() (SelfControlApp#5)
- SCFileWatcher: use passRetained instead of passUnretained in FSEvents
  context to prevent use-after-free on deallocation (SelfControlApp#6)
- SCSettings: schedule sync timer and notification observer on main run
  loop so they fire in the daemon (XPC threads have no run loop) (SelfControlApp#7)
- DomainListWindowController: use DistributedNotificationCenter with
  correct StoneConstants name so domain edits refresh the main UI (SelfControlApp#8)
- SCHelperToolUtilities: derive user home from controlling UID via
  getpwuid when running as root, instead of NSHomeDirectory() which
  returns /var/root in the daemon (SelfControlApp#11)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The programmatic window was created but never brought to front.
Added makeKeyAndOrderFront and NSApp.activate to showMainWindow.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three issues combined to prevent the window from showing:
- NSWindow.isReleasedWhenClosed defaults to true for programmatic
  windows, causing premature deallocation
- applicationShouldTerminateAfterLastWindowClosed was returning true
  and racing with window creation, quitting the app
- start() was doing a clever state-inversion trick that fought with
  the explicit showMainWindow() call in applicationDidFinishLaunching

Simplified: start() just initializes state, AppDelegate decides
which window to show based on block state, and the app never
auto-terminates on last window close.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Programmatic Cocoa apps without a MainMenu.xib default to a
background/accessory activation policy. NSApp.setActivationPolicy(.regular)
is required for the app to show windows in the foreground.

Also extracted showInitialWindow() to AppController for cleaner
separation of launch logic.

Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The programmatic NSWindow approach had persistent issues with window
visibility on launch (activation policy, window lifecycle, ordering).
SwiftUI's WindowGroup handles all of this automatically.

New files:
- StoneApp.swift: @main App entry point with WindowGroup
- MainView.swift: SwiftUI version of the main window using @AppStorage

AppDelegate is now used via NSApplicationDelegateAdaptor for AppKit
interop (XPC, daemon management) while SwiftUI owns the window lifecycle.

The main view uses @AppStorage for BlockDuration, MaxBlockLength,
BlockAsWhitelist, and Blocklist — buttons are stubbed for wiring to
AppController in the next step.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
BlocklistEditorView: add/remove domains with text field, swipe-to-delete
list, entry count. Reads/writes UserDefaults["Blocklist"] directly
as string array (fixing the @AppStorage Data mismatch).

ScheduleEditorView: list of schedules with enable toggle, edit/delete.
Add/edit form with day picker, time stepper, duration, and domain
text editor. All CRUD through SCScheduleManager with launchd sync.

MainView: blocklist now loaded from UserDefaults.stringArray instead
of @AppStorage Data. Both buttons open sheets.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MainView: duration is now the centerpiece — large rounded bold number
with uppercase caption label. Centered layout feels focused instead of
left-aligned and sparse. Mode picker and site count in a compact row.
Action buttons use labels with SF Symbols. Start Block is prominent.

BlocklistEditorView: monospaced domain entries, plus-circle icon on
the add field, auto-focus on appear, empty state with guidance text,
subtle "Remove All" in footer.

ScheduleEditorView: green status dots for enabled schedules, compact
info row with interpuncts, pill-style On/Off toggle, pencil edit
button. Form uses uppercase micro-labels for sections, capsule day
picker with accent color, monospaced time/duration steppers.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MainView now takes an AppController parameter, passed from StoneApp
via the AppDelegate adaptor. The Start Block button calls
appController.startBlock() which validates preconditions, installs
the daemon via SMJobBless, and starts the block via XPC.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Updated SMJobBless requirement strings in both Info.plists to include
certificate leaf[subject.OU] matching the team ID. Set automatic
signing with Apple Development identity across all targets.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In debug builds, startBlock() writes block state directly to
SCSettings instead of going through SMJobBless + XPC. This lets
the full UI flow (main window → timer window → block end) work
during development without code signing the daemon.

Production builds still use the full daemon path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… windows

Three fixes:
- SCSettings: allow writes in DEBUG builds (was read-only because
  app doesn't run as root). Production still enforces root-only writes.
- AppController: now ObservableObject with @published blockIsOn/addingBlock.
  refreshUserInterface simplified — no longer creates AppKit windows.
- MainView: switches between setup view and TimerView based on
  appController.blockIsOn. No more duplicate AppKit windows.
- TimerView: SwiftUI countdown (HH:MM:SS) with 1-second timer,
  auto-clears block state when expired.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SMJobBless error code 8 was caused by the daemon binary missing
embedded plist sections. Fixed by:

- Created stonectld-launchd.plist with Label and MachServices
- Added -sectcreate linker flags to embed both __info_plist and
  __launchd_plist sections in the daemon binary
- Set CREATE_INFOPLIST_SECTION_IN_BINARY=NO to prevent double-embed

The daemon binary now has both plists embedded and is correctly
placed at Contents/Library/LaunchServices/com.max4c.stonectld.
Signing requirements match between app and daemon (team H9N9P29TX5).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Now that SMJobBless is properly configured with embedded plists,
removed the #if DEBUG simulation path. The app always goes through
the real daemon installation and XPC flow. Block state is updated
via SCBlockUtilities.anyBlockIsRunning() after XPC completes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 17, 2026

📝 Walkthrough

Walkthrough

This pull request introduces a comprehensive Swift rewrite of SelfControl, including a SwiftUI/AppKit macOS application, privileged daemon for block enforcement via PacketFilter and hosts file manipulation, XPC communication layer, schedule management with launchd integration, and CLI tooling with supporting shared libraries and project configuration.

Changes

Cohort / File(s) Summary
Application Shell
App/AppDelegate.swift, App/StoneApp.swift, App/AppController.swift
Main application entry point, delegate, and central controller orchestrating block lifecycle, window management, settings synchronization, and daemon interaction via XPC.
SwiftUI User Interface
App/UI/MainView.swift, App/UI/TimerView.swift, App/UI/BlocklistEditorView.swift, App/UI/ScheduleEditorView.swift
SwiftUI view components for main window, countdown timer, blocklist/allowlist editing, and schedule management.
AppKit Window Controllers
App/UI/MainWindowController.swift, App/UI/TimerWindowController.swift, App/UI/DomainListWindowController.swift, App/UI/PreferencesWindowController.swift, App/UI/ScheduleListWindowController.swift
Legacy Cocoa-based window controllers for programmatic UI layout, domain management, preferences, and schedule editing.
Block Enforcement & Rules
Common/Block/BlockManager.swift, Common/Block/PacketFilter.swift, Common/Block/HostFileBlocker.swift, Common/Block/HostFileBlockerSet.swift
Core block enforcement infrastructure using PacketFilter and hosts file manipulation, with DNS resolution, common subdomain expansion, and append-mode support.
Daemon Core
Daemon/DaemonMain.swift, Daemon/SCDaemon.swift, Daemon/SCDaemonBlockMethods.swift, Daemon/SCDaemonXPC.swift, Daemon/AuditTokenBridge...
Privileged helper daemon for handling block lifecycle, XPC connections, integrity checks, and periodic housekeeping with process audit token verification.
XPC Communication
Common/XPC/SCXPCClient.swift, Common/XPC/SCXPCAuthorization.swift, Common/Protocol/SCDaemonProtocol.swift
App-side XPC client with daemon installation, connection management, authorization via SMJobBless, and privilege escalation handling.
Settings & Shared State
Common/Settings/SCSettings.swift, Common/Model/StoneConstants.swift, Common/Errors/SCError.swift
Cross-process persistent settings store, centralized constants, and error type definitions for consistent error handling.
Block Data Models
Common/Model/BlockEntry.swift, Common/Model/SCSchedule.swift
Data structures for blocklist entries with parsing and IP detection, and recurring schedule definitions with launchd label generation.
Scheduling System
ScheduleManager/SCScheduleManager.swift, ScheduleManager/LaunchAgentWriter.swift, SCScheduleManager.m/.h, SCSchedule.m/.h, ScheduleListWindowController.m/.h, SCConstants.m/.h
Schedule persistence in UserDefaults, launchd plist generation and loading, and both Swift/Objective-C implementations for recurring block scheduling.
Utilities & Helpers
Common/Utilities/SCBlockUtilities.swift, Common/Utilities/SCBlockFileReaderWriter.swift, Common/Utilities/SCHelperToolUtilities.swift, Common/Utilities/SCFileWatcher.swift, Common/Utilities/SCMiscUtilities.swift
Helper functions for block state management, plist I/O, browser cache clearing, filesystem watching, and cryptographic utilities.
Command-Line Interface
CLI/CLIMain.swift, cli-main.m
Swift CLI entry point with command routing and daemon interaction, plus duration parameter support in legacy Objective-C CLI.
Project Configuration
Stone.xcodeproj/project.pbxproj, Stone.xcodeproj/xcshareddata/xcschemes/*, project.yml, App/Stone-Info.plist, App/Stone.entitlements, Daemon/stonectld-Info.plist, Daemon/stonectld-launchd.plist, CLI/stone-cli-Info.plist
Complete Xcode project structure with three targets (app, daemon, CLI), build schemes, build configuration, entitlements, and launchd daemon plist.
Documentation
.go/progress.md
Project status report documenting completed infrastructure, UI components, block enforcement, XPC communication, and remaining items (icons, localization, code signing, tests, polish).

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant App as Stone App
    participant Auth as Authorization
    participant Daemon as stonectld Daemon
    participant PF as PacketFilter
    participant Hosts as Host File

    User->>App: Start Block
    activate App
    App->>App: Validate blocklist/enddate
    App->>Auth: Request privilege escalation
    activate Auth
    Auth->>Auth: SMJobBless
    Auth-->>App: Auth data
    deactivate Auth
    
    App->>Daemon: startBlock(blocklist, duration, settings)
    activate Daemon
    Daemon->>Daemon: Verify authorization
    Daemon->>Daemon: Validate block state
    
    par Block Installation
        Daemon->>PF: addRule(IP/hostname)
        activate PF
        PF->>PF: DNS resolution
        PF->>PF: Write pf.conf
        PF->>PF: pfctl load anchor
        PF-->>Daemon: Success
        deactivate PF
    and
        Daemon->>Hosts: addEntry(hostname)
        activate Hosts
        Hosts->>Hosts: Read /etc/hosts
        Hosts->>Hosts: Insert block sentinel
        Hosts->>Hosts: Write /etc/hosts
        Hosts-->>Daemon: Success
        deactivate Hosts
    end
    
    Daemon->>Daemon: Persist block state
    Daemon->>Daemon: Start integrity checker
    Daemon-->>App: Success
    deactivate Daemon
    
    App->>App: Show timer window
    App->>User: Block active (countdown display)
    deactivate App
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

🐰 Behold the fortress of pure Stone!
Rules cascade through kernel zones,
Schedules hop on launchd's stage,
While XPC dances, page by page.
A mighty rewrite, blocks stand tall,
Where PacketFilter guards them all. 🛡️

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Swift rewrite' is vague and generic, using non-descriptive language that doesn't convey meaningful information about the specific changes in this large, complex changeset. Use a more specific title that describes the primary objective, such as 'Rewrite SelfControl app in Swift with new daemon architecture' or 'Add Swift app, daemon, and CLI implementations for Stone rewrite'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can approve the review once all CodeRabbit's comments are resolved.

Enable the reviews.request_changes_workflow setting to automatically approve the review once all CodeRabbit's comments are resolved.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

🟠 Major comments (27)
Common/Utilities/SCMiscUtilities.swift-34-37 (1)

34-37: ⚠️ Potential issue | 🟠 Major

Don't make the fallback settings filename globally predictable.

When serialNumber() returns nil, the hash is computed from a constant "unknown" salt. That defeats the machine-specific obscurity this path is trying to provide on every install that hits the fallback.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Utilities/SCMiscUtilities.swift` around lines 34 - 37, The fallback
uses the literal "unknown" when serialNumber() is nil, making the resulting hash
predictable; change settingsFilePath() to use a machine-specific persisted
fallback identifier instead: introduce or call a helper like
fallbackIdentifier() that if a persisted ID exists returns it, otherwise
generates a secure random UUID, persists it (e.g., in a secure/owned file or
keychain) and returns it, then compute
sha1Hex("\(StoneConstants.settingsFilePrefix)\(serialOrFallback)") using that
ID; update settingsFilePath() to use serialNumber() ?? fallbackIdentifier() and
keep using sha1Hex and StoneConstants.settingsFilePrefix so the fallback is
unique per install.
Common/Utilities/SCBlockFileReaderWriter.swift-7-29 (1)

7-29: ⚠️ Potential issue | 🟠 Major

Validate the .stone schema before letting it cross this API boundary.

readBlocklist only checks that "Blocklist" exists, and writeBlocklist serializes any plist-shaped dictionary. That lets malformed files get past this utility and fail deeper in block application.

💡 Proposed fix
     static func readBlocklist(from url: URL) -> [String: Any]? {
         guard let data = try? Data(contentsOf: url) else { return nil }

         guard let plist = try? PropertyListSerialization.propertyList(
             from: data, options: [], format: nil
         ) as? [String: Any] else {
             return nil
         }

-        guard plist["Blocklist"] != nil else { return nil }
+        guard plist["Blocklist"] is [String] else { return nil }
+        if let blockAsWhitelist = plist["BlockAsWhitelist"], !(blockAsWhitelist is Bool) {
+            return nil
+        }
         return plist
     }
@@
     `@discardableResult`
     static func writeBlocklist(to url: URL, blockInfo: [String: Any]) throws -> Bool {
+        guard blockInfo["Blocklist"] is [String] else {
+            throw SCError.invalidBlocklistEntry
+        }
+        if let blockAsWhitelist = blockInfo["BlockAsWhitelist"], !(blockAsWhitelist is Bool) {
+            throw SCError.invalidBlocklistEntry
+        }
         let data = try PropertyListSerialization.data(
             fromPropertyList: blockInfo,
             format: .binary,
             options: 0
         )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Utilities/SCBlockFileReaderWriter.swift` around lines 7 - 29,
readBlocklist and writeBlocklist currently accept any plist-shaped dictionary
which permits malformed blocklist data to propagate; update readBlocklist to
fully validate the expected .stone schema (required top-level keys like
"Blocklist" and any nested keys, their types, and value constraints) before
returning by either decoding into a strict Swift model (Codable/struct) or by
explicit type-checked assertions and return/throw on failure, and update
writeBlocklist to accept and serialize only that validated model (or perform the
same schema validation on blockInfo before serializing) so malformed
dictionaries are rejected at this boundary (refer to readBlocklist(...) and
writeBlocklist(..., blockInfo: ...) to locate and change the logic).
SCSchedule.m-26-35 (1)

26-35: ⚠️ Potential issue | 🟠 Major

Preserve the model defaults when keys are missing.

integerValue/boolValue on nil collapse to 0/NO, so partial or older dictionaries deserialize to midnight, zero duration, and disabled instead of the defaults from -init.

💡 Proposed fix
 - (instancetype)initWithDictionary:(NSDictionary *)dict {
-    if (self = [super init]) {
-        _identifier = dict[@"identifier"] ?: [[NSUUID UUID] UUIDString];
-        _name = dict[@"name"] ?: @"";
-        _weekdays = dict[@"weekdays"] ?: @[];
-        _hour = [dict[@"hour"] integerValue];
-        _minute = [dict[@"minute"] integerValue];
-        _durationMinutes = [dict[@"durationMinutes"] integerValue];
-        _blocklist = dict[@"blocklist"] ?: @[];
-        _enabled = [dict[@"enabled"] boolValue];
+    if (self = [self init]) {
+        _identifier = dict[@"identifier"] ?: _identifier;
+        _name = dict[@"name"] ?: _name;
+        _weekdays = dict[@"weekdays"] ?: _weekdays;
+        _hour = dict[@"hour"] ? [dict[@"hour"] integerValue] : _hour;
+        _minute = dict[@"minute"] ? [dict[@"minute"] integerValue] : _minute;
+        _durationMinutes = dict[@"durationMinutes"] ? [dict[@"durationMinutes"] integerValue] : _durationMinutes;
+        _blocklist = dict[@"blocklist"] ?: _blocklist;
+        _enabled = dict[@"enabled"] ? [dict[@"enabled"] boolValue] : _enabled;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@SCSchedule.m` around lines 26 - 35, The initWithDictionary: should preserve
defaults previously set by -init by only overwriting properties when the
dictionary actually contains a non-nil/non-NSNull value; change each assignment
in initWithDictionary: (e.g. _identifier, _name, _weekdays, _hour, _minute,
_durationMinutes, _blocklist, _enabled) to first check the dictionary key (e.g.
id v = dict[@"hour"]; if (v && v != [NSNull null]) _hour = [v integerValue];)
and for object properties only assign when dict[@"key"] is present, and for
numeric/bool properties only call integerValue/boolValue when the value exists,
so missing keys leave the model defaults intact.
Common/Utilities/SCFileWatcher.swift-15-16 (1)

15-16: ⚠️ Potential issue | 🟠 Major

Address memory leak and re-entrance bugs in start().

The passRetained(self) at line 21 increments the reference count before the FSEventStreamCreate guard at line 31. If stream creation fails, the function returns without releasing the retained context, causing a memory leak.

Additionally, calling start() twice overwrites the previous stream reference without stopping it. The old stream continues running with callbacks while orphaned, causing duplicate callbacks and resource leaks.

Finally, FSEventStreamStart at line 47 ignores its Boolean return value. If it fails, the stream is scheduled but not started, leaving resources in an inconsistent state.

Recommended fixes:

  • Guard against re-entrance with guard stream == nil else { return } at the start
  • Store the retained self before creating the context and release it if FSEventStreamCreate fails
  • Check the return value of FSEventStreamStart and clean up resources if it fails
  • Assign to self.stream only after FSEventStreamStart succeeds
Proposed diff
     func start() {
+        guard stream == nil else { return }
         let pathsToWatch = [path] as CFArray

         // [Fix `#6`] Use passRetained to prevent use-after-free if watcher is
         // deallocated before the stream is invalidated.
-        var context = FSEventStreamContext()
-        context.info = Unmanaged.passRetained(self).toOpaque()
+        let retainedSelf = Unmanaged.passRetained(self)
+        var context = FSEventStreamContext()
+        context.info = retainedSelf.toOpaque()
         context.release = { info in
             guard let info = info else { return }
             Unmanaged<SCFileWatcher>.fromOpaque(info).release()
         }

         let flags: FSEventStreamCreateFlags = UInt32(
             kFSEventStreamCreateFlagFileEvents | kFSEventStreamCreateFlagUseCFTypes
         )

         guard let stream = FSEventStreamCreate(
             nil,
             { _, info, _, _, _, _ in
                 guard let info = info else { return }
                 let watcher = Unmanaged<SCFileWatcher>.fromOpaque(info).takeUnretainedValue()
                 watcher.callback()
             },
             &context,
             pathsToWatch,
             FSEventStreamEventId(kFSEventStreamEventIdSinceNow),
             1.0, // latency in seconds
             flags
-        ) else { return }
+        ) else {
+            retainedSelf.release()
+            return
+        }

         FSEventStreamScheduleWithRunLoop(stream, CFRunLoopGetCurrent(), CFRunLoopMode.defaultMode.rawValue)
-        FSEventStreamStart(stream)
+        guard FSEventStreamStart(stream) else {
+            FSEventStreamInvalidate(stream)
+            FSEventStreamRelease(stream)
+            return
+        }
+        self.stream = stream
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Utilities/SCFileWatcher.swift` around lines 15 - 16, start() must
defend against re-entrance and avoid leaking the retained context or orphaning
streams: add a guard at the beginning (guard stream == nil else { return }) to
prevent double-starts; call CFBridgingRetain(self) (the current
passRetained(self) usage) and store the retained reference in a local variable
before calling FSEventStreamCreate, and if FSEventStreamCreate returns nil
release that retained reference to avoid the leak; call FSEventStreamStart and
check its Boolean result—if it fails, invalidate/stop unsafely-created resources
and release the retained context; only assign the created stream to self.stream
after FSEventStreamStart returns true so you don’t overwrite a running stream.
App/UI/TimerView.swift-51-56 (1)

51-56: ⚠️ Potential issue | 🟠 Major

Make the timer lifecycle idempotent.

startTimer() always installs a new repeating timer without stopping an existing one, so if onAppear triggers multiple times (which can occur during view hierarchy changes), the old timer reference is overwritten and never invalidated. Additionally, when endDate is nil in updateTimeRemaining(), the early return leaves any existing timer running indefinitely instead of stopping it.

💡 Proposed fix
     private func startTimer() {
+        stopTimer()
         updateTimeRemaining()
         timer = Timer.scheduledTimer(withTimeInterval: 1, repeats: true) { _ in
             updateTimeRemaining()
         }
     }
@@
     private func updateTimeRemaining() {
         guard let end = endDate else {
+            stopTimer()
             timeRemaining = 0
             return
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@App/UI/TimerView.swift` around lines 51 - 56, startTimer() currently always
schedules a new Timer without stopping any existing one and
updateTimeRemaining() returns early when endDate is nil leaving timers running;
before creating a new Timer in startTimer() check if the existing timer (timer)
is active and invalidate it (set timer = nil) so scheduling is idempotent, and
in updateTimeRemaining() where you early-return on a nil endDate also invalidate
and nil out the timer to ensure no stray timer runs; reference the startTimer(),
updateTimeRemaining(), and timer property to locate the fixes.
Common/Utilities/SCMiscUtilities.swift-16-22 (1)

16-22: ⚠️ Potential issue | 🟠 Major

Use takeRetainedValue() for IORegistryEntryCreateCFProperty.

This is a Create-rule CoreFoundation API that returns a reference with +1 retain count. takeUnretainedValue() leaks the returned property every time serialNumber() succeeds, as it prevents ARC from releasing the owned reference. Use takeRetainedValue() instead to transfer ownership to ARC.

Proposed fix
-        return serialRef.takeUnretainedValue() as? String
+        return serialRef.takeRetainedValue() as? String
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Utilities/SCMiscUtilities.swift` around lines 16 - 22, The code in
serialNumber() uses IORegistryEntryCreateCFProperty (a Create-rule CF API) but
calls serialRef.takeUnretainedValue(), which leaks the returned CF object;
change the ownership transfer to serialRef.takeRetainedValue() so ARC takes
ownership and releases the object properly—update the return in serialNumber()
to use takeRetainedValue() instead of takeUnretainedValue().
Common/Errors/SCError.swift-4-20 (1)

4-20: ⚠️ Potential issue | 🟠 Major

Add CustomNSError conformance to expose the domain to NSError/ObjC consumers.

LocalizedError only populates localized descriptions in userInfo. When bridging to NSError, Swift defaults the domain to the mangled type name unless CustomNSError is adopted. XPC and Objective-C consumers checking for com.max4c.stone.error will not find it without this conformance.

Proposed fix
-enum SCError: Int, LocalizedError {
+enum SCError: Int, LocalizedError, CustomNSError {
     var errorDescription: String? {
         // ...
     }
 
+    static var errorDomain: String { domain }
     static let domain = "com.max4c.stone.error"
+    var errorCode: Int { rawValue }
 }

Also applies to: 43-43

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Errors/SCError.swift` around lines 4 - 20, The enum SCError should
adopt CustomNSError so NSError/ObjC/XPC consumers see the correct domain; update
the declaration of enum SCError to conform to CustomNSError and implement the
static var errorDomain returning "com.max4c.stone.error" and the var errorCode
to return rawValue (and optionally errorUserInfo to map
LocalizedError.localizedDescription), ensuring SCError (the enum name) exposes
the canonical Objective-C domain when bridged.
App/UI/DomainListWindowController.swift-84-91 (1)

84-91: ⚠️ Potential issue | 🟠 Major

Don't persist the empty placeholder row.

Line 112 auto-creates a placeholder row, and Line 125 immediately routes it through saveDomainList(). Opening the editor with an empty list therefore changes persisted configuration before the user has entered anything. Filter blank entries out of the save path, or defer persistence until edit commit.

🩹 Suggested change
 private func saveDomainList() {
-    defaults.set(domainList, forKey: "Blocklist")
+    let persistedDomains = domainList
+        .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) }
+        .filter { !$0.isEmpty }
+    defaults.set(persistedDomains, forKey: "Blocklist")
     // [Fix `#8`] Use DistributedNotificationCenter with the correct name
     DistributedNotificationCenter.default().postNotificationName(
         NSNotification.Name(StoneConstants.configurationChangedNotification),
         object: nil
     )
 }

Also applies to: 109-115, 123-129

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@App/UI/DomainListWindowController.swift` around lines 84 - 91, The
saveDomainList() function is persisting the auto-created empty placeholder row;
before calling defaults.set and posting the configurationChangedNotification,
filter domainList to remove blank/whitespace-only entries (e.g. let cleaned =
domainList.filter { !$0.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty
}) and save only cleaned, or alternatively defer calling saveDomainList() until
the editor commit path; update saveDomainList() (and any callers that
immediately invoke it after placeholder creation) to use the cleaned list and
only post the DistributedNotificationCenter notification when the persisted list
actually changes.
App/UI/DomainListWindowController.swift-98-115 (1)

98-115: ⚠️ Potential issue | 🟠 Major

Reload domainList before showing a reused controller.

domainList is only refreshed when somebody explicitly calls refreshDomainList(). If this window controller instance is reopened after another code path changed Blocklist, it will show stale rows and can later save that stale array back over the newer value.

🩹 Suggested change
 override func showWindow(_ sender: Any?) {
+    refreshDomainList()
     window?.makeKeyAndOrderFront(sender)
     if domainList.isEmpty && !SCBlockUtilities.anyBlockIsRunning() {
         addDomain(self)
     }
     updateWindowTitle()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@App/UI/DomainListWindowController.swift` around lines 98 - 115, The
showWindow(_:) path can present stale domainList when the controller is reused;
before making the window key/visible, call the existing refresh logic so the
controller always reloads the latest Blocklist data. Modify showWindow(_:) to
invoke refreshDomainList() (or directly call
loadDomainList()/tableView.reloadData() if you need synchronous behavior) before
window?.makeKeyAndOrderFront(sender) so domainList is up-to-date; keep the
existing addDomain(self) and updateWindowTitle() behavior after the refresh.
App/UI/DomainListWindowController.swift-123-154 (1)

123-154: ⚠️ Potential issue | 🟠 Major

Guard every mutation path while a block is active.

Unlike removeDomain(_:) and tableView(_:shouldEdit:row:), addDomain(_:) and quickAdd(_:) do not check SCBlockUtilities.anyBlockIsRunning(). During an active block the user can still mutate Blocklist, which leaves the editor half-locked and can persist entries they cannot edit or remove until the block ends.

🩹 Suggested change
 `@objc` private func addDomain(_ sender: Any) {
+    guard !SCBlockUtilities.anyBlockIsRunning() else { return }
     domainList.append("")
     saveDomainList()
     tableView.reloadData()
     let lastRow = domainList.count - 1
     tableView.selectRowIndexes(IndexSet(integer: lastRow), byExtendingSelection: false)
     tableView.editColumn(0, row: lastRow, with: nil, select: true)
 }

 `@objc` private func quickAdd(_ sender: NSTextField) {
+    guard !SCBlockUtilities.anyBlockIsRunning() else { return }
     let text = sender.stringValue.trimmingCharacters(in: .whitespacesAndNewlines)
     guard !text.isEmpty else { return }
     if !domainList.contains(text) {
         domainList.append(text)
         saveDomainList()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@App/UI/DomainListWindowController.swift` around lines 123 - 154,
addDomain(_:) and quickAdd(_:) mutate domainList without checking
SCBlockUtilities.anyBlockIsRunning(), allowing edits during an active block; add
the same guard used in removeDomain(_:) (call
SCBlockUtilities.anyBlockIsRunning() and return if true) at the start of both
addDomain(_:) and quickAdd(_:) so no mutations occur while a block is running,
then proceed with existing append/save/reload logic.
Common/Settings/SCSettings.swift-99-119 (1)

99-119: ⚠️ Potential issue | 🟠 Major

Only broadcast after the plist write succeeds.

Lines 115-119 run even when serialization or data.write(...) throws, so peer processes are told to reload a state that never reached disk.

🩹 Suggested change
     private func writeToDisk() {
         lock.lock()
         var toWrite = settings
         toWrite["SettingsVersionNumber"] = versionNumber
         toWrite["LastSettingsUpdate"] = lastUpdate
         lock.unlock()

+        var didWriteFile = false
         do {
             let data = try PropertyListSerialization.data(
                 fromPropertyList: toWrite, format: .binary, options: 0
             )
             try data.write(to: URL(fileURLWithPath: filePath), options: .atomic)
+            didWriteFile = true

             // Set file permissions: root-owned, world-readable
             let fm = FileManager.default
             try fm.setAttributes([
                 .posixPermissions: 0o755,
@@
         } catch {
             NSLog("SCSettings: Failed to write settings: %@", error.localizedDescription)
         }

+        guard didWriteFile else { return }
         // Notify other processes
         DistributedNotificationCenter.default().postNotificationName(
             NSNotification.Name(StoneConstants.configurationChangedNotification),
             object: nil
         )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Settings/SCSettings.swift` around lines 99 - 119, The notification is
sent even when writing the plist fails; move the
DistributedNotificationCenter.default().postNotificationName(...) call inside
the do block so it only runs after PropertyListSerialization.data(...),
data.write(...), and fm.setAttributes(...) all succeed; update SCSettings.swift
to post the StoneConstants.configurationChangedNotification at the end of the do
scope (and do not post it in the catch) so peers are only notified after a
successful write.
Common/Model/BlockEntry.swift-26-33 (1)

26-33: ⚠️ Potential issue | 🟠 Major

IPv6 addresses are misinterpreted as having ports.

The port extraction logic uses lastIndex(of: ":") which matches colons in IPv6 addresses. For example, ::1 (IPv6 localhost) would be parsed as hostname :: with port 1.

Consider checking isIPv6 before port extraction, or using a more robust pattern like ]:<port> for bracketed IPv6 (e.g., [::1]:443).

🐛 Proposed fix: Skip port extraction for IPv6
         // Extract port (hostname:port)
         var port: Int? = nil
-        if let colonIndex = working.lastIndex(of: ":"),
-           !working.contains("/"),
-           let portNum = Int(working[working.index(after: colonIndex)...]) {
+        // Skip port extraction if this looks like IPv6 (contains multiple colons)
+        let colonCount = working.filter { $0 == ":" }.count
+        if colonCount == 1,
+           let colonIndex = working.lastIndex(of: ":"),
+           let portNum = Int(working[working.index(after: colonIndex)...]) {
             port = portNum
             working = String(working[working.startIndex..<colonIndex])
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Model/BlockEntry.swift` around lines 26 - 33, The port-extraction code
incorrectly treats IPv6 addresses as having ports by using lastIndex(of: ":") on
the working string; update the logic in BlockEntry (the block that sets var
port: Int? and mutates working) to skip naive colon-based port extraction for
IPv6—detect IPv6 (e.g., presence of multiple ':' or a leading '['/']' bracketed
address) and only attempt port parsing for bracketed IPv6 in the form "]:<port>"
or for IPv4/hostname forms with a single trailing ":<port>"; ensure you only
trim the port part from working when you successfully detect and parse a valid
port using the bracketed pattern or a non-IPv6 single-colon hostname.
ScheduleListWindowController.m-258-260 (1)

258-260: ⚠️ Potential issue | 🟠 Major

Use a multiline text control for blocklist entry.

Line 258 uses NSTextField, but save parsing expects newline-delimited domains (Line 336-341). NSTextField is single-line, so the UI does not match intended behavior.

Also applies to: 336-341

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ScheduleListWindowController.m` around lines 258 - 260, The UI uses a
single-line NSTextField for _blocklistField but the parsing in
ScheduleListWindowController expects newline-delimited domains; replace the
NSTextField with a multiline NSTextView hosted inside an NSScrollView (create
the NSScrollView with the same frame, add an NSTextView as its documentView, and
add the scroll view to content) and update the _blocklistField property/type to
reference the NSTextView (or add a new ivar) so the parsing code reads the text
via the NSTextView API (e.g., its string content) instead of NSTextField.
CLI/CLIMain.swift-64-100 (1)

64-100: ⚠️ Potential issue | 🟠 Major

Fail fast on malformed start arguments instead of silently falling back.

When user passes invalid --duration, invalid --enddate, or partial explicit args, control drops into UserDefaults fallback (Line 93-100) and may start an unintended block.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/CLIMain.swift` around lines 64 - 100, The code silently falls back to
UserDefaults when users supply a malformed start argument; change validation so
any explicit start input must parse or the program fails fast: if durationStr is
non-nil but cannot be parsed to a positive Int or is <=0, call NSLog with a
clear error about invalid --duration and exit(64); if endDateStr is non-nil but
ISO8601DateFormatter().date(from: endDateStr) returns nil, NSLog an invalid
--enddate error and exit(64); similarly, when using legacy positional args (args
and blocklistPath/args[3]/args[4]), if args.count>4 and args[3] or args[4] are
present but args[4] fails to parse to a Date, NSLog an error about malformed
positional enddate and exit(64); ensure these checks occur before the
UserDefaults fallback so blockEndDate, durationStr, endDateStr, and
blocklistPath cannot silently trigger defaults.
Common/Block/HostFileBlocker.swift-30-57 (1)

30-57: ⚠️ Potential issue | 🟠 Major

Synchronize newEntries access and clear after successful write.

newEntries is mutated under lock in Line 16-22 but read in Line 43 without locking, and successful writes leave entries in memory, causing repeat insertion on the next call.

Suggested fix
     func writeNewFileContents() -> Bool {
-        guard !newEntries.isEmpty else { return true }
+        let entriesSnapshot: [String]
+        lock.lock()
+        entriesSnapshot = newEntries
+        lock.unlock()
+
+        guard !entriesSnapshot.isEmpty else { return true }
@@
-        for entry in newEntries {
+        for entry in entriesSnapshot {
             block += entry + "\n"
         }
@@
         do {
             try contents.write(toFile: filePath, atomically: true, encoding: .utf8)
+            lock.lock()
+            newEntries.removeAll()
+            lock.unlock()
             return true
         } catch {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Block/HostFileBlocker.swift` around lines 30 - 57, The
writeNewFileContents() function reads newEntries without locking and doesn't
clear them after a successful write, causing duplicate inserts; fix by
synchronizing access to newEntries: acquire the same lock used when mutating
newEntries to copy its contents into a local array at the start of
writeNewFileContents(), release the lock before performing file I/O, and after a
successful write re-acquire the lock and clear newEntries (or remove only the
written items) to avoid repeated insertion; ensure you do not hold the lock
during the file read/write to prevent blocking other threads.
CLI/CLIMain.swift-124-156 (1)

124-156: ⚠️ Potential issue | 🟠 Major

Add a timeout for daemon start wait to prevent indefinite hangs.

The semaphore wait has no timeout, so CLI can block forever if install/connect callbacks never complete.

Suggested fix
-        if Thread.isMainThread {
-            while semaphore.wait(timeout: .now()) != .success {
+        let timeout = DispatchTime.now() + .seconds(30)
+        if Thread.isMainThread {
+            while semaphore.wait(timeout: .now()) != .success {
+                if DispatchTime.now() >= timeout {
+                    NSLog("ERROR: Timed out waiting for daemon response")
+                    exit(70)
+                }
                 RunLoop.current.run(mode: .default, before: Date())
             }
         } else {
-            semaphore.wait()
+            if semaphore.wait(timeout: timeout) != .success {
+                NSLog("ERROR: Timed out waiting for daemon response")
+                exit(70)
+            }
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/CLIMain.swift` around lines 124 - 156, The semaphore wait can block
forever; add a bounded timeout when waiting for the daemon start in the
installDaemon -> startBlock completion path: replace the indefinite wait loop
around semaphore.wait in the main thread with a timed wait (e.g.,
DispatchTime.now() + N seconds) and handle timeout by logging an error (NSLog)
and exiting with a non-zero code, and likewise replace the unconditional
semaphore.wait() in the non-main-thread branch with semaphore.wait(timeout:) and
the same timeout handling; ensure the timeout value and the error handling are
applied where xpc.installDaemon, xpc.refreshConnectionAndRun, and xpc.startBlock
signal the semaphore so the CLI fails fast instead of hanging indefinitely.
project.yml-58-58 (1)

58-58: ⚠️ Potential issue | 🟠 Major

Don’t suppress daemon copy failures in build script.

Line 58 masks copy failures with || true, so packaging can silently miss the daemon binary.

Suggested fix
-          cp "${BUILT_PRODUCTS_DIR}/com.max4c.stonectld" "${BUILT_PRODUCTS_DIR}/${FRAMEWORKS_FOLDER_PATH}/../Library/LaunchServices/" 2>/dev/null || true
+          install -m 755 "${BUILT_PRODUCTS_DIR}/com.max4c.stonectld" \
+            "${BUILT_PRODUCTS_DIR}/${FRAMEWORKS_FOLDER_PATH}/../Library/LaunchServices/com.max4c.stonectld"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@project.yml` at line 58, The build script currently hides copy failures by
appending "|| true" to the cp invocation that copies com.max4c.stonectld into
the LaunchServices folder; remove the "|| true" suppression from that cp command
so any failure returns a non‑zero exit status and fails the build, and
optionally add an explicit error log/exit path (e.g., check the cp exit code and
call echo/process exit) to surface the failure during packaging.
Common/Protocol/SCDaemonProtocol.swift-12-14 (1)

12-14: ⚠️ Potential issue | 🟠 Major

Configure NSXPCInterface with explicit class whitelisting for blockSettings.

Line 12 declares blockSettings: [String: Any] without configuring the NSXPCInterface to whitelist allowed types. While current usage only passes simple types (booleans and strings), the heterogeneous dictionary type signature creates a runtime safety risk if any non-whitelisted types are sent across the XPC boundary.

Add interface.setClasses(...) in Daemon/SCDaemon.swift:56 and other interface setup locations to explicitly whitelist the permitted types for this parameter, or refactor blockSettings to a strongly-typed struct.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Protocol/SCDaemonProtocol.swift` around lines 12 - 14, The protocol
currently exposes blockSettings: [String: Any], which is unsafe over XPC; update
the NSXPCInterface setup where the SCDaemon XPC interface is configured to
whitelist concrete classes for this dictionary (use
interface.setClasses(_:for:argumentIndex:ofReply:) to allow only NSString,
NSNumber, NSDictionary, NSArray, NSNull or a custom Codable struct/class), or
change the protocol signature to use a strongly-typed struct/DTO for
blockSettings and encode/decode it; adjust the interface.setClasses calls in the
XPC interface initialization (the place that creates the NSXPCInterface for
SCDaemon) and any other XPC interface registration sites to reflect the
permitted classes.
Common/XPC/SCXPCAuthorization.swift-40-52 (1)

40-52: ⚠️ Potential issue | 🟠 Major

Missing size validation for authData before reading AuthorizationExternalForm.

If authData is smaller than MemoryLayout<AuthorizationExternalForm>.size, the load(as:) call will read beyond the buffer bounds, potentially causing undefined behavior or a crash.

🛡️ Proposed fix
     static func checkAuthorization(_ authData: Data, for commandName: String) throws {
         let rightName = self.rightName(for: commandName)
 
+        let expectedSize = MemoryLayout<AuthorizationExternalForm>.size
+        guard authData.count >= expectedSize else {
+            throw SCError.authorizationFailed
+        }
+
         var authRef: AuthorizationRef?
         let status = authData.withUnsafeBytes { rawPtr -> OSStatus in
             guard let ptr = rawPtr.baseAddress else { return errAuthorizationInvalidRef }
             var extForm = ptr.load(as: AuthorizationExternalForm.self)
             return AuthorizationCreateFromExternalForm(&extForm, &authRef)
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/XPC/SCXPCAuthorization.swift` around lines 40 - 52, In
checkAuthorization(_ authData: Data, for commandName: String) validate that
authData.count is at least MemoryLayout<AuthorizationExternalForm>.size before
calling withUnsafeBytes and load(as:); if it's too small, throw
SCError.authorizationFailed (or a more specific SCError if available). Locate
the code in checkAuthorization where authData.withUnsafeBytes and load(as:
AuthorizationExternalForm.self) are used, add the size guard (authData.count >=
MemoryLayout<AuthorizationExternalForm>.size) and only then perform the unsafe
load or alternatively copy the bytes into a local AuthorizationExternalForm
buffer to avoid out-of-bounds access.
Daemon/SCDaemonBlockMethods.swift-161-171 (1)

161-171: ⚠️ Potential issue | 🟠 Major

Serialize checkBlockIntegrity() too.

SCDaemon calls this from the /etc/hosts watcher without holding lock, so installBlockRulesFromSettings() can race with startBlock() or the update methods. Either acquire the same lock here or split out a private locked helper so every entry point stays serialized.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Daemon/SCDaemonBlockMethods.swift` around lines 161 - 171,
checkBlockIntegrity() is called without holding the daemon's serialization lock
and can race with startBlock()/the update methods when calling
SCHelperToolUtilities.installBlockRulesFromSettings(); fix by acquiring the same
lock used by SCDaemon around the integrity check and call (or move the body into
a private helper like lockedCheckBlockIntegrity() that assumes the lock and call
that helper from all entry points), ensuring checkBlockIntegrity() either takes
the lock or delegates to the locked helper so installBlockRulesFromSettings() is
always invoked under the daemon's lock.
Daemon/SCDaemonBlockMethods.swift-97-99 (1)

97-99: ⚠️ Potential issue | 🟠 Major

Lock timeouts must surface as errors.

Both update paths return on lock timeout instead of throwing. That turns a rejected mutation into an apparent success for the XPC caller even though nothing changed.

Also applies to: 130-132

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Daemon/SCDaemonBlockMethods.swift` around lines 97 - 99, The guard in
updateBlocklist currently returns on lock timeout (guard lock.lock(before: ...)
else { return }) which hides failures; replace that return with throwing a
descriptive error (e.g., throw SCDaemonError.lockTimeout or add a LockError) so
the XPC caller observes the failure, and apply the same change to the other
methods that use lock.lock(before:) around lines ~130-132 so all lock timeouts
surface as thrown errors and are propagated to callers.
Daemon/SCDaemonBlockMethods.swift-13-17 (1)

13-17: ⚠️ Potential issue | 🟠 Major

Persist controllingUID in SCSettings.

SCHelperToolUtilities.clearBrowserCaches() later resolves the real home directory from "ControllingUID", but this method never stores the explicit controllingUID argument. In the root daemon that falls back to /var/root, so cache cleanup targets the wrong account.

Suggested fix
         settings.setValue(blocklist, for: "ActiveBlocklist")
         settings.setValue(isAllowlist, for: "ActiveBlockAsWhitelist")
         settings.setValue(endDate, for: "BlockEndDate")
+        settings.setValue(controllingUID, for: "ControllingUID")

Also applies to: 38-46

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Daemon/SCDaemonBlockMethods.swift` around lines 13 - 17, The
startBlock(controllingUID:uid_t, ...) method currently never stores
controllingUID into SCSettings, causing
SCHelperToolUtilities.clearBrowserCaches() to resolve the wrong home; persist
the UID under the "ControllingUID" key in SCSettings (e.g., via
SCSettings.set("ControllingUID", controllingUID) or the existing settings API)
before returning or throwing so the helper can resolve the correct home; apply
the same change to the other startBlock overload/variant referenced (lines
~38-46) to ensure both code paths write the controlling UID into SCSettings.
Common/Block/PacketFilter.swift-36-41 (1)

36-41: ⚠️ Potential issue | 🟠 Major

Integrity checks should inspect active PF state, not just pf.conf.

blockFoundInPF() only greps /etc/pf.conf. If someone flushes the Stone anchor or disables PF while leaving the file intact, checkBlockIntegrity() still sees a healthy block and skips reapplying it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Block/PacketFilter.swift` around lines 36 - 41, blockFoundInPF
currently only reads the pf.conf file, which misses cases where PF is disabled
or the anchor was flushed; update blockFoundInPF (and any callers like
checkBlockIntegrity) to inspect the active PF state by invoking pfctl (e.g., run
"/sbin/pfctl -sa" or an appropriate pfctl subcommand via Process) and search
that output for the Stone anchor name (StoneConstants.pfAnchorName) and its
rules; if pfctl fails or is unavailable, fall back to the existing pf.conf file
check so the function still returns a conservative result.
Common/Utilities/SCHelperToolUtilities.swift-32-45 (1)

32-45: ⚠️ Potential issue | 🟠 Major

Don't clear block state if teardown failed.

BlockManager.clearBlock() returns a success flag, but removeBlock() ignores it and always wipes settings. If PF or hosts cleanup fails, the daemon can advertise “not blocked” while enforcement is still partially active.

Suggested fix
         let manager = BlockManager(isAllowlist: isAllowlist)
-        manager.clearBlock()
+        guard manager.clearBlock() else {
+            NSLog("SCHelperToolUtilities: Failed to clear block enforcement")
+            return
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Utilities/SCHelperToolUtilities.swift` around lines 32 - 45, The
removeBlock() function currently ignores the result of
BlockManager.clearBlock(); update it to capture the Bool returned by
manager.clearBlock() and only proceed to call
SCBlockUtilities.removeBlockFromSettings(),
sendConfigurationChangedNotification(), and
clearBrowserCaches()/clearOSDNSCache() when that Bool is true; if clearBlock()
returns false, do not modify settings or send the configuration notification
(optionally log the failure via existing logging facilities) so teardown
failures (in BlockManager.clearBlock) don't cause the daemon to advertise an
unblocked state.
Common/XPC/SCXPCClient.swift-86-90 (1)

86-90: ⚠️ Potential issue | 🟠 Major

Complete each request when the XPC proxy errors.

When using remoteObjectProxyWithErrorHandler, XPC guarantees that either the reply block or the error handler runs—never both. If the connection fails, the reply block is not invoked. The current error handler only logs; it doesn't complete the reply closures that callers provide in startBlock, updateBlocklist, updateBlockEndDate, and getVersion. This leaves callers waiting indefinitely. Add completion handling in the error handler path to forward failures to each pending request.

Also applies to: 95-135

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/XPC/SCXPCClient.swift` around lines 86 - 90, The proxy() method’s
error handler only logs errors and never completes callers’ reply closures;
modify the error handler passed to connection?.remoteObjectProxyWithErrorHandler
in proxy() (and similar handlers in the other proxy usages between lines
~95–135) to iterate any pending request callbacks and invoke them with an
appropriate failure/result so callers of startBlock, updateBlocklist,
updateBlockEndDate, and getVersion are completed; locate the storage of pending
replies (e.g., any arrays or dictionaries holding reply closures), and in the
error handler call each stored reply with an error/empty result and then clear
the pending list so no caller waits indefinitely.
App/UI/ScheduleListWindowController.swift-227-233 (1)

227-233: ⚠️ Potential issue | 🟠 Major

Replace NSTextField with NSTextView in NSScrollView for multiline blocklist input.

blocklistField is created as NSTextField (lines 227-233) with placeholder text "Enter domains, one per line", but NSTextField cannot accept multiline input. In AppKit, pressing Return in a text field ends editing; it does not insert a newline. The save code (line 295) splits on .newlines, but users will never be able to enter newlines into the field, making the entire feature non-functional. Use NSTextView embedded in NSScrollView for editable multiline text input.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@App/UI/ScheduleListWindowController.swift` around lines 227 - 233, The
blocklist UI currently creates a single-line NSTextField (bf) assigned to
blocklistField which prevents entering newlines; replace this with an NSTextView
embedded in an NSScrollView so users can enter multiline domains. In
ScheduleListWindowController.swift, remove the NSTextField creation (bf) and
instead create an NSScrollView containing an NSTextView sized like the current
bf rect, add it to content, and store a reference in blocklistField (or
introduce blocklistTextView/blocklistScrollView if you change the stored type).
Ensure existing save/parse logic that reads blocklistField.text (or .string) is
updated to read the NSTextView's string to preserve the split-on-newlines
behavior.
App/AppController.swift-208-210 (1)

208-210: ⚠️ Potential issue | 🟠 Major

Thread safety issue: @Published property updated on background thread.

installBlock() is called from DispatchQueue.global (line 175), so addingBlock = true on line 209 executes on a background thread. However, @Published properties must be updated on the main thread for SwiftUI/Combine observation to work correctly. Note that line 258 correctly wraps the update in DispatchQueue.main.async.

🔧 Proposed fix
     private func installBlock() {
-        addingBlock = true
-        DispatchQueue.main.async { self.refreshUserInterface() }
+        DispatchQueue.main.async {
+            self.addingBlock = true
+            self.refreshUserInterface()
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@App/AppController.swift` around lines 208 - 210, installBlock() sets the
`@Published` property addingBlock from a background thread; move the mutation onto
the main thread by performing the assignment inside DispatchQueue.main.async
before calling refreshUserInterface(), i.e. ensure addingBlock = true is
executed on the main queue (similar to how the later update is wrapped) so
SwiftUI/Combine observers see the change safely; update the installBlock()
implementation to call DispatchQueue.main.async { addingBlock = true;
self.refreshUserInterface() } rather than assigning addingBlock on the
background thread.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: aeedb83a-4fac-4043-987f-c74907cf91d7

📥 Commits

Reviewing files that changed from the base of the PR and between 34774c4 and 780a71a.

📒 Files selected for processing (62)
  • .go/progress.md
  • App/AppController.swift
  • App/AppDelegate.swift
  • App/Stone-Info.plist
  • App/Stone.entitlements
  • App/StoneApp.swift
  • App/UI/BlocklistEditorView.swift
  • App/UI/DomainListWindowController.swift
  • App/UI/MainView.swift
  • App/UI/MainWindowController.swift
  • App/UI/PreferencesWindowController.swift
  • App/UI/ScheduleEditorView.swift
  • App/UI/ScheduleListWindowController.swift
  • App/UI/TimerView.swift
  • App/UI/TimerWindowController.swift
  • AppController.h
  • AppController.m
  • CLI/CLIMain.swift
  • CLI/stone-cli-Info.plist
  • Common/Block/BlockManager.swift
  • Common/Block/HostFileBlocker.swift
  • Common/Block/HostFileBlockerSet.swift
  • Common/Block/PacketFilter.swift
  • Common/Errors/SCError.swift
  • Common/Model/BlockEntry.swift
  • Common/Model/SCSchedule.swift
  • Common/Model/StoneConstants.swift
  • Common/Protocol/SCDaemonProtocol.swift
  • Common/Settings/SCSettings.swift
  • Common/Utilities/SCBlockFileReaderWriter.swift
  • Common/Utilities/SCBlockUtilities.swift
  • Common/Utilities/SCFileWatcher.swift
  • Common/Utilities/SCHelperToolUtilities.swift
  • Common/Utilities/SCMiscUtilities.swift
  • Common/XPC/SCXPCAuthorization.swift
  • Common/XPC/SCXPCClient.swift
  • Daemon/AuditTokenBridge.h
  • Daemon/AuditTokenBridge.m
  • Daemon/Daemon-Bridging-Header.h
  • Daemon/DaemonMain.swift
  • Daemon/SCDaemon.swift
  • Daemon/SCDaemonBlockMethods.swift
  • Daemon/SCDaemonXPC.swift
  • Daemon/stonectld-Info.plist
  • Daemon/stonectld-launchd.plist
  • SCConstants.h
  • SCConstants.m
  • SCSchedule.h
  • SCSchedule.m
  • SCScheduleManager.h
  • SCScheduleManager.m
  • ScheduleListWindowController.h
  • ScheduleListWindowController.m
  • ScheduleManager/LaunchAgentWriter.swift
  • ScheduleManager/SCScheduleManager.swift
  • SelfControl.xcodeproj/project.pbxproj
  • Stone.xcodeproj/project.pbxproj
  • Stone.xcodeproj/xcshareddata/xcschemes/Stone.xcscheme
  • Stone.xcodeproj/xcshareddata/xcschemes/stone-cli.xcscheme
  • Stone.xcodeproj/xcshareddata/xcschemes/stonectld.xcscheme
  • cli-main.m
  • project.yml

Comment on lines +15 to +19
var controllingUID = getuid()
if let uidIdx = args.firstIndex(of: "--uid"), uidIdx + 1 < args.count,
let uid = UInt32(args[uidIdx + 1]) {
controllingUID = uid_t(uid)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify daemon-side checks for controllingUID/auth binding.
# Expected: startBlock implementation validates UID against authorization/audit token identity.

rg -nP -C5 'startBlock\(|controllingUID|authorization|audit|AuditToken|uid|geteuid|getuid'

Repository: SelfControlApp/selfcontrol

Length of output: 50383


🏁 Script executed:

#!/bin/bash
# Find the actual implementation details of AuditTokenBridge
fd -t f "AuditTokenBridge\.(h|m)$" --exec head -100 {} \;

Repository: SelfControlApp/selfcontrol

Length of output: 539


🏁 Script executed:

#!/bin/bash
# Look for where audit token is used in daemon block methods - need full impl
fd -t f "SCDaemonBlockMethods\.(m|swift)$" --exec wc -l {} \;

Repository: SelfControlApp/selfcontrol

Length of output: 145


🏁 Script executed:

#!/bin/bash
# Search for audit token validation or caller UID checking in daemon
rg -n "audit|AuditToken|callerUID|getuid|geteuid" --type swift --type objc Daemon/ -A 3 | head -150

Repository: SelfControlApp/selfcontrol

Length of output: 2419


🏁 Script executed:

#!/bin/bash
# Look for where authorization data is checked and how it relates to controllingUID
rg -n "checkAuthorization|authData|controllingUID" --type swift Daemon/ -B 2 -A 5 | head -200

Repository: SelfControlApp/selfcontrol

Length of output: 3026


🏁 Script executed:

#!/bin/bash
# Check if controllingUID is validated against the caller's actual UID in startBlock
rg -n "controllingUID" Daemon/SCDaemonBlockMethods.swift -B 5 -A 20

Repository: SelfControlApp/selfcontrol

Length of output: 1013


🏁 Script executed:

#!/bin/bash
# Look for any UID validation or comparison logic in the daemon
rg -n "uid_t|getuid|caller.*uid|controllingUID.*audit" Daemon/ -B 2 -A 2

Repository: SelfControlApp/selfcontrol

Length of output: 2847


🏁 Script executed:

#!/bin/bash
# Check if there's any code that extracts UID from audit token and validates it
rg -n "au_to_|audit_to_|audit.*uid|uid.*audit" Daemon/ -B 2 -A 3

Repository: SelfControlApp/selfcontrol

Length of output: 52


🏁 Script executed:

#!/bin/bash
# Check if there's anywhere that compares caller's actual UID to the supplied controllingUID
rg -n "getuid|geteuid|audit_token" Daemon/SCDaemonXPC.swift -B 3 -A 3

Repository: SelfControlApp/selfcontrol

Length of output: 52


🏁 Script executed:

#!/bin/bash
# Check the full startBlock implementation in SCDaemonBlockMethods.swift for any UID validation
cat Daemon/SCDaemonBlockMethods.swift | head -60

Repository: SelfControlApp/selfcontrol

Length of output: 2047


🏁 Script executed:

#!/bin/bash
# Search more broadly for any UID validation logic in daemon methods
rg -n "controllingUID.*==" Daemon/ --type swift --type objc -B 2 -A 2

Repository: SelfControlApp/selfcontrol

Length of output: 52


🏁 Script executed:

#!/bin/bash
# Check what happens with controllingUID after it's received - is it stored/validated?
rg -n "SCSettings.*ControllingUID|controllingUID.*store|saveControllingUID" Daemon/ -B 2 -A 2

Repository: SelfControlApp/selfcontrol

Length of output: 52


🏁 Script executed:

#!/bin/bash
# Check if controllingUID is used to store/retrieve user-specific settings and what might validate it
rg -n "controllingUID" Daemon/SCDaemonBlockMethods.swift

Repository: SelfControlApp/selfcontrol

Length of output: 115


🏁 Script executed:

#!/bin/bash
# Look at the SCDaemonXPC implementation to see if it validates the caller's UID
cat Daemon/SCDaemonXPC.swift

Repository: SelfControlApp/selfcontrol

Length of output: 2100


🏁 Script executed:

#!/bin/bash
# Check if there's any XPC connection access in startBlock that could validate caller
rg -n "newConnection|connection|caller|audit" Daemon/SCDaemonXPC.swift -B 3 -A 5

Repository: SelfControlApp/selfcontrol

Length of output: 384


🏁 Script executed:

#!/bin/bash
# Check if startBlock receives XPC connection context for validation
rg -n "class SCDaemonXPC|func startBlock" Daemon/SCDaemonXPC.swift -B 2 -A 15 | head -100

Repository: SelfControlApp/selfcontrol

Length of output: 1009


🏁 Script executed:

#!/bin/bash
# Check how controllingUID is actually used after being stored - is it security-critical?
rg -n "ControllingUID|controllingUID" Common/Settings/ Daemon/ -B 2 -A 2 | head -100

Repository: SelfControlApp/selfcontrol

Length of output: 4395


🏁 Script executed:

#!/bin/bash
# Check if the XPC connection's audit token is accessible in SCDaemonXPC 
# and whether it's being used to validate caller identity
rg -n "SCDaemonXPC|connection" Daemon/SCDaemon.swift -B 3 -A 5

Repository: SelfControlApp/selfcontrol

Length of output: 1203


🏁 Script executed:

#!/bin/bash
# Check the full daemon setup to see where SCDaemonXPC instances are created
# and whether audit token validation is done there
fd -t f "SCDaemon.swift" --exec cat {} \;

Repository: SelfControlApp/selfcontrol

Length of output: 2956


🏁 Script executed:

#!/bin/bash
# Verify that the audit token validation code is truly commented out in the TODO
cat -n Daemon/SCDaemon.swift | sed -n '46,54p'

Repository: SelfControlApp/selfcontrol

Length of output: 626


🏁 Script executed:

#!/bin/bash
# Check if controllingUID is ever compared to the actual caller's uid
rg -n "audit_token_t|uid_t.*audit|getuid.*caller|caller.*uid" Daemon/ --type swift --type objc

Repository: SelfControlApp/selfcontrol

Length of output: 625


🏁 Script executed:

#!/bin/bash
# Verify that SCXPCAuthorization.checkAuthorization only validates rights, not caller identity
cat Common/XPC/SCXPCAuthorization.swift

Repository: SelfControlApp/selfcontrol

Length of output: 4712


Daemon does not validate that supplied controllingUID matches the caller's actual identity.

While authorization rights are checked, there is no validation binding the controllingUID parameter to the caller's audit token. An admin user could invoke --uid <other_user> and the daemon accepts it (SCDaemonXPC.swift startBlock only calls checkAuthorization() for rights, then passes controllingUID directly to SCDaemonBlockMethods). Audit token access infrastructure exists (AuditTokenBridge) but the validation code is commented out in SCDaemon.swift (lines 47–54, marked TODO). Implement a check that rejects controllingUID when it differs from the connecting process's UID extracted from the XPC connection audit token.

Also applies to: 134-134

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/CLIMain.swift` around lines 15 - 19, The CLI currently allows overriding
controllingUID from args (variable controllingUID set from args and passed into
SCDaemonXPC.startBlock) without validating it against the caller's actual UID;
update the flow so that when a non-default --uid is supplied you extract the
caller UID from the XPC connection audit token (use the AuditTokenBridge helper
used elsewhere and the SCDaemon.swift TODO location where audit-token validation
was commented out) and compare it to the parsed controllingUID, and if they
differ reject the request (return an error/deny access) instead of accepting the
supplied controllingUID; ensure this validation is performed before passing
controllingUID into SCDaemonBlockMethods/startBlock.

Comment on lines +35 to +37
for string in strings {
let entry = BlockEntry(string: string)
allEntries.append(entry.hostname)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Keep the original BlockEntry metadata when expanding entries.

The first pass appends only entry.hostname, then the second pass reparses that stripped string. Inputs like 10.0.0.0/8 and 1.2.3.4:443 lose their mask/port and get turned into broader whole-host rules.

Also applies to: 47-63

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Block/BlockManager.swift` around lines 35 - 37, The loop that expands
input strings into BlockEntry instances currently appends only entry.hostname
(losing port/mask), then reparses that stripped hostname later; update the logic
in the expansion loop that iterates over strings to append the full BlockEntry
(or at minimum preserve and re-use entry.port and entry.mask alongside
entry.hostname) instead of only entry.hostname so metadata like ports and CIDR
masks are retained; adjust any downstream code that reparses entries (the
second-pass expansion logic referenced around the BlockEntry creation and the
block-expansion section at 47–63) to read port/mask from the preserved
BlockEntry fields rather than re-parsing the hostname string.

Comment on lines +55 to +57
if !isAppending {
hostsBlocker.addEntry(hostname: entry.hostname)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Append mode is incomplete for hostnames, and allowlist updates can no-op.

While isAppending is true, new hostnames never reach hostsBlocker, and finalizeBlock() only refreshes PF. For allowlists it is worse: PacketFilter.enterAppendMode() exits early, so neither PF nor hosts is updated even though the stored blocklist is later extended.

Also applies to: 68-76, 87-90

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Block/BlockManager.swift` around lines 55 - 57, The append-mode logic
skips updating hostsBlocker and PF which causes new hostnames and allowlist
changes to be lost; update the code paths so that when isAppending is true you
still call hostsBlocker.addEntry(hostname: entry.hostname) (and any equivalent
calls in the other blocks noted) and ensure finalizeBlock() (and
PacketFilter.enterAppendMode()) performs the necessary PF and hostsBlocker
refreshes after an append completes rather than returning early; specifically,
remove or bypass the early return in PacketFilter.enterAppendMode(), add
host-update calls in the isAppending branches (the places that currently skip
hostsBlocker.addEntry), and make finalizeBlock() explicitly refresh both PF and
hostsBlocker from the stored blocklist so appended entries are applied.

Comment on lines +115 to +123
guard let beginRange = contents.range(of: StoneConstants.hostsSentinelBegin),
let endRange = contents.range(of: StoneConstants.hostsSentinelEnd) else {
return contents
}

// Include the newline after END marker
let removeEnd = contents.index(after: endRange.upperBound) < contents.endIndex
? contents.index(after: endRange.upperBound)
: endRange.upperBound
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix removeBlockSection index handling to avoid runtime trap.

Line 121 can trap when hostsSentinelEnd is at EOF (index(after: endIndex) is invalid). Also validate END appears after BEGIN before removing.

Suggested fix
     private func removeBlockSection(from contents: String) -> String {
         guard let beginRange = contents.range(of: StoneConstants.hostsSentinelBegin),
               let endRange = contents.range(of: StoneConstants.hostsSentinelEnd) else {
             return contents
         }
+        guard beginRange.lowerBound <= endRange.lowerBound else {
+            return contents
+        }
 
-        // Include the newline after END marker
-        let removeEnd = contents.index(after: endRange.upperBound) < contents.endIndex
-            ? contents.index(after: endRange.upperBound)
-            : endRange.upperBound
+        // Include one trailing newline after END if present.
+        let removeEnd: String.Index
+        if endRange.upperBound < contents.endIndex,
+           contents[endRange.upperBound] == "\n" {
+            removeEnd = contents.index(after: endRange.upperBound)
+        } else {
+            removeEnd = endRange.upperBound
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
guard let beginRange = contents.range(of: StoneConstants.hostsSentinelBegin),
let endRange = contents.range(of: StoneConstants.hostsSentinelEnd) else {
return contents
}
// Include the newline after END marker
let removeEnd = contents.index(after: endRange.upperBound) < contents.endIndex
? contents.index(after: endRange.upperBound)
: endRange.upperBound
guard let beginRange = contents.range(of: StoneConstants.hostsSentinelBegin),
let endRange = contents.range(of: StoneConstants.hostsSentinelEnd) else {
return contents
}
guard beginRange.lowerBound <= endRange.lowerBound else {
return contents
}
// Include one trailing newline after END if present.
let removeEnd: String.Index
if endRange.upperBound < contents.endIndex,
contents[endRange.upperBound] == "\n" {
removeEnd = contents.index(after: endRange.upperBound)
} else {
removeEnd = endRange.upperBound
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Block/HostFileBlocker.swift` around lines 115 - 123, The removal logic
in removeBlockSection must validate that the END sentinel is found after the
BEGIN sentinel and avoid calling index(after:) when endRange.upperBound ==
contents.endIndex; update the guard to check that endRange.upperBound >
beginRange.upperBound (so END is after BEGIN) and compute removeEnd by using
contents.index(after: endRange.upperBound) only if endRange.upperBound <
contents.endIndex, otherwise use endRange.upperBound, referencing beginRange,
endRange, StoneConstants.hostsSentinelBegin, StoneConstants.hostsSentinelEnd and
the removeEnd variable so the function returns contents unchanged on invalid
ranges and never attempts an out-of-bounds index(after:).

Comment on lines +222 to +229
let args: [String]
if let token = token, !token.isEmpty, !force {
args = ["-X", token, "-f", store.pfConfPath]
} else {
args = ["-d", "-f", store.pfConfPath]
}

let (status, _) = runPfctl(args)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

On macOS Packet Filter, what does pfctl -d do? Does it disable PF globally or only remove rules for a specific anchor/token?

💡 Result:

On macOS, pfctl -d disables PF packet filtering globally (it turns PF off), not just for a particular anchor or token. That means PF stops filtering traffic system-wide; it’s not scoped to -a <anchor> and it doesn’t “remove rules from one anchor” as its main effect. [1]

macOS also has Apple’s PF enable reference / token mechanism (commonly used with pfctl -E to enable and return a token, and pfctl -X <token> to release that enable reference). That token mechanism is about who is keeping PF enabled; it’s distinct from the global “disable PF” action of -d. [2] Some macOS PF documentation notes that -d can be used to override the reference-count enabling mechanism and force PF off. [3]

If your goal is to remove rules for only a specific anchor, you’d typically use anchor operations (for example flushing rules in that anchor with pfctl -a <anchor> -F rules / -F all), rather than pfctl -d. [2]

Sources: [1] [2] [3]


Don't disable Packet Filter for the whole machine here.

When the token is missing or force == true, the code calls pfctl -d, which disables PF globally across the entire system. This is problematic because stopping SelfControl will remove all PF rules system-wide, not just the rules for this application, weakening the host's security posture and affecting unrelated firewall rules.

To cleanly remove only the rules added by this application, use pfctl -a <anchor> -F rules or similar anchor-scoped operations instead of the global disable flag.

Comment on lines +21 to +41
// Strip trailing path/query
if let slashIndex = working.firstIndex(of: "/") {
working = String(working[working.startIndex..<slashIndex])
}

// Extract port (hostname:port)
var port: Int? = nil
if let colonIndex = working.lastIndex(of: ":"),
!working.contains("/"),
let portNum = Int(working[working.index(after: colonIndex)...]) {
port = portNum
working = String(working[working.startIndex..<colonIndex])
}

// Extract mask (/24)
var maskLen: Int? = nil
if let slashIndex = working.firstIndex(of: "/"),
let mask = Int(working[working.index(after: slashIndex)...]) {
maskLen = mask
working = String(working[working.startIndex..<slashIndex])
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

CIDR mask parsing is broken due to ordering.

The path stripping (lines 21-24) runs before mask extraction (lines 35-41), so CIDR notation like 10.0.0.0/8 has the /8 stripped as a "path" before the mask can be extracted. The mask extraction will never find a / because it was already removed.

🐛 Proposed fix: Extract mask before stripping paths
         // Strip protocol prefixes
         for prefix in ["http://", "https://", "ftp://"] {
             if working.hasPrefix(prefix) {
                 working = String(working.dropFirst(prefix.count))
                 break
             }
         }

-        // Strip trailing path/query
-        if let slashIndex = working.firstIndex(of: "/") {
-            working = String(working[working.startIndex..<slashIndex])
-        }
-
-        // Extract port (hostname:port)
-        var port: Int? = nil
-        if let colonIndex = working.lastIndex(of: ":"),
-           !working.contains("/"),
-           let portNum = Int(working[working.index(after: colonIndex)...]) {
-            port = portNum
-            working = String(working[working.startIndex..<colonIndex])
-        }
-
-        // Extract mask (/24)
+        // Extract CIDR mask (/24) before stripping paths
         var maskLen: Int? = nil
         if let slashIndex = working.firstIndex(of: "/"),
            let mask = Int(working[working.index(after: slashIndex)...]) {
             maskLen = mask
             working = String(working[working.startIndex..<slashIndex])
+        } else if let slashIndex = working.firstIndex(of: "/") {
+            // Strip trailing path/query (no valid mask)
+            working = String(working[working.startIndex..<slashIndex])
         }
+
+        // Extract port (hostname:port)
+        var port: Int? = nil
+        if let colonIndex = working.lastIndex(of: ":"),
+           let portNum = Int(working[working.index(after: colonIndex)...]) {
+            port = portNum
+            working = String(working[working.startIndex..<colonIndex])
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Strip trailing path/query
if let slashIndex = working.firstIndex(of: "/") {
working = String(working[working.startIndex..<slashIndex])
}
// Extract port (hostname:port)
var port: Int? = nil
if let colonIndex = working.lastIndex(of: ":"),
!working.contains("/"),
let portNum = Int(working[working.index(after: colonIndex)...]) {
port = portNum
working = String(working[working.startIndex..<colonIndex])
}
// Extract mask (/24)
var maskLen: Int? = nil
if let slashIndex = working.firstIndex(of: "/"),
let mask = Int(working[working.index(after: slashIndex)...]) {
maskLen = mask
working = String(working[working.startIndex..<slashIndex])
}
// Extract CIDR mask (/24) before stripping paths
var maskLen: Int? = nil
if let slashIndex = working.firstIndex(of: "/"),
let mask = Int(working[working.index(after: slashIndex)...]) {
maskLen = mask
working = String(working[working.startIndex..<slashIndex])
} else if let slashIndex = working.firstIndex(of: "/") {
// Strip trailing path/query (no valid mask)
working = String(working[working.startIndex..<slashIndex])
}
// Extract port (hostname:port)
var port: Int? = nil
if let colonIndex = working.lastIndex(of: ":"),
let portNum = Int(working[working.index(after: colonIndex)...]) {
port = portNum
working = String(working[working.startIndex..<colonIndex])
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Model/BlockEntry.swift` around lines 21 - 41, CIDR mask parsing is
broken because the code strips the path before extracting the mask; move the
mask extraction block (the logic that sets maskLen by finding a "/" and parsing
an Int from working[working.index(after: slashIndex)...]) to run before the
path-stripping block, then keep the port extraction (the colonIndex/portNum
logic that uses working.lastIndex(of: ":") and updates port and working) after
mask extraction and either adapt or remove the "!working.contains("/")" check
since the path will be stripped later; update references to working, maskLen,
slashIndex, colonIndex and port to reflect the new order.

Comment on lines +92 to +103
private func writeToDisk() {
lock.lock()
var toWrite = settings
toWrite["SettingsVersionNumber"] = versionNumber
toWrite["LastSettingsUpdate"] = lastUpdate
lock.unlock()

do {
let data = try PropertyListSerialization.data(
fromPropertyList: toWrite, format: .binary, options: 0
)
try data.write(to: URL(fileURLWithPath: filePath), options: .atomic)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

writeToDisk() can publish a stale snapshot.

After Line 97 the lock is released, so another thread can run setValue(_:,for:) or loadFromDisk() before this snapshot reaches disk. That lets synchronize() write an already-stale plist and even clobber a newer state that arrived while the file write was in progress.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/Settings/SCSettings.swift` around lines 92 - 103, The snapshot in
writeToDisk() is released before the actual file write, allowing concurrent
setValue(_:for:) or loadFromDisk() to mutate state and cause a stale write; fix
by keeping the same synchronization until the disk write completes (e.g., move
lock.unlock() to after the PropertyListSerialization/data.write calls or wrap
the entire serialization+write sequence while holding the lock), or
alternatively switch to a serial queue/dispatch barrier for all setters,
loadFromDisk(), and writeToDisk() to ensure synchronize() cannot write a stale
snapshot; update writeToDisk(), setValue(_:for:), and loadFromDisk() to use the
chosen approach so the saved plist reflects a consistent, current settings
state.

Comment on lines +68 to +93
static func createAuthorizationData() throws -> Data {
var authRef: AuthorizationRef?
var status = AuthorizationCreate(nil, nil, [], &authRef)
guard status == errAuthorizationSuccess, let auth = authRef else {
throw SCError.authorizationFailed
}

// Pre-authorize with admin rights
let rightName = "com.max4c.stone.startBlock"
var item = AuthorizationItem(name: rightName, valueLength: 0, value: nil, flags: 0)
var rights = AuthorizationRights(count: 1, items: &item)
let flags: AuthorizationFlags = [.interactionAllowed, .extendRights, .preAuthorize]

status = AuthorizationCopyRights(auth, &rights, nil, flags, nil)
guard status == errAuthorizationSuccess else {
throw SCError.authorizationFailed
}

var extForm = AuthorizationExternalForm()
status = AuthorizationMakeExternalForm(auth, &extForm)
guard status == errAuthorizationSuccess else {
throw SCError.authorizationFailed
}

return Data(bytes: &extForm, count: MemoryLayout<AuthorizationExternalForm>.size)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how createAuthorizationData is used and if other rights need pre-auth
rg -n "createAuthorizationData" --type swift -A5 -B2

Repository: SelfControlApp/selfcontrol

Length of output: 1377


🏁 Script executed:

# Search for all authorization right names and checkAuthorization usage
rg -n "rightStartBlock|rightUpdateBlocklist|rightUpdateEndDate|updateBlockEndDate|updateBlocklist" --type swift -B2 -A2

Repository: SelfControlApp/selfcontrol

Length of output: 6796


🏁 Script executed:

# Look for checkAuthorization function definition
rg -n "checkAuthorization|AuthorizationCopyRights" --type swift -B3 -A10

Repository: SelfControlApp/selfcontrol

Length of output: 6232


🏁 Script executed:

# Check what operations are performed with the authorization data
rg -n "sendMessage\|authData" --type swift -B2 -A2

Repository: SelfControlApp/selfcontrol

Length of output: 52


Pre-authorize all required rights in createAuthorizationData.

The function only pre-authorizes startBlock, but the same authorization data is reused for all three privileged operations: startBlock, updateBlocklist, and updateBlockEndDate. The checkAuthorization calls for updateBlocklist and updateBlockEndDate will fail because those rights were never pre-authorized—the .extendRights flag used during verification cannot grant rights that weren't requested upfront.

Pre-authorize all required rights:

Suggested fix
let rightsToAuthorize = [rightStartBlock, rightUpdateBlocklist, rightUpdateEndDate]
var items: [AuthorizationItem] = rightsToAuthorize.map {
    AuthorizationItem(name: $0, valueLength: 0, value: nil, flags: 0)
}
var rights = AuthorizationRights(count: items.count, items: &items)
let flags: AuthorizationFlags = [.interactionAllowed, .extendRights, .preAuthorize]

status = AuthorizationCopyRights(auth, &rights, nil, flags, nil)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Common/XPC/SCXPCAuthorization.swift` around lines 68 - 93,
createAuthorizationData currently only pre-authorizes the single right
"com.max4c.stone.startBlock" but the same AuthorizationExternalForm is reused
for updateBlocklist and updateBlockEndDate; modify createAuthorizationData to
pre-authorize all required rights (startBlock, updateBlocklist,
updateBlockEndDate) by building an array of AuthorizationItem for each right
(e.g., rightStartBlock, rightUpdateBlocklist, rightUpdateEndDate), create
AuthorizationRights with count equal to items.count and items pointer to that
array, and call AuthorizationCopyRights(auth, &rights, nil, flags, nil) with the
existing flags so all three rights are pre-authorized before calling
AuthorizationMakeExternalForm.

Comment on lines +46 to +63
func listener(_ listener: NSXPCListener, shouldAcceptNewConnection newConnection: NSXPCConnection) -> Bool {
// TODO: Validate client code signature via audit token
#if DEBUG
// In debug builds, accept all connections for easier testing
#else
// In release builds, validate the connecting client's code signature
// let auditToken = SCGetAuditToken(newConnection)
// TODO: Verify code signature matches com.max4c.stone
#endif

let interface = NSXPCInterface(with: SCDaemonProtocol.self)
newConnection.exportedInterface = interface
newConnection.exportedObject = SCDaemonXPC()
newConnection.resume()

resetInactivityTimer()
return true
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Authenticate the XPC client before exporting the daemon interface.

The TODO is still the only protection here; this method returns true in every build. That lets any local process connect to the privileged helper and call into SCDaemonXPC.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Daemon/SCDaemon.swift` around lines 46 - 63, The
listener(_:shouldAcceptNewConnection:) currently accepts everyone; validate the
connecting client's audit token/code signature before exporting the XPC
interface. Use SCGetAuditToken(newConnection) or newConnection.auditToken to
obtain the audit token, perform code signature verification (e.g.,
SecCodeCreateWithAuditToken/SecCodeCopySigningInformation or verify the code
signing identifier/entitlements) and ensure it matches the expected identifier
(com.max4c.stone) and required entitlements; only then set
newConnection.exportedInterface = NSXPCInterface(with: SCDaemonProtocol.self),
newConnection.exportedObject = SCDaemonXPC(), call resume(),
resetInactivityTimer(), and return true — otherwise log/handle the failure and
return false without setting exportedInterface/exportedObject.

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.

1 participant