From 3730a95c020250699aa4fd20c626ce9325829a69 Mon Sep 17 00:00:00 2001
From: catborise <catborise@gmail.com>
Date: Sat, 24 Oct 2026 19:57:34 +0300
Subject: [PATCH] fix(instances): the XML dialog keeps the edit and defines
 only this VM

Found by review (Codex and agy) of the XML editor's dialog:

- A busy or moved VM made the page reload and the edit was lost. The
  dialog's requests now get these answers as JSON (X-Requested-With, as the
  create page's flavor forms) and show them; any other failure says the
  edit is still in the editor.
- libvirt finds the VM to define by the XML's UUID: another VM's UUID
  overwrote that VM past its lock, a new or missing one added a VM. The
  XML must keep this VM's UUID; a malformed one is refused with the
  parser's error, as on the create page.
- Save sends the XML whose diff the dialog shows, also when two previews
  answer out of order; a failed save keeps the diff and Save.
- The edited XML is defined with the line breaks libvirt gave, not the
  form's CRLF, and the VM's state is read before the definition, so a
  failed read cannot turn a saved change into an error.
---
 .../templates/instances/settings_tab.html     |  2 +-
 instances/tests/test_change_xml.py            | 45 +++++++++++++++++--
 instances/views.py                            | 39 +++++++++-------
 static/js/xml_editor.js                       | 34 +++++++-------
 4 files changed, 85 insertions(+), 35 deletions(-)

diff --git a/instances/templates/instances/settings_tab.html b/instances/templates/instances/settings_tab.html
index 1b2ec0e4..d874d7a4 100644
--- a/instances/templates/instances/settings_tab.html
+++ b/instances/templates/instances/settings_tab.html
@@ -669,7 +669,7 @@
             </form>
             <div class="clearfix"></div>
             <div class="modal fade" id="xml-review" tabindex="-1" aria-labelledby="xml-review-title" aria-hidden="true"
-                 data-failed="{% trans "The XML could not be checked. Try again." %}">
+                 data-failed="{% trans "The request did not go through; your edit is still in the editor. Try again, or copy it and reload the page." %}">
                 <div class="modal-dialog modal-xl modal-dialog-scrollable">
                     <div class="modal-content">
                         <div class="modal-header">
diff --git a/instances/tests/test_change_xml.py b/instances/tests/test_change_xml.py
index 058d0d82..88515de7 100644
--- a/instances/tests/test_change_xml.py
+++ b/instances/tests/test_change_xml.py
@@ -9,10 +9,12 @@ from instances.models import Instance
 from libvirt import libvirtError
 from vrtManager.instance import PERSISTENT_XML
 from logs.models import Logs
+from operations import claims
 
-CURRENT = "<domain>\n  <name>xml-vm</name>\n  <memory>1024</memory>\n</domain>"
-LIVE = "<domain id='3'>\n  <name>xml-vm</name>\n  <memory>512</memory>\n</domain>"
-EDITED = "<domain>\n  <name>xml-vm</name>\n  <memory>2048</memory>\n</domain>"
+UUID = "11111111-2222-3333-4444-888888888888"
+CURRENT = f"<domain>\n  <name>xml-vm</name>\n  <uuid>{UUID}</uuid>\n  <memory>1024</memory>\n</domain>"
+LIVE = f"<domain id='3'>\n  <name>xml-vm</name>\n  <uuid>{UUID}</uuid>\n  <memory>512</memory>\n</domain>"
+EDITED = CURRENT.replace("1024", "2048")
 
 
 class ChangeXmlTestCase(TestCase):
@@ -23,7 +25,7 @@ class ChangeXmlTestCase(TestCase):
     def setUp(self):
         self.client.force_login(get_user_model().objects.create_superuser("xml_admin", "x@example.com", "pw"))
         compute = Compute.objects.create(name="xml-compute", hostname="127.0.0.1", login="", password="", type=4)
-        self.instance = Instance.objects.create(compute=compute, name="xml-vm", uuid="11111111-2222-3333-4444-888888888888")
+        self.instance = Instance.objects.create(compute=compute, name="xml-vm", uuid=UUID)
 
     def post(self, status=5, xml=EDITED, base=CURRENT, confirm=False, define_error=None):
         data = {"inst_xml": xml, "base": base}
@@ -85,3 +87,38 @@ class ChangeXmlTestCase(TestCase):
         response, define = self.post(xml="  ", confirm=True)
         define.assert_not_called()
         self.assertEqual(response.status_code, 400)
+
+    def test_a_busy_vm_answers_the_dialog_so_the_edit_stays(self):
+        # the dialog's script sends X-Requested-With; a page form gets the redirect
+        with patch("instances.views.libvirt_instance_lock", side_effect=claims.Busy("instance:xml-vm")), \
+                patch("instances.models.wvmInstance") as wvm:
+            url = reverse("instances:change_xml", args=[self.instance.id])
+            data = {"inst_xml": EDITED, "base": CURRENT, "confirm": "1"}
+            response = self.client.post(url, data, HTTP_X_REQUESTED_WITH="XMLHttpRequest")
+            self.assertEqual(response.status_code, 409)
+            self.assertIn("try again", response.json()["detail"])
+            self.assertEqual(list(get_messages(response.wsgi_request)), [])
+            self.assertEqual(self.client.post(url, data).status_code, 302)
+        wvm.return_value._defineXML.assert_not_called()
+
+    def test_the_xml_must_keep_the_vms_uuid(self):
+        # libvirt finds the VM to define by the XML's UUID: another VM's would
+        # overwrite that VM, a new or missing one would add a VM
+        other = CURRENT.replace(UUID, "99999999-2222-3333-4444-888888888888")
+        for xml in (other, CURRENT.replace(f"  <uuid>{UUID}</uuid>\n", "")):
+            for confirm in (False, True):
+                with self.subTest(xml=xml, confirm=confirm):
+                    response, define = self.post(xml=xml, confirm=confirm)
+                    define.assert_not_called()
+                    self.assertEqual(response.status_code, 400)
+                    self.assertIn(UUID, response.json()["error"])
+
+    def test_a_malformed_xml_is_refused_with_the_parser_error(self):
+        response, define = self.post(xml="<domain><name>xml-vm</domain>", confirm=True)
+        define.assert_not_called()
+        self.assertEqual(response.status_code, 400)
+        self.assertIn("not well-formed", response.json()["error"])
+
+    def test_the_edited_xml_is_defined_with_the_line_breaks_libvirt_gave(self):
+        _, define = self.post(xml=EDITED.replace("\n", "\r\n"), confirm=True)
+        define.assert_called_once_with(EDITED)
diff --git a/instances/views.py b/instances/views.py
index f27008d3..39187e3a 100755
--- a/instances/views.py
+++ b/instances/views.py
@@ -431,8 +431,14 @@ def get_instance(user, pk, perm_type="view"):
     return instance
 
 
+def from_script(request):
+    """A form a page's script sends and answers itself: the create page's
+    flavor forms (it swaps in the reply), the XML editor's dialog (JSON)."""
+    return request.headers.get("X-Requested-With") == "XMLHttpRequest"
+
+
 def _busy(request, pk, busy):
-    if hasattr(request, "accepted_renderer"):
+    if hasattr(request, "accepted_renderer") or from_script(request):
         blocker = busy.blocker
         return JsonResponse({"detail": busy_message(busy, request.user),
                              "operation": summary(blocker, request.user) if blocker else None}, status=409)
@@ -441,7 +447,7 @@ def _busy(request, pk, busy):
 
 
 def _moved(request, pk):
-    if hasattr(request, "accepted_renderer"):
+    if hasattr(request, "accepted_renderer") or from_script(request):
         return JsonResponse({"detail": "The VM was moved meanwhile; try again."}, status=409)
     messages.error(request, _("The VM was moved meanwhile; try again."))
     return get_safe_redirect(request, default=reverse("instances:instance", args=[pk]))
@@ -1460,13 +1466,21 @@ def change_xml(request, pk):
     in the persistent definition, to review; with confirm, the edited XML is
     defined. A running VM uses it at its next start.
     Only over the definition the editor was opened with (base), so a change
-    made since is not overwritten; a form posts its line breaks as CRLF. A refusal comes back to the dialog and the
-    edited XML stays in the editor.
+    made since is not overwritten, and only with the VM's UUID, by which
+    libvirt finds the VM to define. A form posts its line breaks as CRLF. A
+    refusal comes back to the dialog and the edited XML stays in the editor.
     """
     instance = get_instance(request.user, pk)
-    new_xml = request.POST.get("inst_xml", "")
+    new_xml = request.POST.get("inst_xml", "").replace("\r\n", "\n")
     if not new_xml.strip():
         return JsonResponse({"error": _("The XML is empty")}, status=400)
+    try:
+        uuid = util.get_xml_path(new_xml, "/domain/uuid")
+    except util.etree.Error as err:
+        return JsonResponse({"error": _("The XML is not well-formed: %(error)s") % {"error": err}}, status=400)
+    if (uuid or "").strip().lower() != instance.uuid:
+        return JsonResponse({"error": _("The XML must keep the VM's UUID %(uuid)s.") % {"uuid": instance.uuid}},
+                            status=400)
     current = instance.inst_xml
     if request.POST.get("base", "").replace("\r\n", "\n") != current:
         return JsonResponse({"error": _("The definition has changed since the editor was opened. "
@@ -1474,18 +1488,18 @@ def change_xml(request, pk):
     if "confirm" not in request.POST:
         diff = difflib.unified_diff(current.splitlines(), new_xml.splitlines(), "current", "new", lineterm="")
         return JsonResponse({"diff": list(diff)})
+    shut_off = instance.proxy.get_status() == 5
     try:
         with audit_vm(request, instance, "change XML"):
             instance.proxy._defineXML(new_xml)
     except libvirtError as err:
         return JsonResponse({"error": error_text(request, err)}, status=400)
-    if instance.proxy.get_status() == 5:
+    if shut_off:
         messages.success(request, _("The XML definition was saved."))
     else:
         messages.success(request, _("The XML definition was saved; the VM uses it from its next start."))
     return JsonResponse({"saved": True})
 
-
 @require_POST
 @superuser_only
 @serialize_instance_mutation
@@ -2101,11 +2115,6 @@ def create_instance(request, compute_id, arch, machine):
     return render(request, "create_instance_w2.html", locals())
 
 
-def from_create_page(request):
-    """A flavor form sent from the create page's script, which swaps in the reply."""
-    return request.headers.get("X-Requested-With") == "XMLHttpRequest"
-
-
 def flavors_reply(request, flavor_form):
     return render(request, "create_flavors_reply.html",
                   {"flavors": Flavor.objects.order_by("id"), "flavor_form": flavor_form})
@@ -2117,11 +2126,11 @@ def flavor_create(request):
     if form.is_valid():
         with audit(request, None, f"Flavor {form.cleaned_data['label']}", "add"):
             form.save()
-        if from_create_page(request):
+        if from_script(request):
             return flavors_reply(request, FlavorForm())
         messages.success(request, _("Flavor Created"))
         return get_safe_redirect(request, default=reverse("instances:index"))
-    if from_create_page(request):
+    if from_script(request):
         return flavors_reply(request, form)
 
     return render(
@@ -2158,7 +2167,7 @@ def flavor_delete(request, pk):
     if request.method == "POST":
         with audit(request, None, f"Flavor {flavor.label}", "delete"):
             flavor.delete()
-        if from_create_page(request):
+        if from_script(request):
             return flavors_reply(request, FlavorForm())
         messages.success(request, _("Flavor Deleted"))
         return get_safe_redirect(request, default=reverse("instances:index"))
diff --git a/static/js/xml_editor.js b/static/js/xml_editor.js
index fa7b6aa7..f2527856 100644
--- a/static/js/xml_editor.js
+++ b/static/js/xml_editor.js
@@ -17,7 +17,8 @@
     var save = document.getElementById("xml-review-save");
     var reviewed = ""; // Save defines the XML the dialog shows, not later edits
 
-    // null after a redirect (the VM is busy or moved): the page says why
+    // The answer: diff, saved, or error (detail when the VM is busy or moved).
+    // Any other failure, such as a redirect, keeps the edit and says so.
     function post(xml, confirm) {
         var data = new FormData(form);
         data.set("inst_xml", xml);
@@ -25,12 +26,12 @@
         if (confirm) {
             data.set("confirm", "1");
         }
-        return fetch(form.action, {method: "POST", body: data, redirect: "manual"}).then(function (response) {
-            if (response.type === "opaqueredirect") {
-                location.reload();
-                return null;
-            }
+        return fetch(form.action, {
+            method: "POST", body: data, redirect: "manual", headers: {"X-Requested-With": "XMLHttpRequest"},
+        }).then(function (response) {
             return response.json();
+        }).then(function (answer) {
+            return {diff: answer.diff, saved: answer.saved, error: answer.error || answer.detail};
         }).catch(function () {
             return {error: dialog.dataset.failed};
         });
@@ -56,23 +57,26 @@
 
     form.addEventListener("submit", function (event) {
         event.preventDefault();
-        reviewed = editor.getValue();
-        post(reviewed, false).then(function (answer) {
-            if (answer) {
-                show(answer);
-                modal.show();
-            }
+        var xml = editor.getValue();
+        // the last answer shown is the one Save sends, whatever order answers come in
+        post(xml, false).then(function (answer) {
+            reviewed = xml;
+            show(answer);
+            modal.show();
         });
     });
 
     save.addEventListener("click", function () {
         save.disabled = true;
         post(reviewed, true).then(function (answer) {
-            if (answer && answer.saved) {
+            if (answer.saved) {
                 location.hash = "xmledit";
                 location.reload();
-            } else if (answer) {
-                show({error: answer.error, diff: []});
+            } else {
+                // the diff stays, and Save can be tried again
+                error.textContent = answer.error;
+                error.classList.remove("d-none");
+                save.disabled = false;
             }
         });
     });