Merge remote-tracking branch 'origin/pr/447'

marmarek · marmarek · commit 4164ac4a84e8 · 2026-04-12T20:16:53.000+02:00
* origin/pr/447:
  straight forward placeholder
  firewall: Add comment field to firewall rules
diff --git a/qubesmanager/firewall.py b/qubesmanager/firewall.py
@@ -107,6 +107,10 @@ def try_to_create_rule(self):
                             "invalid.".format(parsed_service)))
                     return False
 
+        comment_text = self.commentLineEdit.text().strip()
+        if comment_text:
+            rule.comment = comment_text
+
         if self.model.current_row is not None:
             self.model.set_child(self.model.current_row, rule)
         else:
@@ -173,7 +177,12 @@ def __init__(self, parent=None):
         self.current_row = None
         self.current_dialog = None
 
-        self.__column_names = {0: "Address", 1: "Port/Service", 2: "Protocol", }
+        self.__column_names = {
+            0: "Address",
+            1: "Port/Service",
+            2: "Protocol",
+            3: "Comment",
+        }
         self.__services = []
 
         self.port_range_pattern = re.compile(r'\d+-\d+')
@@ -243,6 +252,13 @@ def get_column_string(self, col, rule):
             if rule.proto is None:
                 return "any"
             return str(rule.proto)
+
+        # Comment
+        if col == 3:
+            if rule.comment is None:
+                return ""
+            return str(rule.comment)
+
         return "unknown"
 
     def get_firewall_conf(self, vm):
@@ -392,6 +408,8 @@ def populate_edit_dialog(self, dialog, row):
             dialog.udp_radio.setChecked(True)
         else:
             dialog.any_radio.setChecked(True)
+        comment = self.get_column_string(3, self.children[row])
+        dialog.commentLineEdit.setText(comment)
 
     def run_rule_dialog(self, dialog, row=None):
         self.current_row = row
diff --git a/qubesmanager/i18n/qubesmanager_en.ts b/qubesmanager/i18n/qubesmanager_en.ts
@@ -793,6 +793,21 @@ p, li { white-space: pre-wrap; }
         <source>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;Port/service can be provided as either port number (e.g. 122), port range (1024-1234) or service name (e.g. smtp) . For full list of services known, see /etc/services in dom0.&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <location filename="../ui_newfwruledlg.py" line="105"/>
+        <source>Comment</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <location filename="../ui_newfwruledlg.py" line="106"/>
+        <source>Optional note for your own reference.</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <location filename="../ui_newfwruledlg.py" line="107"/>
+        <source>Optional note (e.g. reason for this rule)</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>QubeManager</name>
diff --git a/qubesmanager/i18n/qubesmanager_es.ts b/qubesmanager/i18n/qubesmanager_es.ts
@@ -815,6 +815,21 @@ p, li { white-space: pre-wrap; }
         <source>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;Port/service can be provided as either port number (e.g. 122), port range (1024-1234) or service name (e.g. smtp) . For full list of services known, see /etc/services in dom0.&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <location filename="../ui_newfwruledlg.py" line="105"/>
+        <source>Comment</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <location filename="../ui_newfwruledlg.py" line="106"/>
+        <source>Optional note for your own reference.</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <location filename="../ui_newfwruledlg.py" line="107"/>
+        <source>Optional note (e.g. reason for this rule)</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>QubeManager</name>
diff --git a/qubesmanager/tests/test_vm_settings.py b/qubesmanager/tests/test_vm_settings.py
@@ -1459,6 +1459,42 @@ def test_305_firewall_edit_rule(settings_fixture):
     assert expected_call in settings_window.qubesapp.actual_calls
 
 
+@check_errors
+@pytest.mark.parametrize("settings_fixture", ["test-vm-set"], indirect=True)
+def test_305b_firewall_add_rule_with_comment(settings_fixture):
+    settings_window, page, vm_name = settings_fixture
+
+    assert settings_window.policy_deny_radio_button.isChecked()
+
+    settings_window.new_rule_button.click()
+    settings_window.fw_model.current_dialog.addressComboBox.setCurrentText(
+        "example.com"
+    )
+    settings_window.fw_model.current_dialog.commentLineEdit.setText(
+        "temporary access for testing"
+    )
+    settings_window.fw_model.current_dialog.buttonBox.button(
+        QtWidgets.QDialogButtonBox.StandardButton.Ok
+    ).click()
+
+    expected_call = (
+        "test-vm-set",
+        "admin.vm.firewall.Set",
+        None,
+        b"action=accept dsthost=qubes-os.org\n"
+        b"action=accept dsthost=example.com"
+        b" comment=temporary access for testing\n"
+        b"action=accept specialtarget=dns\n"
+        b"action=accept proto=icmp\n"
+        b"action=drop\n",
+    )
+    assert expected_call not in settings_window.qubesapp.actual_calls
+    settings_window.qubesapp.expected_calls[expected_call] = b"0\x00"
+
+    settings_window.accept()
+
+    assert expected_call in settings_window.qubesapp.actual_calls
+
 @check_errors
 @pytest.mark.parametrize("settings_fixture", ["test-vm-set"], indirect=True)
 def test_306_firewall_unlimit(settings_fixture):
@@ -1494,6 +1530,66 @@ def test_306_firewall_unlimit(settings_fixture):
     assert expected_call in settings_window.qubesapp.actual_calls
 
 
+@check_errors
+@mock.patch("subprocess.check_output")
+def test_306b_firewall_preserve_cli_comment(mock_subprocess, qapp, test_qubes_app):
+    """Rules created via CLI with comments should survive GUI round-trip."""
+    mock_subprocess.return_value = b""
+
+    fw_rules = [
+        {"action": "accept", "dsthost": "gitlab.com",
+         "comment": "IP for gitlab at the time"},
+        {"action": "accept", "specialtarget": "dns"},
+        {"action": "accept", "proto": "icmp"},
+        {"action": "drop"},
+    ]
+
+    test_qubes_app._qubes["test-vm-comment"] = MockQube(
+        name="test-vm-comment",
+        qapp=test_qubes_app,
+        label="green",
+        firewall_rules=fw_rules,
+    )
+    test_qubes_app.expected_calls[
+        ("test-vm-comment", "admin.vm.notes.Get", None, None)
+    ] = b"0\x00"
+    test_qubes_app.update_vm_calls()
+
+    settings_window = vm_settings.VMSettingsWindow(
+        "test-vm-comment", "firewall", qapp, test_qubes_app
+    )
+
+    # Comment should be visible in the model
+    assert settings_window.fw_model.get_column_string(
+        3, settings_window.fw_model.children[0]
+    ) == "IP for gitlab at the time"
+
+    # Edit the rule (opens dialog, accept it unchanged) to trigger fw_changed
+    settings_window.rulesTreeView.setCurrentIndex(
+        settings_window.fw_model.index(0, 0))
+    settings_window.edit_rule_button.click()
+    settings_window.fw_model.current_dialog.buttonBox.button(
+        QtWidgets.QDialogButtonBox.StandardButton.Ok
+    ).click()
+
+    # The comment must survive the edit round-trip
+    expected_call = (
+        "test-vm-comment",
+        "admin.vm.firewall.Set",
+        None,
+        b"action=accept dsthost=gitlab.com"
+        b" comment=IP for gitlab at the time\n"
+        b"action=accept specialtarget=dns\n"
+        b"action=accept proto=icmp\n"
+        b"action=drop\n",
+    )
+    settings_window.qubesapp.expected_calls[expected_call] = b"0\x00"
+
+    settings_window.accept()
+
+    assert expected_call in settings_window.qubesapp.actual_calls
+
+
 @check_errors
 @mock.patch("subprocess.check_output")
 def test_307_open_with_limit(mock_subprocess, qapp, test_qubes_app):
diff --git a/ui/newfwruledlg.ui b/ui/newfwruledlg.ui
@@ -10,7 +10,7 @@
     <x>0</x>
     <y>0</y>
     <width>381</width>
-    <height>193</height>
+    <height>225</height>
    </rect>
   </property>
   <property name="windowTitle">
@@ -105,8 +105,28 @@
         <bool>true</bool>
        </property>
       </widget>
-     </item>
-     <item row="1" column="1">
+      </item>
+      <item row="3" column="0">
+      <widget class="QLabel" name="label_5">
+       <property name="text">
+        <string>Comment</string>
+       </property>
+      </widget>
+      </item>
+      <item row="3" column="1" colspan="3">
+      <widget class="QLineEdit" name="commentLineEdit">
+       <property name="toolTip">
+        <string>Comments are for user reference only</string>
+       </property>
+       <property name="placeholderText">
+        <string>Comment (optional)</string>
+       </property>
+       <property name="maxLength">
+        <number>512</number>
+       </property>
+      </widget>
+      </item>
+      <item row="1" column="1">
       <widget class="QRadioButton" name="tcp_radio">
        <property name="sizePolicy">
         <sizepolicy hsizetype="Expanding" vsizetype="Fixed">
@@ -152,6 +172,7 @@
   <tabstop>udp_radio</tabstop>
   <tabstop>any_radio</tabstop>
   <tabstop>serviceComboBox</tabstop>
+  <tabstop>commentLineEdit</tabstop>
   <tabstop>buttonBox</tabstop>
  </tabstops>
  <resources/>
