Bug: Fixed Security Vulnerabilities#24
Conversation
🚀 Deployment PreviewBranch: To deploy:
Deployment URL: https://solstice-api.azurewebsites.net |
There was a problem hiding this comment.
Pull request overview
This pull request addresses security vulnerabilities in the backend authentication and caching systems. The changes improve production safety by replacing blocking Redis operations, enforcing strong JWT secrets, preventing token type confusion attacks, and removing development-only endpoints.
Changes:
- Replaced Redis KEYS command with SCAN-based iteration to prevent blocking in production
- Added JWT_SECRET validation requiring minimum 32-character length in production environments
- Implemented token type validation to prevent access/refresh token confusion
- Removed unused imports and development-only debugging endpoint
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/utils/redis.js | Replaced blocking KEYS command with non-blocking SCAN for pattern-based cache invalidation |
| backend/src/utils/jwt.js | Added JWT_SECRET strength validation and token type checking in verifyToken |
| backend/src/routes/proof.js | Removed unused cache import |
| backend/src/routes/identity.js | Updated cache structure to use verificationTimestamp instead of createdAt |
| backend/src/routes/challenges.js | Removed development-only endpoint for listing all challenges |
| backend/src/routes/auth.js | Removed unused authenticateJWT and challengeManager imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async invalidatePattern(pattern) { | ||
| const client = getRedisClient(); | ||
| const keys = await client.keys(`cache:${pattern}`); | ||
| if (keys.length > 0) { | ||
| await client.del(keys); | ||
| logger.debug(`Invalidated ${keys.length} cache entries for pattern: ${pattern}`); | ||
| const matchPattern = `cache:${pattern}`; | ||
| let cursor = 0; | ||
| let totalDeleted = 0; | ||
| const batchSize = 1000; | ||
|
|
||
| do { | ||
| const reply = await client.scan(cursor, { | ||
| MATCH: matchPattern, | ||
| COUNT: batchSize | ||
| }); | ||
| cursor = Number(reply.cursor); | ||
| const keys = reply.keys; | ||
|
|
||
| if (keys && keys.length > 0) { | ||
| const deleted = await client.del(keys); | ||
| totalDeleted += deleted; | ||
| } | ||
| } while (cursor !== 0); | ||
|
|
||
| if (totalDeleted > 0) { | ||
| logger.debug(`Invalidated ${totalDeleted} cache entries for pattern: ${pattern}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The SCAN implementation should include error handling to prevent the application from crashing if Redis operations fail during cache invalidation. Consider wrapping the SCAN loop in a try-catch block and logging any errors that occur.
| verificationTimestamp: identity.verification_timestamp, | ||
| attributesVerified: identity.attributes_verified |
There was a problem hiding this comment.
This change replaces 'createdAt' with 'verificationTimestamp' in the API response. This is a breaking change that will affect API consumers. The 'verificationTimestamp' field can be null for unverified identities, whereas 'createdAt' would always have a value. Ensure that API consumers and frontend code are updated to handle this change, or consider including both fields during a transition period to maintain backward compatibility.
No description provided.