From d36484e128fb414e7b0f33f831754b7538d7cf9c Mon Sep 17 00:00:00 2001 From: Ahmad Farhat Date: Thu, 3 Sep 2026 10:56:20 -0400 Subject: [PATCH 1/3] Round of security fixes --- .../api/v1/admin/server_rooms_controller.rb | 2 +- .../api/v1/reset_password_controller.rb | 1 + .../api/v1/shared_accesses_controller.rb | 6 +++ app/controllers/api/v1/users_controller.rb | 10 +++-- .../reset_password_controller_spec.rb | 10 +++++ .../shared_accesses_controller_spec.rb | 29 ++++++++++++ spec/controllers/users_controller_spec.rb | 44 +++++++++++++++++++ 7 files changed, 98 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/v1/admin/server_rooms_controller.rb b/app/controllers/api/v1/admin/server_rooms_controller.rb index f509982642..da78e395e5 100644 --- a/app/controllers/api/v1/admin/server_rooms_controller.rb +++ b/app/controllers/api/v1/admin/server_rooms_controller.rb @@ -52,7 +52,7 @@ def resync private def find_room - @room = Room.find_by!(friendly_id: params[:friendly_id]) + @room = Room.joins(:user).with_provider(current_provider).find_by!(friendly_id: params[:friendly_id]) end end end diff --git a/app/controllers/api/v1/reset_password_controller.rb b/app/controllers/api/v1/reset_password_controller.rb index 2384fd832d..13bb2a9d73 100644 --- a/app/controllers/api/v1/reset_password_controller.rb +++ b/app/controllers/api/v1/reset_password_controller.rb @@ -56,6 +56,7 @@ def reset return render_error status: :internal_server_error unless @user.invalidate_reset_token @user.update! password: new_password + @user.generate_session_token! render_data status: :ok end diff --git a/app/controllers/api/v1/shared_accesses_controller.rb b/app/controllers/api/v1/shared_accesses_controller.rb index c8260bd0bd..11488f5a3f 100644 --- a/app/controllers/api/v1/shared_accesses_controller.rb +++ b/app/controllers/api/v1/shared_accesses_controller.rb @@ -28,6 +28,8 @@ class SharedAccessesController < ApiController ensure_authorized(%w[ManageRooms SharedRoom], friendly_id: params[:friendly_id]) end + before_action :ensure_sharing_enabled, only: %i[create shareable_users] + # GET /api/v1/shared_accesses/:friendly_id.json # Returns a list of all of the room's shared users def show @@ -87,6 +89,10 @@ def shareable_users def find_room @room = Room.find_by(friendly_id: params[:friendly_id]) end + + def ensure_sharing_enabled + render_error status: :forbidden unless SettingGetter.new(setting_name: 'ShareRooms', provider: current_provider).call + end end end end diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 6b7d2dd03a..447c0511ad 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -161,6 +161,10 @@ def change_password end current_user.update! password: new_password + current_user.generate_session_token! + session[:session_token] = current_user.session_token + cookies.delete :_extended_session + render_data status: :ok end @@ -200,13 +204,13 @@ def valid_domain? def permitted_params is_admin = PermissionsChecker.new(current_user:, permission_names: 'ManageUsers', current_provider:).call - return %i[password avatar language role_id invite_token] if external_auth? && !is_admin + return %i[avatar language role_id invite_token] if external_auth? && !is_admin allow_name_update = SettingGetter.new(setting_name: 'AllowNameUpdate', provider: current_provider).call - return %i[password avatar language role_id invite_token] if !allow_name_update && !is_admin + return %i[avatar language role_id invite_token] if !allow_name_update && !is_admin - %i[name password avatar language role_id invite_token] + %i[name avatar language role_id invite_token] end end end diff --git a/spec/controllers/reset_password_controller_spec.rb b/spec/controllers/reset_password_controller_spec.rb index c307057823..ca8afe762d 100644 --- a/spec/controllers/reset_password_controller_spec.rb +++ b/spec/controllers/reset_password_controller_spec.rb @@ -77,6 +77,16 @@ expect(user.reset_sent_at).to be_blank end + it 'rotates the session token so that pre-reset sessions are signed out' do + user = create(:user, password: 'Test12345678+') + old_session_token = user.session_token + allow(User).to receive(:verify_reset_token).with(valid_params[:token]).and_return(user) + + post :reset, params: { user: valid_params } + expect(response).to have_http_status(:ok) + expect(user.reload.session_token).not_to eq(old_session_token) + end + it 'returns :forbidden for invalid tokens' do user = create(:user, password: 'Test12345678+') allow(User).to receive(:verify_reset_token).with(valid_params[:token]).and_return(user) diff --git a/spec/controllers/shared_accesses_controller_spec.rb b/spec/controllers/shared_accesses_controller_spec.rb index 1511f8108c..1813cf0de3 100644 --- a/spec/controllers/shared_accesses_controller_spec.rb +++ b/spec/controllers/shared_accesses_controller_spec.rb @@ -25,6 +25,9 @@ before do request.headers['ACCEPT'] = 'application/json' sign_in_user(user) + + setting = Setting.find_or_create_by(name: 'ShareRooms') + create(:site_setting, setting:, provider: 'greenlight', value: 'true') end describe '#create' do @@ -33,6 +36,19 @@ post :create, params: { friendly_id: room.friendly_id, shared_users: [new_user.id] } expect(new_user.shared_rooms).to include(room) end + + context 'when room sharing is disabled' do + before do + SiteSetting.joins(:setting).find_by(provider: 'greenlight', setting: { name: 'ShareRooms' }).update!(value: 'false') + end + + it 'does not share the room with a user' do + new_user = create(:user) + post :create, params: { friendly_id: room.friendly_id, shared_users: [new_user.id] } + expect(response).to have_http_status(:forbidden) + expect(new_user.shared_rooms).not_to include(room) + end + end end describe '#destroy' do @@ -124,5 +140,18 @@ expect(response_users_ids).to match_array([]) end end + + context 'when room sharing is disabled' do + before do + SiteSetting.joins(:setting).find_by(provider: 'greenlight', setting: { name: 'ShareRooms' }).update!(value: 'false') + end + + it 'does not return any shareable users' do + create_list(:user, 5, name: 'John Doe') + + get :shareable_users, params: { friendly_id: room.friendly_id, search: 'John Doe' } + expect(response).to have_http_status(:forbidden) + end + end end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 64618a0338..9fee72d136 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -426,6 +426,29 @@ expect(user.role_id).not_to eq(updated_params[:role_id]) end + it 'doesnt allow a user to change their password without their old password' do + user.update!(password: 'Test12345678+') + + patch :update, params: { id: user.id, user: { password: 'Attacker12345+' } } + + user.reload + + expect(user.authenticate('Attacker12345+')).to be_falsy + expect(user.authenticate('Test12345678+')).to be_truthy + end + + it 'doesnt allow an admin to change another users password' do + sign_in_user(user_with_manage_users_permission) + other_user = create(:user, password: 'Test12345678+') + + patch :update, params: { id: other_user.id, user: { password: 'Attacker12345+' } } + + other_user.reload + + expect(other_user.authenticate('Attacker12345+')).to be_falsy + expect(other_user.authenticate('Test12345678+')).to be_truthy + end + it 'allows a user to change their own name' do updated_params = { name: 'New Awesome Name' @@ -531,6 +554,27 @@ expect(user.reload.authenticate(valid_params[:new_password])).to be_truthy end + it 'rotates the session token but keeps the current user signed in' do + old_session_token = user.session_token + valid_params = { old_password: 'Test12345678+', new_password: 'Glv3IsAwesome!' } + + post :change_password, params: { user: valid_params } + + expect(response).to have_http_status(:ok) + expect(user.reload.session_token).not_to eq(old_session_token) + expect(session[:session_token]).to eq(user.session_token) + end + + it 'does not rotate the session token when the old password is incorrect' do + old_session_token = user.session_token + invalid_params = { old_password: 'NotMine!', new_password: 'ThisIsMine!' } + + post :change_password, params: { user: invalid_params } + + expect(response).to have_http_status(:bad_request) + expect(user.reload.session_token).to eq(old_session_token) + end + it 'returns :bad_request response for invalid old_password' do invalid_params = { old_password: 'NotMine!', new_password: 'ThisIsMine!' } post :change_password, params: { user: invalid_params } From d4f469f378382555b886f108a00abb8c583a978d Mon Sep 17 00:00:00 2001 From: Ahmad Farhat Date: Thu, 3 Sep 2026 15:19:35 -0400 Subject: [PATCH 2/3] second set of changes --- .../api/v1/invitations_controller.rb | 4 +- app/controllers/api/v1/users_controller.rb | 4 +- app/controllers/external_controller.rb | 14 ++++- app/models/invitation.rb | 6 ++ app/serializers/invitation_serializer.rb | 2 +- app/services/big_blue_button_api.rb | 5 ++ app/services/meeting_starter.rb | 9 ++- spec/controllers/external_controller_spec.rb | 59 +++++++++++++++++-- spec/controllers/users_controller_spec.rb | 15 +++++ spec/services/meeting_starter_spec.rb | 3 +- 10 files changed, 106 insertions(+), 15 deletions(-) diff --git a/app/controllers/api/v1/invitations_controller.rb b/app/controllers/api/v1/invitations_controller.rb index 26597a2656..0f022ee66d 100644 --- a/app/controllers/api/v1/invitations_controller.rb +++ b/app/controllers/api/v1/invitations_controller.rb @@ -24,9 +24,9 @@ class InvitationsController < ApiController # GET /api/v1/invitations/:token # Returns the invitation details for the given token (public endpoint for signup pre-fill) def show - invitation = Invitation.find_by(token: params[:token], provider: current_provider) + invitation = Invitation.unexpired.find_by(token: params[:token], provider: current_provider) - if invitation && invitation.updated_at > Invitation::INVITATION_VALIDITY_PERIOD.ago + if invitation render_data data: invitation, serializer: InvitationSerializer, status: :ok else render_error status: :not_found diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 447c0511ad..4d3631497e 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -186,8 +186,8 @@ def valid_invite_token return false if create_user_params[:invite_token].blank? # Try to delete the invitation and return true if it succeeds - Invitation.destroy_by(email: create_user_params[:email].downcase, provider: current_provider, - token: create_user_params[:invite_token]).present? + Invitation.unexpired.destroy_by(email: create_user_params[:email].downcase, provider: current_provider, + token: create_user_params[:invite_token]).present? end def valid_domain? diff --git a/app/controllers/external_controller.rb b/app/controllers/external_controller.rb index 5873d1d9f2..28bb03b4e7 100644 --- a/app/controllers/external_controller.rb +++ b/app/controllers/external_controller.rb @@ -125,7 +125,8 @@ def recording_ready # GET /meeting_ended # Increments a rooms recordings_processing if the meeting was recorded def meeting_ended - # TODO: - ahmad: Add some sort of validation + return render json: {}, status: :unauthorized unless valid_meeting_ended_token? + @room = Room.find_by(meeting_id: extract_meeting_id) return render json: {}, status: :ok unless @room @@ -162,13 +163,22 @@ def extract_meeting_id meeting_id end + def valid_meeting_ended_token? + return false if params[:token].blank? || params[:meetingID].blank? + + payload = BigBlueButtonApi.new(provider: current_provider).decode_jwt(params[:token]) + payload[0]['meeting_id'] == extract_meeting_id + rescue JWT::DecodeError + false + end + def valid_invite_token(email:) token = cookies[:inviteToken] return false if token.blank? # Try to delete the invitation and return true if it succeeds - Invitation.destroy_by(email: email.downcase, provider: current_provider, token:).present? + Invitation.unexpired.destroy_by(email: email.downcase, provider: current_provider, token:).present? end def build_user_info(credentials) diff --git a/app/models/invitation.rb b/app/models/invitation.rb index 0601b79ec8..d6b3d3bbfb 100644 --- a/app/models/invitation.rb +++ b/app/models/invitation.rb @@ -25,6 +25,12 @@ class Invitation < ApplicationRecord validates :provider, presence: true validates :token, uniqueness: true + scope :unexpired, -> { where('invitations.updated_at > ?', INVITATION_VALIDITY_PERIOD.ago) } + + def expired? + updated_at <= INVITATION_VALIDITY_PERIOD.ago + end + def self.search(input) return where('email ILIKE ?', "%#{input}%") if input diff --git a/app/serializers/invitation_serializer.rb b/app/serializers/invitation_serializer.rb index d3504c89d4..bf546f6183 100644 --- a/app/serializers/invitation_serializer.rb +++ b/app/serializers/invitation_serializer.rb @@ -20,6 +20,6 @@ class InvitationSerializer < ApplicationSerializer attributes :id, :email, :name, :updated_at, :valid def valid - object.updated_at > Invitation::INVITATION_VALIDITY_PERIOD.ago + !object.expired? end end diff --git a/app/services/big_blue_button_api.rb b/app/services/big_blue_button_api.rb index 7a0ab11680..40b4eb9936 100644 --- a/app/services/big_blue_button_api.rb +++ b/app/services/big_blue_button_api.rb @@ -105,6 +105,11 @@ def decode_jwt(token) JWT.decode token, @secret, true, { algorithm: 'HS256' } end + # Encodes a JWT using the BBB secret as key (Used to sign the Meeting Ended Callback url) + def encode_jwt(payload) + JWT.encode payload, @secret, 'HS256' + end + private def retrieve_credentials diff --git a/app/services/meeting_starter.rb b/app/services/meeting_starter.rb index 1e7ef4946c..df31f2d698 100644 --- a/app/services/meeting_starter.rb +++ b/app/services/meeting_starter.rb @@ -41,11 +41,12 @@ def call handle_server_tag(meeting_options: options) - options.merge!(computed_options(access_code: viewer_code['glViewerAccessCode'])) options.delete('muteOnStart') unless options['muteOnStart'] == 'true' retries = 0 begin + options.merge!(computed_options(access_code: viewer_code['glViewerAccessCode'])) + meeting = BigBlueButtonApi.new(provider: @provider).start_meeting(room: @room, options:, presentation_url:) @room.update!(online: true, last_session: DateTime.strptime(meeting[:createTime].to_s, '%Q')) @@ -68,7 +69,7 @@ def computed_options(access_code:) moderatorOnlyMessage: moderator_message, loginURL: room_url, logoutURL: room_url, - meta_endCallbackUrl: meeting_ended_url(host: @base_url), + meetingEndedURL: meeting_ended_url(host: @base_url, token: meeting_ended_token), 'meta_bbb-recording-ready-url': recording_ready_url(host: @base_url), 'meta_bbb-origin': 'greenlight', 'meta_bbb-origin-server-name': URI(@base_url).host, @@ -78,6 +79,10 @@ def computed_options(access_code:) } end + def meeting_ended_token + BigBlueButtonApi.new(provider: @provider).encode_jwt({ meeting_id: @room.meeting_id }) + end + def handle_server_tag(meeting_options:) if meeting_options['serverTag'].present? tag_names = Rails.configuration.server_tag_names diff --git a/spec/controllers/external_controller_spec.rb b/spec/controllers/external_controller_spec.rb index 1f3aac75fa..ddf4d4c7dc 100644 --- a/spec/controllers/external_controller_spec.rb +++ b/spec/controllers/external_controller_spec.rb @@ -336,6 +336,23 @@ expect(response).to redirect_to(root_path(error: Rails.configuration.custom_error_msgs[:invite_token_invalid])) end + + it 'returns an InviteInvalid error if the invitation has expired' do + request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect] + invite = create( + :invitation, + email: OmniAuth.config.mock_auth[:openid_connect][:info][:email], + updated_at: Invitation::INVITATION_VALIDITY_PERIOD.ago - 1.day + ) + cookies[:inviteToken] = { + value: invite.token + } + + expect { get :create_user, params: { provider: 'openid_connect' } }.not_to change(User, :count) + + expect(Invitation.exists?(id: invite.id)).to be(true) + expect(response).to redirect_to(root_path(error: Rails.configuration.custom_error_msgs[:invite_token_invalid])) + end end context 'approval' do @@ -554,10 +571,11 @@ describe '#meeting_ended' do let(:room) { create(:room, online: true) } + let(:token) { BigBlueButtonApi.new(provider: 'greenlight').encode_jwt({ meeting_id: room.meeting_id }) } context 'Recorded session' do it 'sets online to false' do - get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true' } + get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true', token: } expect(room.reload.online).to be(false) expect(response).to have_http_status(:ok) @@ -565,10 +583,10 @@ end it 'increments a rooms recordings processing value if the meeting was recorded' do - get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true' } + get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true', token: } expect(room.reload.recordings_processing).to eq(1) - get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true' } + get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true', token: } expect(room.reload.recordings_processing).to eq(2) end end @@ -576,7 +594,7 @@ context 'Unrecorded session' do it 'sets online to false without incrementing a rooms recordings processing' do expect do - get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'false' } + get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'false', token: } end.not_to(change { room.reload.recordings_processing }) expect(room.online).to be(false) @@ -587,12 +605,43 @@ context 'Inexistent room' do it 'silently fail' do - get :meeting_ended, params: { meetingID: '404', recordingmarks: 'false' } + get :meeting_ended, params: { + meetingID: '404', recordingmarks: 'false', + token: BigBlueButtonApi.new(provider: 'greenlight').encode_jwt({ meeting_id: '404' }) + } expect(response).to have_http_status(:ok) expect(response.parsed_body).to eq({}) end end + + context 'Invalid token' do + it 'does not update the room without a token' do + expect do + get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true' } + end.not_to(change { room.reload.attributes }) + + expect(response).to have_http_status(:unauthorized) + end + + it 'does not update the room for a token that is not signed with the BBB secret' do + expect do + get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true', token: 'not-a-jwt' } + end.not_to(change { room.reload.attributes }) + + expect(response).to have_http_status(:unauthorized) + end + + it 'does not update the room for a token signed for a different meeting' do + other_token = BigBlueButtonApi.new(provider: 'greenlight').encode_jwt({ meeting_id: create(:room).meeting_id }) + + expect do + get :meeting_ended, params: { meetingID: room.meeting_id, recordingmarks: 'true', token: other_token } + end.not_to(change { room.reload.attributes }) + + expect(response).to have_http_status(:unauthorized) + end + end end private diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 9fee72d136..15868b78a4 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -271,6 +271,21 @@ expect(response).to have_http_status(:bad_request) expect(response.parsed_body['errors']).to eq(Rails.configuration.custom_error_msgs[:invite_token_invalid]) end + + it 'returns an InviteInvalid error if the invitation has expired' do + invite = create( + :invitation, + email: user_params[:user][:email], + updated_at: Invitation::INVITATION_VALIDITY_PERIOD.ago - 1.day + ) + user_params[:user][:invite_token] = invite.token + + expect { post :create, params: user_params }.not_to change(User, :count) + + expect(Invitation.exists?(id: invite.id)).to be(true) + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body['errors']).to eq(Rails.configuration.custom_error_msgs[:invite_token_invalid]) + end end context 'approval' do diff --git a/spec/services/meeting_starter_spec.rb b/spec/services/meeting_starter_spec.rb index 4fb0464bb0..5d6f066560 100644 --- a/spec/services/meeting_starter_spec.rb +++ b/spec/services/meeting_starter_spec.rb @@ -41,7 +41,8 @@ moderatorOnlyMessage: "#{I18n.t('meeting.moderator_message', locale: user.language.to_sym)}
#{url}", loginURL: url, logoutURL: url, - meta_endCallbackUrl: File.join(base_url, '/meeting_ended'), + meetingEndedURL: "#{File.join(base_url, '/meeting_ended')}?token=" \ + "#{BigBlueButtonApi.new(provider: 'greenlight').encode_jwt({ meeting_id: room.meeting_id })}", 'meta_bbb-recording-ready-url': File.join(base_url, '/recording_ready'), 'meta_bbb-origin-version': 'v3', 'meta_bbb-origin': 'greenlight', From 42e0a70a2c0fc56ef94cb15725e86993ab43b1be Mon Sep 17 00:00:00 2001 From: Ahmad Farhat Date: Fri, 4 Sep 2026 11:48:11 -0400 Subject: [PATCH 3/3] Fixes --- bin/start | 5 +++++ gl-install.sh | 1 + sample.env | 6 +++--- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/bin/start b/bin/start index 31c4875980..150ac39ac5 100755 --- a/bin/start +++ b/bin/start @@ -2,6 +2,11 @@ source bin/config.env +if [ "$RAILS_ENV" = "production" ] && [ -z "$URL_HOST" ] && [ "$BYPASS_HOST" != "true" ]; then + echo "ERROR: URL_HOST must be set to the hostname Greenlight is served from." + exit 1 +fi + echo "Greenlight-v3 starting on port: $PORT" echo "Postgres host: $PGHOST" diff --git a/gl-install.sh b/gl-install.sh index 3b17309706..5551a89b70 100755 --- a/gl-install.sh +++ b/gl-install.sh @@ -450,6 +450,7 @@ install_greenlight_v3(){ sed -i "s|^[# \t]*SECRET_KEY_BASE=[ \t]*$|SECRET_KEY_BASE=$SECRET_KEY_BASE|" $GL3_DIR/.env # Do not overwrite the value if not empty. sed -i "s|^[# \t]*DATABASE_URL=[ \t]*$|DATABASE_URL=$DATABASE_URL_ROOT/$PGDBNAME|" $GL3_DIR/.env # Do not overwrite the value if not empty. sed -i "s|^[# \t]*REDIS_URL=[ \t]*$|REDIS_URL=$REDIS_URL_ROOT/|" $GL3_DIR/.env # Do not overwrite the value if not empty. + sed -i "s|^[# \t]*URL_HOST=[ \t]*$|URL_HOST=$HOST|" $GL3_DIR/.env # Do not overwrite the value if not empty. # Placing greenlight-v3 nginx file, this will enable greenlight-v3 as your Bigbluebutton frontend (bbb-fe). cp -v $NGINX_FILES_DEST/greenlight-v3.nginx $NGINX_FILES_DEST/greenlight-v3.nginx.old && say "old greenlight-v3 nginx config can be retrieved at $NGINX_FILES_DEST/greenlight-v3.nginx.old" #Backup diff --git a/sample.env b/sample.env index 36f0d8f118..4f0832914d 100644 --- a/sample.env +++ b/sample.env @@ -20,6 +20,9 @@ DATABASE_URL= # E.g. redis://redis:6379 REDIS_URL= +# The hostname Greenlight is served from +URL_HOST= + ### OPTIONAL ENV VARS ### SMTP CONFIGURATION @@ -79,9 +82,6 @@ REDIS_URL= #GCS_CLIENT_ID= #GCS_CLIENT_CERT= -# Set this to explicitly specify base hostname -#URL_HOST= - # Define the default locale language code (i.e. 'en' for English) from the following list: # [en, ar, fr, es, fa_IR] # The DEFAULT_LOCALE setting specifies the default language, overriding the browser language which is always set.