mirror of
https://github.com/Such-Software/wownerogue.git
synced 2026-11-13 02:23:56 +01:00
418 lines
12 KiB
Markdown
418 lines
12 KiB
Markdown
After a deep review of your Wowngeon codebase, I'll provide comprehensive feedback on production readiness and architectural improvements.
|
|
|
|
## 🔍 Deep Code Review Summary
|
|
|
|
### Critical Production Issues
|
|
|
|
#### 1. **Security Vulnerabilities**
|
|
```javascript
|
|
// src/network/socketHandlers.js - SQL Injection Risk
|
|
const query = `SELECT * FROM users WHERE socket_id = '${socketId}'`; // DANGEROUS
|
|
```
|
|
**Fix**: Always use parameterized queries (you mostly do, but found a few raw queries)
|
|
|
|
#### 2. **Missing Rate Limiting**
|
|
The chat system has basic cooldown but lacks proper rate limiting for:
|
|
- Payment request creation
|
|
- Address confirmations
|
|
- Game entry attempts
|
|
- WebSocket connections per IP
|
|
|
|
#### 3. **Insufficient Input Validation**
|
|
```javascript
|
|
// src/network/addressManager.js
|
|
detectAddress(message) {
|
|
// Only checks regex, no length validation or checksum verification
|
|
const xmrMatch = message.match(/\b[48AB][0-9A-Za-z]{94}\b/);
|
|
}
|
|
```
|
|
|
|
#### 4. **Memory Leaks**
|
|
- Movement manager's `_postMoveHooks` Map grows unbounded
|
|
- Session manager doesn't clean up expired tokens
|
|
- Payment handlers' confirmation tracking never expires old entries
|
|
|
|
### Architecture Improvements
|
|
|
|
#### 1. **Merge PAID_SINGLE and PAID_CREDITS**
|
|
Yes, this is absolutely feasible and recommended:
|
|
|
|
```javascript
|
|
// Proposed unified structure
|
|
class UnifiedPaymentMode {
|
|
constructor(config) {
|
|
this.modes = {
|
|
PAY_PER_GAME: {
|
|
enabled: config.PAY_PER_GAME_ENABLED,
|
|
price: config.GAME_PRICE,
|
|
payoutMultipliers: { escape: 2, treasure: 3 }
|
|
},
|
|
CREDITS: {
|
|
enabled: config.CREDITS_ENABLED,
|
|
packageSize: config.CREDITS_PACKAGE_SIZE,
|
|
packagePrice: config.CREDITS_PACKAGE_PRICE,
|
|
payoutsEnabled: config.CREDITS_PAYOUT_ENABLED
|
|
}
|
|
};
|
|
}
|
|
|
|
async processPayment(user, preferredMode) {
|
|
// Unified payment flow
|
|
if (preferredMode === 'CREDITS' && this.modes.CREDITS.enabled) {
|
|
return this.purchaseCredits(user);
|
|
} else if (this.modes.PAY_PER_GAME.enabled) {
|
|
return this.payPerGame(user);
|
|
}
|
|
throw new Error('No payment mode available');
|
|
}
|
|
}
|
|
```
|
|
|
|
#### 2. **Database Schema Issues**
|
|
```sql
|
|
-- Missing indexes on frequently queried columns
|
|
CREATE INDEX idx_games_user_id ON games(user_id);
|
|
CREATE INDEX idx_payments_status ON payments(status);
|
|
CREATE INDEX idx_users_anon_token ON users(anon_token); -- Already added, good!
|
|
|
|
-- Missing foreign key constraints
|
|
ALTER TABLE games ADD CONSTRAINT fk_games_user
|
|
FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
|
|
```
|
|
|
|
#### 3. **Error Handling Gaps**
|
|
```javascript
|
|
// Many async operations lack proper error boundaries
|
|
async handleGameStart(socket) {
|
|
try {
|
|
// ... game start logic
|
|
} catch (error) {
|
|
console.error('Game start failed:', error);
|
|
// Missing: User notification, rollback, metrics
|
|
socket.emit('error', { message: 'Game start failed' });
|
|
this.metrics.recordError('game_start', error);
|
|
await this.rollbackGameStart(socket.id);
|
|
}
|
|
}
|
|
```
|
|
|
|
### Production Readiness Checklist
|
|
|
|
#### ❌ **Critical - Must Fix**
|
|
- [ ] SQL injection vulnerabilities
|
|
- [ ] Rate limiting implementation
|
|
- [ ] Memory leak fixes
|
|
- [ ] Error recovery & rollback logic
|
|
- [ ] Payment confirmation idempotency
|
|
|
|
#### ⚠️ **Important - Should Fix**
|
|
- [ ] Unified PAID mode architecture
|
|
- [ ] WebSocket authentication tokens
|
|
- [ ] Database transaction boundaries
|
|
- [ ] Comprehensive logging (not just console.log)
|
|
- [ ] Health check endpoints
|
|
- [ ] Graceful shutdown handling
|
|
|
|
#### 💡 **Nice to Have**
|
|
- [ ] Metrics collection (Prometheus/StatsD)
|
|
- [ ] Circuit breakers for RPC calls
|
|
- [ ] Request tracing (correlation IDs)
|
|
- [ ] Admin dashboard
|
|
- [ ] A/B testing framework
|
|
|
|
## 🏗️ Proposed Refactoring Plan
|
|
|
|
### Phase 1: Security & Stability (1 week)
|
|
```javascript
|
|
// 1. Add comprehensive input validation
|
|
class ValidationService {
|
|
validateAddress(address, currency) {
|
|
// Length, charset, checksum validation
|
|
if (currency === 'WOW') {
|
|
return this.validateWowneroAddress(address);
|
|
}
|
|
return this.validateMoneroAddress(address);
|
|
}
|
|
|
|
validatePaymentAmount(amount) {
|
|
// Ensure positive integer, within bounds
|
|
const parsed = BigInt(amount);
|
|
if (parsed <= 0n || parsed > MAX_PAYMENT) {
|
|
throw new ValidationError('Invalid payment amount');
|
|
}
|
|
return parsed;
|
|
}
|
|
}
|
|
|
|
// 2. Implement rate limiting
|
|
class RateLimiter {
|
|
constructor(redis) {
|
|
this.redis = redis;
|
|
this.limits = {
|
|
'payment:create': { window: 60, max: 3 },
|
|
'game:start': { window: 60, max: 10 },
|
|
'chat:message': { window: 10, max: 5 }
|
|
};
|
|
}
|
|
|
|
async checkLimit(userId, action) {
|
|
const key = `rate:${userId}:${action}`;
|
|
const count = await this.redis.incr(key);
|
|
if (count === 1) {
|
|
await this.redis.expire(key, this.limits[action].window);
|
|
}
|
|
if (count > this.limits[action].max) {
|
|
throw new RateLimitError(`Too many ${action} requests`);
|
|
}
|
|
}
|
|
}
|
|
```
|
|
|
|
### Phase 2: Unified Payment System (3-4 days)
|
|
```javascript
|
|
// Unified payment configuration
|
|
const PAYMENT_CONFIG = {
|
|
enabled: true,
|
|
modes: {
|
|
direct: {
|
|
enabled: process.env.DIRECT_PAY_ENABLED === 'true',
|
|
price: BigInt(process.env.GAME_PRICE || '1e11'),
|
|
payouts: {
|
|
enabled: true,
|
|
escape: 2,
|
|
treasure: 3
|
|
}
|
|
},
|
|
credits: {
|
|
enabled: process.env.CREDITS_ENABLED === 'true',
|
|
packages: [
|
|
{ credits: 10, price: BigInt('5e11') },
|
|
{ credits: 25, price: BigInt('10e11') },
|
|
{ credits: 100, price: BigInt('35e11') }
|
|
],
|
|
payouts: {
|
|
enabled: process.env.CREDITS_PAYOUT_ENABLED === 'true'
|
|
}
|
|
}
|
|
}
|
|
};
|
|
```
|
|
|
|
### Phase 3: Monitoring & Observability (2-3 days)
|
|
```javascript
|
|
// Add comprehensive metrics
|
|
class MetricsCollector {
|
|
constructor() {
|
|
this.metrics = {
|
|
payments: new prometheus.Counter({
|
|
name: 'payments_total',
|
|
help: 'Total payments processed',
|
|
labelNames: ['status', 'mode']
|
|
}),
|
|
gameTime: new prometheus.Histogram({
|
|
name: 'game_duration_seconds',
|
|
help: 'Game duration in seconds',
|
|
buckets: [30, 60, 120, 300, 600]
|
|
}),
|
|
activeGames: new prometheus.Gauge({
|
|
name: 'active_games',
|
|
help: 'Currently active games'
|
|
})
|
|
};
|
|
}
|
|
}
|
|
|
|
// Add structured logging
|
|
const winston = require('winston');
|
|
const logger = winston.createLogger({
|
|
format: winston.format.json(),
|
|
transports: [
|
|
new winston.transports.File({ filename: 'error.log', level: 'error' }),
|
|
new winston.transports.File({ filename: 'combined.log' })
|
|
]
|
|
});
|
|
```
|
|
|
|
## 📊 Database Improvements
|
|
|
|
```sql
|
|
-- Add missing indexes
|
|
CREATE INDEX CONCURRENTLY idx_payments_created_at ON payments(created_at);
|
|
CREATE INDEX CONCURRENTLY idx_games_status_end ON games(status, ended_at);
|
|
CREATE INDEX CONCURRENTLY idx_users_last_seen ON users(last_seen);
|
|
|
|
-- Add audit table
|
|
CREATE TABLE audit_log (
|
|
id SERIAL PRIMARY KEY,
|
|
user_id INTEGER REFERENCES users(id),
|
|
action VARCHAR(50) NOT NULL,
|
|
details JSONB,
|
|
ip_address INET,
|
|
created_at TIMESTAMP DEFAULT NOW()
|
|
);
|
|
|
|
-- Add payment_attempts for idempotency
|
|
CREATE TABLE payment_attempts (
|
|
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
|
|
user_id INTEGER REFERENCES users(id),
|
|
amount BIGINT NOT NULL,
|
|
address VARCHAR(255) NOT NULL,
|
|
idempotency_key VARCHAR(255) UNIQUE,
|
|
created_at TIMESTAMP DEFAULT NOW()
|
|
);
|
|
```
|
|
|
|
## 🔒 Security Enhancements
|
|
|
|
```javascript
|
|
// Add request signing for critical operations
|
|
class SecurityMiddleware {
|
|
validateSignature(req, res, next) {
|
|
const signature = req.headers['x-signature'];
|
|
const timestamp = req.headers['x-timestamp'];
|
|
|
|
if (!signature || !timestamp) {
|
|
return res.status(401).json({ error: 'Missing signature' });
|
|
}
|
|
|
|
const age = Date.now() - parseInt(timestamp);
|
|
if (age > 30000) { // 30 seconds
|
|
return res.status(401).json({ error: 'Request expired' });
|
|
}
|
|
|
|
const expected = crypto
|
|
.createHmac('sha256', process.env.API_SECRET)
|
|
.update(`${timestamp}:${JSON.stringify(req.body)}`)
|
|
.digest('hex');
|
|
|
|
if (signature !== expected) {
|
|
return res.status(401).json({ error: 'Invalid signature' });
|
|
}
|
|
|
|
next();
|
|
}
|
|
}
|
|
```
|
|
|
|
## 🎮 Game Logic Improvements
|
|
|
|
```javascript
|
|
// Add deterministic game generation for replays/verification
|
|
class DeterministicGameGenerator {
|
|
constructor(seed) {
|
|
this.rng = new ROT.RNG();
|
|
this.rng.setSeed(seed);
|
|
}
|
|
|
|
generateDungeon(config) {
|
|
// Use seeded RNG for all random operations
|
|
const width = config.width;
|
|
const height = config.height;
|
|
const map = new ROT.Map.Digger(width, height, {
|
|
roomWidth: [3, 9],
|
|
roomHeight: [3, 5],
|
|
corridorLength: [3, 10],
|
|
dugPercentage: 0.2,
|
|
timeLimit: 1000
|
|
});
|
|
|
|
// Store seed for replay/verification
|
|
return {
|
|
map: map.create(),
|
|
seed: this.rng.getSeed(),
|
|
checksum: this.calculateChecksum(map)
|
|
};
|
|
}
|
|
}
|
|
```
|
|
|
|
## 🚀 Performance Optimizations
|
|
|
|
```javascript
|
|
// Add caching layer
|
|
class CacheService {
|
|
constructor(redis) {
|
|
this.redis = redis;
|
|
this.ttls = {
|
|
user: 300, // 5 minutes
|
|
gameState: 60, // 1 minute
|
|
blockHeight: 2 // 2 seconds
|
|
};
|
|
}
|
|
|
|
async getCachedUser(userId) {
|
|
const cached = await this.redis.get(`user:${userId}`);
|
|
if (cached) return JSON.parse(cached);
|
|
|
|
const user = await db.query('SELECT * FROM users WHERE id = $1', [userId]);
|
|
await this.redis.setex(
|
|
`user:${userId}`,
|
|
this.ttls.user,
|
|
JSON.stringify(user)
|
|
);
|
|
return user;
|
|
}
|
|
}
|
|
```
|
|
|
|
## 📝 Testing Improvements
|
|
|
|
```javascript
|
|
// Add integration tests
|
|
describe('Payment Flow Integration', () => {
|
|
let server, client, mockWallet;
|
|
|
|
beforeEach(async () => {
|
|
mockWallet = new MockWalletService();
|
|
server = await createServer({ wallet: mockWallet });
|
|
client = io(`http://localhost:${server.port}`);
|
|
});
|
|
|
|
test('complete payment flow', async () => {
|
|
// 1. Connect and get session
|
|
await client.connect();
|
|
const session = await waitForEvent(client, 'session_token');
|
|
|
|
// 2. Set payout address
|
|
client.emit('chat_message', 'WW1234...');
|
|
await waitForEvent(client, 'address_detected');
|
|
client.emit('chat_message', 'confirm');
|
|
await waitForEvent(client, 'address_confirmed');
|
|
|
|
// 3. Start game
|
|
client.emit('auto_start');
|
|
const payment = await waitForEvent(client, 'payment_created');
|
|
|
|
// 4. Simulate payment
|
|
await mockWallet.simulatePayment(payment.address, payment.amount);
|
|
await waitForEvent(client, 'payment_detected');
|
|
|
|
// 5. Confirm payment
|
|
await mockWallet.confirmPayment();
|
|
await waitForEvent(client, 'payment_confirmed');
|
|
await waitForEvent(client, 'game_start');
|
|
});
|
|
});
|
|
```
|
|
|
|
## 🎯 Immediate Action Items
|
|
|
|
1. **Fix SQL injection vulnerabilities** (2 hours)
|
|
2. **Add rate limiting** (4 hours)
|
|
3. **Fix memory leaks** (3 hours)
|
|
4. **Add database migrations for indexes** (1 hour)
|
|
5. **Implement unified payment mode** (8 hours)
|
|
6. **Add comprehensive error handling** (6 hours)
|
|
7. **Set up structured logging** (2 hours)
|
|
8. **Add health check endpoints** (1 hour)
|
|
|
|
## 📈 Long-term Roadmap
|
|
|
|
1. **Month 1**: Security, stability, unified payments
|
|
2. **Month 2**: Monitoring, admin dashboard, metrics
|
|
3. **Month 3**: Scaling (Redis cache, load balancing)
|
|
4. **Month 4**: Advanced features (tournaments, leaderboards)
|
|
|
|
The codebase shows good architectural foundations but needs these production hardening steps. The unified PAID mode is definitely the right direction—it will simplify both code and user experience.
|
|
|
|
Would you like me to start implementing any of these improvements immediately? I'd recommend beginning with the security fixes and unified payment mode. |