Skip to content

Fix six bugs from the code audit, and cut 1.0.1 - #4

Merged
fredsimard merged 12 commits into
mainfrom
fix/critical-bugs-audit-2026
Aug 16, 2026
Merged

fredsimard merged 12 commits into
mainfrom
fix/critical-bugs-audit-2026

Conversation

@fredsimard

@fredsimard fredsimard commented Aug 16, 2026 •

Copy link
Copy Markdown
Owner

What this changes

Fixes the six bugs found in a full audit of the Swift sources, corrects the documentation where it described behaviour the code did not have, and records the result as 1.0.1. One commit per issue.

Commit Fix
aef05c8 .bool results from executeAppleScriptViaCommandLine(_:with:) were always false. osascript prints true/false as text, and Int("true") is nil. The string is now compared, with integer coercion kept as a fallback so both execution paths agree.
cf777d5 The same method deadlocked on output larger than the pipe buffer (~64 KB): it waited for the process to exit before draining the pipe, while the child blocked on write. The pipe is now read first.
3c1403a Variable substitution rescanned text it had already substituted, so a value containing a placeholder name was rewritten by a later variable — a file named $folderPath.txt passed as folderPath had that variable's value spliced into its name. The script is now walked once, matching the longest key at each $.
39af9de toHFSPath() prepended the startup disk name to every path, so /Volumes/Backup/notes.txt became Macintosh HD:Volumes:Backup:notes.txt instead of Backup:notes.txt. The volume name and mount point now come from the path itself.
e0161b9 An NSNumber bridges to Bool whatever it holds, so NSNumber(value: 1) from JSONSerialization or Objective-C was substituted as true instead of 1. Numbers are unwrapped to the Swift type they actually carry before any type matching.
a5f8452 resetAutomationPermission(for:) returned true whenever tccutil launched, ignoring its exit status.

4ef0079 covers the documentation corrections, and 95b1c9d the changelog. 5956262 adds the local review notes to .gitignore.

Why

Three of these produce silently wrong results rather than errors: a .bool script that answers true is read as false, a filename that happens to contain a variable name is corrupted, and a count arriving as NSNumber is substituted as true. The pipe deadlock hangs the calling thread permanently, and is reachable from a .json listing of a large folder.

How it was tested

  • macOS version: 26.6.1, Swift 6.3.3
  • Tested against application: none — no change touches a specific app's dictionary.

swift build and swift test pass (22 tests). The two path-conversion tests were updated: they used /Users/roger/Desktop, which does not exist, and toHFSPath() now reads the volume from the file system, so they use NSHomeDirectory().

Four regression tests were added, one per fix that can be pinned deterministically. Each was run against the pre-fix sources and fails there, so they guard the behaviour rather than merely describe it:

Test Result before the fix
testSubstitutedValueIsNotRescanned set p to "Roger.txt", the name variable spliced into a filename that contained its placeholder
testNSNumberIsNotRenderedAsBoolean true true, a count of 1 rendered as a boolean
testJSONNumbersKeepTheirType true true 1.5, the same through JSONSerialization
testCommandLineBooleanResults false for a script returning true

The last one executes, unlike the rest of the suite. It runs return true and return false through osascript, which drives no application and needs no Automation permission, so it is safe in CI; the file header notes the exception.

Checklist

  • swift build and swift test pass.
  • New or changed public API has a /// doc comment describing purpose, parameters and return value.
  • Docs updated if behaviour, parameters or return values changed.
  • Tests added for anything deterministic (escaping, substitution, parsing).
  • No breaking change to public API — no signature changed, but three behaviour changes are worth a reviewer's attention:
    • toHFSPath() now requires the path to exist and returns nil otherwise, where it previously returned a string for any input.
    • Float and UInt variables now render as numeric literals instead of escaped text.
    • resetAutomationPermission(for:) now returns false when tccutil exits non-zero.

fredsimard and others added 9 commits August 16, 2026 16:45
osascript prints booleans as the strings "true" and "false", so Int(result) was always nil and every true result was received as false. Compare the string case-insensitively, keeping the integer coercion as a fallback so this path stays consistent with executeAppleScript, where NSAppleScript reports booleans through int32Value.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
waitUntilExit() ran before the pipe was drained: output larger than the pipe buffer (~64 KB) blocks the child on write while the parent waits for it to exit, deadlocking permanently. A .json listing of a large folder reaches that size easily. readDataToEndOfFile() now runs first and returns at EOF.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sequential replacingOccurrences loop rescanned text it had already substituted, so a value containing a placeholder name was itself rewritten by a later key: a file named $folderPath.txt passed as the folderPath variable had the folderPath value spliced into its name. Values stay escaped, so this was silent data corruption rather than injection. The script is now walked once, matching the longest key at each $ and appending its substitution to the result.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
toHFSPath() prepended the startup disk name to every path, so /Volumes/Backup/notes.txt became Macintosh HD:Volumes:Backup:notes.txt instead of Backup:notes.txt. An HFS path is volume-relative, and /Volumes/Backup on the startup disk is only a mount point, so the result pointed at nothing. The volume name and mount point are now read from the path itself, which also covers disk images and network shares, and handles a volume whose mount directory was renamed on a name collision.

The path must exist for the volume to be read; both overloads document this, and the path tests use NSHomeDirectory() instead of a fictional path. startupDiskName() had no callers left and is removed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CODE_REVIEW.md is a working document for the current audit and is not meant to be tracked.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An NSNumber bridges to Bool whatever it holds, so NSNumber(value: 1) as? Bool is true: a count arriving from JSONSerialization or from Objective-C matched the boolean case and was substituted as true instead of 1. Numbers are now unwrapped to the Swift type they actually carry before any type matching, using the CoreFoundation type to tell a boolean from a number and a floating-point value from an integral one.

The switch keeps one case per Swift type, all of them reachable. Float and UInt now reach the numeric cases rather than falling through to the escaped-text default.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
resetAutomationPermission returned true whenever the process launched, ignoring its exit status, so a tccutil failure was reported as a successful reset. The status is now checked and logged when non-zero.

The two remaining uses of localizedDescription are replaced by String(describing:), matching the rest of the file: localizedDescription returns a system-localized string, and the package messages are English only, as stated in README.md and CONTRIBUTING.md.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The throws list for executeAppleScript(_:with:) promised failedToReadOutput, which it never throws: an unparseable record or JSON result yields nil. Both execution methods documented a return type without [String: Any], which .record and .json produce. The record parser was documented as requiring alphanumeric keys, while it reads everything up to the first colon and rejects only an empty key. The .bool rows in README.md and API.md described integer coercion alone, now that text is read as well. The CLI method was described as throwing only on a launch failure, contradicting the table below it.

Documents that the command-line method passes the substituted script to osascript as an argument, readable by any local process through ps, and updates toHFSPath() for the volume-relative behaviour. Fixes a typo in the AppleScriptObject property list.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Unreleased section becomes 1.0.1 and gains a Fixed list for the six bugs found in the audit, three of which change what an existing call returns. A fresh, empty Unreleased section and the compare links follow.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fredsimard fredsimard self-assigned this Aug 16, 2026
@fredsimard fredsimard added documentation Improvements or additions to documentation bugfix This is a bug fix, nothing more to say. labels Aug 16, 2026
fredsimard and others added 3 commits August 16, 2026 17:49
Covers the three deterministic fixes and the one that needs execution: a substituted value is no longer rescanned for placeholders, an NSNumber renders as its value rather than as true, numbers surviving JSONSerialization keep their type, and a boolean result from the command-line method is read from the text osascript prints.

Each test was checked against the pre-fix sources and fails there, so they guard the behaviour rather than merely describe it. The osascript tests run self-contained scripts, driving no application and needing no Automation permission, which the file header now notes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SwiftLint runs with --strict and rejected the double space after a comma, left over from aligning the two script declarations.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The macOS runner image ships with aws/tap configured, and recent Homebrew versions warn about untrusted taps on every brew command, which surfaced as an annotation on the lint job. Untapping it before installing SwiftLint keeps that output clean. The || true keeps the job green once the runner image stops shipping that tap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fredsimard
fredsimard merged commit 56b6a0c into main Aug 16, 2026
3 checks passed
@fredsimard
fredsimard deleted the fix/critical-bugs-audit-2026 branch August 16, 2026 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This is a bug fix, nothing more to say. documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant