chore: Validate envs and secrets during build time (MetaMask#29634)

Cal-L · web-flow · commit dedb5ddfdd1d · 2026-05-05T22:10:36.000Z
## **Description**  This safeguard is a follow up to catch malformed envs and secrets during build time to prevent issues similar to incident [1578](https://consensys.slack.com/archives/C0AUU2AT0C9/p1777390058735369). If any issues are detected during build time, the `Build Mobile App` workflow will fail with a message listing the offending value ## **Changelog**  CHANGELOG entry: ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/MCWP-564 ## **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**  ### **Before**  ### **After**  ## **Pre-merge author checklist**  - [ ] 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**  - [ ] 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** > Tightens CI validation by failing builds on previously-accepted env/secret values (e.g., trailing newlines/whitespace), which may block releases if any existing configuration relies on those formats. Risk is limited to build/CI scripts but impacts all build pipelines consuming `builds.yml` and GitHub secrets. > > **Overview** > Adds a shared `checkValue` validator for CI-injected configuration values, flagging missing/empty values plus common paste artifacts (leading/trailing whitespace, `\r`/CRLF, NUL/control chars, and zero-width Unicode) while ensuring error messages never include secret contents. > > Build-time checks are expanded to run this hygiene validation across `builds.yml` `env` entries (with a small allowlist for intentionally-empty keys) and across required secrets in `validate-secrets-from-config.js`, which now emits GitHub Actions `::error` annotations and reports all offenders in one pass. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 165a500. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>
diff --git a/scripts/lib/validate-value.js b/scripts/lib/validate-value.js
@@ -0,0 +1,187 @@
+/**
+ * Shared hygiene checks for CI-injected values (GitHub secrets and builds.yml env values).
+ *
+ * The goal is to catch operator mistakes (trailing newline, Windows line endings,
+ * invisible characters pasted from rich text editors, etc.) at build time, before
+ * malformed values end up baked into a production binary.
+ *
+ * Intentionally format-agnostic: it does not try to understand whether a value
+ * is a URL, base64 blob, JWT, etc. It only enforces generic hygiene.
+ *
+ * Usage:
+ *   const { checkValue } = require('./lib/validate-value');
+ *   const issues = checkValue('MM_SENTRY_DSN', value);
+ *   // issues is an array; empty => value is clean.
+ *
+ * Output contract: issue messages MUST NOT include the value itself or any
+ * substring of it. Only the length, offsets, and character code points are
+ * safe to surface.
+ */
+
+/* global Buffer */
+
+// eslint-disable-next-line no-misleading-character-class -- intentional set of invisible code points (ZWSP/ZWNJ/ZWJ/BOM)
+const ZERO_WIDTH_CHARS = /[\u200B\u200C\u200D\uFEFF]/;
+
+// C0/C1 control chars, excluding tab (\u0009), line feed (\u000A), and
+// carriage return (\u000D). CR is reported separately with a friendlier
+// message. LF is allowed mid-value for multi-line secrets (PEM keys, base64).
+/* eslint-disable no-control-regex -- intentionally matches control characters to flag them */
+const CONTROL_CHARS =
+  /[\u0001-\u0008\u000B\u000C\u000E-\u001F\u007F-\u009F]/;
+/* eslint-enable no-control-regex */
+
+function formatCodePoint(ch) {
+  return `U+${ch.charCodeAt(0).toString(16).padStart(4, '0').toUpperCase()}`;
+}
+
+/**
+ * @param {string} name - Identifier to report in violations (e.g. secret name or env key).
+ * @param {unknown} value - The value to check. Non-string values are coerced via String().
+ * @param {object} [options]
+ * @param {boolean} [options.allowEmpty=false] - If true, an empty string is not a violation.
+ *   Whitespace-only strings still fail even when allowEmpty is true (they indicate a typo).
+ * @returns {{ code: string, message: string }[]} - One entry per distinct violation; empty array means clean.
+ */
+function checkValue(name, value, options = {}) {
+  const { allowEmpty = false } = options;
+  const violations = [];
+
+  /**
+   * `missing`: the value is `undefined` or `null`. For secrets, this means the
+   * referenced GitHub Environment secret was never set (or the Environment
+   * itself is misconfigured). For YAML env entries, it usually means a key
+   * like `FOO:` was written with no value, which js-yaml parses as `null`.
+   * Short-circuits: nothing else can be checked without a value.
+   */
+  if (value === undefined || value === null) {
+    violations.push({
+      code: 'missing',
+      message: `${name}: value is null or not defined`,
+    });
+    return violations;
+  }
+
+  const str = String(value);
+  const len = Buffer.byteLength(str, 'utf8');
+
+  /**
+   * `empty`: the value is the empty string `""`. Skipped when the caller
+   * passes `{ allowEmpty: true }`, used for intentionally-empty YAML entries
+   * such as optional allowlists (e.g. `MM_PERPS_HIP3_ALLOWLIST_MARKETS: ''`).
+   * Short-circuits: the remaining checks don't apply to an empty string.
+   */
+  if (str === '') {
+    if (!allowEmpty) {
+      violations.push({
+        code: 'empty',
+        message: `${name}: value is an empty string`,
+      });
+    }
+    return violations;
+  }
+
+  /**
+   * `whitespace_only`: the value is non-empty but contains nothing except
+   * whitespace. Almost always a typo (e.g. someone pasted a single space
+   * into the GitHub Secret UI). Fails even with `allowEmpty: true`, because
+   * "intentionally empty" should be `""`, not `"   "`.
+   * Short-circuits: the value has no meaningful content to inspect further.
+   */
+  if (str.trim() === '') {
+    violations.push({
+      code: 'whitespace_only',
+      message: `${name}: value is whitespace-only (${len} bytes)`,
+    });
+    return violations;
+  }
+
+  /**
+   * `leading_whitespace`: the value begins with whitespace (space, tab, LF,
+   * etc.). Accidental leading whitespace breaks URL parsing, base64 decoding,
+   * and token comparisons — almost never intentional.
+   */
+  if (/^\s/.test(str)) {
+    violations.push({
+      code: 'leading_whitespace',
+      message: `${name}: value has leading whitespace (${len} bytes total)`,
+    });
+  }
+
+  /**
+   * `trailing_whitespace`: the value ends with whitespace. This is the
+   * single most common real-world paste mistake — e.g. copying a token from
+   * a terminal or editor ends up including the trailing `\n`. It's the
+   * specific failure mode this entire module was built to catch.
+   */
+  if (/\s$/.test(str)) {
+    violations.push({
+      code: 'trailing_whitespace',
+      message: `${name}: value has trailing whitespace (${len} bytes total); a trailing newline pasted from a terminal or editor is the most common cause`,
+    });
+  }
+
+  /**
+   * `carriage_return`: the value contains `\r` anywhere. CR is not part of
+   * the base64 alphabet, PEM uses LF, and no sanctioned secret format needs
+   * it — its presence is essentially always an artifact of Windows CRLF line
+   * endings surviving a copy-paste. Reported separately from `control_chars`
+   * so the remediation message can specifically call out Windows endings.
+   */
+  const crIndex = str.indexOf('\r');
+  if (crIndex !== -1) {
+    violations.push({
+      code: 'carriage_return',
+      message: `${name}: value contains a carriage return (\\r) at offset ${crIndex}/${len}; strip Windows line endings before saving the secret`,
+    });
+  }
+
+  /**
+   * `nul_byte`: the value contains `\u0000`. Never legitimate in any secret
+   * format we use. A NUL byte can terminate strings prematurely in C-based
+   * tooling and is a classic source of silent truncation bugs.
+   */
+  const nulIndex = str.indexOf('\u0000');
+  if (nulIndex !== -1) {
+    violations.push({
+      code: 'nul_byte',
+      message: `${name}: value contains a NUL byte at offset ${nulIndex}/${len}`,
+    });
+  }
+
+  /**
+   * `control_chars`: the value contains any other C0/C1 control character
+   * (range defined in CONTROL_CHARS — excludes tab, LF, and CR, which are
+   * allowed or reported separately). Hits here usually indicate non-text
+   * binary data was pasted as if it were a string, or a stray escape
+   * sequence. The message includes the specific code point (e.g. U+0007)
+   * so operators can identify what was pasted.
+   */
+  const ctrlMatch = CONTROL_CHARS.exec(str);
+  if (ctrlMatch) {
+    violations.push({
+      code: 'control_chars',
+      message: `${name}: value contains control character ${formatCodePoint(ctrlMatch[0])} at offset ${ctrlMatch.index}/${len}`,
+    });
+  }
+
+  /**
+   * `zero_width`: the value contains an invisible Unicode character —
+   * zero-width space (U+200B), zero-width non-joiner (U+200C), zero-width
+   * joiner (U+200D), or byte-order mark (U+FEFF). These are introduced when
+   * copying from rich-text sources (Google Docs, Slack, Notion, Confluence)
+   * and are invisible to the human eye but break exact-match comparisons
+   * and break formats like base64 that only accept a strict alphabet.
+   */
+  const zwMatch = ZERO_WIDTH_CHARS.exec(str);
+  if (zwMatch) {
+    violations.push({
+      code: 'zero_width',
+      message: `${name}: value contains invisible character ${formatCodePoint(zwMatch[0])} at offset ${zwMatch.index}/${len}; likely pasted from a rich text source`,
+    });
+  }
+
+  return violations;
+}
+
+module.exports = { checkValue };
diff --git a/scripts/lib/validate-value.test.js b/scripts/lib/validate-value.test.js
@@ -0,0 +1,145 @@
+const { checkValue } = require('./validate-value');
+
+const codes = (issues) => issues.map((i) => i.code);
+
+describe('checkValue', () => {
+  describe('happy path', () => {
+    it('returns no issues for a typical single-line secret', () => {
+      expect(checkValue('X', 'key_live_abc123xyz')).toEqual([]);
+    });
+
+    it('returns no issues for a multi-line base64 blob (embedded \\n mid-value is allowed)', () => {
+      const b64 = 'eyJhcGlLZXkiOiJhYmMifQ==\nsecond-line-content==';
+      expect(checkValue('GOOGLE_SERVICES_B64_IOS', b64)).toEqual([]);
+    });
+
+    it('coerces non-string values via String()', () => {
+      expect(checkValue('N', 42)).toEqual([]);
+      expect(checkValue('B', true)).toEqual([]);
+    });
+  });
+
+  describe('missing / empty', () => {
+    it('flags undefined', () => {
+      expect(codes(checkValue('X', undefined))).toEqual(['missing']);
+    });
+
+    it('flags null', () => {
+      expect(codes(checkValue('X', null))).toEqual(['missing']);
+    });
+
+    it('flags empty string by default', () => {
+      expect(codes(checkValue('X', ''))).toEqual(['empty']);
+    });
+
+    it('allows empty string when allowEmpty=true', () => {
+      expect(checkValue('X', '', { allowEmpty: true })).toEqual([]);
+    });
+
+    it('flags whitespace-only even when allowEmpty=true', () => {
+      expect(codes(checkValue('X', '   ', { allowEmpty: true }))).toEqual([
+        'whitespace_only',
+      ]);
+    });
+  });
+
+  describe('leading / trailing whitespace', () => {
+    it('flags trailing newline (the classic paste mistake)', () => {
+      expect(codes(checkValue('X', 'value\n'))).toEqual([
+        'trailing_whitespace',
+      ]);
+    });
+
+    it('flags trailing space', () => {
+      expect(codes(checkValue('X', 'value '))).toEqual([
+        'trailing_whitespace',
+      ]);
+    });
+
+    it('flags trailing tab', () => {
+      expect(codes(checkValue('X', 'value\t'))).toEqual([
+        'trailing_whitespace',
+      ]);
+    });
+
+    it('flags leading space', () => {
+      expect(codes(checkValue('X', ' value'))).toEqual([
+        'leading_whitespace',
+      ]);
+    });
+
+    it('flags both leading and trailing whitespace in one pass', () => {
+      expect(codes(checkValue('X', ' value '))).toEqual([
+        'leading_whitespace',
+        'trailing_whitespace',
+      ]);
+    });
+  });
+
+  describe('control characters', () => {
+    it('flags any \\r (Windows line endings)', () => {
+      expect(codes(checkValue('X', 'abc\r\ndef'))).toContain('carriage_return');
+    });
+
+    it('flags a standalone \\r mid-value', () => {
+      expect(codes(checkValue('X', 'abc\rdef'))).toContain('carriage_return');
+    });
+
+    it('flags NUL bytes', () => {
+      expect(codes(checkValue('X', 'abc\u0000def'))).toContain('nul_byte');
+    });
+
+    it('flags other C0 control characters', () => {
+      expect(codes(checkValue('X', 'abc\u0007def'))).toContain(
+        'control_chars',
+      );
+    });
+
+    it('flags DEL (U+007F)', () => {
+      expect(codes(checkValue('X', 'abc\u007Fdef'))).toContain(
+        'control_chars',
+      );
+    });
+
+    it('does NOT flag tab mid-value', () => {
+      expect(checkValue('X', 'abc\tdef')).toEqual([]);
+    });
+
+    it('does NOT flag LF mid-value (allowed for PEM / base64)', () => {
+      expect(checkValue('X', 'abc\ndef')).toEqual([]);
+    });
+  });
+
+  describe('invisible characters', () => {
+    it('flags zero-width space', () => {
+      expect(codes(checkValue('X', 'abc\u200Bdef'))).toContain('zero_width');
+    });
+
+    it('flags BOM', () => {
+      expect(codes(checkValue('X', '\uFEFFvalue'))).toEqual(
+        expect.arrayContaining(['leading_whitespace', 'zero_width']),
+      );
+    });
+
+    it('flags zero-width joiner', () => {
+      expect(codes(checkValue('X', 'abc\u200Ddef'))).toContain('zero_width');
+    });
+  });
+
+  describe('message safety', () => {
+    it('never includes the value in the message', () => {
+      const secret = 'super-secret-token-xyz';
+      const issues = checkValue('X', `${secret}\n`);
+      for (const { message } of issues) {
+        expect(message).not.toContain(secret);
+      }
+    });
+
+    it('includes the name, a byte length, and the code in the output', () => {
+      const [issue] = checkValue('MM_SENTRY_DSN', 'abc ');
+      expect(issue.code).toBe('trailing_whitespace');
+      expect(issue.message).toContain('MM_SENTRY_DSN');
+      expect(issue.message).toMatch(/\d+ bytes/);
+    });
+  });
+});
diff --git a/scripts/validate-build-config.js b/scripts/validate-build-config.js
@@ -6,9 +6,21 @@
 const fs = require('fs');
 const path = require('path');
 const yaml = require('js-yaml');
+const { checkValue } = require('./lib/validate-value');
 
 const BUILDS_PATH = path.join(__dirname, '../builds.yml');
 
+// Env keys in builds.yml that may legitimately be the empty string. Every
+// other declared env key must have a non-empty value, so an accidental
+// `PORTFOLIO_API_URL: ''` (or a botched YAML anchor merge) fails validation
+// instead of shipping with a blank value. Adding to this list should be
+// deliberate: the review on that PR is the gate for "yes, this key is
+// intentionally optional."
+const ENV_KEYS_ALLOWED_EMPTY = new Set([
+  'MM_PERPS_HIP3_ALLOWLIST_MARKETS',
+  'MM_PERPS_HIP3_BLOCKLIST_MARKETS',
+]);
+
 function validate() {
   if (!fs.existsSync(BUILDS_PATH)) {
     console.error('❌ builds.yml not found');
@@ -46,6 +58,20 @@ function validate() {
     if (!build.github_environment) {
       errors.push(`${name}: missing github_environment`);
     }
+
+    // Hygiene checks on env values: catch trailing whitespace, stray \r,
+    // invisible characters, accidental empties, etc. so operator typos fail
+    // CI before the build fans out. Strict by default; only the keys in
+    // ENV_KEYS_ALLOWED_EMPTY may be the empty string. Whitespace-only values
+    // always fail, since "intentionally empty" should be `''` not `'   '`.
+    if (build.env && typeof build.env === 'object') {
+      for (const [envKey, envVal] of Object.entries(build.env)) {
+        const issues = checkValue(`${name}.env.${envKey}`, envVal, {
+          allowEmpty: ENV_KEYS_ALLOWED_EMPTY.has(envKey),
+        });
+        issues.forEach((issue) => errors.push(issue.message));
+      }
+    }
   });
 
   if (errors.length > 0) {
diff --git a/scripts/validate-secrets-from-config.js b/scripts/validate-secrets-from-config.js
