Skip to content

fix(ci): safeguard against trailing whitespace in build secrets#29151

Closed
tommasini wants to merge 3 commits into
mainfrom
fix/safeguard-against-whitespaces-secrets
Closed

fix(ci): safeguard against trailing whitespace in build secrets#29151
tommasini wants to merge 3 commits into
mainfrom
fix/safeguard-against-whitespaces-secrets

Conversation

@tommasini

@tommasini tommasini commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

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. Because babel-plugin-transform-inline-environment-variables inlines env vars into the JS bundle as literal strings, the \n was baked into the shipped binary and silently broke OAuth on production — Apple SSO on Android surfaced this with the following sapphire auth error:

could not validate field client_id, token io.metamask.appleloginclient.prod
, expected io.metamask.appleloginclient.prod

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:

  1. Trim at the injection chokepoints. Every secret mapped via builds.yml now goes through a defensive .trim() before it's written to $GITHUB_ENV (GitHub Actions path, in scripts/set-secrets-from-config.js) or export-ed (Bitrise legacy path, in scripts/build.sh::remapEnvVariable). Only leading/trailing whitespace is stripped, so line-wrapped base64 payloads (GOOGLE_SERVICES_B64_*) keep their interior newlines — base64 -d tolerates those. A warning is logged whenever a trim actually changes the value, identifying the affected secret by name and length delta (never the value).

  2. Fail-fast validator after injection. A new script, scripts/check-env-whitespace.js, iterates the CONFIG_SECRETS mapping after set-secrets-from-config.js runs and fails the build if any of those env vars still have leading/trailing whitespace. This guards against:

    • future regressions in the trim logic, and
    • new workflow steps that bypass the chokepoint by reading ${{ 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 the Set secrets step, 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

Feature: CI safeguard against whitespace in build secrets

  Scenario: Clean secret passes through unchanged
    Given a GitHub secret stored without trailing whitespace
    When the "Set secrets" step of the build workflow runs
    Then scripts/set-secrets-from-config.js writes the value verbatim to $GITHUB_ENV
    And scripts/check-env-whitespace.js reports "✅ All N env var(s) mapped from builds.yml secrets are whitespace-clean."
    And the build continues to the next step

  Scenario: Secret stored with a trailing newline is sanitized
    Given a GitHub secret stored with a trailing "\n"
    When the "Set secrets" step of the build workflow runs
    Then scripts/set-secrets-from-config.js logs "⚠ Trimmed whitespace from <SECRET_NAME> (len X -> Y) while mapping to <ENV_VAR>..."
    And the trimmed value (without the "\n") is written to $GITHUB_ENV
    And scripts/check-env-whitespace.js reports no whitespace issues
    And the build continues to the next step
    And the resulting JS bundle contains the OAuth client ID without a trailing "\n"

  Scenario: Regression or bypass causes whitespace to reach the build env
    Given a hypothetical workflow step that sets a builds.yml-mapped env var directly from ${{ secrets.X }} without trimming
    And that secret contains a trailing "\n"
    When scripts/check-env-whitespace.js runs
    Then it logs "❌ N environment variable(s) have leading/trailing whitespace after secret application:"
    And it lists the offending env vars with length deltas (never the values)
    And the step exits with code 1, failing the build before any binary is produced

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)

  • 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 to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

Pre-merge reviewer checklist

  • 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.

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 any builds.yml-mapped env var still contains surrounding whitespace, and applies the same defensive trimming in the legacy Bitrise scripts/build.sh env remapping path.

Reviewed by Cursor Bugbot for commit 0297c8d. Bugbot is set up for automated code reviews on this repo. Configure here.

@tommasini tommasini self-assigned this Apr 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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.

@metamaskbotv2 metamaskbotv2 Bot added the team-mobile-platform Mobile Platform team label Apr 22, 2026
@github-project-automation github-project-automation Bot moved this to Needs dev review in PR review queue Apr 22, 2026
@tommasini tommasini marked this pull request as ready for review April 22, 2026 06:46
@tommasini tommasini requested review from a team as code owners April 22, 2026 06:46
@github-actions github-actions Bot added the risk-low Low testing needed · Low bug introduction risk label Apr 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: None (no tests recommended)
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: low
  • AI Confidence: 97%
click to see 🤖 AI reasoning details

E2E Test Selection:
All 5 changed files are purely CI/build infrastructure changes with no impact on application code:

  1. scripts/build.sh: Added defensive whitespace trimming in remapEnvVariable() — a build-time shell function that sanitizes environment variable values before export. No app logic affected.

  2. scripts/set-secrets-from-config.js: Enhanced secret mapping to trim whitespace from GitHub secrets before writing to GITHUB_ENV. Adds warnings for whitespace-only values. Pure CI tooling.

  3. scripts/check-env-whitespace.js (new): A new validation script that verifies mapped env vars are whitespace-clean after set-secrets-from-config.js runs. Exits 1 if dirty secrets are found, preventing bad builds from proceeding.

  4. .github/workflows/build.yml: Added a single new step "Check for whitespace in applied secrets" that runs check-env-whitespace.js after secret application. No changes to test execution steps, build matrix, or artifact handling.

  5. .github/workflows/push-eas-update.yml: Same new validation step added to the EAS update workflow.

Why no E2E tests are needed:

  • Zero application source code changes — no UI components, controllers, navigation, state management, or user flows are touched.
  • The changes are purely defensive CI hygiene: preventing secrets with trailing newlines from being baked into the JS bundle.
  • These changes do not affect any E2E test scenarios, test infrastructure (Detox, fixtures, page objects), or test execution pipelines.
  • If the whitespace check fails, the build fails before tests even run — which is the intended behavior and self-validating.
  • No shared components (BrowserTab, TabBar, Modals, Confirmations) are affected.

Why no performance tests are needed:

  • No UI rendering, data loading, state management, or critical user flow code is changed.
  • Build scripts and CI workflows have no runtime performance impact on the app.

Performance Test Selection:
No application code changes that could affect runtime performance. All changes are CI build scripts and GitHub Actions workflow steps for secret sanitization. These have zero impact on app startup, rendering, data loading, or any user-facing performance metrics.

View GitHub Actions results


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.`,
@tommasini tommasini mentioned this pull request Apr 22, 2026
10 tasks
@sonarqubecloud

Copy link
Copy Markdown

tommasini added a commit that referenced this pull request Apr 22, 2026
<!--
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 -->
@tommasini tommasini changed the title fix(ci): safeguard against trailing whitespace in build secrets fix(ci): safeguard against trailing whitespace in build secrets cp-7.74.0 Apr 22, 2026
@tommasini tommasini changed the title fix(ci): safeguard against trailing whitespace in build secrets cp-7.74.0 fix(ci): safeguard against trailing whitespace in build secrets Apr 23, 2026
@tommasini tommasini closed this Apr 23, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

risk-low Low testing needed · Low bug introduction risk size-M team-mobile-platform Mobile Platform team

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants