Add macos_verify_code_signing action#757
Conversation
Extracted from the `verify_code_signing` lane in the WordPress Contributor Toolkit (WordPress/experimental-wp-dev-env#22), so that the other apps we build for macOS can assert their artifacts are signed and notarized instead of shipping a silently unsigned build. `electron-builder` only warns when it can't find a valid signing identity — it skips signing and produces an artifact that looks fine until users try to launch it. That's what motivated the original lane. On top of the original lane's checks, this also runs `xcrun stapler validate` so a missing notarization ticket fails the job, and makes the notarization checks opt-out for callers that verify at a point where the app is signed but not yet notarized, such as an `electron-builder` `afterSign` hook. Part of AINFRA-2709. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>
--- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>
Disk images can't reuse the app bundle's checks. Measured against two real `.dmg`s, including one of our own electron-builder artifacts: neither is codesigned — electron-builder signs only the app inside — and Gatekeeper rejects an unsigned image with `no usable signature` even when it does carry a notarization ticket. So `codesign --verify` and `spctl` are the wrong checks for our images; the stapled ticket is the only one that means anything. Hence dispatch on the extension rather than a separate action per artifact type: callers pass paths and get the checks that apply. An unsigned image warns and skips its signature checks, while a *broken* signature still fails — a distinction worth keeping, since the first is expected and the second never is. `app_path` becomes `artifact_path` now that it takes more than app bundles. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>
The check claimed to accept "an Array of Strings" but only checked for an Array, so `[42]` reached `File.exist?` and raised a bare TypeError instead of the message the check exists to produce. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
`sh` logs a non-zero exit in red whether or not the caller handles it, so the unsigned-image path painted a passing job's log with what looks like a failure. The explanation came after, by which point a reader skimming for red has already stopped. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable Fastlane action to verify that macOS build artifacts are properly code signed and (optionally) notarized, addressing cases where tooling like electron-builder may silently skip signing and produce unusable artifacts.
Changes:
- Added
macos_verify_code_signingFastlane action to verify signature validity, optional signing authority, Gatekeeper acceptance, and stapled notarization tickets for.appand.dmgartifacts. - Added comprehensive RSpec coverage for the action’s behavior across signed/unsigned images, authority matching, and failure modes.
- Documented the new action in
CHANGELOG.mdunder## Trunk.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/fastlane/plugin/wpmreleasetoolkit/actions/macos/macos_verify_code_signing.rb |
New action implementing macOS artifact signature / Gatekeeper / stapler validation logic. |
spec/macos_verify_code_signing_spec.rb |
New unit tests validating the action’s command invocations and error handling. |
CHANGELOG.md |
Adds a ## Trunk “New Features” entry for the new action. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
AliSoftware
left a comment
There was a problem hiding this comment.
Left some nitpicks / cosmetic feedbacks / questions / suggestions, but none of them are blockers (except maybe the question on the action name if we want to later support iOS but don't want to have to rename?), so approving to unblock you.
| FastlaneCore::ConfigItem.new( | ||
| key: :artifact_path, | ||
| description: 'The path, or list of paths, to the `.app` bundle(s) or `.dmg` disk image(s) to verify', | ||
| is_string: false, |
There was a problem hiding this comment.
is_string: us deprecated. Can't we use something like type: Array instead? 🤔
(iirc, if you pass a String to a ConfigItem expecting an Array, then fastlane automatically wraps that string into a single-item array for you? To be double-checked with live testing that theory though)
| description: 'The signing authority the artifact is expected to be signed by, e.g. `Developer ID Application: ACME, Inc. (ABCDE12345)`. ' \ | ||
| + 'When omitted, any valid signature is accepted', |
There was a problem hiding this comment.
💡 It would be nice to provide some guidance in that description on how to get that signing authority (e.g. provide a command to run on a certificate to extract and print that value, or how to get it from your Xcode project, or whatever)
| case File.extname(path).downcase | ||
| when '.app' | ||
| verify_app_bundle(path: path, expected_authority: params[:expected_authority], verify_notarization: params[:verify_notarization]) | ||
| when '.dmg' | ||
| verify_disk_image(path: path, expected_authority: params[:expected_authority], verify_notarization: params[:verify_notarization]) | ||
| else | ||
| UI.user_error!("Don't know how to verify #{path}. Supported artifacts are `.app` bundles and `.dmg` disk images") | ||
| end |
There was a problem hiding this comment.
💡 Should we also support .ipa files and iOS artifacts?
I know you named that action macos/macos_verify_code_signing.rb / MacosVerifyCodeSigningAction, and that the main use case we have today is for Electron macOS apps, but I figured since the main thing it does it call codesign --verify with the proper arguments, maybe it could be useful to support .ipa iOS products as well? 🤔
I know that's not a super strong argument, but that would also avoid the strange case of if we don't rename that action but later need to support iOS/.ipa too after all, the macos_ action name would be awkward to keep and we'd have to either rename the action or implement that is a separate ios_verify_code_signing action with very similar logic 🙃
Anyway, probably not that important, but I still figured I'd leave this as food for thoughts.
There was a problem hiding this comment.
On the one hand, Apple code signing is rather similar between platforms, so it makes sense to have a generic action that can handle any mix bag of Apple binaries. On the other, the presence of notarization in macOS represent a substantial difference. An API that allows to check code signing for an .ipa with verify_notarization: true would feel odd, even if the action would print an helpful log such as "Ignoring notarization on IPA input".
If we'll feel the need for an iOS counterpart, I think we could address the code duplication concern by extracting helper objects.
| # Distinguishes an artifact that carries no signature at all from one whose signature is | ||
| # broken: the former is expected for a disk image, the latter always a failure. | ||
| # | ||
| def self.signed?(path) |
There was a problem hiding this comment.
💭 The name of this helper makes sense on its own, but looking at the rest of the actions' code I see other places where we also call codesign --verify … to check a signature (e.g. line 32 in verify_app_bundle). which made me wonder why that line didn't also call signed? and what was the difference between those two call sites.
❓ I can see there are subtle differences between L32 and here indeed, but I wonder it would be worth it to make the call site on L32 rely on this helper rather than itself also passing the codesign … command and arguments? 🤔 💭
There was a problem hiding this comment.
Addressed via fd9fe78 adding a deep parameter.
| UI.user_error!("The code signature of #{path} is not valid:\n#{output}") | ||
| end | ||
|
|
||
| def self.verify!(error_message, *command) |
There was a problem hiding this comment.
💄 Pure style nitpick:
While I like the Ruby splat operator and its ability to make a function take arbitrary parameters and convert them into an array for you, in this particular case when I'm looking at the call sites for verify! in this code (e.g. verify!("#{path} was rejected by Gatekeeper", 'spctl', '--assess', '--type', 'execute', '--verbose=2', path)) for some reason it feels a bit weird to me.
I think it's a combination of having those *command parameters being mixed inline with the error_message parameter + the fact that neither of the error_message nor the *command have a parameter label / is a keyword param.
So instead I think one of those alternatives would read better:
verify!('spctl', '--assess', '--type', 'execute', '--verbose=2', path, error_message: "#{path} was rejected by Gatekeeper")
verify!("#{path} was rejected by Gatekeeper", command: ['spctl', '--assess', '--type', 'execute', '--verbose=2', path])
verify!(
command: ['spctl', '--assess', '--type', 'execute', '--verbose=2', path],
error_message: "#{path} was rejected by Gatekeeper"
)wdyt?
There was a problem hiding this comment.
Good point. The error message up front followed by all the arguments felt iffy to me too...
"verify" as a verb for what the method does is confusing, also. In the end, it's just a convenience wrapper for
sh(*command, error_callback: ->(_) { UI.user_error!(error_message) })I think maybe a combination of your suggestion to explicitly pass command: and error_message: plus a rename, run! or execute!?, would make the code read better.
Given this is an internal refactor and that I'm keep to ship the action to then use it officially in Automattic/wp-calypso#112767, I'm going to punt working on this in a follow up, though. Thanks!
There was a problem hiding this comment.
Opus 4.8 input on the naming:
Thread context read. verify! is just sh with a custom failure message; every call site is an assertion. Here's a draft reply:
Some options for the follow-up, roughly in increasing order of how much they change:
1. Name it after the mechanism — it's
shwith a custom failure message, nothing more:sh_or_fail!(command: ['xcrun', 'stapler', 'validate', path], error_message: "#{path} has no notarization ticket stapled to it") run!(command: [...], error_message: "...") execute!(command: [...], error_message: "...")
sh_or_fail!is my pick of the three:run!/execute!don't say what the!is for, and plainshalready raises — the only thing this wrapper adds is the message.2. Name it after what the call sites actually do — assert:
assert_command!(command: [...], error_message: "...") fail_unless!(command: [...], message: "#{path} was rejected by Gatekeeper")
fail_unless!reads as a sentence at the call site: fail unless this command succeeds.3. Drop the generic helper entirely and give each check its own method:
assert_gatekeeper_accepts!(path: path, type: 'execute') assert_notarization_stapled!(path: path)The message stops being a parameter — it lives next to the command it describes — and the ordering/labelling problem disappears. Downside is two more small methods, but they'd sit right alongside
verify_authority!, which is already shaped this way.I'd go 3 for the two
stapler/spctlchecks and keep something likesh_or_fail!around only if a third caller shows up.
Option 3, drop generic helper entirely is attractive because a method named can be clearer than an array of command parameters. It doesn't solve the repetition of the sh... UI.user_error!... pattern, but that's a small price to pay and could anyway be addressed anyway with one of our suggestions above, or, my favorite at this time, fail_unless!(command: [...], failure_message: ...).
Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>
Review feedback: two call sites each spelled out their own `codesign --verify` invocation, leaving the reader to work out how they differed. The helper now owns the command, and the difference lives at the call site instead: an unsigned disk image is tolerated, an unsigned app bundle is not. Failures on an app bundle now carry the `codesign` output in the error message, which the previous fixed string dropped. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review feedback: `is_string:` is deprecated. `type: Array` replaces both it and most of the `verify_block`, and fastlane converts a String into an array for us, so a single path still works — it splits on commas, which means a path containing one would be read as two. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What does it do?
Extracts the
verify_code_signinglane written for the WordPress Contributor Toolkit (WordPress/experimental-wp-dev-env#22) into a reusable action, so every app we build for macOS can assert its artifacts are signed and notarized.electron-builder, in particular, only warns when it can't find a valid signing identity — it skips signing and produces an artifact that looks fine until a user tries to launch it.Two deliberate additions over the original lane:
xcrun stapler validate, so a missing notarization ticket fails the job (thanks @iangmaia for the suggestion).verify_notarization(defaulttrue), so callers can verify at a point where the app is signed but not yet notarized — e.g. from anelectron-builderafterSignhook.See https://linear.app/a8c/issue/AINFRA-2709.
How to test
bundle exec rspec spec/macos_verify_code_signing_spec.rb.I also smoke-ran it against a real notarized app, which exercises the actual
codesign/spctl/staplerinvocations the specs mock:It passes, and fails as expected when given a different
expected_authority.Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendations.specs/*_spec.rb) if applicable.bundle exec rspecto run the whole test suite and ensure all your tests pass.CHANGELOG.mdfile to describe your changes under the appropriate existing###subsection of the existing## Trunksection.MIGRATION.mdfile to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.