fix(rules): enforce object-level authorization on rule endpoints

SharedObjectManager scopes querysets to what a user may see, which
includes other people's public and shared-with-them objects. Several
mutating rule endpoints treated that visibility as permission to write.

Generalise get_owned_object_or_403 into get_shared_object_or_error, which
takes an explicit access level instead of inferring one:

- READ requires the object to be visible; denial is 404 so the response
  does not confirm that an id exists.
- EDIT requires ownership; denial is 403, but only after the visibility
  check, so 403 never leaks the existence of an invisible object. This
  matters for TransactionRuleAction, whose manager is unscoped.

The previous owner_path resolved to a User and discarded the object, so
it could not express visibility at all. via= now points at the governing
SharedObject and is resolved with a plain getattr, so an unresolvable
path raises instead of silently granting access.

is_visible_to/is_editable_by replace is_accessible_by, which was never
called and tested visibility == "shared", a value that does not exist in
Visibility.

Also fixes two further holes in the same module:

- transaction_rule_delete fell through to delete() whenever the caller
  was not in shared_with, so any user could delete a public rule. Now
  only the owner deletes; a shared user revokes their own access.
- transaction_rule_view was read-only but is now explicitly READ, so
  rules shared with a user stay viewable.

The activate/deactivate control is hidden for rules the user does not
own, instead of rendering a button that always fails.

Refs GHSA-83g9-vjqf-2j5q

app/apps/common/functions/permissions.py

@@ -1,36 +1,58 @@
 from django.core.exceptions import PermissionDenied
+from django.http import Http404
 from django.shortcuts import get_object_or_404
 
+READ = "read"
+EDIT = "edit"
 
-def get_owned_object_or_403(klass, request, *args, owner_path="owner", **kwargs):
-    """Fetch an object like ``get_object_or_404`` while enforcing ownership.
 
-    Returns the object when it has no owner, or when it is owned by
-    ``request.user``; otherwise raises :class:`~django.core.exceptions.PermissionDenied`
-    (HTTP 403). This mirrors the owner check used by ``transaction_rule_edit``
-    (``if obj.owner and obj.owner != request.user``) so authorization is applied
-    uniformly across handlers that resolve an object from a URL id.
+def get_shared_object_or_error(klass, request, *, level=EDIT, via=None, **kwargs):
+    """Fetch an object like ``get_object_or_404`` while enforcing access control.
 
-    An object with no owner stays accessible to everyone, preserving the
-    existing behaviour for legacy/unowned objects.
+    ``SharedObjectManager`` scopes querysets to what a user may *see*, which is
+    not the same as what they may *change*. Views that resolve an object from a
+    URL id must state which of the two they need, otherwise a shared or public
+    object becomes writable by anyone who can see it.
 
-    ``owner_path`` is a dotted attribute path to the owning user, so nested
-    ownership is supported for objects owned through a relation, e.g. a rule
-    action owned via its parent rule::
+    ``level`` selects the check applied to the governing ``SharedObject``:
 
-        get_owned_object_or_403(
-            TransactionRuleAction, request, id=pk, owner_path="rule.owner"
+    ``READ``
+        The object must be visible to the user. Denial raises :class:`Http404`
+        so the response does not confirm that the id exists.
+    ``EDIT``
+        The object must be owned by the user. Denial raises
+        :class:`~django.core.exceptions.PermissionDenied` (HTTP 403), which the
+        frontend surfaces as an "Access Denied" dialog. An object the user
+        cannot even see raises :class:`Http404` instead, so 403 never confirms
+        the existence of an object they were not allowed to know about.
+
+    Objects with no owner stay accessible to everyone, preserving the existing
+    behaviour for legacy/unowned objects.
+
+    ``via`` is a dotted path to the ``SharedObject`` that governs access, for
+    models owned through a relation, e.g. a rule action governed by its parent
+    rule::
+
+        get_shared_object_or_error(
+            TransactionRuleAction, request, id=pk, level=EDIT, via="rule"
         )
+
+    The path is resolved with a plain ``getattr``, so a path that does not
+    resolve raises ``AttributeError`` rather than silently granting access.
     """
-    obj = get_object_or_404(klass, *args, **kwargs)
+    obj = get_object_or_404(klass, **kwargs)
 
-    owner = obj
-    for attr in owner_path.split("."):
-        owner = getattr(owner, attr, None)
-        if owner is None:
-            break
+    guard = obj
+    for attr in via.split(".") if via else []:
+        guard = getattr(guard, attr)
 
-    if owner is not None and owner != request.user:
+    if level not in (READ, EDIT):
+        raise ValueError(f"Unknown access level: {level!r}")
+
+    if not guard.is_visible_to(request.user):
+        raise Http404
+
+    if level == EDIT and not guard.is_editable_by(request.user):
         raise PermissionDenied
 
     return obj

app/apps/common/models.py

@@ -58,13 +58,35 @@ class SharedObject(models.Model):
             models.Index(fields=["visibility"]),
         ]
 
-    def is_accessible_by(self, user):
-        """Check if a user can access this object"""
-        return (
-            self.visibility == "public"
-            or self.owner == user
-            or (self.visibility == "shared" and user in self.shared_with.all())
-        )
+    # NOTE: these two predicates must stay in sync with the ``Q`` objects built
+    # by ``SharedObjectManager.get_queryset`` above. The manager filters at the
+    # queryset level and these check a single instance, so they cannot share an
+    # implementation; ``SharedObjectPredicateParityTests`` asserts they agree.
+    def is_visible_to(self, user):
+        """Whether ``user`` may read this object.
+
+        Mirrors ``SharedObjectManager``: public objects, objects with no owner,
+        the owner's own objects, and objects explicitly shared with the user.
+        """
+        if self.owner is None or self.visibility == "public":
+            return True
+
+        if not user or not user.is_authenticated:
+            return False
+
+        return self.owner_id == user.pk or self.shared_with.filter(pk=user.pk).exists()
+
+    def is_editable_by(self, user):
+        """Whether ``user`` may mutate this object.
+
+        Sharing grants read access only; mutation stays with the owner. Objects
+        with no owner remain editable by everyone, preserving the behaviour of
+        legacy/unowned objects.
+        """
+        if self.owner is None:
+            return True
+
+        return bool(user and user.is_authenticated and self.owner_id == user.pk)
 
     def save(self, *args, **kwargs):
         if not self.pk and not self.owner:

app/apps/rules/views.py

@@ -4,14 +4,19 @@ from copy import deepcopy
 
 from django.contrib import messages
 from django.contrib.auth.decorators import login_required
+from django.core.exceptions import PermissionDenied
 from django.db import transaction
 from django.http import HttpResponse
-from django.shortcuts import render, get_object_or_404, redirect
-from apps.common.functions.permissions import get_owned_object_or_403
+from django.shortcuts import render, redirect
 from django.utils.translation import gettext_lazy as _
 from django.views.decorators.http import require_http_methods
 
 from apps.common.decorators.htmx import only_htmx
+from apps.common.functions.permissions import (
+    EDIT,
+    READ,
+    get_shared_object_or_error,
+)
 from apps.rules.forms import (
     TransactionRuleForm,
     TransactionRuleActionForm,
@@ -63,7 +68,9 @@ def rules_list(request):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def transaction_rule_toggle_activity(request, transaction_rule_id, **kwargs):
-    transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id)
+    transaction_rule = get_shared_object_or_error(
+        TransactionRule, request, id=transaction_rule_id, level=EDIT
+    )
     current_active = transaction_rule.active
     transaction_rule.active = not current_active
     transaction_rule.save(update_fields=["active"])
@@ -113,17 +120,9 @@ def transaction_rule_add(request, **kwargs):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def transaction_rule_edit(request, transaction_rule_id):
-    transaction_rule = get_object_or_404(TransactionRule, id=transaction_rule_id)
-
-    if transaction_rule.owner and transaction_rule.owner != request.user:
-        messages.error(request, _("Only the owner can edit this"))
-
-        return HttpResponse(
-            status=204,
-            headers={
-                "HX-Trigger": "updated, hide_offcanvas",
-            },
-        )
+    transaction_rule = get_shared_object_or_error(
+        TransactionRule, request, id=transaction_rule_id, level=EDIT
+    )
 
     if request.method == "POST":
         form = TransactionRuleForm(request.POST, instance=transaction_rule)
@@ -152,7 +151,9 @@ def transaction_rule_edit(request, transaction_rule_id):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def transaction_rule_view(request, transaction_rule_id):
-    transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id)
+    transaction_rule = get_shared_object_or_error(
+        TransactionRule, request, id=transaction_rule_id, level=READ
+    )
 
     edit_actions = transaction_rule.transaction_actions.all()
     update_or_create_actions = (
@@ -176,17 +177,20 @@ def transaction_rule_view(request, transaction_rule_id):
 @disabled_on_demo
 @require_http_methods(["DELETE"])
 def transaction_rule_delete(request, transaction_rule_id):
-    transaction_rule = get_object_or_404(TransactionRule, id=transaction_rule_id)
+    transaction_rule = get_shared_object_or_error(
+        TransactionRule, request, id=transaction_rule_id, level=READ
+    )
 
-    if (
-        transaction_rule.owner != request.user
-        and request.user in transaction_rule.shared_with.all()
-    ):
+    if transaction_rule.is_editable_by(request.user):
+        transaction_rule.delete()
+        messages.success(request, _("Rule deleted successfully"))
+    elif transaction_rule.shared_with.filter(pk=request.user.pk).exists():
+        # Someone else's rule shared with us: we can drop our own access to it,
+        # but never delete it.
         transaction_rule.shared_with.remove(request.user)
         messages.success(request, _("Item no longer shared with you"))
     else:
-        transaction_rule.delete()
-        messages.success(request, _("Rule deleted successfully"))
+        raise PermissionDenied
 
     return HttpResponse(
         status=204,
@@ -201,7 +205,9 @@ def transaction_rule_delete(request, transaction_rule_id):
 @disabled_on_demo
 @require_http_methods(["GET"])
 def transaction_rule_take_ownership(request, transaction_rule_id):
-    transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id)
+    transaction_rule = get_shared_object_or_error(
+        TransactionRule, request, id=transaction_rule_id, level=EDIT
+    )
 
     if not transaction_rule.owner:
         transaction_rule.owner = request.user
@@ -223,17 +229,7 @@ def transaction_rule_take_ownership(request, transaction_rule_id):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def transaction_rule_share(request, pk):
-    obj = get_object_or_404(TransactionRule, id=pk)
-
-    if obj.owner and obj.owner != request.user:
-        messages.error(request, _("Only the owner can edit this"))
-
-        return HttpResponse(
-            status=204,
-            headers={
-                "HX-Trigger": "updated, hide_offcanvas",
-            },
-        )
+    obj = get_shared_object_or_error(TransactionRule, request, id=pk, level=EDIT)
 
     if request.method == "POST":
         form = SharedObjectForm(request.POST, instance=obj, user=request.user)
@@ -262,7 +258,9 @@ def transaction_rule_share(request, pk):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def transaction_rule_action_add(request, transaction_rule_id):
-    transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id)
+    transaction_rule = get_shared_object_or_error(
+        TransactionRule, request, id=transaction_rule_id, level=EDIT
+    )
 
     if request.method == "POST":
         form = TransactionRuleActionForm(request.POST, rule=transaction_rule)
@@ -290,12 +288,14 @@ def transaction_rule_action_add(request, transaction_rule_id):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def transaction_rule_action_edit(request, transaction_rule_action_id):
-    transaction_rule_action = get_owned_object_or_403(
-        TransactionRuleAction, request, id=transaction_rule_action_id, owner_path="rule.owner"
-    )
-    transaction_rule = get_object_or_404(
-        TransactionRule, id=transaction_rule_action.rule.id
+    transaction_rule_action = get_shared_object_or_error(
+        TransactionRuleAction,
+        request,
+        id=transaction_rule_action_id,
+        level=EDIT,
+        via="rule",
     )
+    transaction_rule = transaction_rule_action.rule
 
     if request.method == "POST":
         form = TransactionRuleActionForm(
@@ -328,8 +328,12 @@ def transaction_rule_action_edit(request, transaction_rule_action_id):
 @disabled_on_demo
 @require_http_methods(["DELETE"])
 def transaction_rule_action_delete(request, transaction_rule_action_id):
-    transaction_rule_action = get_owned_object_or_403(
-        TransactionRuleAction, request, id=transaction_rule_action_id, owner_path="rule.owner"
+    transaction_rule_action = get_shared_object_or_error(
+        TransactionRuleAction,
+        request,
+        id=transaction_rule_action_id,
+        level=EDIT,
+        via="rule",
     )
 
     transaction_rule_action.delete()
@@ -349,7 +353,9 @@ def transaction_rule_action_delete(request, transaction_rule_action_id):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def update_or_create_transaction_rule_action_add(request, transaction_rule_id):
-    transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id)
+    transaction_rule = get_shared_object_or_error(
+        TransactionRule, request, id=transaction_rule_id, level=EDIT
+    )
 
     if request.method == "POST":
         form = UpdateOrCreateTransactionRuleActionForm(
@@ -381,8 +387,8 @@ def update_or_create_transaction_rule_action_add(request, transaction_rule_id):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def update_or_create_transaction_rule_action_edit(request, pk):
-    linked_action = get_owned_object_or_403(
-        UpdateOrCreateTransactionRuleAction, request, id=pk, owner_path="rule.owner"
+    linked_action = get_shared_object_or_error(
+        UpdateOrCreateTransactionRuleAction, request, id=pk, level=EDIT, via="rule"
     )
     transaction_rule = linked_action.rule
 
@@ -418,8 +424,8 @@ def update_or_create_transaction_rule_action_edit(request, pk):
 @disabled_on_demo
 @require_http_methods(["DELETE"])
 def update_or_create_transaction_rule_action_delete(request, pk):
-    linked_action = get_owned_object_or_403(
-        UpdateOrCreateTransactionRuleAction, request, id=pk, owner_path="rule.owner"
+    linked_action = get_shared_object_or_error(
+        UpdateOrCreateTransactionRuleAction, request, id=pk, level=EDIT, via="rule"
     )
 
     linked_action.delete()
@@ -441,7 +447,7 @@ def update_or_create_transaction_rule_action_delete(request, pk):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def dry_run_rule_created(request, pk):
-    rule = get_owned_object_or_403(TransactionRule, request, id=pk)
+    rule = get_shared_object_or_error(TransactionRule, request, id=pk, level=EDIT)
     logs = None
     results = None
 
@@ -486,7 +492,7 @@ def dry_run_rule_created(request, pk):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def dry_run_rule_deleted(request, pk):
-    rule = get_owned_object_or_403(TransactionRule, request, id=pk)
+    rule = get_shared_object_or_error(TransactionRule, request, id=pk, level=EDIT)
     logs = None
     results = None
 
@@ -531,7 +537,7 @@ def dry_run_rule_deleted(request, pk):
 @disabled_on_demo
 @require_http_methods(["GET", "POST"])
 def dry_run_rule_updated(request, pk):
-    rule = get_owned_object_or_403(TransactionRule, request, id=pk)
+    rule = get_shared_object_or_error(TransactionRule, request, id=pk, level=EDIT)
     logs = None
     results = None
 

app/templates/rules/fragments/list.html

@@ -64,14 +64,21 @@
                   </div>
                 </td>
                 <td class="table-col-auto">
-                  <a class="no-underline cursor-pointer"
-                     role="button"
-                     data-tippy-content="
-                       {% if rule.active %}{% translate "Deactivate" %}{% else %}{% translate "Activate" %}{% endif %}"
-                     hx-get="{% url 'transaction_rule_toggle_activity' transaction_rule_id=rule.id %}">
-                    {% if rule.active %}<i class="fa-solid fa-toggle-on text-success"></i>{% else %}
-                      <i class="fa-solid fa-toggle-off text-error"></i>{% endif %}
-                  </a>
+                  {% if not rule.owner or user == rule.owner %}
+                    <a class="no-underline cursor-pointer"
+                       role="button"
+                       data-tippy-content="
+                         {% if rule.active %}{% translate "Deactivate" %}{% else %}{% translate "Activate" %}{% endif %}"
+                       hx-get="{% url 'transaction_rule_toggle_activity' transaction_rule_id=rule.id %}">
+                      {% if rule.active %}<i class="fa-solid fa-toggle-on text-success"></i>{% else %}
+                        <i class="fa-solid fa-toggle-off text-error"></i>{% endif %}
+                    </a>
+                  {% else %}
+                    <span data-tippy-content="{% translate "Only the owner can change this" %}">
+                      {% if rule.active %}<i class="fa-solid fa-toggle-on text-success opacity-50"></i>{% else %}
+                        <i class="fa-solid fa-toggle-off text-error opacity-50"></i>{% endif %}
+                    </span>
+                  {% endif %}
                 </td>
                 <td class="table-col-auto text-center">
                   <div>{{ rule.order }}</div>