vim-qrexec - A Qrexec companion for the policy breakers

[Ben Grande]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Contrary to what doc/package-contributions says to do a brief
description, I prefer a long explanation than having to answer questions
in future mails when I could have answered them upfront.

Index:
- - Presentation
- - Implementation
- - Questions
- - Packaging

Presentation
============
I am doing vimfiles for the Qrexec Policy, Qrexec Service Policy and
Qrexec Config. Qubes Code Styling was followed when applicable.
You may find the names strange, but let me show what they mean:
1. qrexecpolicy: /etc/qubes/policy.d/\*.policy
2. qrexecpolicyservice: /etc/qubes/policy.d/include/*
3. qrexecconfig: /etc/qubes/rpc-config/*

1. Qrexec was chosen instead of Qubes RPC because it is shorter.
2. Qrexec Policy Service was chosen because it is included by
'!include-service'. The name could also be 'qrexecpolicyinclude'.
3. Qrexec Config is never mentioned in the documentation by this name,
so I am open to change to a more suitable name, but 'qubesrpcconfig'
was too long for that matter, 2 chars longer makes a huge
difference.

Licensed under Vim's license, GPL compatible. See ':h license' for more
information on this topic. qubes-core-qrexec is under GPL-2+ according
to debian/copyright and rpm_spec/qubes-qrexec-dom0.spec.in, therefore it
would be okay for Qubes to hold the vimfiles in the same repository,
changing the license if needed for less complications.

Below I present the features of the plugin. The mail is quite long, but
this is intended to avoid unnecessary back-and-forths. At the end, there
are open questions for the mailing list.

You can find the code at https://codeberg.org/ben.grande.b/vim-qrexec.

I saw that Marta Marczykowska-Gorecka is doing the a GUI application
called qubes-policy-editor. I can't deny it has a better usability for
people not used to terminal applications but it doesn't work for
headless dom0, or embedding the policy syntax into fenced code blocks in
Markdown or Rst. Because the syntax also can be used in fenced code
blocks, people who edit the Qrexec Policy documentation will have the
benefit of getting the syntax inline (g:markdown_fenced_languages). In
the end, the user will choose what suits him best, I am just providing
an alternative.

Implementation
==============
Features:
- - Syntax Highlighting: strict check, errors are emphasized
- - Lint integration: native mode or via plugins (Dispatch, Ale)
- - Code completion: based on syntax and previous entries
- - Spell file: good words list

Dependencies
- - Vim version 8.2: stable version for Dom0 Fedora 32 and Debian 11
- - qubes-policy-lint: lint tool, optional (needs to be in Dom0)

The details provided here are intended to be short and explicit of the
project objective, you should definitely read the reference manual at
doc/qrexec.txt to gather more information.

This plugin focus on readability, lines are broken into multiple lines
if they become more readable and easier to edit, this hugely impacts on
the number of lines, but greatly decreases the difficulty to get a hold
of what the code is doing. Breaking into multiple lines is not common
according to my search to $VIMRUNTIME, but I find the lines that wrap
multiple times difficult to read and edit, so I am not following Vim's
convention, but my own perception of what makes a clean code, despite
legacy. One side effect is that command options might loose syntax
highlighting, but I believe it is fixed on Vim9 and beyond.


Syntax
- ------
The syntax highlighting follows Woju's code[0].

The valid highlighting is nice, but the real benefit of using the syntax
file is by making it strict:
- - Validation for every field
- - No field can exist without a group
- - Requires minimum field number to be reached
- - Shows clearly where the error is
- - Items of the 2nd field and beyond are always contained and only
matched by 'syntax match' option 'nextgroup='.

Lint
- ----
Wrapper around 'makeprg' with qubes-policy-lint for native method.
If you prefer a third-party plugin, variable for Dispatch and lint
script for ALE is defined.

Lint catches cases where the syntax would not be a good way to show the
problem. There is no perfect solution anyway, it is not possible to
handle runtime bugs.

Code completion
- ---------------
When the field has know possible values such as the default arguments,
tokens, resolutions, parameters, it is added by default to the
completion list.

For those same fields, the list is incremented for every line that
reaches the minimum number of fields. If a rule has a qube called
'sourcevm' as the source for the rule, the 'sourcevm' will be added to
the list of possible values for source completion.

Spell checking
- --------------
If you set 'spell', you will benefit by using the plugin spell file as a
secondary good word list, after your primary language. It helps to
reduce the number of false positives spelling errors.

Credits
- -------
Wojtek Porczyk for the initial syntax file[0].

References
- ----------
[0] https://github.com/qubesos/qubes-core-qrexec/vim/syntax/qubespolicy.vim
[1] https://github.com/junegunn/vader.vim/issues/221

Questions
=========
I have a few questions, if you would be kind to answer them:

1 - Would QubesOS Team package the vimfiles to for Dom0 and DomUs?
Only Dom0 can lint, as it requires the qubes-qrexec-dom0 to be
installed, but DomUs can greatly benefit from this plugin by using
all of the rest it offers, syntax highlighting, code completion,
spell check.

1.1 - Is VimScript a barrier for inclusion as it greatly decreases the
chances of someone reviewing it? Total lines of code excluding
comments, empty lines or lines that multi-lines: 957, as of
2023-04-15, first draft of this e-mail.

1.2 - How can I help reviewers? I documented the code via comments and
the usage via the help file. Is there anything that I could clarify
further? Don't hesitate to ask questions.

2 - There is one binary and only because Vim requires the spell file
to be compiled, it is at spell/qrexec.ascii.add.spl. Information on
how to generate the file is documented at spell/qrexec.ascii.add,
scripting the compilation is possible so you don't depend on me. It
may vary on different Vim versions and must be compiled with the
same version that is gonna later be used by the users else Vim will
throw an error.

3 - What is the recommended style for indentation? Tabs? Spaces?
Textwidth? I made some assumptions based on the current files
shipped by Qubes. Tabs are expanded to spaces and indentation is
made as 2 (two) spaces. Textwidth is set at 78 for comments, leaving
2 columns for the sign column or line number. Rules are wrapped,
they do not break with the textwidth, but comments break after
textwdith for uniformity. Please search the code for
'g:qrexec_recommended_style' to suggest changes.

Note I am not proficient in VimScript, I learned through the examples at
$VIMRUNTIME, so code review is highly appreciated.

Code is never complete, tasks can be found by searching for "TODO:".

Packaging
=========
Currently, there is a Vim syntax file at qubes-core-qrexec repository,
but it is not installed to Dom0. If Qubes Team decides to package the
plugin, I recommend using the path
/usr/share/vim/vimfiles/pack/qubes/start/vim-qrexec, to keep the
plugin files together, organized, instead of dispersed. A package names
"vim-qrexec" would be a meaningful name and easy to search for.

Furthermore, I didn't put the build files of qubes-skeleton in place
because I am waiting for the acceptance of this proposal, if it is going
to qubes-core-qrexec, if it is going to stay as a separate repository,
if it is going to become a contributed package or denied the inclusion.

There are no automated tests as I haven't found a suitable framework,
therefore tests are done manually. There is Vader[1], but it not
actively maintained, but it is the easiest one to test syntax.

- --
Benjamin Grande
-----BEGIN PGP SIGNATURE-----

iNUEARYKAH0WIQRklnEdsUUe50UmvUUbcxS/DMyWhwUCZG36z18UgAAAAAAuAChp
c3N1ZXItZnByQG5vdGF0aW9ucy5vcGVucGdwLmZpZnRoaG9yc2VtYW4ubmV0NjQ5
NjcxMURCMTQ1MUVFNzQ1MjZCRDQ1MUI3MzE0QkYwQ0NDOTY4NwAKCRAbcxS/DMyW
h8iOAQCOZm7p0l3hAvzy1OlaKf/T6Fvh0Ndei4DO02KNw/dnnAEAlwPDikJbhHjf
Z9bG8PNip3DFUD9f1CZyM01Bfk09zgA=
=180p
-----END PGP SIGNATURE-----

[Demi Marie Obenour]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On Wed, May 24, 2023 at 11:53:51AM +0000, Ben Grande wrote:
> Contrary to what doc/package-contributions says to do a brief
> description, I prefer a long explanation than having to answer questions
> in future mails when I could have answered them upfront.

I like this approach :). Thanks for the contribution!

> Index:
> - Presentation
> - Implementation
> - Questions

Nice! This does mean that the syntax file needs to be secure against
maliciously crafted policy. It looks like there are some dynamically
generated regular expressions that could be replaced with stridx() and
string slicing. Otherwise, the code looks to be pretty good: there are
no calls to execute() or system(), and the code doesn’t open any files
with paths that are determined by the policy.

> Implementation
> ==============
> Features:

> - Syntax Highlighting: strict check, errors are emphasized
> - Lint integration: native mode or via plugins (Dispatch, Ale)
> - Code completion: based on syntax and previous entries
> - Spell file: good words list
>
> Dependencies
> - Vim version 8.2: stable version for Dom0 Fedora 32 and Debian 11

Do later versions work as well?

> - qubes-policy-lint: lint tool, optional (needs to be in Dom0)
>
> The details provided here are intended to be short and explicit of the
> project objective, you should definitely read the reference manual at
> doc/qrexec.txt to gather more information.
>
> This plugin focus on readability, lines are broken into multiple lines
> if they become more readable and easier to edit, this hugely impacts on
> the number of lines, but greatly decreases the difficulty to get a hold
> of what the code is doing. Breaking into multiple lines is not common
> according to my search to $VIMRUNTIME, but I find the lines that wrap
> multiple times difficult to read and edit, so I am not following Vim's
> convention, but my own perception of what makes a clean code, despite
> legacy. One side effect is that command options might loose syntax
> highlighting, but I believe it is fixed on Vim9 and beyond.

That is definitely a good idea. Code that is easier to read is easier
to review, and code that is easier to review is more likely to be
accepted.

> Syntax

> ------
> The syntax highlighting follows Woju's code[0].
>
> The valid highlighting is nice, but the real benefit of using the syntax
> file is by making it strict:
> - Validation for every field
> - No field can exist without a group
> - Requires minimum field number to be reached
> - Shows clearly where the error is
> - Items of the 2nd field and beyond are always contained and only
> matched by 'syntax match' option 'nextgroup='.

That is awesome!

> Lint

> ----
> Wrapper around 'makeprg' with qubes-policy-lint for native method.
> If you prefer a third-party plugin, variable for Dispatch and lint
> script for ALE is defined.
>
> Lint catches cases where the syntax would not be a good way to show the
> problem. There is no perfect solution anyway, it is not possible to
> handle runtime bugs.
>
> Code completion

> ---------------
> When the field has know possible values such as the default arguments,
> tokens, resolutions, parameters, it is added by default to the
> completion list.
>
> For those same fields, the list is incremented for every line that
> reaches the minimum number of fields. If a rule has a qube called
> 'sourcevm' as the source for the rule, the 'sourcevm' will be added to
> the list of possible values for source completion.

NICE!!!

> Spell checking

> --------------
> If you set 'spell', you will benefit by using the plugin spell file as a
> secondary good word list, after your primary language. It helps to
> reduce the number of false positives spelling errors.

Is this handled automatically?

> Credits

> -------
> Wojtek Porczyk for the initial syntax file[0].
>
> References

> ----------
> [0] https://github.com/qubesos/qubes-core-qrexec/vim/syntax/qubespolicy.vim
> [1] https://github.com/junegunn/vader.vim/issues/221
>
> Questions
> =========
> I have a few questions, if you would be kind to answer them:
>
> 1 - Would QubesOS Team package the vimfiles to for Dom0 and DomUs?
> Only Dom0 can lint, as it requires the qubes-qrexec-dom0 to be
> installed, but DomUs can greatly benefit from this plugin by using
> all of the rest it offers, syntax highlighting, code completion,
> spell check.

Personally I don’t see any reason not to ship them. What would be
needed to support linting outside of dom0?

> 1.1 - Is VimScript a barrier for inclusion as it greatly decreases the
> chances of someone reviewing it? Total lines of code excluding
> comments, empty lines or lines that multi-lines: 957, as of
> 2023-04-15, first draft of this e-mail.

I am not personally proficient in VimScript, but IIUC syntax files are
largely declarative, which should help.

> 1.2 - How can I help reviewers? I documented the code via comments and
> the usage via the help file. Is there anything that I could clarify
> further? Don't hesitate to ask questions.
>
> 2 - There is one binary and only because Vim requires the spell file
> to be compiled, it is at spell/qrexec.ascii.add.spl. Information on
> how to generate the file is documented at spell/qrexec.ascii.add,
> scripting the compilation is possible so you don't depend on me. It
> may vary on different Vim versions and must be compiled with the
> same version that is gonna later be used by the users else Vim will
> throw an error.

Simplest option would be to build it in an postinstall script and use a
trigger to make sure that it gets updated when the Vim package is
updated.

> 3 - What is the recommended style for indentation? Tabs? Spaces?
> Textwidth? I made some assumptions based on the current files
> shipped by Qubes. Tabs are expanded to spaces and indentation is
> made as 2 (two) spaces. Textwidth is set at 78 for comments, leaving
> 2 columns for the sign column or line number. Rules are wrapped,
> they do not break with the textwidth, but comments break after
> textwdith for uniformity. Please search the code for
> 'g:qrexec_recommended_style' to suggest changes.

Qubes OS uses 4 spaces for indentation in C and Python, but I am not
sure what the standard is for VimScript.

> Note I am not proficient in VimScript, I learned through the examples at
> $VIMRUNTIME, so code review is highly appreciated.
>
> Code is never complete, tasks can be found by searching for "TODO:".
>
> Packaging
> =========
> Currently, there is a Vim syntax file at qubes-core-qrexec repository,
> but it is not installed to Dom0. If Qubes Team decides to package the
> plugin, I recommend using the path
> /usr/share/vim/vimfiles/pack/qubes/start/vim-qrexec, to keep the
> plugin files together, organized, instead of dispersed. A package names
> "vim-qrexec" would be a meaningful name and easy to search for.
>
> Furthermore, I didn't put the build files of qubes-skeleton in place
> because I am waiting for the acceptance of this proposal, if it is going
> to qubes-core-qrexec, if it is going to stay as a separate repository,
> if it is going to become a contributed package or denied the inclusion.
>
> There are no automated tests as I haven't found a suitable framework,
> therefore tests are done manually. There is Vader[1], but it not
> actively maintained, but it is the easiest one to test syntax.
>
> --

> Benjamin Grande
- --
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmRuXgcACgkQsoi1X/+c
IsG6dQ/+J/kr4Jayy7zIRUqG4C5Ak0cmRtk/u1OIelNaXjKUWxuzeJ/krfutvsNI
426UIdnn9aZ9vcYWwmUe0M191C8TXty5ihARU0J+1aKRyx6n1GHVVrTkYLDudZY/
dTkW42EGzeU2cAKecttcQl/jY1l/VjT/j81InMf0kF/eSXalCZxTn2U1w0d5560Z
9YETsyTxrxWH6e4ZyRLNcZ22TWgi/0kzMjxAV4YiAaiS5pUwMgr4CFU1GEu1Gdfr
k72Xwim7QSg0uTfA22pFtkzwY7EVZ0X9z79Uq1nqiD60v7GsWpNbgqGbvN8ezj/E
Varn/wtOVdgxlmeakXNvyfL2yFZ9qKX9ljGcrGhYHQ+uVRLamNaVdc/nNJUOGEcG
CQcCc66VfY4xg6173OKv2EZHKT5gQyk31+4S4E0oqzF6P52ZlKPzeoU5k3r14mN2
Ed6OjpwUPgegYyd4ucSpKdGud1lmIPN7PutZXWfx4arfiJsHhEyxAllyLk5Y07pe
FFWdBYUqMVI2xggAwMiii6I50wHIo6nKOZ4ckvzGEVylREwaZP/ul5/15lC0KMPZ
YjHH3YS/T2Di0Dt04xnqvdzpyvsJsqcjOmSmf1omKFLJ6r5aTRqTaH/GYi9PQMzL
LtA17BixHFGvGesXog3HjYtaPdQC+8ZwxhYBTaxD6yF71Fj/9bo=
=iOuq
-----END PGP SIGNATURE-----

[Ben Grande]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 23-05-24 14:57:12, Demi Marie Obenour wrote:
> On Wed, May 24, 2023 at 11:53:51AM +0000, Ben Grande wrote:
> > Contrary to what doc/package-contributions says to do a brief
> > description, I prefer a long explanation than having to answer questions
> > in future mails when I could have answered them upfront.
>
> I like this approach :). Thanks for the contribution!

Thanks for the review.

> > I saw that Marta Marczykowska-Gorecka is doing the a GUI application
> > called qubes-policy-editor. I can't deny it has a better usability for
> > people not used to terminal applications but it doesn't work for
> > headless dom0, or embedding the policy syntax into fenced code blocks in
> > Markdown or Rst. Because the syntax also can be used in fenced code
> > blocks, people who edit the Qrexec Policy documentation will have the
> > benefit of getting the syntax inline (g:markdown_fenced_languages). In
> > the end, the user will choose what suits him best, I am just providing
> > an alternative.
>
> Nice! This does mean that the syntax file needs to be secure against
> maliciously crafted policy. It looks like there are some dynamically
> generated regular expressions that could be replaced with stridx() and
> string slicing. Otherwise, the code looks to be pretty good: there are
> no calls to execute() or system(), and the code doesn’t open any files
> with paths that are determined by the policy.

About being secure against a maliciously crafted policy, normally, the
best thing is to open the policy without a modeline.
```sh
vim -c "setlocal nomodeline ft=qrexecpolicy spell" malicious.policy
```
The option 'nomodeline' is set in $VIMRUNTIME/ftplugin/mail.vim with the
following comment:
```vim
" Don't use modelines in e-mail messages, avoid trojan horses and nasty
" "jokes" (e.g., setting 'textwidth' to 5).
setlocal nomodeline
```

The downside is that the filetype will not be set by the modeline when
using a policy that is not in the expected path (ftdetect). So using
```qrexecpolicy
# vim: ft=qrexecpolicy
# My custom policy
!compat-4.0
```
wouldn't work if the path to policy is /tmp/custom.policy for example.

Currently, modeline is not disabled, it uses Vim's or user settings,
which defaults to enabled.

Can you please point to the dynamically generated regular expression?
My understanding is that dynamically generated regular expressions
requires the execute command:
https://github.com/vim/vim/blob/master/runtime/syntax/yaml.vim#L146

It has some nice uses cases to avoid code repetition and using variables
to keep regex, but I didn't do that for vim-qrexec.

The only file read is the current buffer during code completion
autoload/qrexeccomplete.vim - getline(). Which Vim has already read so
it can show you its contents, now it is read to be saved to a variable.

> > Dependencies
> > - Vim version 8.2: stable version for Dom0 Fedora 32 and Debian 11
>
> Do later versions work as well?

Tested now on Fedora 37, Vim 9.0. It worked.
No complaints about spell file compiled on an older version (8.2), and
the spell file worked. Maybe Vim's error E770 appears only when Vim does
breaking changes to the spell file.

Vim has history of avoiding breaking changes, so I assumed it will
continue working for later versions. But it still makes sense to use a
postinstall to recompile the spell file when Vim is updated.

> > Syntax
> > ------
> > The syntax highlighting follows Woju's code[0].
> >
> > The valid highlighting is nice, but the real benefit of using the syntax
> > file is by making it strict:
> > - Validation for every field
> > - No field can exist without a group
> > - Requires minimum field number to be reached
> > - Shows clearly where the error is
> > - Items of the 2nd field and beyond are always contained and only
> > matched by 'syntax match' option 'nextgroup='.
>
> That is awesome!

Thanks, this is a thank you letter to the Qubes Team. Also a way to
mitigate easy debugging and avoid breaking Qrexec call because you
couldn't know what was wrong without watching the logs with:
```sh
sudo journalctl -fu qubes-qrexec-policy-daemon.service
```
Which has a bad usability and unnecessary on most cases as this plugin
will prove.

> > Lint
> > ----
> > Wrapper around 'makeprg' with qubes-policy-lint for native method.
> > If you prefer a third-party plugin, variable for Dispatch and lint
> > script for ALE is defined.
> >
> > Lint catches cases where the syntax would not be a good way to show the
> > problem. There is no perfect solution anyway, it is not possible to
> > handle runtime bugs.
> >
> > Code completion
> > ---------------
> > When the field has know possible values such as the default arguments,
> > tokens, resolutions, parameters, it is added by default to the
> > completion list.
> >
> > For those same fields, the list is incremented for every line that
> > reaches the minimum number of fields. If a rule has a qube called
> > 'sourcevm' as the source for the rule, the 'sourcevm' will be added to
> > the list of possible values for source completion.
>
> NICE!!!

On the code completion case, it is dynamically generated, so we might
need to do something here. I don't know the risk of inserting text that
is already on the file to the completion list and then accepted by the
user. What would be a good filtering method? Block characters that are
not allowed by Qrexec? Chars outside of the range: [A-Za-z0-9_+*.=-]
Currently not implemented.

> > Spell checking
> > --------------
> > If you set 'spell', you will benefit by using the plugin spell file as a
> > secondary good word list, after your primary language. It helps to
> > reduce the number of false positives spelling errors.
>
> Is this handled automatically?

No. But I could improve it a little by setting in ftplugin the
following:
```vim
setlocal spell
setlocal spelllang=en_us
```
The only thing I am afraid of is the user not understand how spell works
and starts adding his words to the plugin good words list, which will be
overwritten on updates. User still needs to set his good words list with
'spellfile'.

A more long explanation of the current state below.

The ftplugin appends the qrexec spell file to Vim's option 'spellfile'.
By itself, it is not of much use, it requires the that the user has
already set their spelling configuration:
```sh
mkdir -p ~/.vim/after/spell
```
```vim
set spell
set spelllang=en_us
set spellfile=~/.vim/after/spell/en.utf-8.add
```

The policy comments are written in English. The Spell is not enabled by
default. I'd rather not set the language spell file for the user.
If the user wants spell checking for comments, he needs to set the above
options.

Do you think it makes sense force 'spell'? I think so, but it doesn't
work if user doesn't set the 'spelllang'.
Should we force the 'spelllang' to english? If someone writes their
comments in another language, they can also append to the spelllang:
```vim
# vimrc
# Append Polish to the spelling language of qrexec files:
augroup qrexec
autocmd!
autocmd FileType qrexecpolicy,qrexecpolicyservice,qrexeconfig
\ setlocal spelllang+=pl
augroup END
```

The 'spellfile' is the user good words list. We should not modify it.
But perhaps we can enable 'spell' and set 'spelllang' to US English.

> > Questions
> > =========
> > I have a few questions, if you would be kind to answer them:
> >
> > 1 - Would QubesOS Team package the vimfiles to for Dom0 and DomUs?
> > Only Dom0 can lint, as it requires the qubes-qrexec-dom0 to be
> > installed, but DomUs can greatly benefit from this plugin by using
> > all of the rest it offers, syntax highlighting, code completion,
> > spell check.
>
> Personally I don’t see any reason not to ship them. What would be
> needed to support linting outside of dom0?

If the parser can be packaged to DomUs, that would make linting policies
from any qube possible. These are the dependencies I mapped:

qrexec
__init__
exc
utils
policy.parser

See the imports at:
https://codeberg.org/ben.grande.b/qubes-tools/src/branch/main/qubes-policy-lint

> > 1.1 - Is VimScript a barrier for inclusion as it greatly decreases the
> > chances of someone reviewing it? Total lines of code excluding
> > comments, empty lines or lines that multi-lines: 957, as of
> > 2023-04-15, first draft of this e-mail.
>
> I am not personally proficient in VimScript, but IIUC syntax files are
> largely declarative, which should help.

Great. The most complicated is the code completion at
autoload/qrexeccomplete.vim.

>
> > 1.2 - How can I help reviewers? I documented the code via comments and
> > the usage via the help file. Is there anything that I could clarify
> > further? Don't hesitate to ask questions.
> >
> > 2 - There is one binary and only because Vim requires the spell file
> > to be compiled, it is at spell/qrexec.ascii.add.spl. Information on
> > how to generate the file is documented at spell/qrexec.ascii.add,
> > scripting the compilation is possible so you don't depend on me. It
> > may vary on different Vim versions and must be compiled with the
> > same version that is gonna later be used by the users else Vim will
> > throw an error.
>
> Simplest option would be to build it in an postinstall script and use a
> trigger to make sure that it gets updated when the Vim package is
> updated.

Yes. For reference, see :h E771
All issues in that section are applicable, E770, E771, E772.
When I tested on Vim 9.0, this issue didn't occur.

> > 3 - What is the recommended style for indentation? Tabs? Spaces?
> > Textwidth? I made some assumptions based on the current files
> > shipped by Qubes. Tabs are expanded to spaces and indentation is
> > made as 2 (two) spaces. Textwidth is set at 78 for comments, leaving
> > 2 columns for the sign column or line number. Rules are wrapped,
> > they do not break with the textwidth, but comments break after
> > textwdith for uniformity. Please search the code for
> > 'g:qrexec_recommended_style' to suggest changes.
>
> Qubes OS uses 4 spaces for indentation in C and Python, but I am not
> sure what the standard is for VimScript.

I meant the policy coding style. For Vim I used 2 spaces, 78 textwidth.
You can see the modeline on each file, which, should be the same.

- --
Benjamin Grande
-----BEGIN PGP SIGNATURE-----

iNUEARYKAH0WIQRklnEdsUUe50UmvUUbcxS/DMyWhwUCZG8+eF8UgAAAAAAuAChp
c3N1ZXItZnByQG5vdGF0aW9ucy5vcGVucGdwLmZpZnRoaG9yc2VtYW4ubmV0NjQ5
NjcxMURCMTQ1MUVFNzQ1MjZCRDQ1MUI3MzE0QkYwQ0NDOTY4NwAKCRAbcxS/DMyW
h/raAQDNp/9pQ7D2BHaLTK4vQtzQ/n699PggVhvYPmd/be3stwEA/CZVjRN30vi6
BDt6chgbQ5shibsZZVgWz1xBJRlo7QQ=
=44YA
-----END PGP SIGNATURE-----

[Ben Grande]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 23-05-25 10:54:48, Ben Grande wrote:
> On the code completion case, it is dynamically generated, so we might
> need to do something here. I don't know the risk of inserting text that
> is already on the file to the completion list and then accepted by the
> user. What would be a good filtering method? Block characters that are
> not allowed by Qrexec? Chars outside of the range: [A-Za-z0-9_+*.=-]
> Currently not implemented.

Commit e0243c6 fixes this problem.
Only characters that are in range are added to the completion.

> > > Spell checking
> > > --------------
> > > If you set 'spell', you will benefit by using the plugin spell file as a
> > > secondary good word list, after your primary language. It helps to
> > > reduce the number of false positives spelling errors.
> >
> > Is this handled automatically?

Now it is: 26d2115
spell is enabled.
en_us is appended to spelllang instead of overwrite its value.

- --
Benjamin Grande
-----BEGIN PGP SIGNATURE-----

iNUEARYKAH0WIQRklnEdsUUe50UmvUUbcxS/DMyWhwUCZG9pul8UgAAAAAAuAChp
c3N1ZXItZnByQG5vdGF0aW9ucy5vcGVucGdwLmZpZnRoaG9yc2VtYW4ubmV0NjQ5
NjcxMURCMTQ1MUVFNzQ1MjZCRDQ1MUI3MzE0QkYwQ0NDOTY4NwAKCRAbcxS/DMyW
h4cCAQD1f33RieAoUKbqcBYZ8zQFpKOj3HjdEmOVRPUy9G43uAD+MD7PpfjPxeVj
F/de4KFYHtxoDPu/0o+y5tX74crtkgo=
=ANp1
-----END PGP SIGNATURE-----

[Demi Marie Obenour]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

=~ interprets its RHS as a regular expression, so any VimScript that
uses =~ with a non-constant RHS needs to ensure that the RHS is not
controlled by an attacker. In your case it appears that e.g.
subscripting and stridx() would be better.

I would go with the former.

I’ll leave this to the other Qubes developers.

> > > Questions
> > > =========
> > > I have a few questions, if you would be kind to answer them:
> > >
> > > 1 - Would QubesOS Team package the vimfiles to for Dom0 and DomUs?
> > > Only Dom0 can lint, as it requires the qubes-qrexec-dom0 to be
> > > installed, but DomUs can greatly benefit from this plugin by using
> > > all of the rest it offers, syntax highlighting, code completion,
> > > spell check.
> >
> > Personally I don’t see any reason not to ship them. What would be
> > needed to support linting outside of dom0?
>
> If the parser can be packaged to DomUs, that would make linting policies
> from any qube possible. These are the dependencies I mapped:
>
> qrexec
> __init__
> exc
> utils
> policy.parser
>
> See the imports at:
> https://codeberg.org/ben.grande.b/qubes-tools/src/branch/main/qubes-policy-lint

That should definitely be doable.

> > > 1.1 - Is VimScript a barrier for inclusion as it greatly decreases the
> > > chances of someone reviewing it? Total lines of code excluding
> > > comments, empty lines or lines that multi-lines: 957, as of
> > > 2023-04-15, first draft of this e-mail.
> >
> > I am not personally proficient in VimScript, but IIUC syntax files are
> > largely declarative, which should help.
>
> Great. The most complicated is the code completion at
> autoload/qrexeccomplete.vim.

Yeah, that’s where I saw the dynamically generated regexps.

> > > 1.2 - How can I help reviewers? I documented the code via comments and
> > > the usage via the help file. Is there anything that I could clarify
> > > further? Don't hesitate to ask questions.
> > >
> > > 2 - There is one binary and only because Vim requires the spell file
> > > to be compiled, it is at spell/qrexec.ascii.add.spl. Information on
> > > how to generate the file is documented at spell/qrexec.ascii.add,
> > > scripting the compilation is possible so you don't depend on me. It
> > > may vary on different Vim versions and must be compiled with the
> > > same version that is gonna later be used by the users else Vim will
> > > throw an error.
> >
> > Simplest option would be to build it in an postinstall script and use a
> > trigger to make sure that it gets updated when the Vim package is
> > updated.
>
> Yes. For reference, see :h E771
> All issues in that section are applicable, E770, E771, E772.
> When I tested on Vim 9.0, this issue didn't occur.

Good to know!

> > > 3 - What is the recommended style for indentation? Tabs? Spaces?
> > > Textwidth? I made some assumptions based on the current files
> > > shipped by Qubes. Tabs are expanded to spaces and indentation is
> > > made as 2 (two) spaces. Textwidth is set at 78 for comments, leaving
> > > 2 columns for the sign column or line number. Rules are wrapped,
> > > they do not break with the textwidth, but comments break after
> > > textwdith for uniformity. Please search the code for
> > > 'g:qrexec_recommended_style' to suggest changes.
> >
> > Qubes OS uses 4 spaces for indentation in C and Python, but I am not
> > sure what the standard is for VimScript.
>
> I meant the policy coding style. For Vim I used 2 spaces, 78 textwidth.
> You can see the modeline on each file, which, should be the same.
>

> --
> Benjamin Grande

- --
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmRvgqkACgkQsoi1X/+c
IsGA5w//bCndEID+TBhnfsuxB34Zjg18Ot+yCinw083kr4iubazDzyGsLsGj83ig
e3Lm/N68NB4xIKsbnwVL/BDllgo/3owsFzoWEgEZWQRRPXumDRakLW9wNsMsaLTD
6nGWdhIeTEmE+v+xCIllkSHQJcTIj95VuvNdEtp4LjKdF0PFzeyrAVC00SU8k/Pb
ArttfJCdtHKsZCj3Uk1K2gLD15JkiqyfIiLGuP1CTtE9NZoVvsKbhdYUuplWJbyi
hUOLOZ+PDmZQKUJZLinh1Ev99qplyc/NADvSNHs30VfKnM8FSmhStlleQ9XLhZmL
DTw7V5dB4di3+eQLLDzqCZnN/AT2sXzUxF/igEL/o0PDZMCMq6Y2V4WjOvRmAXZf
fD5HtNKkWlCm/31f8Sgz+456NzQH2xiF/I32xsPpudKB247HCvMt8P0QtLce6TIa
BslflWmhX2pT3wzaPhNqmkGYhOW8N3qxu2XufE28EEeQjcysk/rYYtG9JU7Oh4Gw
YS6HPLRGcs+Zo9LtJqMZOqMAx0nwrhk2ta0kXkZqgL4ufJLBHe5i5ycMRReX6jyN
HFk/jLnmcu9F2tw58GNEf34NYlnd3FFSHv93TWmoF5HjK8RrSUZSobcQJpTNvdNf
BGf2bYa+FC0n+FYBxkitxo4u75+/v/mgHQrdagXBi8BUz4IDdXo=
=AKoK
-----END PGP SIGNATURE-----

[Ben Grande]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 23-05-25 11:45:45, Demi Marie Obenour wrote:
> On Thu, May 25, 2023 at 10:54:48AM +0000, Ben Grande wrote:
> > On 23-05-24 14:57:12, Demi Marie Obenour wrote:
> > > On Wed, May 24, 2023 at 11:53:51AM +0000, Ben Grande wrote:
> >
> > Can you please point to the dynamically generated regular expression?
> > My understanding is that dynamically generated regular expressions
> > requires the execute command:
> > https://github.com/vim/vim/blob/master/runtime/syntax/yaml.vim#L146
>
> =~ interprets its RHS as a regular expression, so any VimScript that
> uses =~ with a non-constant RHS needs to ensure that the RHS is not
> controlled by an attacker. In your case it appears that e.g.
> subscripting and stridx() would be better.

Fixed on 9676d46.
Let me know if that is what you wanted and if there is any remaining
security concerns.
The remaining '=~' are only used for values already know to us, so not
an untrusted input.

> > On the code completion case, it is dynamically generated, so we might
> > need to do something here. I don't know the risk of inserting text that
> > is already on the file to the completion list and then accepted by the
> > user. What would be a good filtering method? Block characters that are
> > not allowed by Qrexec? Chars outside of the range: [A-Za-z0-9_+*.=-]
> > Currently not implemented.
>
> I would go with the former.

I did go with both. First it blocks any character that the parse would
not like in general. Second, for every field, there is a know character
range for it so that is also used.

> > > > Spell checking
> > > > --------------
> > > > If you set 'spell', you will benefit by using the plugin spell file as a
> > > > secondary good word list, after your primary language. It helps to
> > > > reduce the number of false positives spelling errors.
> > >
> > > Is this handled automatically?
> >
> > No.

> > [REDACTED]

> >
> > The 'spellfile' is the user good words list. We should not modify it.
> > But perhaps we can enable 'spell' and set 'spelllang' to US English.
>
> I’ll leave this to the other Qubes developers.

As from my previous mail before your reply, spell is enabled an
spelllang has the value 'en_us' appended to it, so not overwriting user
spelling language but allowing user's custom language to exist without
having to use autocommands.
The only downside is if some Qubes developer writes his comments in
Polish or any other language when testing and doesn't notice that when
shipping the policy, in that case, overriding user defaults would be the
best solution.

> > > > Questions
> > > > =========
> > > > I have a few questions, if you would be kind to answer them:
> > > >
> > > > 1 - Would QubesOS Team package the vimfiles to for Dom0 and DomUs?
> > > > Only Dom0 can lint, as it requires the qubes-qrexec-dom0 to be
> > > > installed, but DomUs can greatly benefit from this plugin by using
> > > > all of the rest it offers, syntax highlighting, code completion,
> > > > spell check.
> > >
> > > Personally I don’t see any reason not to ship them. What would be
> > > needed to support linting outside of dom0?
> >
> > If the parser can be packaged to DomUs, that would make linting policies
> > from any qube possible. These are the dependencies I mapped:
> >
> > qrexec
> > __init__
> > exc
> > utils
> > policy.parser
> >
> > See the imports at:
> > https://codeberg.org/ben.grande.b/qubes-tools/src/branch/main/qubes-policy-lint
>
> That should definitely be doable.

Should I open another thread so this doesn't get lost as it is not
solely related to Vim?
I didn't try building qubes-core-qrexec yet so I am not near of
implementing this, if someone steps up, I'd be grateful, else, I might
take some time to do this.
Seems like I need to modify the Makefile and the setup.py script, but
they were made to include all files in the tools directory and I am
unsure it will be easy to set it to only some specific files.

> > > > 1.1 - Is VimScript a barrier for inclusion as it greatly decreases the
> > > > chances of someone reviewing it? Total lines of code excluding
> > > > comments, empty lines or lines that multi-lines: 957, as of
> > > > 2023-04-15, first draft of this e-mail.
> > >
> > > I am not personally proficient in VimScript, but IIUC syntax files are
> > > largely declarative, which should help.
> >
> > Great. The most complicated is the code completion at
> > autoload/qrexeccomplete.vim.
>
> Yeah, that’s where I saw the dynamically generated regexps.

Fixed on 9676d46.

- --
Benjamin Grande
-----BEGIN PGP SIGNATURE-----

iNUEARYKAH0WIQRklnEdsUUe50UmvUUbcxS/DMyWhwUCZG/ew18UgAAAAAAuAChp
c3N1ZXItZnByQG5vdGF0aW9ucy5vcGVucGdwLmZpZnRoaG9yc2VtYW4ubmV0NjQ5
NjcxMURCMTQ1MUVFNzQ1MjZCRDQ1MUI3MzE0QkYwQ0NDOTY4NwAKCRAbcxS/DMyW
hz2NAQDa9pEjA67T4QQbXRwLPyfxNOm51CYcOZ5380/2+PJDrAD/fIxOk6El2S2J
5a2/ZLsflYNa6fP9FbD1lzTIQPBgCA0=
=E7I8
-----END PGP SIGNATURE-----

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Thu, May 25, 2023 at 10:18:43PM +0000, Ben Grande wrote:
> On 23-05-25 11:45:45, Demi Marie Obenour wrote:
> > On Thu, May 25, 2023 at 10:54:48AM +0000, Ben Grande wrote:
> > > If the parser can be packaged to DomUs, that would make linting policies
> > > from any qube possible. These are the dependencies I mapped:
> > >
> > > qrexec
> > > __init__
> > > exc
> > > utils
> > > policy.parser
> > >
> > > See the imports at:
> > > https://codeberg.org/ben.grande.b/qubes-tools/src/branch/main/qubes-policy-lint
> >
> > That should definitely be doable.

That should already be the case, qubes-core-qrexec should be installed
in domU too.

- --
Best Regards,
Marek Marczykowski-Górecki

Invisible Things Lab
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAmRv58AACgkQ24/THMrX
1yyxeQf+JB0k8IskH/pyS0GokVsnyW2K3RCOUpdGdlwS6IZ0ok7/q4VeuEB0RGzm
lavVrrkow7NguTBKSppKxxPEMq8GyNw4VdWP60R5QFP5+j/QzYk+vSAsv+mwAmmt
RT39UjzqJYEa3WNBUpZFMmlO/TP6IXdwhzwobU7RlX2z/Y/uIQQn0eeyIR0+g+5C
jRLuWuTw0cSKgm5N7Qd9dCEhhtvbBdTbQCnTZVJb1aW+jQDevR7VmGJOK026OsOI
79xn+s0sL/tr34XTzfuh2xszVVXD7t04ToE+y10oZDfv/LQ9+vVhQ9i8jzkiaxGd
vXI6Q5DGkIP+H+bzrJJPDKFqrDNbxA==
=gn2o
-----END PGP SIGNATURE-----

[Ben Grande]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 23-05-26 00:57:04, Marek Marczykowski-Górecki wrote:
> On Thu, May 25, 2023 at 10:18:43PM +0000, Ben Grande wrote:
> > On 23-05-25 11:45:45, Demi Marie Obenour wrote:
> > > On Thu, May 25, 2023 at 10:54:48AM +0000, Ben Grande wrote:
> > > > If the parser can be packaged to DomUs, that would make linting policies
> > > > from any qube possible. These are the dependencies I mapped:
> > > >
> > > > qrexec
> > > > __init__
> > > > exc
> > > > utils
> > > > policy.parser
> > > >
> > > > See the imports at:
> > > > https://codeberg.org/ben.grande.b/qubes-tools/src/branch/main/qubes-policy-lint
> > >
> > > That should definitely be doable.
>
> That should already be the case, qubes-core-qrexec should be installed
> in domU too.

Hum, on Fedora yes, on Debian, incomplete.

Fedora - expect qrexec denied:
$ qubes-policy
Request refused
Command Failed

Debian - unexpected module not found:
$ qubes-policy
Traceback (most recent call last):
File "/usr/bin/qubes-policy", line 2, in <module>
from qrexec.tools.qubes_policy import main
ModuleNotFoundError: No module named 'qrexec'

- --
Benjamin Grande
-----BEGIN PGP SIGNATURE-----

iNUEARYKAH0WIQRklnEdsUUe50UmvUUbcxS/DMyWhwUCZG/uIF8UgAAAAAAuAChp
c3N1ZXItZnByQG5vdGF0aW9ucy5vcGVucGdwLmZpZnRoaG9yc2VtYW4ubmV0NjQ5
NjcxMURCMTQ1MUVFNzQ1MjZCRDQ1MUI3MzE0QkYwQ0NDOTY4NwAKCRAbcxS/DMyW
hz/RAP9GGdzvt8ImBFX0bFzn1QKV3GWsMqx13E5LfqNbHmNCZgD9GWts5snW9SH2
Cnz3dA5e//svcK1jFyncBov4CtbduQo=
=qWF0
-----END PGP SIGNATURE-----

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Ah, that looks like a missing dependency on python3-qrexec.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAmRv85gACgkQ24/THMrX
1yyligf9E9Y+fyLF7fmjT9q4e8qS00KYRDB9W5ocrwUvW5+hUCBZlBMlMb6g1uPX
Tl+jvwIegV1ivTXt6qJk3Su4RGot/SlTJ1r/i+u0sokS4I+NGJ+2qWV1jxQEHDnb
sou/p2156+ufo0dkAB2XOm3tR5QQSQ1ODMdtZsVnsrFtE9LYxAMje4aSWBab0LEs
YIT8Ila5VQ9miRgJfcTjdcPXPkVBXEPsE0KwdoB1Fra/skwFR7zSPFmnVVVoh3e9
W+HR0umTm1YRSQ+mwyxajeoCWRwgywDXiUqOBun6Eg2EJM60EJc3wck3Ynd8Idrf
9NsgsJA0EJEJ/FNOl3PRO9fPTYzVxw==
=bJVk
-----END PGP SIGNATURE-----

[Ben Grande]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 23-05-26 01:47:36, Marek Marczykowski-Górecki wrote:
> On Thu, May 25, 2023 at 11:24:15PM +0000, Ben Grande wrote:

> > [REDACTED]

> > Debian - unexpected module not found:
> > $ qubes-policy
> > Traceback (most recent call last):
> > File "/usr/bin/qubes-policy", line 2, in <module>
> > from qrexec.tools.qubes_policy import main
> > ModuleNotFoundError: No module named 'qrexec'
>
> Ah, that looks like a missing dependency on python3-qrexec.
>

> --
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab

Problem above was fixed: https://github.com/QubesOS/qubes-core-qrexec/commit/587fa6825e8abdb8c51bd90f94075acedf7749d7

Pending review of vim-qrexec.

Depends on qubes-policy-lint to have lint capabilities but it is not a
blocker for vim-qrexec inclusion, maximum it can happen is return 127
when trying to lint.

- --
Benjamin Grande
-----BEGIN PGP SIGNATURE-----

iNUEARYKAH0WIQRklnEdsUUe50UmvUUbcxS/DMyWhwUCZNZENV8UgAAAAAAuAChp
c3N1ZXItZnByQG5vdGF0aW9ucy5vcGVucGdwLmZpZnRoaG9yc2VtYW4ubmV0NjQ5
NjcxMURCMTQ1MUVFNzQ1MjZCRDQ1MUI3MzE0QkYwQ0NDOTY4NwAKCRAbcxS/DMyW
h0lDAQDFPBcS5uqan6BygeNmr9XMHFIfKZDaixJnWdMsgfgtnQD+MD3oImBjwlDg
P5j4jznIBr61eWaXw/5w5L8UAP6K/As=
=UjOR
-----END PGP SIGNATURE-----