diff --git a/app/assets/tailwind/application.css b/app/assets/tailwind/application.css index 65792d6..e3971af 100644 --- a/app/assets/tailwind/application.css +++ b/app/assets/tailwind/application.css @@ -3,6 +3,12 @@ @custom-variant dark (&:where(.dark, .dark *)); @layer base { + input:where([type="text"], [type="email"], [type="password"], [type="number"], [type="url"], [type="tel"], [type="search"]), + textarea, + select { + padding: 0.5rem 0.75rem; + } + .dark input:where([type="text"], [type="email"], [type="password"], [type="number"], [type="url"], [type="tel"], [type="search"]), .dark textarea, .dark select { diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 7c86216..694c0f0 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -122,15 +122,14 @@ module Admin end def user_params - # Base attributes that all admins can modify - base_params = params.require(:user).permit(:email_address, :username, :name, :password, :status, :totp_required, :custom_claims) + permitted = [:email_address, :username, :name, :password, :status, :totp_required, :custom_claims] # Only allow modifying admin status when editing other users (prevent self-demotion) if params[:id] != Current.session.user.id.to_s - base_params[:admin] = params[:user][:admin] if params[:user][:admin].present? + permitted << :admin end - base_params + params.require(:user).permit(*permitted) end end end diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index c0ddd7a..8ce26c2 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -31,7 +31,7 @@ module Authentication end def find_session_by_cookie - Session.find_by(id: cookies.signed[:session_id]) if cookies.signed[:session_id] + Session.active.find_by(id: cookies.signed[:session_id]) if cookies.signed[:session_id] end def request_authentication @@ -58,8 +58,8 @@ module Authentication { value: session.id, httponly: true, - same_site: :none, # Allow cross-site cookies for OIDC testing - secure: true # Required for SameSite=None + same_site: :lax, + secure: true } else { diff --git a/app/controllers/oidc_controller.rb b/app/controllers/oidc_controller.rb index cc18974..b322d1f 100644 --- a/app/controllers/oidc_controller.rb +++ b/app/controllers/oidc_controller.rb @@ -1,4 +1,6 @@ class OidcController < ApplicationController + SUPPORTED_SCOPES = %w[openid profile email groups offline_access].freeze + # Discovery and JWKS endpoints are public # authorize is also unauthenticated to handle prompt=none and prompt=login specially allow_unauthenticated_access only: [:discovery, :jwks, :token, :revoke, :userinfo, :logout, :authorize] @@ -29,7 +31,7 @@ class OidcController < ApplicationController grant_types_supported: ["authorization_code", "refresh_token"], subject_types_supported: ["pairwise"], id_token_signing_alg_values_supported: ["RS256"], - scopes_supported: ["openid", "profile", "email", "groups", "offline_access"], + scopes_supported: SUPPORTED_SCOPES, token_endpoint_auth_methods_supported: ["client_secret_post", "client_secret_basic"], claims_supported: [ "sub", # Always included @@ -42,7 +44,7 @@ class OidcController < ApplicationController # Note: Custom claims are also supported but not listed here # ID-token-only claims (auth_time, acr, azp, at_hash, nonce) are not listed ], - code_challenge_methods_supported: ["plain", "S256"], + code_challenge_methods_supported: ["S256"], backchannel_logout_supported: true, backchannel_logout_session_supported: true, request_parameter_supported: false, @@ -67,7 +69,7 @@ class OidcController < ApplicationController scope = params[:scope] || "openid" response_type = params[:response_type] code_challenge = params[:code_challenge] - code_challenge_method = params[:code_challenge_method] || "plain" + code_challenge_method = params[:code_challenge_method] || "S256" # Validate client_id first (required before we can look up the application) # OAuth2 RFC 6749 Section 4.1.2.1: If client_id is missing/invalid, show error page (don't redirect) @@ -146,10 +148,10 @@ class OidcController < ApplicationController # Validate PKCE parameters if present (now we can safely redirect with error) if code_challenge.present? - unless %w[plain S256].include?(code_challenge_method) + unless code_challenge_method == "S256" Rails.logger.error "OAuth: Invalid code_challenge_method: #{code_challenge_method}" error_uri = "#{redirect_uri}?error=invalid_request" - error_uri += "&error_description=#{CGI.escape("Invalid code_challenge_method: must be 'plain' or 'S256'")}" + error_uri += "&error_description=#{CGI.escape("Invalid code_challenge_method: only 'S256' is supported")}" error_uri += "&state=#{CGI.escape(state)}" if state.present? redirect_to error_uri, allow_other_host: true return @@ -289,7 +291,15 @@ class OidcController < ApplicationController return end - requested_scopes = scope.split(" ") + requested_scopes = scope.split(" ") & SUPPORTED_SCOPES + scope = requested_scopes.join(" ") + + unless requested_scopes.include?("openid") + error_uri = "#{redirect_uri}?error=invalid_scope&error_description=#{CGI.escape("The 'openid' scope is required")}" + error_uri += "&state=#{CGI.escape(state)}" if state.present? + redirect_to error_uri, allow_other_host: true + return + end # Check if application is configured to skip consent # If so, automatically create consent and proceed without showing consent screen @@ -420,8 +430,7 @@ class OidcController < ApplicationController user = Current.session.user - # Record user consent - requested_scopes = oauth_params["scope"].split(" ") + requested_scopes = oauth_params["scope"].split(" ") & SUPPORTED_SCOPES parsed_claims = begin JSON.parse(oauth_params["claims_requests"]) rescue @@ -1041,16 +1050,14 @@ class OidcController < ApplicationController # Recreate code challenge based on method expected_challenge = case auth_code.code_challenge_method - when "plain" - code_verifier when "S256" Base64.urlsafe_encode64(Digest::SHA256.digest(code_verifier), padding: false) else return { valid: false, - error: "server_error", - error_description: "Unsupported code challenge method", - status: :internal_server_error + error: "invalid_request", + error_description: "Unsupported code challenge method: only 'S256' is supported", + status: :bad_request } end @@ -1156,6 +1163,7 @@ class OidcController < ApplicationController # id_token and/or userinfo keys, each mapping to claim requests def parse_claims_parameter(claims_string) return {} if claims_string.blank? + return nil if claims_string.length > 4096 parsed = JSON.parse(claims_string) return nil unless parsed.is_a?(Hash) diff --git a/app/models/application.rb b/app/models/application.rb index b7e8f93..ad2d81b 100644 --- a/app/models/application.rb +++ b/app/models/application.rb @@ -26,7 +26,7 @@ class Application < ApplicationRecord has_one_attached :icon - # Fix SVG content type after attachment + before_validation :sanitize_svg_icon, if: -> { attachment_changes["icon"].present? } after_save :fix_icon_content_type, if: -> { icon.attached? && saved_change_to_attribute?(:id) == false } has_many :application_groups, dependent: :destroy @@ -283,6 +283,21 @@ class Application < ApplicationRecord end end + def sanitize_svg_icon + return unless icon.content_type == "image/svg+xml" + + raw_svg = icon.download + doc = Loofah.xml_document(raw_svg) + doc.scrub!(SvgScrubber.new) + clean_svg = doc.to_xml + + icon.attach( + io: StringIO.new(clean_svg), + filename: icon.filename.to_s, + content_type: "image/svg+xml" + ) + end + def icon_validation return unless icon.attached? diff --git a/app/models/oidc_authorization_code.rb b/app/models/oidc_authorization_code.rb index 825cdd8..f9be0ba 100644 --- a/app/models/oidc_authorization_code.rb +++ b/app/models/oidc_authorization_code.rb @@ -9,7 +9,7 @@ class OidcAuthorizationCode < ApplicationRecord validates :code_hmac, presence: true, uniqueness: true validates :redirect_uri, presence: true - validates :code_challenge_method, inclusion: {in: %w[plain S256], allow_nil: true} + validates :code_challenge_method, inclusion: {in: %w[S256], allow_nil: true} validate :validate_code_challenge_format, if: -> { code_challenge.present? } scope :valid, -> { where(used: false).where("expires_at > ?", Time.current) } diff --git a/app/models/user.rb b/app/models/user.rb index 1966f93..0fa1cc4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -107,12 +107,12 @@ class User < ApplicationRecord save! # Save the updated array # Log successful backup code usage for security monitoring - Rails.logger.info "Backup code used successfully - User ID: #{id}, IP: #{Current.session&.client_ip}" + Rails.logger.info "Backup code used successfully - User ID: #{id}, IP: #{Current.session&.ip_address}" true else # Increment failed attempt counter and log for security monitoring increment_backup_code_failed_attempts - Rails.logger.warn "Failed backup code attempt - User ID: #{id}, IP: #{Current.session&.client_ip}" + Rails.logger.warn "Failed backup code attempt - User ID: #{id}, IP: #{Current.session&.ip_address}" false end end diff --git a/app/services/oidc_jwt_service.rb b/app/services/oidc_jwt_service.rb index 534f8f0..a6bf96b 100644 --- a/app/services/oidc_jwt_service.rb +++ b/app/services/oidc_jwt_service.rb @@ -1,6 +1,8 @@ class OidcJwtService extend ClaimsMerger + RESERVED_CLAIMS = %i[iss sub aud exp iat nbf jti nonce azp].freeze + class << self # Generate an ID token (JWT) for the user def generate_id_token(user, application, consent: nil, nonce: nil, access_token: nil, auth_time: nil, acr: nil, scopes: "openid", claims_requests: {}) @@ -79,15 +81,16 @@ class OidcJwtService # Merge custom claims from groups (arrays are combined, not overwritten) # Note: Custom claims from groups are always merged (not scope-dependent) + # Reserved claims are stripped as defense-in-depth (also validated at model layer) user.groups.each do |group| - payload = deep_merge_claims(payload, group.parsed_custom_claims) + payload = deep_merge_claims(payload, group.parsed_custom_claims.except(*RESERVED_CLAIMS)) end # Merge custom claims from user (arrays are combined, other values override) - payload = deep_merge_claims(payload, user.parsed_custom_claims) + payload = deep_merge_claims(payload, user.parsed_custom_claims.except(*RESERVED_CLAIMS)) # Merge app-specific custom claims (highest priority, arrays are combined) - payload = deep_merge_claims(payload, application.custom_claims_for_user(user)) + payload = deep_merge_claims(payload, application.custom_claims_for_user(user).except(*RESERVED_CLAIMS)) # Filter custom claims based on claims parameter # If claims parameter is present, only include requested custom claims diff --git a/config/initializers/content_security_policy.rb b/config/initializers/content_security_policy.rb index 926cfd5..607d7df 100644 --- a/config/initializers/content_security_policy.rb +++ b/config/initializers/content_security_policy.rb @@ -11,7 +11,7 @@ Rails.application.configure do # Scripts: Allow self, importmaps, unsafe-inline for Turbo/StimulusJS, and blob: for downloads # Note: unsafe_inline is needed for Stimulus controllers and Turbo navigation - policy.script_src :self, :unsafe_inline, :unsafe_eval, "blob:" + policy.script_src :self, :unsafe_inline, "blob:" # Styles: Allow self and unsafe_inline for TailwindCSS dynamic classes # and Stimulus controller style manipulations diff --git a/test/controllers/oidc_pkce_controller_test.rb b/test/controllers/oidc_pkce_controller_test.rb index 75fe1f8..238c578 100644 --- a/test/controllers/oidc_pkce_controller_test.rb +++ b/test/controllers/oidc_pkce_controller_test.rb @@ -33,8 +33,7 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest config = JSON.parse(@response.body) assert config.key?("code_challenge_methods_supported") - assert_includes config["code_challenge_methods_supported"], "S256" - assert_includes config["code_challenge_methods_supported"], "plain" + assert_equal ["S256"], config["code_challenge_methods_supported"] end test "authorization endpoint accepts PKCE parameters (S256)" do @@ -58,7 +57,7 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest assert_match(/consent/, @response.body.downcase) end - test "authorization endpoint accepts PKCE parameters (plain)" do + test "authorization endpoint rejects PKCE plain method" do code_challenge = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" auth_params = { @@ -74,9 +73,9 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest get "/oauth/authorize", params: auth_params - # Should show consent page (user is already authenticated) - assert_response :success - assert_match(/consent/, @response.body.downcase) + assert_response :redirect + assert_match(/error=invalid_request/, @response.location) + assert_match(/S256/, @response.location) end test "authorization endpoint rejects invalid code_challenge_method" do @@ -153,7 +152,7 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest assert_match(/code_verifier is required/, error["error_description"]) end - test "token endpoint requires code_verifier when PKCE was used (plain)" do + test "token endpoint requires code_verifier when PKCE was used" do # Create consent for token endpoint OidcUserConsent.create!( user: @user, @@ -163,14 +162,16 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest sid: "test-sid-123" ) - # Create authorization code with PKCE plain + code_verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + code_challenge = Base64.urlsafe_encode64(Digest::SHA256.digest(code_verifier), padding: false) + auth_code = OidcAuthorizationCode.create!( application: @application, user: @user, redirect_uri: "http://localhost:4000/callback", scope: "openid profile", - code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", - code_challenge_method: "plain", + code_challenge: code_challenge, + code_challenge_method: "S256", expires_at: 10.minutes.from_now ) @@ -274,28 +275,24 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest assert_equal "Bearer", tokens["token_type"] end - test "token endpoint accepts valid code_verifier (plain)" do - # Create consent for token endpoint - OidcUserConsent.create!( - user: @user, - application: @application, - scopes_granted: "openid profile", - granted_at: Time.current, - sid: "test-sid-123" - ) - + test "token endpoint rejects code_verifier with plain challenge method" do code_verifier = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" - # Create authorization code with PKCE plain - auth_code = OidcAuthorizationCode.create!( + # Directly insert a plain auth code to simulate legacy data + # Generate code HMAC manually since save!(validate: false) skips before_validation + plaintext_code = SecureRandom.urlsafe_base64(32) + auth_code = OidcAuthorizationCode.new( application: @application, user: @user, redirect_uri: "http://localhost:4000/callback", scope: "openid profile", - code_challenge: code_verifier, # Same as verifier for plain method + code_challenge: code_verifier, code_challenge_method: "plain", + code_hmac: OidcAuthorizationCode.compute_code_hmac(plaintext_code), expires_at: 10.minutes.from_now ) + auth_code.plaintext_code = plaintext_code + auth_code.save!(validate: false) token_params = { grant_type: "authorization_code", @@ -308,11 +305,9 @@ class OidcPkceControllerTest < ActionDispatch::IntegrationTest "Authorization" => "Basic " + Base64.strict_encode64("#{@application.client_id}:#{@application.client_secret}") } - assert_response :success - tokens = JSON.parse(@response.body) - assert tokens.key?("access_token") - assert tokens.key?("id_token") - assert_equal "Bearer", tokens["token_type"] + assert_response :bad_request + body = JSON.parse(@response.body) + assert_equal "invalid_request", body["error"] end test "token endpoint works without PKCE (backward compatibility)" do diff --git a/test/models/pkce_authorization_code_test.rb b/test/models/pkce_authorization_code_test.rb index 280aa4d..241657b 100644 --- a/test/models/pkce_authorization_code_test.rb +++ b/test/models/pkce_authorization_code_test.rb @@ -38,23 +38,18 @@ class PkceAuthorizationCodeTest < ActiveSupport::TestCase assert auth_code.uses_pkce? end - test "authorization code can store PKCE challenge with plain method" do - code_challenge = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" - code_challenge_method = "plain" - - auth_code = OidcAuthorizationCode.create!( - application: @application, - user: @user, - redirect_uri: "http://localhost:4000/callback", - scope: "openid profile", - code_challenge: code_challenge, - code_challenge_method: code_challenge_method, - expires_at: 10.minutes.from_now - ) - - assert_equal code_challenge, auth_code.code_challenge - assert_equal code_challenge_method, auth_code.code_challenge_method - assert auth_code.uses_pkce? + test "authorization code rejects plain PKCE method" do + assert_raises(ActiveRecord::RecordInvalid) do + OidcAuthorizationCode.create!( + application: @application, + user: @user, + redirect_uri: "http://localhost:4000/callback", + scope: "openid profile", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + code_challenge_method: "plain", + expires_at: 10.minutes.from_now + ) + end end test "authorization code works without PKCE (backward compatibility)" do