Demo of backend adaption #308
Conversation
- Introduced the @burnt-labs/abstraxion-backend package, including essential files such as package.json, tsconfig.json, and ESLint configuration. - Set up Jest for testing with a basic configuration. - Updated pnpm-lock.yaml to reflect new dependencies and versions. - Added README and CHANGELOG for documentation and version tracking.
… and encryption - Implemented core functionality for session key management, including storing, retrieving, and revoking session keys. - Added encryption services using AES-256-GCM for secure session key storage. - Created a modular architecture with database adapter interfaces for extensibility. - Introduced comprehensive test coverage for encryption and session key management functionalities. - Updated README and documentation to reflect new features and usage examples.
- Added validation for configuration parameters in the AbstraxionBackend constructor to ensure required fields are provided. - Implemented input validation for userId and other parameters in various methods to improve error handling. - Introduced new error types: UserIdRequiredError and UnknownError for better clarity in error reporting. - Updated error handling to throw specific errors instead of generic ones, enhancing debugging and user feedback.
- Removed the encryptionService property and its instantiation from the AbstraxionBackend class as it is no longer needed. - This change simplifies the class structure and improves maintainability.
- Changed Jest test environment from jsdom to node for better compatibility with backend testing. - Simplified session key generation by directly creating a wallet with a default HD path, removing the previous mnemonic generation method and related code. - Updated the session key object to reference the wallet's mnemonic directly.
- Reformatted code across multiple files for consistent style, including spacing and line breaks. - Updated comments to enhance clarity and maintainability. - Ensured consistent use of double quotes for strings throughout the codebase. - Improved the structure of import statements for better organization. - Enhanced readability of function parameters and object properties by aligning them properly.
🦋 Changeset detectedLatest commit: 7e2cda6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
… handling improvements - Integrated node-cache for improved state management with automatic cleanup. - Added comprehensive error handling with custom error classes for better clarity in error reporting. - Updated session key management to utilize the new error handling structure. - Enhanced configuration validation to ensure required fields are provided. - Introduced new error types for session key operations to improve debugging and user feedback.
- Introduced EncryptionService for AES-256-GCM encryption and decryption of session keys. - Added SessionKeyManager to manage session key lifecycle, including storage, retrieval, and revocation. - Updated AbstraxionBackend to utilize the new SessionKeyManager and adjusted import paths accordingly. - Enhanced test coverage for EncryptionService and SessionKeyManager functionalities.
- Changed import statements for crypto and util modules to use the node: prefix for better clarity and consistency across the codebase.
- Added patch version for @burnt-labs/tailwind-config. - Updated main and types fields in tailwind-config package.json to point to tailwind.config.ts. - Removed obsolete .env.example file from abstraxion-backend package.
- Added indexes for username and email in User model for improved query performance. - Changed sessionKeyExpiry type from Int to DateTime in SessionKey model for better date handling. - Updated AuditLog model to use DateTime for timestamp and added index for userId and timestamp. - Modified seed script to use Date objects for timestamps and added sessionKeyAddress in upsert conditions. - Introduced new methods in database adapter for revoking session keys and managing session key states. - Enhanced SessionKeyManager to handle session key creation, updates, and revocations more effectively. - Added comprehensive tests for wallet API and session key management functionalities.
- Introduced new methods for retrieving the last session key and active session keys in the database adapter. - Updated the session key management logic to utilize the new retrieval methods. - Refactored session key update and revocation methods to improve clarity and maintainability. - Adjusted session key info structure to allow optional createdAt and updatedAt fields. - Improved validation logic for session key info to reflect the updated structure.
- Introduced a comprehensive API middleware system for consistent error handling, rate limiting, and request validation across all API routes. - Added utility functions for standardized API responses, including success and error formats. - Created specific wrappers for health check and wallet operations to streamline API handler creation. - Enhanced type safety and maintainability with TypeScript support and structured context management. - Updated existing API routes to utilize the new middleware and wrapper functions for improved clarity and functionality.
…thods - Changed time handling in tests to use Date objects for better clarity and consistency. - Updated session key revocation method to accept session key address as a parameter. - Enhanced TestDatabaseAdapter with new methods for retrieving last and active session keys. - Refactored session key storage and update methods to support multiple session keys per user. - Improved audit log retrieval to handle Date objects correctly.
- Modified the test description to clarify that it initiates wallet connection for an existing user. - Added user creation step within the test to ensure the user exists before testing the wallet connection. - Removed the verification step for user creation as it is now handled within the test setup.
- Updated wallet test to utilize the public sessionKeyManager for better access and clarity. - Refactored session key management logic to streamline the retrieval of the sessionKeyManager instance. - Enhanced code readability by removing unnecessary import statements and simplifying the test setup.
- Changed sessionPermissions from an array to an object in multiple test cases for consistency and accuracy in session key management.
- Removed unnecessary imports from SessionKeyManager tests for improved clarity. - Changed sessionPermissions from an array to an object in TestDatabaseAdapter for consistency in session key management.
- Modified callbackSchema to include 'granted' and 'granter' fields for improved authorization handling. - Updated AbstraxionBackend to enforce required redirectUrl and treasury parameters. - Enhanced session key management by storing permissions and granter address during callback processing. - Refactored error handling to include new errors for missing redirectUrl and granter address. - Adjusted README to reflect changes in functionality and integration with frontend SDK.
- Added XION_REDIRECT_URL and XION_TREASURY to env.example for improved configuration. - Updated callbackSchema to include 'state' for better validation. - Enhanced AbstraxionBackend to validate presence of redirectUrl and treasury during initialization. - Refactored error handling to utilize centralized error types for consistency. - Improved session key management by incorporating new parameters in callback processing.
|
Summary
Testing
|
Can we make this more in line other Xion applications? |
| user.id, | ||
| context.request as any, | ||
| ); | ||
| const signer = await abstraxionAuth.getSigner( |
There was a problem hiding this comment.
You can use the Cosmwasm client here. Hopefully simpler for you.
const client = await abstraxionAuth.getCosmWasmClient()
client.sendTokens(....)There was a problem hiding this comment.
Emmm,
ESLint failed, looks like no this method
| // Convert amount to micro units | ||
| const amountNum = parseFloat(amount); | ||
| const microAmount = Math.floor(amountNum * 1_000_000).toString(); | ||
| const denomMicro = denom === "XION" ? "uxion" : "uusdc"; |
There was a problem hiding this comment.
We should really make the usdc IBC denoms a constant someplace.
Okay |
…Default method to directly return the gas price from xionGasValues
- Changed background colors to black and adjusted text colors to white across various components including HomePage, SignInPage, SignUpPage, ProfilePage, TransferComponent, and WalletComponent. - Updated loading indicators and error messages to match the new color scheme. - Enhanced button styles for better visibility and interaction feedback.
|
@codex Can you review this PR again? |
Pull Request Review: Backend Session Demo ApplicationOverviewThis PR introduces a comprehensive backend session management demo application for XION wallet connections. The implementation includes session key management, encryption, audit logging, and a full-featured API layer with Next.js frontend. ✅ Strengths1. Security Implementation
2. Code Architecture
3. Test Coverage
4. Documentation
🔴 Critical Issues1. Security: Timing Attack Vulnerability (apps/backend-session/src/lib/xion/security.ts:110-118)static generateSecureString(
length: number,
charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789",
): string {
let result = "";
for (let i = 0; i < length; i++) {
result += charset.charAt(Math.floor(Math.random() * charset.length));
}
return result;
}Issue: Using Fix: Replace with cryptographically secure random generation: static generateSecureString(
length: number,
charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789",
): string {
const bytes = randomBytes(length);
let result = "";
for (let i = 0; i < length; i++) {
result += charset.charAt(bytes[i] % charset.length);
}
return result;
}2. Security: Sensitive Data Logging (apps/backend-session/src/lib/xion/backend/AbstraxionBackend.ts:370-375)console.error(
"Error checking status:",
error instanceof Error ? error.message : String(error),
);Issue: Error messages may contain sensitive information (session keys, user IDs) that shouldn't be logged in production. Recommendation: Implement structured logging with sensitive data redaction. 3. Race Condition Risk (apps/backend-session/src/lib/xion/backend/services/SessionKeyManager.ts:356-379)const existingSessionKey = await this.databaseAdapter.getSessionKey(
userId,
sessionAddress,
);
if (!existingSessionKey) {
throw new SessionKeyNotFoundError(/*...*/);
}
if (existingSessionKey.sessionState === SessionState.PENDING) {
await this.databaseAdapter.updateSessionKeyWithParams(/*...*/);
}Issue: No database transaction wrapping the check-then-update pattern, which could lead to race conditions with concurrent requests. Recommendation: Use database transactions or implement optimistic locking with version numbers.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async getSessionKey( | ||
| userId: string, | ||
| sessionKeyAddress: string, | ||
| ): Promise<SessionKeyInfo | null> { | ||
| const sessionKey = await this.prisma.sessionKey.findUnique({ | ||
| where: { | ||
| userId, | ||
| sessionKeyAddress, | ||
| }, |
There was a problem hiding this comment.
The new Prisma adapter calls findUnique with where: { userId, sessionKeyAddress } even though the Prisma schema only marks sessionKeyAddress as unique (schema.prisma has no @@unique([userId, sessionKeyAddress])). Prisma Client rejects non-unique fields in findUnique/update filters, so any callback/disconnect path that hits getSessionKey (and the similar revoke/update methods) will throw a runtime validation error before reaching the database. Consider querying by the actual unique key or adding a compound unique constraint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Updated session key lookups to query by the unique
sessionKeyAddresswhile guarding against cross-user access and rewired revoke/update helpers to use filters compatible with Prisma’s unique constraints. apps/backend-session/src/lib/xion/database.tsL54-L183 - Added unit tests validating the unique-key lookup path and the filtered update logic for session keys in the Prisma database adapter. apps/backend-session/src/tests/lib/database.test.tsL1-L79
Testing
⚠️ pnpm --filter backend-session test -- --runTestsByPath apps/backend-session/src/__tests__/lib/database.test.ts(fails: jest not available in environment)
- add unit tests for PrismaDatabaseAdapter methods - Introduced comprehensive tests for session key management functions including getSessionKey, revokeSessionKey, and updateSessionKeyWithParams. - Implemented setup and teardown for a test database using Prisma. - Validated session key retrieval, revocation, and updates with user ID checks to ensure proper access control. - Enhanced error handling and cleanup processes in the test suite.
Code Review - PR #308: Backend Adaptation DemoThank you for this comprehensive PR adding the abstraxion-backend package and demo application! This is a substantial addition with well-structured code. I've conducted a thorough review focusing on code quality, security, performance, bugs, and test coverage. Overall AssessmentStrengths:
Concerns: Several security and correctness issues need attention before production use. Critical Issues (Must Fix Before Merge)1. Encryption Service - Improper AAD UsageFile: cipher.setAAD(salt);Issue: Using salt as Additional Authenticated Data (AAD) in GCM mode is cryptographically incorrect. AAD should be public metadata (like user ID), not cryptographic material like salt. Recommendation: Remove AAD usage or use proper authenticated data: // Option 1: Remove AAD entirely
// cipher.setAAD(salt); // Remove this line
// Option 2: Use proper AAD (public metadata)
const aad = Buffer.from(`userId:${userId}:purpose:sessionKey`);
cipher.setAAD(aad);2. Validation Type MismatchFile: if (
typeof sessionKeyInfo.sessionKeyExpiry !== "number" ||
sessionKeyInfo.sessionKeyExpiry <= 0
)
if (!Array.isArray(sessionKeyInfo.sessionPermissions)) {
return false;
}Issue:
Recommendation: if (
!(sessionKeyInfo.sessionKeyExpiry instanceof Date) ||
isNaN(sessionKeyInfo.sessionKeyExpiry.getTime())
)
if (typeof sessionKeyInfo.sessionPermissions !== 'object' ||
sessionKeyInfo.sessionPermissions === null) {
return false;
}3. Race Condition in Session Key RefreshFile: Issue: Multiple concurrent requests can trigger simultaneous session key refreshes, creating multiple pending keys. Recommendation: Add check for existing pending keys: // Check if refresh already in progress
const existingPending = await this.databaseAdapter.getLastSessionKeyInfo(userId);
if (existingPending?.sessionState === SessionState.PENDING) {
return existingPending;
}
const timeUntilExpiry = sessionKeyInfo.sessionKeyExpiry.getTime() - Date.now();
if (timeUntilExpiry <= this.refreshThresholdMs) {
const newSessionKey = await this.generateSessionKeypair();
await this.createPendingSessionKey(userId, newSessionKey);
return newSessionKey;
}4. Information Disclosure in Error MessagesFile: const sanitizedMessage = errorMessage
.replace(/sessionKey[^,\s]*/gi, "[REDACTED]")
.replace(/userId[^,\s]*/gi, "[REDACTED]")
.replace(/address[^,\s]*/gi, "[REDACTED]");Issue: Regex patterns too simple - can be bypassed by variations like "sessionKey_material", "sessionkey2", etc. Recommendation: Use more robust patterns or whitelist approach: const sanitizedMessage = errorMessage
.replace(/sessionKey\w*/gi, "[REDACTED]")
.replace(/userId[\w]*/gi, "[REDACTED]")
.replace(/address[\w]*/gi, "[REDACTED]")
.replace(/xion1[a-z0-9]{38,}/gi, "[ADDRESS_REDACTED]") // Bech32 addresses
.replace(/0x[a-fA-F0-9]{64,}/gi, "[KEY_REDACTED]"); // Hex keysHigh Priority Issues (Should Fix)5. Weak Random String GenerationFile: Issue: Modulo bias in random generation: Recommendation: static generateSecureString(length: number, charset = "..."): string {
let result = "";
const charsetLength = charset.length;
for (let i = 0; i < length; i++) {
// Use rejection sampling to avoid bias
let randomValue;
do {
randomValue = randomBytes(1)[0];
} while (randomValue >= 256 - (256 % charsetLength));
result += charset.charAt(randomValue % charsetLength);
}
return result;
}6. State Cleanup IncompleteFile: Issue: When authorization is denied, state is cleaned but pending session key remains in database. Recommendation: if (!request.granted) {
this._stateStore.del(request.state);
// Also clean up pending session key
try {
const lastKey = await this.sessionKeyManager.getLastSessionKeyInfo(userId);
if (lastKey?.sessionState === SessionState.PENDING) {
await this.sessionKeyManager.revokeSessionKey(userId, lastKey.sessionKeyAddress);
}
} catch (err) {
this.logError("Failed to cleanup pending session key", err);
}
return { success: false, error: "Authorization was not granted by user" };
}7. Floating-Point Financial CalculationsFile: Issue: Using floating-point arithmetic for financial amounts risks precision loss. Recommendation: Use integer-only arithmetic or decimal library: // Option 1: String-based conversion
const [whole, decimal = ""] = amount.split(".");
const paddedDecimal = decimal.padEnd(6, "0").slice(0, 6);
const microAmount = whole + paddedDecimal;
// Option 2: Use decimal.js
import Decimal from "decimal.js";
const microAmount = new Decimal(amount).mul(1_000_000).floor().toString();8. Missing CSRF Protection on CallbackFile: Issue: Callback endpoint lacks CSRF protection beyond state parameter. Recommendation: Add origin/referer validation: // Validate request origin
const origin = request.headers.get("origin");
const expectedOrigin = process.env.XION_DASHBOARD_URL;
if (origin && origin !== expectedOrigin) {
return apiError("Invalid request origin", 403);
}9. Database Connection Pool Not ConfiguredFile: Issue: PrismaClient created without explicit connection pool configuration. Recommendation: const prisma = new PrismaClient({
datasources: {
db: {
url: process.env.DATABASE_URL,
},
},
// Add connection pool configuration
pool: {
max: 10,
min: 2,
idleTimeoutMillis: 30000,
},
});10. Rate Limiting Header SpoofingFile: Issue: IP extraction from Recommendation: Add configuration option: function getClientIp(request: NextRequest): string {
const trustProxy = process.env.TRUST_PROXY === "true";
if (trustProxy) {
const forwarded = request.headers.get("x-forwarded-for");
if (forwarded) {
return forwarded.split(",")[0].trim();
}
}
// Fallback to direct connection
return request.ip || "unknown";
}Medium Priority Issues (Consider Fixing)11. Type Safety ViolationFile: const adapter = this.databaseAdapter as any;Recommendation: Use proper type guard: interface ExtendedDatabaseAdapter extends DatabaseAdapter {
updateSessionKeyWithParamsAtomic?: (params: UpdateParams) => Promise<void>;
}
const adapter = this.databaseAdapter as ExtendedDatabaseAdapter;
if (adapter.updateSessionKeyWithParamsAtomic) {
await adapter.updateSessionKeyWithParamsAtomic({...});
}12. N+1 Query PatternFile: Recommendation: Batch audit log insertions: const auditEntries = activeSessionKeys.map(key => ({
userId,
action: AuditAction.SESSION_KEY_REVOKED,
details: { sessionKeyAddress: key.sessionKeyAddress }
}));
await this.databaseAdapter.createAuditLogs(auditEntries); // Add batch method
await this.databaseAdapter.revokeActiveSessionKeys(userId);13. Inconsistent Password HashingFile: Recommendation: Standardize on bcrypt throughout: // Remove PBKDF2 implementation, use bcrypt everywhere
import bcrypt from 'bcryptjs';
static async hashPassword(password: string): Promise<string> {
return bcrypt.hash(password, 12);
}
static async verifyPassword(password: string, hash: string): Promise<boolean> {
return bcrypt.compare(password, hash);
}14. NodeCache Memory ConcernsFile: Recommendation: Add memory limit: this._stateStore = new NodeCache({
stdTTL: 600,
checkperiod: 60,
useClones: false,
maxKeys: 1000, // Reduced from 10000
deleteOnExpire: true,
});15. Raw SQL UsageFile: await this.prisma.$queryRaw`SELECT 1`;Recommendation: Use Prisma's built-in method: // Better health check
await this.prisma.$connect();Test Coverage GapsMissing Tests:
Recommendation: Add comprehensive integration test suite before production deployment. Documentation Improvements
Minor Issues / Nits
SummaryThis is a well-architected implementation with solid fundamentals. The critical issues are primarily around:
Once the critical and high-priority issues are addressed, this will be a robust backend solution for session management. Recommendation: Please address the 4 critical issues and at least issues #5-10 from high priority before merging. The remaining items can be tracked as follow-up work. Great work overall! Happy to discuss any of these findings. |
…n session key management - Improved error message redaction with more robust patterns to protect sensitive information. - Added a check in SessionKeyManager to return an existing pending session key instead of creating a new one, preventing potential race conditions. - Removed outdated validation functions from the validation utility file to streamline the codebase.
PR Review: Backend Session Demo ImplementationOverviewComprehensive PR adding backend session management demo with ~20K lines across 74 files. Strengths
Critical Issues to Fix Before Merge1. Race Condition (HIGH)Location: SessionKeyManager.ts:392-429 2. Database Security Issue (HIGH)Location: database.ts:68-71 3. Type Safety Bypass (MEDIUM)Location: SessionKeyManager.ts:369 4. Hardcoded Test Password (SECURITY)Location: prisma/seed.ts:10 Security Concerns
Performance Considerations
Test Coverage Gaps
Code Consistency
RecommendationsMust Fix Before Merge
Should Fix Soon
Nice to Have
Final VerdictHigh-quality implementation with strong foundations. Architecture is sound and code is well-tested. Please address the 4 critical issues above before merge. After fixes, ready to merge! Great work overall! 🎉 Review generated with Claude Code |
- Introduced helper functions for user creation and database cleanup to streamline test setup. - Updated wallet API tests to utilize the new helper functions, enhancing readability and maintainability. - Ensured proper user existence checks before session key creation in the database adapter. - Improved error handling in session key management to prevent operations on non-existent users.
PR Review: Backend Adaptation DemoOverviewThis PR introduces a comprehensive backend session management demo application for XION wallet connections with ~19,685 additions across 74 files. The implementation includes a new Next.js app with session key management, encryption, database integration, and extensive test coverage. ✅ StrengthsArchitecture & Design
Security Implementation
Code Quality
🔴 Critical Issues1. Race Condition in Session Key UpdatesLocation: SessionKeyManager.ts:368-427 The code implements atomic updates for Prisma but falls back to non-atomic updates for other adapters, creating a check-then-update race condition window. Impact: Multiple simultaneous callbacks could update the same session key. Recommendation: Either require atomic operations or implement distributed locking. 2. Missing Input Validation on Transaction AmountLocation: route.ts:48-56
Recommendation: Add regex validation for decimal format and check against permission limits. 3. Error Message Information DisclosureLocation: AbstraxionBackend.ts:373-381 Regex-based sanitization may not catch all variations. Server logs could capture sensitive data. Recommendation: Use structured logging with automatic PII redaction. 🟡 Bugs & Issues4. Session Key Expiry Check InconsistencyPENDING keys can expire but aren't checked. Keys never activated remain in limbo. Recommendation: Add expiry check for PENDING state and implement cleanup job. 5. Cache Size Limit10,000 max states with 10-min TTL = ~1,000 attempts/min max. Under load, legitimate requests could be denied. Recommendation: Add monitoring, consider Redis for production. 6. Encryption Key ValidationOnly checks length, not cryptographic strength. Recommendation: Add entropy validation. 🟠 Performance Considerations7. N+1 Queries in Audit LoggingEach operation logs individually without batching. Recommendation: Implement batch audit logging. 8. Encryption in Hot PathEvery getSessionKeypair call decrypts synchronously. Recommendation: Consider short-lived caching. 9. Missing Database IndexesAdd compound index on (userId, sessionState, sessionKeyExpiry) for common queries. ✅ Test CoverageExcellent coverage with 8 comprehensive test files covering API endpoints, encryption, session management, database operations, rate limiting, and security utilities. Missing tests: Integration tests for full OAuth flow, load testing, concurrency tests. 📋 Additional Recommendations
🎯 SummaryWell-architected backend session management system with strong security foundations, high code quality, and excellent test coverage. Must Fix Before Merge:
Should Fix Soon: Verdict: Approve with required changes. Foundation is solid for production with noted fixes. Great work on security details and comprehensive testing! 🎉 |
|
Created a separated demo repo for this usage: |
No description provided.