diff --git a/app/controllers/oidc_controller.rb b/app/controllers/oidc_controller.rb index fd32862..9301fa3 100644 --- a/app/controllers/oidc_controller.rb +++ b/app/controllers/oidc_controller.rb @@ -58,13 +58,13 @@ class OidcController < ApplicationController # Validate PKCE parameters if present if code_challenge.present? unless %w[plain S256].include?(code_challenge_method) - render plain: "Invalid request", status: :bad_request + render plain: "Invalid code_challenge_method: must be 'plain' or 'S256'", status: :bad_request return end # Validate code challenge format (base64url-encoded, 43-128 characters) unless code_challenge.match?(/\A[A-Za-z0-9\-_]{43,128}\z/) - render plain: "Invalid request", status: :bad_request + render plain: "Invalid code_challenge format: must be 43-128 characters of base64url encoding", status: :bad_request return end end @@ -170,9 +170,21 @@ class OidcController < ApplicationController # Add the redirect URI to CSP form-action for this specific request # This allows the OAuth redirect to work while maintaining security + # CSP must allow the OAuth client's redirect_uri as a form submission target if redirect_uri.present? - redirect_host = URI.parse(redirect_uri).host - request.content_security_policy.form_action << "https://#{redirect_host}" if redirect_host + begin + redirect_host = URI.parse(redirect_uri).host + csp = request.content_security_policy + if csp && redirect_host + # Only modify if form_action is available and mutable + if csp.respond_to?(:form_action) && csp.form_action.respond_to?(:<<) + csp.form_action << "https://#{redirect_host}" + end + end + rescue => e + # Log CSP modification errors but don't fail the request + Rails.logger.warn "OAuth: Could not modify CSP for redirect_uri #{redirect_uri}: #{e.message}" + end end render :consent @@ -268,8 +280,7 @@ class OidcController < ApplicationController auth_code = OidcAuthorizationCode.find_by( application: application, - code: code, - used: false + code: code ) unless auth_code @@ -277,55 +288,84 @@ class OidcController < ApplicationController return end - # Check if code is expired - if auth_code.expires_at < Time.current - render json: { error: "invalid_grant", error_description: "Authorization code expired" }, status: :bad_request - return + # Use a transaction with pessimistic locking to prevent code reuse + begin + OidcAuthorizationCode.transaction do + # Lock the record to prevent concurrent access + auth_code.lock! + + # Check if code has already been used (CRITICAL: check AFTER locking) + if auth_code.used? + # Per OAuth 2.0 spec, if an auth code is reused, revoke all tokens issued from it + Rails.logger.warn "OAuth Security: Authorization code reuse detected for code #{auth_code.id}" + + # Revoke all access tokens issued from this authorization code + OidcAccessToken.where( + application: application, + user: auth_code.user, + created_at: auth_code.created_at..Time.current + ).update_all(expires_at: Time.current) + + render json: { + error: "invalid_grant", + error_description: "Authorization code has already been used" + }, status: :bad_request + return + end + + # Check if code is expired + if auth_code.expires_at < Time.current + render json: { error: "invalid_grant", error_description: "Authorization code expired" }, status: :bad_request + return + end + + # Validate redirect URI matches + unless auth_code.redirect_uri == redirect_uri + render json: { error: "invalid_grant", error_description: "Redirect URI mismatch" }, status: :bad_request + return + end + + # Validate PKCE if code challenge is present + pkce_result = validate_pkce(auth_code, code_verifier) + unless pkce_result[:valid] + render json: { + error: pkce_result[:error], + error_description: pkce_result[:error_description] + }, status: pkce_result[:status] + return + end + + # Mark code as used BEFORE generating tokens (prevents reuse) + auth_code.update!(used: true) + + # Get the user + user = auth_code.user + + # Generate access token + access_token = SecureRandom.urlsafe_base64(32) + OidcAccessToken.create!( + application: application, + user: user, + token: access_token, + scope: auth_code.scope, + expires_at: 1.hour.from_now + ) + + # Generate ID token + id_token = OidcJwtService.generate_id_token(user, application, nonce: auth_code.nonce) + + # Return tokens + render json: { + access_token: access_token, + token_type: "Bearer", + expires_in: 3600, + id_token: id_token, + scope: auth_code.scope + } + end + rescue ActiveRecord::RecordNotFound + render json: { error: "invalid_grant" }, status: :bad_request end - - # Validate redirect URI matches - unless auth_code.redirect_uri == redirect_uri - render json: { error: "invalid_grant", error_description: "Redirect URI mismatch" }, status: :bad_request - return - end - - # Validate PKCE if code challenge is present - pkce_result = validate_pkce(auth_code, code_verifier) - unless pkce_result[:valid] - render json: { - error: pkce_result[:error], - error_description: pkce_result[:error_description] - }, status: pkce_result[:status] - return - end - - # Mark code as used - auth_code.update!(used: true) - - # Get the user - user = auth_code.user - - # Generate access token - access_token = SecureRandom.urlsafe_base64(32) - OidcAccessToken.create!( - application: application, - user: user, - token: access_token, - scope: auth_code.scope, - expires_at: 1.hour.from_now - ) - - # Generate ID token - id_token = OidcJwtService.generate_id_token(user, application, nonce: auth_code.nonce) - - # Return tokens - render json: { - access_token: access_token, - token_type: "Bearer", - expires_in: 3600, - id_token: id_token, - scope: auth_code.scope - } end # GET /oauth/userinfo diff --git a/app/services/oidc_jwt_service.rb b/app/services/oidc_jwt_service.rb index 52f709e..8c415b8 100644 --- a/app/services/oidc_jwt_service.rb +++ b/app/services/oidc_jwt_service.rb @@ -63,7 +63,9 @@ class OidcJwtService def issuer_url # In production, this should come from ENV or config # For now, we'll use a placeholder that can be overridden - "https://#{ENV.fetch("CLINCH_HOST", "localhost:3000")}" + host = ENV.fetch("CLINCH_HOST", "localhost:3000") + # Ensure URL has https:// protocol + host.match?(/^https?:\/\//) ? host : "https://#{host}" end private diff --git a/test/controllers/oidc_pkce_controller_test.rb b/test/controllers/oidc_pkce_controller_test.rb index b9ebaa4..1b70649 100644 --- a/test/controllers/oidc_pkce_controller_test.rb +++ b/test/controllers/oidc_pkce_controller_test.rb @@ -11,13 +11,14 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest active: true ) - # Sign in the user by creating a session directly - @session = Session.create!(user: @user) - cookies[:session_id] = @session.id + # Sign in the user using the test helper + sign_in_as(@user) end def teardown + Current.session&.destroy OidcAuthorizationCode.where(application: @application).destroy_all + OidcAccessToken.where(application: @application).destroy_all @user.destroy @application.destroy end @@ -50,9 +51,9 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest get "/oauth/authorize", params: auth_params - # Should redirect to consent page (user is already authenticated) + # Should show consent page (user is already authenticated) assert_response :success - assert_template "consent" + assert_match /consent/, @response.body.downcase end test "authorization endpoint accepts PKCE parameters (plain)" do @@ -71,9 +72,9 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest get "/oauth/authorize", params: auth_params - # Should redirect to consent page (user is already authenticated) + # Should show consent page (user is already authenticated) assert_response :success - assert_template "consent" + assert_match /consent/, @response.body.downcase end test "authorization endpoint rejects invalid code_challenge_method" do @@ -184,7 +185,8 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest grant_type: "authorization_code", code: auth_code.code, redirect_uri: "http://localhost:4000/callback", - code_verifier: "invalid_verifier" + # Use a properly formatted but wrong verifier (43+ chars, base64url) + code_verifier: "wrongverifier_with_enough_characters_base64url" } post "/oauth/token", params: token_params, headers: {