diff --git a/app/controllers/api/csp_controller.rb b/app/controllers/api/csp_controller.rb index 0a8cdab..856f4fc 100644 --- a/app/controllers/api/csp_controller.rb +++ b/app/controllers/api/csp_controller.rb @@ -10,6 +10,13 @@ module Api report_data = JSON.parse(request.body.read) csp_report = report_data['csp-report'] + # Validate that we have a proper CSP report + unless csp_report.is_a?(Hash) && csp_report.present? + Rails.logger.warn "Received empty or invalid CSP violation report" + head :bad_request + return + end + # Log the violation for security monitoring Rails.logger.warn "CSP Violation Report:" Rails.logger.warn " Blocked URI: #{csp_report['blocked-uri']}" diff --git a/app/controllers/api/forward_auth_controller.rb b/app/controllers/api/forward_auth_controller.rb index 85d491b..933d81b 100644 --- a/app/controllers/api/forward_auth_controller.rb +++ b/app/controllers/api/forward_auth_controller.rb @@ -221,7 +221,9 @@ module Api # Try CLINCH_HOST environment variable first if ENV['CLINCH_HOST'].present? - "https://#{ENV['CLINCH_HOST']}" + host = ENV['CLINCH_HOST'] + # Ensure URL has https:// protocol + host.match?(/^https?:\/\//) ? host : "https://#{host}" else # Fallback to the request host request_host = request.host || request.headers['X-Forwarded-Host'] diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index d623d3f..542db5f 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -134,13 +134,16 @@ module Authentication original_url = controller_session[:return_to_after_authenticating] uri = URI.parse(original_url) - # Add token as query parameter - query_params = URI.decode_www_form(uri.query || "").to_h - query_params['fa_token'] = token - uri.query = URI.encode_www_form(query_params) + # Skip adding fa_token for OAuth URLs (OAuth flow should not have forward auth tokens) + unless uri.path&.start_with?("/oauth/") + # Add token as query parameter + query_params = URI.decode_www_form(uri.query || "").to_h + query_params['fa_token'] = token + uri.query = URI.encode_www_form(query_params) - # Update the session with the tokenized URL - controller_session[:return_to_after_authenticating] = uri.to_s + # Update the session with the tokenized URL + controller_session[:return_to_after_authenticating] = uri.to_s + end end end end diff --git a/app/controllers/oidc_controller.rb b/app/controllers/oidc_controller.rb index 62e4067..fd32862 100644 --- a/app/controllers/oidc_controller.rb +++ b/app/controllers/oidc_controller.rb @@ -34,7 +34,7 @@ class OidcController < ApplicationController # GET /oauth/authorize def authorize - # Get parameters + # Get parameters (ignore forward auth tokens and other unknown params) client_id = params[:client_id] redirect_uri = params[:redirect_uri] state = params[:state] @@ -46,7 +46,12 @@ class OidcController < ApplicationController # Validate required parameters unless client_id.present? && redirect_uri.present? && response_type == "code" - render plain: "Invalid request", status: :bad_request + error_details = [] + error_details << "client_id is required" unless client_id.present? + error_details << "redirect_uri is required" unless redirect_uri.present? + error_details << "response_type must be 'code'" unless response_type == "code" + + render plain: "Invalid request: #{error_details.join(', ')}", status: :bad_request return end @@ -67,13 +72,33 @@ class OidcController < ApplicationController # Find the application @application = Application.find_by(client_id: client_id, app_type: "oidc") unless @application - render plain: "Invalid request", status: :bad_request + # Log all OIDC applications for debugging + all_oidc_apps = Application.where(app_type: "oidc") + Rails.logger.error "OAuth: Invalid request - application not found for client_id: #{client_id}" + Rails.logger.error "OAuth: Available OIDC applications: #{all_oidc_apps.pluck(:id, :client_id, :name)}" + + error_msg = if Rails.env.development? + "Invalid request: Application not found for client_id '#{client_id}'. Available OIDC applications: #{all_oidc_apps.pluck(:name, :client_id).map { |name, id| "#{name} (#{id})" }.join(', ')}" + else + "Invalid request: Application not found" + end + + render plain: error_msg, status: :bad_request return end # Validate redirect URI unless @application.parsed_redirect_uris.include?(redirect_uri) - render plain: "Invalid request", status: :bad_request + Rails.logger.error "OAuth: Invalid request - redirect URI mismatch. Expected: #{@application.parsed_redirect_uris}, Got: #{redirect_uri}" + + # For development, show detailed error + error_msg = if Rails.env.development? + "Invalid request: Redirect URI mismatch. Application is configured for: #{@application.parsed_redirect_uris.join(', ')}, but received: #{redirect_uri}" + else + "Invalid request: Redirect URI not registered for this application" + end + + render plain: error_msg, status: :bad_request return end @@ -139,9 +164,17 @@ class OidcController < ApplicationController code_challenge_method: code_challenge_method } - # Render consent page + # Render consent page with dynamic CSP for OAuth redirect @redirect_uri = redirect_uri @scopes = requested_scopes + + # Add the redirect URI to CSP form-action for this specific request + # This allows the OAuth redirect to work while maintaining security + if redirect_uri.present? + redirect_host = URI.parse(redirect_uri).host + request.content_security_policy.form_action << "https://#{redirect_host}" if redirect_host + end + render :consent end diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 7ff2a79..9f3d1c0 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,4 +1,4 @@ class ApplicationMailer < ActionMailer::Base - default from: ENV.fetch('CLINCH_EMAIL_FROM', 'clinch@example.com') + default from: ENV.fetch('CLINCH_FROM_EMAIL', 'clinch@example.com') layout "mailer" end diff --git a/app/views/admin/applications/index.html.erb b/app/views/admin/applications/index.html.erb index 49be1b8..58e0ed4 100644 --- a/app/views/admin/applications/index.html.erb +++ b/app/views/admin/applications/index.html.erb @@ -37,6 +37,8 @@ <% case application.app_type %> <% when "oidc" %> OIDC + <% when "forward_auth" %> + Forward Auth <% when "saml" %> SAML <% end %> diff --git a/app/views/admin/groups/index.html.erb b/app/views/admin/groups/index.html.erb index 063886e..f6cb3a7 100644 --- a/app/views/admin/groups/index.html.erb +++ b/app/views/admin/groups/index.html.erb @@ -39,9 +39,11 @@ <%= pluralize(group.applications.count, "app") %> - <%= link_to "View", admin_group_path(group), class: "text-blue-600 hover:text-blue-900 mr-4" %> - <%= link_to "Edit", edit_admin_group_path(group), class: "text-blue-600 hover:text-blue-900 mr-4" %> - <%= button_to "Delete", admin_group_path(group), method: :delete, data: { turbo_confirm: "Are you sure you want to delete this group?" }, class: "text-red-600 hover:text-red-900" %> +
+ <%= link_to "View", admin_group_path(group), class: "text-blue-600 hover:text-blue-900 whitespace-nowrap" %> + <%= link_to "Edit", edit_admin_group_path(group), class: "text-blue-600 hover:text-blue-900 whitespace-nowrap" %> + <%= button_to "Delete", admin_group_path(group), method: :delete, data: { turbo_confirm: "Are you sure you want to delete this group?" }, class: "text-red-600 hover:text-red-900 whitespace-nowrap" %> +
<% end %> diff --git a/app/views/oidc/consent.html.erb b/app/views/oidc/consent.html.erb index d0312c0..ed05e87 100644 --- a/app/views/oidc/consent.html.erb +++ b/app/views/oidc/consent.html.erb @@ -57,7 +57,7 @@ - <%= form_with url: oauth_consent_path, method: :post, class: "space-y-3", data: { turbo: false } do |form| %> + <%= form_with url: "/oauth/authorize/consent", method: :post, class: "space-y-3", data: { turbo: false }, local: true do |form| %> <%= form.submit "Authorize", class: "w-full flex justify-center py-2 px-4 border border-transparent rounded-md shadow-sm text-sm font-medium text-white bg-blue-600 hover:bg-blue-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500" %> diff --git a/config/environments/production.rb b/config/environments/production.rb index 894ac25..3c99fe2 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -80,14 +80,28 @@ Rails.application.configure do # Only use :id for inspections in production. config.active_record.attributes_for_inspect = [ :id ] + # Helper method to extract domain from CLINCH_HOST (removes protocol if present) + def self.extract_domain(host) + return host if host.blank? + # Remove protocol (http:// or https://) if present + host.gsub(/^https?:\/\//, '') + end + + # Helper method to ensure URL has https:// protocol + def self.ensure_https(url) + return url if url.blank? + # Add https:// if no protocol is present + url.match?(/^https?:\/\//) ? url : "https://#{url}" + end + # Enable DNS rebinding protection and other `Host` header attacks. # Configure allowed hosts based on deployment scenario allowed_hosts = [ - ENV.fetch('CLINCH_HOST', 'auth.example.com'), # External domain (auth service itself) + extract_domain(ENV.fetch('CLINCH_HOST', 'auth.example.com')), # External domain (auth service itself) ] # Use PublicSuffix to extract registrable domain and allow all subdomains - host_domain = ENV.fetch('CLINCH_HOST', 'auth.example.com') + host_domain = extract_domain(ENV.fetch('CLINCH_HOST', 'auth.example.com')) if host_domain.present? begin # Use PublicSuffix to properly extract the domain diff --git a/config/initializers/content_security_policy.rb b/config/initializers/content_security_policy.rb index a95a0f2..6089c09 100644 --- a/config/initializers/content_security_policy.rb +++ b/config/initializers/content_security_policy.rb @@ -39,6 +39,7 @@ Rails.application.configure do policy.base_uri :self # Form actions: Allow self for all form submissions + # Note: OAuth redirects will be handled dynamically in the consent page policy.form_action :self # Manifest sources: Allow self for PWA manifest @@ -53,9 +54,12 @@ Rails.application.configure do # Additional security headers for WebAuthn # Required for WebAuthn to work properly policy.require_trusted_types_for :none + + # CSP reporting using report_uri (supported method) policy.report_uri "/api/csp-violation-report" end + # Start with CSP in report-only mode for testing # Set to false after verifying everything works in production config.content_security_policy_report_only = Rails.env.development? diff --git a/config/initializers/csp_local_logger.rb b/config/initializers/csp_local_logger.rb index cb0032f..854949e 100644 --- a/config/initializers/csp_local_logger.rb +++ b/config/initializers/csp_local_logger.rb @@ -23,6 +23,12 @@ Rails.application.config.after_initialize do def self.emit(event) csp_data = event[:payload] || {} + # Skip logging if there's no meaningful violation data + return if csp_data.empty? || + (csp_data[:violated_directive].nil? && + csp_data[:blocked_uri].nil? && + csp_data[:document_uri].nil?) + # Build a structured log message violated_directive = csp_data[:violated_directive] || "unknown" blocked_uri = csp_data[:blocked_uri] || "unknown" diff --git a/config/initializers/webauthn.rb b/config/initializers/webauthn.rb index 607795e..d5567bb 100644 --- a/config/initializers/webauthn.rb +++ b/config/initializers/webauthn.rb @@ -1,14 +1,31 @@ # WebAuthn configuration for Clinch Identity Provider WebAuthn.configure do |config| # Relying Party name (displayed in authenticator prompts) - # For development, use http://localhost to match passkey in Passwords app + # CLINCH_HOST should include protocol (https://) for WebAuthn origin_host = ENV.fetch("CLINCH_HOST", "http://localhost") config.allowed_origins = [origin_host] - # Relying Party ID (must match origin domain) - # Extract domain from origin for RP ID - origin_uri = URI.parse(origin_host) - config.rp_id = ENV.fetch("CLINCH_RP_ID", "localhost") + # Relying Party ID (must match origin domain without protocol) + # Extract domain from origin for RP ID if CLINCH_RP_ID not set + if ENV["CLINCH_RP_ID"].present? + config.rp_id = ENV["CLINCH_RP_ID"] + else + # Extract registrable domain from CLINCH_HOST using PublicSuffix + origin_uri = URI.parse(origin_host) + if origin_uri.host + begin + # Use PublicSuffix to get the registrable domain (e.g., "aapamilne.com" from "auth.aapamilne.com") + domain = PublicSuffix.parse(origin_uri.host) + config.rp_id = domain.domain || origin_uri.host + rescue PublicSuffix::DomainInvalid => e + Rails.logger.warn "WebAuthn: Failed to parse domain '#{origin_uri.host}': #{e.message}, using host as fallback" + config.rp_id = origin_uri.host + end + else + Rails.logger.error "WebAuthn: Could not extract host from CLINCH_HOST '#{origin_host}'" + config.rp_id = "localhost" + end + end # For development, we also allow localhost with common ports and without port if Rails.env.development?