Fix: Prevent unauthorized deletion of anonymous pushes (#4703)

Anonymous pushes with deletable_by_viewer disabled were still
deletable because nil owner matched nil current_user. Require an
authenticated owner or explicit viewer-deletion permission.

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

Author: pglombardo

app/controllers/api/v1/pushes_controller.rb

@@ -308,7 +308,7 @@ def audit
     https://docs.pwpush.com/docs/json-api/
   EOS
   def destroy
-    if (@push.user == current_user) || @push.deletable_by_viewer
+    if @push.deletable_by?(current_user)
       unless @push.expired?
         # Deletable by the owner or viewer
         @push.expire!

app/controllers/pushes_controller.rb

@@ -332,7 +332,7 @@ def audit
 
   def expire
     # Check if the push is deletable by the viewer or if the user is the owner
-    unless @push.deletable_by_viewer || (@push.user == current_user)
+    unless @push.deletable_by?(current_user)
       redirect_to :root, notice: I18n._("That push is not deletable by viewers and does not belong to you.")
       return
     end

app/models/push.rb

@@ -193,6 +193,13 @@ def expire!
     save!
   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.
+  def deletable_by?(user)
+    (user.present? && user_id == user.id) || deletable_by_viewer == true
+  end
+
   def settings_for_kind
     if text?
       Settings.pw

test/integration/api/api_v2_pushes_test.rb

@@ -268,6 +268,27 @@ def test_destroy_allowed_for_anonymous_viewer_when_deletable_by_viewer
     assert push.reload.expired?
   end
 
+  def test_destroy_forbidden_for_anonymous_push_when_not_deletable_by_viewer
+    post "/api/v2/pushes",
+      params: {
+        push: {
+          payload: "anonymous-secret",
+          deletable_by_viewer: false
+        }
+      },
+      as: :json
+    assert_response :created
+
+    token = JSON.parse(@response.body)["url_token"]
+    push = Push.find_by!(url_token: token)
+    assert_nil push.user_id
+    assert_equal false, push.deletable_by_viewer
+
+    delete "/api/v2/pushes/#{token}", as: :json
+    assert_response :unauthorized
+    assert_not push.reload.expired?
+  end
+
   def test_create_requires_auth_when_allow_anonymous_disabled
     Settings.allow_anonymous = false
 

test/integration/password/password_deletion_test.rb

@@ -80,4 +80,19 @@ def test_delete_already_expired_goes_to_expired_path
     assert_response :success
     assert response.body.include?("We apologize but this secret link has expired.")
   end
+
+  def test_anonymous_push_not_deletable_by_viewer_cannot_be_expired
+    post pushes_path, params: {push: {kind: "text", payload: "testpw", deletable_by_viewer: "0"}}
+    assert_response :redirect
+
+    push = Push.order(:created_at).last
+    assert_nil push.user_id
+    assert_equal false, push.deletable_by_viewer
+
+    delete expire_push_path(push)
+    assert_response :redirect
+    follow_redirect!
+    assert_not push.reload.expired?
+    assert_equal "testpw", push.payload
+  end
 end

test/integration/password/password_json_deletion_test.rb

@@ -72,4 +72,36 @@ def test_deletion
     assert res.key?("views_remaining")
     assert_equal Settings.pw.expire_after_views_default - 1, res["views_remaining"]
   end
+
+  def test_anonymous_push_not_deletable_by_viewer_cannot_be_deleted_unauthenticated
+    post passwords_path(format: :json), params: {
+      password: {
+        payload: "SUPER-SECRET",
+        passphrase: "s3cr3t",
+        deletable_by_viewer: "false"
+      }
+    }
+    assert_response :success
+
+    res = JSON.parse(@response.body)
+    token = res["url_token"]
+    assert_equal false, res["deletable_by_viewer"]
+    push = Push.find_by!(url_token: token)
+    assert_nil push.user_id
+
+    # Passphrase still blocks reading
+    get "/p/#{token}.json"
+    assert_response :unauthorized
+
+    # Unauthenticated DELETE must not destroy the push
+    delete "/p/#{token}.json"
+    assert_response :unauthorized
+    assert_not push.reload.expired?
+    assert_equal "SUPER-SECRET", push.payload
+
+    # Intended recipient can still retrieve with passphrase
+    get "/p/#{token}.json", params: {passphrase: "s3cr3t"}
+    assert_response :success
+    assert_equal "SUPER-SECRET", JSON.parse(@response.body)["payload"]
+  end
 end

test/models/push_edit_test.rb

@@ -392,4 +392,27 @@ class PushEditTest < ActiveSupport::TestCase
     assert_not push.valid?
     assert_includes push.errors[:files], "You can only attach up to #{max_files} files per push."
   end
+
+  test "deletable_by? requires authenticated owner or explicit viewer deletion" do
+    owner = users(:luca)
+    other = users(:one)
+
+    owned = Push.create!(kind: "text", payload: "owned", user: owner, deletable_by_viewer: false)
+    assert owned.deletable_by?(owner)
+    assert_not owned.deletable_by?(other)
+    assert_not owned.deletable_by?(nil)
+
+    anonymous = Push.create!(kind: "text", payload: "anon", deletable_by_viewer: false)
+    assert_nil anonymous.user_id
+    assert_not anonymous.deletable_by?(nil)
+    assert_not anonymous.deletable_by?(other)
+
+    anonymous_deletable = Push.create!(kind: "text", payload: "anon-del", deletable_by_viewer: true)
+    assert anonymous_deletable.deletable_by?(nil)
+    assert anonymous_deletable.deletable_by?(other)
+
+    url_push = Push.create!(kind: "url", payload: "https://example.com", deletable_by_viewer: nil)
+    assert_not url_push.deletable_by?(nil)
+    assert_not url_push.deletable_by?(other)
+  end
 end