multi-apply-tool: first version

[nephros]

Inspired by #277 (and giving up expanding the dbus-calls in the binary for this), this little tools makes life easier for power users handling large amounts of patches.

Features:

• takes a list of patch names, and either activates or deactivates them
• rootless mode: can read and write list of patches to be de/activated as user
• rootless mode: if run as non-root, output script file with corresponding actions
• read list of patches to be de/activated from pm config file
• read list of patches to be de/activated from user-specified file
• save list of currently activated patches to user-specified file

Nothing magic, but useful.

Bugs:

• trying to --apply after an unapply-all without giving a user-specified list will probably error out because patchmanager2.conf has an empty applied= field
• trying to --apply without giving a user-specified will just unnnecessarily re-apply the already applied patches

[nephros]

Example use case:

• while testing some terrible developer's history app, a tester notices that this app crashes lipstick
• this makes PM go into failure state, forcing them to manually re-activate all patches
• which is majorly annoying for users who have a lot of patches, and makes them want to stop debugging the broken app
• with multi-tool, the tester can now conveniently save the list of favourite patches into a backup file: pm-multi-tool -e ~/patches.list, and continue the lipstick-crashing tests
• some crashes later, they can restore their patch configuration: pm-multi-tool -A -f ~/patches.list
• to guard against other occasions of Lipstick crashes, one might also put the patch-saving command into a cron job

[Olf0]

Questions:

1. Where does $EUID come from?
2. write_list() is only used once and a single line function → Better inline it, or is there another reason?
3. I am not a fan of capital letter options. I understand, that this is intended to indicate that they should be executed as root, right? But for Patchmanager proper all options are lower-case, specifically the -a and -u options. Hence I believe they should stay lower-case for consistency. If you are O.K. with that, I will do that.
4. Why is the variable operation preset to -u when the case statement covers all cases? → Preset it to "".
5. More commonality of the executable name with the patchmanager executable is desirable (to be found per tab-completion etc.), e.g., patchman-multi or patcher-multi.
6. I considered my former write-up, but many aliased options are not nice. Ideas?
7. Do the long-opts --apply and --unapply really work for patchmanager proper? Then please also name them here. We should also document all available options somewhere, including Secret functionality: --reset-system #204

[nephros]

7. Do the long-opts --apply and --unapply really work for patchmanager proper? Then please also name them here. We should also document all available options somewhere, including Secret functionality: --reset-system #204

Argh! yes!
I have a patch somewhere which implements these, but haven't submitted it yet. Will clean it up and do another PR on them.

[nephros]

1. Where does $EUID come from?

POSIX I think. Might be a GNU-ism.
It's "effective" UID, i.e. the UID a process is running as (as opposed to UID, the ID of the user running something). E.g. if user mark runs a setuid program whose executable file is owned by mary, UID is mark and EUID is mary.

EDIT: apparently it's a bash-ism, although there's some history in other traditional Unixes.
Will fix to use id instead.

[nephros]

No objections to these three:

• I am not a fan of capital letter options. I understand, that this is intended to indicate that they should be executed as root, right? But for Patchmanager proper all options are lower-case, specifically the -a and -u options. Hence I believe they should stay lower-case for consistency. If you are O.K. with that, I will do that.

My thought was as they are potentially "destructive", including one more keypress id a bit of a safeguard. Also, the original idea came from modifying the original patchmanager program, where there is -a and -u for one patch, and it makes sense there for the capitalized version to mean "all".

• Why is the variable operation preset to -u when the case statement covers all cases? → Preset it to "".
• More commonality of the executable name with the patchmanager executable is desirable (to be found per tab-completion etc.), e.g., patchman-multi or patcher-multi.

Will do!

[Olf0]

1. Where does $EUID come from?

POSIX I think. Might be a GNU-ism. It's "effective" UID, i.e. the UID a process is running as (as opposed to UID, the ID of the user running something). E.g. if user mark runs a setuid program whose executable file is owned by mary, UID is mark and EUID is mary.

EDIT: apparently it's a bash-ism, although there's some history in other traditional Unixes. Will fix to use id instead.

Neither EUID or ID is in the environment (env | fgrep ID) of SFOS 3.2.1 and Ubuntu 20.04.
Maybe it needs a euid="$(ps --someopts)" somewhere in this shell script.

Edit: The id command was meant, which provides exactly this information, "In which user context am I running?".

[Olf0]

7. Do the long-opts --apply and --unapply really work for patchmanager proper? Then please also name them here. We should also document all available options somewhere, including Secret functionality: --reset-system #204

Argh! yes! I have a patch somewhere which implements these, but haven't submitted it yet. Will clean it up and do another PR on them.

But then (as currently only -a, -u, --unapply-all and --reset-system are defined, we may really go for

1. -a, --activate
2. -d, --deactivate and for backward compatibility -u
3. --deactivate-all and for backward compatibility --unapply-all

That would make the terms consistent with the ones used at the GUI.

Reasoning: "To apply a patch" sure is a standing phrase for users of the patch utility. But it is only comprehensible for those who understand the technical mechanisms behind Patchmanager, which should be irrelevant for users (GUI and TUI). Furthermore, since Patchmanager 3.0, the patch files of PM-Patches are only virtually applied (to a copy of the real file). This (and the "fake" word "unapply") made me prefer "activate / active" & "deactivate / inactive" to unambiguously denote if a Patch is effective or not.

[Olf0]

Idea to unify the options: Make -e just an "export" option, which writes to STDout, when no -f FILE is provided. With -f FILE it writes to FILE.

Additional idea: -a and -u-d read from STDin (oh, I just realise that I forgot how to do this in a shell script; well, Stackexchange will tell), when no -f FILE option is set. It should only stubbornly activate all enabled Patches, when there is no input at STDin.

Both suggestion would make PM easily scriptable without resorting to xargs.

And if you are building a front end for Patchmanager you could also implement the -l option etc. there, see #198

We could even let that script take patchmanager's place (and rename the Patchmanager executable to patchmanager-backend or so), if it transparently passes all four extant options and implement everything else in the patchmanager script (including your long-opts).

And another idea: Let -a and -d/-u accept comma-separated lists of Patch names, so they become a superset of the original functionality.
But then we really should return to your original concept of -A | --activate-all and -D | --deactivate-all in order to not overload -a and -d/-u.

[Olf0]

I hope you like my overhauling it. Sorry, I became (side-) tracked into it. I rather would have posed a PR, but started at the GitHub web-frontend, which does not allow for that.

I think I implemented all my suggestions. I plan to review / revisit it in a couple of days.

[Olf0]

Didn't we document somewhere what the allowed character set for (internal) Patch names is?
I did not find it, in order to check if my guess was O.K., see line 153.

[nephros]

Didn't we document somewhere what the allowed character set for (internal) Patch names is? I did not find it, in order to check if my guess was O.K., see line 153.

Not that I remember, but I guess that's checked in the web catalog code in some way.

[Olf0]

I plan to review / revisit it in a couple of days.

Done, see commit f43e8b6.

[Olf0]

Please re-review @nephros.

^{Simulating this by "request changes".}

[Olf0]

Didn't we document somewhere what the allowed character set for (internal) Patch names is? I did not find it, in order to check if my guess was O.K., see line 153.

Not that I remember, but I guess that's checked in the web catalog code in some way.

I was unable to find the code which does that at https://github.com/sailfishos-patches/pm-web-catalog. Please check that my checks are not too strict or too relaxed; if anything then rather "too relaxed", because this also checks RPM Patches. But either you or me (or both) should compare this to the Web Catalog, which is more or less a reference.

[nephros]

Please re-review @nephros.

Simulating this by "request changes".

Looks good, I can't review my own PR :)

[Olf0]

Please re-review @nephros.

Looks good, I can't review my own PR :)

1. Makes sense, because I could not request you as a reviewer.

2. What about multi-apply-tool: first version #302 (comment)?

[Olf0]

@nephros, the last open point is the one below. Can you please check this? If that results in a need for a change, you are welcome to delegate that to me. Then just drop a pointer to the corresponding check in the Web Catalog's code here.

Didn't we document somewhere what the allowed character set for (internal) Patch names is? I did not find it, in order to check if my guess was O.K., see line 153.

Not that I remember, but I guess that's checked in the web catalog code in some way.

I was unable to find the code which does that at https://github.com/sailfishos-patches/pm-web-catalog. Please check that my checks are not too strict or too relaxed; if anything then rather "too relaxed", because this also checks RPM Patches. But either you or me (or both) should compare this to the Web Catalog, which is more or less a reference.

[nephros]

@nephros, the last open point is the one below. Can you please check this? If that results in a need for a change, you are welcome to delegate that to me. Then just drop a pointer to the corresponding check in the Web Catalog's code here.

Didn't we document somewhere what the allowed character set for (internal) Patch names is? I did not find it, in order to check if my guess was O.K., see line 153.

Not that I remember, but I guess that's checked in the web catalog code in some way.

I was unable to find the code which does that at https://github.com/sailfishos-patches/pm-web-catalog. Please check that my checks are not too strict or too relaxed; if anything then rather "too relaxed", because this also checks RPM Patches. But either you or me (or both) should compare this to the Web Catalog, which is more or less a reference.

Thanks for the ping.

I looked a bit through the catalog code, all I can find are checks for archive files, and the "project name".

1. doing a check_output() when calling the unpack functions for an uploaded archive:

• https://github.com/sailfishos-patches/pm-web-catalog/blob/master/example/demo/filehandler.py#L16
• https://github.com/sailfishos-patches/pm-web-catalog/blob/master/example/demo/forms.py#L58

so I guess for uploaded archives anything goes that doesn't have something particularly weird as a file name.
The unpack tests only test archives and do not attempt to write any files, so if the archive contains "illegal" file names that would not be detected.

2. The upload itself writes to tmp/%s so any illegal characters in the archive file would fail here, and the archive wouldn't upload successfully.

3. Now, project names are checked here: https://github.com/sailfishos-patches/pm-web-catalog/blob/master/example/demo/models.py#L23 and allow ^([\w-]*)$'. I think this is the relevant check, and yours is more specific than that, but it looks like they should be able to "co-exist" just fine.
   There may be some very subtle differences in what \w means in python vs. the matches in grep (what with unicode and all that), but I doubt it will matter in practice.

[Olf0]

@nephros, the last open point is the one below. Can you please check this? If that results in a need for a change, you are welcome to delegate that to me. Then just drop a pointer to the corresponding check in the Web Catalog's code here.

Didn't we document somewhere what the allowed character set for (internal) Patch names is? I did not find it, in order to check if my guess was O.K., see line 153.

Not that I remember, but I guess that's checked in the web catalog code in some way.

I was unable to find the code which does that at https://github.com/sailfishos-patches/pm-web-catalog. Please check that my checks are not too strict or too relaxed; if anything then rather "too relaxed", because this also checks RPM Patches. But either you or me (or both) should compare this to the Web Catalog, which is more or less a reference.

I looked a bit through the catalog code, all I can find are checks for archive files, and the "project name".

1. doing a check_output() when calling the unpack functions for an uploaded archive:

   • https://github.com/sailfishos-patches/pm-web-catalog/blob/master/example/demo/filehandler.py#L16
   • https://github.com/sailfishos-patches/pm-web-catalog/blob/master/example/demo/forms.py#L58

   so I guess for uploaded archives anything goes that doesn't have something particularly weird as a file name. The unpack tests only test archives and do not attempt to write any files, so if the archive contains "illegal" file names that would not be detected.

2. The upload itself writes to tmp/%s so any illegal characters in the archive file would fail here, and the archive wouldn't upload successfully.

3. Now, project names are checked here: https://github.com/sailfishos-patches/pm-web-catalog/blob/master/example/demo/models.py#L23 and allow ^([\w-]*)$'. I think this is the relevant check, and yours is more specific than that, but it looks like they should be able to "co-exist" just fine.
   There may be some very subtle differences in what \w means in python vs. the matches in grep (what with unicode and all that), but I doubt it will matter in practice.

I checked the last aspect first, i.e., if there are "subtle differences in what \w means in python vs. the matches in grep (what with unicode and all that)", by reading docs.python.org/3/howto/regex and docs.python.org/3/library/re. AFAIU, the baseline is, "do not use \w, if one does not want a locale specific interpretation of the character class", i.e. here, write ^([\w-]*)$' as ^([a-zA-Z0-9_-]*)$'.

For "yours is more specific than that, but it looks like they should be able to "co-exist" just fine" in point 3: Well, mine is more specific (must start with [a-zA-Z] and end with [a-zA-Z0-9]), but also allows for .+ in between (in addition to the extant [a-zA-Z0-9_-]), plus mine must be at least two characters long. IMO these are improvements, hence I will pose a PR for pm-web-catalog/…/models.py: CODeRUS/django-test#8

For the general question, "What are we dealing with there?": To be honest I am a bit confused! Are the Patch names we are dealing with in our script what is called "name - Internal name" in the web-catalog? I.e., are these the names of a downloaded Patchmanager-Patch as a whole (i.e., not unpacked)?
Isn't this exactly the file-name filehandler.py handles?

I assume the answer to all three questions above is "Yes", but I am not sure. Maybe @CODeRUS can enlighten us with a brief statement on this.

[CODeRUS]

iirc name is the final destination of unpack procedure. if you allow it to have some / or . it can possibly escape patches folder by doing ../.. etc and damage filesystem.

[Olf0]

@CODeRUS, thank you very much for your prompt reply.

iirc name is the final destination of unpack procedure. if you allow it to have some / or . it can possibly escape patches folder by doing ../.. etc and damage filesystem.

a. The RegEx I designed does not allow for using "/". As "/" and "." are necessary to construct "../", I cannot see any danger here.
Still I always appreciate very much to take security considerations into account, because there may be someone trying to write a malicious Patch for Patchmanager or (much more likely) someone accidentally does something wrong.

b. As the "name is the final destination of [the] unpack procedure" (AFAICS the "project name", which is validated and this validation is reworked in "[models.py] Improve RegExes", CODeRUS/django-test#8), it is better to also validate the file-name of the uploaded file and additionally the ones in the archive ("Validate file names", CODeRUS/django-test#10).

[CODeRUS]

can you somehow setup a test server to try all these before it will reach production? :)

[Olf0]

Unfortunately not, I lack the infrastructure and know-how (never did anything with Django).

But the logic changes are minimal, just three more if-statements with the same fairly loose RegEx. And it is plain and simple Python code. The biggest danger I see are typos breaking syntax (and one never detects one's own typos well).

I think thoroughly reviewing these two PRs will do, and rather provide a couple of days for that. I will take another close look tomorrow (Sunday) or on Tuesday (and denote when I did that), maybe @nephros can also take a took. Or @nephros, do you have a staging instance of the web-catalog running?

[CODeRUS]

you someone :)

[nephros]

Or @nephros, do you have a staging instance of the web-catalog running?

Hm.

I think I lost something in my basement, let me go upstairs and check!
-- M.C.Escher

Yeah I had it running for a while on a verry unstable and slowly-connected home server. They are offline meanwhile though.

But I could provide a testing instance in a pinch.

Would an empty one (i.e. one with NO user accounts, no files uploaded etc) suffice, or would it be better to have a clone of the production catalog?

[Olf0]

0. Background / Motivation

When I analysed in detail which checks for the file-names of uploads to Patchmanager3's web-catalog currently exist (i.e., the archive name and the names of the files in the archive), there was none (except for the one nephros already mentioned, the archive file can be successfully written to the file-system, plus a file named unified_diff.patch exists in the uploaded archive).
While a checks for file name extensions are performed for the archive file ('tar.gz', '.tar.bz2', '.tar.xz' or '.zip') twice ([1] and [2], which makes sense, because [1] happens before the archive is written to the file-system and [2] after that when listing the archive's content) and for the files in the archive ('.qml', '.js', '.png', '.svg', '.qm', plus the file-name 'unified_diff.patch') there are no checks for the full file names, specifically preventing a ../ to escape the working directory. Hence I applied the simple RegEx I constructed for checking a Patch's "project name" (first for the "multi-apply-tool" originally discussed here, then also for the "project name" supplied in the web-form for uploads to the web-catalog) also to the uploaded archive name and the file-names in the archive, which does not accept a slash ("/") in a name.

1. Reviewing the changes to Patchmanager3's web-catalog code by "someone"

Task: Review of Python code changes and RegEx changes in the PRs "[models.py] Improve RegExes", CODeRUS/django-test#8 and "Validate file names", CODeRUS/django-test#10.

@pherjung, as you mentioned that you have some experience with Python code(ing), and these changes are really small (by amount of changed / added code), may I ask you to review these two PRs? As already discussed and because I try to pull you into reviewing something you have not volunteered for (Patchmanager3's web-catalog), you sure have the freedom to say "No"; unfortunately I have no idea who else I may ask (CODeRUS lacks any time, nephros usually does not code in Python, I have written these changes). To understand and check the motivation and logic behind these changes, start reading at this comment of this thread here up to this message you are currently reading, and the commit comments for the individual commits in the two PRs. Take your time for this review (i.e., this review is not temporally critical), and I would appreciate much, if you help to alleviate CODeRUS' (and to a lesser extent also my) concerns of deploying unreviewed changes in a production environment; no matter how small these changes are, typos (syntax or structural errors) or think'os (logic or reasoning flaws) may have slipped in. Feedback is best provided as comments to these two PRs, maybe except for fundamental think'os, which are better discussed here so nephros can participate.

2. Testing the changes to Patchmanager3's web-catalog by "someone"

Or @nephros, do you have a staging instance of the web-catalog running?

Yeah I had it running for a while on a very unstable and slowly-connected home server. They are offline meanwhile though.
But I could provide a testing instance in a pinch.
Would an empty one (i.e. one with NO user accounts, no files uploaded etc) suffice, or would it be better to have a clone of the production catalog?

Because I solely altered code, which is executed when uploading a Patch to the web-catalog, I think that uploading a few extant Patches with the PRs "[models.py] Improve RegExes", CODeRUS/django-test#8 and "Validate file names", CODeRUS/django-test#10 employed is fully sufficient. If you have an idea how to easily automate uploading of Patches to the web-catalog, then you might clone the production catalog and re-upload all of them to the test instance. As I mentioned to Patrick (pherjung), this is not temporally critical.

[pherjung]

I'll take a look next week.

[pherjung]

Speaking of a test instance, I have a VM running where I can do such job. I can add your SSH public key if you want to access that VM.

[Olf0]

Speaking of a test instance, I have a VM running where I can do such job. I can add your SSH public key if you want to access that VM.

Thank you for the offer, but I deliberately wanted to avoid learning how to install and configure Django and the Web Catalog software, because that would likely cost me some hours, which translates to at least a week in wall time. I also remembered that nephros once had a test instance of the Web Catalog running, but did not know its status and accessibility: As nephros pointed out, he was willing to reactivate it, and the security, bandwidth and maybe also stability limitations are not relevant for testing these additional input filters. Meanwhile I have gained access to it and yesterday we managed to deploy the current master branch plus my two PRs on it. I will now consider how to test effectively and create another PR which overhauls the strings.

Still I would appreciate very much, if you thoroughly scrutinise my two PRs by reviewing the changes and their reasoning. While I am quite convinced that these small changes are fine (minus own typos and think'os I might fail to detect), for a good part this is about assuring coderus that it is O.K. to deploy these changes in his production environment (because he lacks the time to test and to quickly react, if there is some fall-out).

[Olf0]

@nephros, ultimately two topics became intermingled here (well, actually I did that):
The multi-apply-tool and enhancements of the Web-Catalog (though they are connected by the task of determining which characters / file names shall be allowed for Patches and their files).

While I hope to return to the latter in the second week of January 2023, IMO this MR can and should be merged now, because it has been ready for months. I also prepared a release of v3.2.3, because we already have accumulated quite some commits, which luckily all look non-critical (and so does this one, thus it fits well), hence this should be an easy release.

I was about to hit the "Squash and merge" button in GitHub's web-frontend for this MR, but first wanted align with you(r opinion); also about releasing v3.2.3 when this is merged.

[nephros]

IMO this MR can and should be merged now,
[...]
I was about to hit the "Squash and merge" button in GitHub's web-frontend for this MR, but first wanted align with you(r opinion); also about releasing v3.2.3 when this is merged.

Agreed, and I have nothing queued for this branch IIRC.

Therefore: LGTM, and merging.