fix: complete Safe Release security and reliability work - #367
Merged
Conversation
This was referenced Aug 8, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request adds authenticated local HTTP access, reviewed cleanup authorization, identity-aware privileged execution, verified macOS updates, privacy-filtered Sentry reporting, deterministic project generation, and stricter release validation across macOS and Windows. ChangesSecurity and release controls
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryServer
participant QueryRateLimiter
participant Route
Client->>QueryServer: Send local HTTP request
QueryServer->>QueryRateLimiter: Check request window
QueryRateLimiter-->>QueryServer: Allow or reject
QueryServer->>QueryServer: Validate bearer token, Host, origin, method, and body
QueryServer->>Route: Dispatch authenticated request
Route-->>Client: Return structured HTTP response
sequenceDiagram
participant UpdatesView
participant UpdateWorkflow
participant StagingDirectory
participant InstalledApp
UpdatesView->>UpdateWorkflow: Start update check
UpdateWorkflow->>StagingDirectory: Create isolated staging area
UpdateWorkflow->>StagingDirectory: Download and hash archive
UpdateWorkflow->>InstalledApp: Validate candidate identity
InstalledApp-->>UpdateWorkflow: Accept or reject replacement
UpdateWorkflow-->>UpdatesView: Publish update phase and result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
macos/HelperSources/HelperService.swift (2)
435-452: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse one
HelperLineSplitterper pipe.Both reader threads share one splitter, and the splitter keeps a single byte buffer. If a stdout chunk ends mid-line, the next stderr chunk is appended to the same partial line. The two streams then merge into one garbled output line. The lock prevents data corruption, but it does not keep the streams separate.
Give each handle its own splitter, and flush both after the group completes.
🔧 Proposed fix
- let splitter = HelperLineSplitter() let group = DispatchGroup() + var splitters: [HelperLineSplitter] = [] for handle in [outPipe.fileHandleForReading, errPipe.fileHandleForReading] { + let splitter = HelperLineSplitter() + splitters.append(splitter) group.enter() DispatchQueue.global(qos: .utility).async { while case let chunk = handle.availableData, !chunk.isEmpty { for line in splitter.ingest(chunk) { emit(line) } } group.leave() } } group.wait() - for line in splitter.flush() { emit(line) } + for splitter in splitters { + for line in splitter.flush() { emit(line) } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/HelperSources/HelperService.swift` around lines 435 - 452, Update the pipe-reading loop to create a separate HelperLineSplitter for each handle, so stdout and stderr maintain independent partial-line buffers. Use that per-pipe splitter for ingest calls inside each reader task, then flush each splitter after group.wait() and emit its remaining lines.
596-636: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdmit the operation ID before the reviewed-path filesystem work.
HelperReviewedPathPolicy.validateruns beforereplayGuard.admit. Each request can therefore drive up toHelperReviewedPathPolicy.maximumTargetslstatandrealpathcalls in the root daemon without consuming an operation ID and without any authorization. A client that sends the same payload repeatedly repeats that work every time.Move the replay check above the identity and path validation. The path check still runs before authorization, so a bad list still produces no prompt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/HelperSources/HelperService.swift` around lines 596 - 636, Move the replayGuard.admit(request.operationID) check in the request-handling method to occur before HelperDaemonIdentityResolver.resolve and HelperReviewedPathPolicy.validate, rejecting replayed IDs immediately. Preserve the existing invalid-identity and invalid-reviewed-path responses, and keep reviewed-path validation before any authorization or prompt.macos/Sources/TuneUpView.swift (1)
407-421: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the stale-plan refusal to the user.
The plan sheet sets
showPlan = falseand then callsrunSafeSet(). IfpendingCleanupPlanis nil orvalidateForLaunch()fails,runSafeSetreturns without any feedback. The sheet closes and nothing happens. The user has no way to tell that the run was refused.The same guard also drops the maintenance step, because it returns before the
includeOptimizebranch appends.optimize.Show the same "cleanup preview can't be authorized" alert that
prepareRunReviewuses, and keep the maintenance step when only the clean step is refused.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/TuneUpView.swift` around lines 407 - 421, Update runSafeSet so a missing or invalid pendingCleanupPlan presents the same “cleanup preview can't be authorized” alert used by prepareRunReview instead of returning silently. Treat the cleanup step as refused rather than aborting the entire run, allowing the includeOptimize branch to append .optimize and proceed when maintenance remains; preserve the existing clean-step behavior when the plan validates.macos/Sources/PrivilegedHelper/HelperContract.swift (1)
616-676: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the replay guard memory.
evictExpiredonly removes entries older thanretention, which is one hour. When the request rate is high,orderandseengrow without limit, because thecapacitycap discards nothing while every entry is fresh.HelperService.executecallsadmitbefore authorization, so an unauthenticated caller controls that rate.The age-based rule is correct for replay safety. Add a hard ceiling so the daemon fails closed instead of growing. For example, refuse admission once
order.countexceeds a high multiple ofcapacity, and returnfalseso the request is rejected rather than served.🔧 Sketch of a fail-closed ceiling
func admit(_ operationID: String) -> Bool { lock.lock(); defer { lock.unlock() } let moment = now() evictExpired(before: moment.addingTimeInterval(-retention)) + // Nothing expired and the set is far past its budget: refuse rather + // than forget, so a flood cannot exhaust memory OR replay an ID. + guard order.count < capacity * hardCeilingFactor else { return false } guard !seen.contains(operationID) else { return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/PrivilegedHelper/HelperContract.swift` around lines 616 - 676, Update HelperReplayGuard.admit and its capacity handling to enforce a hard memory ceiling based on a high multiple of capacity after evicting expired entries. When the ceiling is reached, return false without inserting the operation ID so the request fails closed; preserve age-based eviction and normal admission below the ceiling.
🧹 Nitpick comments (10)
macos/Tests/QueryServerTests.swift (1)
84-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReference
tokenin the helper default.The default
credentialrepeats the literal"test-only-query-credential". Iftokenat line 15 changes, every test that relies on the default silently starts asserting a rejected credential.♻️ Proposed refactor
private func request(_ target: String, method: String = "GET", host: String? = "127.0.0.1:9277", - credential: String? = "test-only-query-credential", + credential: String? = nil, extraHeaders: [String] = []) -> String { var lines = ["\(method) \(target) HTTP/1.1"] if let host { lines.append("Host: \(host)") } - if let credential { lines.append("Authorization: Bearer \(credential)") } + lines.append("Authorization: Bearer \(credential ?? token)") lines.append(contentsOf: extraHeaders) return lines.joined(separator: "\r\n") + "\r\n\r\n" }This change needs a separate flag for the "no credential" case, because
nilnow means "use the valid token". An alternative that keeps the current call sites is to add an explicitomitCredential: Bool = falseparameter and defaultcredentialtotokenthrough a computed helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Tests/QueryServerTests.swift` around lines 84 - 94, Update the request helper to derive its default credential from the shared token symbol instead of duplicating the literal. Preserve an explicit way for callers to omit the Authorization header, since nil now represents the valid default token.macos/Sources/QueryServer.swift (1)
376-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParse the request once per connection.
parseRequestruns up to three times for one request: once intryServeEvents, once insideauthorize, and once here. The duplicate parsing also duplicates the rejection policy in two places.Consider parsing once in
receive, then passing the parsedRequestinto anauthorize(_ request:token:port:)overload and intoroute. This keeps the string-basedauthorizefor the existing tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/QueryServer.swift` around lines 376 - 410, Update the request flow so `receive` parses the raw input once and passes the resulting `Request` through authorization and routing. Add or use an `authorize(_ request:token:port:)` overload and a parsed-request `route` path, while retaining the existing string-based `authorize` for current tests and removing duplicate parsing/rejection checks.macos/Tests/HelperContractTests.swift (1)
203-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass reviewed paths to the shell-check loop.
testSteps_neverInvokeAShellcallssteps(interface: "en0")with no reviewed paths.cleanReviewedthen returns an empty array, so the one operation that accepts caller data contributes no assertions here. Pass a reviewed path, astestArguments_neverEmptyAndNeverShellMetacharactersdoes, so thefindstep is covered by this rule too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Tests/HelperContractTests.swift` around lines 203 - 214, Update testSteps_neverInvokeAShell to pass a representative reviewed path into operation.steps(interface:), matching the reviewed-path setup used by testArguments_neverEmptyAndNeverShellMetacharacters. Keep the existing shell and permitted-executable assertions so the find step from the caller-data operation is included.macos/Tests/PrivilegedSecurityTests.swift (1)
423-453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote the root-execution assumption.
This test makes an unlink fail by clearing the write bit on
locked. Root ignores directory write permission, so the delete succeeds when the test host runs as root andXCTAssertNotEqual(status, 0)fails.Add
try XCTSkipIf(getuid() == 0)so a root test run skips instead of reporting a false failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Tests/PrivilegedSecurityTests.swift` around lines 423 - 453, Add a root-execution guard at the start of testIrreversibleCleanupContinuesPastOneFailingEntryAndReportsFailure using try XCTSkipIf(getuid() == 0), so the permission-based failure scenario is skipped when running as root.macos/Sources/PrivilegedHelper/HelperCodeRequirement.swift (1)
237-248: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider the cost of the non-clone fallback.
COPYFILE_CLONEonly succeeds when the source andparentDirectoryare on the same volume. If the app runs from a different volume, such as a mounted image or an external disk, every privileged engine operation performs a full recursive copy of the app bundle into/private/var/tmp. That adds seconds of latency and transient disk use to each authenticated run.Consider logging when the fallback is taken, so this case is visible in the field.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/PrivilegedHelper/HelperCodeRequirement.swift` around lines 237 - 248, In the copy fallback within the surrounding snapshot method, log an informative message when the COPYFILE_CLONE attempt fails and the non-clone copy is about to run. Include enough context to identify the source and fallback path, while preserving the existing retry and SnapshotError behavior.macos/Sources/CleanReviewView.swift (1)
265-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the refusal reason to VoiceOver.
.help(reason)shows the reason as a tooltip only. The row uses.accessibilityElement(children: .combine), so VoiceOver announces the chip text "Can't clean" without the reason. Add an accessibility label that carries the reason.♻️ Proposed change
case .notCleanable(let reason): Chip(text: NSLocalizedString("Can't clean", comment: "clean badge"), color: Brand.amber) .help(reason) + .accessibilityLabel(String(format: NSLocalizedString("Can't clean: %@", comment: "clean badge accessibility"), reason))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/CleanReviewView.swift` around lines 265 - 267, Update the .notCleanable(let reason) branch in CleanReviewView so the chip’s accessibility label includes the refusal reason, ensuring VoiceOver announces both “Can’t clean” and reason while preserving the existing tooltip and visual styling.macos/Sources/PrivilegedExecution.swift (1)
343-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
exclusiveCreationShellinMoleCLI.elevatedScript.
PrivilegedSecurityTests.swift:490is the only caller. Production code duplicates the sink-creation contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/PrivilegedExecution.swift` around lines 343 - 348, The elevatedScript implementation should reuse the existing exclusiveCreationShell property instead of duplicating the sink-creation command and contract. Update MoleCLI.elevatedScript to invoke exclusiveCreationShell while preserving its current command sequencing and exit-code behavior.macos/Sources/UpdateWorkflow.swift (2)
369-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the named Security constants instead of raw flag values.
checkAllArchitecturesandsigningInformationhardcode1 << 0and0x2. The SDK exportskSecCSCheckAllArchitecturesandkSecCSSigningInformationfor these. Named constants keep this signature check correct if the values ever change, and they make the intent explicit at the call site.♻️ Proposed refactor
- let checkAllArchitectures = SecCSFlags(rawValue: 1 << 0) + let checkAllArchitectures = SecCSFlags(rawValue: kSecCSCheckAllArchitectures) @@ - var information: CFDictionary? - let signingInformation: UInt32 = 0x2 guard SecCodeCopySigningInformation( staticCode, - SecCSFlags(rawValue: signingInformation), + SecCSFlags(rawValue: kSecCSSigningInformation), &information ) == errSecSuccess,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/UpdateWorkflow.swift` around lines 369 - 394, Update read(appURL:) to use the named Security constants kSecCSCheckAllArchitectures and kSecCSSigningInformation instead of constructing SecCSFlags from the raw values 1 << 0 and 0x2. Preserve the existing validation and signing-information calls while making their flag intent explicit.
1186-1212: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the downloaded archive and the extracted tree.
URLSession.downloadwrites the whole response to disk before the SHA-512 check runs.ditto -x -kthen expands the archive without a size limit. A feed or server that returns an oversized archive fills the volume before verification rejects it. The staging directory is discarded on failure, so the damage is transient, but the disk pressure is real while the download runs.Check
http.expectedContentLengthagainst a maximum before you move the file, and check the archive size on disk before extraction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/UpdateWorkflow.swift` around lines 1186 - 1212, In the download flow around URLSession.shared.download and the ditto extraction, enforce the archive-size limit before moving the temporary file: reject any HTTP response whose expectedContentLength exceeds the configured maximum, then inspect the downloaded temporary file’s actual size and reject it if it exceeds that maximum before sha512 verification or extraction. Reuse the existing UpdateFailure error path and ensure oversized archives never reach FileManager.moveItem or MoEngine.shared.capture.macos/Sources/UpdatesView.swift (1)
592-610: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
inFlightis vestigial.
inFlightis incremented in the priming loop and never read again. The concurrency bound holds because thefor awaitloop enqueues exactly one task per completed task. Remove the variable so the bound is expressed by the constant alone.♻️ Proposed refactor
var iterator = items.makeIterator() - var inFlight = 0 func enqueue(_ item: AppUpdateItem) { @@ - while inFlight < 6, let next = iterator.next() { enqueue(next); inFlight += 1 } + for _ in 0..<6 { + guard let next = iterator.next() else { break } + enqueue(next) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Sources/UpdatesView.swift` around lines 592 - 610, Remove the unused inFlight variable and its increments from the withTaskGroup scheduling logic around enqueue and the priming while loop. Preserve the concurrency limit by retaining the constant-based initial enqueue condition and the existing one-task-per-completion behavior in the for await loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/macos-signing.md`:
- Around line 193-194: Update Section 5 of the macOS signing documentation to
include SENTRY_AUTH_TOKEN in the required-secret list, the secret-list
verification command, and the corresponding gh secret set command, keeping the
existing credential-gate instructions consistent.
In `@macos/Sources/CleanSelection.swift`:
- Around line 21-24: Update lockedSummary to restrict the locked-item count and
collected names to entries with the .appOpen reason, excluding .notCleanable
entries from the close-app guidance while preserving the existing app-name
summary behavior.
In `@macos/Sources/LaunchDiagnostics.swift`:
- Around line 429-443: The safeDiagnosticLabel function must reject credential
assignments such as access_token = value and api key=value, including optional
whitespace before : or =. Extend its sensitive-marker validation to detect these
credential-name patterns, and add regression coverage for the specified forms.
In `@macos/Sources/MoleCLI.swift`:
- Around line 95-101: Update the nil-handling failure message in
OperationFlow.start when trustedExecutable() returns no path, replacing the
Homebrew-specific wording with the bundled engine guidance, preferably reusing
MoleCLI.currentEngineUpdateInstruction for the .unavailable case.
In `@macos/Sources/OperationFlow.swift`:
- Around line 538-556: Update the successful-spawn cleanup around the
elevated-run handling in OperationFlow so the parent closes its pipe write
descriptors for elevated runs as well as non-elevated runs. Ensure both stdout
and stderr write handles are closed before the termination handler’s
readDataToEndOfFile calls, while preserving the existing behavior for failed
spawns.
In `@macos/Sources/PrivilegedHelper/HelperContract.swift`:
- Around line 329-342: Update HelperReviewedCleanup.approvedRoots so the
/Library/Logs root uses allowsForeignOwner: false, while preserving the existing
true setting for /Library/Caches. Apply the same correction to the corresponding
approved-root definition near HelperReviewedRoot usage.
In `@macos/Sources/SettingsView.swift`:
- Line 628: Update the “Authentication” infoRow call in SettingsView so its
“Bearer token required” value is localized at the call site, matching the
existing Chinese localization entries; do not rely on infoRow’s label-only
localization behavior.
In `@macos/Sources/TuneUp.swift`:
- Around line 20-24: Update the notice computed property to wrap both
user-facing string literals in NSLocalizedString, preserving the existing
includesClean conditional and text while enabling the zh-Hans and zh-Hant
localized values.
In `@macos/Sources/UpdatesView.swift`:
- Around line 904-909: Update cancel(_:) to tear down any sparkleSessions entry
for item.id by stopping the matching ExternalSparkleUpdateSession, removing it
via finishSparkleSession, and resuming its checked continuation exactly once.
Preserve the existing cancelAppTask, staged update cleanup, and failed-cancelled
phase assignment.
In `@macos/Sources/UpdateWorkflow.swift`:
- Around line 16-22: Update the file-removal implementation around
descriptorRelativeRemoveFile to use the SDK-imported removefileat declaration
instead of the private `@_silgen_name` wrapper. Replace hardcoded removal flags
with REMOVEFILE_RECURSIVE and REMOVEFILE_KEEP_PARENT, and remove the unsupported
REMOVEFILE_RECURSIVE_SLIM (1 << 11) flag.
In `@macos/Tests/UpdateWorkflowTests.swift`:
- Around line 116-124: Replace the ineffective path assertions in the test
covering the generated temporary directories with assertions that validate the
actual mkdtemp template shape, using the expected template/prefix symbol or
pattern. Keep the uniqueness and permission assertions unchanged.
In `@SECURITY.md`:
- Around line 111-118: Update the tool-count wording in the security
documentation near the engine execution description from “those four Apple
tools” to “those five Apple tools,” matching the five tools listed earlier and
leaving the surrounding security guarantees unchanged.
In `@windows/Services/LocalMcpServerService.cs`:
- Around line 1398-1401: Update EvaluateRequest and its callers to reject any
Sec-Fetch-* header, not just Sec-Fetch-Site; preferably replace the secFetchSite
parameter with a boolean such as hasFetchMetadata and set it when any
fetch-metadata header is present. Preserve the existing forbidden decision
alongside the other request checks and update related tests.
In `@windows/Tools/McpStdioBridge/Program.cs`:
- Around line 20-40: Validate BURROWWIN_MCP_ENDPOINT in the startup
endpoint-selection flow before applying the stored token, accepting only the
configured loopback MCP URI and otherwise falling back to the safe default or
rejecting the configuration. Ensure the same restriction is enforced in the
request/bridge handling around the code corresponding to lines 62-84, before any
credential-bearing request is sent.
---
Outside diff comments:
In `@macos/HelperSources/HelperService.swift`:
- Around line 435-452: Update the pipe-reading loop to create a separate
HelperLineSplitter for each handle, so stdout and stderr maintain independent
partial-line buffers. Use that per-pipe splitter for ingest calls inside each
reader task, then flush each splitter after group.wait() and emit its remaining
lines.
- Around line 596-636: Move the replayGuard.admit(request.operationID) check in
the request-handling method to occur before HelperDaemonIdentityResolver.resolve
and HelperReviewedPathPolicy.validate, rejecting replayed IDs immediately.
Preserve the existing invalid-identity and invalid-reviewed-path responses, and
keep reviewed-path validation before any authorization or prompt.
In `@macos/Sources/PrivilegedHelper/HelperContract.swift`:
- Around line 616-676: Update HelperReplayGuard.admit and its capacity handling
to enforce a hard memory ceiling based on a high multiple of capacity after
evicting expired entries. When the ceiling is reached, return false without
inserting the operation ID so the request fails closed; preserve age-based
eviction and normal admission below the ceiling.
In `@macos/Sources/TuneUpView.swift`:
- Around line 407-421: Update runSafeSet so a missing or invalid
pendingCleanupPlan presents the same “cleanup preview can't be authorized” alert
used by prepareRunReview instead of returning silently. Treat the cleanup step
as refused rather than aborting the entire run, allowing the includeOptimize
branch to append .optimize and proceed when maintenance remains; preserve the
existing clean-step behavior when the plan validates.
---
Nitpick comments:
In `@macos/Sources/CleanReviewView.swift`:
- Around line 265-267: Update the .notCleanable(let reason) branch in
CleanReviewView so the chip’s accessibility label includes the refusal reason,
ensuring VoiceOver announces both “Can’t clean” and reason while preserving the
existing tooltip and visual styling.
In `@macos/Sources/PrivilegedExecution.swift`:
- Around line 343-348: The elevatedScript implementation should reuse the
existing exclusiveCreationShell property instead of duplicating the
sink-creation command and contract. Update MoleCLI.elevatedScript to invoke
exclusiveCreationShell while preserving its current command sequencing and
exit-code behavior.
In `@macos/Sources/PrivilegedHelper/HelperCodeRequirement.swift`:
- Around line 237-248: In the copy fallback within the surrounding snapshot
method, log an informative message when the COPYFILE_CLONE attempt fails and the
non-clone copy is about to run. Include enough context to identify the source
and fallback path, while preserving the existing retry and SnapshotError
behavior.
In `@macos/Sources/QueryServer.swift`:
- Around line 376-410: Update the request flow so `receive` parses the raw input
once and passes the resulting `Request` through authorization and routing. Add
or use an `authorize(_ request:token:port:)` overload and a parsed-request
`route` path, while retaining the existing string-based `authorize` for current
tests and removing duplicate parsing/rejection checks.
In `@macos/Sources/UpdatesView.swift`:
- Around line 592-610: Remove the unused inFlight variable and its increments
from the withTaskGroup scheduling logic around enqueue and the priming while
loop. Preserve the concurrency limit by retaining the constant-based initial
enqueue condition and the existing one-task-per-completion behavior in the for
await loop.
In `@macos/Sources/UpdateWorkflow.swift`:
- Around line 369-394: Update read(appURL:) to use the named Security constants
kSecCSCheckAllArchitectures and kSecCSSigningInformation instead of constructing
SecCSFlags from the raw values 1 << 0 and 0x2. Preserve the existing validation
and signing-information calls while making their flag intent explicit.
- Around line 1186-1212: In the download flow around URLSession.shared.download
and the ditto extraction, enforce the archive-size limit before moving the
temporary file: reject any HTTP response whose expectedContentLength exceeds the
configured maximum, then inspect the downloaded temporary file’s actual size and
reject it if it exceeds that maximum before sha512 verification or extraction.
Reuse the existing UpdateFailure error path and ensure oversized archives never
reach FileManager.moveItem or MoEngine.shared.capture.
In `@macos/Tests/HelperContractTests.swift`:
- Around line 203-214: Update testSteps_neverInvokeAShell to pass a
representative reviewed path into operation.steps(interface:), matching the
reviewed-path setup used by testArguments_neverEmptyAndNeverShellMetacharacters.
Keep the existing shell and permitted-executable assertions so the find step
from the caller-data operation is included.
In `@macos/Tests/PrivilegedSecurityTests.swift`:
- Around line 423-453: Add a root-execution guard at the start of
testIrreversibleCleanupContinuesPastOneFailingEntryAndReportsFailure using try
XCTSkipIf(getuid() == 0), so the permission-based failure scenario is skipped
when running as root.
In `@macos/Tests/QueryServerTests.swift`:
- Around line 84-94: Update the request helper to derive its default credential
from the shared token symbol instead of duplicating the literal. Preserve an
explicit way for callers to omit the Authorization header, since nil now
represents the valid default token.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 050121c0-83f8-4085-8282-a5546ec5bead
📒 Files selected for processing (88)
.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/sentry-issues.ymlREADME.mdSECURITY.mdTELEMETRY.mddocs/macos-signing.mdmacos/HelperSources/HelperService.swiftmacos/Resources/Info.plistmacos/Resources/zh-Hans.lproj/Localizable.stringsmacos/Resources/zh-Hant.lproj/Localizable.stringsmacos/Sources/CleanReviewView.swiftmacos/Sources/CleanSelection.swiftmacos/Sources/CleanView.swiftmacos/Sources/CleanupAuthorization.swiftmacos/Sources/CrashReporter.swiftmacos/Sources/ExternalSparkleUpdateSession.swiftmacos/Sources/LaunchDiagnostics.swiftmacos/Sources/MCP.swiftmacos/Sources/MoleCLI.swiftmacos/Sources/OperationFlow.swiftmacos/Sources/PopupView.swiftmacos/Sources/PortsView.swiftmacos/Sources/PrivilegeBroker.swiftmacos/Sources/PrivilegedExecution.swiftmacos/Sources/PrivilegedHelper/HelperCodeRequirement.swiftmacos/Sources/PrivilegedHelper/HelperContract.swiftmacos/Sources/PrivilegedHelperClient.swiftmacos/Sources/ProcessActions.swiftmacos/Sources/ProcessWatchdog.swiftmacos/Sources/QueryServer.swiftmacos/Sources/SettingsView.swiftmacos/Sources/SoftwareView.swiftmacos/Sources/StatusView.swiftmacos/Sources/Store.swiftmacos/Sources/TuneUp.swiftmacos/Sources/TuneUpView.swiftmacos/Sources/UpdateSources.swiftmacos/Sources/UpdateWorkflow.swiftmacos/Sources/UpdatesView.swiftmacos/Tests/CrashReporterPolicyTests.swiftmacos/Tests/Fixtures/SentrySensitiveEvent.jsonmacos/Tests/HelperCodeRequirementTests.swiftmacos/Tests/HelperContractTests.swiftmacos/Tests/MCPEnvelopeTests.swiftmacos/Tests/MoleCLITests.swiftmacos/Tests/OperationFlowTests.swiftmacos/Tests/PrivilegeBrokerTests.swiftmacos/Tests/PrivilegeRouteTests.swiftmacos/Tests/PrivilegedSecurityTests.swiftmacos/Tests/ProcessActionsTests.swiftmacos/Tests/ProcessWatchdogTests.swiftmacos/Tests/QueryEventsTests.swiftmacos/Tests/QueryServerTests.swiftmacos/Tests/SoftwareModelTests.swiftmacos/Tests/StoreTests.swiftmacos/Tests/TuneUpTests.swiftmacos/Tests/UpdateSeenStoreTests.swiftmacos/Tests/UpdateWorkflowTests.swiftmacos/Tests/UpdatesModelTests.swiftmacos/project.ymlmacos/vendor/burrow-enginescripts/fetch-sentry-cli.shscripts/fetch-sentry.shscripts/fetch-sparkle.shscripts/fetch-xcodegen.shscripts/release-input.pyscripts/release-inputs.jsonscripts/release.shscripts/sentry_public_summary.pyscripts/tests/fixtures/sentry-sensitive-event.jsonscripts/tests/fixtures/sentry-sensitive-issue.jsonscripts/tests/test_helper_version_sync.pyscripts/tests/test_release_workflows.pyscripts/tests/test_sentry_issues_workflow.pyscripts/tests/test_validate_release_notes.pyscripts/verify-dsym-uuids.shscripts/verify-macos-release.shscripts/verify-project-generation.pywindows/BURROW_WINDOWS_ALIGNMENT.mdwindows/Models/BurrowSettings.cswindows/Services/JsonApplicationSettingsService.cswindows/Services/LocalMcpServerService.cswindows/Tests/BurrowWin.Tests/JsonApplicationSettingsServiceTests.cswindows/Tests/BurrowWin.Tests/LocalMcpServerServiceTests.cswindows/Tools/McpStdioBridge/Program.cswindows/ViewModels/SettingsViewModel.cswindows/run-local.ps1
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macos/Sources/UpdateWorkflow.swift`:
- Around line 1203-1217: Replace the post-download size checks in the download
flow with a URLSession download delegate that tracks received bytes and cancels
the task as soon as Self.maximumArchiveBytes is exceeded. Before invoking ditto,
enforce limits on the archive’s uncompressed byte total and entry count, or
extract into storage with an equivalent quota; ensure both checks reject
oversized archives before extraction begins.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02e32b93-8bde-4443-9bec-466ffdb299a4
📒 Files selected for processing (23)
SECURITY.mddocs/macos-signing.mdmacos/HelperSources/HelperService.swiftmacos/Sources/CleanReviewView.swiftmacos/Sources/CleanSelection.swiftmacos/Sources/ExternalSparkleUpdateSession.swiftmacos/Sources/LaunchDiagnostics.swiftmacos/Sources/OperationFlow.swiftmacos/Sources/PrivilegedHelper/HelperContract.swiftmacos/Sources/SettingsView.swiftmacos/Sources/TuneUp.swiftmacos/Sources/TuneUpView.swiftmacos/Sources/UpdateWorkflow.swiftmacos/Sources/UpdatesView.swiftmacos/Tests/CleanSelectionTests.swiftmacos/Tests/HelperContractTests.swiftmacos/Tests/LaunchDiagnosticsTests.swiftmacos/Tests/PrivilegedSecurityTests.swiftmacos/Tests/QueryServerTests.swiftmacos/Tests/UpdateWorkflowTests.swiftwindows/Services/LocalMcpServerService.cswindows/Tests/BurrowWin.Tests/LocalMcpServerServiceTests.cswindows/Tools/McpStdioBridge/Program.cs
🚧 Files skipped from review as they are similar to previous changes (16)
- macos/Sources/SettingsView.swift
- macos/Sources/TuneUp.swift
- macos/Sources/CleanReviewView.swift
- macos/Sources/LaunchDiagnostics.swift
- macos/Tests/UpdateWorkflowTests.swift
- windows/Tests/BurrowWin.Tests/LocalMcpServerServiceTests.cs
- macos/Tests/HelperContractTests.swift
- macos/Tests/PrivilegedSecurityTests.swift
- SECURITY.md
- macos/Sources/OperationFlow.swift
- macos/Tests/QueryServerTests.swift
- windows/Services/LocalMcpServerService.cs
- docs/macos-signing.md
- macos/Sources/UpdatesView.swift
- macos/Sources/PrivilegedHelper/HelperContract.swift
- macos/HelperSources/HelperService.swift
Owner
Author
|
@coderabbitai review |
|
Reverts the manifest-based irreversible cleanup added in 9f3208f and restores `find -x <path> -depth -delete` behind the existing boundary checks. The manifest walked every reviewed tree in the GUI to enumerate descendants, then spawned one root process per node; on a large Xcode cache that is tens of thousands of spawns, and any entry whose volume differed from the quarantine root aborted the whole run. The deletes now live next to the plan that authorizes them, as `CleanupExecutionPlan.irreversibleCleanupShell()`, so a test can execute the real shell rather than a reimplementation of it. Two reporting bugs went with it: * `find -delete` is documented to always return true. It exits 0 having printed "Permission denied" and removed nothing, so its exit status was never a success signal -- a clean that freed nothing reported success. Each entry is now re-checked after the delete and the shell fails on any survivor. * One failing entry aborted the remaining entries and the run was reported as if nothing had happened. Entries are independent now, and `failureMessage` distinguishes "nothing ran" from "ran and partly failed" instead of telling users to rescan after a partial delete. Wrapper exit codes are named in `ElevatedExitCode` rather than bare 124/125/126/127 spread across two files, and a refusal before the auth prompt now carries its reason into the transcript instead of surfacing as an unexplained 126.
`ValidatedElevatedCommand.prepare` required root ownership of the privileged executable and of every ancestor directory, with no group-writable component anywhere. No shipped copy of Burrow can satisfy that. `/Applications` is root:admin mode 0775 on stock macOS, and an app dragged there -- or installed by the Homebrew cask -- is owned by the account that installed it. Optimize therefore refused before raising a prompt on any install that wasn't running from a root-owned build directory, which is every install except a developer's. Ownership is now a policy rather than one rule. System tools outside the bundle keep the strict version. Inside our own signed bundle, ownership may be root or the invoking user and only world-writable components are refused -- group-writable is how macOS ships /Applications, so refusing it refuses the platform. What pays for the relaxation is the resource seal: `codesign --verify --strict` already runs as root at the execution boundary, and every ancestor stays pinned and re-checked there regardless of who owns it. The tests build the real /Applications layout -- group-writable parent, user-owned bundle -- which is the coverage whose absence let a rule that cannot hold in production ship as a hardening change.
Clean was the one elevated operation that never reached the privileged helper. Routing recognises engine argv, and once the permanent clean became a reviewed plan rather than a `mo clean` invocation it had no argv to recognise, so it fell through to osascript -- which authenticates through SecurityAgent's classic mechanism and is password-only by construction. The most consequential operation Burrow performs quietly lost Touch ID while every other one kept it. It cannot simply route to the existing `clean` operation: that runs the engine's own selection rules and discards the entries the user reviewed. So this adds `HelperOperation.cleanReviewed`, the only operation that carries data from the caller beyond a verb. That does not weaken the property the helper rests on. argv is still composed daemon-side, the executable is still a fixed absolute path (/usr/bin/find joins the closed system-tool set), and the paths are operands to a delete rather than anything executed. What makes it safe is that the daemon does not trust the list: it rebuilds the approved roots from its own getpwuid record and re-derives every fact with its own lstat. An entry must exist, not be a symlink, equal its own canonical path -- so no ancestor is a link either -- sit strictly inside the invoking user's home or a system cache, be on that root's volume, and outside the shared system caches belong to the invoking user. That last rule is the one that matters: /private/var/folders holds other accounts' trees, and root deleting those is an escalation the user could not perform themselves. The list is capped and one bad entry refuses the whole request. Success is the postcondition, not find's exit status, for the same reason as on the osascript path. Two daemon fixes in the same files: * Cancellation is bound to the invoking uid. On a Mac with several signed-in users, knowing an operation ID was enough to stop another session's root work. * The output splitter buffers bytes rather than strings. A read ending mid-UTF-8-sequence made `String(data:encoding:)` return nil and the whole chunk was dropped, losing entire lines of a root operation's output -- and paths are exactly where non-ASCII shows up.
Replay guard: eviction was a count-bounded FIFO, so a caller could send `capacity` fresh operation IDs to push an older one out of the set and then replay that older payload. The bound meant to prevent replay was itself the bypass. Eviction is by age now -- an ID is only forgotten once it is far older than any authorization could still be valid for -- with the count cap kept as a backstop that only discards already-expired entries. Update verification: `SecStaticCodeCheckValidity` ran with a nil requirement, which validates against the code's own designated requirement -- something a self-signed bundle satisfies trivially. The only other thing pinned was the team identifier, a string in the certificate subject that a self-signed certificate is free to claim. It now checks `anchor apple generic`, which only Apple-issued chains satisfy, and a requirement that fails to build refuses rather than skipping the check. Rate limiting (macOS and Windows): the limiter ran only after a request had authenticated, leaving rejected requests uncounted. That gave an attacker unlimited token guesses and unlimited parse work from the same loopback socket -- the exact load the limiter exists to cap. Both count before authorization now. Elevated log tail: the poll ran on the main run loop, so a busy or modal UI could miss the short window the root shell allows before its trap unlinks the sink, losing the entire transcript. It runs on the stream queue now, which also puts every access to the log handle on the one queue that owns it -- previously the timer and the termination handler raced to open, read and close it, including reads against a descriptor the other side had already closed. The GUI's decode sites use lossy decoding so a chunk boundary mid-sequence can no longer drop output. Also corrects the helper code-requirement comment, which claimed no team identifier appears anywhere in the repository; the release workflow pins one as a build-time assertion. The runtime check still discovers it.
The engine's export list names a parent AND its own children as separate entries. The helper deleted them in plan order, so a parent went first and every nested entry under it was already gone when its turn came -- `find` exited nonzero with "No such file or directory" for work that had actually succeeded, and the run surfaced as a failure. Two causes, both fixed. The helper path used raw plan order while the osascript path had always sorted deepest-first; both now share `orderedReviewedPaths()`, so no entry can vanish before it is reached. And the daemon treated `find`'s status as the verdict despite its own comment saying the postcondition is authoritative -- `-delete` returns true when it removed nothing and false for an entry a deeper delete already took, so it was never a usable signal. For reviewed cleans the daemon now decides purely on whether each path is gone afterwards, in both directions: survivors fail the run, and an absent entry is a success whatever `find` said on its way out. Entries the snapshot refuses now carry a `notCleanable` lock reason and render a "Can't clean" chip with the reason on hover, rather than sitting tickable in a list that could never act on them. They stay listed because the scan preview counted them, and silently dropping a line the user was just shown reads as a miscount. Clean now sits beside Scan your Mac on the idle hero for anyone who already trusts the engine's selection. It routes to the plain `clean` operation -- no plan, so no review -- and the elevation prompt is still ahead of any deletion.
Verified each finding against the code rather than applying it; two were rejected on evidence and are left alone, with the reasons recorded below. Genuine defects: - The reviewed-clean header counted every locked entry but collected only .appOpen names, so a run whose sole locked entry was one the snapshot refused rendered "Close to clean another 12 GB" -- an empty app name promising that quitting something frees bytes no app holds. Restricted to .appOpen, with the neutral message when nothing is app-held. - HelperService shared ONE HelperLineSplitter between the stdout and stderr reader threads. That buffer exists to carry a partial line, so sharing it was both a data race and a splice: half a stdout line could be prefixed onto the next stderr chunk, emitting lines that appeared on neither stream. One splitter per pipe, each flushed separately. - HelperReplayGuard grew without bound. Age-based eviction is right -- a count-bounded FIFO lets a flood push out an ID that is still replayable -- but "keep everything inside the retention window" has no ceiling in a process running as root. It now refuses new IDs at a hard ceiling instead of forgetting old ones: a refused request is recoverable, a forgotten ID is replayable. - Cancelling a Sparkle update never resumed its checked continuation. Cancelling a Task does not resume one, so the session stayed registered and every `sparkleSessions.isEmpty` gate stayed shut for the rest of the launch. Cancel now settles through the existing idempotent finishOnce. - OperationFlow closed its pipe write copies only on the unelevated path, though the elevated branch hands the same two pipes to osascript and ends with readDataToEndOfFile on each -- a read that returns only once every write descriptor is gone, this parent's included. - The elevation failure named Homebrew as the trusted location, which trustedExecutable() deliberately stopped honouring; it pointed users at the one place that could never satisfy it. - safeDiagnosticLabel matched `token=` only when written tight, so `access_token = ...` and `api key=...` carried credentials into an uploaded diagnostic. Now matched by credential-name-plus-separator. - The stdio bridge took BURROWWIN_MCP_ENDPOINT unvalidated and then attached the stored bearer token to every request sent there, so any process able to set that variable redirected the bridge AND handed over the local MCP credential. Non-loopback endpoints now fall back. - Only Sec-Fetch-Site was rejected, so a request carrying just Sec-Fetch-Mode or Sec-Fetch-Dest reached the loopback server. Any fetch-metadata header now forbids. - No size ceiling existed on an update archive before it was kept and handed to ditto, which is where a small archive expands without bound. Correctness of the record, and tests that proved nothing: - SECURITY.md said four permitted Apple tools; the helper permits five. - macos-signing.md omitted SENTRY_AUTH_TOKEN, which sentry-issues.yml requires and which fails by warning rather than erroring when absent. - The staging-directory test asserted the name did not contain the literal source text "UUID().uuidString", which no path could contain, so it passed regardless. It now checks the real mkdtemp shape. - testSteps_neverInvokeAShell passed no reviewed paths, so the one step built from caller-supplied data was never examined. - The query-server request helper duplicated the token literal, and the cleanup test failed as root, where the permission denial it needs cannot happen. - TuneUp's notices and the query-server auth value were never localizable; zh-Hans already carried a translation that could not be reached. - Pressing Run with a stale cleanup plan returned silently. It now says so, and refuses only the clean so maintenance still runs. Rejected, with the evidence: - Making /Library/Logs allowsForeignOwner: false would disable the feature. That tree is root:wheel with 38 root-owned and 2 _windowserver-owned entries and none owned by the invoking user, so requiring an owner match refuses every entry. Foreign ownership is exactly why it needs true; the escalation concern applies to per-user trees, which are already false. - Admitting the replay guard before identity resolution would let anyone flood it with unvalidated IDs -- the surface the new ceiling closes -- and a full guard then fails closed on the legitimate user. Today a caller must pass the same-user identity check before it can burn an ID.
…371) * test(macos): stop conductor tests depending on what the build staged Six tests asserted that no `burrow` conductor was bundled, without ever saying so. Resources/burrow only appears when the vendor/burrow-cli submodule is checked out -- which a developer must do for the Network, Orphans and Photos panes to work at all -- so the suite was green on CI, where actions/checkout fetches no submodules, and red on a correctly-configured checkout, for reasons unrelated to the code under test. BurrowConductor now resolves its sidecars through an injectable `resourceDirectory` instead of reading Bundle.main directly, and ConductorBundleFixture stages a temporary directory that either is empty or holds an executable stub. Each test now declares which build it exercises. Verified green both with the conductor bundled and with it removed. Two of those tests were not testing what they claimed. Streaming is ON by default -- `streamingEnabled` returns true when the switch is unset, as its own comment says -- so `testStreamOverride_offByDefault_keepsDirectEngine` passed only because streamOverride bailed at its last guard, the executable lookup. It asserted nothing about the switch and would have kept passing had the default flipped either way. It is now three tests: no conductor falls back, a bundled conductor routes through by default, and the documented kill-switch actually kills it. testMissingExecutableFailsBeforeSpawn had the same shape -- with a conductor staged, streamOverride supplies an executable before resolveMo is consulted, so the branch under test was unreachable. Adds the conductor-present coverage that did not exist: bad arguments still surface as badArguments rather than a conductor error, and a non-executable file named `burrow` does not count as one. * test(macos): stop the conductor tests editing real preferences The streaming-switch tests wrote and then REMOVED `BurrowStreamViaConductor`, and the suite runs against UserDefaults for the real app domain — so running the tests erased a kill-switch a developer had genuinely set. They now save the prior value, absent or not, and restore it. Two assertions also proved less than they looked: the bad-arguments check matched substrings of `String(describing:)` rather than the error, so a different error carrying the same words would have passed, and the non-executable fixture never checked that its file was actually created.
caezium
force-pushed
the
bur-011-safe-release
branch
from
August 10, 2026 14:57
e0547eb to
aa94e78
Compare
This was referenced Aug 10, 2026
caezium
added a commit
that referenced
this pull request
Aug 10, 2026
The assertion I added in #367 read "no X remains in the suffix", on the theory that a leftover X meant mkdtemp had not substituted the template. It does not: mkdtemp draws replacements from an alphanumeric set that INCLUDES 'X', so a perfectly good name like BurrowUpdate.YmzfxX tripped it. Roughly a one-in-eight chance per directory, and it passed locally purely by luck before failing on CI. The failure actually worth guarding against is the template surviving whole, so assert that instead — the suffix is not "XXXXXX" — plus the charset. Ran 30 consecutive times with no failures.
caezium
added a commit
that referenced
this pull request
Aug 10, 2026
…nt" respond (#377) * fix(macos): notify when the scan finishes, and make "Stop after current" respond Two things found hand-testing the release candidate. The scan never posted a completion notice. It is the step people walk away from — it can run for minutes on a full disk and, unlike the clean, it ends by just sitting there with a number — so it now opts into the same `notifyOnEnd` the real run already used, inheriting the Settings toggle and staying silent for anyone who turned notifications off. It also needs its own `finalDetail`. The notification body defaults to whatever the last streamed HUD line happened to be, and for a task report that is the "=====" separator, so the first notice arrived as a row of equals signs. It now says what was found, or that there was nothing. "Stop after current" looked broken. `cancelUpdateAll()` set a flag that was private and not @published, so SwiftUI could not observe it and the button never changed state — the stop was queued correctly but nothing on screen said so, and the wait for the in-flight item made the click look ignored. The flag is published now, the label becomes "Stopping after current…", and the button disables so it cannot be pressed twice. * test(macos): stop the staging-directory test failing on a random X The assertion I added in #367 read "no X remains in the suffix", on the theory that a leftover X meant mkdtemp had not substituted the template. It does not: mkdtemp draws replacements from an alphanumeric set that INCLUDES 'X', so a perfectly good name like BurrowUpdate.YmzfxX tripped it. Roughly a one-in-eight chance per directory, and it passed locally purely by luck before failing on CI. The failure actually worth guarding against is the template surviving whole, so assert that instead — the suffix is not "XXXXXX" — plus the charset. Ran 30 consecutive times with no failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the version-agnostic Burrow — Safe Release project: BUR-71–BUR-85 and BUR-87–BUR-90.
Summary
Key changes
Privilege and destructive-operation boundaries
HelperService.swift,HelperContract.swift, andPrivilegedHelperClient.swiftbind requests to the XPC peer UID, resolve the canonical account throughgetpwuid_r, reject unsafe homes, and construct an explicit environment.CleanupAuthorization.swift,PrivilegedExecution.swift, andOperationFlow.swiftpin reviewed descendants, serialize an integrity-bound manifest, capture exact nodes into root-only quarantine, and treat every nonzero exit as failure.HelperCodeRequirement.swiftverifies and launches the same root-private app snapshot, preventing signature-check/path-use races.Updates and process safety
UpdateWorkflow.swift,UpdatesView.swift, andExternalSparkleUpdateSession.swiftadd bounded retries, generation ownership, HTTPS-only Electron staging, exact sealed version/build/CDHash/inode checks, explicit restart consent, rollback verification, and descriptor-relative cleanup.ProcessActions.swiftandProcessWatchdog.swiftconfirm termination and revalidate immutable process identity immediately before signaling.SoftwareView.swiftand the Updates model discard stale and out-of-order async results.Release, privacy, and local API
QueryServer.swiftand the Windows bridge require the per-install bearer credential, exact localhost host handling, browser-origin rejection, request bounds, and rate limiting.Usage
The optional loopback API now requires the per-install token:
The stdio MCP bridges add the credential automatically.
Nested engine dependency
This PR advances
macos/vendor/burrow-enginefroma6d1a98to9d5a102for BUR-72/BUR-89 operation-log hardening. The exact nested diff is available on burrow-digger/compare/a6d1a98...bur-011-safe-release. No separate engine PR was opened.Test plan
scripts/check.sh --no-formatdotnetwas unavailable on the macOS verification hostOut of scope
PR #249 and BUR-86 are intentionally excluded. The iMessage PR remains open and unmerged; its Linear ticket is backlogged under Burrow — Agent-Native Product.
Summary by CodeRabbit
New Features
Bug Fixes
Privacy