Tails

#1 Updated by Anonymous 2018-03-13 14:29:41

• related to Bug #15185: Automatically update Weblate components on mdwn file add/rename/delete in tails.bo git added

#2 Updated by Anonymous 2018-03-13 15:53:38

• Description updated

#3 Updated by Anonymous 2018-04-06 10:32:50

• Subject changed from Post merge hook for Weblate to Pre-receive hook for Main git when handling pushs from weblate

#!/usr/bin/env python
# Pre-receive or update hook for main git
# Weblate is only allowed to push po files.

from subprocess import call

expected = "tails-l10n@boum.org"
regex = "/\*\.po\$/"

def check_commit_author_weblate():
    rev_list = subprocess.run("git rev-list #{$oldrev}..#{$newrev}", shell=True, check=True)
    rev_list = rev_list.split("\n")
    for rev in rev_list:
        # get committer email
        committer = subprocess.run("git cat-file commit #{rev} | grep 'committer' | grep -o -P '(?<=\<).*(?=\>)'")
        files = subprocess.run("git diff-tree --no-commit-id --name-only -r #{rev}")
        if expected == committer and not regex.search(files)
            print "[POLICY] Weblate is only allowed to modify po files."
            return False
check_commit_author_weblate()

For now, the criteria to check against a commit author is very weak. This should be completed by some environment variable when pushing over SSH -> this would ensure we do this check only when weblate pushes, not somebody else.

#4 Updated by bertagaz 2018-05-10 11:09:12

• Target version changed from Tails_3.7 to Tails_3.8

#5 Updated by Anonymous 2018-06-12 09:31:26

• Assignee set to groente
• QA Check set to Info Needed

Hi groente, as our preferred rubber duck for this project, here is a question for you:

How can we ensure to check from the production machine that a push over SSH is indeed done by Weblate and not anybody else? We want to ensure that Weblate only pushed po modifications and nothing else.

I wrote the above script with the criteria to check against a commit author, but I’m totally aware that this check is very weak, if not useless. The only other way we could think of is to check for an environment variable. But maybe rubber ducks have better ideas here?

#6 Updated by intrigeri 2018-06-26 16:27:47

• Target version changed from Tails_3.8 to Tails_3.9

#7 Updated by groente 2018-06-27 13:09:43

• Assignee deleted (groente)
• QA Check changed from Info Needed to Dev Needed

Heya, so after testing with printenv as a git-hook, i found out that gitolite-shell seems to store the username associated with the key that was used to authenticate in an environment variable called GL_USER.
Checking whether this variable matches the desired username would be a lot safer than checking the commit author name.

#9 Updated by Anonymous 2018-09-03 17:57:44

• Target version changed from Tails_3.9 to Tails_3.10.1

#10 Updated by intrigeri 2018-10-24 17:03:34

• Target version changed from Tails_3.10.1 to Tails_3.11

#11 Updated by Anonymous 2018-11-02 16:39:26

• Assignee set to hefee

groente wrote:
> Heya, so after testing with printenv as a git-hook, i found out that gitolite-shell seems to store the username associated with the key that was used to authenticate in an environment variable called GL_USER.
> Checking whether this variable matches the desired username would be a lot safer than checking the commit author name.

@groente, thanks! We’ll look into it.

#12 Updated by hefee 2018-11-05 17:20:27

• related to Bug #15401: Create hooks to be implemented on the Weblate instance added

#13 Updated by hefee 2018-11-12 14:48:39

• Status changed from Confirmed to In Progress
• Assignee changed from hefee to groente
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch set to https://salsa.debian.org/hefee/puppet-tails/tree/bugfix/15402-weblate-hook

I now checked the script and add it puppet-tails. Locally I checked following scenarios:

• tries to push commits as other committer
• tries to update non po file
• tries to delete a po file
• tries to create a new branch

I install this hook as update hook, because pre-receive hook had not the needed information oldrev and newrev. It may be different, if installed on server.

#14 Updated by groente 2018-11-13 10:16:50

• Assignee changed from groente to bertagaz
• % Done changed from 0 to 50
• Feature Branch changed from https://salsa.debian.org/hefee/puppet-tails/tree/bugfix/15402-weblate-hook to puppet-tails:bugfix/15402-update-hook-for-weblate-pushes

hey bertagaz,

i’ve reviewed hefee’s script, which looks good to me, can you check if i forgot about anything in the deploy? thank you!

#16 Updated by groente 2018-11-16 11:48:34

• Assignee changed from bertagaz to hefee
• QA Check changed from Ready for QA to Dev Needed

intri does raise a good point, that regex atleast needs a trailing $ to prevent weblate from modifying any file which happens to have .po in it (.pools, .policy, etc).

#17 Updated by hefee 2018-11-16 13:06:41

• Assignee changed from hefee to bertagaz
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch changed from puppet-tails:bugfix/15402-update-hook-for-weblate-pushes to https://salsa.debian.org/hefee/puppet-tails/tree/bugfix/15402-update-hook-for-weblate-pushes

groente wrote:
> intri does raise a good point, that regex atleast needs a trailing $ to prevent weblate from modifying any file which happens to have .po in it (.pools, .policy, etc).

ACK - fixed. I thoiught that re.match does check, that the complete string matches to content, in contrast to re.search. But the only difference is, that it searches from the beginning.

Another thing I found is:

if newrev == “0000000000000000000000000000000000000000”:
sys.exit(“[POLICY] Weblate is not allowed to delete branches.”)

Maybe also replace this long 0 with a regex ^0*$, don’t know if the length is fixed.

Do we also want to close the allowed refname updates, as it is done in templates/gitolite/hooks/tails-post-receive.erb

#19 Updated by CyrilBrulebois 2018-12-16 13:58:21

• Target version changed from Tails_3.11 to Tails_3.12

#20 Updated by intrigeri 2019-01-08 15:51:35

• Parent task changed from Feature #15079 to Feature #15082

#21 Updated by Anonymous 2019-01-11 13:30:09

• Assignee deleted (bertagaz)

#22 Updated by intrigeri 2019-01-11 16:32:45

• Target version changed from Tails_3.12 to Tails_3.14

#23 Updated by intrigeri 2019-01-11 16:33:32

• Target version changed from Tails_3.14 to Tails_3.13

(Actually, it would be nice if a first security-oriented review was done sufficiently in advance before our sprint.)

#24 Updated by Anonymous 2019-02-07 15:05:39

• Assignee set to enrico

I propose that this can have a first round of review by enrico for security issues. When done, it can be reassigned to intrigeri.

#25 Updated by hefee 2019-02-08 18:13:44

• Description updated

I have upded the description of this ticket so encrio should now have enough context to do a security review of this script.

#26 Updated by enrico 2019-03-11 16:16:55

• Assignee changed from enrico to hefee

Hello,

here are my comments on the tails-weblate-update.hook script. Thanks for the
detailed introduction, it had all I needed to get this going!

I have some structural questions, before looking at the code in detail.

The script currently uses git cat-file and git diff-tree to get committer
name and files changed, and does quite a bit of parsing on their output. I
would suggest to ask git for a machine readable version of what we need,
with something like git show --format="%ce" --name-status <shasum>, to
minimize the amount of parsing on our side that might go wrong.

Alternatively, could gitpython be used instead of running git with subprocess?
Then we wouldn’t need to bother with writing parsing code.

The script currently checks commits only if the committer is the weblate
user. What happens if I merge commits from weblate into some branch of mine,
then push them? Should those weblate changes be checked by the script, or
should they be considered as reviewed by the actual committer?

Looking at the code in detail, I have mostly nitpicks.

English tweaks:

-# Update git hook to ensure, that a user is only allowed to push modifications
-# to existiong files. Only works with gitolite ( as this sets the env variable GL_USER
+# Update git hook to ensure that a user is only allowed to push modifications
+# to existing files. Only works with gitolite, as this sets the env variable GL_USER
 # that is connected to the ssh key used for pushing.

-# It is used to ensure, that Weblate is only allowed to push po files.
+# It is used to ensure that Weblate is only allowed to push po files.

I would use variable names that are more representative of how they are used:

 # Gitolite sets env variable for the user for the SSH user
 GL_USER = "weblate"
+# EXPECTED_GL_USER

 # Only allowed commiter mail
 COMMITTER = "weblate <tails-l10n@boum.org>"
+# EXPECTED_COMMITTER

This regexp missed the r-string marker:

 # Regex to match for modified po files in raw output format of git diff-tree
 # in-place edit  :100644 100644 bcd1234... 0123456... M file0
-reModifyFile = re.compile("^[^\t]* M\t.*\.po$")
+reModifyFile = re.compile(r"^[^\t]* M\t.*\.po$")

I like regexps to be anchored if they need to match at the beginning of the
line, even though they are run with .match(). This way if someone
accidentally changes match with a search, the intent of the regexp doesn’t
change. I’m also afraid of using .+ except when really needed, and I would
opt for being super-strict instead:

 # Regex to match the commiter line of git cat-file.
 # E.g "committer Sandro Knauß <hefee@debian.org> 1542020415 +0100"
-reCommitter = re.compile("committer (?P<committer>(?P<name>.*) (<(?P<mail>.*)>)) .*")
+reCommitter = re.compile(r"^committer [^<]+<(?P<email>[^>]+)> \d+ [-+][0-9]+$")

In the committer function, I notice that only the committer string is ever
used, and it’s only ever checked against COMMITTER. At this point, one could
just switch to matching the whole line with a regexp like this one:

re_committer = re.compile(r"^committer weblate <tails-l10n@boum.org> \d+ [-+][0-9]+$")

Note that in the committer function you also want to stop parsing at the
first empty line, or you might match a phony committer line inside the commit
message. Alternatively, just let git do the work and use
git show --format=….

Note that I managed to create commit objects with pretty much any string I
wanted in them (git hash-object -t commit -w <filename> gives you a commit
that you can push, and it contains whatever is in that file), so depending on
how untrustful you want to be of weblate pushes, you can decide not to have git
do any parsing and strictly validate any line of the commit file that comes
before the empty lines that marks the begin of the commit message.

Random python tidbits:

Where you do: _ = subprocess.check_output, normally the _ variable in
python is used to mean “this value is ignored”, and I would use a different
variable name.

If you always decode the result of subprocess.check_output, you may want to
pass universal_newlines=True to check_output to get unicode strings
instead.

As a general practice in python scripts, I like to put my main code inside a
function, and then call it with:

def main():
    ref = sys.argv[1]
    oldrev = sys.argv[2]
    newrev = sys.argv[3]
    …


if __name__ == "__main__":
    main()

This makes the variables used by main scoped at a function level instead of
module level (I spent some unpleasant time debugging issued caused by this in
the past).

Also, calling the function inside if __name__ == "__main__": makes the script
importable as a module, and therefore unit-testable. Would it be worth having
unit tests for this?

In the main if os.environ.get("GL_USER")…:, I would explicitly have
an else: branch even if it does nothing: in security sensitive code, I’d like
to make any code that does nothing explicit, like this:

if os.environ.get("GL_USER") == GL_USER:
    …
else:
    # We do not interfere with commits from other users
    pass

#28 Updated by Anonymous 2019-03-12 14:05:10

• QA Check changed from Ready for QA to Dev Needed

#29 Updated by hefee 2019-03-12 19:58:26

• Assignee changed from hefee to enrico
• QA Check changed from Dev Needed to Info Needed

Thanks a lot for your review. I have no further comments on them atm and fill fix them soon after the further input, also started to write unit-tests for the script.

enrico wrote:
> Alternatively, could gitpython be used instead of running git with subprocess?
Then we wouldn’t need to bother with writing parsing code.

@intrigeri : I would highly like to use gitpython for this code. Is it possible to install python3-git on the gitolite server? What python3 version do we have on gitolite server?

enrico wrote:
> [treppenwitz] Another design question: how much should we not trust weblate pushes?

@enrico: What scenarios are you thinking of?

> In particular, should we check things like these?
> * the .po files have the right format
this stuff should be checker later on by Jenkins and unify_po (Bug #15403).

> * the .po files are in the correct paths
> * the .po file name matches a language code
> * the .po files have the right permissions

that makes sense it easy to check.

#31 Updated by enrico 2019-03-13 09:14:33

• Assignee changed from enrico to hefee

hefee wrote:

> enrico wrote:
> > [treppenwitz] Another design question: how much should we not trust weblate pushes?
>
> @enrico: What scenarios are you thinking of?

The description of this issue says: “the translation-server may be compromised and the weblate user may pushes malicious commits”, and turning my level of paranoia waay up, I was wondering about what a malicious commit could do that is limited to files with .po extension, and I thought:

• add malicious data in potfiles to trigger potential buffer overflows in tools that process them
• put .po files in directories where they would be read as something else, like, say, configuration files that are read regardless of their extensions

Also, I don’t know what kind of damage one could do down the toolchain by putting garbage in the git commit object. I can’t come up with a reasonable attack scenario here, and unless someone can, feel free to disregard this potential attack vector.

#33 Updated by CyrilBrulebois 2019-03-20 14:34:04

• Target version changed from Tails_3.13 to Tails_3.14

#34 Updated by hefee 2019-04-01 17:10:04

• Assignee changed from hefee to enrico
• QA Check changed from Info Needed to Ready for QA

Thaks for you detailed review. Now the second round of review.

enrico wrote:
> Alternatively, could gitpython be used instead of running git with subprocess?
> Then we wouldn’t need to bother with writing parsing code.

good point use now gitpython.

> The script currently checks commits only if the committer is the weblate
> user. What happens if I merge commits from weblate into some branch of mine,
> then push them? Should those weblate changes be checked by the script, or
> should they be considered as reviewed by the actual committer?

yes if a core dev pushes changes from weblate user and the checks should not take place, in such a scenario. A core dev is responsible for all their commits they push.

> Looking at the code in detail, I have mostly nitpicks.

done.

> I like regexps to be anchored if they need to match at the beginning of the
> line, even though they are run with .match(). This way if someone
> accidentally changes match with a search, the intent of the regexp doesn’t
> change. I’m also afraid of using .+ except when really needed, and I would
> opt for being super-strict instead:

done.

> In the committer function, I notice that only the committer string is ever
> used, and it’s only ever checked against COMMITTER. At this point, one could
> just switch to matching the whole line with a regexp like this one:

not needed anymore, as we use gitpython.

> Note that in the committer function you also want to stop parsing at the
> first empty line, or you might match a phony committer line inside the commit
> message. Alternatively, just let git do the work and use
> git show --format=….

not needed anymore, as we use gitpython.

> Note that I managed to create commit objects with pretty much any string I
> wanted in them (git hash-object -t commit -w <filename> gives you a commit
> that you can push, and it contains whatever is in that file), so depending on
> how untrustful you want to be of weblate pushes, you can decide not to have git
> do any parsing and strictly validate any line of the commit file that comes
> before the empty lines that marks the begin of the commit message.

should be fixed with using gitpython.

> Random python tidbits:
>
> Where you do: _ = subprocess.check_output, normally the _ variable in
> python is used to mean “this value is ignored”, and I would use a different
> variable name.

ACK.

> If you always decode the result of subprocess.check_output, you may want to
> pass universal_newlines=True to check_output to get unicode strings
> instead.

ACK..

> As a general practice in python scripts, I like to put my main code inside a
> function, and then call it with:

On the other side, it makes it harder to run interactive python shell, or do you know a way, how I can make the python interactive shell stay in a functions scope?

> This makes the variables used by main scoped at a function level instead of
> module level (I spent some unpleasant time debugging issued caused by this in
> the past).

Me, too ;)

> Also, calling the function inside if __name__ == "__main__": makes the script
> importable as a module, and therefore unit-testable. Would it be worth having
> unit tests for this?

Created tests for main function. But not for the check_commit_author_weblate, as I don’t have a good idea, how a good test environment should look like. As I think the tests should use a real git repository. But downloading every time the tails.git would make the tests very slow and network intensive. Any ideas?

@intrigeri: it would make sense to run the tests on CI, but therefore the file needs to have a .py suffix instead of .hook. But than the files on the server are named differently than in puppet. Aand shipping the tests inside the file, makes the file hard to read, that’s why better create a sub directory with those tests.

> In the main if os.environ.get("GL_USER")…:, I would explicitly have
> an else: branch even if it does nothing: in security sensitive code, I’d like
> to make any code that does nothing explicit, like this:

ACK

additional checks:

> the .po files are in the correct paths
done.

> the .po files have the right permissions

done.

> the .po file name matches a language code

As source of allowed language codes I use the langs.json, that is created out of templates/weblate/langs.json.erb. This limits the allowed languages for those, that are handled by Weblate at all. Do you have any ideas, how to make ADDITIONAL_LANGUAGES lazy loading, because currently the json file is parsed for every commit also if we do not need those languages at all.

#36 Updated by enrico 2019-04-22 10:09:24

• File test-tails-weblate-update added
• Assignee changed from enrico to hefee

> yes if a core dev pushes changes from weblate user and the checks should not
> take place, in such a scenario. A core dev is responsible for all their
> commits they push.

Ack.

> On the other side, it makes it harder to run interactive python shell, or do
> you know a way, how I can make the python interactive shell stay in a
> functions scope?

I think I don’t understand the question: can you give me an example of what you would do with the shell?

> Created tests for main function. But not for the check_commit_author_weblate,
> as I don’t have a good idea, how a good test environment should look like. As
> I think the tests should use a real git repository. But downloading every
> time the tails.git would make the tests very slow and network intensive. Any
> ideas?

Given that check_commit_author_weblate takes a repo and a revision range, I’d create mock git repos in a tempdir using gitpython, rather than work with the real ones. It would also make it possible to hand-craft all sorts of malicious one-off repos for testing.

> As source of allowed language codes I use the langs.json, that is created out of templates/weblate/langs.json.erb. This limits the allowed languages for those, that are handled by Weblate at all. Do you have any ideas, how to make ADDITIONAL_LANGUAGES lazy loading, because currently the json file is parsed for every commit also if we do not need those languages at all.

If parsing langs.json is not slowing things down significantly, I’d just leave it as it is, as I expect that there aren’t going to be so many pushes that it would create trouble.

Otherwise, this could be a lazy loading system:

class AllowedLanguages:
    def __init__(self, pathname):
        self.pathname = pathname
        self.result = None

    def load(self):
        # Current getAllowedLanguages implementation, assigning to self.result

    def __iter__(self):
        if self.result is None:
            self.load()
        return self.result


ALLOWED_LANGUAGES = AllowedLanguages(pathlib.Path(os.path.realpath(__file__)))

With regards to a review, I don’t see any potential security issues in the code, besides the possibility that there are checks we have not thought of or that are not part of the current requirements, like validating the contents of .po files, or making sure .po files are only committed to a limited set of directories.

For example, an attack scenario could be adding a .po file that contains malicious commands into a config directory read by something that does something like for file in myconfig.rc/*; do source $file; done

Some nitpicks:

getAllowedLanguages and reModifyFile should probably be get_allowed_languages and re_modify_file (see
https://www.python.org/dev/peps/pep-0008/#function-and-variable-names )

In check_commit_author_weblate, I’d keep iterating all commits appending errors to a list, and return the list of error messages: that way one can see all the issues in a push and not just the first one. In main(), if the result is the empty string we are fine, oherwise one can print the list to stderr and exit with an error code.

The check_commit_author_weblate name does not seem to match its function anymore: I propose something like validate_commit_range instead.

I like the idea of moving the test suite to a different file and location in the git repo. I’m attaching an example of the split test suite, which can load the hook from an arbitrary path.

#38 Updated by hefee 2019-05-08 14:56:37

• Assignee changed from hefee to enrico

updated my branch on salsa.

enrico wrote:
> > On the other side, it makes it harder to run interactive python shell, or do
> > you know a way, how I can make the python interactive shell stay in a
> > functions scope?
>
> I think I don’t understand the question: can you give me an example of what you would do with the shell?

This is a more gerneral question about Python. If I debugging a function/class how do I manage to be in the method/class context, so I can look at all local variables interactivly. In meanwhile I found out, that I can use ipython and breakpoints to do such stuff. But this topic is out of scope.

> > Created tests for main function. But not for the check_commit_author_weblate,
> > as I don’t have a good idea, how a good test environment should look like. As
> > I think the tests should use a real git repository. But downloading every
> > time the tails.git would make the tests very slow and network intensive. Any
> > ideas?
>
> Given that check_commit_author_weblate takes a repo and a revision range, I’d create mock git repos in a tempdir using gitpython, rather than work with the real ones. It would also make it possible to hand-craft all sorts of malicious one-off repos for testing.

Done.

> > As source of allowed language codes I use the langs.json, that is created out of templates/weblate/langs.json.erb. This limits the allowed languages for those, that are handled by Weblate at all. Do you have any ideas, how to make ADDITIONAL_LANGUAGES lazy loading, because currently the json file is parsed for every commit also if we do not need those languages at all.
>
> If parsing langs.json is not slowing things down significantly, I’d just leave it as it is, as I expect that there aren’t going to be so many pushes that it would create trouble.

Also for testing purpose I need to disable the langs.json loading, as this file is not available at expected location.

> With regards to a review, I don’t see any potential security issues in the code, besides the possibility that there are checks we have not thought of or that are not part of the current requirements, like validating the contents of .po files, or making sure .po files are only committed to a limited set of directories.

We have the limitation of directories (see ALLOWED_DIRECTORIES). Or are you referring to something different. We desided that the content of po files will be checked by Jenkins.

> For example, an attack scenario could be adding a .po file that contains malicious commands into a config directory read by something that does something like for file in myconfig.rc/*; do source $file; done

@intrigeri - I see, maybe we need to move all non wiki files out of wiki/src, so that this folder only contains data and no configs.

>
> Some nitpicks:
>
> getAllowedLanguages and reModifyFile should probably be get_allowed_languages and re_modify_file (see
> https://www.python.org/dev/peps/pep-0008/#function-and-variable-names )

Me personally do not like the “_” variable names.

> In check_commit_author_weblate, I’d keep iterating all commits appending errors to a list, and return the list of error messages: that way one can see all the issues in a push and not just the first one. In main(), if the result is the empty string we are fine, oherwise one can print the list to stderr and exit with an error code.

done

> The check_commit_author_weblate name does not seem to match its function anymore: I propose something like validate_commit_range instead.

done

> I like the idea of moving the test suite to a different file and location in the git repo. I’m attaching an example of the split test suite, which can load the hook from an arbitrary path.

done

#45 Updated by enrico 2019-05-20 12:26:07

• Assignee changed from enrico to hefee

hefee wrote:

> > For example, an attack scenario could be adding a .po file that contains malicious commands into a config directory read by something that does something like for file in myconfig.rc/*; do source $file; done
>
> @intrigeri - I see, maybe we need to move all non wiki files out of wiki/src, so that this folder only contains data and no configs.

I’m leaving this discussion to the people who are in charge of the design; I see no reason to block in a review given the current specification, and I’d leave this discussion to the specification of a following release, so at least something useful can get deployed in the meantime.

> > getAllowedLanguages and reModifyFile should probably be get_allowed_languages and re_modify_file (see
> > https://www.python.org/dev/peps/pep-0008/#function-and-variable-names )
>
> Me personally do not like the “_” variable names.

I respect your preference, but if other people are going to have to maintain your Python code, they will be used to, and expect, the coding conventions of the Python ecosystem, and will find it harder to read your code if you do not follow them.

Unless you are writing code that is intended to be forever maintained only by you and you alone, which I assume is not the case here, I consider not following established naming conventions an issue :)

Everything else looks good to me!

#46 Updated by CyrilBrulebois 2019-05-23 21:23:19

• Target version changed from Tails_3.14 to Tails_3.15

#47 Updated by hefee 2019-05-24 17:45:37

• Assignee deleted (hefee)
• Target version deleted (Tails_3.15)
• QA Check changed from Ready for QA to Pass

#49 Updated by hefee 2019-05-24 17:56:21

• related to Bug #16760: remove executable bit for po files added

#51 Updated by intrigeri 2019-06-01 12:43:15

• blocks Bug #16712: Run tests for tails-weblate-update.hook added

#52 Updated by intrigeri 2019-06-01 12:46:48

• related to Bug #16761: Move non wiki related files out of wiki/src subdir added

#53 Updated by intrigeri 2019-06-01 13:08:09

• Assignee set to hefee
• Target version set to Tails_3.15
• QA Check deleted (Pass)

@hefee, is it on purpose that you’ve marked this as “QA Check = Pass” in Bug #15402#note-47, despite the PEP 8 violations Enrico pointed out?

If yes: I can surely live with it and won’t make a bigger fuss about it than this, but Enrico made a good point, and FYI we’ve been trying to make our code base more PEP 8 compliant in other places in the last few years, so introducing new non-compliant code feels like a small step backward to me.

Other than that, about for fname in commit.stats.files.keys() (https://salsa.debian.org/hefee/puppet-tails/blob/bugfix/15402-update-hook-for-weblate-pushes/files/gitolite/hooks/tails-weblate-update.hook#L68), I think it’s worth adding a simplistic security discussion as a comment, that proves why it’s safe wrt. an adversarial carefully crafted merge commit: https://github.com/gitpython-developers/GitPython/blob/master/git/objects/commit.py#L233 says that only the first parent will be diff’ed against. At first glance it’s not a problem but I’d rather not have to wonder again next time I see this code and am like “uh oh, this looks potentially unsafe”.

#55 Updated by intrigeri 2019-06-26 09:40:59

• Status changed from In Progress to Needs Validation
• Assignee changed from hefee to intrigeri

hefee wrote:
> intrigeri wrote:
> > Other than that, about for fname in commit.stats.files.keys() (https://salsa.debian.org/hefee/puppet-tails/blob/bugfix/15402-update-hook-for-weblate-pushes/files/gitolite/hooks/tails-weblate-update.hook#L68), I think it’s worth adding a simplistic security discussion as a comment, that proves why it’s safe wrt. an adversarial carefully crafted merge commit: https://github.com/gitpython-developers/GitPython/blob/master/git/objects/commit.py#L233 says that only the first parent will be diff’ed against. At first glance it’s not a problem but I’d rather not have to wonder again next time I see this code and am like “uh oh, this looks potentially unsafe”.
>
> I’m not that deep in git, to understand your comment and why you think, this may be unsafe. So far I understand this is about merge commits. Isn’t the first parent always the old commit before you merged? And the diff against the first parent is the diff that the merge did on our branch. As you could also used merge strategy “ours” where the diff against our branch is zero.

FTR, 2c3bbce07ac9d79bd4e53cbcb99c10dab429b9b5 in puppet-tails.git addresses this, by validating content via the complete diff instead of each individual commit.

#56 Updated by intrigeri 2019-06-26 10:17:29

• Subject changed from Pre-receive hook for Main git when handling pushs from weblate to update hook for Main git when handling push from weblate

#57 Updated by intrigeri 2019-06-26 10:54:52

• Status changed from Needs Validation to Resolved
• Assignee deleted (intrigeri)

Deployed. Let’s see what happens once we allow weblate to push there and this hook tries to do actual work.