Handle unsigned macOS game executable signing#150
Conversation
There was a problem hiding this comment.
Pull request overview
Updates macOS launcher entitlement handling to correctly support the case where the game’s main executable is completely unsigned, avoiding codesign failures caused by invalid nested bundles during signing.
Changes:
- Adds an
EntitlementCheckResultstate machine to distinguishvalid,missing, andunsignedentitlement/signing states. - Introduces a new unsigned flow that signs a temporary copy of the main executable and writes the signed result back to the original file.
- Updates entitlement verification call sites to match against
.validinstead of a boolean.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
How did you hit a state where the game app is unsigned? |
wycats
force-pushed
the
wycats/macos-entitlement-signing-fallback
branch
from
e128672 to
cf7bf59
Compare
I honestly don't know but it's what happened. I can try to provide more logs or other information. I agree it's weeeeird. |
|
Nah it's fine. It's a decent edge case to have coverage for |
| UserDefaults.standard.set(false, forKey: "forceEntitlementReapplication") | ||
| return | ||
| case .missing: | ||
| break |
There was a problem hiding this comment.
If an entitlement key is missing, nothing happens? Yet the code returning this breaks out of the loop on the first missing entitlement.... surely we should be saying something somewhere?
There was a problem hiding this comment.
Reviewer note: the concern is valid, though partly a readability issue. Two things:
breakhere just exits theswitch(Swiftbreak, not a loop break), so control does fall through to the re-signing block below, which logslogger.warning("Game is missing required loader entitlements, re-signing is needed"). So it isn't a no-op — but a barecase .missing: breakwith no comment reads like one, which is what makes this confusing.- The genuinely weak part: which entitlement is missing is only logged at
.debug(incheckEntitlements, the "not found"/"mismatch" branches). On a default logger that's invisible, so when the re-sign path runs you can't tell why from normal logs.
Suggested fix — make the fall-through explicit and surface the missing key at a visible level:
case .missing:
// Fall through to the re-signing flow below.
breakand bump the checkEntitlements diagnostics from .debug to .notice so the missing/mismatched key shows up in logs.
|
|
||
| // Now sign just the main executable with the entitlements | ||
| // This preserves nested code signatures and only modifies what we need | ||
| logger.info("Signing executable with entitlements...") |
There was a problem hiding this comment.
Do we care that a bunch of debug info was removed?
There was a problem hiding this comment.
Reviewer note: mostly fine, one line worth restoring.
- The
--remove-signaturestdout/stderr logging is gone because the step itself is gone — the new copy-then---force-sign approach never removes a signature first, so that logging is correctly obsolete. No loss. - The explicit
logger.info("Command: codesign --force --sign ... \(mainExecutablePath)")line, however, was genuinely useful for diagnosing signing failures and is gone. The sign process's stdout/stderr is still logged, so you're not blind, but you lose the exact invocation.
Given this PR exists because of a hard-to-reproduce signing edge case, keeping that diagnostic has real value. Suggest restoring an equivalent line before signProcess.run():
logger.info("Command: codesign --force --options runtime --sign \(signingIdentity) --entitlements \(entitlementsPath) \(tempExecutable.path)")
a5ehren
left a comment
a5ehren
left a comment
There was a problem hiding this comment.
A few additional findings from a fresh review pass, alongside the replies on netniV's threads. None are blocking on their own, but I'd want answers on the .error fall-through change and the unsigned-path consent divergence before merge.
| UserDefaults.standard.set(false, forKey: "forceEntitlementReapplication") | ||
| } | ||
| return | ||
| case .unsigned: |
There was a problem hiding this comment.
Behavioral divergence worth confirming is intentional: the .unsigned path calls applyEntitlementsDirectly directly, skipping the requestGameBundleAccess file picker that the .missing path uses for explicit user consent. Since the launcher is not sandboxed (app-sandbox = false in macOSLauncher.entitlements), file access still works via the App Management TCC grant, so this isn't a correctness bug — but one path asks "may I modify your game binary?" and the other silently rewrites it. Note the doc comments on applyEntitlementsDirectly/executeCodesignDirectly still say "Must be called while security-scoped resource access is active", which this caller now violates. Either route unsigned through the same consent flow or relax those comments.
| case .missing: | ||
| break | ||
| case .error: | ||
| throw EntitlementError.entitlementVerificationFailed |
There was a problem hiding this comment.
Behavior change to confirm: previously any falsy checkEntitlements result (codesign failure, empty output, parse failure) fell through to the re-signing path. Now .error throws entitlementVerificationFailed immediately and never attempts the file-picker fallback. This is arguably more correct (don't modify the binary when you couldn't even read entitlements), but it's a real change for transient codesign failures the old code would have recovered from. Intentional?
|
|
||
| // Verify the entitlements were applied successfully to the main executable | ||
| guard checkEntitlements(appPath: gamePath, expectedEntitlements: loaderEntitlements) else { | ||
| guard case .valid = checkEntitlements(appPath: gamePath, expectedEntitlements: loaderEntitlements) else { |
There was a problem hiding this comment.
Indentation glitch: this guard is over-indented relative to the function body. No SwiftLint/swift-format config exists in macos-launcher/, so CI won't catch it — worth fixing by hand for consistency.
| let entitlementsDict = plist as? [String: Any] else { | ||
| logger.error("Failed to parse entitlements plist") | ||
| return false | ||
| return .error |
There was a problem hiding this comment.
Indentation glitch: return .error is over-indented compared to the surrounding block. Align with the other statements in this guard body.
|
|
||
| do { | ||
| let executableURL = URL(fileURLWithPath: mainExecutablePath) | ||
| try fileManager.replaceItemAt(executableURL, withItemAt: tempExecutable) |
There was a problem hiding this comment.
replaceItemAt (without .usingNewMetadataOnly) is what actually backs the PR's "preserve original permissions" claim — it preserves the destination's metadata. That's load-bearing and easy to silently break later by adding the wrong option. Worth a one-line comment here noting the intent so a future change doesn't regress it.
|
If you can address these @wycats we can look at getting this merged in. |
This handles a macOS launcher case where the game executable is unsigned.
Why:
code object is not signed at all.codesignwalked the app bundle and failed on an invalid nestedNativeScript.bundle.Changes:
Validation:
Game executable is unsigned, thenSuccessfully applied loader entitlements to unsigned game executable../scripts/mac-build-test-debug.sh -m release buildI split this from the local build-script PR so the signing behavior can be reviewed independently.