Harden push ownership checks against nil comparisons (#4704)

* Harden push ownership checks against nil comparisons

Centralize ownership in Push#owned_by? so audit, edit, update,
and related paths require a present authenticated owner instead
of comparing optional user associations directly.

* Require present user_id in Push#owned_by?

Guard against nil == nil when a non-persisted User.new is passed
for an anonymous push, and cover that case in tests.

---------

Co-authored-by: Peter Giacomo Lombardo <pglombardo@apnotic.com>

Author: pglombardo

app/controllers/api/v1/pushes_controller.rb

@@ -262,7 +262,7 @@ def preview
     https://docs.pwpush.com/docs/json-api/
   EOS
   def audit
-    if @push.user != current_user
+    unless @push.owned_by?(current_user)
       render json: {error: I18n._("That push doesn't belong to you.")}, status: :forbidden
       return
     end

app/controllers/api/v2/pushes_controller.rb

@@ -54,7 +54,7 @@ def create
   def audit
     authenticate_user!
 
-    if @push.user != current_user
+    unless @push.owned_by?(current_user)
       render json: {error: I18n._("That push doesn't belong to you.")}, status: :forbidden
       return
     end
@@ -74,7 +74,7 @@ def audit
   def notify_emails
     authenticate_user!
 
-    if @push.user != current_user
+    unless @push.owned_by?(current_user)
       render json: {error: I18n._("That push doesn't belong to you.")}, status: :forbidden
       return
     end

app/controllers/concerns/log_events.rb

@@ -11,7 +11,7 @@ def log_view(push)
     elsif user_signed_in? && current_user.admin?
       # Admin views take precedence over owner views
       log_event(push, :admin_view)
-    elsif user_signed_in? && push.user_id == current_user.id
+    elsif push.owned_by?(current_user)
       log_event(push, :owner_view)
     else
       log_event(push, :view)

app/controllers/pushes_controller.rb

@@ -130,7 +130,7 @@ def new
   # GET /p/:url_token/edit
   def edit
     # Verify the push belongs to the current user
-    if @push.user_id != current_user.id
+    unless @push.owned_by?(current_user)
       redirect_to :root, notice: I18n._("That push doesn't belong to you.")
       return
     end
@@ -172,7 +172,7 @@ def create
   # PATCH/PUT /p/:url_token
   def update
     # Verify the push belongs to the current user
-    if @push.user_id != current_user.id
+    unless @push.owned_by?(current_user)
       redirect_to :root, notice: I18n._("That push doesn't belong to you.")
       return
     end
@@ -266,7 +266,7 @@ def preview
   def notify_emails
     authenticate_user!
 
-    if @push.user_id != current_user.id
+    unless @push.owned_by?(current_user)
       redirect_to :root, notice: I18n._("That push doesn't belong to you.")
       return
     end
@@ -322,7 +322,7 @@ def audit
       end
       return
     end
-    if @push.user_id != current_user.id
+    unless @push.owned_by?(current_user)
       redirect_to :root, notice: I18n._("That push doesn't belong to you.")
       return
     end
@@ -352,7 +352,7 @@ def expire
 
   def delete_file
     # Verify the push belongs to the current user
-    if @push.user_id != current_user.id
+    unless @push.owned_by?(current_user)
       redirect_to :root, notice: I18n._("That push doesn't belong to you.")
       return
     end

app/models/push.rb

@@ -193,11 +193,17 @@ def expire!
     save!
   end
 
+  # True when +user+ is the authenticated owner of this push.
+  # Require a real owner id on both sides so nil == nil (anonymous push /
+  # non-persisted User.new) never counts as ownership.
+  def owned_by?(user)
+    user_id.present? && user.present? && user_id == user.id
+  end
+
   # True when +user+ is the authenticated owner, or when viewer deletion is
-  # explicitly enabled. Anonymous pushes have a nil owner; comparing two nils
-  # must not count as ownership.
+  # explicitly enabled.
   def deletable_by?(user)
-    (user.present? && user_id == user.id) || deletable_by_viewer == true
+    owned_by?(user) || deletable_by_viewer == true
   end
 
   def settings_for_kind

test/models/push_edit_test.rb

@@ -393,6 +393,23 @@ class PushEditTest < ActiveSupport::TestCase
     assert_includes push.errors[:files], "You can only attach up to #{max_files} files per push."
   end
 
+  test "owned_by? requires an authenticated matching owner" do
+    owner = users(:luca)
+    other = users(:one)
+
+    owned = Push.create!(kind: "text", payload: "owned", user: owner)
+    assert owned.owned_by?(owner)
+    assert_not owned.owned_by?(other)
+    assert_not owned.owned_by?(nil)
+    assert_not owned.owned_by?(User.new)
+
+    anonymous = Push.create!(kind: "text", payload: "anon")
+    assert_nil anonymous.user_id
+    assert_not anonymous.owned_by?(nil)
+    assert_not anonymous.owned_by?(other)
+    assert_not anonymous.owned_by?(User.new)
+  end
+
   test "deletable_by? requires authenticated owner or explicit viewer deletion" do
     owner = users(:luca)
     other = users(:one)