fix(build): sign canonical framework binary once and sync to duplicates#1256
Conversation
Greptile SummaryFixes Apple notarization Confidence Score: 5/5Safe to merge — the core fix is correct for the PyInstaller Python.framework use case; remaining findings are P2 edge cases and a defensive hardening suggestion. No P0/P1 issues found. The sign-canonical-then-copy approach correctly resolves the inconsistent-signature notarization failure. The two P2 comments cover an unlikely edge case (distinct binaries in an ambiguous-bundle path) and a defensive cp -p suggestion. Neither represents a present defect in the target scenario. scripts/package/build_app_tauri.sh — review the edge-case overwrite concern at the duplicate-sync loop (lines 198–201).
|
| Filename | Overview |
|---|---|
| scripts/package/build_app_tauri.sh | Adds "sign canonical + copy to duplicates" fallback for ambiguous PyInstaller framework bundles; logic is correct for the described use case but silently corrupts non-canonical binaries if distinct Mach-O files trigger the same fallback path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["codesign $fw (bundle dir)"] -->|success| B["Signed bundle ✓"]
A -->|error| C{Error message?}
C -->|"bundle format is ambiguous"| D["find all Mach-O files in $fw\nsort lexicographically → fw_bins[]"]
C -->|other error| E["EXIT 1 (fatal)"]
D --> F{fw_bins empty?}
F -->|yes| G["ERROR: no Mach-O found → EXIT 1"]
F -->|no| H["canonical = fw_bins[0]"]
H --> I["Read existing codesign identifier\n(fallback: basename of canonical)"]
I --> J["mktemp → cp canonical → tmp\ncodesign --identifier ... tmp"]
J -->|failure| K["rm tmp → EXIT 1"]
J -->|success| L["cp tmp → canonical\nrm tmp"]
L --> M["for each fw_bins[1..]:\ncp canonical → duplicate"]
M --> N["Log: signed 1 + synced N duplicates ✓"]
Reviews (1): Last reviewed commit: "fix(build): sign canonical framework bin..." | Re-trigger Greptile
| for fw_bin in "${fw_bins[@]:1}"; do | ||
| echo " Syncing signed binary to duplicate path: $fw_bin" | ||
| cp "$canonical" "$fw_bin" || exit 1 | ||
| done |
There was a problem hiding this comment.
Overwriting distinct binaries if triggered for non-duplicate frameworks
The fallback copies the signed canonical binary over every other Mach-O found inside $fw. For the PyInstaller Python.framework case this is correct (all are duplicates), but if a different framework somehow triggers "bundle format is ambiguous" AND contains genuinely distinct Mach-O files (e.g., a library and a helper tool), every non-canonical binary would be silently replaced with the canonical's bytes. No error is produced, so the corruption would go undetected at signing time and only surface as a runtime crash or notarization rejection for the corrupted binary.
The PR description states distinct binaries "will each be signed separately," but the code does the opposite — it signs only the canonical and then overwrites the others. A content-equality guard (e.g., cmp -s "$canonical" "$fw_bin") before each copy would limit the blast radius and make the intent explicit:
for fw_bin in "${fw_bins[@]:1}"; do
if ! cmp -s "$canonical" "$fw_bin"; then
echo "WARNING: $fw_bin differs from canonical; signing separately" >&2
tmp2=$(mktemp)
cp "$fw_bin" "$tmp2"
codesign --force --options runtime --timestamp \
--entitlements "$ENTITLEMENTS" \
--sign "$APPLE_PERSONALID" \
"$tmp2" || { rm -f "$tmp2"; exit 1; }
cp "$tmp2" "$fw_bin" || { rm -f "$tmp2"; exit 1; }
rm -f "$tmp2"
else
echo " Syncing signed binary to duplicate path: $fw_bin"
cp "$canonical" "$fw_bin" || exit 1
fi
doneThere was a problem hiding this comment.
Fixed in 21b624d — added cmp -s guard before each copy: if the non-canonical binary differs from the canonical, it's now signed separately via its own temp-copy instead of being silently overwritten. The happy path (all duplicates identical) is unchanged.
There was a problem hiding this comment.
Addressed in 01c1233 — duplicates are now compared against an unsigned reference copy via cmp -s before syncing. Genuinely distinct binaries get their own signing pass.
| tmp_binary=$(mktemp) | ||
| cp "$canonical" "$tmp_binary" | ||
| codesign --force --options runtime --timestamp \ | ||
| --entitlements "$ENTITLEMENTS" \ | ||
| --identifier "$existing_id" \ | ||
| --sign "$APPLE_PERSONALID" \ | ||
| "$tmp_binary" || { rm -f "$tmp_binary"; exit 1; } | ||
| cp "$tmp_binary" "$canonical" || { rm -f "$tmp_binary"; exit 1; } | ||
| rm -f "$tmp_binary" |
There was a problem hiding this comment.
Temp file created with mode 0600 — execute bit not restored
mktemp creates the temp file with mode 0600 (no execute bit). cp "$canonical" "$tmp_binary" writes canonical's content into the already-existing temp inode, which preserves 0600. After signing, cp "$tmp_binary" "$canonical" writes into the existing canonical inode, so canonical's original permissions (typically 0755) are preserved — cp without -p retains the destination's mode when the destination already exists. However, this is subtly fragile: if cp on some macOS version ever unlinks-and-recreates the destination, the canonical would inherit 0600 and become non-executable. Using cp -p or explicitly chmod +x "$canonical" after the copy would make the intent robust.
| tmp_binary=$(mktemp) | |
| cp "$canonical" "$tmp_binary" | |
| codesign --force --options runtime --timestamp \ | |
| --entitlements "$ENTITLEMENTS" \ | |
| --identifier "$existing_id" \ | |
| --sign "$APPLE_PERSONALID" \ | |
| "$tmp_binary" || { rm -f "$tmp_binary"; exit 1; } | |
| cp "$tmp_binary" "$canonical" || { rm -f "$tmp_binary"; exit 1; } | |
| rm -f "$tmp_binary" | |
| cp -p "$canonical" "$tmp_binary" |
There was a problem hiding this comment.
Fixed in 21b624d — changed to cp -p so the temp file inherits the execute bit from the canonical binary, making the permission preservation explicit.
There was a problem hiding this comment.
Addressed in 01c1233 — all temp copies now use cp -p to explicitly preserve permissions from the source binary.
56cd932 to
21b624d
Compare
… copy - Use cp -p when copying canonical to temp file to preserve execute bits, making the behavior explicit rather than relying on destination inode mode. - Add cmp -s guard before overwriting each non-canonical path: if a binary differs from the canonical, sign it separately via temp copy instead of silently replacing it. This prevents silent corruption if a framework with genuinely distinct Mach-O files triggers the ambiguous-bundle fallback. Addresses Greptile P2 review findings on PR ActivityWatch#1256.
|
Addressed Greptile P2 findings in 21b624d (rebased onto master):
CI is green. Ready for merge. |
PyInstaller copies Python.framework contents as separate regular files — Python, Versions/Current/Python, and Versions/3.9/Python have identical content but different inodes. The previous approach signed each independently via temp copy, producing 3 different signature blocks (different timestamps, random nonces). Apple's notarization service detects these as inconsistently signed and reports: 'The signature of the binary is invalid.' for all three paths, even though each individual signature is technically valid. Fix: sign only the first (canonical) binary via temp copy, then copy the signed result to all duplicate paths. All three end up with byte-identical content including the embedded signature, so Apple's hash check passes consistently across every path.
… copy - Use cp -p when copying canonical to temp file to preserve execute bits, making the behavior explicit rather than relying on destination inode mode. - Add cmp -s guard before overwriting each non-canonical path: if a binary differs from the canonical, sign it separately via temp copy instead of silently replacing it. This prevents silent corruption if a framework with genuinely distinct Mach-O files triggers the ambiguous-bundle fallback. Addresses Greptile P2 review findings on PR ActivityWatch#1256.
21b624d to
b2ed942
Compare
|
Rebased onto current master (which now includes #1250/#1254/#1255) to resolve merge conflict. This commit replaces the sign-all-individually approach with sign-canonical-once-and-sync:
Both Greptile P2 findings are addressed in this version. |
|
macOS Build Tauri jobs passed on the rebased commit:
Linux/Windows jobs still running but don't touch Apple codesigning. PR is ready for merge when you're ready. |
…rk binary duplicates The cmp -s guard in ActivityWatch#1256 runs after signing the canonical binary, comparing signed bytes against unsigned duplicates — they always differ. This causes all three Python.framework copies to be signed separately with independent codesign invocations (different nonces/timestamps), producing inconsistent signature blocks that Apple rejects with 'The signature of the binary is invalid.' Fix: compute shasum of canonical BEFORE signing, then compare each duplicate's checksum against that pre-signing hash. Identical files (PyInstaller duplicate copies) are correctly detected and receive the byte-identical signed binary. Genuinely distinct binaries still fall through to the separate-signing path.
…rk binary duplicates (#1257) * fix(build): snapshot pre-signing checksum to correctly detect framework binary duplicates The cmp -s guard in #1256 runs after signing the canonical binary, comparing signed bytes against unsigned duplicates — they always differ. This causes all three Python.framework copies to be signed separately with independent codesign invocations (different nonces/timestamps), producing inconsistent signature blocks that Apple rejects with 'The signature of the binary is invalid.' Fix: compute shasum of canonical BEFORE signing, then compare each duplicate's checksum against that pre-signing hash. Identical files (PyInstaller duplicate copies) are correctly detected and receive the byte-identical signed binary. Genuinely distinct binaries still fall through to the separate-signing path. * fix(build): track synced/separately-signed counts accurately in summary log
Root Cause
Closes the Apple notarization
status: InvalidforPython.frameworkbinaries that has persisted across PRs #1250 → #1255.PyInstaller copies
Python.frameworkcontents as separate regular files —Python,Versions/Current/Python, andVersions/3.9/Pythonhave identical content but different inodes.The previous approach (PR #1254 / #1255) signed each independently via temp copy:
Apple's notarization service sees three files that should be the same binary (same code hash) but have different signature blocks (different timestamps/nonces in
__LINKEDIT). It reports:for all three paths — not because any individual signature is wrong, but because they're inconsistently signed copies of the same content.
Fix
Sign only the canonical (first sorted) binary via temp copy, then copy the signed result to all duplicate paths. All three end up with byte-identical content including the embedded signature:
Apple sees three files with identical hashes and one consistent valid signature. ✓
Notes
--identifierpreservation from PR fix(build): preserve binary identifier when signing via temp-path copy #1255 is kept (correct approach for the canonical binary)