fix(ci): safeguard against trailing whitespace in build secrets#29151
fix(ci): safeguard against trailing whitespace in build secrets#29151tommasini wants to merge 3 commits into
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
Why no E2E tests are needed:
Why no performance tests are needed:
Performance Test Selection: |
|
|
||
| if (typeof rawValue === 'string' && value.length !== rawValue.length) { | ||
| console.warn( | ||
| `⚠ Trimmed whitespace from ${secretName} (len ${rawValue.length} -> ${value.length}) while mapping to ${envVar}. Re-save the CI secret without a trailing newline to silence this warning.`, |
|
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until this PR meets the canonical Definition of Ready For Review in `docs/readme/ready-for-review.md`. In short: the template must be materially complete (not just section titles present), all status checks must be currently passing, and the only expected follow-up commits must be reviewer-driven. --> ## **Description** Cherry pick: #29151 <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** <!-- Every checklist item must be consciously assessed before marking this PR as "Ready for review". A checked box means you deliberately considered that responsibility, not that you literally performed every action listed. Unchecked boxes are ambiguous: they are not an implicit "N/A" and they are not a silent "skip". See `docs/readme/ready-for-review.md` for the full checklist semantics. --> - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I've included tests if applicable - [ ] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. #### Performance checks (if applicable) - [ ] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [ ] I've tested with a power user scenario - Use these [power-user SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [ ] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **Pre-merge reviewer checklist** <!-- Reviewer checklist items follow the same semantics as the author checklist: an unchecked box is ambiguous, a checked box means the reviewer consciously assessed that responsibility. See `docs/readme/ready-for-review.md`. --> - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches CI secret injection and env-var remapping used during builds; a mistake could break build pipelines or alter expected secret values, though changes are limited to trimming surrounding whitespace and adding validation. > > **Overview** > Build workflows now **defensively strip leading/trailing whitespace from CI secrets** before exporting them to the build environment, and warn when trimming occurs. > > GitHub Actions builds additionally **fail fast if any mapped env var still contains surrounding whitespace** via a new `scripts/check-env-whitespace.js` step after `set-secrets-from-config.js`, reducing the chance of shipping binaries with broken OAuth/API keys caused by trailing newlines. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 216761d. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->



Description
Reason for the change. Several GitHub repository secrets (e.g.
IOS_GOOGLE_CLIENT_ID_PROD,ANDROID_GOOGLE_CLIENT_ID_PROD,ANDROID_APPLE_CLIENT_ID) were stored with a stray trailing newline. Becausebabel-plugin-transform-inline-environment-variablesinlines env vars into the JS bundle as literal strings, the\nwas baked into the shipped binary and silently broke OAuth on production — Apple SSO on Android surfaced this with the following sapphire auth error:Re-saving each affected secret manually fixes the symptom but not the class of bug: the CI pipeline should not allow whitespace-polluted secrets to reach the build at all.
Solution — two defensive layers:
Trim at the injection chokepoints. Every secret mapped via
builds.ymlnow goes through a defensive.trim()before it's written to$GITHUB_ENV(GitHub Actions path, inscripts/set-secrets-from-config.js) orexport-ed (Bitrise legacy path, inscripts/build.sh::remapEnvVariable). Only leading/trailing whitespace is stripped, so line-wrapped base64 payloads (GOOGLE_SERVICES_B64_*) keep their interior newlines —base64 -dtolerates those. A warning is logged whenever a trim actually changes the value, identifying the affected secret by name and length delta (never the value).Fail-fast validator after injection. A new script,
scripts/check-env-whitespace.js, iterates theCONFIG_SECRETSmapping afterset-secrets-from-config.jsruns and fails the build if any of those env vars still have leading/trailing whitespace. This guards against:${{ secrets.X }}directly.The validator is wired into both workflows that inject secrets from
builds.yml:.github/workflows/build.yml(native prod/beta/dev/QA/RC/E2E builds) and.github/workflows/push-eas-update.yml(OTA updates).Net effect. The next native build and the next OTA update produce clean, whitespace-free OAuth client IDs / URLs / API keys, restoring Apple & Google SSO in production. A short cleanup task remains for whoever owns the GitHub secrets: re-save any secret flagged by the new
⚠ Trimmed whitespace from ...warning in theSet secretsstep, so the warnings go away for good. Until then, the trim keeps the build correct.Changelog
CHANGELOG entry: null
Related issues
Fixes:
No issue: incident fix for a whitespace footgun in CI secret injection that was breaking Apple/Google SSO in production builds.
Manual testing steps
Screenshots/Recordings
Before
N/A — changes are CI-only and have no UI impact. The end-user symptom (Apple/Google SSO failures caused by the
\n) is described in the Description section.After
N/A — changes are CI-only and have no UI impact.
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist
Note
Medium Risk
Touches CI secret injection and validation paths; mistakes could cause unexpected build failures or subtly alter secret values (though only leading/trailing whitespace is affected).
Overview
Prevents trailing/leading whitespace in CI-injected secrets from being baked into shipped builds by trimming secrets at injection time and warning when a value was modified.
Adds a fail-fast validation step (
scripts/check-env-whitespace.js) to both the native build and OTA update GitHub Actions workflows to abort if anybuilds.yml-mapped env var still contains surrounding whitespace, and applies the same defensive trimming in the legacy Bitrisescripts/build.shenv remapping path.Reviewed by Cursor Bugbot for commit 0297c8d. Bugbot is set up for automated code reviews on this repo. Configure here.