From bb5aa2e6d6ddd33818082e01fa8a1cd3a5b25265 Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Wed, 31 Dec 2025 10:33:56 +1100 Subject: [PATCH] Add rails encryption for totp - allow configuration of encryption secrets from env, or derive them from SECRET_KEY_BASE. Don't leak email address via web_authn, rate limit web_authn, escape oidc state value, require password for changing email address, allow settings the hmac secret for token prefix generation --- .env.example | 18 +++++++++++++++++- app/controllers/oidc_controller.rb | 8 ++++---- app/controllers/profiles_controller.rb | 12 ++++++++++-- app/controllers/webauthn_controller.rb | 15 +++++++++++---- app/models/user.rb | 3 +++ app/views/profiles/show.html.erb | 9 +++++++++ config/initializers/token_hmac.rb | 3 ++- 7 files changed, 56 insertions(+), 12 deletions(-) diff --git a/.env.example b/.env.example index 680a8ca..3155b5d 100644 --- a/.env.example +++ b/.env.example @@ -1,5 +1,21 @@ # Rails Configuration -SECRET_KEY_BASE=generate-with-bin-rails-secret +# SECRET_KEY_BASE is used for: +# - Session cookie encryption +# - Signed token verification +# - ActiveRecord encryption (currently: TOTP secrets) +# - OIDC token prefix HMAC derivation +# +# CRITICAL: Do NOT change SECRET_KEY_BASE after deployment. Changing it will: +# - Invalidate all user sessions (users must re-login) +# - Break encrypted data (users must re-setup 2FA) +# - Invalidate all OIDC access/refresh tokens (clients must re-authenticate) +# +# Optional: Override encryption keys with env vars for key rotation: +# - ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY +# - ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY +# - ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT +# - OIDC_TOKEN_PREFIX_HMAC +SECRET_KEY_BASE=generate-with-bin/rails/secret RAILS_ENV=development # Database diff --git a/app/controllers/oidc_controller.rb b/app/controllers/oidc_controller.rb index d6aa5d9..54744f2 100644 --- a/app/controllers/oidc_controller.rb +++ b/app/controllers/oidc_controller.rb @@ -169,7 +169,7 @@ class OidcController < ApplicationController # Redirect back to client with authorization code redirect_uri = "#{redirect_uri}?code=#{code}" - redirect_uri += "&state=#{state}" if state.present? + redirect_uri += "&state=#{CGI.escape(state)}" if state.present? redirect_to redirect_uri, allow_other_host: true return end @@ -224,7 +224,7 @@ class OidcController < ApplicationController if params[:deny].present? session.delete(:oauth_params) error_uri = "#{oauth_params['redirect_uri']}?error=access_denied" - error_uri += "&state=#{oauth_params['state']}" if oauth_params['state'] + error_uri += "&state=#{CGI.escape(oauth_params['state'])}" if oauth_params['state'] redirect_to error_uri, allow_other_host: true return end @@ -276,7 +276,7 @@ class OidcController < ApplicationController # Redirect back to client with authorization code redirect_uri = "#{oauth_params['redirect_uri']}?code=#{code}" - redirect_uri += "&state=#{oauth_params['state']}" if oauth_params['state'] + redirect_uri += "&state=#{CGI.escape(oauth_params['state'])}" if oauth_params['state'] redirect_to redirect_uri, allow_other_host: true end @@ -724,7 +724,7 @@ class OidcController < ApplicationController if validated_uri redirect_uri = validated_uri - redirect_uri += "?state=#{state}" if state.present? + redirect_uri += "?state=#{CGI.escape(state)}" if state.present? redirect_to redirect_uri, allow_other_host: true else # Invalid redirect URI - log warning and go to default diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index 30614d0..89ce2bf 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -19,13 +19,21 @@ class ProfilesController < ApplicationController else render :show, status: :unprocessable_entity end - else - # Updating email + elsif params[:user][:email_address].present? + # Updating email - requires current password (security: prevents account takeover) + unless @user.authenticate(params[:user][:current_password]) + @user.errors.add(:current_password, "is required to change email") + render :show, status: :unprocessable_entity + return + end + if @user.update(email_params) redirect_to profile_path, notice: "Email updated successfully." else render :show, status: :unprocessable_entity end + else + render :show, status: :unprocessable_entity end end diff --git a/app/controllers/webauthn_controller.rb b/app/controllers/webauthn_controller.rb index 9b7d511..1f0603c 100644 --- a/app/controllers/webauthn_controller.rb +++ b/app/controllers/webauthn_controller.rb @@ -2,6 +2,11 @@ class WebauthnController < ApplicationController before_action :set_webauthn_credential, only: [:destroy] skip_before_action :require_authentication, only: [:check] + # Rate limit check endpoint to prevent enumeration attacks + rate_limit to: 10, within: 1.minute, only: [:check], with: -> { + render json: { error: "Too many requests. Try again later." }, status: :too_many_requests + } + # GET /webauthn/new def new @webauthn_credential = WebauthnCredential.new @@ -131,25 +136,27 @@ class WebauthnController < ApplicationController # GET /webauthn/check # Check if user has WebAuthn credentials (for login page detection) + # Security: Returns identical responses for non-existent users to prevent enumeration def check email = params[:email]&.strip&.downcase if email.blank? - render json: { has_webauthn: false, error: "Email is required" } + render json: { has_webauthn: false, requires_webauthn: false } return end user = User.find_by(email_address: email) + # Security: Return identical response for non-existent users + # Combined with rate limiting (10/min), this prevents account enumeration if user.nil? - render json: { has_webauthn: false, message: "User not found" } + render json: { has_webauthn: false, requires_webauthn: false } return end + # Only return minimal necessary info - no user_id or preferred_method render json: { has_webauthn: user.can_authenticate_with_webauthn?, - user_id: user.id, - preferred_method: user.preferred_authentication_method, requires_webauthn: user.require_webauthn? } end diff --git a/app/models/user.rb b/app/models/user.rb index 20280f5..de156d5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,4 +1,7 @@ class User < ApplicationRecord + # Encrypt TOTP secrets at rest (key derived from SECRET_KEY_BASE) + encrypts :totp_secret + has_secure_password has_many :sessions, dependent: :destroy has_many :user_groups, dependent: :destroy diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index e909c6d..4384fc1 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -31,6 +31,15 @@ class: "mt-1 block w-full rounded-md border-gray-300 shadow-sm focus:border-blue-500 focus:ring-blue-500 sm:text-sm" %> +
+ <%= form.label :current_password, "Current Password", class: "block text-sm font-medium text-gray-700" %> + <%= form.password_field :current_password, + autocomplete: "current-password", + placeholder: "Required to change email", + class: "mt-1 block w-full rounded-md border-gray-300 shadow-sm focus:border-blue-500 focus:ring-blue-500 sm:text-sm" %> +

Enter your current password to confirm this change

+
+
<%= form.submit "Update Email", class: "inline-flex justify-center rounded-md border border-transparent bg-blue-600 py-2 px-4 text-sm font-medium text-white shadow-sm hover:bg-blue-700 focus:outline-none focus:ring-2 focus:ring-blue-500 focus:ring-offset-2" %>
diff --git a/config/initializers/token_hmac.rb b/config/initializers/token_hmac.rb index ec16f8d..7d6608a 100644 --- a/config/initializers/token_hmac.rb +++ b/config/initializers/token_hmac.rb @@ -1,6 +1,7 @@ # Token HMAC key derivation # This key is used to compute HMAC-based token prefixes for fast lookup # Derived from SECRET_KEY_BASE - no storage needed, deterministic output +# Optional: Set OIDC_TOKEN_PREFIX_HMAC env var to override with explicit key module TokenHmac - KEY = Rails.application.key_generator.generate_key('oidc_token_prefix', 32) + KEY = ENV['OIDC_TOKEN_PREFIX_HMAC'] || Rails.application.key_generator.generate_key('oidc_token_prefix', 32) end