From 44ea07330603703ea59fdbe9785ac3ff944c9ad3 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Mon, 2 Nov 2026 01:51:15 -0500 Subject: [PATCH 1/5] Apply suggested fix to django_forms_workflows/form_builder_views.py from Copilot Autofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> --- django_forms_workflows/form_builder_views.py | 36 ++++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/django_forms_workflows/form_builder_views.py b/django_forms_workflows/form_builder_views.py index b7c3157..1824773 100644 --- a/django_forms_workflows/form_builder_views.py +++ b/django_forms_workflows/form_builder_views.py @@ -528,6 +528,13 @@ def save_form_definition_from_builder_data(data, user, form_definition=None): field_id_mapping = {} # Map old IDs to new IDs for frontend update # Create or update fields + # Pre-compute valid workflow stages for this form_definition + allowed_workflow_stage_ids = set( + WorkflowStage.objects.filter( + workflow_definition__form_definition=form_definition + ).values_list("id", flat=True) + ) + for field_data in fields_data: field_id = field_data.get("id") old_id = field_id # Store original ID for mapping @@ -586,14 +593,37 @@ def save_form_definition_from_builder_data(data, user, form_definition=None): # Add workflow stage FK (for staged approval workflows) wf_stage_id = field_data.get("workflow_stage_id") if wf_stage_id is not None: - field_props["workflow_stage_id"] = int(wf_stage_id) if wf_stage_id else None + if wf_stage_id: + try: + wf_stage_id_int = int(wf_stage_id) + except (TypeError, ValueError): + return JsonResponse( + {"error": f"Invalid workflow_stage_id for field '{field_data.get('field_name', '')}'"}, + status=400, + ) + + if wf_stage_id_int not in allowed_workflow_stage_ids: + return JsonResponse( + {"error": f"workflow_stage_id {wf_stage_id_int} is not valid for this form"}, + status=400, + ) + field_props["workflow_stage_id"] = wf_stage_id_int + else: + field_props["workflow_stage_id"] = None else: field_props["workflow_stage_id"] = None # Create or update field if field_id and isinstance(field_id, int): - # Update existing field - FormField.objects.filter(id=field_id).update(**field_props) + # Update existing field, scoped to current form_definition + updated_count = FormField.objects.filter( + id=field_id, form_definition=form_definition + ).update(**field_props) + if updated_count == 0: + return JsonResponse( + {"error": f"Invalid field id {field_id} for this form"}, + status=400, + ) updated_field_ids.add(field_id) field_id_mapping[str(old_id)] = field_id else: From 01d927721caf70271379631b66f583989fd53c95 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Mon, 2 Nov 2026 01:51:15 -0500 Subject: [PATCH 2/5] Apply suggested fix to django_forms_workflows/form_builder_views.py from Copilot Autofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> --- django_forms_workflows/form_builder_views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/django_forms_workflows/form_builder_views.py b/django_forms_workflows/form_builder_views.py index 1824773..58c1a90 100644 --- a/django_forms_workflows/form_builder_views.py +++ b/django_forms_workflows/form_builder_views.py @@ -153,6 +153,7 @@ def form_builder_clone(request, form_id): # Copy group permissions cloned_form.submit_groups.set(original_form.submit_groups.all()) cloned_form.view_groups.set(original_form.view_groups.all()) + cloned_form.reviewer_groups.set(original_form.reviewer_groups.all()) cloned_form.admin_groups.set(original_form.admin_groups.all()) # Clone workflows, stages, and stage approval groups From fdc857cd3bdfcf7c51a3f7c0346ef6013da675d7 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Mon, 2 Nov 2026 02:22:14 -0500 Subject: [PATCH 3/5] fix: repair form builder save validation Filter stages on the real workflow FK; workflow_definition raised FieldError and turned every builder save into a 500. Raise FormBuilderPayloadError instead of returning a JsonResponse from the helper. Its caller unpacks a tuple, so the rejection could never reach the client, and raising also rolls back the partial save. Add tests for stage and field scoping, rollback, and cloned reviewer groups. Co-Authored-By: Claude Opus 5.5 --- django_forms_workflows/form_builder_views.py | 49 +++--- tests/test_builders.py | 163 +++++++++++++++++++ 2 files changed, 188 insertions(+), 24 deletions(-) diff --git a/django_forms_workflows/form_builder_views.py b/django_forms_workflows/form_builder_views.py index 58c1a90..ee6a3fe 100644 --- a/django_forms_workflows/form_builder_views.py +++ b/django_forms_workflows/form_builder_views.py @@ -404,6 +404,9 @@ def form_builder_save(request): } ) + except FormBuilderPayloadError as exc: + # Raised inside the atomic block above, so the partial save is rolled back. + return JsonResponse({"success": False, "error": str(exc)}, status=400) except json.JSONDecodeError: return JsonResponse( {"success": False, "error": "Invalid JSON data"}, status=400 @@ -415,6 +418,10 @@ def form_builder_save(request): ) +class FormBuilderPayloadError(ValueError): + """The builder payload is invalid; the message is safe to return to the client.""" + + def save_form_definition_from_builder_data(data, user, form_definition=None): """ Create or update a FormDefinition and its FormFields from form builder @@ -528,14 +535,14 @@ def save_form_definition_from_builder_data(data, user, form_definition=None): updated_field_ids = set() field_id_mapping = {} # Map old IDs to new IDs for frontend update - # Create or update fields - # Pre-compute valid workflow stages for this form_definition + # Stages a field on this form may be attached to allowed_workflow_stage_ids = set( WorkflowStage.objects.filter( - workflow_definition__form_definition=form_definition + workflow__form_definition=form_definition ).values_list("id", flat=True) ) + # Create or update fields for field_data in fields_data: field_id = field_data.get("id") old_id = field_id # Store original ID for mapping @@ -593,24 +600,19 @@ def save_form_definition_from_builder_data(data, user, form_definition=None): # Add workflow stage FK (for staged approval workflows) wf_stage_id = field_data.get("workflow_stage_id") - if wf_stage_id is not None: - if wf_stage_id: - try: - wf_stage_id_int = int(wf_stage_id) - except (TypeError, ValueError): - return JsonResponse( - {"error": f"Invalid workflow_stage_id for field '{field_data.get('field_name', '')}'"}, - status=400, - ) - - if wf_stage_id_int not in allowed_workflow_stage_ids: - return JsonResponse( - {"error": f"workflow_stage_id {wf_stage_id_int} is not valid for this form"}, - status=400, - ) - field_props["workflow_stage_id"] = wf_stage_id_int - else: - field_props["workflow_stage_id"] = None + if wf_stage_id: + try: + wf_stage_id = int(wf_stage_id) + except (TypeError, ValueError) as exc: + raise FormBuilderPayloadError( + "Invalid workflow_stage_id for field " + f"'{field_data.get('field_name', '')}'" + ) from exc + if wf_stage_id not in allowed_workflow_stage_ids: + raise FormBuilderPayloadError( + f"workflow_stage_id {wf_stage_id} is not valid for this form" + ) + field_props["workflow_stage_id"] = wf_stage_id else: field_props["workflow_stage_id"] = None @@ -621,9 +623,8 @@ def save_form_definition_from_builder_data(data, user, form_definition=None): id=field_id, form_definition=form_definition ).update(**field_props) if updated_count == 0: - return JsonResponse( - {"error": f"Invalid field id {field_id} for this form"}, - status=400, + raise FormBuilderPayloadError( + f"Invalid field id {field_id} for this form" ) updated_field_ids.add(field_id) field_id_mapping[str(old_id)] = field_id diff --git a/tests/test_builders.py b/tests/test_builders.py index 9ac7ec2..e4a3fc5 100644 --- a/tests/test_builders.py +++ b/tests/test_builders.py @@ -13,10 +13,12 @@ ) from django_forms_workflows.models import ( FormDefinition, + FormField, NotificationRule, PostSubmissionAction, StageApprovalGroup, WorkflowDefinition, + WorkflowStage, ) from django_forms_workflows.workflow_builder_views import ( convert_visual_to_workflow, @@ -1388,6 +1390,146 @@ def test_keeps_fields_present_in_the_payload_by_id(self, user, form_with_fields) # ── Form builder view tests ───────────────────────────────────────────────── +class TestFormBuilderSavePayloadValidation: + """form_builder_save must keep fields and stages scoped to the form being + saved, and reject bad references without persisting any of the save.""" + + @staticmethod + def _save(client, form, fields): + return client.post( + _fb_url("builder/api/save/"), + data=json.dumps( + { + "id": form.id, + "name": form.name, + "slug": form.slug, + "fields": fields, + } + ), + content_type="application/json", + ) + + @staticmethod + def _field_payload(field, **overrides): + return { + "id": field.id, + "field_name": field.field_name, + "field_label": "Changed Label", + "field_type": field.field_type, + "order": field.order, + **overrides, + } + + @pytest.fixture + def other_form(self, db): + return FormDefinition.objects.create( + name="Other Form", slug="other-form", description="", instructions="" + ) + + @pytest.fixture + def field(self, form_with_fields): + return form_with_fields.fields.get(field_name="full_name") + + def test_assigns_stage_from_same_form( + self, client, superuser, form_with_fields, field + ): + wf = WorkflowDefinition.objects.create(form_definition=form_with_fields) + stage = WorkflowStage.objects.create(workflow=wf, name="Review", order=1) + client.force_login(superuser) + + resp = self._save( + client, + form_with_fields, + [self._field_payload(field, workflow_stage_id=str(stage.id))], + ) + + assert resp.status_code == 200 + field.refresh_from_db() + assert field.workflow_stage_id == stage.id + + def test_clears_stage_when_blank(self, client, superuser, form_with_fields, field): + wf = WorkflowDefinition.objects.create(form_definition=form_with_fields) + field.workflow_stage = WorkflowStage.objects.create( + workflow=wf, name="Review", order=1 + ) + field.save() + client.force_login(superuser) + + resp = self._save( + client, form_with_fields, [self._field_payload(field, workflow_stage_id="")] + ) + + assert resp.status_code == 200 + field.refresh_from_db() + assert field.workflow_stage_id is None + + def test_rejects_stage_from_another_form_and_rolls_back( + self, client, superuser, form_with_fields, field, other_form + ): + wf = WorkflowDefinition.objects.create(form_definition=other_form) + foreign_stage = WorkflowStage.objects.create( + workflow=wf, name="Foreign", order=1 + ) + original_version = form_with_fields.version + original_label = field.field_label + client.force_login(superuser) + + resp = self._save( + client, + form_with_fields, + [self._field_payload(field, workflow_stage_id=foreign_stage.id)], + ) + + assert resp.status_code == 400 + assert resp.json()["success"] is False + assert "not valid for this form" in resp.json()["error"] + form_with_fields.refresh_from_db() + field.refresh_from_db() + assert form_with_fields.version == original_version + assert field.field_label == original_label + assert field.workflow_stage_id is None + + def test_rejects_malformed_stage_id( + self, client, superuser, form_with_fields, field + ): + client.force_login(superuser) + + resp = self._save( + client, + form_with_fields, + [self._field_payload(field, workflow_stage_id="not-a-number")], + ) + + assert resp.status_code == 400 + assert "Invalid workflow_stage_id" in resp.json()["error"] + + def test_rejects_field_from_another_form_and_rolls_back( + self, client, superuser, form_with_fields, field, other_form + ): + foreign_field = FormField.objects.create( + form_definition=other_form, + field_name="foreign", + field_label="Foreign Label", + field_type="text", + order=1, + ) + original_field_count = form_with_fields.fields.count() + client.force_login(superuser) + + resp = self._save( + client, form_with_fields, [self._field_payload(foreign_field)] + ) + + assert resp.status_code == 400 + assert "Invalid field id" in resp.json()["error"] + foreign_field.refresh_from_db() + assert foreign_field.form_definition_id == other_form.id + assert foreign_field.field_label == "Foreign Label" + # The removal of this form's own fields must be rolled back as well. + assert form_with_fields.fields.count() == original_field_count + + +@pytest.mark.django_db class TestFormBuilderViews: """Integration tests for the form builder HTTP endpoints.""" @@ -1477,6 +1619,27 @@ def test_clone_form(self, client, superuser, form_with_fields): assert cloned.id != form_with_fields.id assert cloned.fields.count() == form_with_fields.fields.count() + def test_clone_copies_all_permission_groups( + self, client, superuser, form_with_fields, approval_group + ): + for groups in ( + form_with_fields.submit_groups, + form_with_fields.view_groups, + form_with_fields.reviewer_groups, + form_with_fields.admin_groups, + ): + groups.set([approval_group]) + client.force_login(superuser) + + resp = client.post(_fb_url(f"builder/api/clone/{form_with_fields.id}/")) + + assert resp.status_code == 200 + cloned = FormDefinition.objects.get(id=resp.json()["form_id"]) + assert list(cloned.submit_groups.all()) == [approval_group] + assert list(cloned.view_groups.all()) == [approval_group] + assert list(cloned.reviewer_groups.all()) == [approval_group] + assert list(cloned.admin_groups.all()) == [approval_group] + def test_clone_requires_staff(self, client, user, form_with_fields): client.force_login(user) url = _fb_url(f"builder/api/clone/{form_with_fields.id}/") From 008fd8a7287163d18d3576cc5f3ec24771288af2 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Mon, 2 Nov 2026 02:22:14 -0500 Subject: [PATCH 4/5] chore: bump pyasn1 to 0.6.5 for audit failures Resolves CVE-2026-119194, CVE-2026-119195, CVE-2026-119196 and CVE-2026-119197, which fail the CI dependency audit. Co-Authored-By: Claude Opus 5.5 --- poetry.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/poetry.lock b/poetry.lock index 66656e0..05c26d3 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2636,15 +2636,15 @@ files = [ [[package]] name = "pyasn1" -version = "0.6.4" +version = "0.6.5" description = "Pure-Python implementation of ASN.1 types and DER/BER/CER codecs (X.208)" optional = true python-versions = ">=3.8" groups = ["main"] markers = "extra == \"ldap\" or extra == \"all\" or extra == \"gmail\"" files = [ - {file = "pyasn1-0.6.4-py3-none-any.whl", hash = "sha256:deda9277cfd454080ec40b207fb6df82206a3a2688735233cdcd8d3d565f088b"}, - {file = "pyasn1-0.6.4.tar.gz", hash = "sha256:9c447d8431c947fe4c8febc4ed9e760bc29011a5b01e5c74b67025bd9fb8ce81"}, + {file = "pyasn1-0.6.5-py3-none-any.whl", hash = "sha256:4017b0fce125096b8573b39c989fdb1a4b19cf761e683398184550fea0e66587"}, + {file = "pyasn1-0.6.5.tar.gz", hash = "sha256:6b11616a6bcf3a40e635f082bb232fa57b5e0dd7f517bd7ddd69ec3794508a47"}, ] [[package]] From 6e9366a8e46ce80af8469742fe19f5e5be8cd890 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Mon, 2 Nov 2026 02:38:49 -0500 Subject: [PATCH 5/5] fix: return only an explicit client message from builder errors Stop stringifying the caught exception in form_builder_save, which CodeQL flagged as py/stack-trace-exposure. The specific validation messages stay available to the builder UI through FormBuilderPayloadError.client_message. Co-Authored-By: Claude Opus 5.5 --- django_forms_workflows/form_builder_views.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/django_forms_workflows/form_builder_views.py b/django_forms_workflows/form_builder_views.py index ee6a3fe..95db09e 100644 --- a/django_forms_workflows/form_builder_views.py +++ b/django_forms_workflows/form_builder_views.py @@ -406,7 +406,7 @@ def form_builder_save(request): except FormBuilderPayloadError as exc: # Raised inside the atomic block above, so the partial save is rolled back. - return JsonResponse({"success": False, "error": str(exc)}, status=400) + return JsonResponse({"success": False, "error": exc.client_message}, status=400) except json.JSONDecodeError: return JsonResponse( {"success": False, "error": "Invalid JSON data"}, status=400 @@ -419,7 +419,15 @@ def form_builder_save(request): class FormBuilderPayloadError(ValueError): - """The builder payload is invalid; the message is safe to return to the client.""" + """The builder payload is invalid. + + ``client_message`` is the only text returned to the client, so exception + details can never reach the response by accident. + """ + + def __init__(self, client_message): + super().__init__(client_message) + self.client_message = client_message def save_form_definition_from_builder_data(data, user, form_definition=None):