SECURITY: disable user entered badge SQL by default

- Hidden site settings now must be change via rails console

app/assets/javascripts/admin/templates/badges-show.hbs

@@ -60,42 +60,44 @@
       {{/if}}
     </div>
 
-    <div>
-      <label for="query">{{i18n 'admin.badges.query'}}</label>
-      {{textarea name="query" value=buffered.query disabled=readOnly}}
-    </div>
+    {{#if siteSettings.enable_badge_sql}}
+      <div>
+        <label for="query">{{i18n 'admin.badges.query'}}</label>
+        {{textarea name="query" value=buffered.query disabled=readOnly}}
+      </div>
 
-    {{#if hasQuery}}
-      <a href {{action "preview" buffered "false"}}>{{i18n 'admin.badges.preview.link_text'}}</a>
-      |
-      <a href {{action "preview" buffered "true"}}>{{i18n 'admin.badges.preview.plan_text'}}</a>
-      {{#if preview_loading}}
-        {{i18n 'loading'}}...
+      {{#if hasQuery}}
+        <a href {{action "preview" buffered "false"}}>{{i18n 'admin.badges.preview.link_text'}}</a>
+        |
+        <a href {{action "preview" buffered "true"}}>{{i18n 'admin.badges.preview.plan_text'}}</a>
+        {{#if preview_loading}}
+          {{i18n 'loading'}}...
+        {{/if}}
+
+        <div>
+          <label>
+            {{input type="checkbox" checked=buffered.auto_revoke disabled=readOnly}}
+            {{i18n 'admin.badges.auto_revoke'}}
+          </label>
+        </div>
+
+        <div>
+          <label>
+            {{input type="checkbox" checked=buffered.target_posts disabled=readOnly}}
+            {{i18n 'admin.badges.target_posts'}}
+          </label>
+        </div>
+
+        <div>
+          <label for="trigger">{{i18n 'admin.badges.trigger'}}</label>
+          {{combo-box name="trigger"
+                      value=buffered.trigger
+                      content=badgeTriggers
+                      optionValuePath="content.id"
+                      optionLabelPath="content.name"
+                      disabled=readOnly}}
+        </div>
       {{/if}}
-
-      <div>
-        <label>
-          {{input type="checkbox" checked=buffered.auto_revoke disabled=readOnly}}
-          {{i18n 'admin.badges.auto_revoke'}}
-        </label>
-      </div>
-
-      <div>
-        <label>
-          {{input type="checkbox" checked=buffered.target_posts disabled=readOnly}}
-          {{i18n 'admin.badges.target_posts'}}
-        </label>
-      </div>
-
-      <div>
-        <label for="trigger">{{i18n 'admin.badges.trigger'}}</label>
-        {{combo-box name="trigger"
-                    value=buffered.trigger
-                    content=badgeTriggers
-                    optionValuePath="content.id"
-                    optionLabelPath="content.name"
-                    disabled=readOnly}}
-      </div>
     {{/if}}
 
     <div>

app/controllers/admin/badges_controller.rb

@@ -15,6 +15,12 @@ class Admin::BadgesController < Admin::AdminController
   end
 
   def preview
+
+    unless SiteSetting.enable_badge_sql
+      render json: "preview not allowed", status: 403
+      return
+    end
+
     render json: BadgeGranter.preview(params[:sql],
                                       target_posts: params[:target_posts] == "true",
                                       explain: params[:explain] == "true",
@@ -95,6 +101,8 @@ class Admin::BadgesController < Admin::AdminController
         allowed = Badge.column_names.map(&:to_sym)
         allowed -= [:id, :created_at, :updated_at, :grant_count]
         allowed -= Badge.protected_system_fields if badge.system?
+        allowed -= [:query] unless SiteSetting.enable_badge_sql
+
         params.permit(*allowed)
 
         allowed.each do |key|
@@ -103,7 +111,9 @@ class Admin::BadgesController < Admin::AdminController
 
         # Badge query contract checks
         begin
-          BadgeGranter.contract_checks!(badge.query, { target_posts: badge.target_posts, trigger: badge.trigger })
+          if SiteSetting.enable_badge_sql
+            BadgeGranter.contract_checks!(badge.query, { target_posts: badge.target_posts, trigger: badge.trigger })
+          end
         rescue => e
           errors << e.message
           raise ActiveRecord::Rollback

app/controllers/admin/site_settings_controller.rb

@@ -10,6 +10,10 @@ class Admin::SiteSettingsController < Admin::AdminController
     value = params[id]
     value.strip! if value.is_a?(String)
     begin
+      # note, as of Ruby 2.3 symbols are GC'd so this is considered safe
+      if SiteSetting.hidden_settings.include?(id.to_sym)
+        raise Discourse::InvalidParameters, "You are not allowed to change hidden settings"
+      end
       SiteSetting.set_and_log(id, value, current_user)
       render nothing: true
     rescue Discourse::InvalidParameters => e

config/site_settings.yml

@@ -199,17 +199,19 @@ basic:
   fixed_category_positions:
     client: true
     default: false
-
   fixed_category_positions_on_create:
     client: true
     default: false
-
   show_subcategory_list:
     default: false
     client: true
   enable_badges:
     client: true
     default: true
+  enable_badge_sql:
+    client: true
+    default: false
+    hidden: true
   enable_whispers:
     client: true
     default: false

spec/controllers/admin/badges_controller_spec.rb

@@ -13,6 +13,19 @@ describe Admin::BadgesController do
       end
     end
 
+    context 'preview' do
+      it 'allows preview enable_badge_sql is enabled' do
+        SiteSetting.enable_badge_sql = true
+        result = xhr :get, :preview, sql: 'select id as user_id, created_at granted_at from users'
+        expect(JSON.parse(result.body)["grant_count"]).to be > 0
+      end
+      it 'does not allow anything if enable_badge_sql is disabled' do
+        SiteSetting.enable_badge_sql = false
+        result = xhr :get, :preview, sql: 'select id as user_id, created_at granted_at from users'
+        expect(result.status).to eq(403)
+      end
+    end
+
     context '.save_badge_groupings' do
 
       it 'can save badge groupings' do
@@ -62,14 +75,45 @@ describe Admin::BadgesController do
     end
 
     context '.update' do
-      it 'returns success' do
-        xhr :put, :update, id: badge.id, name: "123456", badge_type_id: badge.badge_type_id, allow_title: false, multiple_grant: false, enabled: true
+
+      it 'does not allow query updates if badge_sql is disabled' do
+        badge.query = "select 123"
+        badge.save
+
+        SiteSetting.enable_badge_sql = false
+
+        xhr :put, :update,
+            id: badge.id,
+            name: "123456",
+            query: "select id user_id, created_at granted_at from users",
+            badge_type_id: badge.badge_type_id,
+            allow_title: false,
+            multiple_grant: false,
+            enabled: true
+
         expect(response).to be_success
+        badge.reload
+        expect(badge.name).to eq('123456')
+        expect(badge.query).to eq('select 123')
       end
 
       it 'updates the badge' do
-        xhr :put, :update, id: badge.id, name: "123456", badge_type_id: badge.badge_type_id, allow_title: false, multiple_grant: true, enabled: true
-        expect(badge.reload.name).to eq('123456')
+        SiteSetting.enable_badge_sql = true
+        sql = "select id user_id, created_at granted_at from users"
+
+        xhr :put, :update,
+            id: badge.id,
+            name: "123456",
+            query: sql,
+            badge_type_id: badge.badge_type_id,
+            allow_title: false,
+            multiple_grant: false,
+            enabled: true
+
+        expect(response).to be_success
+        badge.reload
+        expect(badge.name).to eq('123456')
+        expect(badge.query).to eq(sql)
       end
     end
   end

spec/controllers/admin/site_settings_controller_spec.rb

@@ -46,6 +46,13 @@ describe Admin::SiteSettingsController do
         xhr :put, :update, id: 'test_setting', test_setting: 'hello'
       end
 
+      it 'does not allow changing of hidden settings' do
+        SiteSetting.setting(:hidden_setting, "hidden", hidden: true)
+        result = xhr :put, :update, id: 'hidden_setting', hidden_setting: 'not allowed'
+        expect(SiteSetting.hidden_setting).to eq("hidden")
+        expect(result.status).to eq(422)
+      end
+
       it 'fails when a setting does not exist' do
         expect {
           xhr :put, :update, id: 'provider', provider: 'gotcha'