Merge pull request #2759 from evazion/fix-dmail-filters

Don't filter dmails from moderators; fix dmail filter exploit.

app/controllers/maintenance/user/dmail_filters_controller.rb

@@ -3,6 +3,7 @@ module Maintenance
     class DmailFiltersController < ApplicationController
       before_filter :ensure_ownership
       before_filter :member_only
+      respond_to :html, :json, :xml
 
       def edit
         @dmail_filter = CurrentUser.dmail_filter || DmailFilter.new
@@ -10,9 +11,9 @@ module Maintenance
 
       def update
         @dmail_filter = CurrentUser.dmail_filter || DmailFilter.new
-        @dmail_filter.update_attributes(params[:dmail_filter])
+        @dmail_filter.update(params.require(:dmail_filter).permit(:words), :as => CurrentUser.role)
         flash[:notice] = "Filter updated"
-        redirect_to(dmail_path(@dmail.id))
+        respond_with(@dmail)
       end
 
     private

app/models/dmail_filter.rb

@@ -1,6 +1,6 @@
 class DmailFilter < ActiveRecord::Base
   belongs_to :user
-  attr_accessible :user_id, :words, :as => [:moderator, :janitor, :gold, :member, :anonymous, :default, :builder, :admin]
+  attr_accessible :words, :as => [:moderator, :janitor, :gold, :member, :anonymous, :default, :builder, :admin]
   validates_presence_of :user
   before_validation :initialize_user
 
@@ -11,7 +11,7 @@ class DmailFilter < ActiveRecord::Base
   end
 
   def filtered?(dmail)
-    dmail.from.level <= User::Levels::MODERATOR && has_filter? && (dmail.body =~ regexp || dmail.title =~ regexp || dmail.from.name =~ regexp)
+    dmail.from.level < User::Levels::MODERATOR && has_filter? && (dmail.body =~ regexp || dmail.title =~ regexp || dmail.from.name =~ regexp)
   end
 
   def has_filter?

test/functional/maintenance/user/dmail_filters_controller_test.rb

@@ -0,0 +1,40 @@
+require "test_helper"
+
+module Maintenance
+  module User
+    class DmailFiltersControllerTest < ActionController::TestCase
+      context "The dmail filters controller" do
+        setup do
+          @user1 = FactoryGirl.create(:user)
+          @user2 = FactoryGirl.create(:user)
+          CurrentUser.user = @user1
+          CurrentUser.ip_addr = "127.0.0.1"
+        end
+
+        teardown do
+          CurrentUser.user = nil
+          CurrentUser.ip_addr = nil
+        end
+
+        context "update action" do
+          should "not allow a user to create a filter belonging to another user" do
+            @dmail = FactoryGirl.create(:dmail, :owner => @user1)
+
+            params = {
+              :dmail_id => @dmail.id,
+              :dmail_filter => {
+                :words => "owned",
+                :user_id => @user2.id
+              }
+            }
+
+            post :update, params, { :user_id => @user1.id }
+
+            assert_not_equal("owned", @user2.reload.dmail_filter.try(&:words))
+            assert_redirected_to(@dmail)
+          end
+        end
+      end
+    end
+  end
+end

test/unit/dmail_test.rb

@@ -38,6 +38,16 @@ class DmailTest < ActiveSupport::TestCase
         assert_equal(false, @recipient.has_mail?)
       end
 
+      should "be ignored when sender is a moderator" do
+        CurrentUser.scoped(FactoryGirl.create(:moderator_user), "127.0.0.1") do
+          @dmail = FactoryGirl.create(:dmail, :owner => @recipient, :body => "banned word here", :to => @recipient)
+        end
+
+        assert_equal(false, !!@recipient.dmail_filter.filtered?(@dmail))
+        assert_equal(false, @dmail.is_read?)
+        assert_equal(true, @recipient.has_mail?)
+      end
+
       context "that is empty" do
         setup do
           @recipient.dmail_filter.update_attributes(:words => "   ")