feat(auth): Implement SEP-10 Challenge-Response Authentication#20
Conversation
Detailed changes: - Created RedisService for secure nonce management with 5-minute TTLs. - Updated AuthService to handle complete SEP-10 Challenge and Verification lifecycle: - Generates 32-byte entropy nonces for challenge transactions. - Correctly applies network passphrases and auth server keys dynamically (Mainnet vs Testnet). - Validates single-use nonces via Redis. - Tolerates up to 60s of clock skew during timeBounds verification (logs at >30s). - Implemented robust network resilience for Horizon API queries: - Added strict 5s timeout limits on all account requests. - Implemented 2x exponential backoff retries (1s, 2s). - Added a Circuit Breaker that triggers a 30s pause after 3 consecutive errors. - Secured verification endpoint natively via Redis rate-limiting (max 10 attempts / wallet / minute). - Rewrote testing suite to thoroughly evaluate edge cases including mock Horizon API delays, expired transactions, invalid signatures, rate-limit thresholds, and circuit breaker activation, attaining >84% code coverage in auth.service.ts. Closes issue: [SEC] Implement SEP-10 Challenge-Response Authentication
- Auto-format auth.service.ts and auth.service.spec.ts with prettier - Remove unused imports (BadRequestException, jwtService, redisService)
|
Hi @Queenode! Thanks for the SEP-10 implementation — the approach is solid. CI is failing due to lint errors. Here's how to fix: 1. Run prettier to auto-fix formatting: 2. Fix unused variables in
After these changes, run |
|
Hi @Queenode! I compared your implementation against issue #4 requirements. Here's the full checklist: Requirement Coverage
Missing Test ScenariosThe issue explicitly requires these tests:
Code Issues
CI StatusCI was failing on lint (prettier formatting + unused vars). I pushed a fix commit earlier. Please verify the lint passes after pulling the latest. |
- Add missing test scenarios (wrong passphrase, forged signature, clock skew, horizon timeout) - Replace local RedisService with a shared ioredis module to resolve duplication - Fix rate limiting to only increment counters on failure - Remove redundant Math.max on nonce bytes - Cache server keypair generation
|
thank you @oomokaro1 I've successfully addressed all the feedback points from the PR review |
|
Hey @Queenode, thanks for the great work on this PR! I noticed a small issue in Here's the fix:
// Remove:
import { RedisModule } from '../redis/redis.module';
// Add:
import { RedisService } from './redis.service';
// Change:
providers: [AuthService, JwtStrategy],
// To:
providers: [AuthService, JwtStrategy, RedisService],Once you apply this change, CI should pass. Let me know if you have any questions! |
oomokaro1
left a comment
There was a problem hiding this comment.
LGTM! This is a solid implementation of SEP-10 Challenge-Response authentication. Great work on the circuit breaker, rate limiting, and comprehensive test coverage.
|
Thanks for the awesome contribution @Queenode! This SEP-10 implementation is solid — great job on the circuit breaker, rate limiting, and thorough test coverage. I'll handle the |
Closes #4
Detailed changes: