e2e test using passport and EU-ID #1281
Conversation
WalkthroughAdds a new test harness: TypeScript and Go API services exposing /api/verify, in-memory config stores, multi-stage Docker builds and docker-compose configs, cryptographic assets and Solidity verifier, end-to-end parity tests and test utilities, and workspace/package adjustments. All changes are additive; no existing source modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TestRunner as Test Runner
participant TS as TS API
participant Go as Go API
participant CfgTS as TS In-Mem Store
participant CfgGo as Go In-Mem Store
participant Verifier as Verifier (ZK/Rules)
TestRunner->>TS: POST /api/verify {attestationId, proof, publicSignals, userContextData}
TS->>CfgTS: getConfig / getActionId
CfgTS-->>TS: config/options
TS->>Verifier: verify(proof, publicSignals, config)
Verifier-->>TS: result, subject, flags
TS-->>TestRunner: response (success/failure)
TestRunner->>Go: POST /api/verify {attestationId, proof, publicSignals, userContextData}
Go->>CfgGo: GetConfig / GetActionId
CfgGo-->>Go: config/options
Go->>Verifier: verify(proof, publicSignals, config)
Verifier-->>Go: result, subject, flags
Go-->>TestRunner: response (success/failure)
sequenceDiagram
autonumber
participant Dev as Developer
participant Compose as docker-compose
participant TScont as ts-api container
participant Gocont as go-api container
Dev->>Compose: up -d
Compose->>TScont: build + start (3000)
Compose->>Gocont: build + start (8080)
TScont-->>Compose: HEALTHCHECK /health
Gocont-->>Compose: HEALTHCHECK /health
Dev->>Compose: logs / status / down
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Nitpick comments (16)
sdk/tests/ts-api/tsconfig.json (1)
14-16: Disable declaration emit in test tsconfig to cut build time and noiseEmitting .d.ts for tests is unnecessary and slows CI, while increasing the chance of type‑emit failures.
Apply:
- "declaration": true, + "declaration": false, "outDir": "./dist", "rootDir": "./",Optionally split build vs test configs (e.g., extend a base tsconfig and enable declaration only in tsconfig.build.json).
sdk/tests/ts-api/utils/assests/aws_root_pem.ts (1)
1-1: Typo in path: “assests” → “assets”Standardize folder name to assets across repo to avoid brittle imports and future confusion. Plan a coordinated rename and update references.
sdk/tests/package.json (1)
14-21: Review and bump test dependencies to latest secure majors/minors
- Mocha: consider 11.x for security/bugfixes.
- Chai: 6.x is current ESM-first; verify plugin compat before upgrading.
- Axios: bump to latest 1.x for fixes.
Based on learnings
sdk/tests/ts-api/package.json (1)
15-35: Update Axios, assess Express major upgrade, and tighten CORS
- Axios: bump to ^1.12.2 for the latest 1.x security/bug fixes.
- Express: ^4.18.2 is the newest 4.x release; migrating to v5.1.0 is a major change—plan and test separately.
- In your server setup, lock CORS to explicit origins and methods (no wildcards).
sdk/tests/go-api/main.go (1)
75-92: Duplicate CORS header configuration detected.The
corsMiddlewaresets CORS headers globally (lines 79-81), but theVerifyHandlerinapi/verify.goalso sets identical CORS headers. This creates redundancy and potential inconsistencies if one location is updated without the other.Consider removing CORS headers from individual handlers and relying solely on the middleware:
Remove these lines from
sdk/tests/go-api/api/verify.go:// In VerifyHandler function w.Header().Set("Access-Control-Allow-Origin", "*") w.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS") w.Header().Set("Access-Control-Allow-Headers", "Content-Type")sdk/tests/run-apis.sh (1)
6-6: Harden the script with strict modeUse errexit, nounset, and pipefail for safer orchestration.
-set -e +set -Eeuo pipefailsdk/tests/ts-api/api/verify.ts (1)
110-159: Remove unreachable duplicate invalid branchThe earlier return on invalid makes this else path dead code. Simplify the handler to the success path only here.
Apply this diff to drop the else block:
- if (result.isValidDetails.isValid) { + if (result.isValidDetails.isValid) { const filteredSubject = { ...result.discloseOutput }; ... res.status(200).json({ ... }); - } else { - res.status(400).json({ - status: "error", - result: result.isValidDetails.isValid, - message: "Verification failed", - details: result, - }); - } + }Also consider 422 for invalid proofs (see earlier diff).
sdk/tests/ts-api/utils/assests/verifier.sol (2)
21-21: Pin Solidity compiler version for reproducibility and safetyAvoid open-ended pragma; pin to a vetted compiler (e.g., 0.8.20) to prevent semantic drift and supply‑chain surprises.
-pragma solidity >=0.7.0 <0.9.0; +pragma solidity 0.8.20;
119-152: Defense-in-depth: range‑check proof coordinates (A, B, C) against qCurrently only public inputs are field‑checked. Add checks for proof point coordinates to fail fast on malformed inputs before precompiles. This is common hardening in verifiers.
assembly { function checkField(v) { if iszero(lt(v, r)) { mstore(0, 0) return(0, 0x20) } } + // Base field check for curve coordinates (q) + function checkFieldQ(v) { + if iszero(lt(v, q)) { + mstore(0, 0) + return(0, 0x20) + } + } @@ - // -A + // Validate proof coordinates ∈ F_q + checkFieldQ(calldataload(pA)) // A.x + checkFieldQ(calldataload(add(pA, 32))) // A.y + checkFieldQ(calldataload(pC)) // C.x + checkFieldQ(calldataload(add(pC, 32))) // C.y + checkFieldQ(calldataload(pB)) // B.x.c0 + checkFieldQ(calldataload(add(pB, 32))) // B.x.c1 + checkFieldQ(calldataload(add(pB, 64))) // B.y.c0 + checkFieldQ(calldataload(add(pB, 96))) // B.y.c1 + + // -A mstore(_pPairing, calldataload(pA)) mstore(add(_pPairing, 32), mod(sub(q, calldataload(add(pA, 32))), q))Also applies to: 206-215, 231-246, 256-300
sdk/tests/ts-api/Dockerfile (1)
34-37: Production hardening: set NODE_ENV and make healthcheck fail fast on errors
- Set NODE_ENV=production in runner.
- Add error handling to healthcheck so connection failures return non‑zero exit.
RUN corepack enable COPY .yarnrc.yml . +# Ensure production mode in the runtime image +ENV NODE_ENV=production @@ -HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ - CMD node -e "require('http').get('http://localhost:3000/health', (res) => { process.exit(res.statusCode === 200 ? 0 : 1) })" +HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ + CMD node -e "const http=require('http');const req=http.get('http://localhost:3000/health',res=>process.exit(res.statusCode===200?0:1));req.on('error',()=>process.exit(1));setTimeout(()=>process.exit(1),2500);"Also consider pinning the base image to a digest (e.g., node:22-alpine@sha256:...) for supply‑chain reproducibility.
Also applies to: 71-73
sdk/tests/go-api/api/verify.go (2)
163-163: Use request-scoped context to enable cancellations/timeoutsPrefer r.Context() over context.Background() inside handlers.
- ctx := context.Background() + ctx := r.Context()
198-206: Reuse a singleton verifier to avoid repeated RPC dials per requestConstructing a new BackendVerifier (with ethclient.Dial) on every request is costly and fragile under load. Cache it with sync.Once.
@@ -import ( +import ( "context" "encoding/json" "log" "net/http" "strconv" + "sync" @@ -var configStoreInstance *config.InMemoryConfigStore +var configStoreInstance *config.InMemoryConfigStore +var ( + verifierOnce sync.Once + backendVerifier *self.BackendVerifier +) @@ - verifier, err := self.NewBackendVerifier( - "self-playground", - verifyEndpoint, - true, // Use testnet for testing - allowedIds, - configStoreInstance, - self.UserIDTypeUUID, // Use UUID format for user IDs - ) - if err != nil { - log.Printf("Failed to initialize verifier: %v", err) - w.WriteHeader(http.StatusInternalServerError) - json.NewEncoder(w).Encode(VerifyResponse{ - Status: "error", - Result: false, - Message: "Internal server error", - }) - return - } + var initErr error + verifierOnce.Do(func() { + backendVerifier, initErr = self.NewBackendVerifier( + "self-playground", + verifyEndpoint, + true, // Use testnet for testing + allowedIds, + configStoreInstance, + self.UserIDTypeUUID, + ) + }) + if initErr != nil || backendVerifier == nil { + log.Printf("Failed to initialize verifier: %v", initErr) + w.WriteHeader(http.StatusInternalServerError) + json.NewEncoder(w).Encode(VerifyResponse{Status: "error", Result: false, Message: "Internal server error"}) + return + } @@ - result, err := verifier.Verify( + result, err := backendVerifier.Verify( ctx, attestationIdInt, vcProof, publicSignals, userContextDataStr, )sdk/tests/utils.ts (2)
282-282: Document the hardcoded delay.The 5-second sleep is hardcoded without explanation. Document why this delay is necessary (presumably waiting for tree synchronization) and consider making it configurable or adding retry logic.
+ // Wait for identity tree to sync with the newly registered passport + const TREE_SYNC_DELAY_MS = 5000; console.log("Waiting for identity tree to sync..."); - await new Promise(resolve => setTimeout(resolve, 5000)); + await new Promise(resolve => setTimeout(resolve, TREE_SYNC_DELAY_MS));
315-323: Document the userContextData hex format.The hardcoded hex strings lack documentation explaining their structure. Based on the comment at lines 315-316, the format is: destChainId (32 bytes) + userIdentifier (32 bytes) + userDefinedData.
Consider adding JSDoc comments:
/** * Returns valid user context data as hex string. * Format: destChainId (32 bytes) + userIdentifier (32 bytes) + userDefinedData * - destChainId: 0xa4ec (Celo testnet) * - userIdentifier: 0x94ba0db8a9db66979905784a9d6b2d286e55bd27 * - userDefinedData: "hello from the playground" (hex encoded) */ export function getUserContextData() { return "000000000000000000000000000000000000000000000000000000000000a4ec00000000000000000000000094ba0db8a9db66979905784a9d6b2d286e55bd2768656c6c6f2066726f6d2074686520706c617967726f756e64"; } /** * Returns invalid user context data (userIdentifier has one byte modified). * Used for testing error handling. */ export function getInvalidUserContextData() { return "000000000000000000000000000000000000000000000000000000000000a4ec00000000000000000000000094ba0db8a9db66979905784a9d6b2d286e55bd2868656c6c6f2066726f6d2074686520706c617967726f756e64"; }sdk/tests/ts-api/utils/helper.ts (2)
62-97: Consolidate duplicate circuit input generation.The two branches for attestation ID 1 vs others have nearly identical code except for the
passportNo_smtparameter (null in one case). This duplication makes maintenance harder.Apply this diff to consolidate:
- let vcAndDiscloseCircuitInputs: CircuitSignals; - if(attestationId === "1"){ - vcAndDiscloseCircuitInputs = generateCircuitInputsVCandDisclose( - secret, - attestationId, - passportData, - scope, - selectorDg1, - selectorOlderThan, - merkletree, - majority, - passportNo_smt, - nameAndDob_smt, - nameAndYob_smt, - selectorOfac, - forbiddenCountriesList, - userIdentifier, - ); - } - else { - vcAndDiscloseCircuitInputs = generateCircuitInputsVCandDisclose( - secret, - attestationId, - passportData, - scope, - selectorDg1, - selectorOlderThan, - merkletree, - majority, - null, - nameAndDob_smt, - nameAndYob_smt, - selectorOfac, - forbiddenCountriesList, - userIdentifier, - ); - } + const vcAndDiscloseCircuitInputs: CircuitSignals = generateCircuitInputsVCandDisclose( + secret, + attestationId, + passportData, + scope, + selectorDg1, + selectorOlderThan, + merkletree, + majority, + attestationId === "1" ? passportNo_smt : null, + nameAndDob_smt, + nameAndYob_smt, + selectorOfac, + forbiddenCountriesList, + userIdentifier, + );
166-166: Extract hardcoded chain ID and address to configuration.The chain ID (42220 - Celo testnet) and contract address are hardcoded. Consider extracting these to a configuration file or constants module for easier maintenance and environment-specific overrides.
In
constant.ts, add:export const DEFAULT_CHAIN_ID = 42220; // Celo testnet export const DEFAULT_CONTRACT_ADDRESS = "94ba0DB8A9Db66979905784A9d6B2D286e55Bd27";Then import and use:
+import { DEFAULT_CHAIN_ID, DEFAULT_CONTRACT_ADDRESS } from "./constant.js"; + - const userIdentifier= calculateUserIdentifierHash(42220, "94ba0DB8A9Db66979905784A9d6B2D286e55Bd27", userContextData); + const userIdentifier = calculateUserIdentifierHash(DEFAULT_CHAIN_ID, DEFAULT_CONTRACT_ADDRESS, userContextData);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
sdk/tests/go-api/go.sumis excluded by!**/*.sumsdk/tests/ts-api/utils/assests/vc_and_disclose.wasmis excluded by!**/*.wasmsdk/tests/ts-api/utils/assests/vc_and_disclose_id.wasmis excluded by!**/*.wasmyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
.dockerignore(1 hunks)package.json(1 hunks)sdk/tests/docker-compose.yml(1 hunks)sdk/tests/go-api/.dockerignore(1 hunks)sdk/tests/go-api/Dockerfile(1 hunks)sdk/tests/go-api/README.md(1 hunks)sdk/tests/go-api/api/verify.go(1 hunks)sdk/tests/go-api/config/config.go(1 hunks)sdk/tests/go-api/docker-compose.yml(1 hunks)sdk/tests/go-api/go.mod(1 hunks)sdk/tests/go-api/main.go(1 hunks)sdk/tests/package.json(1 hunks)sdk/tests/run-apis.sh(1 hunks)sdk/tests/ts-api/.dockerignore(1 hunks)sdk/tests/ts-api/Dockerfile(1 hunks)sdk/tests/ts-api/README.md(1 hunks)sdk/tests/ts-api/api/verify.ts(1 hunks)sdk/tests/ts-api/docker-compose.yml(1 hunks)sdk/tests/ts-api/package.json(1 hunks)sdk/tests/ts-api/server.ts(1 hunks)sdk/tests/ts-api/tsconfig.json(1 hunks)sdk/tests/ts-api/utils/assests/aws_root_pem.ts(1 hunks)sdk/tests/ts-api/utils/assests/verification_key.json(1 hunks)sdk/tests/ts-api/utils/assests/verification_key_id.json(1 hunks)sdk/tests/ts-api/utils/assests/verifier.sol(1 hunks)sdk/tests/ts-api/utils/constant.ts(1 hunks)sdk/tests/ts-api/utils/helper.ts(1 hunks)sdk/tests/unified-api.test.ts(1 hunks)sdk/tests/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
sdk/tests/ts-api/utils/assests/aws_root_pem.tssdk/tests/ts-api/api/verify.tssdk/tests/ts-api/server.tssdk/tests/ts-api/utils/assests/verifier.solsdk/tests/ts-api/utils/constant.tssdk/tests/unified-api.test.tssdk/tests/ts-api/utils/helper.tssdk/tests/utils.ts
sdk/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
sdk/**/*.{ts,tsx,js,jsx}: Review SDK packages for:
- Public API design and stability
- Asynchronous flows and error handling
- Security and input validation
- Compatibility across environments
Files:
sdk/tests/ts-api/utils/assests/aws_root_pem.tssdk/tests/ts-api/api/verify.tssdk/tests/ts-api/server.tssdk/tests/ts-api/utils/constant.tssdk/tests/unified-api.test.tssdk/tests/ts-api/utils/helper.tssdk/tests/utils.ts
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use Yarn only for package management (yarn install/add/remove); do not use npm or pnpm in scripts
Files:
package.jsonsdk/tests/ts-api/package.jsonsdk/tests/package.json
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
sdk/tests/unified-api.test.ts
🧠 Learnings (4)
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Use actual imports from selfxyz/mobile-sdk-alpha in tests
Applied to files:
sdk/tests/ts-api/package.jsonsdk/tests/package.json
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Test isPassportDataValid() with realistic synthetic passport data (never real user data)
Applied to files:
sdk/tests/unified-api.test.tssdk/tests/utils.ts
📚 Learning: 2025-08-24T18:54:04.809Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursor/rules/mobile-sdk-migration.mdc:0-0
Timestamp: 2025-08-24T18:54:04.809Z
Learning: Applies to packages/mobile-sdk-alpha/package.json : Expose a 'test:build' script in the SDK's package.json that runs build, test, types, and lint
Applied to files:
sdk/tests/package.json
📚 Learning: 2025-08-24T18:54:04.809Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursor/rules/mobile-sdk-migration.mdc:0-0
Timestamp: 2025-08-24T18:54:04.809Z
Learning: Applies to app/package.json : Expose a 'test:build' script in the app's package.json that builds deps, types, performs bundle analysis, and runs tests
Applied to files:
sdk/tests/package.json
🧬 Code graph analysis (8)
sdk/tests/go-api/config/config.go (1)
sdk/sdk-go/utils.go (7)
Ofac(90-90)IssuingState(82-82)Name(83-83)Nationality(85-85)DateOfBirth(86-86)Gender(87-87)ExpiryDate(88-88)
sdk/tests/ts-api/api/verify.ts (1)
sdk/tests/go-api/config/config.go (1)
SelfAppDisclosureConfig(13-24)
sdk/tests/ts-api/server.ts (1)
sdk/tests/ts-api/api/verify.ts (1)
verifyHandler(50-168)
sdk/tests/go-api/main.go (1)
sdk/tests/go-api/api/verify.go (1)
VerifyHandler(52-328)
sdk/tests/unified-api.test.ts (3)
sdk/tests/utils.ts (6)
callAPI(175-201)compareAPIs(203-264)getUserContextData(317-319)getInvalidUserContextData(321-323)setupTestData(270-303)getTestData(305-313)sdk/core/src/utils/proof.ts (1)
getRevealedDataBytes(42-58)common/src/utils/bytes.ts (2)
packBytes(94-108)packBytesArray(110-138)
sdk/tests/ts-api/utils/helper.ts (1)
sdk/tests/ts-api/utils/constant.ts (1)
WSS_URL(7-7)
sdk/tests/utils.ts (2)
sdk/tests/ts-api/utils/helper.ts (3)
handshakeAndGetUuid(240-311)getProofGeneratedUpdate(313-343)runGenerateVcAndDiscloseRawProof(137-221)sdk/tests/ts-api/utils/constant.ts (2)
DSC_URL(3-3)REGISTER_URL(1-1)
sdk/tests/go-api/api/verify.go (5)
sdk/tests/go-api/config/config.go (3)
InMemoryConfigStore(33-37)NewKVConfigStoreFromEnv(93-95)SelfAppDisclosureConfig(13-24)sdk/sdk-go/common/constants.go (2)
PAK(175-175)IRN(112-112)sdk/sdk-go/utils.go (10)
Ofac(90-90)Passport(17-17)EUCard(18-18)IssuingState(82-82)Name(83-83)Nationality(85-85)DateOfBirth(86-86)Gender(87-87)ExpiryDate(88-88)IdNumber(84-84)sdk/sdk-go/verifier.go (1)
NewBackendVerifier(90-133)sdk/sdk-go/types.go (2)
UserIDTypeUUID(66-66)IsValidDetails(25-29)
🪛 LanguageTool
sdk/tests/go-api/README.md
[grammar] ~58-~58: There might be a mistake here.
Context: ... in-memory storage for testing purposes: - Verification configuration is hard-coded...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...data is stored in memory - Data is lost when server restarts ## Docker Setup ### ...
(QB_NEW_EN)
[grammar] ~93-~93: There might be a mistake here.
Context: ...uild ``` The Docker container includes: - Health check endpoint at /health - Aut...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...er includes: - Health check endpoint at /health - Automatic restart policy - Non-root user...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ... at /health - Automatic restart policy - Non-root user for security ### Docker E...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
sdk/tests/go-api/README.md
30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
77-77: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
86-86: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
sdk/tests/ts-api/README.md
30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
66-66: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
75-75: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: type-check
🔇 Additional comments (2)
sdk/tests/ts-api/server.ts (1)
3-3: ESM import resolution already configured
Thesdk/tests/ts-api/tsconfig.jsonuses"module": "NodeNext"and"moduleResolution": "NodeNext", so importing./api/verify.jsis supported.sdk/tests/ts-api/utils/helper.ts (1)
494-498: Document the security implications of dev mode.The
devflag allows bypassing the debug status check, which could accept attestations from TEE instances with debug mode enabled. While this may be necessary for testing, it's important to ensure this flag is never true in production environments.Consider adding explicit guards:
export function validatePKIToken( attestationToken: string, - dev: boolean = true + dev: boolean = false // Default to stricter production mode ): { userPubkey: Buffer; serverPubkey: Buffer; imageHash: string; verified: boolean; } { + if (dev) { + console.warn("⚠️ DEV MODE: Skipping debug status verification. DO NOT USE IN PRODUCTION."); + } // ... rest of function
| # Documentation | ||
| **/README.md | ||
| **/docs/ | ||
| **/*.md | ||
| !package.json | ||
|
|
||
| # Compiled circuits (large files) |
There was a problem hiding this comment.
*Global ignore of .md may strip LICENSE/NOTICE from images — add allowlist to maintain compliance
Unintentionally excluding license/notice docs can create compliance issues for distributed images.
Apply allowlist rules:
**/*.md
+!**/LICENSE
+!**/LICENSE.*
+!**/NOTICE
+!**/NOTICE.*
+!**/THIRD_PARTY_LICENSES*
!package.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Documentation | |
| **/README.md | |
| **/docs/ | |
| **/*.md | |
| !package.json | |
| # Compiled circuits (large files) | |
| # Documentation | |
| **/README.md | |
| **/docs/ | |
| **/*.md | |
| !**/LICENSE | |
| !**/LICENSE.* | |
| !**/NOTICE | |
| !**/NOTICE.* | |
| !**/THIRD_PARTY_LICENSES* | |
| !package.json | |
| # Compiled circuits (large files) |
🤖 Prompt for AI Agents
In .dockerignore around lines 57 to 63, the global ignore pattern "**/*.md" will
also exclude important license/notice files; modify the rules to keep the global
markdown ignore but add allowlist exceptions for licensing files (e.g. add
patterns like "!**/LICENSE*", "!**/NOTICE*", "!**/COPYING*", and optionally
"!**/LICENSE.*" or specific filenames) so LICENSE/NOTICE/COPYING and similarly
named license files are not excluded from Docker build context.
| - NODE_ENV=production | ||
| - PORT=3000 | ||
| healthcheck: | ||
| test: ["CMD", "curl", "-f", "http://localhost:3000/health"] |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that healthcheck commands are available in containers.
The healthchecks rely on curl (ts-api) and wget (go-api) being present in the container images. These tools may not be included in minimal base images, causing healthchecks to fail.
Also applies to: 32-32
🏁 Script executed:
#!/bin/bash
# Verify if curl/wget are available in the Dockerfiles or base images
echo "Checking ts-api Dockerfile for curl availability:"
if [ -f "sdk/tests/ts-api/Dockerfile" ]; then
cat sdk/tests/ts-api/Dockerfile | grep -i "curl\|apt-get\|apk"
else
echo "ts-api Dockerfile not found"
fi
echo -e "\nChecking go-api Dockerfile for wget availability:"
if [ -f "sdk/tests/go-api/Dockerfile" ]; then
cat sdk/tests/go-api/Dockerfile | grep -i "wget\|apt-get\|apk"
else
echo "go-api Dockerfile not found"
fiLength of output: 346
🏁 Script executed:
#!/bin/bash
echo "Displaying first 50 lines of ts-api Dockerfile:"
sed -n '1,50p' sdk/tests/ts-api/DockerfileLength of output: 1779
Install curl in ts-api Dockerfile runner stage
ts-api’s runner image (node:22-alpine) doesn’t include curl, so the healthcheck in docker-compose.yml will always fail. In sdk/tests/ts-api/Dockerfile’s runner stage add:
RUN apk --no-cache add curlbefore running the healthcheck.
🤖 Prompt for AI Agents
In sdk/tests/ts-api/Dockerfile (runner stage) add a package install for curl
before the image is used in the docker-compose healthcheck: insert a RUN apk
--no-cache add curl line in the runner stage (the node:22-alpine based stage)
before any commands that expect curl so the docker-compose.yml healthcheck at
sdk/tests/docker-compose.yml line 13 can succeed; rebuild the image or update
your CI to use the new image tag.
| volumes: | ||
| ts-api-logs: | ||
| go-api-logs: |
There was a problem hiding this comment.
Declared volumes are not mounted to any service.
The volumes ts-api-logs and go-api-logs are declared but never referenced in the service definitions. Either mount them to the services or remove the declarations to avoid confusion.
Example of mounting volumes:
ts-api:
build:
context: ../..
dockerfile: sdk/tests/ts-api/Dockerfile
ports:
- "3000:3000"
+ volumes:
+ - ts-api-logs:/app/logs
environment:
- NODE_ENV=production📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| volumes: | |
| ts-api-logs: | |
| go-api-logs: | |
| ts-api: | |
| build: | |
| context: ../.. | |
| dockerfile: sdk/tests/ts-api/Dockerfile | |
| ports: | |
| - "3000:3000" | |
| volumes: | |
| - ts-api-logs:/app/logs | |
| environment: | |
| - NODE_ENV=production |
🤖 Prompt for AI Agents
In sdk/tests/docker-compose.yml around lines 45 to 47, the named volumes
ts-api-logs and go-api-logs are declared but not referenced by any service;
either remove these unused volume declarations or mount them into the
appropriate services by adding volume entries under each service (for example,
add - ts-api-logs:/path/to/logs for the TypeScript service and -
go-api-logs:/path/to/logs for the Go service) and ensure the paths match where
each service writes logs so the volumes are actually used.
| w.Header().Set("Content-Type", "application/json") | ||
|
|
||
| var req VerifyRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| json.NewEncoder(w).Encode(map[string]string{"message": "Invalid JSON"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Bound request body and tighten JSON decoding
Prevent large-body DoS and reject unknown fields early.
- var req VerifyRequest
- if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+ // Limit body to 1MB
+ r.Body = http.MaxBytesReader(w, r.Body, 1<<20)
+ var req VerifyRequest
+ dec := json.NewDecoder(r.Body)
+ dec.DisallowUnknownFields()
+ if err := dec.Decode(&req); err != nil {
w.WriteHeader(http.StatusBadRequest)
json.NewEncoder(w).Encode(map[string]string{"message": "Invalid JSON"})
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| w.Header().Set("Content-Type", "application/json") | |
| var req VerifyRequest | |
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| w.WriteHeader(http.StatusBadRequest) | |
| json.NewEncoder(w).Encode(map[string]string{"message": "Invalid JSON"}) | |
| return | |
| } | |
| w.Header().Set("Content-Type", "application/json") | |
| // Limit body to 1MB | |
| r.Body = http.MaxBytesReader(w, r.Body, 1<<20) | |
| var req VerifyRequest | |
| dec := json.NewDecoder(r.Body) | |
| dec.DisallowUnknownFields() | |
| if err := dec.Decode(&req); err != nil { | |
| w.WriteHeader(http.StatusBadRequest) | |
| json.NewEncoder(w).Encode(map[string]string{"message": "Invalid JSON"}) | |
| return | |
| } |
🤖 Prompt for AI Agents
In sdk/tests/go-api/api/verify.go around lines 70–77, the JSON decoding reads
the raw r.Body without size limits and allows unknown fields; wrap the request
body with http.MaxBytesReader to enforce a reasonable max (e.g., 1MB) to prevent
large-body DoS, create a json.Decoder from that limited body, call
decoder.DisallowUnknownFields() to reject unknown JSON fields, then decode into
VerifyRequest and handle decode errors by returning HTTP 400 with a clear
message when decoding fails or the body is too large.
| if result == nil || !result.IsValidDetails.IsValid { | ||
| log.Printf("Verification failed - invalid result") | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| json.NewEncoder(w).Encode(VerifyResponse{ | ||
| Status: "error", | ||
| Result: false, | ||
| Message: "Verification failed", | ||
| Details: result.IsValidDetails, | ||
| }) | ||
| return |
There was a problem hiding this comment.
Fix potential nil dereference when verification returns nil result
The code accesses result.IsValidDetails when result can be nil. This will panic and drop the request.
- if result == nil || !result.IsValidDetails.IsValid {
- log.Printf("Verification failed - invalid result")
- w.WriteHeader(http.StatusInternalServerError)
- json.NewEncoder(w).Encode(VerifyResponse{
- Status: "error",
- Result: false,
- Message: "Verification failed",
- Details: result.IsValidDetails,
- })
- return
- }
+ if result == nil {
+ log.Printf("Verification failed - nil result")
+ w.WriteHeader(http.StatusInternalServerError)
+ json.NewEncoder(w).Encode(VerifyResponse{
+ Status: "error",
+ Result: false,
+ Message: "Verification failed",
+ })
+ return
+ }
+ if !result.IsValidDetails.IsValid {
+ log.Printf("Verification failed - invalid result")
+ w.WriteHeader(http.StatusInternalServerError)
+ json.NewEncoder(w).Encode(VerifyResponse{
+ Status: "error",
+ Result: false,
+ Message: "Verification failed",
+ Details: result.IsValidDetails,
+ })
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if result == nil || !result.IsValidDetails.IsValid { | |
| log.Printf("Verification failed - invalid result") | |
| w.WriteHeader(http.StatusInternalServerError) | |
| json.NewEncoder(w).Encode(VerifyResponse{ | |
| Status: "error", | |
| Result: false, | |
| Message: "Verification failed", | |
| Details: result.IsValidDetails, | |
| }) | |
| return | |
| // Handle nil result to avoid panicking on result.IsValidDetails | |
| if result == nil { | |
| log.Printf("Verification failed - nil result") | |
| w.WriteHeader(http.StatusInternalServerError) | |
| json.NewEncoder(w).Encode(VerifyResponse{ | |
| Status: "error", | |
| Result: false, | |
| Message: "Verification failed", | |
| }) | |
| return | |
| } | |
| // Now safe to inspect result.IsValidDetails | |
| if !result.IsValidDetails.IsValid { | |
| log.Printf("Verification failed - invalid result") | |
| w.WriteHeader(http.StatusInternalServerError) | |
| json.NewEncoder(w).Encode(VerifyResponse{ | |
| Status: "error", | |
| Result: false, | |
| Message: "Verification failed", | |
| Details: result.IsValidDetails, | |
| }) | |
| return | |
| } |
🤖 Prompt for AI Agents
In sdk/tests/go-api/api/verify.go around lines 235-244, the handler dereferences
result.IsValidDetails without ensuring result is non-nil which can cause a nil
pointer panic; update the flow to check for result == nil before accessing any
fields, and when result is nil return a safe VerifyResponse with Details set to
a nil/zero value (or an empty/default IsValidDetails struct) so no fields are
accessed on a nil pointer; adjust the log message to indicate result was nil and
return the error response immediately without referencing result.IsValidDetails.
| export const handshakeAndGetUuid = async ( | ||
| wsUrl: string, | ||
| inputs: any, | ||
| proofType: "register" | "dsc" | "disclose" | "register_id" | "dsc_id", | ||
| circuitName: string | ||
| ): Promise<string> => { | ||
| const pubkey = key1.getPublic(true, "hex"); | ||
|
|
||
| const helloBody = { | ||
| jsonrpc: "2.0", | ||
| method: "openpassport_hello", | ||
| id: 1, | ||
| params: { | ||
| user_pubkey: [...Array.from(Buffer.from(pubkey, "hex"))], | ||
| uuid: v4(), | ||
| }, | ||
| }; | ||
|
|
||
| const ws = new WebSocket(wsUrl); | ||
|
|
||
| ws.on("open", async () => { | ||
| ws.send(JSON.stringify(helloBody)); | ||
| }); | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| ws.on("message", async (data) => { | ||
| let textDecoder = new TextDecoder(); | ||
| //@ts-ignore | ||
| let result = JSON.parse(textDecoder.decode(Buffer.from(data))); | ||
| console.log(result); | ||
| if (result.result.attestation !== undefined) { | ||
| const { userPubkey, serverPubkey, imageHash, verified } = await validatePKIToken( | ||
| Buffer.from(result.result.attestation).toString('utf-8') | ||
| ); | ||
| //check if key1 is the same as userPubkey | ||
| const key2 = ec.keyFromPublic(serverPubkey, "hex"); | ||
| const sharedKey = key1.derive(key2.getPublic()); | ||
|
|
||
| const endpoint = { | ||
| endpointType: "staging_celo", | ||
| endpoint: "0x3Dd6fc52d2bA4221E02ae3A0707377B56FEA845a", | ||
| }; | ||
| const encryptionData = encryptAES256GCM( | ||
| JSON.stringify({ | ||
| type: proofType, | ||
| onchain: true, | ||
| ...endpoint, | ||
| circuit: { | ||
| name: circuitName, | ||
| inputs: JSON.stringify(inputs), | ||
| }, | ||
| }), | ||
| Buffer.from(sharedKey.toString("hex").padStart(64, "0"), "hex") | ||
| ); | ||
| const submitBody = { | ||
| jsonrpc: "2.0", | ||
| method: "openpassport_submit_request", | ||
| id: 1, | ||
| params: { | ||
| uuid: result.result.uuid, | ||
| ...encryptionData, | ||
| }, | ||
| }; | ||
| ws.send(JSON.stringify(submitBody)); | ||
| } else { | ||
| ws.close(); | ||
|
|
||
| resolve(result.result); | ||
| } | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add timeout to WebSocket handshake.
The WebSocket connection and message handling lack timeout mechanisms. If the TEE service is unresponsive, the Promise will never resolve or reject, causing tests to hang indefinitely.
Add a timeout wrapper:
export const handshakeAndGetUuid = async (
wsUrl: string,
inputs: any,
proofType: "register" | "dsc" | "disclose" | "register_id" | "dsc_id",
circuitName: string,
timeoutMs: number = 30000 // 30 second default timeout
): Promise<string> => {
// ... existing code ...
return Promise.race([
new Promise<string>((resolve, reject) => {
// ... existing WebSocket logic with resolve/reject ...
}),
new Promise<string>((_, reject) =>
setTimeout(() => reject(new Error(`WebSocket handshake timeout after ${timeoutMs}ms`)), timeoutMs)
)
]);
};🤖 Prompt for AI Agents
In sdk/tests/ts-api/utils/helper.ts around lines 240 to 311, the WebSocket
handshake promise can hang indefinitely if the remote TEE is unresponsive; add a
timeout parameter (e.g., timeoutMs: number = 30000) to the function signature,
wrap the existing WebSocket logic promise in Promise.race with a timeout promise
that closes the ws and rejects with a clear timeout Error after timeoutMs,
ensure you register ws.on('error') to reject the promise immediately, clear the
timeout timer and remove listeners when the handshake resolves or rejects, and
always close the WebSocket on timeout or final rejection to avoid resource
leaks.
| export const getProofGeneratedUpdate = async ( | ||
| uuid: string | ||
| ): Promise<{ | ||
| created_at: string; | ||
| witness_generated_at: string; | ||
| proof_generated_at: string; | ||
| }> => { | ||
| const socket2 = io(WSS_URL, { transports: ["websocket"] }); | ||
| socket2.on("connect", () => { | ||
| socket2.emit("subscribe", uuid); | ||
| }); | ||
| socket2.on("error", (err) => { | ||
| console.error("Socket.IO error:", err); | ||
| }); | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| socket2.on("status", (data) => { | ||
| try { | ||
| if (data.status === 3 || data.status === 5) { | ||
| socket2.close(); | ||
| reject(`Proof generation failed: ${data.request_id}`); | ||
| } else if (data.status === 4) { | ||
| socket2.close(); | ||
| resolve(data); | ||
| } | ||
| } catch (e) { | ||
| console.error("Error parsing message:", e); | ||
| } | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add timeout to proof generation monitoring.
The Socket.IO connection lacks a timeout mechanism. If the proof generation service doesn't respond or the status update is never received, the Promise will hang indefinitely.
Add a timeout:
export const getProofGeneratedUpdate = async (
- uuid: string
+ uuid: string,
+ timeoutMs: number = 300000 // 5 minute default for proof generation
): Promise<{
created_at: string;
witness_generated_at: string;
proof_generated_at: string;
}> => {
const socket2 = io(WSS_URL, { transports: ["websocket"] });
socket2.on("connect", () => {
socket2.emit("subscribe", uuid);
});
socket2.on("error", (err) => {
console.error("Socket.IO error:", err);
});
- return new Promise((resolve, reject) => {
+ return Promise.race([
+ new Promise((resolve, reject) => {
socket2.on("status", (data) => {
try {
if (data.status === 3 || data.status === 5) {
socket2.close();
reject(`Proof generation failed: ${data.request_id}`);
} else if (data.status === 4) {
socket2.close();
resolve(data);
}
} catch (e) {
console.error("Error parsing message:", e);
}
});
- });
+ }),
+ new Promise((_, reject) =>
+ setTimeout(() => {
+ socket2.close();
+ reject(new Error(`Proof generation timeout after ${timeoutMs}ms`));
+ }, timeoutMs)
+ )
+ ]);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getProofGeneratedUpdate = async ( | |
| uuid: string | |
| ): Promise<{ | |
| created_at: string; | |
| witness_generated_at: string; | |
| proof_generated_at: string; | |
| }> => { | |
| const socket2 = io(WSS_URL, { transports: ["websocket"] }); | |
| socket2.on("connect", () => { | |
| socket2.emit("subscribe", uuid); | |
| }); | |
| socket2.on("error", (err) => { | |
| console.error("Socket.IO error:", err); | |
| }); | |
| return new Promise((resolve, reject) => { | |
| socket2.on("status", (data) => { | |
| try { | |
| if (data.status === 3 || data.status === 5) { | |
| socket2.close(); | |
| reject(`Proof generation failed: ${data.request_id}`); | |
| } else if (data.status === 4) { | |
| socket2.close(); | |
| resolve(data); | |
| } | |
| } catch (e) { | |
| console.error("Error parsing message:", e); | |
| } | |
| }); | |
| }); | |
| }; | |
| export const getProofGeneratedUpdate = async ( | |
| uuid: string, | |
| timeoutMs: number = 300000 // 5 minute default for proof generation | |
| ): Promise<{ | |
| created_at: string; | |
| witness_generated_at: string; | |
| proof_generated_at: string; | |
| }> => { | |
| const socket2 = io(WSS_URL, { transports: ["websocket"] }); | |
| socket2.on("connect", () => { | |
| socket2.emit("subscribe", uuid); | |
| }); | |
| socket2.on("error", (err) => { | |
| console.error("Socket.IO error:", err); | |
| }); | |
| return Promise.race([ | |
| new Promise((resolve, reject) => { | |
| socket2.on("status", (data) => { | |
| try { | |
| if (data.status === 3 || data.status === 5) { | |
| socket2.close(); | |
| reject(`Proof generation failed: ${data.request_id}`); | |
| } else if (data.status === 4) { | |
| socket2.close(); | |
| resolve(data); | |
| } | |
| } catch (e) { | |
| console.error("Error parsing message:", e); | |
| } | |
| }); | |
| }), | |
| new Promise((_, reject) => | |
| setTimeout(() => { | |
| socket2.close(); | |
| reject(new Error(`Proof generation timeout after ${timeoutMs}ms`)); | |
| }, timeoutMs) | |
| ) | |
| ]); | |
| }; |
🤖 Prompt for AI Agents
In sdk/tests/ts-api/utils/helper.ts around lines 313 to 343, the Promise created
by the Socket.IO listener can hang forever if no status update arrives; add a
timeout to reject the Promise after a configurable period (e.g., 30s). Implement
a timer started when the Promise is created that rejects with a clear timeout
error, and ensure the timer is cleared when resolving or rejecting; also
remove/cleanup the socket listeners and close the socket in all code paths
(resolve, reject, timeout, and socket error) to avoid leaks. Optionally make the
timeout duration a parameter or constant so tests can override it.
sdk/tests/unified-api.test.ts
Outdated
| import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData } from './utils.ts'; | ||
| import { getRevealedDataBytes } from '../core/src/utils/proof.js'; | ||
| import { packBytes, packBytesArray } from '../../common/src/utils/bytes.js'; | ||
| import { runGenerateVcAndDiscloseRawProof } from './ts-api/utils/helper.ts'; | ||
| import { hashEndpointWithScope } from '@selfxyz/common'; |
There was a problem hiding this comment.
🧩 Analysis chain
Normalize import specifiers to avoid ESM/TS module resolution pitfalls
Mixing .ts and .js extensions when importing TS sources often breaks under ts-node/ESM. Prefer extensionless (resolved by TS) or import compiled outputs consistently.
-import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData } from './utils.ts';
-import { getRevealedDataBytes } from '../core/src/utils/proof.js';
-import { packBytes, packBytesArray } from '../../common/src/utils/bytes.js';
-import { runGenerateVcAndDiscloseRawProof } from './ts-api/utils/helper.ts';
+import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData } from './utils';
+import { getRevealedDataBytes } from '../core/src/utils/proof';
+import { packBytes, packBytesArray } from '../../common/src/utils/bytes';
+import { runGenerateVcAndDiscloseRawProof } from './ts-api/utils/helper';Run to confirm TS config supports current import style (allowImportingTsExtensions or NodeNext):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching tsconfig and test runner setup..."
fd -a tsconfig.json | xargs -I{} sh -c 'echo "--- {} ---"; cat "{}"'
echo
echo "Searching for ts-node/tsx usage in scripts..."
cat package.json 2>/dev/null | jq -r '.scripts // {}'
echo
echo "Scanning for route registration in Go API..."
rg -n -C2 '/api/verify|/verify' sdk/tests/go-api -g '!**/vendor/**'Length of output: 12095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "sdk/core/src/utils:"
ls sdk/core/src/utils
echo
echo "common/src/utils:"
ls common/src/utils
echo
echo "sdk/tests/ts-api/utils:"
ls sdk/tests/ts-api/utils
echo
echo "sdk/tests:"
ls sdk/testsLength of output: 497
Use extensionless imports in unified-api.test.ts
Specifiers with “.ts” or “.js” extensions don’t match the actual files and will break resolution under ESM.
--- sdk/tests/unified-api.test.ts
-import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData } from './utils.ts';
-import { getRevealedDataBytes } from '../core/src/utils/proof.js';
-import { packBytes, packBytesArray } from '../../common/src/utils/bytes.js';
-import { runGenerateVcAndDiscloseRawProof } from './ts-api/utils/helper.ts';
+import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData } from './utils';
+import { getRevealedDataBytes } from '../core/src/utils/proof';
+import { packBytes, packBytesArray } from '../../common/src/utils/bytes';
+import { runGenerateVcAndDiscloseRawProof } from './ts-api/utils/helper';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData } from './utils.ts'; | |
| import { getRevealedDataBytes } from '../core/src/utils/proof.js'; | |
| import { packBytes, packBytesArray } from '../../common/src/utils/bytes.js'; | |
| import { runGenerateVcAndDiscloseRawProof } from './ts-api/utils/helper.ts'; | |
| import { hashEndpointWithScope } from '@selfxyz/common'; | |
| // sdk/tests/unified-api.test.ts | |
| import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData } from './utils'; | |
| import { getRevealedDataBytes } from '../core/src/utils/proof'; | |
| import { packBytes, packBytesArray } from '../../common/src/utils/bytes'; | |
| import { runGenerateVcAndDiscloseRawProof } from './ts-api/utils/helper'; | |
| import { hashEndpointWithScope } from '@selfxyz/common'; |
🤖 Prompt for AI Agents
In sdk/tests/unified-api.test.ts around lines 3 to 7, several imports use
explicit “.ts” or “.js” extensions which will break ESM resolution; remove the
extensions and import the modules extensionless (e.g. change './utils.ts' →
'./utils', '../core/src/utils/proof.js' → '../core/src/utils/proof',
'../../common/src/utils/bytes.js' → '../../common/src/utils/bytes',
'./ts-api/utils/helper.ts' → './ts-api/utils/helper') and ensure the resulting
paths match the actual module filenames/exports.
sdk/tests/utils.ts
Outdated
| try { | ||
| let dscCircuitName; | ||
| let dscUuid ; | ||
|
|
||
| const cscaTreeData = JSON.parse(cscaTree.data.data); | ||
| const dscInputs = generateCircuitInputsDSC(passportData, cscaTreeData); | ||
| if(ispassport){ | ||
| console.log("Generating DSC proof"); | ||
| dscCircuitName = getCircuitNameFromPassportData(passportData, "dsc"); | ||
| dscUuid = await handshakeAndGetUuid( | ||
| DSC_URL, | ||
| dscInputs, | ||
| "dsc", | ||
| dscCircuitName | ||
| ); | ||
| } else { | ||
| console.log("Generating DSC ID proof"); | ||
| dscCircuitName = getCircuitNameFromPassportData(passportData, "dsc_id"); | ||
| dscUuid = await handshakeAndGetUuid( | ||
| DSC_URL, | ||
| dscInputs, | ||
| "dsc_id", | ||
| dscCircuitName | ||
| ); | ||
| } | ||
|
|
||
| const dscData = await getProofGeneratedUpdate(dscUuid); | ||
| //pretty print the circuit name | ||
| console.log("\x1b[34m%s\x1b[0m", "dsc uuid:", dscUuid); | ||
| console.log("\x1b[34m%s\x1b[0m", "circuit:", dscCircuitName); | ||
| console.log( | ||
| "\x1b[34m%s\x1b[0m", | ||
| "witness generation duration:", | ||
| //@ts-ignore | ||
| (new Date(dscData.witness_generated_at) - new Date(dscData.created_at)) / | ||
| 1000, | ||
| " seconds" | ||
| ); | ||
| console.log( | ||
| "\x1b[34m%s\x1b[0m", | ||
| "proof generation duration:", | ||
| //@ts-ignore | ||
| (new Date(dscData.proof_generated_at) - | ||
| //@ts-ignore | ||
| new Date(dscData.witness_generated_at)) / | ||
| 1000, | ||
| " seconds" | ||
| ); | ||
| } catch (error) { | ||
| console.log("DSC is already generated or failed to generate:", error); | ||
|
|
||
| } |
There was a problem hiding this comment.
Silently swallowing DSC generation errors.
The try-catch block catches errors during DSC proof generation but only logs them without propagating the failure. If DSC generation fails, the function continues to register proofs, which may result in registration failures downstream.
Consider propagating the error or adding a flag to track DSC generation status:
try {
// ... DSC generation code ...
} catch (error) {
- console.log("DSC is already generated or failed to generate:", error);
+ console.error("DSC generation failed:", error);
+ throw new Error(`DSC generation failed: ${error instanceof Error ? error.message : String(error)}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| let dscCircuitName; | |
| let dscUuid ; | |
| const cscaTreeData = JSON.parse(cscaTree.data.data); | |
| const dscInputs = generateCircuitInputsDSC(passportData, cscaTreeData); | |
| if(ispassport){ | |
| console.log("Generating DSC proof"); | |
| dscCircuitName = getCircuitNameFromPassportData(passportData, "dsc"); | |
| dscUuid = await handshakeAndGetUuid( | |
| DSC_URL, | |
| dscInputs, | |
| "dsc", | |
| dscCircuitName | |
| ); | |
| } else { | |
| console.log("Generating DSC ID proof"); | |
| dscCircuitName = getCircuitNameFromPassportData(passportData, "dsc_id"); | |
| dscUuid = await handshakeAndGetUuid( | |
| DSC_URL, | |
| dscInputs, | |
| "dsc_id", | |
| dscCircuitName | |
| ); | |
| } | |
| const dscData = await getProofGeneratedUpdate(dscUuid); | |
| //pretty print the circuit name | |
| console.log("\x1b[34m%s\x1b[0m", "dsc uuid:", dscUuid); | |
| console.log("\x1b[34m%s\x1b[0m", "circuit:", dscCircuitName); | |
| console.log( | |
| "\x1b[34m%s\x1b[0m", | |
| "witness generation duration:", | |
| //@ts-ignore | |
| (new Date(dscData.witness_generated_at) - new Date(dscData.created_at)) / | |
| 1000, | |
| " seconds" | |
| ); | |
| console.log( | |
| "\x1b[34m%s\x1b[0m", | |
| "proof generation duration:", | |
| //@ts-ignore | |
| (new Date(dscData.proof_generated_at) - | |
| //@ts-ignore | |
| new Date(dscData.witness_generated_at)) / | |
| 1000, | |
| " seconds" | |
| ); | |
| } catch (error) { | |
| console.log("DSC is already generated or failed to generate:", error); | |
| } | |
| try { | |
| // ... DSC generation code ... | |
| } catch (error) { | |
| console.error("DSC generation failed:", error); | |
| throw new Error( | |
| `DSC generation failed: ${error instanceof Error ? error.message : String(error)}` | |
| ); | |
| } |
🤖 Prompt for AI Agents
In sdk/tests/utils.ts around lines 72 to 123, the catch block for DSC proof
generation merely logs the error and allows execution to continue, which can
cause downstream registration to run with missing/invalid proofs; change the
behavior to either rethrow the caught error or set and return a clear failure
flag so callers can abort registration: specifically, remove the silent swallow,
propagate the error (throw error) after logging or return an object/boolean
indicating DSC generation failed, and update any calling test code to check that
flag or handle the thrown error so registration does not proceed on DSC
generation failure.
sdk/tests/utils.ts
Outdated
| "self-playground" | ||
| ); | ||
|
|
||
| globalPassportData = await registerMockPassportOrEUid(secret, attestationId == "1"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use strict equality operator.
The equality check uses == instead of ===, which can lead to unexpected type coercion.
Apply this diff:
- globalPassportData = await registerMockPassportOrEUid(secret, attestationId == "1");
+ globalPassportData = await registerMockPassportOrEUid(secret, attestationId === "1");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| globalPassportData = await registerMockPassportOrEUid(secret, attestationId == "1"); | |
| globalPassportData = await registerMockPassportOrEUid(secret, attestationId === "1"); |
🤖 Prompt for AI Agents
In sdk/tests/utils.ts around line 279, the code uses a loose equality operator
(attestationId == "1"); change it to a strict equality (attestationId === "1")
to avoid type coercion—ensure attestationId is the expected type (string) or
coerce it explicitly before comparison if needed.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
sdk/tests/utils.ts (3)
309-309: Use strict equalityAvoid type coercion in critical control flow.
- globalPassportData = await registerMockPassportOrEUid(secret, attestationId == "1"); + globalPassportData = await registerMockPassportOrEUid(secret, attestationId === "1");
17-19: Fix TS/ESM import specifiers: remove .ts extensionsThese cause resolution failures under NodeNext/ESM and ts-node.
-} from "./ts-api/utils/helper.ts"; -import { DSC_URL, REGISTER_URL } from "./ts-api/utils/constant.ts"; +} from "./ts-api/utils/helper"; +import { DSC_URL, REGISTER_URL } from "./ts-api/utils/constant";
150-153: Don’t swallow DSC generation failuresSilently continuing after DSC failure can corrupt downstream registration flow. Propagate the error.
- } catch (error) { - console.log("DSC is already generated or failed to generate:", error); - - } + } catch (error) { + console.error("DSC generation failed:", error); + throw (error instanceof Error ? error : new Error(String(error))); + }sdk/tests/unified-api.test.ts (1)
3-5: Fix ESM import specifiers: remove explicit .ts/.js extensionsThese paths will break under NodeNext/ESM and ts-node. Import extensionless; let TS/Node resolve.
-import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData, setupTestDataAadhaar } from './utils.ts'; -import { getRevealedDataBytes } from '../core/src/utils/proof.js'; -import { packBytes, packBytesArray } from '../../common/src/utils/bytes.js'; +import { callAPI, compareAPIs, setupTestData, getTestData, getGlobalPassportData, getUserContextData, getInvalidUserContextData, setupTestDataAadhaar } from './utils'; +import { getRevealedDataBytes } from '../core/src/utils/proof'; +import { packBytes, packBytesArray } from '../../common/src/utils/bytes';sdk/tests/ts-api/utils/helper.ts (2)
240-311: Add WebSocket error handling and timeout to handshakeMissing timeout and
errorhandling can hang forever. Add a timeout param and reject on errors/close.-export const handshakeAndGetUuid = async ( +export const handshakeAndGetUuid = async ( wsUrl: string, inputs: any, proofType: "register" | "dsc" | "disclose" | "register_id" | "dsc_id" | "register_aadhaar", - circuitName: string + circuitName: string, + timeoutMs: number = 30000 ): Promise<string> => { @@ - return new Promise((resolve, reject) => { + return Promise.race<string>([ + new Promise((resolve, reject) => { ws.on("message", async (data) => { @@ ws.send(JSON.stringify(submitBody)); } else { ws.close(); resolve(result.result); } }); - }); + ws.on("error", (err) => { + try { ws.close(); } catch {} + reject(err); + }); + }), + new Promise((_, reject) => + setTimeout(() => { + try { ws.close(); } catch {} + reject(new Error(`WebSocket handshake timeout after ${timeoutMs}ms`)); + }, timeoutMs) + ) + ]);
313-343: Add timeout and robust error handling to proof status subscriptionPromise can hang indefinitely if no status event arrives.
-export const getProofGeneratedUpdate = async ( - uuid: string +export const getProofGeneratedUpdate = async ( + uuid: string, + timeoutMs: number = 300000 ): Promise<{ @@ - return new Promise((resolve, reject) => { + return Promise.race([ + new Promise((resolve, reject) => { socket2.on("status", (data) => { try { if (data.status === 3 || data.status === 5) { socket2.close(); reject(`Proof generation failed: ${data.request_id}`); } else if (data.status === 4) { socket2.close(); resolve(data); } } catch (e) { console.error("Error parsing message:", e); } }); - }); + socket2.on("disconnect", () => { + // Best-effort cleanup; let timeout handle if needed + }); + socket2.on("error", (err) => { + socket2.close(); + reject(err); + }); + }), + new Promise((_, reject) => + setTimeout(() => { + socket2.close(); + reject(new Error(`Proof generation timeout after ${timeoutMs}ms`)); + }, timeoutMs) + ) + ]);
🧹 Nitpick comments (4)
sdk/tests/unified-api.test.ts (1)
25-26: Avoid infinite test timeoutsthis.timeout(0) + network calls without per-call timeouts can hang CI indefinitely. Use a large but finite timeout and ensure request-level timeouts exist.
- this.timeout(0); + this.timeout(600000); // 10 minutesRecommend pairing with request-level timeouts in helpers (WS/Socket.IO/fetch). As per coding guidelines.
Also applies to: 243-244, 466-467
sdk/tests/ts-api/utils/helper.ts (2)
223-227: Fix invalid Buffer typing
Buffer<ArrayBuffer>is not a valid type. UseBuffer.-function encryptAES256GCM(plaintext: string, key: Buffer<ArrayBuffer>) { +function encryptAES256GCM(plaintext: string, key: Buffer) {
11-11: Normalize local import to extensionlessAvoid
.jsin TS import to prevent ESM resolution issues.-import { WSS_URL } from "./constant.js"; +import { WSS_URL } from "./constant";sdk/tests/utils.ts (1)
85-87: Set timeouts on axios calls to staging treesNetwork hiccups will otherwise hang the run.
- dscTree = await axios.get("http://tree.staging.self.xyz/dsc"); - cscaTree = await axios.get("http://tree.staging.self.xyz/csca"); + dscTree = await axios.get("http://tree.staging.self.xyz/dsc", { timeout: 15000 }); + cscaTree = await axios.get("http://tree.staging.self.xyz/csca", { timeout: 15000 }); @@ - dscTree = await axios.get("http://tree.staging.self.xyz/dsc-id"); - cscaTree = await axios.get("http://tree.staging.self.xyz/csca-id"); + dscTree = await axios.get("http://tree.staging.self.xyz/dsc-id", { timeout: 15000 }); + cscaTree = await axios.get("http://tree.staging.self.xyz/csca-id", { timeout: 15000 });Also applies to: 97-99
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
sdk/tests/package.json(1 hunks)sdk/tests/ts-api/utils/helper.ts(1 hunks)sdk/tests/unified-api.test.ts(1 hunks)sdk/tests/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdk/tests/package.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
sdk/tests/utils.tssdk/tests/unified-api.test.tssdk/tests/ts-api/utils/helper.ts
sdk/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
sdk/**/*.{ts,tsx,js,jsx}: Review SDK packages for:
- Public API design and stability
- Asynchronous flows and error handling
- Security and input validation
- Compatibility across environments
Files:
sdk/tests/utils.tssdk/tests/unified-api.test.tssdk/tests/ts-api/utils/helper.ts
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
sdk/tests/unified-api.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Test isPassportDataValid() with realistic synthetic passport data (never real user data)
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Test isPassportDataValid() with realistic synthetic passport data (never real user data)
Applied to files:
sdk/tests/utils.tssdk/tests/unified-api.test.ts
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Use actual imports from selfxyz/mobile-sdk-alpha in tests
Applied to files:
sdk/tests/unified-api.test.ts
🧬 Code graph analysis (3)
sdk/tests/utils.ts (2)
sdk/tests/ts-api/utils/helper.ts (3)
handshakeAndGetUuid(240-311)getProofGeneratedUpdate(313-343)runGenerateVcAndDiscloseRawProof(137-221)sdk/tests/ts-api/utils/constant.ts (2)
DSC_URL(3-3)REGISTER_URL(1-1)
sdk/tests/unified-api.test.ts (3)
sdk/tests/utils.ts (7)
callAPI(205-231)compareAPIs(233-294)getUserContextData(422-424)getInvalidUserContextData(426-428)setupTestData(300-333)getTestData(410-418)setupTestDataAadhaar(335-409)sdk/core/src/utils/proof.ts (1)
getRevealedDataBytes(42-58)common/src/utils/bytes.ts (2)
packBytes(94-108)packBytesArray(110-138)
sdk/tests/ts-api/utils/helper.ts (1)
sdk/tests/ts-api/utils/constant.ts (1)
WSS_URL(7-7)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: type-check
sdk/tests/ts-api/utils/helper.ts
Outdated
| const { userPubkey, serverPubkey, imageHash, verified } = await validatePKIToken( | ||
| Buffer.from(result.result.attestation).toString('utf-8') | ||
| ); | ||
| //check if key1 is the same as userPubkey | ||
| const key2 = ec.keyFromPublic(serverPubkey, "hex"); | ||
| const sharedKey = key1.derive(key2.getPublic()); | ||
|
|
||
| const endpoint = { | ||
| endpointType: "staging_celo", | ||
| endpoint: "0x3Dd6fc52d2bA4221E02ae3A0707377B56FEA845a", | ||
| }; | ||
| const encryptionData = encryptAES256GCM( | ||
| JSON.stringify({ | ||
| type: proofType, | ||
| onchain: true, | ||
| ...endpoint, | ||
| circuit: { | ||
| name: circuitName, | ||
| inputs: JSON.stringify(inputs), | ||
| }, | ||
| }), | ||
| Buffer.from(sharedKey.toString("hex").padStart(64, "0"), "hex") | ||
| ); |
There was a problem hiding this comment.
Validate TEE attestation before deriving ECDH
Proceeding when verified === false will likely throw (empty serverPubkey) and is unsafe. Check and reject early. Also fix keyFromPublic usage with Buffer.
- const { userPubkey, serverPubkey, imageHash, verified } = await validatePKIToken(
+ const { userPubkey, serverPubkey, imageHash, verified } = await validatePKIToken(
Buffer.from(result.result.attestation).toString('utf-8')
);
- //check if key1 is the same as userPubkey
- const key2 = ec.keyFromPublic(serverPubkey, "hex");
+ if (!verified) {
+ ws.close();
+ return reject(new Error('TEE attestation JWT verification failed'));
+ }
+ // Derive ECDH shared key
+ const key2 = ec.keyFromPublic(serverPubkey); // Buffer input, no encoding arg
const sharedKey = key1.derive(key2.getPublic());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { userPubkey, serverPubkey, imageHash, verified } = await validatePKIToken( | |
| Buffer.from(result.result.attestation).toString('utf-8') | |
| ); | |
| //check if key1 is the same as userPubkey | |
| const key2 = ec.keyFromPublic(serverPubkey, "hex"); | |
| const sharedKey = key1.derive(key2.getPublic()); | |
| const endpoint = { | |
| endpointType: "staging_celo", | |
| endpoint: "0x3Dd6fc52d2bA4221E02ae3A0707377B56FEA845a", | |
| }; | |
| const encryptionData = encryptAES256GCM( | |
| JSON.stringify({ | |
| type: proofType, | |
| onchain: true, | |
| ...endpoint, | |
| circuit: { | |
| name: circuitName, | |
| inputs: JSON.stringify(inputs), | |
| }, | |
| }), | |
| Buffer.from(sharedKey.toString("hex").padStart(64, "0"), "hex") | |
| ); | |
| const { userPubkey, serverPubkey, imageHash, verified } = await validatePKIToken( | |
| Buffer.from(result.result.attestation).toString('utf-8') | |
| ); | |
| if (!verified) { | |
| ws.close(); | |
| return reject(new Error('TEE attestation JWT verification failed')); | |
| } | |
| // Derive ECDH shared key | |
| const key2 = ec.keyFromPublic(serverPubkey); | |
| const sharedKey = key1.derive(key2.getPublic()); | |
| const endpoint = { | |
| endpointType: "staging_celo", | |
| endpoint: "0x3Dd6fc52d2bA4221E02ae3A0707377B56FEA845a", | |
| }; | |
| const encryptionData = encryptAES256GCM( | |
| JSON.stringify({ | |
| type: proofType, | |
| onchain: true, | |
| ...endpoint, | |
| circuit: { | |
| name: circuitName, | |
| inputs: JSON.stringify(inputs), | |
| }, | |
| }), | |
| Buffer.from(sharedKey.toString("hex").padStart(64, "0"), "hex") | |
| ); |
🤖 Prompt for AI Agents
In sdk/tests/ts-api/utils/helper.ts around lines 271-293, the code derives an
ECDH shared key and uses ec.keyFromPublic without first checking that the TEE
attestation was verified and that serverPubkey is present/valid; this can throw
or be unsafe. Fix by: after validatePKIToken, immediately check if verified is
truthy and serverPubkey is a non-empty hex string (and throw or return an error
if not); only then call ec.keyFromPublic using a Buffer for the public key
(e.g., ec.keyFromPublic(Buffer.from(serverPubkey, "hex"))) and proceed to derive
the shared key; this prevents deriving with an empty/invalid key and ensures
safe early rejection.
sdk/tests/utils.ts
Outdated
| // API testing utilities | ||
| export async function callAPI( | ||
| url: string, | ||
| requestBody: any | ||
| ): Promise<APIResponse> { | ||
| try { | ||
| const response = await fetch(`${url}/api/verify`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(requestBody), | ||
| }); | ||
|
|
||
| const data = await response.text(); | ||
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(data); | ||
| } catch { | ||
| parsedData = { rawResponse: data }; | ||
| } | ||
| return { | ||
| status: response.status, | ||
| data: parsedData, | ||
| success: true, | ||
| }; | ||
| } catch (error: any) { | ||
| return { status: 0, data: { error: error.message }, success: false }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add HTTP timeout to callAPI to prevent hangs
fetch without a timeout can stall the entire suite.
-export async function callAPI(
+export async function callAPI(
url: string,
requestBody: any
): Promise<APIResponse> {
try {
- const response = await fetch(`${url}/api/verify`, {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 30000); // 30s
+ const response = await fetch(`${url}/api/verify`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify(requestBody),
+ signal: controller.signal,
});
const data = await response.text();
+ clearTimeout(timeout);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // API testing utilities | |
| export async function callAPI( | |
| url: string, | |
| requestBody: any | |
| ): Promise<APIResponse> { | |
| try { | |
| const response = await fetch(`${url}/api/verify`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify(requestBody), | |
| }); | |
| const data = await response.text(); | |
| let parsedData; | |
| try { | |
| parsedData = JSON.parse(data); | |
| } catch { | |
| parsedData = { rawResponse: data }; | |
| } | |
| return { | |
| status: response.status, | |
| data: parsedData, | |
| success: true, | |
| }; | |
| } catch (error: any) { | |
| return { status: 0, data: { error: error.message }, success: false }; | |
| } | |
| } | |
| // API testing utilities | |
| export async function callAPI( | |
| url: string, | |
| requestBody: any | |
| ): Promise<APIResponse> { | |
| try { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 30000); // 30s | |
| const response = await fetch(`${url}/api/verify`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify(requestBody), | |
| signal: controller.signal, | |
| }); | |
| const data = await response.text(); | |
| clearTimeout(timeout); | |
| let parsedData; | |
| try { | |
| parsedData = JSON.parse(data); | |
| } catch { | |
| parsedData = { rawResponse: data }; | |
| } | |
| return { | |
| status: response.status, | |
| data: parsedData, | |
| success: true, | |
| }; | |
| } catch (error: any) { | |
| return { status: 0, data: { error: error.message }, success: false }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In sdk/tests/utils.ts around lines 204 to 231, the callAPI helper uses fetch
with no timeout which can stall tests; add an AbortController and a setTimeout
(configurable, e.g. 5000ms) to abort the request if it exceeds the timeout, pass
signal to fetch, clear the timeout when fetch completes, and catch the abort
error to return a clear timeout failure (status 0 and an error message
indicating timeout) while preserving existing error handling for other errors.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
sdk/tests/go-api/api/verify.go (2)
72-77: Limit JSON body and reject unknown fieldsRequest bodies remain unbounded and unknown JSON fields slip through, so a single huge POST can exhaust memory and bypass validation. Wrap the body with
http.MaxBytesReader(pick a sane cap, e.g. 1 MiB) and callDisallowUnknownFields()on the decoder before decoding.- var req VerifyRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + // Cap body size and reject stray fields + r.Body = http.MaxBytesReader(w, r.Body, 1<<20) + var req VerifyRequest + dec := json.NewDecoder(r.Body) + dec.DisallowUnknownFields() + if err := dec.Decode(&req); err != nil {
236-244: Fix nil-result dereference before accessing fields
resultcan benil; dereferencingresult.IsValidDetailswill panic under load and crash the handler. Split the checks so you bail out immediately whenresult == nil, then handle the invalid-details case separately.- if result == nil || !result.IsValidDetails.IsValid { - log.Printf("Verification failed - invalid result") + if result == nil { + log.Printf("Verification failed - nil result") w.WriteHeader(http.StatusInternalServerError) json.NewEncoder(w).Encode(VerifyResponse{ Status: "error", Result: false, Message: "Verification failed", - Details: result.IsValidDetails, }) return } + if !result.IsValidDetails.IsValid { + log.Printf("Verification failed - invalid result") + w.WriteHeader(http.StatusInternalServerError) + json.NewEncoder(w).Encode(VerifyResponse{ + Status: "error", + Result: false, + Message: "Verification failed", + Details: result.IsValidDetails, + }) + return + }sdk/tests/unified-api.test.ts (1)
3-13: Remove file extensions from imports to fix ESM/TS resolution.The explicit
.tsand.jsextensions in import paths will break module resolution under ESM. Remove all extensions and let the TypeScript compiler resolve them.-import { - callAPI, - compareAPIs, - setupTestData, - getProof, - getUserContextData, - getInvalidUserContextData, - setupTestDataAadhaar, -} from './utils.ts'; -import { getRevealedDataBytes } from '../core/src/utils/proof.js'; -import { packBytes, packBytesArray } from '../../common/src/utils/bytes.js'; +import { + callAPI, + compareAPIs, + setupTestData, + getProof, + getUserContextData, + getInvalidUserContextData, + setupTestDataAadhaar, +} from './utils'; +import { getRevealedDataBytes } from '../core/src/utils/proof'; +import { packBytes, packBytesArray } from '../../common/src/utils/bytes';sdk/tests/ts-api/utils/helper.ts (3)
247-323: Add timeout to WebSocket handshake to prevent indefinite hangs.The WebSocket connection lacks a timeout mechanism. If the TEE service is unresponsive, the Promise will never resolve or reject, causing tests to hang.
Wrap the Promise in
Promise.racewith a timeout that closes the WebSocket and rejects with a clear error after a configurable duration (e.g., 30 seconds).
278-286: Remove sensitive attestation data from logs and validate TEE attestation before proceeding.Two critical issues:
resultcontains the full attestation payload including tokens and PII, which must not be logged.- The code proceeds to derive ECDH keys without checking if
verified === true, which is unsafe and may throw with empty/invalid keys.As per coding guidelines.
ws.on('message', async data => { let textDecoder = new TextDecoder(); //@ts-ignore let result = JSON.parse(textDecoder.decode(Buffer.from(data))); if (result.result.attestation !== undefined) { - const { userPubkey, serverPubkey, imageHash, verified } = - await validatePKIToken( - Buffer.from(result.result.attestation).toString('utf-8'), - ); + const { userPubkey, serverPubkey, imageHash, verified } = await validatePKIToken( + Buffer.from(result.result.attestation).toString('utf-8'), + ); + + if (!verified) { + ws.close(); + reject(new Error('TEE attestation JWT verification failed')); + return; + } + //check if key1 is the same as userPubkey - const key2 = ec.keyFromPublic(serverPubkey, 'hex'); + const key2 = ec.keyFromPublic(Buffer.from(serverPubkey, 'hex'));
325-352: Add timeout to proof generation monitoring to prevent indefinite hangs.The Socket.IO connection lacks a timeout mechanism. If the proof generation service doesn't respond, the Promise will hang indefinitely.
Wrap the Promise in
Promise.racewith a configurable timeout (e.g., 300 seconds for proof generation) that closes the socket and rejects with a clear timeout error.sdk/tests/utils.ts (3)
94-144: Propagate DSC generation errors instead of silently continuing.The catch block logs DSC generation errors but allows execution to continue. If DSC proof generation fails, downstream registration may fail with confusing errors.
} catch (error) { - console.log('DSC is already generated or failed to generate:', error); + console.error('DSC generation failed:', error); + throw new Error(`DSC generation failed: ${error instanceof Error ? error.message : String(error)}`); }
296-299: Use strict equality operator to avoid type coercion.globalPassportData = await registerMockPassportOrEUid( secret, - attestationId == '1', + attestationId === '1', );
194-220: Add timeout to fetch to prevent test hangs.The
fetchcall lacks a timeout mechanism. If the API is unresponsive, tests will hang indefinitely.export async function callAPI( url: string, requestBody: any, ): Promise<APIResponse> { try { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 30000); // 30s const response = await fetch(`${url}/api/verify`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(requestBody), + signal: controller.signal, }); const data = await response.text(); + clearTimeout(timeout); let parsedData; try { parsedData = JSON.parse(data); } catch { parsedData = { rawResponse: data }; } return { status: response.status, data: parsedData, success: true, }; } catch (error: any) { + if (error.name === 'AbortError') { + return { status: 0, data: { error: 'Request timeout' }, success: false }; + } return { status: 0, data: { error: error.message }, success: false }; } }
🧹 Nitpick comments (1)
sdk/tests/unified-api.test.ts (1)
438-560: Consider adding missing Aadhaar test cases for completeness.The Aadhaar test suite has fewer test cases than Passport and EU ID Card suites. Consider adding:
- Forbidden countries list mismatch test
- Config not found test (only has ConfigID not found)
- Future timestamp test
- Old timestamp test
While not critical, these additional tests would ensure comprehensive coverage and parity validation across all attestation types.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
sdk/tests/ts-api/utils/assests/vc_and_disclose_aadhaar.wasmis excluded by!**/*.wasmyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
sdk/tests/go-api/api/verify.go(1 hunks)sdk/tests/package.json(1 hunks)sdk/tests/ts-api/utils/assests/verification_key_aadhaar.json(1 hunks)sdk/tests/ts-api/utils/helper.ts(1 hunks)sdk/tests/unified-api.test.ts(1 hunks)sdk/tests/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdk/tests/ts-api/utils/assests/verification_key_aadhaar.json
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/tests/package.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
sdk/tests/utils.tssdk/tests/unified-api.test.tssdk/tests/ts-api/utils/helper.ts
sdk/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
sdk/**/*.{ts,tsx,js,jsx}: Review SDK packages for:
- Public API design and stability
- Asynchronous flows and error handling
- Security and input validation
- Compatibility across environments
Files:
sdk/tests/utils.tssdk/tests/unified-api.test.tssdk/tests/ts-api/utils/helper.ts
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
sdk/tests/unified-api.test.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Test isPassportDataValid() with realistic synthetic passport data (never real user data)
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Test isPassportDataValid() with realistic synthetic passport data (never real user data)
Applied to files:
sdk/tests/utils.tssdk/tests/unified-api.test.ts
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to **/*.{js,ts,tsx,jsx,sol,nr} : ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g., `***-***-1234` for passport numbers, `J*** D***` for names).
Applied to files:
sdk/tests/utils.ts
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to **/*.{js,ts,tsx,jsx,sol,nr} : NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
Applied to files:
sdk/tests/utils.ts
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Use actual imports from selfxyz/mobile-sdk-alpha in tests
Applied to files:
sdk/tests/unified-api.test.ts
🧬 Code graph analysis (4)
sdk/tests/go-api/api/verify.go (5)
sdk/tests/go-api/config/config.go (3)
InMemoryConfigStore(33-37)NewKVConfigStoreFromEnv(93-95)SelfAppDisclosureConfig(13-24)sdk/sdk-go/common/constants.go (2)
PAK(175-175)IRN(112-112)sdk/sdk-go/utils.go (11)
Ofac(90-90)Passport(17-17)EUCard(18-18)Aadhaar(19-19)IssuingState(82-82)Name(83-83)Nationality(85-85)DateOfBirth(86-86)Gender(87-87)ExpiryDate(88-88)IdNumber(84-84)sdk/sdk-go/verifier.go (1)
NewBackendVerifier(90-133)sdk/sdk-go/types.go (2)
UserIDTypeUUID(66-66)IsValidDetails(25-29)
sdk/tests/utils.ts (3)
common/src/mock_certificates/aadhaar/mockAadhaarCert.ts (2)
AADHAAR_MOCK_PRIVATE_KEY_PEM(4-32)AADHAAR_MOCK_PUBLIC_KEY_PEM(34-56)sdk/tests/ts-api/utils/helper.ts (3)
handshakeAndGetUuid(247-323)getProofGeneratedUpdate(325-352)runGenerateVcAndDiscloseRawProof(140-228)sdk/tests/ts-api/utils/constant.ts (2)
DSC_URL(3-3)REGISTER_URL(1-1)
sdk/tests/unified-api.test.ts (3)
sdk/tests/utils.ts (7)
callAPI(194-220)compareAPIs(222-283)getUserContextData(460-462)getInvalidUserContextData(464-466)setupTestData(289-323)getProof(448-456)setupTestDataAadhaar(325-447)sdk/core/src/utils/proof.ts (1)
getRevealedDataBytes(42-58)common/src/utils/bytes.ts (2)
packBytes(94-108)packBytesArray(110-138)
sdk/tests/ts-api/utils/helper.ts (1)
sdk/tests/ts-api/utils/constant.ts (1)
WSS_URL(7-7)
🔇 Additional comments (1)
sdk/tests/unified-api.test.ts (1)
562-593: Appreciate the thorough documentation.The inline documentation of the public signals structure is excellent and makes the test mutations clear and reviewable.
|
@Nesopie can u review it ? |
Summary by CodeRabbit
New Features
Tests
Documentation
Chores