Moving proving Utils to common #935
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -280,6 +280,11 @@ | |
| "types": "./dist/esm/src/utils/passports/passport_parsing/parseDscCertificateData.d.ts", | ||
| "import": "./dist/esm/src/utils/passports/passport_parsing/parseDscCertificateData.js", | ||
| "require": "./dist/cjs/src/utils/passports/passport_parsing/parseDscCertificateData.cjs" | ||
| }, | ||
| "./utils/proving": { | ||
| "types": "./dist/esm/src/utils/proving.d.ts", | ||
| "import": "./dist/esm/src/utils/proving.js", | ||
| "require": "./dist/cjs/src/utils/proving.cjs" | ||
| }, | ||
|
Comment on lines
+284
to
288
Contributor
There was a problem hiding this comment. 💡 Verification agent 🧩 Analysis chainValidate new export path against build outputs (CJS + ESM) to avoid runtime resolution failures The "./utils/proving" export looks correct, but the CJS artifact can be missing unless the CJS build includes an entry for src/utils/proving.ts (currently absent in tsup CJS config). If not fixed, require('@selfxyz/common/utils/proving') will fail at runtime. Execute after building to confirm both files exist: 🏁 Script executed: #!/bin/bash
set -euo pipefail
# From repo root
pushd common >/dev/null
yarn build
fd -a 'proving\.(js|cjs|d\.ts)$' dist | sed 's/^/found: /'
# Minimal assertion
test -f dist/esm/src/utils/proving.js
test -f dist/cjs/src/utils/proving.cjs
test -f dist/esm/src/utils/proving.d.ts
echo "exports for ./utils/proving are backed by built files."
popd >/dev/nullLength of output: 517 To ensure we use the correct Yarn version when building, let’s activate Corepack and re-run the verification: 🏁 Script executed: #!/bin/bash
set -euo pipefail
# From repo root
pushd common >/dev/null
# Activate Yarn v4.6.0 via Corepack
corepack prepare [email protected] --activate
# Build the package
yarn build
# List any proving artifacts
fd -a 'proving\.(js|cjs|d\.ts)$' dist | sed 's/^/found: /'
# Assertions to verify exports
test -f dist/esm/src/utils/proving.js
test -f dist/cjs/src/utils/proving.cjs
test -f dist/esm/src/utils/proving.d.ts
echo "exports for ./utils/proving are backed by built files."
popd >/dev/nullLength of output: 607 Let’s enable Corepack before activating the correct Yarn version and re-run the build check: 🏁 Script executed: #!/bin/bash
set -euo pipefail
# From repo root
pushd common >/dev/null
# Enable Corepack so [email protected] is honored
corepack enable
# Activate the exact Yarn version this project requires
corepack prepare [email protected] --activate
# Build artifacts
yarn build
# Verify presence of proving files
fd -a 'proving\.(js|cjs|d\.ts)$' dist | sed 's/^/found: /'
# Assertions
test -f dist/esm/src/utils/proving.js
test -f dist/cjs/src/utils/proving.cjs
test -f dist/esm/src/utils/proving.d.ts
echo "✅ exports for ./utils/proving are backed by built files."
popd >/dev/nullLength of output: 801 Let’s verify the CJS build configuration directly by inspecting the tsup config and build script: 🏁 Script executed: #!/bin/bash
set -euo pipefail
# From repo root
pushd common >/dev/null
# Show build script from package.json
echo "=== package.json scripts.build ==="
grep -R "\"build\"" -n package.json -C1
# Locate tsup config file
echo
echo "=== Locating tsup config ==="
cfg=$(fd --absolute-path 'tsup.config.*' || echo "")
if [[ -n "$cfg" ]]; then
echo "Found tsup config at: $cfg"
echo
echo "=== tsup config contents (first 200 lines) ==="
sed -n '1,200p' "$cfg"
echo
echo "=== Entry and format settings ==="
grep -R -nE "entry|format" "$cfg" || echo "(no explicit entry/format found)"
else
echo "No tsup.config.* file found in common/"
fi
popd >/dev/nullLength of output: 12546 Add missing CJS entry for The CJS build config in common/tsup.config.ts does not include an entry for "./utils/proving": {
"types": "./dist/esm/src/utils/proving.d.ts",
"import": "./dist/esm/src/utils/proving.js",
"require": "./dist/cjs/src/utils/proving.cjs"
},will fail at runtime when requiring the CJS artifact. Please update the CJS entry list: • File: common/tsup.config.ts Suggested diff: // Granular utils exports (CJS)
'src/utils/hash': 'src/utils/hash.ts',
'src/utils/bytes': 'src/utils/bytes.ts',
'src/utils/trees': 'src/utils/trees.ts',
+ 'src/utils/proving': 'src/utils/proving.ts',
'src/utils/scope': 'src/utils/scope.ts',
'src/utils/appType': 'src/utils/appType.ts',
// …After adding, rebuild and verify that: dist/cjs/src/utils/proving.cjsexists before merging. 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. this is done. didn you notice?
Contributor
There was a problem hiding this comment. 🧩 Analysis chain@aaronmgdr You're absolutely right, and I apologize for missing that! Looking at the tsup.config.ts, I can see that you've already added the The proving utilities export is properly configured for both module formats. Thanks for pointing out my oversight! 🏁 Script executed: Length of output: 1620 @aaronmgdr You're absolutely right, and I apologize for missing that! Looking at the tsup.config.ts, I can see that you've already added the The proving utilities export is properly configured for both module formats. Thanks for pointing out my oversight! 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
||
| "./utils/scope": { | ||
| "types": "./dist/esm/src/utils/scope.d.ts", | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,12 +2,9 @@ | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| import forge from 'node-forge'; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| import { WS_DB_RELAYER, WS_DB_RELAYER_STAGING } from '../constants/index.js'; | ||||||||||||||||||||||||||||||||||||||
| import { initElliptic } from '../utils/certificate_parsing/elliptic.js'; | ||||||||||||||||||||||||||||||||||||||
| import type { EndpointType } from './appType.js'; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const elliptic = initElliptic(); | ||||||||||||||||||||||||||||||||||||||
| const { ec: EC } = elliptic; | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -32,28 +29,24 @@ export type TEEPayloadDisclose = TEEPayloadBase & { | |||||||||||||||||||||||||||||||||||||
| version: number; | ||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // // eslint-disable-next-line -- ec must be created first | ||||||||||||||||||||||||||||||||||||||
| export const ec = new EC('p256'); | ||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line -- clientKey is created from ec so must be second | ||||||||||||||||||||||||||||||||||||||
| export const clientKey = ec.genKeyPair(); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+32
to
35
Contributor
There was a problem hiding this comment. 🛠️ Refactor suggestion Avoid exporting a process-wide private EC keypair; generate lazily and keep private key scoped Exporting clientKey leaks private key material to any importer and creates it at module-load time (before RNG/polyfills may be ready in React Native). This is a security footgun and can also yield weak randomness if polyfills aren’t initialized yet.
Apply this diff to defer generation and reduce exposure while retaining backward compatibility with the clientKey export: -// // eslint-disable-next-line -- ec must be created first
-export const ec = new EC('p256');
-// eslint-disable-next-line -- clientKey is created from ec so must be second
-export const clientKey = ec.genKeyPair();
+export const ec = new EC('p256');
+// Lazily initialize to ensure CSPRNG/polyfills are ready (especially on RN).
+let _clientKey: ReturnType<typeof ec.genKeyPair> | undefined;
+export function getClientKey() {
+ if (!_clientKey) {
+ _clientKey = ec.genKeyPair();
+ }
+ return _clientKey;
+}
+// Back-compat: keep named export, but source from the lazy getter.
+export const clientKey = getClientKey();If feasible in this PR (or next), stop exporting clientKey entirely and instead export:
📝 Committable suggestion
Suggested change
Contributor
Author
There was a problem hiding this comment. this is existing code im not changing it
Contributor
There was a problem hiding this comment. @aaronmgdr I understand - you're moving existing code to the common module without modifying its functionality. That makes perfect sense for the scope of this PR. Would you like me to create a follow-up issue to track the security concern I mentioned about the exported private key? This way it can be addressed separately without blocking your current refactoring work. |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| type RegisterSuffixes = '' | '_id'; | ||||||||||||||||||||||||||||||||||||||
| type DscSuffixes = '' | '_id'; | ||||||||||||||||||||||||||||||||||||||
| type DiscloseSuffixes = '' | '_id'; | ||||||||||||||||||||||||||||||||||||||
| type ProofTypes = 'register' | 'dsc' | 'disclose'; | ||||||||||||||||||||||||||||||||||||||
| type RegisterProofType = `${Extract<ProofTypes, 'register'>}${RegisterSuffixes}`; | ||||||||||||||||||||||||||||||||||||||
| type DscProofType = `${Extract<ProofTypes, 'dsc'>}${DscSuffixes}`; | ||||||||||||||||||||||||||||||||||||||
| type DiscloseProofType = `${Extract<ProofTypes, 'disclose'>}${DiscloseSuffixes}`; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| export const clientPublicKeyHex = | ||||||||||||||||||||||||||||||||||||||
| clientKey.getPublic().getX().toString('hex').padStart(64, '0') + | ||||||||||||||||||||||||||||||||||||||
| clientKey.getPublic().getY().toString('hex').padStart(64, '0'); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| export function encryptAES256GCM(plaintext: string, key: forge.util.ByteStringBuffer) { | ||||||||||||||||||||||||||||||||||||||
| const iv = forge.random.getBytesSync(12); | ||||||||||||||||||||||||||||||||||||||
| const cipher = forge.cipher.createCipher('AES-GCM', key); | ||||||||||||||||||||||||||||||||||||||
| cipher.start({ iv: iv, tagLength: 128 }); | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -75,7 +68,7 @@ export function getPayload( | |||||||||||||||||||||||||||||||||||||
| endpointType: EndpointType, | ||||||||||||||||||||||||||||||||||||||
| endpoint: string, | ||||||||||||||||||||||||||||||||||||||
| version: number = 1, | ||||||||||||||||||||||||||||||||||||||
| userDefinedData: string = '' | ||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||
| if (circuitType === 'disclose') { | ||||||||||||||||||||||||||||||||||||||
| const payload: TEEPayloadDisclose = { | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smart. yeah makes more sense in
common