Skip to content

Ack/dev - #3

Merged
Acktarius merged 9 commits into
mainfrom
ack/dev
Aug 18, 2026
Merged

Ack/dev#3
Acktarius merged 9 commits into
mainfrom
ack/dev

Conversation

@Acktarius

Copy link
Copy Markdown
Member

No description provided.

Acktarius and others added 8 commits November 16, 2025 20:40
Records commands and the standing biome-update convention:
plan $schema bump, verify schema on web, consider breaking changes,
adapt config in a PR, downgrade + admin review if Biome still errors,
verify with lint.
Summary
- pin react and react-native peers to the Nitro 0.30 / RN 0.82 stack instead of wildcard ranges
- bump biome to 2.5.9 and switch linter rules from recommended to preset
- tighten brace-expansion, minimatch, ws, and yaml overrides to patched transitive versions
- bump package version to 0.3.0
- image-size was evaluated: no patched release exists as of today (all versions <=2.0.2 still flagged via Metro);
risk is low because exploitation would require file injection in CI at build

Files:
- in `package.json`: 0.3.0, biome 2.5.9, peer pins, extra ws/yaml overrides
- in `package-lock.json`: lockfile for biome 2.5.9, ws 7.5.13, yaml 2.9.0, and peer metadata
- in `biome.json`: schema 2.5.9 and `preset: "recommended"`
@roomote-conceal-network

roomote-conceal-network Bot commented Aug 18, 2026

Copy link
Copy Markdown

3 issues outstanding. See task

  • cpp/Hmac.cpp:129 — hand-rolled SHA-256/SHA-512/HMAC where libsodium (already linked) provides vetted crypto_auth_hmacsha256/512; add RFC 4231 vector tests or delegate to sodium
  • .github/workflows/ci.yml:23npm test removed but CLAUDE.md still documents it in the required quality gate; npm test now errors with "Missing script"
  • package.json:3 — v0.3.0 released with new hmacSha256/hmacSha512 API but CHANGELOG.md has no v0.3.0 entry

Code review: Ack/dev @ a9c1c2a

The bulk of this PR is a clean clang-format pass — I verified the reformatted crypto files (crypto-ops.c, crypto.cpp, keccak.c, WordList.h, etc.) are token-identical to the previous versions apart from comments and include ordering; the 1626-word mnemonic list is unchanged. The new HMAC-SHA256/SHA-512 implementations pass all FIPS 180-4 digest vectors and RFC 4231 HMAC test cases I ran against them.

# Severity File Line(s) Issue
1 🟡 Medium cpp/Hmac.cpp 129–345 Hand-rolled SHA-2/HMAC primitives where libsodium is already a hard dependency; tests removed in the same PR, so future regressions would go undetected
2 🟡 Medium .github/workflows/ci.yml 21–24 Removing npm test breaks the documented CLAUDE.md quality gate ("Missing script: test")
3 🟢 Low package.json 3 v0.3.0 + new public API shipped without a CHANGELOG.md entry

Details are in the inline threads. No approval action taken.

Reviewed a9c1c2a

Modify the npm audit step to continue on error. This prevents the CI
pipeline from failing due to known vulnerabilities in `image-size`
(GHSA-w3rx-r6r6-pgpr, GHSA-5p2g-fcmc-qvqq) which currently lack a
patched release. The risk is considered low as it is a build-time DoS
vulnerability requiring file injection in the CI environment.
@Acktarius
Acktarius merged commit 06554e9 into main Aug 18, 2026
2 checks passed
Comment thread cpp/Hmac.cpp
return result;
}

std::vector<uint8_t> Hmac::sha256(const std::vector<uint8_t>& data) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds ~220 lines of hand-rolled SHA-256/SHA-512/HMAC while libsodium is already a hard dependency of this library (linked unconditionally in android/CMakeLists.txt via FetchContent, and used directly in HybridConcealCrypto.cpp for secretbox/base64). libsodium provides vetted, constant-time implementations via crypto_auth_hmacsha256 / crypto_auth_hmacsha512 that would replace all of this code.

To be clear on correctness: I extracted sha256/sha512/hmacImpl from this file and verified them against FIPS 180-4 digest vectors and RFC 4231 HMAC test cases 1, 2, and 6 — all pass. The concern is risk posture and maintenance, not a current bug: hand-maintained hash primitives in a crypto library are easy to regress, and this PR simultaneously removes the jest test infrastructure, so nothing would catch a future edit breaking these functions.

Suggestion: either delegate to libsodium's crypto_auth_hmacsha256/crypto_auth_hmacsha512 (keeping the existing ArrayBuffer API), or keep the implementation and add the RFC 4231 vectors as unit tests.

Comment thread .github/workflows/ci.yml
# (GHSA-w3rx-r6r6-pgpr, GHSA-5p2g-fcmc-qvqq; all versions <=2.0.2).
# Risk is low: build-time DoS that would require file injection in CI.
# Keep the report; do not fail the job until a fix exists.
- run: npm audit --audit-level=high

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR removes jest, the test script, and the CI npm test step, but CLAUDE.md still documents npm test in three places, including the required quality gate npm run lint && npm run check && npm run cpp && npm test. After this change npm test fails with "Missing script: test", so the documented contribution workflow is broken.

If the goal was just to drop the unused jest dependency, consider a zero-dependency replacement (e.g. node --test) so the quality gate stays green — that would also give the new HMAC code (see the Hmac.cpp comment) a place for RFC 4231 vectors. Otherwise, update CLAUDE.md to remove the npm test references.

Comment thread package.json
{
"name": "react-native-conceal-crypto",
"version": "0.2.8",
"version": "0.3.0",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version is bumped to 0.3.0 and two new public API methods (hmacSha256 / hmacSha512) are documented in the README, but CHANGELOG.md still ends at v0.2.8. The repo tracks every release in that table, so this release should get a v0.3.0 row (new features: hmacSha256/hmacSha512; also the RN ~0.82 peer pin is a notable compatibility change worth noting there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants