Conversation
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>
📝 WalkthroughWalkthroughThis 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
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
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
There was a problem hiding this comment.
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 | 🟠 MajorDon't make the fallback settings filename globally predictable.
When
serialNumber()returnsnil, 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 | 🟠 MajorValidate the
.stoneschema before letting it cross this API boundary.
readBlocklistonly checks that"Blocklist"exists, andwriteBlocklistserializes 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 | 🟠 MajorPreserve the model defaults when keys are missing.
integerValue/boolValueonnilcollapse to0/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 | 🟠 MajorAddress memory leak and re-entrance bugs in
start().The
passRetained(self)at line 21 increments the reference count before theFSEventStreamCreateguard 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,
FSEventStreamStartat 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
FSEventStreamCreatefails- Check the return value of
FSEventStreamStartand clean up resources if it fails- Assign to
self.streamonly afterFSEventStreamStartsucceedsProposed 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 | 🟠 MajorMake the timer lifecycle idempotent.
startTimer()always installs a new repeating timer without stopping an existing one, so ifonAppeartriggers multiple times (which can occur during view hierarchy changes), the old timer reference is overwritten and never invalidated. Additionally, whenendDateis nil inupdateTimeRemaining(), 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 | 🟠 MajorUse
takeRetainedValue()forIORegistryEntryCreateCFProperty.This is a Create-rule CoreFoundation API that returns a reference with +1 retain count.
takeUnretainedValue()leaks the returned property every timeserialNumber()succeeds, as it prevents ARC from releasing the owned reference. UsetakeRetainedValue()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 | 🟠 MajorAdd
CustomNSErrorconformance to expose the domain to NSError/ObjC consumers.
LocalizedErroronly populates localized descriptions inuserInfo. When bridging toNSError, Swift defaults the domain to the mangled type name unlessCustomNSErroris adopted. XPC and Objective-C consumers checking forcom.max4c.stone.errorwill 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 | 🟠 MajorDon'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 | 🟠 MajorReload
domainListbefore showing a reused controller.
domainListis only refreshed when somebody explicitly callsrefreshDomainList(). If this window controller instance is reopened after another code path changedBlocklist, 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 | 🟠 MajorGuard every mutation path while a block is active.
Unlike
removeDomain(_:)andtableView(_:shouldEdit:row:),addDomain(_:)andquickAdd(_:)do not checkSCBlockUtilities.anyBlockIsRunning(). During an active block the user can still mutateBlocklist, 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 | 🟠 MajorOnly 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 | 🟠 MajorIPv6 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 port1.Consider checking
isIPv6before 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 | 🟠 MajorUse a multiline text control for blocklist entry.
Line 258 uses
NSTextField, but save parsing expects newline-delimited domains (Line 336-341).NSTextFieldis 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 | 🟠 MajorFail fast on malformed
startarguments 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 | 🟠 MajorSynchronize
newEntriesaccess and clear after successful write.
newEntriesis 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 | 🟠 MajorAdd 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 | 🟠 MajorDon’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 | 🟠 MajorConfigure 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(...)inDaemon/SCDaemon.swift:56and other interface setup locations to explicitly whitelist the permitted types for this parameter, or refactorblockSettingsto 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 | 🟠 MajorMissing size validation for
authDatabefore readingAuthorizationExternalForm.If
authDatais smaller thanMemoryLayout<AuthorizationExternalForm>.size, theload(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 | 🟠 MajorSerialize
checkBlockIntegrity()too.
SCDaemoncalls this from the/etc/hostswatcher without holdinglock, soinstallBlockRulesFromSettings()can race withstartBlock()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 | 🟠 MajorLock 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 | 🟠 MajorPersist
controllingUIDinSCSettings.
SCHelperToolUtilities.clearBrowserCaches()later resolves the real home directory from"ControllingUID", but this method never stores the explicitcontrollingUIDargument. 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 | 🟠 MajorIntegrity 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 | 🟠 MajorDon't clear block state if teardown failed.
BlockManager.clearBlock()returns a success flag, butremoveBlock()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 | 🟠 MajorComplete 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 thereplyclosures that callers provide instartBlock,updateBlocklist,updateBlockEndDate, andgetVersion. 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 | 🟠 MajorReplace
NSTextFieldwithNSTextViewinNSScrollViewfor multiline blocklist input.
blocklistFieldis created asNSTextField(lines 227-233) with placeholder text "Enter domains, one per line", butNSTextFieldcannot 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. UseNSTextViewembedded inNSScrollViewfor 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 | 🟠 MajorThread safety issue:
@Publishedproperty updated on background thread.
installBlock()is called fromDispatchQueue.global(line 175), soaddingBlock = trueon line 209 executes on a background thread. However,@Publishedproperties must be updated on the main thread for SwiftUI/Combine observation to work correctly. Note that line 258 correctly wraps the update inDispatchQueue.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
📒 Files selected for processing (62)
.go/progress.mdApp/AppController.swiftApp/AppDelegate.swiftApp/Stone-Info.plistApp/Stone.entitlementsApp/StoneApp.swiftApp/UI/BlocklistEditorView.swiftApp/UI/DomainListWindowController.swiftApp/UI/MainView.swiftApp/UI/MainWindowController.swiftApp/UI/PreferencesWindowController.swiftApp/UI/ScheduleEditorView.swiftApp/UI/ScheduleListWindowController.swiftApp/UI/TimerView.swiftApp/UI/TimerWindowController.swiftAppController.hAppController.mCLI/CLIMain.swiftCLI/stone-cli-Info.plistCommon/Block/BlockManager.swiftCommon/Block/HostFileBlocker.swiftCommon/Block/HostFileBlockerSet.swiftCommon/Block/PacketFilter.swiftCommon/Errors/SCError.swiftCommon/Model/BlockEntry.swiftCommon/Model/SCSchedule.swiftCommon/Model/StoneConstants.swiftCommon/Protocol/SCDaemonProtocol.swiftCommon/Settings/SCSettings.swiftCommon/Utilities/SCBlockFileReaderWriter.swiftCommon/Utilities/SCBlockUtilities.swiftCommon/Utilities/SCFileWatcher.swiftCommon/Utilities/SCHelperToolUtilities.swiftCommon/Utilities/SCMiscUtilities.swiftCommon/XPC/SCXPCAuthorization.swiftCommon/XPC/SCXPCClient.swiftDaemon/AuditTokenBridge.hDaemon/AuditTokenBridge.mDaemon/Daemon-Bridging-Header.hDaemon/DaemonMain.swiftDaemon/SCDaemon.swiftDaemon/SCDaemonBlockMethods.swiftDaemon/SCDaemonXPC.swiftDaemon/stonectld-Info.plistDaemon/stonectld-launchd.plistSCConstants.hSCConstants.mSCSchedule.hSCSchedule.mSCScheduleManager.hSCScheduleManager.mScheduleListWindowController.hScheduleListWindowController.mScheduleManager/LaunchAgentWriter.swiftScheduleManager/SCScheduleManager.swiftSelfControl.xcodeproj/project.pbxprojStone.xcodeproj/project.pbxprojStone.xcodeproj/xcshareddata/xcschemes/Stone.xcschemeStone.xcodeproj/xcshareddata/xcschemes/stone-cli.xcschemeStone.xcodeproj/xcshareddata/xcschemes/stonectld.xcschemecli-main.mproject.yml
| var controllingUID = getuid() | ||
| if let uidIdx = args.firstIndex(of: "--uid"), uidIdx + 1 < args.count, | ||
| let uid = UInt32(args[uidIdx + 1]) { | ||
| controllingUID = uid_t(uid) | ||
| } |
There was a problem hiding this comment.
🧩 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 -150Repository: 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 -200Repository: 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 20Repository: 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 2Repository: 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 3Repository: 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 3Repository: 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 -60Repository: 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 2Repository: 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 2Repository: 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.swiftRepository: 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.swiftRepository: 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 5Repository: 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 -100Repository: 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 -100Repository: 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 5Repository: 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 objcRepository: SelfControlApp/selfcontrol
Length of output: 625
🏁 Script executed:
#!/bin/bash
# Verify that SCXPCAuthorization.checkAuthorization only validates rights, not caller identity
cat Common/XPC/SCXPCAuthorization.swiftRepository: 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.
| for string in strings { | ||
| let entry = BlockEntry(string: string) | ||
| allEntries.append(entry.hostname) |
There was a problem hiding this comment.
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.
| if !isAppending { | ||
| hostsBlocker.addEntry(hostname: entry.hostname) | ||
| } |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| 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:).
| 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) |
There was a problem hiding this comment.
🧩 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.
| // 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]) | ||
| } |
There was a problem hiding this comment.
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.
| // 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.
| 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) |
There was a problem hiding this comment.
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.
| 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) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how createAuthorizationData is used and if other rights need pre-auth
rg -n "createAuthorizationData" --type swift -A5 -B2Repository: 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 -A2Repository: SelfControlApp/selfcontrol
Length of output: 6796
🏁 Script executed:
# Look for checkAuthorization function definition
rg -n "checkAuthorization|AuthorizationCopyRights" --type swift -B3 -A10Repository: SelfControlApp/selfcontrol
Length of output: 6232
🏁 Script executed:
# Check what operations are performed with the authorization data
rg -n "sendMessage\|authData" --type swift -B2 -A2Repository: 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.
| 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 | ||
| } |
There was a problem hiding this comment.
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.
Summary by CodeRabbit
Release Notes