Skip to content

firewall: Add comment field to firewall rules #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
marmarek merged 2 commits into from
Apr 12, 2026
Merged

firewall: Add comment field to firewall rules #447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
833c0e6
firewall: Add comment field to firewall rules
nihalxkumar Mar 27, 2026
d4db7b5
straight forward placeholder
nihalxkumar Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion qubesmanager/firewall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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+')
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions qubesmanager/i18n/qubesmanager_en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
15 changes: 15 additions & 0 deletions qubesmanager/i18n/qubesmanager_es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
96 changes: 96 additions & 0 deletions qubesmanager/tests/test_vm_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
27 changes: 24 additions & 3 deletions ui/newfwruledlg.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<x>0</x>
<y>0</y>
<width>381</width>
<height>193</height>
<height>225</height>
</rect>
</property>
<property name="windowTitle">
Expand Down Expand Up @@ -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">
Expand Down Expand Up @@ -152,6 +172,7 @@
<tabstop>udp_radio</tabstop>
<tabstop>any_radio</tabstop>
<tabstop>serviceComboBox</tabstop>
<tabstop>commentLineEdit</tabstop>
<tabstop>buttonBox</tabstop>
</tabstops>
<resources/>
Expand Down