Generate type-safe api client from openapi - #12
Conversation
Co-authored-by: aram.devdocs <aram.devdocs@gmail.com>
|
Cursor Agent can help with this pull request. Just |
Code Review: Type-Safe API Client SDKExecutive SummaryOverall strong implementation with well-structured architecture, good separation of concerns, and adherence to CLAUDE.md standards. However, there are critical security issues that must be addressed before merging. Recommendation: Request changes for security issues, then approve after fixes. Critical Security Issues1. Secrets Committed to RepositorySeverity: CRITICAL Location:
Issue: Secret files are committed to the repository. While these appear to be test secrets, this violates security best practices and sets a dangerous precedent. Impact:
Recommendation: # Add .secrets/ to .gitignore
echo ".secrets/" >> web/packages/sdk/.gitignore
# Remove from git history (after merge, consider rewriting history)
git rm --cached web/packages/sdk/.secrets/*.txtAction Required: Remove secret files and ensure 2. localStorage Token Storage (XSS Vulnerability)Severity: HIGH Location: Issue: Session tokens and API keys stored in Current Code: localStorage.setItem('session_token', this.state.sessionToken);
localStorage.setItem('api_key', this.state.apiKey);Impact:
Recommendation:
Action Required: Add explicit security warnings to documentation and create follow-up security issue. 3. Missing Input ValidationSeverity: MEDIUM Location: Multiple files Issues:
Example: // encryption.ts - should validate apiKey format
export async function encryptData(data: string, apiKey: string): Promise<EncryptedPayload> {
const encoder = new TextEncoder();
// Missing: if (!isValidApiKey(apiKey)) throw new ValidationError(...)
const salt = crypto.getRandomValues(new Uint8Array(32));
// ...
}Recommendation: Add input validation at API boundaries using the existing Action Required: Add input validation before cryptographic operations and API calls. Performance Concerns4. Inefficient Base64 EncodingSeverity: MEDIUM Location: Issue: Using Current Code: return {
ciphertext: btoa(String.fromCharCode(...combined)), // Stack overflow risk
};Recommendation: // Use chunked encoding for large arrays
function arrayBufferToBase64(buffer: Uint8Array): string {
const CHUNK_SIZE = 0x8000; // 32KB chunks
let binary = '';
for (let i = 0; i < buffer.length; i += CHUNK_SIZE) {
const chunk = buffer.subarray(i, Math.min(i + CHUNK_SIZE, buffer.length));
binary += String.fromCharCode(...chunk);
}
return btoa(binary);
}Action Required: Replace spread operator with chunked encoding for production robustness. 5. No Request Deduplication for WebSocketSeverity: LOW Location: Issue: Multiple components can trigger duplicate subscriptions if they mount simultaneously. Recommendation: Add subscription deduplication logic to prevent sending multiple subscribe messages for the same event type. Code Quality Issues6. Inconsistent Error HandlingSeverity: MEDIUM Location: Multiple files Issue: Mix of throwing Examples:
Recommendation: Consistently use typed error classes throughout the codebase. Action Required: // encryption.ts:143-145 should be:
throw new EncryptionError('Decryption failed: Invalid API key or tampered ciphertext');
// AuthManager.ts:113 should be:
throw new AuthenticationError(error.error || 'Login failed');7. Missing TypeScript Strict ChecksSeverity: MEDIUM Location: Issue: Return types use Current Code: getHealth: async (): Promise<any> => { // Should be Promise<HealthCheckResponse>
const response = await fetch(`${this.config.baseUrl}/api/health`);
// ...
}Recommendation: Define proper TypeScript interfaces for all API responses, even if using the generated OpenAPI types. Action Required: Replace 8. Magic Numbers in CodeSeverity: LOW Location: Multiple files Issue: Hardcoded values violate CLAUDE.md principle of centralizing constants. Examples:
Recommendation: Create a constants file: // src/crypto/constants.ts
export const CRYPTO_CONSTANTS = {
SALT_SIZE_BYTES: 32,
IV_SIZE_BYTES: 12,
PBKDF2_ITERATIONS: 100_000,
AES_KEY_LENGTH: 256,
} as const;
// src/auth/constants.ts
export const AUTH_CONSTANTS = {
TOKEN_REFRESH_BUFFER_MS: 5 * 60 * 1000,
SESSION_STORAGE_KEYS: {
API_KEY: 'api_key',
SESSION_TOKEN: 'session_token',
USER_ID: 'user_id',
EXPIRES_AT: 'token_expires_at',
},
} as const;
// src/websocket/constants.ts
export const WS_CONSTANTS = {
PING_INTERVAL_MS: 30_000,
MAX_RECONNECT_DELAY_MS: 30_000,
MAX_RECONNECT_ATTEMPTS: 10,
} as const;Action Required: Extract magic numbers to constants module per CLAUDE.md standards. Positive ObservationsArchitecture
Security Implementation
Developer Experience
Code Organization
Testing GapsMissing Test CoverageIssue: PR description shows all test checkboxes unchecked. Zero test coverage is concerning for a crypto/auth-heavy SDK. Required Tests:
Action Required: Implement at minimum:
Documentation Issues9. Missing Security DocumentationLocation: README.md, package root Issue: No SECURITY.md file documenting:
Action Required: Create # Security Policy
## Known Vulnerabilities
### localStorage Token Storage (PoC Only)
**Severity**: High
**Status**: Known limitation, production migration required
**Description**: Current implementation stores tokens in localStorage, vulnerable to XSS attacks.
**Mitigation**: Production deployments must use HttpOnly cookies or secure token storage.
## Reporting Security Issues
[Contact information]
## Security Best Practices
[Guidelines for SDK users]10. Incomplete Migration GuideLocation: Issue: Migration guide lacks:
Recommendation: Enhance migration guide with side-by-side code comparisons. Build & Configuration11. Missing .secrets/ in .gitignoreLocation: Issue: Current .gitignore: Required Addition: Recommendations SummaryMust Fix Before Merge
Should Fix Before Merge
Nice to Have (Follow-up PRs)
Additional NotesCompliance with CLAUDE.md
Performance Considerations
Deployment ReadinessCurrent State: PoC quality, not production-ready Production Blockers:
Estimated Work to Production: 2-3 days (per completion summary) ConclusionThis is a well-architected SDK with excellent developer experience and clean code organization. The OpenAPI integration, type safety, and layered architecture are exemplary. However, the security issues (committed secrets, localStorage vulnerability) and lack of testing are blockers for production deployment. These issues must be addressed before merging to main branch. Recommended Action: Request changes, then re-review after security fixes and test implementation. Questions for Author
Review Status: Changes Requested Great work overall - the architecture and implementation quality are impressive. Address the security concerns and this will be ready to merge. |
Co-authored-by: aram.devdocs <aram.devdocs@gmail.com>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 21711695 | Triggered | Generic High Entropy Secret | 82e9b23 | web/packages/sdk/src/crypto/encryption.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
2 participants
Pull Request
Overview
Type: Feature
Summary: Implements a comprehensive, type-safe API client SDK (
@goudchain/sdk) from the OpenAPI specification, featuring automatic query management, client-side AES-256-GCM encryption/decryption, unified authentication, and a typed WebSocket client. This replaces manual API client code and significantly reduces frontend boilerplate.Related Issues: Closes #GC-175
Changes
Functionality
A new
@goudchain/sdkpackage has been created, providing a modular, layered API client. Key functionalities include:GoudChainclass for all API interactions, abstracting authentication and encryption.@goudchain/hooksusage to the new SDK.Affected Layers
System Impact
Blockchain: No changes to consensus, block structure, or validation logic. Client-side encryption/decryption has no impact on chain validation. WebSocket events enable reactive UI updates.
Cryptography: Introduces new client-side AES-256-GCM encryption/decryption and PBKDF2 key derivation. API keys are used for client-side crypto, not transmitted to the backend for encryption.
Storage: No RocksDB schema changes. LocalStorage is used for API key and session token persistence (PoC).
API: Frontend now consumes the backend API via a generated, type-safe client. Authentication logic is centralized within the SDK, handling both API key and JWT bearer tokens based on endpoint requirements.
Infrastructure: No changes to Docker, nginx, Terraform, or GCP deployment.
Frontend: Introduces a new
@goudchain/sdkpackage. Existing@goudchain/hookswill be migrated to use this SDK, significantly reducing boilerplate and improving type safety in the dashboard UI.Testing Performed
Unit Tests
Integration Tests
Security Tests
cargo auditpasses with no new vulnerabilities (N/A for TypeScript)Performance Tests
Manual Testing
baseUrlandwsUrl.sdk.auth.createAccount()andsdk.auth.login().sdk.data.submit()performs automatic encryption.sdk.data.listCollections()andsdk.data.decrypt()(with mock data).sdk.ws.connect()andsdk.ws.subscribe()for basic WebSocket connectivity (with mock events).useSubmitData,useListCollections,useDecryptCollection,useWebSocketEvents) integrate correctly within a React component structure.Code Quality
Pre-commit Checks
cargo fmtapplied (N/A for TypeScript)cargo clippy --all-targets --all-features -- -D warningspasses (N/A for TypeScript)pnpm type-checkpassespnpm buildpassesCode Review Checklist
Result<T, E>appropriately (TypeScriptPromise<T>withtry/catchand custom error classes).unwrap()in production code paths (No!assertions in TS)Arc<Mutex<T>>for shared state (N/A, TypeScript)Documentation
Code Documentation
External Documentation
@goudchain/sdkpackage)Breaking Changes
Description of Breaking Changes:
This is a new package. A
MIGRATION.mdguide is provided to transition from the old@goudchain/hookspackage.Deployment Considerations
Configuration Changes
baseUrlandwsUrl)Database Migration
Rollback Plan
Revert this PR. Frontend applications can continue to use the existing
@goudchain/hookspackage.Security Considerations
Client-side encryption ensures API keys are not transmitted for data encryption. JWT session tokens have automatic refresh. Current PoC uses
localStoragefor token persistence; production should considerHttpOnlycookies to mitigate XSS risks.Performance Impact
Leverages TanStack Query for automatic request deduplication, caching, and background refetching, leading to improved perceived performance. Generated code is tree-shakeable. Encryption/decryption operations are designed to be fast (<10ms for typical payloads).
Screenshots (if UI changes)
N/A
Additional Context
This SDK significantly improves developer experience by providing a single source of truth for API contracts, eliminating manual type definitions and boilerplate, and centralizing complex logic like authentication and encryption. It lays the groundwork for a more robust and maintainable frontend.
Reviewer Notes
src/crypto/encryption.tsimplementation for cryptographic best practices.Linear Issue: GC-175