Skip to content
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.

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

Fix markup injection issues #236

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DemiMarie
Copy link

This fixes some (theoretical) markup injection problems. I believe none of the strings that I escape here will ever contain "<" or "&", but it is always safer to escape.

@marmarta
Copy link
Member

merge conflict, otherwise looks fine (but let's tests check it too)

@DemiMarie DemiMarie force-pushed the fix-markup-injection branch 2 times, most recently from 81f3f70 to be5c363 Compare December 13, 2024 02:50
@DemiMarie DemiMarie marked this pull request as draft December 13, 2024 02:53
@DemiMarie DemiMarie force-pushed the fix-markup-injection branch 4 times, most recently from a224c12 to fd9ff2f Compare December 14, 2024 03:09
qui/utils.py Outdated Show resolved Hide resolved
qui/tray/updates.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 14, 2024

Codecov Report

Attention: Patch coverage is 51.51515% with 16 lines in your changes missing coverage. Please review.

Project coverage is 93.04%. Comparing base (1231efc) to head (fa4bcc2).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
qui/utils.py 23.52% 13 Missing ⚠️
qubes_config/widgets/gtk_utils.py 72.72% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #236      +/-   ##
==========================================
- Coverage   93.17%   93.04%   -0.13%     
==========================================
  Files          58       58              
  Lines       11048    11074      +26     
==========================================
+ Hits        10294    10304      +10     
- Misses        754      770      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@qubesos-bot
Copy link

qubesos-bot commented Dec 15, 2024

OpenQA test summary

Complete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025010617-4.3&flavor=pull-requests

Test run included the following:

New failures, excluding unstable

Compared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024111705-4.3&flavor=update

  • system_tests_pvgrub_salt_storage

    • TC_41_HVMGrub_debian-12-xfce: test_000_standalone_vm (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_debian-12-xfce: test_001_standalone_vm_dracut (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_debian-12-xfce: test_010_template_based_vm (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_debian-12-xfce: test_011_template_based_vm_dracut (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_fedora-41-xfce: test_000_standalone_vm (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_fedora-41-xfce: test_010_template_based_vm (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

  • system_tests_extra

    • TC_00_QVCTest_whonix-workstation-17: test_010_screenshare (failure)
      AssertionError: 13.069686413555461 not less than 2.0

    • TC_01_InputProxyExclude_debian-12-xfce: test_000_qemu_tablet (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_01_InputProxyExclude_fedora-41-xfce: test_000_qemu_tablet (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

  • system_tests_manager

    • tests: test_vm_settings (error)
      ModuleNotFoundError: No module named 'pytest'

    • tests: test_clone_vm (error)
      ModuleNotFoundError: No module named 'pytest'

    • tests: test_backup (error)
      ModuleNotFoundError: No module named 'pytest'

    • tests: test_qube_manager (error)
      ModuleNotFoundError: No module named 'pytest'

  • system_tests_kde_gui_interactive

    • gui_keyboard_layout: wait_serial (wait serial expected)
      # wait_serial expected: "echo -e '[Layout]\nLayoutList=us,de' | sud...

Failed tests

15 failures
  • system_tests_pvgrub_salt_storage

    • TC_41_HVMGrub_debian-12-xfce: test_000_standalone_vm (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_debian-12-xfce: test_001_standalone_vm_dracut (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_debian-12-xfce: test_010_template_based_vm (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_debian-12-xfce: test_011_template_based_vm_dracut (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_fedora-41-xfce: test_000_standalone_vm (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_41_HVMGrub_fedora-41-xfce: test_010_template_based_vm (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

  • system_tests_extra

    • TC_00_QVCTest_whonix-workstation-17: test_010_screenshare (failure)
      AssertionError: 13.069686413555461 not less than 2.0

    • TC_01_InputProxyExclude_debian-12-xfce: test_000_qemu_tablet (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

    • TC_01_InputProxyExclude_fedora-41-xfce: test_000_qemu_tablet (error)
      qubes.exc.QubesVMError: Cannot connect to qrexec agent for 120 seco...

  • system_tests_manager

    • tests: test_vm_settings (error)
      ModuleNotFoundError: No module named 'pytest'

    • tests: test_clone_vm (error)
      ModuleNotFoundError: No module named 'pytest'

    • tests: test_backup (error)
      ModuleNotFoundError: No module named 'pytest'

    • tests: test_qube_manager (error)
      ModuleNotFoundError: No module named 'pytest'

  • system_tests_kde_gui_interactive

    • gui_keyboard_layout: wait_serial (wait serial expected)
      # wait_serial expected: "echo -e '[Layout]\nLayoutList=us,de' | sud...

    • gui_keyboard_layout: Failed (test died)
      # Test died: command 'test "$(cd ~user;ls e1*)" = "$(qvm-run -p wor...

Fixed failures

Compared to: https://openqa.qubes-os.org/tests/119126#dependencies

3 fixed
  • system_tests_extra

    • TC_00_QVCTest_whonix-gateway-17: test_010_screenshare (failure)
      ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^... AssertionError: 0 == 0
  • system_tests_basic_vm_qrexec_gui_zfs

    • switch_pool: Failed (test died)
      # Test died: command 'dnf install -y ./zfs-release.rpm' failed at /...
  • system_tests_audio@hw1

Unstable tests

  • system_tests_update

    update2/Failed (1/5 times with errors)
    • job 121711 # Test died: command '(set -o pipefail; qubesctl --show-output stat...
  • system_tests_update@hw7

    update2/Failed (1/5 times with errors)
    • job 121711 # Test died: command '(set -o pipefail; qubesctl --show-output stat...
  • system_tests_update@hw1

    update2/Failed (1/5 times with errors)
    • job 121711 # Test died: command '(set -o pipefail; qubesctl --show-output stat...

@marmarek
Copy link
Member

FWIW openQA says:

Dec 15 19:01:36.069179 dom0 widget-wrapper[3495]: Traceback (most recent call last):
Dec 15 19:01:36.070033 dom0 widget-wrapper[3495]:   File "/usr/lib/python3.13/site-packages/qui/tray/disk_space.py", line 435, in make_menu
Dec 15 19:01:36.070033 dom0 widget-wrapper[3495]:     menu.append(self.make_title_item('Volumes'))
Dec 15 19:01:36.070033 dom0 widget-wrapper[3495]:                 ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
Dec 15 19:01:36.070033 dom0 widget-wrapper[3495]:   File "/usr/lib/python3.13/site-packages/qui/tray/disk_space.py", line 474, in make_title_item
Dec 15 19:01:36.070033 dom0 widget-wrapper[3495]:     label.set_markup("<b>{}</b>").format(GLib.markup_escape_text(text))
Dec 15 19:01:36.070033 dom0 widget-wrapper[3495]:     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Dec 15 19:01:36.070033 dom0 widget-wrapper[3495]: AttributeError: 'NoneType' object has no attribute 'format'

@marmarek
Copy link
Member

Both pylint and tests detected some issues, I think the same one.

@DemiMarie DemiMarie force-pushed the fix-markup-injection branch from 689d549 to e0794e2 Compare December 31, 2024 06:34
@marmarek
Copy link
Member

marmarek commented Jan 3, 2025

This will need rebase on top of #240 (queued for merging already).

@DemiMarie DemiMarie force-pushed the fix-markup-injection branch from e0794e2 to 4f3d7f1 Compare January 4, 2025 02:12
@marmarek
Copy link
Member

marmarek commented Jan 5, 2025

openQA says:

Jan 05 00:05:36 dom0 qui-domains[3238]: Failed to set text '<markup><span color="{colormap[state]}">sys-whonix</span></markup>' from markup due to error parsing markup: Value of 'foreground' attribute on <span> tag on line 1 could not be parsed; should be a color specification, not '{colormap[state]}'

(there may be more issues like this)

@DemiMarie DemiMarie force-pushed the fix-markup-injection branch 2 times, most recently from 8b8a126 to 275d9f6 Compare January 6, 2025 03:57
@DemiMarie DemiMarie marked this pull request as draft January 6, 2025 03:57
@DemiMarie DemiMarie force-pushed the fix-markup-injection branch from 275d9f6 to 99c6f83 Compare January 6, 2025 05:40
@marmarek
Copy link
Member

marmarek commented Jan 6, 2025

Make the widgets work in KDE Wayland session

Mixed up PRs ?

Copy link
Member

@marmarek marmarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that's all what openQA plus some manual testing found.

qubes_config/global_config/usb_devices.py Outdated Show resolved Hide resolved
@marmarek marmarek requested a review from marmarta January 7, 2025 02:31
@DemiMarie DemiMarie force-pushed the fix-markup-injection branch from 99c6f83 to b2733b2 Compare January 7, 2025 04:19
@DemiMarie DemiMarie marked this pull request as ready for review January 8, 2025 00:40
@DemiMarie
Copy link
Author

And that's all what openQA plus some manual testing found.

My test box uses KDE+Wayland, so this change was to make the code easier to test. I’ll revert it.

This fixes some (theoretical) markup injection problems.  I believe none
of the strings that I escape here will ever contain "<" or "&", but it
is always safer to escape.
@DemiMarie DemiMarie force-pushed the fix-markup-injection branch from b2733b2 to fa4bcc2 Compare January 8, 2025 00:41
@DemiMarie DemiMarie requested a review from marmarek January 8, 2025 02:35
marmarta

This comment was marked as outdated.

Copy link
Member

@marmarta marmarta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some unit tests, codecov complains. I'm not sure if this is needed (see: theoretical), and I think your time could be better spent on things like fixing tray icons from non-dom0 VMs under wayland, but sure, we can have this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants