Action fails on not finding comment

[dominikwilkowski]

Hey there

I created a test repo over here: /~https://github.com/dominikwilkowski/test-voting-repo
that is failing on cannot find comment on issue = 2 as you can see here: /~https://github.com/dominikwilkowski/test-voting-repo/actions/runs/4825777737/jobs/8596966422

Any help here is appreciated

[dominikwilkowski]

Upon forking /~https://github.com/myyk/git-democracy-example to /~https://github.com/dominikwilkowski/git-democracy-example and only changing a single name in the voters.yml file The action fails again on the same error which leads me to believe this action isn't maintained and currently broken. Let me know if I am missing something though.
Cheers

[myyk]

Hi, sorry for the late reply! I do try to maintain this. I think something broke along the way and I don't think my testing method kept working.

I'm planning to look at it sometime this week. I want to see if I can improve the testing and also fix this bug for you since it looks like basic functionality is not working.

[dominikwilkowski]

Thank you @myyk

[myyk]

Hi @dominikwilkowski, I started looking into it by creating a "new" branch (reused an old one) in git-democracy-example. It seems to be working there.

Then I looked back at your repo and it seems to be working now - dominikwilkowski/test-voting-repo#2.

Did you figure out what might have gone wrong? Was there a missing permission or something? (I'll update instructions if I can confirm and repro the issue)

[dominikwilkowski]

Hey
Yes I figured out (with the help of @gwyneplaine) it had a permission missing in the settings:

But then when it finally ran it had lot's of issues which you can see in the repo now with never finding the repo comment and creating a new one, loosing the votes etc. For now I will write a private action that is closely inspired by this action. Once I have time again I can feed the learnings back

[myyk]

That would be great, I'll update the docs to mention that setting. A lot has changed with GitHub Actions since I wrote this tool. I'm glad you have something working!

…

On Tue, May 2, 2023, 17:29 Dominik Wilkowski ***@***.***> wrote: Hey Yes I figured out (with the help of @gwyneplaine </~https://github.com/gwyneplaine>) it had a permission missing in the settings: [image: image] <https://user-images.githubusercontent.com/1266923/235630498-3e920bb8-a9bc-4e4e-bf7d-a4f6d97985f2.png> But then when it finally ran it had lot's of issues which you can see in the repo now with never finding the repo comment and creating a new one, loosing the votes etc. For now I will write a private action that is closely inspired by this action. Once I have time again I can feed the learnings back — Reply to this email directly, view it on GitHub <#763 (comment)>, or unsubscribe </~https://github.com/notifications/unsubscribe-auth/AAP4ANFT674IUQRPSYI4S53XEDHZDANCNFSM6AAAAAAXOR42UA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[dominikwilkowski]

FYI I wrote a small action here that was heavily inspired by this one.
/~https://github.com/dominikwilkowski/test-voting-repo

Material differences:

• I use PR reviews as the mechanism of voting rather than reactions which means I can re-run the action on each review
• I delete old bot comments and add a new one each time
• I merged voting.yml and voters.yml into a single file

[myyk]

Thanks for sharing! I don't know why I didn't think of using PR reviews as the mechanism for voting. It fits really well bc PR are already a vote of sorts and I did want that before.

I might do something similar in a v2 of this.

I kept voting/voter separate bc voting.yml would rarely change, I think. And then that would just be easier to see how long the voting system is stable. I probably wouldn't pull that change.

For bot comments, deleting vs updating seems fine. I think this one was working by keeping old votes around after a new commit was added. I think if I rework this to use PR approvals, I would still want to see votes on an earlier push, though voting restarted on a push.

Thanks for inspiring me too!

[dominikwilkowski]

I also noticed one issue while working through this: The config was branch based in other words someone could change the voting.yml or voters.yml in their own branch and that would be used in their PR which could be a security issue of sorts. Perhaps env vars would be better here as they are global to all branches? Food for thought :)

[myyk]

@dominikwilkowski does this not work to prevent that? /~https://github.com/myyk/git-democracy#workflow-integration. Using pull_request_target that is.

[dominikwilkowski]

I tested it and it didn't seem to work but its entirely possible I just mixed something up. I understood the docs as saying it executes the action in the context of the target branch but of course uses the new files that were changed (or the action wouldn't test what is in the PR) which means someone changing the config files would still yield a change in config for the action. But I could be wrong. Worth testing.

[myyk]

Oh yeah, I didn't think that through correctly. The yml files are not protected correctly bc they aren't part of the workflow definition. It's nice to have a collaborator on this to catch these design flaws.

I definitely need to fix this! It's a critical security flaw.

[myyk]

Ok, I think I'm going to go with moving the voting and voters configs into the action's inputs, here's my reasoning.

Features needed:

• Attacker cannot hijack the repo through git-democracy.
• Configuration is voteable - Voters can vote to change the current voters or voting configs.
• Usage is easy.
• git-democracy is updatable.
• Config is transparent.

We could configure with:

1. Files in repo (currently) - Security issue since attacker can change voters to themself in a PR and take over the repo.
2. Files in forked git-democracy repo - I haven't fully tested this idea, but it might be able to work without fully copying all code so that it's still somewhat updatable. This is a lot of overhead to use the action though.
3. Change the config files to Github Action Inputs - This gets protection by using pull_request_target, users can still build 2 if they want. It also could solve Add support for separate voting.yml #448 by allowing multiple configurations of the GHA in different ways in the same repo if desired.
4. Use env vars for configs - This one is not safely updateable through voting. It might not be transparent too (have not confirmed).

It looks like (3) is the only one that hits all of these.

[dominikwilkowski]

I like option 3. You could also just put the config file as it is now (you have 2) and put it into the .GitHub folder. That way you get the same protection and people don't mess with your action workflow files.

[myyk]

Oh, I didn't know that there was an option 5. I'll see if I can get that working too since I might be able to do that first.

That could be easier for any current users or forks to migrate to.