diff --git a/.beads/deletions.jsonl b/.beads/deletions.jsonl index 037a37f..4db060c 100644 --- a/.beads/deletions.jsonl +++ b/.beads/deletions.jsonl @@ -1,2 +1,3 @@ -{"id":"nb-bl3","ts":"2025-12-04T02:21:40.476879Z","by":"git-history-backfill","reason":"recovered from git history (pruned from manifest)"} -{"id":"nb-6yd","ts":"2025-12-04T02:21:40.496433Z","by":"git-history-backfill","reason":"recovered from git history (pruned from manifest)"} +{"id":"nb-vb7","ts":"2025-12-02T11:08:36.201308Z","by":"git-history-backfill","reason":"recovered from git history (pruned from manifest)"} +{"id":"nb-024","ts":"2025-12-02T11:08:36.210645Z","by":"git-history-backfill","reason":"recovered from git history (pruned from manifest)"} +{"id":"nb-xdu","ts":"2025-12-02T11:08:36.215919Z","by":"git-history-backfill","reason":"recovered from git history (pruned from manifest)"} diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index c67f132..f632fa9 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -1,14 +1,14 @@ -{"id":"nb-1","title":"Events Resource","description":"Create Events resource with list, create, show, update, delete operations including RSVP management and event-specific filtering capabilities","design":"Create lib/nationbuilder_api/resources/events.rb with RSVP support. See roadmap item #16.","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:35.786297+07:00","updated_at":"2025-12-02T17:26:17.182348+07:00","dependencies":[{"issue_id":"nb-1","depends_on_id":"nb-5","type":"blocks","created_at":"2025-12-02T17:12:15.193639+07:00","created_by":"daemon"}]} -{"id":"nb-10","title":"Production Memory Adapter Warning","description":"Add runtime warning when Memory adapter is used in production Rails environments to prevent accidental production usage","design":"Add check in Memory adapter initialization that logs warning if Rails.env.production?. See roadmap item #11.","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-02T17:09:13.087752+07:00","updated_at":"2025-12-02T17:26:17.186615+07:00"} -{"id":"nb-11","title":"Donations Resource","description":"Build Donations endpoint support with list/filter/pagination, create, show, update operations and proper handling of money values, dates, and donor relationships","design":"Create lib/nationbuilder_api/resources/donations.rb. Handle money formatting, date parsing. See roadmap item #15.","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:34.642315+07:00","updated_at":"2025-12-02T17:26:17.186862+07:00","dependencies":[{"issue_id":"nb-11","depends_on_id":"nb-5","type":"blocks","created_at":"2025-12-02T17:12:13.279819+07:00","created_by":"daemon"}]} -{"id":"nb-12","title":"Add Explicit Linter Configuration","description":"Create .standard.yml file for explicit StandardRB configuration and team consistency beyond defaults","design":"Add .standard.yml with project-specific linter rules. See roadmap item #12.","status":"open","priority":3,"issue_type":"task","created_at":"2025-12-02T17:09:14.308639+07:00","updated_at":"2025-12-02T17:26:17.187136+07:00"} -{"id":"nb-13","title":"Enhance HTTP Error Context","description":"Add request method and path to error messages for improved debugging (e.g., 'Authentication failed for GET /people')","design":"Update error raising in http_client.rb handle_response method to include method and path context. See roadmap item #8.","status":"open","priority":1,"issue_type":"task","created_at":"2025-12-02T17:09:08.185816+07:00","updated_at":"2025-12-02T17:26:17.187371+07:00"} -{"id":"nb-14","title":"Centralize OAuth HTTP Calls","description":"Refactor OAuth module to use HttpClient instead of direct HTTP.post calls for consistent logging and error handling across all HTTP operations","design":"Replace Net::HTTP direct calls in oauth.rb with HttpClient calls. Ensure token exchange and refresh operations use centralized client. See roadmap item #9.","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-02T17:09:09.633647+07:00","updated_at":"2025-12-02T17:26:17.187606+07:00"} -{"id":"nb-2","title":"Add Network Error Test Coverage","description":"Implement tests for timeout scenarios and network error handling to ensure resilience","design":"Add tests for Net::OpenTimeout, Net::ReadTimeout, connection refused scenarios. See roadmap item #13.","status":"open","priority":3,"issue_type":"task","created_at":"2025-12-02T17:09:16.462163+07:00","updated_at":"2025-12-02T17:26:17.183337+07:00"} -{"id":"nb-3","title":"Add Redis \u0026 ActiveRecord Adapter Tests","description":"Implement comprehensive test suites for Redis and ActiveRecord token storage adapters to match Memory adapter test coverage. This addresses 3 failing ActiveRecord tests found in verification.","design":"Add test files spec/nationbuilder_api/token_storage/redis_spec.rb and enhance active_record_spec.rb. Include store_token, retrieve_token, delete_token, serialization tests. See roadmap item #7.","status":"closed","priority":1,"issue_type":"task","created_at":"2025-12-02T17:09:06.202895+07:00","updated_at":"2025-12-02T17:32:40.986334+07:00","closed_at":"2025-12-02T17:32:40.986334+07:00"} -{"id":"nb-4","title":"Fix Token Adapter Validation","description":"Correct validate_adapter_interface! method in client.rb to use respond_to? instead of instance_methods set difference for accurate interface validation","design":"Change from instance_methods set comparison to respond_to? checks for each required method (get_token, store_token, delete_token). See roadmap item #6.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-12-02T17:09:04.540968+07:00","updated_at":"2025-12-02T17:26:17.184025+07:00","closed_at":"2025-12-02T17:18:07.625684+07:00"} -{"id":"nb-5","title":"Response Object Pattern","description":"Wrap API responses in typed objects instead of raw hashes for better developer experience and type safety (e.g., Person, Donation objects)","design":"Create response object classes in lib/nationbuilder_api/response_objects/. Use method_missing or define_method for attribute access. See roadmap item #18.","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:38.473982+07:00","updated_at":"2025-12-02T17:26:17.184279+07:00"} -{"id":"nb-6","title":"Tags Resource","description":"Implement tag management with list tags, apply tag to person, remove tag from person, and bulk tagging operations for managing supporter segments","design":"Create lib/nationbuilder_api/resources/tags.rb. Support bulk operations. See roadmap item #17.","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:36.894093+07:00","updated_at":"2025-12-02T17:26:17.185491+07:00","dependencies":[{"issue_id":"nb-6","depends_on_id":"nb-5","type":"blocks","created_at":"2025-12-02T17:12:17.88942+07:00","created_by":"daemon"}]} -{"id":"nb-7","title":"People Resource - Full CRUD","description":"Implement full CRUD operations for People endpoint including list with filtering/pagination, create, show, update, delete, and search functionality with proper parameter handling","design":"Create lib/nationbuilder_api/resources/people.rb with methods: list, create, show, update, delete, search. Follow REST conventions. See roadmap item #14.","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:33.096076+07:00","updated_at":"2025-12-02T17:26:17.185816+07:00","dependencies":[{"issue_id":"nb-7","depends_on_id":"nb-5","type":"blocks","created_at":"2025-12-02T17:12:11.989747+07:00","created_by":"daemon"}]} -{"id":"nb-8","title":"Fix OAuth VCR Test Failures","description":"Fix 5 failing OAuth integration tests due to VCR cassette configuration issues. Tests in oauth_flow_spec.rb and client_spec.rb fail with 'VCR does not know how to handle' errors.","design":"Review VCR cassette configuration, regenerate cassettes if needed, ensure Net::HTTP compatibility. See verification report from 2025-11-25-switch-to-net-http spec.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-12-02T17:09:18.00717+07:00","updated_at":"2025-12-02T17:26:17.186112+07:00","closed_at":"2025-12-02T17:21:53.298386+07:00"} -{"id":"nb-9","title":"Add Rate Limit Monitoring","description":"Log rate limit headers (X-RateLimit-Remaining, X-RateLimit-Reset) for proactive rate limit monitoring and debugging","design":"Add rate limit header logging in http_client.rb after response received. Log at info level. See roadmap item #10.","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-02T17:09:10.928534+07:00","updated_at":"2025-12-02T17:26:17.186369+07:00"} +{"id":"nb-2","title":"Add Network Error Test Coverage","description":"Implement tests for timeout scenarios and network error handling to ensure resilience","status":"open","priority":3,"issue_type":"task","created_at":"2025-12-02T17:09:16.462163+07:00","updated_at":"2025-12-02T17:26:17.183337+07:00"} +{"id":"nb-6","title":"Tags Resource","description":"Implement tag management with list tags, apply tag to person, remove tag from person, and bulk tagging operations for managing supporter segments","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:36.894093+07:00","updated_at":"2025-12-02T17:26:17.185491+07:00","dependencies":[{"issue_id":"nb-6","depends_on_id":"nb-5","type":"blocks","created_at":"2025-12-02T17:12:17.88942+07:00","created_by":"daemon"}]} +{"id":"nb-5","title":"Response Object Pattern","description":"Wrap API responses in typed objects instead of raw hashes for better developer experience and type safety (e.g., Person, Donation objects)","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:38.473982+07:00","updated_at":"2025-12-02T17:26:17.184279+07:00"} +{"id":"nb-8","title":"Fix OAuth VCR Test Failures","description":"Fix 5 failing OAuth integration tests due to VCR cassette configuration issues. Tests in oauth_flow_spec.rb and client_spec.rb fail with 'VCR does not know how to handle' errors.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-12-02T17:09:18.00717+07:00","updated_at":"2025-12-02T17:26:17.186112+07:00","closed_at":"2025-12-02T17:21:53.298386+07:00"} +{"id":"nb-4","title":"Fix Token Adapter Validation","description":"Correct validate_adapter_interface! method in client.rb to use respond_to? instead of instance_methods set difference for accurate interface validation","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-12-02T17:09:04.540968+07:00","updated_at":"2025-12-02T17:26:17.184025+07:00","closed_at":"2025-12-02T17:18:07.625684+07:00"} +{"id":"nb-1","title":"Events Resource","description":"Create Events resource with list, create, show, update, delete operations including RSVP management and event-specific filtering capabilities","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:35.786297+07:00","updated_at":"2025-12-02T17:26:17.182348+07:00","dependencies":[{"issue_id":"nb-1","depends_on_id":"nb-5","type":"blocks","created_at":"2025-12-02T17:12:15.193639+07:00","created_by":"daemon"}]} +{"id":"nb-13","title":"Enhance HTTP Error Context","description":"Add request method and path to error messages for improved debugging (e.g., 'Authentication failed for GET /people')","status":"open","priority":1,"issue_type":"task","created_at":"2025-12-02T17:09:08.185816+07:00","updated_at":"2025-12-02T17:26:17.187371+07:00"} +{"id":"nb-3","title":"Add Redis \u0026 ActiveRecord Adapter Tests","description":"Implement comprehensive test suites for Redis and ActiveRecord token storage adapters to match Memory adapter test coverage. This addresses 3 failing ActiveRecord tests found in verification.","status":"closed","priority":1,"issue_type":"task","created_at":"2025-12-02T17:09:06.202895+07:00","updated_at":"2025-12-02T17:32:40.986334+07:00","closed_at":"2025-12-02T17:32:40.986334+07:00"} +{"id":"nb-7","title":"People Resource - Full CRUD","description":"Implement full CRUD operations for People endpoint including list with filtering/pagination, create, show, update, delete, and search functionality with proper parameter handling","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:33.096076+07:00","updated_at":"2025-12-02T17:26:17.185816+07:00","dependencies":[{"issue_id":"nb-7","depends_on_id":"nb-5","type":"blocks","created_at":"2025-12-02T17:12:11.989747+07:00","created_by":"daemon"}]} +{"id":"nb-10","title":"Production Memory Adapter Warning","description":"Add runtime warning when Memory adapter is used in production Rails environments to prevent accidental production usage","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-02T17:09:13.087752+07:00","updated_at":"2025-12-02T17:26:17.186615+07:00"} +{"id":"nb-12","title":"Add Explicit Linter Configuration","description":"Create .standard.yml file for explicit StandardRB configuration and team consistency beyond defaults","status":"open","priority":3,"issue_type":"task","created_at":"2025-12-02T17:09:14.308639+07:00","updated_at":"2025-12-02T17:26:17.187136+07:00"} +{"id":"nb-14","title":"Centralize OAuth HTTP Calls","description":"Refactor OAuth module to use HttpClient instead of direct HTTP.post calls for consistent logging and error handling across all HTTP operations","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-02T17:09:09.633647+07:00","updated_at":"2025-12-02T17:26:17.187606+07:00"} +{"id":"nb-11","title":"Donations Resource","description":"Build Donations endpoint support with list/filter/pagination, create, show, update operations and proper handling of money values, dates, and donor relationships","status":"open","priority":2,"issue_type":"feature","created_at":"2025-12-02T17:09:34.642315+07:00","updated_at":"2025-12-02T17:26:17.186862+07:00","dependencies":[{"issue_id":"nb-11","depends_on_id":"nb-5","type":"blocks","created_at":"2025-12-02T17:12:13.279819+07:00","created_by":"daemon"}]} +{"id":"nb-9","title":"Add Rate Limit Monitoring","description":"Log rate limit headers (X-RateLimit-Remaining, X-RateLimit-Reset) for proactive rate limit monitoring and debugging","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-02T17:09:10.928534+07:00","updated_at":"2025-12-02T17:26:17.186369+07:00"} diff --git a/.gitignore b/.gitignore index 7284ce6..783440f 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,4 @@ # rspec failure tracking .rspec_status - -# Built gems *.gem diff --git a/lib/nationbuilder_api/http_client.rb b/lib/nationbuilder_api/http_client.rb index d790724..02fd153 100644 --- a/lib/nationbuilder_api/http_client.rb +++ b/lib/nationbuilder_api/http_client.rb @@ -3,6 +3,7 @@ require "net/http" require "uri" require "json" +require "monitor" module NationbuilderApi # HTTP client with automatic authentication and error handling @@ -14,6 +15,7 @@ def initialize(config:, token_adapter:, identifier:, logger: nil) @token_adapter = token_adapter @identifier = identifier @logger = logger || NationbuilderApi::Logger.new(config.logger, log_level: config.log_level) + @token_refresh_mutex = Monitor.new end # Wrapper class to maintain interface compatibility with http gem responses @@ -162,7 +164,17 @@ def ensure_fresh_token! token_data = @token_adapter.retrieve_token(@identifier) return unless token_data - if OAuth.token_expired?(token_data[:expires_at]) + # Fast path: check if token is still fresh without locking + return unless OAuth.token_expired?(token_data[:expires_at]) + + # Slow path: acquire lock and double-check before refreshing + @token_refresh_mutex.synchronize do + # Double-check token expiry inside mutex to avoid race condition + # Another thread may have already refreshed the token + 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 diff --git a/lib/nationbuilder_api/logger.rb b/lib/nationbuilder_api/logger.rb index 250b653..c432d85 100644 --- a/lib/nationbuilder_api/logger.rb +++ b/lib/nationbuilder_api/logger.rb @@ -12,7 +12,8 @@ class Logger /secret/i, /password/i, /key/i, - /authorization/i + /authorization/i, + /^code$/i # OAuth authorization code parameter ].freeze def initialize(logger = nil, log_level: :info) @@ -39,7 +40,8 @@ def error(message) # Log HTTP request def log_request(method, url, headers: {}, body: nil) - message = "[NationbuilderApi] #{method.upcase} #{url}" + sanitized_url = sanitize_url(url) + message = "[NationbuilderApi] #{method.upcase} #{sanitized_url}" info(message) if @log_level == :debug @@ -88,6 +90,29 @@ def sanitize_body(body) end end + # Sanitize sensitive query parameters from URL + def sanitize_url(url) + return url unless url.include?("?") + + uri = URI.parse(url) + return url unless uri.query + + # Parse query parameters + params = URI.decode_www_form(uri.query) + + # Filter sensitive parameters + sanitized_params = params.map do |key, value| + [key, sensitive_key?(key) ? FILTERED : value] + end + + # Rebuild URL with sanitized parameters + uri.query = URI.encode_www_form(sanitized_params) + uri.to_s + rescue URI::InvalidURIError + # If URL parsing fails, return original (better to log than crash) + url + end + private def default_logger diff --git a/spec/nationbuilder_api/http_client_concurrency_spec.rb b/spec/nationbuilder_api/http_client_concurrency_spec.rb new file mode 100644 index 0000000..cc7ad23 --- /dev/null +++ b/spec/nationbuilder_api/http_client_concurrency_spec.rb @@ -0,0 +1,116 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe NationbuilderApi::HttpClient, "thread safety" do + let(:config) do + config = NationbuilderApi::Configuration.new + config.client_id = "test_client" + config.client_secret = "test_secret" + config.redirect_uri = "https://example.com/callback" + config.base_url = "https://api.nationbuilder.com/v2" + config + end + + let(:token_adapter) { NationbuilderApi::TokenStorage::Memory.new } + let(:identifier) { "test_user" } + let(:http_client) { described_class.new(config: config, token_adapter: token_adapter, identifier: identifier) } + + describe "concurrent token refresh" do + it "prevents race condition when multiple threads detect expired token" do + # Store an expired token + expired_token = { + access_token: "expired_token", + refresh_token: "refresh_token_123", + expires_at: Time.now - 3600, # Expired 1 hour ago + scopes: ["people:read"], + token_type: "Bearer" + } + token_adapter.store_token(identifier, expired_token) + + # Track how many times OAuth.refresh_access_token is called with thread-safe counter + refresh_count_mutex = Mutex.new + refresh_count = 0 + + # Mock OAuth.refresh_access_token to track calls + new_token_data = { + access_token: "new_access_token", + refresh_token: "new_refresh_token", + expires_at: Time.now + 3600, + scopes: ["people:read"], + token_type: "Bearer" + } + + allow(NationbuilderApi::OAuth).to receive(:refresh_access_token) do + refresh_count_mutex.synchronize { refresh_count += 1 } + # Simulate network delay to increase likelihood of race condition + sleep(0.01) + new_token_data + end + + # Mock HTTP request to avoid actual network calls + stub_request(:get, "https://api.nationbuilder.com/v2/test") + .to_return(status: 200, body: '{"data": []}', headers: {"Content-Type" => "application/json"}) + + # Launch multiple threads that will all detect the expired token + threads = 10.times.map do + Thread.new do + http_client.get("/test") + end + end + + # Wait for all threads to complete + threads.each(&:join) + + # Verify that refresh was only called once (not 10 times) + # The mutex should prevent multiple simultaneous refreshes + expect(refresh_count).to eq(1) + end + + it "allows concurrent requests when token is not expired" do + # Store a fresh token + fresh_token = { + access_token: "fresh_token", + refresh_token: "refresh_token_123", + expires_at: Time.now + 3600, # Expires in 1 hour + scopes: ["people:read"], + token_type: "Bearer" + } + token_adapter.store_token(identifier, fresh_token) + + # Mock HTTP request + stub_request(:get, "https://api.nationbuilder.com/v2/test") + .to_return(status: 200, body: '{"data": []}', headers: {"Content-Type" => "application/json"}) + + # Track concurrent execution with thread-safe counters + counter_mutex = Mutex.new + concurrent_count = 0 + max_concurrent = 0 + + # Stub request execution to track concurrency + allow_any_instance_of(Net::HTTP).to receive(:request).and_wrap_original do |method, *args| + counter_mutex.synchronize do + concurrent_count += 1 + max_concurrent = [max_concurrent, concurrent_count].max + end + sleep(0.01) # Simulate some work + result = method.call(*args) + counter_mutex.synchronize { concurrent_count -= 1 } + result + end + + # Launch multiple threads + threads = 10.times.map do + Thread.new do + http_client.get("/test") + end + end + + # Wait for all threads to complete + threads.each(&:join) + + # Verify that requests were actually concurrent (not serialized) + expect(max_concurrent).to be > 1 + end + end +end diff --git a/spec/nationbuilder_api/logger_spec.rb b/spec/nationbuilder_api/logger_spec.rb index f915e21..a3ecd92 100644 --- a/spec/nationbuilder_api/logger_spec.rb +++ b/spec/nationbuilder_api/logger_spec.rb @@ -78,6 +78,56 @@ expect(log_content).to include("[FILTERED]") expect(log_content).not_to include("secret") end + + it "sanitizes sensitive query parameters in URL" do + url = "https://api.example.com/oauth/token?client_secret=secret123&code=auth456&grant_type=authorization_code" + logger.log_request(:post, url) + + output.rewind + log_content = output.read + + expect(log_content).to include("POST") + # Check for URL-encoded version of [FILTERED] + expect(log_content).to include("%5BFILTERED%5D") + expect(log_content).to include("grant_type=authorization_code") + expect(log_content).not_to include("secret123") + expect(log_content).not_to include("auth456") + end + + it "sanitizes refresh_token in URL" do + url = "https://api.example.com/oauth/token?refresh_token=refresh123&grant_type=refresh_token" + logger.log_request(:post, url) + + output.rewind + log_content = output.read + + # Check for URL-encoded version of [FILTERED] + expect(log_content).to include("%5BFILTERED%5D") + expect(log_content).not_to include("refresh123") + end + + it "sanitizes access_token in URL" do + url = "https://api.example.com/people?access_token=token123" + logger.log_request(:get, url) + + output.rewind + log_content = output.read + + # Check for URL-encoded version of [FILTERED] + expect(log_content).to include("%5BFILTERED%5D") + expect(log_content).not_to include("token123") + end + + it "handles URLs without query parameters" do + url = "https://api.example.com/people" + logger.log_request(:get, url) + + output.rewind + log_content = output.read + + expect(log_content).to include("GET") + expect(log_content).to include("https://api.example.com/people") + end end describe "#log_response" do