[Rt-commit] rt branch, 4.0.8-releng, updated. rt-4.0.8rc2-38-g048ac13

Kevin Falcone falcone at bestpractical.com
Thu Oct 25 18:37:40 EDT 2012


The branch, 4.0.8-releng has been updated
       via  048ac133c6aa7af528a84dc1aece81042ed79c4f (commit)
       via  9b5d6534dc158b4449e6c6587fde19f51ff02721 (commit)
       via  73c3c162183b756bb33378f3def60033de776836 (commit)
       via  f350d859467e04d391491c0129eca052a90059bc (commit)
       via  0b3d6861f99269a40dce601263bba1145e61bd1d (commit)
       via  496e93fe0360954c7d36cecbc2219791752c58c7 (commit)
       via  e6c6ae922252e36b510264472820ea8df72e4b7e (commit)
       via  dba758b4353ed506e1d7527a0aad22b0124c8786 (commit)
       via  42623930c7a4eb524dcd5d702912faae418acab0 (commit)
       via  e0c1ec5163da68ddaf477010b6219d41a7be40bc (commit)
       via  0bd9510437d7adbeb330313eaebc6103eb917026 (commit)
       via  c964114ca495830f70ec8789d4d9de878fbf84b2 (commit)
       via  4cda0e47df5431c59d2a03f61917ab35c0cdbfaf (commit)
       via  ecbdf95e4d4bebf71152fdfa18b11524d364c9a4 (commit)
       via  72ee4047c9855b3c8e1a7e2a0e7635c3a3ed230b (commit)
       via  7e3782deb299becc9221cc83ee2d9cab09721d74 (commit)
       via  5d569351cabe49819641fec936bdcf0e87580922 (commit)
       via  e8d0554663854b857025e7211bd0cc51d09e6223 (commit)
       via  03ff7a5c4492815c749d8245546b4be35cb8b3e1 (commit)
       via  cbdc50385da30d56b996ad33a7e08117f98dc64c (commit)
       via  666d7a4f14e1065a520062b89cccb34932fa4567 (commit)
       via  4c40dd6b6523977abb8ac77276bbeff45df73027 (commit)
       via  e3c2880e6526f9c78079df2097483fea30ba910e (commit)
       via  e50f6496cfd15a3b9e1e24b67c9b1b721107d6a5 (commit)
       via  08555f51ff8a4105aaaf34725d6b8df356203a86 (commit)
       via  8f242072bb196e155ec9a5fbbf06bc8a2d3c2813 (commit)
       via  1f213d6989e646e0163c901b0567ba1c94a311a8 (commit)
       via  8ce033b38ced865622a2823aa266ae9bbc251811 (commit)
       via  cd0235bda39972fabecd837d982dd9f1619296c4 (commit)
       via  b32d9f5ffe7b60816942c91a6b7cb62992910369 (commit)
       via  10e4a18a61b07b505bae8e9ffff65708a97fc659 (commit)
       via  aeaeb9cf58240603bb8a7d6d951518d32e8512a6 (commit)
       via  df1c01d5563883abfcde5c5ed1675d6617256ebe (commit)
       via  46f44f7c2147f674ab0b54e9a80d70076558330a (commit)
      from  e4916c6fd66a4812f47531b865bffca6398bff5d (commit)

Summary of changes:
 lib/RT/Action/SendEmail.pm                  |  54 +++++--------
 lib/RT/Article.pm                           |   2 +-
 lib/RT/Attachment.pm                        |  49 ++++++++----
 lib/RT/Crypt/GnuPG.pm                       |  14 +++-
 lib/RT/Interface/Email.pm                   |  52 +++++++++----
 lib/RT/Interface/Email/Auth/GnuPG.pm        |   3 +-
 lib/RT/Interface/Web.pm                     | 117 +++++++++++++++++++++++-----
 lib/RT/Queue.pm                             |  40 ++++++++--
 lib/RT/Template.pm                          |   1 +
 lib/RT/User.pm                              |   1 +
 share/html/Admin/Queues/Modify.html         |   6 +-
 share/html/Admin/Users/GnuPG.html           |  15 +++-
 share/html/Elements/CSRF                    |   6 +-
 share/html/Elements/GnuPG/SignEncryptWidget |  10 ++-
 share/html/Elements/Login                   |   2 +
 share/html/Elements/LoginRedirectWarning    |  20 +++++
 share/html/NoAuth/css/base/login.css        |   8 ++
 t/web/crypt-gnupg.t                         |  27 +++++--
 t/web/ticket_forward.t                      |   6 +-
 19 files changed, 314 insertions(+), 119 deletions(-)
 create mode 100644 share/html/Elements/LoginRedirectWarning

- Log -----------------------------------------------------------------
commit 46f44f7c2147f674ab0b54e9a80d70076558330a
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Aug 27 19:59:54 2012 -0400

    Fix a typo, preventing emails from setting internal encryption header
    
    X-RT-Incoming-Encryption, along with a number of other headers, is
    removed from all parts of incoming emails because they are used to
    annotate GPG-related properties of the message.  Fix a typo, which
    allowed the properly-spelled header to pass through under specific
    circumstances.

diff --git a/lib/RT/Interface/Email/Auth/GnuPG.pm b/lib/RT/Interface/Email/Auth/GnuPG.pm
index e508908..5fe8759 100644
--- a/lib/RT/Interface/Email/Auth/GnuPG.pm
+++ b/lib/RT/Interface/Email/Auth/GnuPG.pm
@@ -77,7 +77,7 @@ sub GetCurrentUser {
 
     foreach my $p ( $args{'Message'}->parts_DFS ) {
         $p->head->delete($_) for qw(
-            X-RT-GnuPG-Status X-RT-Incoming-Encrypton
+            X-RT-GnuPG-Status X-RT-Incoming-Encryption
             X-RT-Incoming-Signature X-RT-Privacy
         );
     }

commit df1c01d5563883abfcde5c5ed1675d6617256ebe
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Aug 27 21:24:25 2012 -0400

    Remove internal signing and encryption hints from incoming mail
    
    RT uses the existance of the X-RT-Sign and X-RT-Encrypt headers to
    decide if the outgoing mails it sends as a result of the transaction
    should be signed and/or encrypted.  It standardly inserts these headers
    based on checkboxes visable in the web UI; however, it fails to remove
    them from incoming mails, allowing external users to create nearly
    arbitrary messages signed by the queue's key.

diff --git a/lib/RT/Interface/Email/Auth/GnuPG.pm b/lib/RT/Interface/Email/Auth/GnuPG.pm
index 5fe8759..87a523d 100644
--- a/lib/RT/Interface/Email/Auth/GnuPG.pm
+++ b/lib/RT/Interface/Email/Auth/GnuPG.pm
@@ -79,6 +79,7 @@ sub GetCurrentUser {
         $p->head->delete($_) for qw(
             X-RT-GnuPG-Status X-RT-Incoming-Encryption
             X-RT-Incoming-Signature X-RT-Privacy
+            X-RT-Sign X-RT-Encrypt
         );
     }
 

commit aeaeb9cf58240603bb8a7d6d951518d32e8512a6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Aug 30 17:16:41 2012 -0400

    Restrict users to only signing with queue or their own personal keys
    
    While the UI only presents two options for signing, the underlying form
    accepted any key ID or email address, and dutifully signed the mail with
    the appropriate secret key.  Restrict the accepted values of SignUsing
    to the two values that we displayed.

diff --git a/share/html/Elements/GnuPG/SignEncryptWidget b/share/html/Elements/GnuPG/SignEncryptWidget
index 0ae0f84..2f3f103 100644
--- a/share/html/Elements/GnuPG/SignEncryptWidget
+++ b/share/html/Elements/GnuPG/SignEncryptWidget
@@ -129,12 +129,16 @@ if ( $self->{'Sign'} ) {
     $QueueObj ||= $TicketObj->QueueObj
         if $TicketObj;
 
-    my $address = $self->{'SignUsing'};
-    $address ||= ($self->{'UpdateType'} && $self->{'UpdateType'} eq "private")
+    my $private = $session{'CurrentUser'}->UserObj->PrivateKey || '';
+    my $queue = ($self->{'UpdateType'} && $self->{'UpdateType'} eq "private")
         ? ( $QueueObj->CommentAddress || RT->Config->Get('CommentAddress') )
         : ( $QueueObj->CorrespondAddress || RT->Config->Get('CorrespondAddress') );
 
-    unless ( RT::Crypt::GnuPG::DrySign( $address ) ) {
+    my $address = $self->{'SignUsing'} || $queue;
+    if ($address ne $private and $address ne $queue) {
+        push @{ $self->{'GnuPGCanNotSignAs'} ||= [] }, $address;
+        $checks_failure = 1;
+    } elsif ( not RT::Crypt::GnuPG::DrySign( $address ) ) {
         push @{ $self->{'GnuPGCanNotSignAs'} ||= [] }, $address;
         $checks_failure = 1;
     } else {

commit 10e4a18a61b07b505bae8e9ffff65708a97fc659
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Aug 30 18:42:55 2012 -0400

    Don't propose any secret keys to users with no email address
    
    GetKeysForSigning, when called with 'force', returns all keys if no
    email address is provided.  This caused users with no email address to
    have all secret keys offered to them.  Remove the 'force' to ensure that
    empty email address are shown an empty list.

diff --git a/share/html/Admin/Users/GnuPG.html b/share/html/Admin/Users/GnuPG.html
index 90408e4..8925c9f 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -64,7 +64,7 @@
 <& /Widgets/Form/Select,
     Name         => 'PrivateKey',
     Description  => loc('Private Key'),
-    Values       => [ map $_->{'Key'}, @{ $keys_meta{'info'} } ],
+    Values       => \@potential_keys,
     CurrentValue => $UserObj->PrivateKey,
     DefaultLabel => loc('No private key'),
 &>
@@ -91,7 +91,8 @@ unless ( $UserObj->id ) {
 $id = $ARGS{'id'} = $UserObj->id;
 
 my $email = $UserObj->EmailAddress;
-my %keys_meta = RT::Crypt::GnuPG::GetKeysForSigning( $email, 'force' );
+my %keys_meta = RT::Crypt::GnuPG::GetKeysForSigning( $email );
+my @potential_keys = map $_->{'Key'}, @{ $keys_meta{'info'} || [] };
 
 $ARGS{'PrivateKey'} = $m->comp('/Widgets/Form/Select:Process',
     Name      => 'PrivateKey',

commit b32d9f5ffe7b60816942c91a6b7cb62992910369
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Aug 30 22:42:22 2012 -0400

    Explicitly restrict private keys to ones offered

diff --git a/share/html/Admin/Users/GnuPG.html b/share/html/Admin/Users/GnuPG.html
index 8925c9f..373e314 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -101,8 +101,12 @@ $ARGS{'PrivateKey'} = $m->comp('/Widgets/Form/Select:Process',
 );
 
 if ( $Update ) {
-    my ($status, $msg) = $UserObj->SetPrivateKey( $ARGS{'PrivateKey'} );
-    push @results, $msg;
+    if (not $ARGS{'PrivateKey'} or grep {$_ eq $ARGS{'PrivateKey'}} @potential_keys) {
+        my ($status, $msg) = $UserObj->SetPrivateKey( $ARGS{'PrivateKey'} );
+        push @results, $msg;
+    } else {
+        push @results, loc("Invalid key [_1] for address '[_2]'", $ARGS{'PrivateKey'}, $email);
+    }
 }
 
 my $title = loc("[_1]'s GnuPG keys",$UserObj->Name);

commit cd0235bda39972fabecd837d982dd9f1619296c4
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Aug 30 22:46:32 2012 -0400

    Avoid spurious update and warning messages on key update
    
    This avoids an ominous-looking warning about failure to remove the key,
    when no key was previously set but no key was newly selected.

diff --git a/share/html/Admin/Users/GnuPG.html b/share/html/Admin/Users/GnuPG.html
index 373e314..ee58c44 100644
--- a/share/html/Admin/Users/GnuPG.html
+++ b/share/html/Admin/Users/GnuPG.html
@@ -102,8 +102,10 @@ $ARGS{'PrivateKey'} = $m->comp('/Widgets/Form/Select:Process',
 
 if ( $Update ) {
     if (not $ARGS{'PrivateKey'} or grep {$_ eq $ARGS{'PrivateKey'}} @potential_keys) {
-        my ($status, $msg) = $UserObj->SetPrivateKey( $ARGS{'PrivateKey'} );
-        push @results, $msg;
+        if (($ARGS{'PrivateKey'}||'') ne ($UserObj->PrivateKey||'')) {
+            my ($status, $msg) = $UserObj->SetPrivateKey( $ARGS{'PrivateKey'} );
+            push @results, $msg;
+        }
     } else {
         push @results, loc("Invalid key [_1] for address '[_2]'", $ARGS{'PrivateKey'}, $email);
     }

commit 8ce033b38ced865622a2823aa266ae9bbc251811
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Aug 30 23:58:40 2012 -0400

    Require AdminUser to set PGP private key IDs, not merely ModifySelf
    
    Be explicit that modification of the PrivateKey pseudo-column requires
    admin rights -- namely, AdminUser, not just ModifySelf.  While the page
    that controlls that property is in /Admin/Users/GnuPG.html, accessing
    that page requires only the ShowConfigTab right, and modifying the
    property on yourself required only ModifySelf.  Combined with the
    ability (also granted by ModifySelf) for users to change their own email
    addresses, this opened the possibility for users to claim arbitrary
    private keys as their own, for use in signing.

diff --git a/lib/RT/User.pm b/lib/RT/User.pm
index 56a22cd..72d00f3 100644
--- a/lib/RT/User.pm
+++ b/lib/RT/User.pm
@@ -102,6 +102,7 @@ sub _OverlayAccessible {
           AuthSystem            => { public => 1,  admin => 1 },
           Gecos                 => { public => 1,  admin => 1 },
           PGPKey                => { public => 1,  admin => 1 },
+          PrivateKey            => {               admin => 1 },
 
     }
 }

commit 1f213d6989e646e0163c901b0567ba1c94a311a8
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri Aug 31 00:17:08 2012 -0400

    Ensure that no --arguments can be snuck to GPG commands as arguments
    
    GnuPG::Interface 0.46 adds code to prefix command_args with a "--" if it
    doesn't have one already.  Rather than bump the dependency, explicitly
    add the "--" in the two places that we pass in command_args.

diff --git a/lib/RT/Crypt/GnuPG.pm b/lib/RT/Crypt/GnuPG.pm
index 8df1c1b..77f9a07 100644
--- a/lib/RT/Crypt/GnuPG.pm
+++ b/lib/RT/Crypt/GnuPG.pm
@@ -2120,7 +2120,9 @@ sub GetKeysInfo {
     eval {
         local $SIG{'CHLD'} = 'DEFAULT';
         my $method = $type eq 'private'? 'list_secret_keys': 'list_public_keys';
-        my $pid = safe_run_child { $gnupg->$method( handles => $handles, $email? (command_args => $email) : () ) };
+        my $pid = safe_run_child { $gnupg->$method( handles => $handles, $email
+                                                        ? (command_args => [ "--", $email])
+                                                        : () ) };
         close $handle{'stdin'};
         waitpid $pid, 0;
     };
@@ -2314,7 +2316,7 @@ sub DeleteKey {
         my $pid = safe_run_child { $gnupg->wrap_call(
             handles => $handles,
             commands => ['--delete-secret-and-public-key'],
-            command_args => [$key],
+            command_args => ["--", $key],
         ) };
         close $handle{'stdin'};
         while ( my $str = readline $handle{'status'} ) {

commit 8f242072bb196e155ec9a5fbbf06bc8a2d3c2813
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri Aug 31 21:12:02 2012 -0400

    Refactor shared code controlling if a message will be encrypted or signed

diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm
index 4ae1a8b..c301870 100644
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -106,23 +106,10 @@ sub Commit {
     if (   RT->Config->Get('RecordOutgoingEmail')
         && RT->Config->Get('GnuPG')->{'Enable'} )
     {
-
-        # it's hacky, but we should know if we're going to crypt things
-        my $attachment = $self->TransactionObj->Attachments->First;
-
-        my %crypt;
-        foreach my $argument (qw(Sign Encrypt)) {
-            if ( $attachment
-                && defined $attachment->GetHeader("X-RT-$argument") )
-            {
-                $crypt{$argument} = $attachment->GetHeader("X-RT-$argument");
-            } else {
-                $crypt{$argument} = $self->TicketObj->QueueObj->$argument();
-            }
-        }
-        if ( $crypt{'Sign'} || $crypt{'Encrypt'} ) {
-            $orig_message = $message->dup;
-        }
+        $orig_message = $message->dup if RT::Interface::Email::WillSignEncrypt(
+            Attachment => $self->TransactionObj->Attachments->First,
+            Ticket     => $self->TicketObj,
+        );
     }
 
     my ($ret) = $self->SendMessage($message);
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 9270462..6fec643 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -318,6 +318,24 @@ header field then it's value is used
 
 =cut
 
+sub WillSignEncrypt {
+    my %args = @_;
+    my $attachment = delete $args{Attachment};
+    my $ticket     = delete $args{Ticket};
+
+    for my $argument ( qw(Sign Encrypt) ) {
+        next if defined $args{ $argument };
+
+        if ( $attachment and defined $attachment->GetHeader("X-RT-$argument") ) {
+            $args{$argument} = $attachment->GetHeader("X-RT-$argument");
+        } elsif ( $ticket ) {
+            $args{$argument} = $ticket->QueueObj->$argument();
+        }
+    }
+
+    return wantarray ? %args : ($args{Sign} || $args{Encrypt});
+}
+
 sub SendEmail {
     my (%args) = (
         Entity => undef,
@@ -366,23 +384,12 @@ sub SendEmail {
     }
 
     if ( RT->Config->Get('GnuPG')->{'Enable'} ) {
-        my %crypt;
-
-        my $attachment;
-        $attachment = $TransactionObj->Attachments->First
-            if $TransactionObj;
-
-        foreach my $argument ( qw(Sign Encrypt) ) {
-            next if defined $args{ $argument };
-
-            if ( $attachment && defined $attachment->GetHeader("X-RT-$argument") ) {
-                $crypt{$argument} = $attachment->GetHeader("X-RT-$argument");
-            } elsif ( $TicketObj ) {
-                $crypt{$argument} = $TicketObj->QueueObj->$argument();
-            }
-        }
-
-        my $res = SignEncrypt( %args, %crypt );
+        %args = WillSignEncrypt(
+            %args,
+            Attachment => $TransactionObj ? $TransactionObj->Attachments->First : undef,
+            Ticket     => $TicketObj,
+        );
+        my $res = SignEncrypt( %args );
         return $res unless $res > 0;
     }
 

commit 08555f51ff8a4105aaaf34725d6b8df356203a86
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri Aug 31 21:15:26 2012 -0400

    Refactor RT::Action::SendEmail->Commit to consolidate RecordOutgoingEmail path
    
    Instead of interweaving the logic around the case where
    RecordOutgoingEmail is disabled, resolve that simple case first.

diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm
index c301870..2a7a2e3 100644
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -99,34 +99,31 @@ activated in the config.
 sub Commit {
     my $self = shift;
 
-    $self->DeferDigestRecipients() if RT->Config->Get('RecordOutgoingEmail');
+    return abs $self->SendMessage( $self->TemplateObj->MIMEObj )
+        unless RT->Config->Get('RecordOutgoingEmail');
+
+    $self->DeferDigestRecipients();
     my $message = $self->TemplateObj->MIMEObj;
 
     my $orig_message;
-    if (   RT->Config->Get('RecordOutgoingEmail')
-        && RT->Config->Get('GnuPG')->{'Enable'} )
-    {
-        $orig_message = $message->dup if RT::Interface::Email::WillSignEncrypt(
-            Attachment => $self->TransactionObj->Attachments->First,
-            Ticket     => $self->TicketObj,
-        );
-    }
+    $orig_message = $message->dup if RT::Interface::Email::WillSignEncrypt(
+        Attachment => $self->TransactionObj->Attachments->First,
+        Ticket     => $self->TicketObj,
+    );
 
     my ($ret) = $self->SendMessage($message);
-    if ( $ret > 0 && RT->Config->Get('RecordOutgoingEmail') ) {
-        if ($orig_message) {
-            $message->attach(
-                Type        => 'application/x-rt-original-message',
-                Disposition => 'inline',
-                Data        => $orig_message->as_string,
-            );
-        }
-        $self->RecordOutgoingMailTransaction($message);
-        $self->RecordDeferredRecipients();
-    }
-
+    return abs( $ret ) if $ret <= 0;
 
-    return ( abs $ret );
+    if ($orig_message) {
+        $message->attach(
+            Type        => 'application/x-rt-original-message',
+            Disposition => 'inline',
+            Data        => $orig_message->as_string,
+        );
+    }
+    $self->RecordOutgoingMailTransaction($message);
+    $self->RecordDeferredRecipients();
+    return 1;
 }
 
 =head2 Prepare
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 6fec643..d7655bf 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -323,6 +323,11 @@ sub WillSignEncrypt {
     my $attachment = delete $args{Attachment};
     my $ticket     = delete $args{Ticket};
 
+    if ( not RT->Config->Get('GnuPG')->{'Enable'} ) {
+        $args{Sign} = $args{Encrypt} = 0;
+        return wantarray ? %args : 0;
+    }
+
     for my $argument ( qw(Sign Encrypt) ) {
         next if defined $args{ $argument };
 

commit e50f6496cfd15a3b9e1e24b67c9b1b721107d6a5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri Aug 31 23:25:27 2012 -0400

    When creating tickets via the UI, always set signing/encryption headers

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 99aa7b6..b100a11 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1632,9 +1632,8 @@ sub CreateTicket {
         }
     }
 
-    foreach my $argument (qw(Encrypt Sign)) {
-        $MIMEObj->head->replace( "X-RT-$argument" => $ARGS{$argument} ? 1 : 0 )
-          if defined $ARGS{$argument};
+    for my $argument (qw(Encrypt Sign)) {
+        $MIMEObj->head->replace( "X-RT-$argument" => $ARGS{$argument} ? 1 : 0 );
     }
 
     my %create_args = (

commit e3c2880e6526f9c78079df2097483fea30ba910e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri Aug 31 23:27:10 2012 -0400

    Differentiate "always sign" from "default to signing when composing"
    
    Signing incorporates two related but distinct concepts: first,
    verification that the message was transmitted without corruption
    (integrity), and second, proof that the message was indeed sent by the
    sender (authentication).
    
    When the "Sign by default" queue option is disabled, the only messages
    which are signed are those triggered by an action where the sender has
    explicitly checked the "Sign" option.  Consequently, messages which are
    not generated via the web UI, such as auto-replies, or requestors'
    messages which are forwarded to owners, are not signed.  In this
    configuration, the signing conveys authenticity, but does not provide
    integrity for all messages..
    
    When the "Sign by default" queue option is enabled, all messages default
    to being signed, even ones generated automatically.  Thus, emails from
    the requestor (which may have arbitrary content!) will be signed by the
    queue's key when redistributed to owners and AdminCcs.  In this
    configuration, the signing ensures integrity, but (because signing
    happens automatically) means that no authenticity should be attached to
    messages thus signed.
    
    Because the simple configuration option fundamentally changed the
    meaning of the signature, exchanging integrity for authenticity, with no
    warning, alter it to instead provide an additional option ("Sign
    auto-generated messages") which takes the burden of this change in
    meaning.  Degrade the "Sign by default" option to describing the default
    state of the checkbox on the ticket create or reply page.

diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index d7655bf..38157c2 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -333,8 +333,14 @@ sub WillSignEncrypt {
 
         if ( $attachment and defined $attachment->GetHeader("X-RT-$argument") ) {
             $args{$argument} = $attachment->GetHeader("X-RT-$argument");
-        } elsif ( $ticket ) {
-            $args{$argument} = $ticket->QueueObj->$argument();
+        } elsif ( $ticket and $argument eq "Encrypt" ) {
+            $args{Encrypt} = $ticket->QueueObj->Encrypt();
+        } elsif ( $ticket and $argument eq "Sign" ) {
+            # Note that $queue->Sign is UI-only, and that all
+            # UI-generated messages explicitly set the X-RT-Crypt header
+            # to 0 or 1; thus this path is only taken for messages
+            # generated _not_ via the web UI.
+            $args{Sign} = $ticket->QueueObj->SignAuto();
         }
     }
 
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 406df92..a942bb6 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -394,6 +394,7 @@ sub Create {
         FinalPriority     => 0,
         DefaultDueIn      => 0,
         Sign              => undef,
+        SignAuto          => undef,
         Encrypt           => undef,
         _RecordTransaction => 1,
         @_
@@ -436,14 +437,11 @@ sub Create {
     }
     $RT::Handle->Commit;
 
-    if ( defined $args{'Sign'} ) {
-        my ($status, $msg) = $self->SetSign( $args{'Sign'} );
-        $RT::Logger->error("Couldn't set attribute 'Sign': $msg")
-            unless $status;
-    }
-    if ( defined $args{'Encrypt'} ) {
-        my ($status, $msg) = $self->SetEncrypt( $args{'Encrypt'} );
-        $RT::Logger->error("Couldn't set attribute 'Encrypt': $msg")
+    for my $attr (qw/Sign SignAuto Encrypt/) {
+        next unless defined $args{$attr};
+        my $set = "Set" . $attr;
+        my ($status, $msg) = $self->$set( $args{$attr} );
+        $RT::Logger->error("Couldn't set attribute '$attr': $msg")
             unless $status;
     }
 
@@ -595,6 +593,32 @@ sub SetSign {
     return ($status, $self->loc('Signing disabled'));
 }
 
+sub SignAuto {
+    my $self = shift;
+    my $value = shift;
+
+    return undef unless $self->CurrentUserHasRight('SeeQueue');
+    my $attr = $self->FirstAttribute('SignAuto') or return 0;
+    return $attr->Content;
+}
+
+sub SetSignAuto {
+    my $self = shift;
+    my $value = shift;
+
+    return ( 0, $self->loc('Permission Denied') )
+        unless $self->CurrentUserHasRight('AdminQueue');
+
+    my ($status, $msg) = $self->SetAttribute(
+        Name        => 'SignAuto',
+        Description => 'Sign auto-generated outgoing messages',
+        Content     => $value,
+    );
+    return ($status, $msg) unless $status;
+    return ($status, $self->loc('Signing enabled')) if $value;
+    return ($status, $self->loc('Signing disabled'));
+}
+
 sub Encrypt {
     my $self = shift;
     my $value = shift;
diff --git a/share/html/Admin/Queues/Modify.html b/share/html/Admin/Queues/Modify.html
index 85cd62f..c2cf094 100755
--- a/share/html/Admin/Queues/Modify.html
+++ b/share/html/Admin/Queues/Modify.html
@@ -119,6 +119,8 @@
 <td align="right"><input type="checkbox" class="checkbox" name="Encrypt" value="1" <% $QueueObj->Encrypt? 'checked="checked"': '' |n%> /></td>
 <td><&|/l&>Encrypt by default</&></td>
 </tr>
+<tr><td align="right"><input type="checkbox" class="checkbox" name="SignAuto" value="1" <% $QueueObj->SignAuto? 'checked="checked"': '' |n%> /></td>
+<td colspan="3"><&|/l_unsafe, "<b>","</b>","<i>","</i>"&>Sign all auto-generated mail.  [_1]Caution[_2]: Enabling this option alters the signature from providing [_3]authentication[_4] to providing [_3]integrity[_4].</&></td></tr>
 % }
 
 <tr><td align="right"><input type="checkbox" class="checkbox" name="Enabled" value="1" <%$EnabledChecked|n%> /></td>
@@ -181,13 +183,13 @@ unless ($Create) {
 if ( $QueueObj->Id ) {
     $title = loc('Configuration for queue [_1]', $QueueObj->Name );
     my @attribs= qw(Description CorrespondAddress CommentAddress Name
-        InitialPriority FinalPriority DefaultDueIn Sign Encrypt Lifecycle SubjectTag Disabled);
+        InitialPriority FinalPriority DefaultDueIn Sign SignAuto Encrypt Lifecycle SubjectTag Disabled);
 
     # we're asking about enabled on the web page but really care about disabled
     if ( $SetEnabled ) {
         $Disabled = $ARGS{'Disabled'} = $Enabled? 0: 1;
         $ARGS{$_} = 0 foreach grep !defined $ARGS{$_} || !length $ARGS{$_},
-            qw(Sign Encrypt Disabled);
+            qw(Sign SignAuto Encrypt Disabled);
     }
 
     $m->callback(

commit 4c40dd6b6523977abb8ac77276bbeff45df73027
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Wed Sep 26 22:14:42 2012 -0700

    Blacklist components from automatic, argument-based CSRF whitelisting
    
    Not all pages which take only an id query parameter are idempotent.  The
    ticket bookmark toggle — generally used via ajax — requires only id to
    toggle bookmark state.
    
    Start a blacklist, which should remain small, for these components.

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 99aa7b6..8ae93e8 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1189,6 +1189,13 @@ our %is_whitelisted_component = (
     '/m/tickets/search'     => 1,
 );
 
+# Components which are blacklisted from automatic, argument-based whitelisting.
+# These pages are not idempotent when called with just an id.
+our %is_blacklisted_component = (
+    # Takes only id and toggles bookmark state
+    '/Helpers/Toggle/TicketBookmark' => 1,
+);
+
 sub IsCompCSRFWhitelisted {
     my $comp = shift;
     my $ARGS = shift;
@@ -1211,6 +1218,10 @@ sub IsCompCSRFWhitelisted {
         delete $args{pass};
     }
 
+    # Some pages aren't idempotent even with safe args like id; blacklist
+    # them from the automatic whitelisting below.
+    return 0 if $is_blacklisted_component{$comp};
+
     # Eliminate arguments that do not indicate an effectful request.
     # For example, "id" is acceptable because that is how RT retrieves a
     # record.

commit 666d7a4f14e1065a520062b89cccb34932fa4567
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Fri Sep 28 14:07:38 2012 -0700

    Intuit the next page when logging in at the RT web root
    
    Forcing the next page to WebURL, or even WebPath, loses the potential
    query parameters to index.html.  Perhaps most common is the URL
    
        /?q=foo
    
    for a quick search.  It was previously lost during the login flow.

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 99aa7b6..1ed6766 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -304,7 +304,7 @@ sub HandleRequest {
             }
             # Specially handle /index.html so that we get a nicer URL
             elsif ( $m->request_comp->path eq '/index.html' ) {
-                my $next = SetNextPage(RT->Config->Get('WebURL'));
+                my $next = SetNextPage();
                 $m->comp('/NoAuth/Login.html', next => $next, actions => [$msg]);
                 $m->abort;
             }

commit cbdc50385da30d56b996ad33a7e08117f98dc64c
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Fri Sep 28 11:11:19 2012 -0700

    Abstract away reading $session{NextPage} into two functions
    
    Setting is already wrapped, but reading was direct.  Following commits
    will use FetchNextPage for display.

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 1ed6766..ebf306f 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -379,6 +379,27 @@ sub SetNextPage {
     return $hash;
 }
 
+=head2 FetchNextPage HASHKEY
+
+Returns the stashed next page URL for the given hash.
+
+=cut
+
+sub FetchNextPage {
+    my $hash = shift || "";
+    return $HTML::Mason::Commands::session{'NextPage'}->{$hash};
+}
+
+=head2 RemoveNextPage HASHKEY
+
+Removes the stashed next page URL for the given hash and returns it.
+
+=cut
+
+sub RemoveNextPage {
+    my $hash = shift || "";
+    return delete $HTML::Mason::Commands::session{'NextPage'}->{$hash};
+}
 
 =head2 TangentForLogin [HASH]
 
@@ -600,7 +621,7 @@ sub AttemptExternalAuth {
             $user =~ s/^\Q$NodeName\E\\//i;
         }
 
-        my $next = delete $HTML::Mason::Commands::session{'NextPage'}->{$ARGS->{'next'} || ''};
+        my $next = RemoveNextPage($ARGS->{'next'});
         InstantiateNewSession() unless _UserLoggedIn;
         $HTML::Mason::Commands::session{'CurrentUser'} = RT::CurrentUser->new();
         $HTML::Mason::Commands::session{'CurrentUser'}->$load_method($user);
@@ -699,7 +720,7 @@ sub AttemptPasswordAuthentication {
 
         # It's important to nab the next page from the session before we blow
         # the session away
-        my $next = delete $HTML::Mason::Commands::session{'NextPage'}->{$ARGS->{'next'} || ''};
+        my $next = RemoveNextPage($ARGS->{'next'});
 
         InstantiateNewSession();
         $HTML::Mason::Commands::session{'CurrentUser'} = $user_obj;

commit 03ff7a5c4492815c749d8245546b4be35cb8b3e1
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Fri Sep 28 12:51:39 2012 -0700

    Anticipate storing more information about the next page in the session

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index ebf306f..623d2d1 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -374,14 +374,14 @@ sub SetNextPage {
     my $next = shift || IntuitNextPage();
     my $hash = Digest::MD5::md5_hex($next . $$ . rand(1024));
 
-    $HTML::Mason::Commands::session{'NextPage'}->{$hash} = $next;
+    $HTML::Mason::Commands::session{'NextPage'}->{$hash}{'url'} = $next;
     $HTML::Mason::Commands::session{'i'}++;
     return $hash;
 }
 
 =head2 FetchNextPage HASHKEY
 
-Returns the stashed next page URL for the given hash.
+Returns the stashed next page hashref for the given hash.
 
 =cut
 
@@ -392,7 +392,7 @@ sub FetchNextPage {
 
 =head2 RemoveNextPage HASHKEY
 
-Removes the stashed next page URL for the given hash and returns it.
+Removes the stashed next page for the given hash and returns it.
 
 =cut
 
@@ -622,6 +622,7 @@ sub AttemptExternalAuth {
         }
 
         my $next = RemoveNextPage($ARGS->{'next'});
+           $next = $next->{'url'} if $next;
         InstantiateNewSession() unless _UserLoggedIn;
         $HTML::Mason::Commands::session{'CurrentUser'} = RT::CurrentUser->new();
         $HTML::Mason::Commands::session{'CurrentUser'}->$load_method($user);
@@ -721,6 +722,7 @@ sub AttemptPasswordAuthentication {
         # It's important to nab the next page from the session before we blow
         # the session away
         my $next = RemoveNextPage($ARGS->{'next'});
+           $next = $next->{'url'} if $next;
 
         InstantiateNewSession();
         $HTML::Mason::Commands::session{'CurrentUser'} = $user_obj;

commit e8d0554663854b857025e7211bd0cc51d09e6223
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Fri Sep 28 11:15:15 2012 -0700

    Check the original request for side-effects before prompting for login
    
    Sets the foundation for informing users about to login what will happen
    after they do so in order to help prevent phishing.
    
    Note that while this shares some code and config with CSRF, it _isn't_
    CSRF since the user is not yet logged in and it requires a user to fill
    in valid credentials.  Sharing config allows administrators to whitelist
    external sites, such as corporate portals, which deep-link into RT, thus
    preventing RT from crying wolf to the end user about legitimate
    requests.

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 623d2d1..1b37455 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -304,12 +304,12 @@ sub HandleRequest {
             }
             # Specially handle /index.html so that we get a nicer URL
             elsif ( $m->request_comp->path eq '/index.html' ) {
-                my $next = SetNextPage();
+                my $next = SetNextPage($ARGS);
                 $m->comp('/NoAuth/Login.html', next => $next, actions => [$msg]);
                 $m->abort;
             }
             else {
-                TangentForLogin(results => ($msg ? LoginError($msg) : undef));
+                TangentForLogin($ARGS, results => ($msg ? LoginError($msg) : undef));
             }
         }
     }
@@ -362,7 +362,7 @@ sub LoginError {
     return $key;
 }
 
-=head2 SetNextPage [PATH]
+=head2 SetNextPage ARGSRef [PATH]
 
 Intuits and stashes the next page in the sesssion hash.  If PATH is
 specified, uses that instead of the value of L<IntuitNextPage()>.  Returns
@@ -371,10 +371,30 @@ the hash value.
 =cut
 
 sub SetNextPage {
-    my $next = shift || IntuitNextPage();
+    my $ARGS = shift;
+    my $next = $_[0] ? $_[0] : IntuitNextPage();
     my $hash = Digest::MD5::md5_hex($next . $$ . rand(1024));
+    my $page = { url => $next };
+
+    # If an explicit URL was passed and we didn't IntuitNextPage, then
+    # IsPossibleCSRF below is almost certainly unrelated to the actual
+    # destination.  Currently explicit next pages aren't used in RT, but the
+    # API is available.
+    if (not $_[0] and RT->Config->Get("RestrictReferrer")) {
+        # This isn't really CSRF, but the CSRF heuristics are useful for catching
+        # requests which may have unintended side-effects.
+        my ($is_csrf, $msg, @loc) = IsPossibleCSRF($ARGS);
+        if ($is_csrf) {
+            RT->Logger->notice(
+                "Marking original destination as having side-effects before redirecting for login.\n"
+               ."Request: $next\n"
+               ."Reason: " . HTML::Mason::Commands::loc($msg, @loc)
+            );
+            $page->{'HasSideEffects'} = [$msg, @loc];
+        }
+    }
 
-    $HTML::Mason::Commands::session{'NextPage'}->{$hash}{'url'} = $next;
+    $HTML::Mason::Commands::session{'NextPage'}->{$hash} = $page;
     $HTML::Mason::Commands::session{'i'}++;
     return $hash;
 }
@@ -401,15 +421,18 @@ sub RemoveNextPage {
     return delete $HTML::Mason::Commands::session{'NextPage'}->{$hash};
 }
 
-=head2 TangentForLogin [HASH]
+=head2 TangentForLogin ARGSRef [HASH]
 
 Redirects to C</NoAuth/Login.html>, setting the value of L<IntuitNextPage> as
-the next page.  Optionally takes a hash which is dumped into query params.
+the next page.  Takes a hashref of request %ARGS as the first parameter.
+Optionally takes all other parameters as a hash which is dumped into query
+params.
 
 =cut
 
 sub TangentForLogin {
-    my $hash  = SetNextPage();
+    my $ARGS  = shift;
+    my $hash  = SetNextPage($ARGS);
     my %query = (@_, next => $hash);
     my $login = RT->Config->Get('WebURL') . 'NoAuth/Login.html?';
     $login .= $HTML::Mason::Commands::m->comp('/Elements/QueryString', %query);
@@ -424,8 +447,9 @@ calls L<TangentForLogin> with the appropriate results key.
 =cut
 
 sub TangentForLoginWithError {
-    my $key = LoginError(HTML::Mason::Commands::loc(@_));
-    TangentForLogin( results => $key );
+    my $ARGS = shift;
+    my $key  = LoginError(HTML::Mason::Commands::loc(@_));
+    TangentForLogin( $ARGS, results => $key );
 }
 
 =head2 IntuitNextPage
@@ -661,7 +685,7 @@ sub AttemptExternalAuth {
                 delete $HTML::Mason::Commands::session{'CurrentUser'};
 
                 if (RT->Config->Get('WebFallbackToInternalAuth')) {
-                    TangentForLoginWithError('Cannot create user: [_1]', $msg);
+                    TangentForLoginWithError($ARGS, 'Cannot create user: [_1]', $msg);
                 } else {
                     $m->abort();
                 }
@@ -684,13 +708,13 @@ sub AttemptExternalAuth {
             $user = $orig_user;
 
             unless ( RT->Config->Get('WebFallbackToInternalAuth') ) {
-                TangentForLoginWithError('You are not an authorized user');
+                TangentForLoginWithError($ARGS, 'You are not an authorized user');
             }
         }
     } elsif ( RT->Config->Get('WebFallbackToInternalAuth') ) {
         unless ( defined $HTML::Mason::Commands::session{'CurrentUser'} ) {
             # XXX unreachable due to prior defaulting in HandleRequest (check c34d108)
-            TangentForLoginWithError('You are not an authorized user');
+            TangentForLoginWithError($ARGS, 'You are not an authorized user');
         }
     } else {
 

commit 5d569351cabe49819641fec936bdcf0e87580922
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Fri Sep 28 12:44:18 2012 -0700

    Inform the user logging in about potential side-effects
    
    This helps prevent phishing where the user follows a malicious link and
    isn't logged in yet.  Previously there was no indication of what would
    happen after login.  The CSRF protection does somewhat double duty and
    provides the same measure of phishing protection when the user is
    already logged in, but it is never triggered by unauthenticated
    requests.
    
    The warnings on login are controlled by the same configuration as CSRF.

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 1b37455..c8667c1 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1453,6 +1453,30 @@ sub MaybeShowInterstitialCSRFPage {
     # Calls abort, never gets here
 }
 
+our @POTENTIAL_PAGE_ACTIONS = (
+    qr'/Ticket/Create.html' => "create a ticket",              # loc
+    qr'/Ticket/'            => "update a ticket",              # loc
+    qr'/Admin/'             => "modify RT's configuration",    # loc
+    qr'/Approval/'          => "update an approval",           # loc
+    qr'/Articles/'          => "update an article",            # loc
+    qr'/Dashboards/'        => "modify a dashboard",           # loc
+    qr'/m/ticket/'          => "update a ticket",              # loc
+    qr'Prefs'               => "modify your preferences",      # loc
+    qr'/Search/'            => "modify or access a search",    # loc
+    qr'/SelfService/Create' => "create a ticket",              # loc
+    qr'/SelfService/'       => "update a ticket",              # loc
+);
+
+sub PotentialPageAction {
+    my $page = shift;
+    my @potentials = @POTENTIAL_PAGE_ACTIONS;
+    while (my ($pattern, $result) = splice @potentials, 0, 2) {
+        return HTML::Mason::Commands::loc($result)
+            if $page =~ $pattern;
+    }
+    return "";
+}
+
 package HTML::Mason::Commands;
 
 use vars qw/$r $m %session/;
diff --git a/share/html/Elements/Login b/share/html/Elements/Login
index b86bfef..b3f1a24 100755
--- a/share/html/Elements/Login
+++ b/share/html/Elements/Login
@@ -61,6 +61,8 @@
 <div id="login-box">
 <&| /Widgets/TitleBox, title => loc('Login'), titleright => $RT::VERSION, hideable => 0 &>
 
+<& LoginRedirectWarning, %ARGS &>
+
 % unless (RT->Config->Get('WebExternalAuth') and !RT->Config->Get('WebFallbackToInternalAuth')) {
 <form id="login" name="login" method="post" action="<% RT->Config->Get('WebPath') %>/NoAuth/Login.html">
 
diff --git a/share/html/Elements/LoginRedirectWarning b/share/html/Elements/LoginRedirectWarning
new file mode 100644
index 0000000..af3f0a1
--- /dev/null
+++ b/share/html/Elements/LoginRedirectWarning
@@ -0,0 +1,20 @@
+<%args>
+$next => undef
+</%args>
+<%init>
+return unless $next;
+
+my $destination = RT::Interface::Web::FetchNextPage($next);
+return unless $destination and $destination->{'HasSideEffects'};
+
+my $consequence = RT::Interface::Web::PotentialPageAction($destination->{'url'}) || loc("perform actions");
+   $consequence = $m->interp->apply_escapes($consequence => "h");
+</%init>
+<div class="redirect-warning">
+  <p>
+    <&|/l&>After logging in you'll be sent to your original destination:</&>
+    <tt title="<% $destination->{'url'} %>"><% $destination->{'url'} %></tt>
+    <&|/l_unsafe, "<strong>$consequence</strong>" &>which may [_1] on your behalf.</&>
+  </p>
+  <p><&|/l&>If this is not what you expect, leave this page now without logging in.</&></p>
+</div>
diff --git a/share/html/NoAuth/css/base/login.css b/share/html/NoAuth/css/base/login.css
index bd05a28..608ebf8 100644
--- a/share/html/NoAuth/css/base/login.css
+++ b/share/html/NoAuth/css/base/login.css
@@ -100,3 +100,11 @@ margin-right:auto;margin-left:auto;
     padding-left: 1em;
 }
 
+.redirect-warning tt {
+    display: block;
+    margin: 0.5em 0 0.5em 1em;
+    white-space: nowrap;
+    overflow: hidden;
+    text-overflow: ellipsis;
+    width: 90%;
+}

commit 7e3782deb299becc9221cc83ee2d9cab09721d74
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Fri Sep 28 13:06:29 2012 -0700

    Include the potential request's action in the CSRF interstitial
    
    We now have the means to be slightly less vague than "perform actions".

diff --git a/share/html/Elements/CSRF b/share/html/Elements/CSRF
index 4893c12..a3c1943 100644
--- a/share/html/Elements/CSRF
+++ b/share/html/Elements/CSRF
@@ -52,11 +52,11 @@
 
 % my $strong_start = "<strong>";
 % my $strong_end   = "</strong>";
-<p><&|/l_unsafe, $strong_start, $strong_end, $Reason &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3].  This is possibly caused by a malicious attacker trying to perform actions against RT on your behalf. If you did not initiate this request, then you should alert your security team.</&></p>
+<p><&|/l_unsafe, $strong_start, $strong_end, $Reason, $action &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3].  A malicious attacker may be trying to [_1][_4][_2] on your behalf. If you did not initiate this request, then you should alert your security team.</&></p>
 
 % my $start = qq|<strong><a href="$url_with_token">|;
 % my $end   = qq|</a></strong>|;
-<p><&|/l_unsafe, $escaped_path, $start, $end &>If you really intended to visit [_1], then [_2]click here to resume your request[_3].</&></p>
+<p><&|/l_unsafe, $escaped_path, $action, $start, $end &>If you really intended to visit [_1] and [_2], then [_3]click here to resume your request[_4].</&></p>
 
 <& /Elements/Footer, %ARGS &>
 % $m->abort;
@@ -71,4 +71,6 @@ $escaped_path = "<tt>$escaped_path</tt>";
 
 my $url_with_token = URI->new($OriginalURL);
 $url_with_token->query_form([CSRF_Token => $Token]);
+
+my $action = RT::Interface::Web::PotentialPageAction($OriginalURL) || loc("perform actions");
 </%INIT>

commit 72ee4047c9855b3c8e1a7e2a0e7635c3a3ed230b
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Fri Aug 24 11:31:26 2012 -0700

    Load the Class as the current user when creating Articles
    
    We use the loaded class to check the CreateArticle right, which is
    always true for the system user.  This enabled any privileged user to
    create articles in any class that existed given a crafted URL.  Only the
    core article fields were usable (Name, Summary, Class), since custom
    fields are protected by independent rights.
    
    Resolves CVE-2012-4731.

diff --git a/lib/RT/Article.pm b/lib/RT/Article.pm
index 24b952a..678aa11 100644
--- a/lib/RT/Article.pm
+++ b/lib/RT/Article.pm
@@ -102,7 +102,7 @@ sub Create {
         @_
     );
 
-    my $class = RT::Class->new($RT::SystemUser);
+    my $class = RT::Class->new( $self->CurrentUser );
     $class->Load( $args{'Class'} );
     unless ( $class->Id ) {
         return ( 0, $self->loc('Invalid Class') );

commit ecbdf95e4d4bebf71152fdfa18b11524d364c9a4
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Wed Oct 3 12:29:15 2012 -0700

    Headers in the parsed MIME entities of Templates are modifiable
    
    This allows reformatting of inserted headers by canonicalizing tag case
    and, crucially, folding (or refolding) lines.
    
    When the header object is not explicitly marked modifiable — such as
    when generated via parsing a raw MIME message — Mail::Header assumes
    that header values you set should be inserted as-is.  This means
    newlines are not stripped or validated as you're expected to construct
    proper continuations yourself.
    
    RT incorrectly assumed newlines in header values would be stripped,
    leaving open the possibility of header injection via various
    user-controlled inputs.  This commit resolves CVE-2012-4730.
    
    Fixes failing tests by removing the assumptions that:
    
        1) Case of header names is preserved
        2) Header values are always on a single line

diff --git a/lib/RT/Template.pm b/lib/RT/Template.pm
index 117cc3f..e509454 100644
--- a/lib/RT/Template.pm
+++ b/lib/RT/Template.pm
@@ -390,6 +390,7 @@ sub _Parse {
 
     # Unfold all headers
     $self->{'MIMEObj'}->head->unfold;
+    $self->{'MIMEObj'}->head->modify(1);
 
     return ( 1, $self->loc("Template parsed") );
 
diff --git a/t/web/crypt-gnupg.t b/t/web/crypt-gnupg.t
index 8c0eb57..b30edc3 100644
--- a/t/web/crypt-gnupg.t
+++ b/t/web/crypt-gnupg.t
@@ -8,6 +8,7 @@ use RT::Test::GnuPG
     'trust-model' => 'always',
 };
 use Test::Warn;
+use MIME::Head;
 
 use RT::Action::SendEmail;
 
@@ -70,8 +71,7 @@ $user->SetEmailAddress('general at example.com');
 for my $mail (@mail) {
     unlike $mail, qr/Some content/, "outgoing mail was encrypted";
 
-    my ($content_type) = $mail =~ /^(Content-Type: .*)/m;
-    my ($mime_version) = $mail =~ /^(MIME-Version: .*)/m;
+    my ($content_type, $mime_version) = get_headers($mail, "Content-Type", "MIME-Version");
     my $body = strip_headers($mail);
 
     $mail = << "MAIL";
@@ -139,8 +139,7 @@ for my $mail (@mail) {
     like $mail, qr/Some other content/, "outgoing mail was not encrypted";
     like $mail, qr/-----BEGIN PGP SIGNATURE-----[\s\S]+-----END PGP SIGNATURE-----/, "data has some kind of signature";
 
-    my ($content_type) = $mail =~ /^(Content-Type: .*)/m;
-    my ($mime_version) = $mail =~ /^(MIME-Version: .*)/m;
+    my ($content_type, $mime_version) = get_headers($mail, "Content-Type", "MIME-Version");
     my $body = strip_headers($mail);
 
     $mail = << "MAIL";
@@ -212,8 +211,7 @@ ok(@mail, "got some mail");
 for my $mail (@mail) {
     unlike $mail, qr/Some other content/, "outgoing mail was encrypted";
 
-    my ($content_type) = $mail =~ /^(Content-Type: .*)/m;
-    my ($mime_version) = $mail =~ /^(MIME-Version: .*)/m;
+    my ($content_type, $mime_version) = get_headers($mail, "Content-Type", "MIME-Version");
     my $body = strip_headers($mail);
 
     $mail = << "MAIL";
@@ -279,8 +277,7 @@ ok(@mail, "got some mail");
 for my $mail (@mail) {
     like $mail, qr/Thought you had me figured out didya/, "outgoing mail was unencrypted";
 
-    my ($content_type) = $mail =~ /^(Content-Type: .*)/m;
-    my ($mime_version) = $mail =~ /^(MIME-Version: .*)/m;
+    my ($content_type, $mime_version) = get_headers($mail, "Content-Type", "MIME-Version");
     my $body = strip_headers($mail);
 
     $mail = << "MAIL";
@@ -326,6 +323,20 @@ MAIL
     like($attachments[0]->Content, qr/$RT::rtname/, "RT's mail includes this instance's name");
 }
 
+sub get_headers {
+    my $mail = shift;
+    open my $fh, "<", \$mail or die $!;
+    my $head = MIME::Head->read($fh);
+    return @{[
+        map {
+            my $hdr = "$_: " . $head->get($_);
+            chomp $hdr;
+            $hdr;
+        }
+        @_
+    ]};
+}
+
 sub strip_headers
 {
     my $mail = shift;
diff --git a/t/web/ticket_forward.t b/t/web/ticket_forward.t
index 1d74673..0c411b9 100644
--- a/t/web/ticket_forward.t
+++ b/t/web/ticket_forward.t
@@ -49,7 +49,7 @@ diag "Forward Ticket" if $ENV{TEST_VERBOSE};
     my ($mail) = RT::Test->fetch_caught_mails;
     like( $mail, qr!Subject: test forward!,           'Subject field' );
     like( $mail, qr!To: rt-test, rt-to\@example.com!, 'To field' );
-    like( $mail, qr!Cc: rt-cc\@example.com!,          'Cc field' );
+    like( $mail, qr!Cc: rt-cc\@example.com!i,         'Cc field' );
     like( $mail, qr!This is a forward of ticket!,     'content' );
     like( $mail, qr!this is an attachment!,           'att content' );
     like( $mail, qr!$att_name!,                       'att file name' );
@@ -75,8 +75,8 @@ qr/Forwarded Transaction #\d+ to rt-test, rt-to\@example.com, rt-cc\@example.com
     my ($mail) = RT::Test->fetch_caught_mails;
     like( $mail, qr!Subject: test forward!,            'Subject field' );
     like( $mail, qr!To: rt-test, rt-to\@example.com!,  'To field' );
-    like( $mail, qr!Cc: rt-cc\@example.com!,           'Cc field' );
-    like( $mail, qr!Bcc: rt-bcc\@example.com!,         'Bcc field' );
+    like( $mail, qr!Cc: rt-cc\@example.com!i,          'Cc field' );
+    like( $mail, qr!Bcc: rt-bcc\@example.com!i,        'Bcc field' );
     like( $mail, qr!This is a forward of transaction!, 'content' );
     like( $mail, qr!$att_name!,                        'att file name' );
     like( $mail, qr!this is an attachment!,            'att content' );

commit 4cda0e47df5431c59d2a03f61917ab35c0cdbfaf
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Oct 4 10:16:08 2012 -0700

    Comment on our invalid pattern for splitting headers
    
    And why we can't fix it now.

diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index fb17da3..d46a991 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -676,6 +676,12 @@ sub _SplitHeaders {
     my $self = shift;
     my $headers = (shift || $self->_Value('Headers'));
     my @headers;
+    # XXX TODO: splitting on \n\w is _wrong_ as it treats \n[ as a valid
+    # continuation, which it isn't.  The correct split pattern, per RFC 2822,
+    # is /\n(?=[^ \t]|\z)/.  That is, only "\n " or "\n\t" is a valid
+    # continuation.  Older values of X-RT-GnuPG-Status contain invalid
+    # continuations and rely on this bogus split pattern, however, so it is
+    # left as-is for now.
     for (split(/\n(?=\w|\z)/,$headers)) {
         push @headers, $_;
 

commit c964114ca495830f70ec8789d4d9de878fbf84b2
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Oct 4 14:56:33 2012 -0700

    Perltidy only before updating the SetHeader method

diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index d46a991..3209a9f 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -601,7 +601,7 @@ sub DelHeader {
     my $newheader = '';
     foreach my $line ($self->_SplitHeaders) {
         next if $line =~ /^\Q$tag\E:\s+(.*)$/is;
-	$newheader .= "$line\n";
+        $newheader .= "$line\n";
     }
     return $self->__Set( Field => 'Headers', Value => $newheader);
 }
@@ -636,14 +636,13 @@ sub SetHeader {
     my $tag = shift;
 
     my $newheader = '';
-    foreach my $line ($self->_SplitHeaders) {
-        if (defined $tag and $line =~ /^\Q$tag\E:\s+(.*)$/i) {
-	    $newheader .= "$tag: $_[0]\n";
-	    undef $tag;
+    foreach my $line ( $self->_SplitHeaders ) {
+        if ( defined $tag and $line =~ /^\Q$tag\E:\s+(.*)$/i ) {
+            $newheader .= "$tag: $_[0]\n";
+            undef $tag;
+        } else {
+            $newheader .= "$line\n";
         }
-	else {
-	    $newheader .= "$line\n";
-	}
     }
 
     $newheader .= "$tag: $_[0]\n" if defined $tag;

commit 0bd9510437d7adbeb330313eaebc6103eb917026
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Oct 4 15:14:36 2012 -0700

    No need to match on the rest of the header line(s), just the tag
    
    Simpler, less prone to a mistake (like the one in SetHeader to be fixed
    next).

diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index 3209a9f..1566e87 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -600,7 +600,7 @@ sub DelHeader {
 
     my $newheader = '';
     foreach my $line ($self->_SplitHeaders) {
-        next if $line =~ /^\Q$tag\E:\s+(.*)$/is;
+        next if $line =~ /^\Q$tag\E:\s+/i;
         $newheader .= "$line\n";
     }
     return $self->__Set( Field => 'Headers', Value => $newheader);

commit e0c1ec5163da68ddaf477010b6219d41a7be40bc
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Oct 4 15:17:43 2012 -0700

    Don't require a \r before the \n when forcing header continuations
    
    A bare \n needs to be continued just as much as \r\n.
    
    This doesn't fix the bug of adding an extra space to existing valid
    continuations.

diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index 1566e87..039d5e8 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -619,7 +619,7 @@ sub AddHeader {
     while ( my ($tag, $value) = splice @_, 0, 2 ) {
         $value = '' unless defined $value;
         $value =~ s/\s+$//s;
-        $value =~ s/\r+\n/\n /g;
+        $value =~ s/\r*\n/\n /g;
         $newheader .= "$tag: $value\n";
     }
     return $self->__Set( Field => 'Headers', Value => $newheader);

commit 42623930c7a4eb524dcd5d702912faae418acab0
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Oct 4 15:19:35 2012 -0700

    Refactor header value canonicalization for use by other methods
    
    SetHeader in particular does no canonicalization (yet).

diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index 039d5e8..a464004 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -617,9 +617,7 @@ sub AddHeader {
 
     my $newheader = $self->__Value( 'Headers' );
     while ( my ($tag, $value) = splice @_, 0, 2 ) {
-        $value = '' unless defined $value;
-        $value =~ s/\s+$//s;
-        $value =~ s/\r*\n/\n /g;
+        $value = $self->_CanonicalizeHeaderValue($value);
         $newheader .= "$tag: $value\n";
     }
     return $self->__Set( Field => 'Headers', Value => $newheader);
@@ -649,6 +647,17 @@ sub SetHeader {
     $self->__Set( Field => 'Headers', Value => $newheader);
 }
 
+sub _CanonicalizeHeaderValue {
+    my $self  = shift;
+    my $value = shift;
+
+    $value = '' unless defined $value;
+    $value =~ s/\s+$//s;
+    $value =~ s/\r*\n/\n /g;
+
+    return $value;
+}
+
 =head2 SplitHeaders
 
 Returns an array of this attachment object's headers, with one header 

commit dba758b4353ed506e1d7527a0aad22b0124c8786
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Oct 4 15:21:08 2012 -0700

    Fix three bugs in SetHeader
    
    SetHeader now:
    
    • Canonicalizes the inserted header value to handle continuations
    • Removes all existing occurrences of the header instead of only the first.
    • Removes header continuations instead of leaving the continued lines
      behind (rendering them continuations of the previous header!)

diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm
index a464004..f1d9a63 100644
--- a/lib/RT/Attachment.pm
+++ b/lib/RT/Attachment.pm
@@ -630,20 +630,25 @@ Replace or add a Header to the attachment's headers.
 =cut
 
 sub SetHeader {
-    my $self = shift;
-    my $tag = shift;
+    my $self  = shift;
+    my $tag   = shift;
+    my $value = $self->_CanonicalizeHeaderValue(shift);
 
+    my $replaced  = 0;
     my $newheader = '';
     foreach my $line ( $self->_SplitHeaders ) {
-        if ( defined $tag and $line =~ /^\Q$tag\E:\s+(.*)$/i ) {
-            $newheader .= "$tag: $_[0]\n";
-            undef $tag;
+        if ( $line =~ /^\Q$tag\E:\s+/i ) {
+            # replace first instance, skip all the rest
+            unless ($replaced) {
+                $newheader .= "$tag: $value\n";
+                $replaced = 1;
+            }
         } else {
             $newheader .= "$line\n";
         }
     }
 
-    $newheader .= "$tag: $_[0]\n" if defined $tag;
+    $newheader .= "$tag: $value\n" unless $replaced;
     $self->__Set( Field => 'Headers', Value => $newheader);
 }
 

commit e6c6ae922252e36b510264472820ea8df72e4b7e
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Oct 4 21:59:42 2012 -0700

    Let MIME::Head modify the X-RT-GnuPg-Status header to handle continuations
    
    X-RT-GnuPG-Status is a set to a many-line string and previously
    continuations weren't handled properly.
    
    Temporarily letting the inserted headers be modified fixes the known
    problem.  If instead we marked the entire entity as modifiable early
    after parsing and left it that way, the possibility of unforeseen
    breakage would be too high for a stable release series.  It would also
    lead to even further deviation of RT's stored message from the original
    source message.

diff --git a/lib/RT/Crypt/GnuPG.pm b/lib/RT/Crypt/GnuPG.pm
index 8df1c1b..3ebb7d7 100644
--- a/lib/RT/Crypt/GnuPG.pm
+++ b/lib/RT/Crypt/GnuPG.pm
@@ -1077,9 +1077,13 @@ sub VerifyDecrypt {
         }
         if ( $args{'SetStatus'} || $args{'AddStatus'} ) {
             my $method = $args{'AddStatus'} ? 'add' : 'set';
+            # Let the header be modified so continuations are handled
+            my $modify = $status_on->head->modify;
+            $status_on->head->modify(1);
             $status_on->head->$method(
                 'X-RT-GnuPG-Status' => $res[-1]->{'status'}
             );
+            $status_on->head->modify($modify);
         }
     }
     foreach my $item( grep $_->{'Type'} eq 'encrypted', @protected ) {
@@ -1096,9 +1100,13 @@ sub VerifyDecrypt {
         }
         if ( $args{'SetStatus'} || $args{'AddStatus'} ) {
             my $method = $args{'AddStatus'} ? 'add' : 'set';
+            # Let the header be modified so continuations are handled
+            my $modify = $status_on->head->modify;
+            $status_on->head->modify(1);
             $status_on->head->$method(
                 'X-RT-GnuPG-Status' => $res[-1]->{'status'}
             );
+            $status_on->head->modify($modify);
         }
     }
     return @res;

commit 496e93fe0360954c7d36cecbc2219791752c58c7
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Fri Oct 5 15:03:20 2012 -0700

    Don't 500 if we come across a session with NextPage of the old variety
    
    Unlikely, but possible if the session got a login form before the
    security upgrade and submitted the same form after the upgrade.

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index c8667c1..dee87fe 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -646,7 +646,7 @@ sub AttemptExternalAuth {
         }
 
         my $next = RemoveNextPage($ARGS->{'next'});
-           $next = $next->{'url'} if $next;
+           $next = $next->{'url'} if ref $next;
         InstantiateNewSession() unless _UserLoggedIn;
         $HTML::Mason::Commands::session{'CurrentUser'} = RT::CurrentUser->new();
         $HTML::Mason::Commands::session{'CurrentUser'}->$load_method($user);
@@ -746,7 +746,7 @@ sub AttemptPasswordAuthentication {
         # It's important to nab the next page from the session before we blow
         # the session away
         my $next = RemoveNextPage($ARGS->{'next'});
-           $next = $next->{'url'} if $next;
+           $next = $next->{'url'} if ref $next;
 
         InstantiateNewSession();
         $HTML::Mason::Commands::session{'CurrentUser'} = $user_obj;
diff --git a/share/html/Elements/LoginRedirectWarning b/share/html/Elements/LoginRedirectWarning
index af3f0a1..891e381 100644
--- a/share/html/Elements/LoginRedirectWarning
+++ b/share/html/Elements/LoginRedirectWarning
@@ -5,7 +5,7 @@ $next => undef
 return unless $next;
 
 my $destination = RT::Interface::Web::FetchNextPage($next);
-return unless $destination and $destination->{'HasSideEffects'};
+return unless ref $destination and $destination->{'HasSideEffects'};
 
 my $consequence = RT::Interface::Web::PotentialPageAction($destination->{'url'}) || loc("perform actions");
    $consequence = $m->interp->apply_escapes($consequence => "h");

commit 0b3d6861f99269a40dce601263bba1145e61bd1d
Merge: e4916c6 72ee404
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Thu Oct 25 14:37:03 2012 -0400

    Merge branch 'security/4.0/create-article' into 4.0.8-releng


commit f350d859467e04d391491c0129eca052a90059bc
Merge: 0b3d686 4c40dd6
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Thu Oct 25 14:37:08 2012 -0400

    Merge branch 'security/4.0/csrf-blacklist' into 4.0.8-releng


commit 73c3c162183b756bb33378f3def60033de776836
Merge: f350d85 e6c6ae9
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Thu Oct 25 14:37:11 2012 -0400

    Merge branch 'security/4.0/email-header-injection' into 4.0.8-releng


commit 9b5d6534dc158b4449e6c6587fde19f51ff02721
Merge: 73c3c16 e3c2880
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Thu Oct 25 14:37:15 2012 -0400

    Merge branch 'security/4.0/signing' into 4.0.8-releng


commit 048ac133c6aa7af528a84dc1aece81042ed79c4f
Merge: 9b5d653 496e93f
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Thu Oct 25 14:37:20 2012 -0400

    Merge branch 'security/4.0/warn-about-redirect-after-login' into 4.0.8-releng


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


More information about the Rt-commit mailing list