security: stop leaking signing secrets via shell interpolation / argv (PER-8611, PER-8612)

[Shivanshu-07]

Summary

Fourth focused percy-cli security PR — two High-severity release-pipeline secret-handling findings (deadline 2026-06-16).

Ticket	CWE	Finding
PER-8611	CWE-532	GCP KMS key path interpolated into the signtool shell command
PER-8612	CWE-214	Apple app-specific password passed as a CLI argument to notarytool

Changes

.github/workflows/executable.yml (PER-8611): Moved GCP_KMS_KEY_PATH into the signing step's env: map and reference $env:GCP_KMS_KEY_PATH in the PowerShell command, instead of interpolating ${{ secrets.GCP_KMS_KEY_PATH }} into the run: string. GitHub's secret redaction now applies to the value in all (including error) log output.

scripts/executable.sh (PER-8612): xcrun notarytool submit now uses --password "@env:APPLE_ID_KEY" (notarytool's documented env-var form) instead of --password $APPLE_ID_KEY, so the Apple app-specific password — which grants notarization capability under BrowserStack's Developer ID — is no longer visible in the process table.

Scope note (flagged)

The .p12 import passphrase on security import -P $APPLE_CERT_KEY (executable.sh:59) is not changed here: security import has no env/stdin option for -P (confirmed against the macOS security man page), so the only real fix is a keychain-pre-population rework of the signing flow — deferred to avoid risking the release pipeline. The GitHub-hosted macOS runner is single-tenant and ephemeral, which limits that argv-exposure window. Tracked as a follow-up on PER-8612.

Verification

• executable.yml parses as valid YAML; bash -n scripts/executable.sh passes.
• Confirmed GCP_KMS_KEY_PATH now appears only in the env: binding (not inline in the command), and notarytool reads @env:APPLE_ID_KEY.

Closes PER-8611. Addresses the app-specific-password leg of PER-8612 (.p12 import passphrase rework flagged as follow-up).

Note: this PR also touches executable.yml, which #2278 SHA-pins — the two edit different regions and should merge cleanly; rebase if #2278 lands first.

🤖 Generated with Claude Code

[Shivanshu-07]

Claude Code PR Review

PR: #2281 • Head: 88d1330 • Reviewers: stack:code-reviewer

Summary

Two targeted secret-leak fixes in the executable build/release:

1. PER-8612 (scripts/executable.sh): notarytool now reads the Apple app-specific password via --password "@env:APPLE_ID_KEY" instead of passing it as a bare CLI argument, removing it from the process table (CWE-214).
2. PER-8611 (.github/workflows/executable.yml): the GCP KMS key path is bound to a step-level env: and consumed as $env:GCP_KMS_KEY_PATH, instead of being interpolated as ${{ secrets.GCP_KMS_KEY_PATH }} into the PowerShell command string — so GitHub log redaction now applies in all output paths (CWE-532).

Both introduced changes are correct. $APPLE_TEAM_ID was also correctly double-quoted.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No secret leakage via argv/logs (introduced changes)	Pass	@env: + step-level env: binding both correct
High	Correctness	Logic correct	Pass	APPLE_ID_KEY is in the script step's env:, so notarytool resolves it at runtime
High	Security	GH Actions injection	Pass	No untrusted input interpolated into run:
Medium	Quality	Focused changes	Pass	Two-line, single-concern fix
Low	Quality	Shell quoting	Pass	$APPLE_TEAM_ID now quoted

Findings

No Critical/High issues are introduced or worsened by this PR.

Pre-existing (out of scope, recommended follow-up): scripts/executable.sh:59 still passes the .p12 certificate passphrase as security import ... -P $APPLE_CERT_KEY — the same class of argv exposure (CWE-214) this PR fixes for notarytool, and the variable is unquoted. It is not introduced or worsened by this PR (the line is unchanged). security import has no clean @env:/stdin equivalent, so a safe fix needs macOS verification of the signing flow; recommend a follow-up ticket to PER-8612 rather than bundling an untested change into this PR. Practical risk is bounded — ephemeral, isolated, single-tenant runner.

Note: SHA-pinning of the actions in this workflow is handled separately by PER-8604/8608 (#2278); ensure that lands so this workflow doesn't regress to floating tags.

---

Verdict: PASS

[Shivanshu-07]

Claude Code PR Review

PR: #2281 • Head: 8cc62d0 • Reviewers: fallback inline checklist

Summary

Release-secret hardening (PER-8611/8612): bind GCP_KMS_KEY_PATH to a step env: and reference $env:GCP_KMS_KEY_PATH instead of interpolating the secret into the PowerShell command string (CWE-532); and pass the Apple app-specific password to notarytool via @env:APPLE_ID_KEY instead of a CLI argument visible in the process table (CWE-214). Also quotes $APPLE_TEAM_ID.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Both changes strictly reduce secret exposure (log redaction now applies; password no longer in argv) with no behavioral change to signing/notarization. Note (non-blocking, tracked on PER-8612): the .p12 security import -P argv exposure is not addressed here — security import has no env form — and is deferred, out of scope for this diff.
High	Security	Authentication/authorization checks present	N/A	CI/build config, no app auth surface
High	Security	Input validation and sanitization	N/A	No user input paths changed
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Pass	No runtime logic changed
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Unchanged
High	Correctness	No race conditions or concurrency issues	N/A	—
Medium	Testing	New code has corresponding tests	N/A	CI/pipeline config; not unit-testable
Medium	Testing	Error paths and edge cases tested	N/A	—
Medium	Testing	Existing tests still pass (no regressions)	Pass	No source/test changes in net diff
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	—
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Matches repo's pin/permission style
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to the security hardening
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Rationale comments cite the CWE
Low	Quality	No unnecessary dependencies added	Pass	None added

Findings

No blocking findings. Both changes strictly reduce secret exposure (log redaction now applies; password no longer in argv) with no behavioral change to signing/notarization. Note (non-blocking, tracked on PER-8612): the .p12 security import -P argv exposure is not addressed here — security import has no env form — and is deferred, out of scope for this diff.

---

Verdict: PASS