Fail release builds with empty OAuth secret#40
Open
mokagio wants to merge 3 commits into
Open
Conversation
CI's `make release` had no guard against a missing `WPCOM_OAUTH_CLIENT_SECRET` env var: the Makefile recipe wrote whatever `$secret` ended up being (possibly empty) into `Info.plist` and signed the bundle, so a misconfigured agent could ship a notarized artifact that authenticates as nobody. Extract the post-build check into `Tools/verify-oauth-secret.sh` and hook it into `notarize-app` so every release path (`make zip`, `make release`, `Tools/manual-release.sh`) runs the same verification, after build and before the notary submission. The inline check that previously lived in `manual-release.sh` had a latent bug: `plutil -extract ... raw` appends a trailing newline, so `wc -c` returned 1 for the empty-string case and the guard never fired. The new script uses command substitution + `[ -z ]`, which strips the newline and behaves correctly for both empty-string and missing-key cases. See #13 (comment). --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a shared, post-build verification step to ensure release/packaging builds fail if the WordPress.com OAuth client secret wasn’t injected into the app bundle, preventing signed/notarized artifacts that cannot authenticate.
Changes:
- Added
Tools/verify-oauth-secret.shto validateWPCOMOAuthClientSecretin the built app’sInfo.plist. - Updated
Tools/manual-release.shto call the new verifier instead of duplicating the check inline. - Updated
Makefileto add averify-oauth-secrettarget and makenotarize-appdepend on it so all artifact-producing paths run the check.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tools/verify-oauth-secret.sh | New script to validate the OAuth secret exists and is non-empty in the built app bundle. |
| Tools/manual-release.sh | Replaces inline secret validation with a call to the shared verifier script. |
| Makefile | Adds verify-oauth-secret target and wires it into the notarization/release chain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+31
to
+35
| secret=$(plutil -extract WPCOMOAuthClientSecret raw -o - "$PLIST" 2>/dev/null || true) | ||
| if [ -z "$secret" ]; then | ||
| echo "Error: $APP_BUNDLE is missing WPCOMOAuthClientSecret in Info.plist." >&2 | ||
| echo "Set WPCOM_OAUTH_CLIENT_SECRET or pass WPCOM_OAUTH_CLIENT_SECRET_FILE before building." >&2 | ||
| exit 1 |
Contributor
Author
There was a problem hiding this comment.
Good point — confirmed WPCOMClient.swift trims clientSecret with .whitespacesAndNewlines, so a whitespace-only value would authenticate as nobody just like an empty one. Fixed in 12eca98: the check now strips all whitespace before the -z test, so empty and whitespace-only values both fail. Verified against empty, whitespace-only, and real-value fixtures.
Posted by Claude (Opus 4.7) on behalf of @mokagio with approval.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The app trims `WPCOMOAuthClientSecret` with `.whitespacesAndNewlines` when reading it (`WPCOMClient.swift` `clientSecret`), so a whitespace-only plist value authenticates as nobody just like an empty one. Strip all whitespace before the emptiness test so the guard catches that case too. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Rationale
In #13's review, @iangmaia flagged that the Makefile recipe injecting
WPCOMOAuthClientSecrethad no validation: ifWPCOM_OAUTH_CLIENT_SECRETwas unset (or its file path empty) theplutil -replacecall still ran with an empty string, so the build could happily produce a signed-and-notarized artifact that authenticates as nobody. CI's.buildkite/commands/build.shinvokesmake releasedirectly, so the upfront env check inTools/manual-release.shdidn't cover it.This PR extracts the post-build verification into
Tools/verify-oauth-secret.shand makesnotarize-appdepend on it, so every artifact-producing path (make zip,make release,Tools/manual-release.sh) runs the same check. The defaultmaketarget stays untouched — dev iteration without a secret keeps working.Gotcha
The inline check that used to live in
manual-release.shhad a latent bug:plutil -extract … rawemits a trailing newline, sowc -creturned1for the empty case and the guard never fired. The new script usessecret=$(plutil …)+[ -z "$secret" ]; command substitution strips the newline, so both the empty-string and missing-key cases trip the check. See the commit body for more.How to test
From a clean tree (in the worktree):
make --dry-run releasewill showTools/verify-oauth-secret.shrunning after the build and beforeTools/notarize.sh, so a missing secret fails fast (no wasted notary-service round-trip).CI on this branch exercises the happy path.