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

Kevin Falcone falcone at bestpractical.com
Fri Jun 29 14:46:13 EDT 2012


The branch, 4.0/transactionbatch-currentuser-leak has been created
        at  c656be23e06a8cdc6f411673873a47ac878f0a97 (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 cef050d4768ef746685810cf3df9ecbf32638449
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..b46f176 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 does a full 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'} = delete $args{'TicketObj'}->{'_TransactionBatch'};
+        }
     }
     else {
         $self->{'TicketObj'} = RT::Ticket->new( $self->CurrentUser );

commit c656be23e06a8cdc6f411673873a47ac878f0a97
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 b46f176..7559839 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'};
-
 }
 
 
@@ -293,6 +289,9 @@ sub _SetupSourceObjects {
         $self->{'TicketObj'}->Load( $args{'TicketObj'}->Id );
         if ( $args{'TicketObj'}->TransactionBatch ) {
             $self->{'TicketObj'}->{'_TransactionBatch'} = delete $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);
         }
     }
     else {
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 306ad4a..0a4bb24 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
 
@@ -3310,6 +3332,7 @@ batch is applied when object is destroyed, but in some cases it's too late.
 sub ApplyTransactionBatch {
     my $self = shift;
 
+    return if $self->RanTransactionBatch;
     my $batch = $self->TransactionBatch;
     return unless $batch && @$batch;
 
@@ -3320,6 +3343,8 @@ sub ApplyTransactionBatch {
 
 sub _ApplyTransactionBatch {
     my $self = shift;
+
+    $self->RanTransactionBatch(1);
     my $batch = $self->TransactionBatch;
 
     my %seen;
@@ -3367,6 +3392,7 @@ sub DESTROY {
         return;
     }
 
+    return if $self->RanTransactionBatch;
     my $batch = $self->TransactionBatch;
     return unless $batch && @$batch;
 

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


More information about the Rt-commit mailing list