Refactor viewer stability

[jwt625]

No description provided.

[claude]

PR Review: Refactor viewer stability

This PR implements three major improvements: viewer stability fixes, short-lived API token authentication, and ephemeral TURN credentials. Overall, this is a well-executed security and stability enhancement with thorough testing and documentation. Below is detailed feedback:

---

✅ Strengths

1. Security Improvements

• Short-lived scoped tokens: Excellent security upgrade replacing long-lived VITE_FILE_SERVER_TOKEN with ephemeral tokens (5min TTL)
• IP binding: Tokens are bound to client IP addresses, preventing token replay attacks from different locations
• Scope-based authorization: Clean separation of permissions (files:read, files:write, python:execute, turn:read)
• Ephemeral TURN credentials: Using coturn REST API auth correctly with time-based credentials
• Backward compatibility: Smart fallback to AUTH_TOKEN for operational tools and static TURN credentials for legacy setups

2. Code Quality

• Viewer stability fixes properly address event listener lifecycle issues
• Clean separation of concerns with dedicated auth module (server/auth.js)
• Good use of middleware pattern for authentication
• Proper cache invalidation with 30s refresh buffer to avoid expired token usage

3. Test Coverage

• Comprehensive test suite covering all new functionality (7 new test files, 109 total tests)
• Tests cover token caching, expiry, scope validation, IP mismatch, and TURN credential flows
• All validation passes (pnpm check, pnpm test)

4. Documentation

• Excellent DevLog documenting the entire implementation process
• Updated README and environment configuration files
• Clear comments explaining backward compatibility paths

---

⚠️ Issues & Concerns

Security

1. Timing Attack Vulnerability in Token Verification (server/auth.js:70)

if (signature !== expectedSignature) return { valid: false, reason: "Invalid token signature" };

String comparison is vulnerable to timing attacks. Use constant-time comparison:

if (signature.length !== expectedSignature.length || 
    !crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(expectedSignature))) {
    return { valid: false, reason: "Invalid token signature" };
}

2. IP Normalization Can Be Bypassed (server/auth.js:23-31)

The normalizeIp function trusts x-forwarded-for header unconditionally. If the server isn't behind a trusted proxy, attackers can spoof this header:

const forwarded = req.headers["x-forwarded-for"];

Recommendation: Only trust proxy headers when behind a known proxy. Consider using a library like express-ipware or adding configuration to specify trusted proxies.

3. Rate Limiting Per-IP Can Be Bypassed

Since IP extraction trusts x-forwarded-for, rate limiting can be circumvented by spoofing the header. This affects:

• rateLimitApiTokenIssuance in server.js
• rateLimitExecution in pythonExecutor.js
• rateLimitUploads in fileStorage.js

4. No Token Revocation Mechanism

Once issued, tokens remain valid until expiry even if compromised. Consider:

• Adding a token revocation list (simple in-memory Set for blacklisted tokens)
• Or using a nonce/jti (JWT ID) and tracking used tokens

Performance

5. Token Cache Memory Leak (src/lib/api/authTokenClient.ts:13)

const tokenCache = new Map<string, CachedToken>();

The cache grows unbounded. If a client requests many different scope combinations, memory usage increases indefinitely. Recommendation: Add cache size limit or implement LRU eviction.

6. Synchronous IP Normalization Called on Every Request

The normalizeIp() function is called twice per request (once for token generation, once for validation). Consider caching the result on the req object.

Code Quality

7. Inconsistent Error Handling (server/turnCredentials.js:46-49)

error instanceof Error ? error.message : "TURN credential service unavailable"

This pattern is repeated but inconsistent with other error handling in the codebase. Consider creating a shared error handler utility.

8. Magic Numbers (src/lib/api/authTokenClient.ts:14)

const TOKEN_REFRESH_BUFFER_MS = 30_000; // refresh 30s before expiry

Consider making this configurable or calculating it as a percentage of TTL (e.g., 10% of token lifetime).

9. Missing Input Validation (server/server.js:714)

const requestedScopes = Array.isArray(req.body?.scopes) ? req.body.scopes : [];

No validation that scopes are strings or reasonable length. Malicious input could cause issues downstream.

Documentation

10. Environment Variable Documentation Incomplete

server/.env.example added new variables but missing details:

• What happens if TURN_SHARED_SECRET is not set? (Server responds with 503)
• Security implications of using AUTH_TOKEN as fallback for API_TOKEN_SECRET

---

🔍 Potential Bugs

11. Race Condition in Token Caching (src/lib/api/authTokenClient.ts:24-31)

If multiple requests for the same scope occur simultaneously before the first token fetch completes, all will initiate separate fetches:

if (cached && cached.expiresAt - TOKEN_REFRESH_BUFFER_MS > now) {
    return cached.token;
}
// Multiple concurrent calls reach here before any completes
const response = await fetch(...);

Recommendation: Use a promise cache to deduplicate concurrent requests.

12. TURN Credentials Cache Not Cleared (src/lib/api/turnCredentialsClient.ts:11)

let cachedTurnCredentials: TurnCredentialsResponse | null = null;

Module-level cache persists across all sessions. If credentials expire and server is unreachable, stale credentials remain cached. Consider adding error handling to clear cache on fetch failure.

13. Async YjsProvider.connect() Breaking Change

Changed from synchronous to async (line 183 of diff), which could break existing code that doesn't await it. The PR properly updates all call sites, but this is a breaking API change that should be noted for any external consumers.

---

📝 Minor Issues

14. Unused Import - node:crypto prefix is used in turnCredentials.js but not in auth.js (inconsistent)
15. Console.log for Production - server.js and turnCredentials.js use console.log for logging; consider a proper logging library
16. Hard-coded Domain - signaling.gdsjam.com appears in multiple places; consider centralizing configuration

---

🎯 Recommendations

High Priority

1. Fix timing attack vulnerability in token verification
2. Add proper IP validation/trusted proxy configuration
3. Fix token cache race condition
4. Add token cache size limits

Medium Priority

5. Implement token revocation mechanism
6. Add comprehensive input validation for API endpoints
7. Document security model and threat assumptions
8. Consider adding request IDs for better debugging

Low Priority

9. Centralize configuration constants
10. Improve error handling consistency
11. Add structured logging

---

✨ Summary

This is a high-quality PR that significantly improves the security posture of the application. The implementation is well-tested and documented. The main concerns are:

1. Critical: Timing attack vulnerability in HMAC comparison
2. Important: IP spoofing vulnerability in rate limiting and token binding
3. Moderate: Cache-related memory leaks and race conditions

Once the security issues are addressed, this PR will be ready to merge. The ephemeral credentials architecture is sound and follows best practices for TURN/token management.

Test Coverage: ✅ Excellent (109 tests, all passing)
Documentation: ✅ Very good
Security: ⚠️ Needs fixes before merge
Code Quality: ✅ Good overall structure

Great work on this comprehensive refactor! 🚀