[Rt-commit] rt branch, 4.0/transactionbatch-currentuser-leak, created. rt-4.0.6-219-g49841b8

Kevin Falcone falcone at bestpractical.com
Fri Jun 29 20:24:48 EDT 2012


The branch, 4.0/transactionbatch-currentuser-leak has been created
        at  49841b89b621e77b898012efcf54c21b80fcf1bb (commit)

- Log -----------------------------------------------------------------
commit c5de7b01683ce25e9bc3835859637ab127f0ffb6
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Thu Jun 28 16:56:30 2012 -0400

    We're not cleaning up all CurrentUsers in TransactionBatch
    
    After the sequence of commits around 771a9575a2 we started storing the
    CurrentUser of the ticket/transaction in a transaction batch scrip and
    then restoring it.  The rationale is in 771a9575a2.  Unfortunately, we
    only catch the top-level CurrentUser.  If there are objects cached in
    the Ticket or Transaction, they don't get turned into RT_System objects.
    
    This was found because the SLA extension calls
    $self->TicketObj->QueueObj->CustomFields and the Unprivileged user
    emailing RT doesn't have permission to see the SLA Custom Field.

diff --git a/t/web/transaction_batch.t b/t/web/transaction_batch.t
index ae04e1f..12d01fb 100644
--- a/t/web/transaction_batch.t
+++ b/t/web/transaction_batch.t
@@ -12,7 +12,14 @@ my ($val, $msg) =$s1->Create( Queue => $q->Id,
              ScripAction       => 'User Defined',
              CustomIsApplicableCode => 'return ($self->TransactionObj->Field||"") eq "TimeEstimated"',
              CustomPrepareCode => 'return 1',
-             CustomCommitCode  => '$self->TicketObj->SetPriority($self->TicketObj->Priority + 2); return 1;',
+             CustomCommitCode  => '
+if ( $self->TicketObj->CurrentUser->Name ne "RT_System" ) { 
+    warn "Ticket obj has incorrect CurrentUser (should be RT_System) ".$self->TicketObj->CurrentUser->Name
+}
+if ( $self->TicketObj->QueueObj->CurrentUser->Name ne "RT_System" ) { 
+    warn "Queue obj has incorrect CurrentUser (should be RT_System) ".$self->TicketObj->QueueObj->CurrentUser->Name
+}
+$self->TicketObj->SetPriority($self->TicketObj->Priority + 2); return 1;',
              Template          => 'Blank',
              Stage             => 'TransactionBatch',
     );

commit 1e280ff2dc00b99540818f3e5299f582bd1f0c79
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Thu Jun 28 18:57:46 2012 -0400

    Start forcibly loading the Ticket object we use in Scrips
    
    We've done several things with this in the past.
    Take an alias to the TicketObj (which means changing CurrentUser leaks).
    Clone the object, which means that TransactionBatch Scrips run twice.
    
    This code forcibly loads a clean TicketObj for the use of the Scrip
    system.  It then removes the _TransactionBatch cache from tickets being
    processed from ApplyTransactionBatch.
    
    It does not re-process the source objects on Commit, so likely we need a
    bit more poking to make sure no code is just invoking Commit with
    non-superuser Objects.
    
    At the end of the Commit, it removes the _TransactionBatch objects from
    the copied ticket to ensure that we don't run TransactionBatch twice.
    
    We really need a way to say "Run transaction batch, for these objects"
    and not copy around an arrayref of Transaction Objects as a boolean
    flag.

diff --git a/lib/RT/Scrips.pm b/lib/RT/Scrips.pm
index 13a4b7d..8e21ef9 100644
--- a/lib/RT/Scrips.pm
+++ b/lib/RT/Scrips.pm
@@ -178,16 +178,6 @@ Commit all of this object's prepared scrips
 sub Commit {
     my $self = shift;
 
-    # RT::Scrips->_SetupSourceObjects will clobber
-    # the CurrentUser, but we need to keep this ticket
-    # so that the _TransactionBatch cache is maintained
-    # and doesn't run twice.  sigh.
-    $self->_StashCurrentUser( TicketObj => $self->{TicketObj} ) if $self->{TicketObj};
-
-    #We're really going to need a non-acled ticket for the scrips to work
-    $self->_SetupSourceObjects( TicketObj      => $self->{'TicketObj'},
-                                TransactionObj => $self->{'TransactionObj'} );
-    
     foreach my $scrip (@{$self->Prepared}) {
         $RT::Logger->debug(
             "Committing scrip #". $scrip->id
@@ -199,8 +189,10 @@ sub Commit {
                         TransactionObj => $self->{'TransactionObj'} );
     }
 
-    # Apply the bandaid.
-    $self->_RestoreCurrentUser( TicketObj => $self->{TicketObj} ) if $self->{TicketObj};
+    # make sure our cached TicketObj doesn't trigger a DESTROY and
+    # run all the TransactionBatch code again.
+    delete $self->{'TicketObj'}->{'_TransactionBatch'};
+
 }
 
 
@@ -221,12 +213,6 @@ sub Prepare {
                  Type           => undef,
                  @_ );
 
-    # RT::Scrips->_SetupSourceObjects will clobber
-    # the CurrentUser, but we need to keep this ticket
-    # so that the _TransactionBatch cache is maintained
-    # and doesn't run twice.  sigh.
-    $self->_StashCurrentUser( TicketObj => $args{TicketObj} ) if $args{TicketObj};
-
     #We're really going to need a non-acled ticket for the scrips to work
     $self->_SetupSourceObjects( TicketObj      => $args{'TicketObj'},
                                 Ticket         => $args{'Ticket'},
@@ -259,10 +245,6 @@ sub Prepare {
 
     }
 
-    # Apply the bandaid.
-    $self->_RestoreCurrentUser( TicketObj => $args{TicketObj} ) if $args{TicketObj};
-
-
     return (@{$self->Prepared});
 
 };
@@ -279,40 +261,6 @@ sub Prepared {
     return ($self->{'prepared_scrips'} || []);
 }
 
-=head2 _StashCurrentUser TicketObj => RT::Ticket
-
-Saves aside the current user of the original ticket that was passed to these scrips.
-This is used to make sure that we don't accidentally leak the RT_System current user
-back to the calling code.
-
-=cut
-
-sub _StashCurrentUser {
-    my $self = shift;
-    my %args = @_;
-
-    $self->{_TicketCurrentUser} = $args{TicketObj}->CurrentUser;
-}
-
-=head2 _RestoreCurrentUser TicketObj => RT::Ticket
-
-Uses the current user saved by _StashCurrentUser to reset a Ticket object
-back to the caller's current user and avoid leaking an RT_System ticket to
-calling code.
-
-=cut
-
-sub _RestoreCurrentUser {
-    my $self = shift;
-    my %args = @_;
-    unless ( $self->{_TicketCurrentUser} ) {
-        RT->Logger->debug("Called _RestoreCurrentUser without a stashed current user object");
-        return;
-    }
-    $args{TicketObj}->CurrentUser($self->{_TicketCurrentUser});
-
-}
-
 =head2  _SetupSourceObjects { TicketObj , Ticket, Transaction, TransactionObj }
 
 Setup a ticket and transaction for this Scrip collection to work with as it runs through the 
@@ -334,14 +282,18 @@ sub _SetupSourceObjects {
             @_ );
 
 
-    if ( $self->{'TicketObj'} = $args{'TicketObj'} ) {
-        # This clobbers the passed in TicketObj by turning it into one
-        # whose current user is RT_System.  Anywhere in the Web UI
-        # currently calling into this is thus susceptable to a privilege
-        # leak; the only current call site is ->Apply, which bandaids
-        # over the top of this by re-asserting the CurrentUser
-        # afterwards.
-        $self->{'TicketObj'}->CurrentUser( $self->CurrentUser );
+    if ( $args{'TicketObj'} ) {
+        # This loads a clean copy of the Ticket object to ensure that we
+        # don't accidentally escalate the privileges of the passed in
+        # ticket (this function can be invoked from the UI).
+        # We also need to be careful to copy the TransactionBatch
+        # data to the new ticket and ensure that TransactionBatch will
+        # not be rerun on either ticket.
+        $self->{'TicketObj'} = RT::Ticket->new( $self->CurrentUser );
+        $self->{'TicketObj'}->Load( $args{'TicketObj'}->Id );
+        if ( $args{'TicketObj'}->TransactionBatch ) {
+            $self->{'TicketObj'}->{'_TransactionBatch'} = $args{'TicketObj'}->{'_TransactionBatch'};
+        }
     }
     else {
         $self->{'TicketObj'} = RT::Ticket->new( $self->CurrentUser );

commit 8c47a12253ef680d26a8f9d433695e3611a704b0
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Fri Jun 29 14:39:54 2012 -0400

    Implement a TransactionBatch Guard function
    
    RanTransactionBatch indicates whether or not this ticket needs to
    execute TransactionBatch scrips.  This is better than needing to delete
    the _TransactionBatch variable to clear out transactions that were
    queued.

diff --git a/lib/RT/Scrips.pm b/lib/RT/Scrips.pm
index 8e21ef9..fa33f7e 100644
--- a/lib/RT/Scrips.pm
+++ b/lib/RT/Scrips.pm
@@ -189,10 +189,6 @@ sub Commit {
                         TransactionObj => $self->{'TransactionObj'} );
     }
 
-    # make sure our cached TicketObj doesn't trigger a DESTROY and
-    # run all the TransactionBatch code again.
-    delete $self->{'TicketObj'}->{'_TransactionBatch'};
-
 }
 
 
@@ -286,12 +282,16 @@ sub _SetupSourceObjects {
         # This loads a clean copy of the Ticket object to ensure that we
         # don't accidentally escalate the privileges of the passed in
         # ticket (this function can be invoked from the UI).
-        # We also need to be careful to copy the TransactionBatch
-        # data to the new ticket and ensure that TransactionBatch will
-        # not be rerun on either ticket.
+        # We copy the TransactionBatch transactions so that Scrips
+        # running against the new Ticket will have access to them. We
+        # use RanTransactionBatch to guard against running
+        # TransactionBatch Scrips more than once.
         $self->{'TicketObj'} = RT::Ticket->new( $self->CurrentUser );
         $self->{'TicketObj'}->Load( $args{'TicketObj'}->Id );
         if ( $args{'TicketObj'}->TransactionBatch ) {
+            # try to ensure that we won't infinite loop if something dies, triggering DESTROY while 
+            # we have the _TransactionBatch objects;
+            $self->{'TicketObj'}->RanTransactionBatch(1);
             $self->{'TicketObj'}->{'_TransactionBatch'} = $args{'TicketObj'}->{'_TransactionBatch'};
         }
     }
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 306ad4a..a7933a2 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3284,6 +3284,28 @@ sub SeenUpTo {
     return $txns->First;
 }
 
+=head2 RanTransactionBatch
+
+Acts as a guard around running TransactionBatch scrips.
+
+Should be false until you enter the code that runs TransactionBatch scrips
+
+Accepts an optional argument to indicate that TransactionBatch Scrips should no longer be run on this object.
+
+=cut
+
+sub RanTransactionBatch {
+    my $self = shift;
+    my $val = shift;
+
+    if ( defined $val ) {
+        return $self->{_RanTransactionBatch} = $val;
+    } else {
+        return $self->{_RanTransactionBatch};
+    }
+
+}
+
 
 =head2 TransactionBatch
 
@@ -3320,6 +3342,9 @@ sub ApplyTransactionBatch {
 
 sub _ApplyTransactionBatch {
     my $self = shift;
+
+    return if $self->RanTransactionBatch;
+    $self->RanTransactionBatch(1);
     my $batch = $self->TransactionBatch;
 
     my %seen;

commit 49841b89b621e77b898012efcf54c21b80fcf1bb
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Fri Jun 29 14:47:57 2012 -0400

    Remove some repeated code.
    
    DESTROY was doing very similar code to ApplyTransactionBatch
    so just call ApplyTransactionBatch rather than duplicating code and then
    calling _ApplyTransactionBatch.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index a7933a2..48fc9e4 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3392,10 +3392,7 @@ sub DESTROY {
         return;
     }
 
-    my $batch = $self->TransactionBatch;
-    return unless $batch && @$batch;
-
-    return $self->_ApplyTransactionBatch;
+    return $self->ApplyTransactionBatch;
 }
 
 

-----------------------------------------------------------------------


More information about the Rt-commit mailing list