From a69be52b72138cd2a49b87ffb4f70f8cf5d01fe9 Mon Sep 17 00:00:00 2001 From: Runar Ingebrigtsen Date: Mon, 26 Jan 2026 21:38:17 +0100 Subject: [PATCH] fix vulnerabilities --- .../admin/invitations_controller.rb | 8 ++++- app/controllers/admin/users_controller.rb | 19 ++++++++++-- app/controllers/entries_controller.rb | 8 ++++- app/models/entry.rb | 2 +- app/models/user.rb | 3 ++ docs/TODO.md | 31 ++++++++++++------- 6 files changed, 54 insertions(+), 17 deletions(-) diff --git a/app/controllers/admin/invitations_controller.rb b/app/controllers/admin/invitations_controller.rb index 97536e6..940bde2 100644 --- a/app/controllers/admin/invitations_controller.rb +++ b/app/controllers/admin/invitations_controller.rb @@ -44,6 +44,12 @@ class Admin::InvitationsController < Admin::BaseController private def invitation_params - params.require(:user).permit(:email, :name, :role, :primary_language) + permitted = params.require(:user).permit(:email, :name, :primary_language) + + if params[:user][:role].present? && User.roles.key?(params[:user][:role]) + permitted[:role] = params[:user][:role] + end + + permitted end end diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 01ef353..b213313 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -3,14 +3,20 @@ class Admin::UsersController < Admin::BaseController def index @users = User.order(created_at: :desc) - @users = @users.where(role: params[:role]) if params[:role].present? - @users = @users.where("email LIKE ?", "%#{params[:q]}%") if params[:q].present? + .by_role(params[:role]) + .search_email(params[:q]) end def edit end def update + # Prevent users from modifying their own role + if @user == current_user && user_params[:role].present? + redirect_to admin_users_path, alert: "You cannot modify your own role." + return + end + if @user.update(user_params) redirect_to admin_users_path, notice: "User updated successfully." else @@ -40,6 +46,13 @@ class Admin::UsersController < Admin::BaseController end def user_params - params.require(:user).permit(:name, :email, :role, :primary_language) + permitted = params.require(:user).permit(:name, :email, :primary_language) + + # Only allow role if it's a valid role enum value + if params[:user][:role].present? && User.roles.key?(params[:user][:role]) + permitted[:role] = params[:user][:role] + end + + permitted end end diff --git a/app/controllers/entries_controller.rb b/app/controllers/entries_controller.rb index dab8f8a..d8d0060 100644 --- a/app/controllers/entries_controller.rb +++ b/app/controllers/entries_controller.rb @@ -2,7 +2,7 @@ class EntriesController < ApplicationController before_action :set_entry, only: [ :show, :edit, :update ] def index - @language_code = params[:language].presence + @language_code = validate_language_code(params[:language].presence) @category = params[:category].presence @query = params[:q].to_s.strip @starts_with = params[:starts_with].presence @@ -79,4 +79,10 @@ class EntriesController < ApplicationController def entry_params params.require(:entry).permit(:category) end + + def validate_language_code(code) + return nil if code.blank? + + SupportedLanguage.valid_codes.include?(code) ? code : nil + end end diff --git a/app/models/entry.rb b/app/models/entry.rb index 2ff993d..b1e37c3 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -35,7 +35,7 @@ class Entry < ApplicationRecord return none unless valid_lang?(language_code) where.not(language_code => [ nil, "" ]) - .order(Arel.sql("#{language_code} ASC")) + .order(arel_table[language_code].asc) end private diff --git a/app/models/user.rb b/app/models/user.rb index 16ace9b..2da91f3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -21,6 +21,9 @@ class User < ApplicationRecord validates :email, presence: true, uniqueness: true validates :password, length: { minimum: 12 }, if: -> { password.present? } + scope :by_role, ->(role) { where(role: role) if role.present? } + scope :search_email, ->(q) { where("email LIKE ?", "%#{sanitize_sql_like(q)}%") if q.present? } + # Invitation token expires after 14 days INVITATION_TOKEN_EXPIRY = 14.days diff --git a/docs/TODO.md b/docs/TODO.md index 88815d0..12a4edd 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -22,6 +22,24 @@ - [ ] Contributor permissions enforcement (for entry editing) - [ ] Reviewer permissions enforcement (for review queue) +## Security & Vulnerabilities + +- [x] **Fixed user-controlled method execution** (HIGH) + - Added language code validation in EntriesController + - Prevents arbitrary method execution via `public_send()` +- [x] **Fixed SQL injection in Entry model** (MEDIUM) + - Replaced string interpolation with Arel safe column references + - Changed `Arel.sql("#{language_code} ASC")` to `arel_table[language_code].asc` +- [x] **Fixed mass assignment vulnerabilities** (MEDIUM) + - Added role validation in admin invitations and user management + - Only allows valid enum role values + - Prevents users from modifying their own role +- [x] **Fixed SQL LIKE injection** (MEDIUM) + - Added `sanitize_sql_like()` for email search in UsersController + - Prevents wildcard injection attacks + +**Status:** All Brakeman security warnings resolved ✓ + ## Core Features ### Search & Browse @@ -119,24 +137,15 @@ --- -## Completed +## Completed (Not Tracked Above) -- [x] **Invitation system** (complete flow with email, acceptance, and expiry validation) -- [x] **Invitation acceptance flow** (users can accept invitations and set passwords) -- [x] **Invitation mailer** (HTML and text email templates with styled design) -- [x] **Token expiry validation** (14-day expiration for invitation links) -- [x] **Controller tests** (40 tests with 160+ assertions for authentication) -- [x] **Authentication system** (login/logout with session management) - [x] **Admin layout design** updated to match entries page style - [x] **Dynamic navigation** (Admin button for logged-in admins, Sign In for guests) -- [x] **Authorization middleware** (Admin::BaseController with role checks) -- [x] **Invitation token generation** (secure token creation for new users) +- [x] **Controller tests** (40 tests with 160+ assertions for authentication) - [x] **Search input loses focus on filter change** - [x] **Mismatched enum syntax** in models - [x] **Replace hardcoded LANGUAGE_COLUMNS** with dynamic query - [x] **Improve fixture quality** (resolved foreign key violations) -- [x] **XLSX download** button for entries -- [x] **FTS5 integration** (migration added) - [x] **Database schema** implementation (all models and migrations) - [x] **Supported languages** table with seed data - [x] **Filters do not update with new search results**