Conversation
|
|
|
|
b210b1c to
97122d3
Compare
| isCi = System.getenv('CI')?.toBoolean() ?: false | ||
|
|
||
| secretPath = "${System.getProperty("user.home")}/.configure/wordpress-android/secrets/secrets.properties" | ||
| decryptedSecretsDir = file("${System.getProperty("user.home")}/.configure/wordpress-android/secrets") |
There was a problem hiding this comment.
This will become a8c-secrets ad hoc project's folder once we adopt the tool.
There was a problem hiding this comment.
Pull request overview
Updates the Android signing setup so decrypted keystores live alongside secrets.properties in the out-of-repo ~/.configure/wordpress-android/secrets directory, avoiding sensitive keystore files being written into the git checkout.
Changes:
- Switch release signing to use
~/.configure/wordpress-android/secrets/upload.keystore(when present + credentials exist). - Switch shared debug signing to use
~/.configure/wordpress-android/secrets/debug.keystore(when present). - Introduce a shared
decryptedSecretsDirsetting and update.configurecopy destinations to match the new keystore locations/names.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
WordPress/build.gradle |
Reads release/debug keystores from the decrypted secrets directory and gates signing on their presence. |
settings.gradle |
Adds decryptedSecretsDir and builds secretPath from it for consistent out-of-repo secrets resolution. |
.configure |
Renames and relocates keystore destinations to the out-of-repo secrets directory. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def uploadKeystore = new File(gradle.ext.decryptedSecretsDir, "upload.keystore") | ||
| def uploadCredentials = ["uploadStorePassword", "uploadKeyAlias", "uploadKeyPassword"] | ||
| if (uploadKeystore.exists() && uploadCredentials.every { gradle.ext.secretProperties.containsKey(it) }) { | ||
| logger.info("Upload keystore found, configuring signing for release builds.") | ||
| release { |
[DO NOT MERGE] Throwaway validation for #23261, closed once it has answered. Nothing in the PR pipeline exercises release signing: prototype builds assemble the `Debug` build type, lint and the manifest diffs never sign, and `release-builds.yml` and `beta-builds.yml` are API-triggered from release automation and check out a release branch rather than the PR. The first run that would notice a broken release signing config is the scheduled trunk-internal build, after merge. The debug side is worse than uncovered: when the shared keystore is absent AGP falls back to `~/.android/debug.keystore` and the build still goes green, so every prototype build so far has passed without proving the renamed `debug.keystore` was ever read. Both negative cases run against a throwaway `user.home` holding symlinks to the real credentials, so the agent's secrets directory is never mutated and no decrypted secret is copied anywhere. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
| logger.info("App signing properties found in secrets.properties, configuring signing for release builds.") | ||
| def uploadKeystore = new File(gradle.ext.decryptedSecretsDir, "upload.jks") | ||
| def uploadCredentials = ["uploadStorePassword", "uploadKeyAlias", "uploadKeyPassword"] | ||
| if (uploadKeystore.exists() && uploadCredentials.every { gradle.ext.secretProperties.containsKey(it) }) { |
There was a problem hiding this comment.
Perhaps release jobs should fail early when the upload key is missing?
Skipping signing makes sense for contributors without secrets, but with this change, a release build can succeed with an unsigned bundle and only fail later during upload, I think? 🤔
There was a problem hiding this comment.
Good catch — you're right, and that's exactly what happened: the release path inherited a condition that only makes sense for debug, so the build succeeded and produced an unsigned artifact that would only blow up at upload.
Fixed in be163b6. Release packaging now fails before it writes anything:
tasks.matching { it.name.startsWith("package") && it.name.contains("Release") }.configureEach {
doFirst {
if (!uploadKeyAvailable) {
throw new GradleException(...)
}
}
}Two things that shaped where the check lives:
- I first hung it off
validateSigning*, which reads like the natural hook — but AGP only creates that task for variants that already have a signing config, so it's absent in precisely the case we need to catch.package*Releaseis always there. - It covers both output paths. The APK signs inline in
packageWordpressRelease; for the AAB,packageJetpackReleaseBundleruns ahead ofsignJetpackReleaseBundle, so the guard still lands before either artifact exists.
The check reads a boolean captured at configuration time and runs in doFirst, mirroring the existing SentryCliExecTask guard in this file — that keeps it compatible with the configuration cache, which the repo has enabled.
Contributors are unaffected: assembleWordpressDebug's task graph contains no release packaging tasks, so a debug build without the secrets still configures and runs. I verified the four cases (missing key + release APK / release AAB / debug, and key present + release) against a stand-in project reproducing the same task names.
Posted by Claude (Opus 5) on behalf of @mokagio with approval.
There was a problem hiding this comment.
🟡 Changes recommended
One new Gradle line exceeds the repository’s mandatory 120-character limit.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| def uploadKeystore = new File(gradle.ext.decryptedSecretsDir, "upload.jks") | ||
| def uploadCredentials = ["uploadStorePassword", "uploadKeyAlias", "uploadKeyPassword"] | ||
| def uploadKeyAvailable = uploadKeystore.exists() && uploadCredentials.every { gradle.ext.secretProperties.containsKey(it) } |
Applies the convention from the out-of-repo Android keystores RFC: https://appsinfrap2.wordpress.com/2026/08/21/rfc-convention-for-out-of-repo-android-keystores/ The upload keystore was the last genuine secret still decrypted into the working tree. It now lands in the same out-of-repo secrets directory that already holds `secrets.properties`, so nothing sensitive sits inside the checkout for an editor, a backup, or an agent to pick up. Naming the directory `decryptedSecretsDir` is deliberate: it is the name the RFC's snippet uses for the `a8c-secrets` equivalent, so adopting `a8c-secrets` later only changes what the variable points at, not the signing config that reads it. Gating on the keystore file rather than on `uploadStoreFile` lets the path stop being a secret value — a filename the build script owns is not something `secrets.properties` needs to carry. `uploadStoreFile` and `debugStoreFile` are now unused and can be dropped from `mobile-secrets` separately. `.gitignore` keeps its `WordPress/*.jks` entries on purpose. Checkouts that ran `configure_apply` before this change still have a stale `WordPress/upload.jks` on disk, and un-ignoring it would make a real signing key committable. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
Renaming it to `.keystore` alongside the move out of the checkout was gratuitous: the file this PR relocates is already `automattic_upload.jks` in `mobile-secrets`, so keeping the extension makes the move the only change under review. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
Skipping signing is right for contributors without the secrets, but the same condition was applied to release, so a release build succeeded and handed back an unsigned artifact that only failed much later, at upload. Raised by @iangmaia in review. The guard hangs off the packaging tasks rather than `validateSigning*`, which AGP only creates for variants that already have a signing config — the case this needs to catch is exactly the one where that task is absent. `package*Release` covers both output paths: it signs inline for the APK, and for the AAB it runs ahead of `sign*ReleaseBundle`. Checked at execution time, and on the packaging tasks alone, so a contributor's debug build still configures and runs untouched. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
be163b6 to
a9e1fbb
Compare


Description
Closes AINFRA-2968.
The upload keystore now decrypts next to
secrets.properties, outside of the checkout, as described internally in paaHJt-akR-p2..gitignorestill ignoresWordPress/*.jksbecause pre-change checkouts still haveWordPress/upload.jkson disk.uploadStoreFileanddebugStoreFileare unused after this. Dropping them frommobile-secretsis a separate follow-up, so this PR stays revertible on its own.After merging, run
bundle exec fastlane run configure_applyand delete the staleWordPress/upload.jks.Testing instructions
Signing is not in the unit suite.
Compare
./gradlew :WordPress:signingReport.Without secrets:
wordpressRelease/jetpackRelease→Config: nonewordpressDebug→~/.android/debug.keystoreAfter
bundle exec fastlane run configure_apply:Config: release,Store:in the out-of-repo configure directory, not the repoAlias:matchesuploadKeyAliaswordpressDebug→ shareddebug.keystorefrom that directoryFor the record: I run all the check on my end.
PR CI does not exercise release signing.
Prototype builds on this PR did pick up the renamed debug keystore.
Also verified on a CI agent
Because no PR job exercises release signing, the same checks were run on a Buildkite agent from a throwaway stacked PR (#23267, now closed): build #28383, job "🔐 Validate signing config".
After
configure_applyon the agent:Two negative cases ran against a throwaway
user.homesymlinked to the real credentials, so the agent's secrets directory was never touched:Config: none. The keystore file, notsecrets.properties, is what gates signing.debug.keystore→ debug variants fall back to~/.android/debug.keystorewithError: Missing keystore. This is why a green prototype build was never on its own evidence that the shared key was read.Still unproven until the scheduled trunk-internal build: that
bundleReleasesigns and uploads with this keystore.signingReportshows the config resolves, not that the artifact ships.Section added by Claude (Opus 5) on behalf of @mokagio with approval. Re-run after the branch kept the
.jksextension; the earlier run was #23266 / build #28371.