Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#10911 closed defect (fixed)

CommitTicketUpdater makes changes on tickets on behalf of users without checking if they have sufficient permissions

Reported by: nikolay@… Owned by: Remy Blank
Priority: normal Milestone: 1.0.1
Component: ticket system Version: 1.0
Severity: major Keywords: updater
Cc: Branch:
Release Notes:

CommitTicketUpdater checks for TICKET_APPEND when the refs command is provided.

API Changes:
Internal Changes:

Description

We have a setup, where committers can view only their own tickets.

When I reference a ticket, to which I don't have permissions to even see, CommitTicketUpdater still posts a comment on this ticket with my username and I get the notification e-mail, exposing the ticket summary and description.

I expected that if the commit_ticket_update_check_perms option is set, I won't be able to post comments to this ticket and that I won't be able to see the ticket summary and description.

I think we should check if the user has TICKET_APPEND permission before updating the ticket on their behalf.

Attachments (2)

respect-ticket-permissions-in-updater.diff (1.8 KB ) - added by Nikolay Bachiyski <nikolay@…> 8 years ago.
10911-update-perms-r11496.patch (2.3 KB ) - added by Remy Blank 8 years ago.
Notify outside of transaction.

Download all attachments as: .zip

Change History (10)

comment:1 by Remy Blank, 8 years ago

Component: generalticket system
Milestone: 1.0.1
Owner: set to Remy Blank
Status: newassigned

That makes sense, yes. Can you please provide a patch that adds this check in the loop of _update_tickets()?

by Nikolay Bachiyski <nikolay@…>, 8 years ago

comment:2 by Nikolay Bachiyski <nikolay@…>, 8 years ago

Hey rblank, here is the patch.

In order to have different checks in different commands, I moved the permission checks there and now _update_tickets() just make sure all the commands return True, instead of checking the permissions itself.

comment:3 by Remy Blank, 8 years ago

Thanks for the patch!

comment:4 by Ryan J Ollos <ryan.j.ollos@…>, 8 years ago

Perhaps out of scope for this ticket, but I'm thinking it might be useful to log, perhaps at the debug level, the specific causes of a cmd returning False. For example: "%s doesn't have permission to add comments to Ticket #(%s)" % (changeset.author, ticket.id).

by Remy Blank, 8 years ago

Notify outside of transaction.

comment:5 by Remy Blank, 8 years ago

Can you please test 10911-update-perms-r11496.patch? It has two changes from your initial patch:

  • The notification email must be sent outside of the transaction. You had moved it into the transaction, but this is wrong when the transaction fails.
  • Commands returning None don't prevent the change, they must explicitly return False. This should make the change backward-compatible for people who extended the component and added commands.

comment:6 by Remy Blank, 8 years ago

Keywords: updater, → updater
Resolution: fixed
Status: assignedclosed

Modified patch applied in [11531]. The patch above was updating the ticket if all commands succeeded. This was wrong, and the final patch updates the ticket if at least one command succeeds. The refs command is now gated by TICKET_APPEND.

comment:7 by anonymous, 8 years ago

Thanks for committing that rblank, your changes make total sense.

comment:8 by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.