security(cr): writable selected_field bypasses CR conflict checks

spp_change_request_v2/README.rst

@@ -853,6 +853,21 @@ Before declaring a new CR type complete:
 Changelog
 =========
 
+19.0.3.0.3
+~~~~~~~~~~
+
+- fix(security): scope dynamic-approval conflict and duplicate detection
+  to the fields that actually differ from the registrant (the changes
+  the apply strategy will write), instead of a user-writable label. Both
+  ``selected_field_name`` (view-readonly only) and the detail's
+  ``field_to_modify`` are writable by a change-request user and do not
+  constrain what apply changes, so a user could point them at a field
+  outside a rule's ``conflict_fields`` to clear a field-scoped conflict
+  and submit without the configured override path. Detection now derives
+  the changed fields server-side from the detail-vs-registrant diff and
+  ignores those labels; it is skipped for non-dynamic-approval request
+  types.
+
 19.0.3.0.0
 ~~~~~~~~~~
 

spp_change_request_v2/__manifest__.py

@@ -1,6 +1,6 @@
 {
     "name": "OpenSPP Change Request V2",
-    "version": "19.0.3.0.0",
+    "version": "19.0.3.0.3",
     "sequence": 50,
     "category": "OpenSPP",
     "summary": "Configuration-driven change request system with UX improvements, conflict detection and duplicate prevention",

spp_change_request_v2/models/conflict_mixin.py

@@ -291,12 +291,61 @@ def _get_group_member_ids(self):
 
         return list(set(member_ids))
 
+    def _proposed_changed_fields(self):
+        """Return the detail (source) fields a dynamic-approval CR actually
+        proposes to change.
+
+        These are the mapped fields whose detail value differs from the
+        registrant's current value — exactly the set the ``field_mapping`` apply
+        strategy will write. This is derived server-side from the actual data,
+        NOT from a declared label: both ``selected_field_name`` (only
+        view-readonly) and the detail's ``field_to_modify`` (freely writable and
+        validated only to be a real field name, not tied to what apply changes)
+        are attacker-controlled. A user could otherwise clear a field-scoped
+        conflict/duplicate by labelling a different, unchanged field while still
+        changing a scoped field. Scoping to the real diff makes those labels
+        irrelevant to the security decision.
+
+        Returns a set of detail field names for dynamic-approval CR types, or
+        ``None`` for non-dynamic types (where field scoping does not apply and
+        the full configured field set must always be considered — their details
+        are not registrant-prefilled snapshots).
+        """
+        self.ensure_one()
+        if not self.request_type_id.use_dynamic_approval:
+            return None
+        detail = self.get_detail()
+        registrant = self.registrant_id
+        if not detail or not registrant:
+            return set()
+        changed = set()
+        for mapping in self.request_type_id.apply_mapping_ids:
+            source_field = mapping.source_field
+            target_field = mapping.target_field
+            if source_field not in detail._fields or target_field not in registrant._fields:
+                continue
+            detail_value = self._normalize_field_value(getattr(detail, source_field, None))
+            registrant_value = self._normalize_field_value(getattr(registrant, target_field, None))
+            if detail_value != registrant_value:
+                changed.add(source_field)
+        return changed
+
+    def _effective_conflict_fields(self, conflict_fields):
+        """Return the subset of ``conflict_fields`` this CR actually proposes to
+        change (the security-relevant scope). Full set for non-dynamic types."""
+        self.ensure_one()
+        conflict_fields = set(conflict_fields)
+        proposed = self._proposed_changed_fields()
+        if proposed is None:
+            return conflict_fields
+        return proposed & conflict_fields
+
     def _filter_by_field_conflicts(self, candidates, rule):
         """Filter candidate CRs by checking if they modify the same fields.
 
-        For dynamic-approval CRs (where selected_field_name is set), only the
-        selected field is treated as a proposed change. Prefilled fields from
-        the registrant are ignored for conflict purposes.
+        For dynamic-approval CRs, the proposed changes are the conflict fields
+        whose value actually differs from the registrant (derived server-side),
+        not a user-writable label. Prefilled/unchanged fields are ignored.
         """
         self.ensure_one()
 
@@ -308,14 +357,10 @@ def _filter_by_field_conflicts(self, candidates, rule):
         if not my_detail:
             return self.env["spp.change.request"]
 
-        # Dynamic approval: only the selected field is a proposed change
-        my_selected = self.selected_field_name
-        if my_selected:
-            if my_selected not in conflict_fields:
-                return self.env["spp.change.request"]
-            my_effective_fields = [my_selected]
-        else:
-            my_effective_fields = conflict_fields
+        my_effective_fields = self._effective_conflict_fields(conflict_fields)
+        if not my_effective_fields:
+            # This CR changes none of the rule's fields — nothing to conflict on.
+            return self.env["spp.change.request"]
 
         matching = self.env["spp.change.request"]
 
@@ -324,18 +369,10 @@ def _filter_by_field_conflicts(self, candidates, rule):
             if not candidate_detail:
                 continue
 
-            # Determine candidate's effective fields
-            candidate_selected = candidate.selected_field_name
-            if candidate_selected:
-                # Both use dynamic approval: conflict only if same field
-                if my_selected and candidate_selected != my_selected:
-                    continue
-                candidate_effective = [candidate_selected]
-            else:
-                candidate_effective = conflict_fields
+            candidate_effective = candidate._effective_conflict_fields(conflict_fields)
 
-            # Check overlapping effective fields
-            fields_to_check = set(my_effective_fields) & set(candidate_effective)
+            # Overlap = fields BOTH CRs actually propose to change.
+            fields_to_check = my_effective_fields & candidate_effective
 
             for field_name in fields_to_check:
                 if field_name not in my_detail._fields:
@@ -439,9 +476,9 @@ def _detect_duplicates(self):
     def _calculate_similarity(self, other_cr, config):
         """Calculate similarity percentage between this CR and another.
 
-        For dynamic-approval CRs (where selected_field_name is set), only the
-        selected field is compared. Prefilled fields are ignored to prevent
-        inflated similarity scores.
+        For dynamic-approval CRs, only the selected field (derived server-side
+        from the detail's validated ``field_to_modify``) is compared. Prefilled
+        fields are ignored to prevent inflated similarity scores.
 
         Args:
             other_cr: Another spp.change.request record
@@ -458,22 +495,29 @@ def _calculate_similarity(self, other_cr, config):
         if not my_detail or not other_detail:
             return 0.0
 
-        # Dynamic approval: compare only the selected field
-        my_selected = self.selected_field_name
-        other_selected = other_cr.selected_field_name
-        if my_selected and other_selected:
-            # Different fields selected = not duplicates
-            if my_selected != other_selected:
+        # Dynamic approval: compare only the fields actually changed (derived
+        # server-side from the detail-vs-registrant diff, not a writable label).
+        my_changed = self._proposed_changed_fields()
+        other_changed = other_cr._proposed_changed_fields()
+        if my_changed is not None and other_changed is not None:
+            # Different set of changed fields (or neither changed anything) =
+            # not duplicates.
+            if my_changed != other_changed or not my_changed:
                 return 0.0
-            # Same field: compare that field's value only
-            if my_selected in my_detail._fields and my_selected in other_detail._fields:
-                my_value = self._normalize_field_value(getattr(my_detail, my_selected, None))
-                other_value = self._normalize_field_value(getattr(other_detail, my_selected, None))
-                if my_value == other_value:
-                    return 100.0
-                elif self._are_similar(my_value, other_value):
-                    return 80.0
-            return 0.0
+            all_match = True
+            any_similar = False
+            for field_name in my_changed:
+                if field_name not in my_detail._fields or field_name not in other_detail._fields:
+                    continue
+                my_value = self._normalize_field_value(getattr(my_detail, field_name, None))
+                other_value = self._normalize_field_value(getattr(other_detail, field_name, None))
+                if my_value != other_value:
+                    all_match = False
+                    if self._are_similar(my_value, other_value):
+                        any_similar = True
+            if all_match:
+                return 100.0
+            return 80.0 if any_similar else 0.0
 
         # Static CRs (or mixed): original logic
         check_fields = config.get_check_fields_list()

spp_change_request_v2/readme/HISTORY.md

@@ -1,3 +1,16 @@
+### 19.0.3.0.3
+
+- fix(security): scope dynamic-approval conflict and duplicate detection to the
+  fields that actually differ from the registrant (the changes the apply
+  strategy will write), instead of a user-writable label. Both
+  ``selected_field_name`` (view-readonly only) and the detail's
+  ``field_to_modify`` are writable by a change-request user and do not constrain
+  what apply changes, so a user could point them at a field outside a rule's
+  ``conflict_fields`` to clear a field-scoped conflict and submit without the
+  configured override path. Detection now derives the changed fields
+  server-side from the detail-vs-registrant diff and ignores those labels; it
+  is skipped for non-dynamic-approval request types.
+
 ### 19.0.3.0.0
 
 - feat(change_request): redesign the group/membership CR flows (#242) — Create Group (#876), Add Member now searches an existing member (#871), Remove Member first-page/review cleanup (#872), Change Head of Household via a per-member role table (#873), and Split Household as a relational member move with single-head validation (#877). Review pages render the real data as tables / detail sections.

spp_change_request_v2/static/description/index.html

@@ -1339,6 +1339,22 @@ <h2>Changelog</h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.3.0.3</h1>
+<ul class="simple">
+<li>fix(security): scope dynamic-approval conflict and duplicate detection
+to the fields that actually differ from the registrant (the changes
+the apply strategy will write), instead of a user-writable label. Both
+<tt class="docutils literal">selected_field_name</tt> (view-readonly only) and the detail’s
+<tt class="docutils literal">field_to_modify</tt> are writable by a change-request user and do not
+constrain what apply changes, so a user could point them at a field
+outside a rule’s <tt class="docutils literal">conflict_fields</tt> to clear a field-scoped conflict
+and submit without the configured override path. Detection now derives
+the changed fields server-side from the detail-vs-registrant diff and
+ignores those labels; it is skipped for non-dynamic-approval request
+types.</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.3.0.0</h1>
 <ul class="simple">
 <li>feat(change_request): redesign the group/membership CR flows (#242) —
@@ -1360,7 +1376,7 @@ <h1>19.0.3.0.0</h1>
 must adapt (see #1133).</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.8</h1>
 <ul class="simple">
 <li>fix(views): disable inline creation of CR document types on the Change
@@ -1371,7 +1387,7 @@ <h1>19.0.2.0.8</h1>
 Documents” modal (missing Name field) that blocked saving (#1125)</li>
 </ul>
 </div>
-<div class="section" id="section-3">
+<div class="section" id="section-4">
 <h1>19.0.2.0.7</h1>
 <ul class="simple">
 <li>fix(security): align CR Requestor / CR Local Validator / CR HQ
@@ -1383,7 +1399,7 @@ <h1>19.0.2.0.7</h1>
 dependencies.</li>
 </ul>
 </div>
-<div class="section" id="section-4">
+<div class="section" id="section-5">
 <h1>19.0.2.0.6</h1>
 <ul class="simple">
 <li>fix(views): route post-submit CRs (pending / approved / applied /
@@ -1398,7 +1414,7 @@ <h1>19.0.2.0.6</h1>
 list so row-click goes through the stage router.</li>
 </ul>
 </div>
-<div class="section" id="section-5">
+<div class="section" id="section-6">
 <h1>19.0.2.0.5</h1>
 <ul class="simple">
 <li>fix(security): add a global <tt class="docutils literal">ir.rule</tt> on <tt class="docutils literal">spp.change.request</tt> that
@@ -1411,27 +1427,27 @@ <h1>19.0.2.0.5</h1>
 roles).</li>
 </ul>
 </div>
-<div class="section" id="section-6">
+<div class="section" id="section-7">
 <h1>19.0.2.0.3</h1>
 <ul class="simple">
 <li>fix: add HTML escaping to all computed Html fields with
 <tt class="docutils literal">sanitize=False</tt> to prevent stored XSS (#50)</li>
 </ul>
 </div>
-<div class="section" id="section-7">
+<div class="section" id="section-8">
 <h1>19.0.2.0.2</h1>
 <ul class="simple">
 <li>fix: fix batch approval wizard line deletion (#130)</li>
 </ul>
 </div>
-<div class="section" id="section-8">
+<div class="section" id="section-9">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>fix: skip field types before getattr and isolate detail prefetch
 (#129)</li>
 </ul>
 </div>
-<div class="section" id="section-9">
+<div class="section" id="section-10">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>