Closed 0x4007 closed 11 months ago
/start
Deadline | Sun, 10 Sep 2023 21:56:35 UTC |
Registered Wallet | 0x3623338046b101ecEc741De9C3594CC2176f39E5 |
/wallet 0x0000...0000
if you want to update your registered payment wallet address @user.@seprintour curious to know your input on the proposed solution. I didn't do research on this and I am not confident that it is the best approach.
rfc @ubiquity/software-development
I wonder if the bot should automatically resets non-admin's label update.
@seprintour I think we should figure out a good strategy before you start coding.
@seprintour I think we should figure out a good strategy before you start coding.
Yea, not coding yet.. Trying to hotfix the forum issue.
Sent a DM
admin
or billing_manager
organization rolesI haven't found @seprintour to be in any of them.
- Could somebody explain to me how is this possible? There is a special condition that checks who can generate permits when issue is closed:
- repository collaborators
- organization members
- users with
admin
orbilling_manager
organization rolesI haven't found @seprintour to be in any of them.
- @seprintour How come that you can edit labels if you don't have any permissions in the repo or org?
I temporarily added @seprintour as a collaborator on that specific repo to push some hotfixes to the CI on development
directly.
Although collaborators are more trusted positions, projects should still be able to set fine grained permissions i.e. like our /allow
command; but that as we see can be easily circumvented.
Do you have any updates @seprintour? If you would like to release the bounty back to the DevPool, please comment /stop
Last activity time: Mon Sep 11 2023 18:41:07 GMT+0000 (Coordinated Universal Time)
Do you have any updates @seprintour? If you would like to release the bounty back to the DevPool, please comment
/stop
Last activity time: Mon Sep 11 2023 18:41:07 GMT+0000 (Coordinated Universal Time)
📌
@pavlovcik I am going to do some research and write it down here as it comes to mind
Label Edited
and once this label is on an issue.. It's disabled from permit generation pending someone with enough permission to get rid of the label or maybe we can create a command that can only be called by an admin
or billing_manager
.Once the bot sees that the issue once has the label and it was removed by someone who's not one of these two roles.. It rejects permit generation any way.
rfc @rndquu
@pavlovcik
@pavlovcik I am going to do some research and write it down here as it comes to mind
- I think adding a label that means a label was edited is a possible solution, let's assume we have a label
Label Edited
and once this label is on an issue.. It's disabled from permit generation pending someone with enough permission to get rid of the label or maybe we can create a command that can only be called by anadmin
orbilling_manager
.Once the bot sees that the issue once has the label and it was removed by someone who's not one of these two roles.. It rejects permit generation any way.
rfc @rndquu
Can't we simply reject editing labels for all roles unless the role is:
admin
billing_manager
/allow
command (we don't have this permission right now)?
Can't we simply reject editing labels for all roles unless the role is:
We can't really directly intercept the Github UI actions and reject it, all we can do is create a reaction after the webhook is triggered that would permission-gate the action that is forbidden for the bot.
Can't we simply reject editing labels for all roles unless the role is:
We can't really directly intercept the Github UI actions and reject it, all we can do is create a reaction after the webhook is triggered that would permission-gate the action that is forbidden for the bot.
I still don't understand what exactly you edited here
As far as I understand here is what happened:
Priority: 3 (High)
label from the issue and added it againPriority: 3 (High)
label (because you don't have "set priority" permission) which you added and added it backCould you describe what label exactly did you edit and what was the exact flow?
Could you describe what label exactly did you edit and what was the exact flow?
I was already a collaborator
Priority: 2 (High)
became Priority: 3 (High)
, so it could not generate a permitPriority: 2 (High)
with the newly generated Priority: 3 (High)
label using the Github UI, it rejected it (expected behavior from the /allow
feature)Priority: 3 (High)
and quickly renamed the outdated Priority: 2 (High)
to Priority: 3 (High)
This is possible only if you're a member or collaborator, so this is not that big of an issue I guess. Normally we should be able to trust members.
This is possible only if you're a member or collaborator, so this is not that big of an issue I guess. Normally we should be able to trust members.
Same comment I made when the issue was raised, it's not really a vulnerability but if its a feature needed here.. Only way to go around it is a reaction once an unauthorized collaborator edits a label
Although collaborators are more trusted positions, projects should still be able to set fine grained permissions
https://github.com/ubiquity/ubiquibot/issues/743#issuecomment-1718129967
This doesn't seem like a very elegant solution, but it is fairly secure:
All future permits on that repository will throw an error explaining the situation, ideally explaining that the collaborator changed from X to Y label (only concerning pricing related labels defined in our config) and require some type of manual approval (i.e. command invocation) from an admin. /allow-edit-labels @seprintour true
In this scenario, if it is still not authorized, then the admin should change the label back from Y to X and the bot should re-allow permits to be generated.
This doesn't seem like a very elegant solution, but it is fairly secure:
Elegant solutions will be to directly intercept this event but unfortunately we can't do that, is there any other way we can do this that doesn't involve attaching a label that the bot acts on?
Rfc
Well I was thinking that the bot will check the last updated time of a dependent label and if it was between the time the issue was started and completed. This implies that one of the dependent labels was tampered with while a task was being worked on.
Example:
Priority: 4 (Urgent)
and Time: <1 Hour
updated
column for Priority: 4 (Urgent)
(or can we just query this directly from the GitHub API?)Priority: 4 (Urgent)
label was last modified, by an unauthorized contributor, on 2 October. Priority: 4 (Urgent)
being modified after the task was started, the bot refuses to generate the permit and explains the situation.I just asked ChatGPT and we might be able to leverage the audit log.
But I have a feeling we will need to manually track these things with our bot and the database.
The Priority: 4 (Urgent) label was last modified, by an unauthorized contributor, on 2 October.
We can see when last a label was edited but it doesn't keep track of who edited.. Perhaps we can save that to the db when the event is triggered and do the check when the issue is closed
Unauthorized label changes table
- id, int8
- created, timestamptz
- updated, timestamptz
- user id, int8
- label from, string
- label to, string
- approved, bool
I think there's supposed to be a field for repository
3. create
/approve
command (i feel like this part of the proposal can be refined... rfc!)
Using this command (callable by admin) would just approve the change for the specific repo and enable all permits been held by the recent change to be generated
Should we limit the approve command to just admin? and what if the admin want to decline the request and roll back.
Maybe we can make it (/labelEdit approve
and /labelEdit decline
) or something.. rfc @pavlovcik
We can also pass the id of the change like this /labelEdit approve 24
so i can approve a label change even before a permit generation fail
Default state is declined
so I don't see the need to specify state if it's a one bit operation.
Admin
and billing_manager
are both responsible for finances by default.
Also for the access control we should let those with delegated access change the types of labels. See point 4 under Notes
We can also pass the id of the change like this /labelEdit approve 24
Normal users aren't meant to have access to the database so I don't see how they can find out the id of the change
Normal users aren't meant to have access to the database so I don't see how they can find out the id of the change
I'd still leave the id, because this event occurs completely outside and issue or a comment.. It cannot just send a random comment anywhere and it cannot reply.
So to work without the ID, it needs to have an issue where it failed to generate a permit, and perhaps among the reason for failure.. I can attach the id
of the change that triggered the failure
Or we can just search for changes by label. using the label on the issue that was just closed.. would be a better approach
Just needs to check at permit generation time inside of the issue for any unauthorized changes.
When an issue is closed as complete inside of that closed event context we have the labels and the issue.
From there we check the labels and see if any were tampered with.
@pavlovcik check this out
https://github.com/Seprintour-Test/test/issues/43#issuecomment-1734242337
Does the error message work?
Ready for review @pavlovcik
0x36233380...2176f39E5
The bot clearly states that the assignee does not have access to set the
Priority
label, thus funding the bounty. However, it appears that they were able to edit an invalid label that was already on the issue, and rename it to a valid label to generate a payment permit.Proposed Solution
If anybody but an
admin
orbilling_manager
updates pricing related labels, then anadmin
orbilling_manager
must manually re-allow automatic payment permit generation. This state should be saved in our database.The bot will check the last updated time of a dependent (pricing related) label and if it was between the time the issue was started and completed. This implies that one of the dependent labels was tampered with while a task was being worked on.
Example:
Priority: 4 (Urgent)
andTime: <1 Hour
updated
column forPriority: 4 (Urgent)
Priority: 4 (Urgent)
label was last modified, by an unauthorized contributor, on 2 October.Priority: 4 (Urgent)
being modified after the task was started, the bot refuses to generate the permit and explains the situation.Notes
admin
orbilling_manager
should change the label back from Y to X and the bot should re-allow permits to be generated./approve
foradmins
, andbilling_managers
which will "approve" in the database see below./allow set-priority @seprintour true
and if seprintour changed the priority label, it should be approved implicitly.Unauthorized label changes table
Originally posted by @ubiquibot[bot] in https://github.com/ubiquity/telegram-ubiquibot/issues/4#issuecomment-1710631705
Task checklist
/approve
command (i feel like this part of the proposal can be refined... rfc!)