SECURITY: Fix token refresh race condition (nb-6yd)

[ebrett]

Summary

• CRITICAL SECURITY FIX: Thread-safe token refresh using Monitor synchronization
• Prevents authentication failures in multi-threaded environments
• Implements double-check locking pattern for optimal performance
• Comprehensive concurrency test coverage

Problem

The ensure_fresh_token! method in HttpClient was not thread-safe. In multi-threaded environments (Puma, Sidekiq), multiple threads could detect an expired token simultaneously and all attempt to refresh it, causing:

• Multiple wasteful OAuth refresh requests
• Authentication failures due to race conditions
• Increased risk of hitting rate limits
• Potential token invalidation issues

Solution

Added Monitor (reentrant mutex) synchronization with double-check locking:

Fast Path (no lock)

• Check if token is expired
• If fresh, return immediately without locking
• This is the common case and has minimal overhead

Slow Path (with lock)

• Acquire mutex lock
• Double-check token expiry (another thread may have refreshed it)
• Only refresh if still expired
• Ensures only one thread performs the actual refresh

Implementation Details

def ensure_fresh_token!
  token_data = @token_adapter.retrieve_token(@identifier)
  return unless token_data

  # Fast path: check without locking
  return unless OAuth.token_expired?(token_data[:expires_at])

  # Slow path: acquire lock and double-check
  @token_refresh_mutex.synchronize do
    token_data = @token_adapter.retrieve_token(@identifier)
    return unless token_data
    return unless OAuth.token_expired?(token_data[:expires_at])

    refresh_token!(token_data[:refresh_token])
  end
end

Testing

New Concurrency Test Suite

Created spec/nationbuilder_api/http_client_concurrency_spec.rb:

Test 1: Verifies mutex prevents race condition

• Launches 10 threads with expired token
• All threads detect expiry simultaneously
• Result: Only 1 OAuth refresh call (not 10)

Test 2: Verifies normal requests remain concurrent

• Launches 10 threads with fresh token
• Result: Requests execute concurrently (no serialization)

All tests passing: 2 new examples, 0 failures

Security Impact

Before: Race condition could cause authentication failures in production
After: Thread-safe token refresh, reliable in multi-threaded environments

Performance Impact

• Negligible overhead for fresh tokens (fast path, no locking)
• Minimal overhead for expired tokens (one lock acquisition)
• No serialization of normal requests (only token refresh is synchronized)

References

• Fixes: nb-6yd (CRITICAL priority)
• Related: Comprehensive security audit findings
• Pattern: Double-checked locking

🤖 Generated with Claude Code