diff options
Diffstat (limited to 'rt/lib')
31 files changed, 809 insertions, 96 deletions
diff --git a/rt/lib/RT.pm b/rt/lib/RT.pm index 4a20f9b43..84c4ad183 100644 --- a/rt/lib/RT.pm +++ b/rt/lib/RT.pm @@ -57,7 +57,7 @@ use Cwd (); use vars qw($Config $System $SystemUser $Nobody $Handle $Logger $_INSTALL_MODE); -our $VERSION = '3.8.11'; +our $VERSION = '3.8.13'; @@ -688,11 +688,21 @@ sub InitPlugins { sub InstallMode { my $self = shift; if (@_) { - $_INSTALL_MODE = shift; - if($_INSTALL_MODE) { - require RT::CurrentUser; - $SystemUser = RT::CurrentUser->new(); - } + my ($integrity, $state, $msg) = RT::Handle->CheckIntegrity; + if ($_[0] and $integrity) { + # Trying to turn install mode on but we have a good DB! + require Carp; + $RT::Logger->error( + Carp::longmess("Something tried to turn on InstallMode but we have DB integrity!") + ); + } + else { + $_INSTALL_MODE = shift; + if($_INSTALL_MODE) { + require RT::CurrentUser; + $SystemUser = RT::CurrentUser->new(); + } + } } return $_INSTALL_MODE; } diff --git a/rt/lib/RT.pm.in b/rt/lib/RT.pm.in index fafd2b778..5842407ff 100644 --- a/rt/lib/RT.pm.in +++ b/rt/lib/RT.pm.in @@ -688,11 +688,21 @@ sub InitPlugins { sub InstallMode { my $self = shift; if (@_) { - $_INSTALL_MODE = shift; - if($_INSTALL_MODE) { - require RT::CurrentUser; - $SystemUser = RT::CurrentUser->new(); - } + my ($integrity, $state, $msg) = RT::Handle->CheckIntegrity; + if ($_[0] and $integrity) { + # Trying to turn install mode on but we have a good DB! + require Carp; + $RT::Logger->error( + Carp::longmess("Something tried to turn on InstallMode but we have DB integrity!") + ); + } + else { + $_INSTALL_MODE = shift; + if($_INSTALL_MODE) { + require RT::CurrentUser; + $SystemUser = RT::CurrentUser->new(); + } + } } return $_INSTALL_MODE; } diff --git a/rt/lib/RT/ACL_Overlay.pm b/rt/lib/RT/ACL_Overlay.pm index feef257e4..a0429a331 100644 --- a/rt/lib/RT/ACL_Overlay.pm +++ b/rt/lib/RT/ACL_Overlay.pm @@ -175,6 +175,9 @@ sub LimitToPrincipal { FIELD1 => 'PrincipalId', ALIAS2 => $cgm, FIELD2 => 'GroupId' ); + $self->Limit( ALIAS => $cgm, + FIELD => 'Disabled', + VALUE => 0 ); $self->Limit( ALIAS => $cgm, FIELD => 'MemberId', OPERATOR => '=', diff --git a/rt/lib/RT/Action/CreateTickets.pm b/rt/lib/RT/Action/CreateTickets.pm index fd3e77c3a..80c7ac8a7 100644 --- a/rt/lib/RT/Action/CreateTickets.pm +++ b/rt/lib/RT/Action/CreateTickets.pm @@ -1148,6 +1148,7 @@ sub UpdateCustomFields { my $cf = $1; my $CustomFieldObj = RT::CustomField->new($self->CurrentUser); + $CustomFieldObj->SetContextObject( $ticket ); $CustomFieldObj->LoadById($cf); my @values; diff --git a/rt/lib/RT/Action/SendEmail.pm b/rt/lib/RT/Action/SendEmail.pm index 9e93e4aab..a98a7647d 100755 --- a/rt/lib/RT/Action/SendEmail.pm +++ b/rt/lib/RT/Action/SendEmail.pm @@ -409,6 +409,10 @@ sub AddAttachment { my $attach = shift; my $MIMEObj = shift || $self->TemplateObj->MIMEObj; + # $attach->TransactionObj may not always be $self->TransactionObj + return unless $attach->Id + and $attach->TransactionObj->CurrentUserCanSee; + $MIMEObj->attach( Type => $attach->ContentType, Charset => $attach->OriginalEncoding, @@ -467,8 +471,7 @@ sub AddTicket { my $self = shift; my $tid = shift; - # XXX: we need a current user here, but who is current user? - my $attachs = RT::Attachments->new($RT::SystemUser); + my $attachs = RT::Attachments->new( RT::CurrentUser->new($self->TransactionObj->Creator) ); my $txn_alias = $attachs->TransactionAlias; $attachs->Limit( ALIAS => $txn_alias, FIELD => 'Type', VALUE => 'Create' ); $attachs->Limit( diff --git a/rt/lib/RT/Attachments_Overlay.pm b/rt/lib/RT/Attachments_Overlay.pm index d758c76e8..12fc88bcf 100644 --- a/rt/lib/RT/Attachments_Overlay.pm +++ b/rt/lib/RT/Attachments_Overlay.pm @@ -227,15 +227,12 @@ sub Next { my $Attachment = $self->SUPER::Next; return $Attachment unless $Attachment; - my $txn = $Attachment->TransactionObj; - if ( $txn->__Value('Type') eq 'Comment' ) { - return $Attachment if $txn->CurrentUserHasRight('ShowTicketComments'); - } elsif ( $txn->CurrentUserHasRight('ShowTicket') ) { + if ( $Attachment->TransactionObj->CurrentUserCanSee ) { return $Attachment; + } else { + # If the user doesn't have the right to show this ticket + return $self->Next; } - - # If the user doesn't have the right to show this ticket - return $self->Next; } # }}} diff --git a/rt/lib/RT/CustomField_Overlay.pm b/rt/lib/RT/CustomField_Overlay.pm index 25394cf0f..c6fa1851f 100644 --- a/rt/lib/RT/CustomField_Overlay.pm +++ b/rt/lib/RT/CustomField_Overlay.pm @@ -338,10 +338,12 @@ sub LoadByName { } # if we're looking for a queue by name, make it a number - if ( defined $args{'Queue'} && $args{'Queue'} =~ /\D/ ) { + if ( defined $args{'Queue'} && ($args{'Queue'} =~ /\D/ || !$self->ContextObject) ) { my $QueueObj = RT::Queue->new( $self->CurrentUser ); $QueueObj->Load( $args{'Queue'} ); $args{'Queue'} = $QueueObj->Id; + $self->SetContextObject( $QueueObj ) + unless $self->ContextObject; } # XXX - really naive implementation. Slow. - not really. still just one query @@ -399,6 +401,8 @@ sub Values { # if the user has no rights, return an empty object if ( $self->id && $self->CurrentUserHasRight( 'SeeCustomField') ) { $cf_values->LimitToCustomField( $self->Id ); + } else { + $cf_values->Limit( FIELD => 'id', VALUE => 0, SUBCLAUSE => 'acl' ); } return ($cf_values); } @@ -744,7 +748,77 @@ sub ContextObject { my $self = shift; return $self->{'context_object'}; } - + +sub ValidContextType { + my $self = shift; + my $class = shift; + + my %valid; + $valid{$_}++ for split '-', $self->LookupType; + delete $valid{'RT::Transaction'}; + + return $valid{$class}; +} + +=head2 LoadContextObject + +Takes an Id for a Context Object and loads the right kind of RT::Object +for this particular Custom Field (based on the LookupType) and returns it. +This is a good way to ensure you don't try to use a Queue as a Context +Object on a User Custom Field. + +=cut + +sub LoadContextObject { + my $self = shift; + my $type = shift; + my $contextid = shift; + + unless ( $self->ValidContextType($type) ) { + RT->Logger->debug("Invalid ContextType $type for Custom Field ".$self->Id); + return; + } + + my $context_object = $type->new( $self->CurrentUser ); + my ($id, $msg) = $context_object->LoadById( $contextid ); + unless ( $id ) { + RT->Logger->debug("Invalid ContextObject id: $msg"); + return; + } + return $context_object; +} + +=head2 ValidateContextObject + +Ensure that a given ContextObject applies to this Custom Field. +For custom fields that are assigned to Queues or to Classes, this checks that the Custom +Field is actually applied to that objects. For Global Custom Fields, it returns true +as long as the Object is of the right type, because you may be using +your permissions on a given Queue of Class to see a Global CF. +For CFs that are only applied Globally, you don't need a ContextObject. + +=cut + +sub ValidateContextObject { + my $self = shift; + my $object = shift; + + return 1 if $self->IsApplied(0); + + # global only custom fields don't have objects + # that should be used as context objects. + return if $self->ApplyGlobally; + + # Otherwise, make sure we weren't passed a user object that we're + # supposed to treat as a queue. + return unless $self->ValidContextType(ref $object); + + # Check that it is applied correctly + my ($applied_to) = grep {ref($_) eq $self->RecordClassFromLookupType} ($object, $object->ACLEquivalenceObjects); + return unless $applied_to; + return $self->IsApplied($applied_to->id); +} + # {{{ sub _Set sub _Set { @@ -1435,6 +1509,7 @@ sub SetBasedOn { unless defined $value and length $value; my $cf = RT::CustomField->new( $self->CurrentUser ); + $cf->SetContextObject( $self->ContextObject ); $cf->Load( ref $value ? $value->Id : $value ); return (0, "Permission denied") @@ -1450,6 +1525,7 @@ sub SetBasedOn { sub BasedOnObj { my $self = shift; my $obj = RT::CustomField->new( $self->CurrentUser ); + $obj->SetContextObject( $self->ContextObject ); my $attribute = $self->FirstAttribute("BasedOn"); $obj->Load($attribute->Content) if defined $attribute; diff --git a/rt/lib/RT/Date.pm b/rt/lib/RT/Date.pm index 384b74abc..80f181647 100644 --- a/rt/lib/RT/Date.pm +++ b/rt/lib/RT/Date.pm @@ -604,6 +604,10 @@ sub Get my $self = shift; my %args = (Format => 'ISO', @_); my $formatter = $args{'Format'}; + unless ( $self->ValidFormatter($formatter) ) { + RT->Logger->warning("Invalid date formatter '$formatter', falling back to ISO"); + $formatter = 'ISO'; + } $formatter = 'ISO' unless $self->can($formatter); return $self->$formatter( %args ); } @@ -642,6 +646,20 @@ sub Formatters return @FORMATTERS; } +=head3 ValidFormatter FORMAT + +Returns a true value if C<FORMAT> is a known formatter. Otherwise returns +false. + +=cut + +sub ValidFormatter { + my $self = shift; + my $format = shift; + return (grep { $_ eq $format } $self->Formatters and $self->can($format)) + ? 1 : 0; +} + =head3 DefaultFormat =cut @@ -704,8 +722,8 @@ sub LocalizedDateTime my %args = ( Date => 1, Time => 1, Timezone => '', - DateFormat => 'date_format_full', - TimeFormat => 'time_format_medium', + DateFormat => '', + TimeFormat => '', AbbrDay => 1, AbbrMonth => 1, @_, @@ -716,9 +734,12 @@ sub LocalizedDateTime return $self->loc("DateTime doesn't support format_cldr, you must upgrade to use this feature") unless can DateTime::('format_cldr'); + # Require valid names for the format methods + my $date_format = $args{DateFormat} =~ /^\w+$/ + ? $args{DateFormat} : 'date_format_full'; - my $date_format = $args{'DateFormat'}; - my $time_format = $args{'TimeFormat'}; + my $time_format = $args{TimeFormat} =~ /^\w+$/ + ? $args{TimeFormat} : 'time_format_medium'; my $lang = $self->CurrentUser->UserObj->Lang; unless ($lang) { diff --git a/rt/lib/RT/Graph/Tickets.pm b/rt/lib/RT/Graph/Tickets.pm index cab429910..25cc1cbcb 100644 --- a/rt/lib/RT/Graph/Tickets.pm +++ b/rt/lib/RT/Graph/Tickets.pm @@ -104,7 +104,7 @@ EOT sub gv_escape($) { my $value = shift; - $value =~ s{(?=")}{\\}g; + $value =~ s{(?=["\\])}{\\}g; return $value; } @@ -282,6 +282,14 @@ sub TicketLinks { ShowLinkDescriptions => 0, @_ ); + + my %valid_links = map { $_ => 1 } + qw(Members MemberOf RefersTo ReferredToBy DependsOn DependedOnBy); + + # Validate our link types + $args{ShowLinks} = [ grep { $valid_links{$_} } @{$args{ShowLinks}} ]; + $args{LeadingLink} = 'Members' unless $valid_links{ $args{LeadingLink} }; + unless ( $args{'Graph'} ) { $args{'Graph'} = GraphViz->new( name => 'ticket_links_'. $args{'Ticket'}->id, diff --git a/rt/lib/RT/Group_Overlay.pm b/rt/lib/RT/Group_Overlay.pm index 09f30822f..34f8c0b7d 100644 --- a/rt/lib/RT/Group_Overlay.pm +++ b/rt/lib/RT/Group_Overlay.pm @@ -1335,8 +1335,18 @@ sub CurrentUserHasRight { # }}} +=head2 CurrentUserCanSee +Always returns 1; unfortunately, for historical reasons, users have +always been able to examine groups they have indirect access to, even if +they do not have SeeGroup explicitly. +=cut + +sub CurrentUserCanSee { + my $self = shift; + return 1; +} # {{{ Principal related routines diff --git a/rt/lib/RT/Groups_Overlay.pm b/rt/lib/RT/Groups_Overlay.pm index fa39e8c3e..a7b84a435 100644 --- a/rt/lib/RT/Groups_Overlay.pm +++ b/rt/lib/RT/Groups_Overlay.pm @@ -263,6 +263,8 @@ sub WithMember { ALIAS2 => $members, FIELD2 => 'GroupId'); $self->Limit(ALIAS => $members, FIELD => 'MemberId', OPERATOR => '=', VALUE => $args{'PrincipalId'}); + $self->Limit(ALIAS => $members, FIELD => 'Disabled', VALUE => 0) + if $args{'Recursively'}; } sub WithoutMember { @@ -288,6 +290,12 @@ sub WithoutMember { VALUE => $args{'PrincipalId'}, ); $self->Limit( + LEFTJOIN => $members_alias, + ALIAS => $members_alias, + FIELD => 'Disabled', + VALUE => 0 + ) if $args{'Recursively'}; + $self->Limit( ALIAS => $members_alias, FIELD => 'MemberId', OPERATOR => 'IS', diff --git a/rt/lib/RT/Handle.pm b/rt/lib/RT/Handle.pm index 38905de83..bb6142948 100644 --- a/rt/lib/RT/Handle.pm +++ b/rt/lib/RT/Handle.pm @@ -239,8 +239,9 @@ sub CheckIntegrity { return (0, 'no connection', "Failed to connect to $dsn as user '$user': ". $DBI::errstr); } - RT::ConnectToDatabase(); - RT::InitLogging(); + unless ($RT::Handle and $RT::Handle->dbh) { + RT::ConnectToDatabase(); + } require RT::CurrentUser; my $test_user = new RT::CurrentUser; diff --git a/rt/lib/RT/Interface/Email.pm b/rt/lib/RT/Interface/Email.pm index 9216887cd..37b1545d5 100755 --- a/rt/lib/RT/Interface/Email.pm +++ b/rt/lib/RT/Interface/Email.pm @@ -57,6 +57,7 @@ use RT::EmailParser; use File::Temp; use UNIVERSAL::require; use Mail::Mailer (); +use Text::ParseWords qw/shellwords/; BEGIN { use base 'Exporter'; @@ -404,7 +405,7 @@ sub SendEmail { if ( $mail_command eq 'sendmailpipe' ) { my $path = RT->Config->Get('SendmailPath'); - my $args = RT->Config->Get('SendmailArguments'); + my @args = shellwords(RT->Config->Get('SendmailArguments')); # SetOutgoingMailFrom if ( RT->Config->Get('SetOutgoingMailFrom') ) { @@ -423,12 +424,13 @@ sub SendEmail { $OutgoingMailAddress ||= RT->Config->Get('OverrideOutgoingMailFrom')->{'Default'}; - $args .= " -f $OutgoingMailAddress" + push @args, "-f", $OutgoingMailAddress if $OutgoingMailAddress; } # Set Bounce Arguments - $args .= ' '. RT->Config->Get('SendmailBounceArguments') if $args{'Bounce'}; + push @args, shellwords(RT->Config->Get('SendmailBounceArguments')) + if $args{'Bounce'}; # VERP if ( $TransactionObj and @@ -438,32 +440,44 @@ sub SendEmail { my $from = $TransactionObj->CreatorObj->EmailAddress; $from =~ s/@/=/g; $from =~ s/\s//g; - $args .= " -f $prefix$from\@$domain"; + push @args, "-f", "$prefix$from\@$domain"; } eval { # don't ignore CHLD signal to get proper exit code local $SIG{'CHLD'} = 'DEFAULT'; - open( my $mail, '|-', "$path $args >/dev/null" ) - or die "couldn't execute program: $!"; - # if something wrong with $mail->print we will get PIPE signal, handle it local $SIG{'PIPE'} = sub { die "program unexpectedly closed pipe" }; + + # Make it look to open2 like STDIN is on FD 0, like it + # should be; this is necessary because under mod_perl with + # the perl-script handler, it's not. This causes our + # child's "STDIN" (FD 10-ish) to be set to the pipe we want, + # but FD 0 (which the exec'd sendmail assumes is STDIN) is + # still open to /dev/null; this ends disasterously. + local *STDIN = IO::Handle->new_from_fd( 0, "r" ); + + require IPC::Open2; + my ($mail, $stdout); + my $pid = IPC::Open2::open2( $stdout, $mail, $path, @args ) + or die "couldn't execute program: $!"; + $args{'Entity'}->print($mail); + close $mail or die "close pipe failed: $!"; - unless ( close $mail ) { - die "close pipe failed: $!" if $!; # system error + waitpid($pid, 0); + if ($?) { # sendmail exit statuses mostly errors with data not software # TODO: status parsing: core dump, exit on signal or EX_* - my $msg = "$msgid: `$path $args` exitted with code ". ($?>>8); + my $msg = "$msgid: `$path @args` exited with code ". ($?>>8); $msg = ", interrupted by signal ". ($?&127) if $?&127; $RT::Logger->error( $msg ); die $msg; } }; if ( $@ ) { - $RT::Logger->crit( "$msgid: Could not send mail with command `$path $args`: " . $@ ); + $RT::Logger->crit( "$msgid: Could not send mail with command `$path @args`: " . $@ ); if ( $TicketObj ) { _RecordSendEmailFailure( $TicketObj ); } diff --git a/rt/lib/RT/Interface/Web.pm b/rt/lib/RT/Interface/Web.pm index e4167e4cc..aafca1a75 100644 --- a/rt/lib/RT/Interface/Web.pm +++ b/rt/lib/RT/Interface/Web.pm @@ -110,6 +110,25 @@ sub EscapeURI { # }}} +sub _encode_surrogates { + my $uni = $_[0] - 0x10000; + return ($uni / 0x400 + 0xD800, $uni % 0x400 + 0xDC00); +} + +sub EscapeJS { + my $ref = shift; + return unless defined $$ref; + + $$ref = "'" . join('', + map { + chr($_) =~ /[a-zA-Z0-9]/ ? chr($_) : + $_ <= 255 ? sprintf("\\x%02X", $_) : + $_ <= 65535 ? sprintf("\\u%04X", $_) : + sprintf("\\u%X\\u%X", _encode_surrogates($_)) + } unpack('U*', $$ref)) + . "'"; +} + # {{{ WebCanonicalizeInfo =head2 WebCanonicalizeInfo(); @@ -235,6 +254,8 @@ sub HandleRequest { } } + MaybeShowInterstitialCSRFPage($ARGS); + # now it applies not only to home page, but any dashboard that can be used as a workspace $HTML::Mason::Commands::session{'home_refresh_interval'} = $ARGS->{'HomeRefreshInterval'} if ( $ARGS->{'HomeRefreshInterval'} ); @@ -291,8 +312,6 @@ sub SetNextPage { $HTML::Mason::Commands::session{'NextPage'}->{$hash} = $next; $HTML::Mason::Commands::session{'i'}++; - - SendSessionCookie(); return $hash; } @@ -409,7 +428,6 @@ sub MaybeShowNoAuthPage { if $m->base_comp->path eq '/NoAuth/Login.html' and _UserLoggedIn(); # If it's a noauth file, don't ask for auth. - SendSessionCookie(); $m->comp( { base_comp => $m->request_comp }, $m->fetch_next, %$ARGS ); $m->abort; } @@ -436,7 +454,7 @@ sub MaybeRejectPrivateComponentRequest { _elements | # mobile UI Widgets | autohandler | # requesting this directly is suspicious - l ) # loc component + l (_unsafe)? ) # loc component ( $ | / ) # trailing slash or end of path }xi && $path !~ m{ /RTx/Statistics/\w+/Elements/Chart }xi @@ -462,6 +480,8 @@ sub ShowRequestedPage { my $m = $HTML::Mason::Commands::m; + # Ensure that the cookie that we send is up-to-date, in case the + # session-id has been modified in any way SendSessionCookie(); # If the user isn't privileged, they can only see SelfService @@ -602,7 +622,6 @@ sub AttemptPasswordAuthentication { InstantiateNewSession(); $HTML::Mason::Commands::session{'CurrentUser'} = $user_obj; - SendSessionCookie(); $m->callback( %$ARGS, CallbackName => 'SuccessfulLogin', CallbackPage => '/autohandler' ); @@ -657,6 +676,7 @@ sub LoadSessionFromCookie { sub InstantiateNewSession { tied(%HTML::Mason::Commands::session)->delete if tied(%HTML::Mason::Commands::session); tie %HTML::Mason::Commands::session, 'RT::Interface::Web::Session', undef; + SendSessionCookie(); } sub SendSessionCookie { @@ -738,6 +758,10 @@ sub StaticFileHeaders { # make cache public $HTML::Mason::Commands::r->headers_out->{'Cache-Control'} = 'max-age=259200, public'; + # remove any cookie headers -- if it is cached publicly, it + # shouldn't include anyone's cookie! + delete $HTML::Mason::Commands::r->err_headers_out->{'Set-Cookie'}; + # Expire things in a month. $date->Set( Value => time + 30 * 24 * 60 * 60 ); $HTML::Mason::Commands::r->headers_out->{'Expires'} = $date->RFC2616; @@ -749,6 +773,22 @@ sub StaticFileHeaders { # $HTML::Mason::Commands::r->headers_out->{'Last-Modified'} = $date->RFC2616; } +=head2 ComponentPathIsSafe PATH + +Takes C<PATH> and returns a boolean indicating that the user-specified partial +component path is safe. + +Currently "safe" means that the path does not start with a dot (C<.>) and does +not contain a slash-dot C</.>. + +=cut + +sub ComponentPathIsSafe { + my $self = shift; + my $path = shift; + return $path !~ m{(?:^|/)\.}; +} + =head2 PathIsSafe Takes a C<< Path => path >> and returns a boolean indicating that @@ -990,6 +1030,214 @@ sub LogRecordedSQLStatements { } +our %is_whitelisted_component = ( + # The RSS feed embeds an auth token in the path, but query + # information for the search. Because it's a straight-up read, in + # addition to embedding its own auth, it's fine. + '/NoAuth/rss/dhandler' => 1, +); + +sub IsCompCSRFWhitelisted { + my $comp = shift; + my $ARGS = shift; + + return 1 if $is_whitelisted_component{$comp}; + + my %args = %{ $ARGS }; + + # If the user specifies a *correct* user and pass then they are + # golden. This acts on the presumption that external forms may + # hardcode a username and password -- if a malicious attacker knew + # both already, CSRF is the least of your problems. + my $AllowLoginCSRF = not RT->Config->Get('RestrictReferrerLogin'); + if ($AllowLoginCSRF and defined($args{user}) and defined($args{pass})) { + my $user_obj = RT::CurrentUser->new(); + $user_obj->Load($args{user}); + return 1 if $user_obj->id && $user_obj->IsPassword($args{pass}); + + delete $args{user}; + delete $args{pass}; + } + + # Eliminate arguments that do not indicate an effectful request. + # For example, "id" is acceptable because that is how RT retrieves a + # record. + delete $args{id}; + + # If they have a valid results= from MaybeRedirectForResults, that's + # also fine. + delete $args{results} if $args{results} + and $HTML::Mason::Commands::session{"Actions"}->{$args{results}}; + + # The homepage refresh, which uses the Refresh header, doesn't send + # a referer in most browsers; whitelist the one parameter it reloads + # with, HomeRefreshInterval, which is safe + delete $args{HomeRefreshInterval}; + + # If there are no arguments, then it's likely to be an idempotent + # request, which are not susceptible to CSRF + return 1 if !%args; + + return 0; +} + +sub IsRefererCSRFWhitelisted { + my $referer = _NormalizeHost(shift); + my $base_url = _NormalizeHost(RT->Config->Get('WebBaseURL')); + $base_url = $base_url->host_port; + + my $configs; + for my $config ( $base_url, RT->Config->Get('ReferrerWhitelist') ) { + push @$configs,$config; + return 1 if $referer->host_port eq $config; + } + + return (0,$referer,$configs); +} + +=head3 _NormalizeHost + +Takes a URI and creates a URI object that's been normalized +to handle common problems such as localhost vs 127.0.0.1 + +=cut + +sub _NormalizeHost { + + my $uri= URI->new(shift); + $uri->host('127.0.0.1') if $uri->host eq 'localhost'; + + return $uri; + +} + +sub IsPossibleCSRF { + my $ARGS = shift; + + # If first request on this session is to a REST endpoint, then + # whitelist the REST endpoints -- and explicitly deny non-REST + # endpoints. We do this because using a REST cookie in a browser + # would open the user to CSRF attacks to the REST endpoints. + my $comp = $HTML::Mason::Commands::m->request_comp->path; + $HTML::Mason::Commands::session{'REST'} = $comp =~ m{^/REST/\d+\.\d+/} + unless defined $HTML::Mason::Commands::session{'REST'}; + + if ($HTML::Mason::Commands::session{'REST'}) { + return 0 if $comp =~ m{^/REST/\d+\.\d+/}; + my $why = <<EOT; +This login session belongs to a REST client, and cannot be used to +access non-REST interfaces of RT for security reasons. +EOT + my $details = <<EOT; +Please log out and back in to obtain a session for normal browsing. If +you understand the security implications, disabling RT's CSRF protection +will remove this restriction. +EOT + chomp $details; + HTML::Mason::Commands::Abort( $why, Details => $details ); + } + + return 0 if IsCompCSRFWhitelisted( $comp, $ARGS ); + + # if there is no Referer header then assume the worst + return (1, + "your browser did not supply a Referrer header", # loc + ) if !$ENV{HTTP_REFERER}; + + my ($whitelisted, $browser, $configs) = IsRefererCSRFWhitelisted($ENV{HTTP_REFERER}); + return 0 if $whitelisted; + + if ( @$configs > 1 ) { + return (1, + "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2]) or whitelisted hosts ([_3])", # loc + $browser->host_port, + shift @$configs, + join(', ', @$configs) ); + } + + return (1, + "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2])", # loc + $browser->host_port, + $configs->[0]); +} + +sub ExpandCSRFToken { + my $ARGS = shift; + + my $token = delete $ARGS->{CSRF_Token}; + return unless $token; + + my $data = $HTML::Mason::Commands::session{'CSRF'}{$token}; + return unless $data; + return unless $data->{uri} eq $HTML::Mason::Commands::r->uri; + + my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj; + return unless $user->ValidateAuthString( $data->{auth}, $token ); + + %{$ARGS} = %{$data->{args}}; + + # We explicitly stored file attachments with the request, but not in + # the session yet, as that would itself be an attack. Put them into + # the session now, so they'll be visible. + if ($data->{attach}) { + my $filename = $data->{attach}{filename}; + my $mime = $data->{attach}{mime}; + $HTML::Mason::Commands::session{'Attachments'}{$filename} + = $mime; + } + + return 1; +} + +sub StoreRequestToken { + my $ARGS = shift; + + my $token = Digest::MD5::md5_hex(time . {} . $$ . rand(1024)); + my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj; + my $data = { + auth => $user->GenerateAuthString( $token ), + uri => $HTML::Mason::Commands::r->uri, + args => $ARGS, + }; + if ($ARGS->{Attach}) { + my $attachment = HTML::Mason::Commands::MakeMIMEEntity( AttachmentFieldName => 'Attach' ); + my $file_path = delete $ARGS->{'Attach'}; + $data->{attach} = { + filename => Encode::decode_utf8("$file_path"), + mime => $attachment, + }; + } + + $HTML::Mason::Commands::session{'CSRF'}->{$token} = $data; + $HTML::Mason::Commands::session{'i'}++; + return $token; +} + +sub MaybeShowInterstitialCSRFPage { + my $ARGS = shift; + + return unless RT->Config->Get('RestrictReferrer'); + + # Deal with the form token provided by the interstitial, which lets + # browsers which never set referer headers still use RT, if + # painfully. This blows values into ARGS + return if ExpandCSRFToken($ARGS); + + my ($is_csrf, $msg, @loc) = IsPossibleCSRF($ARGS); + return if !$is_csrf; + + $RT::Logger->notice("Possible CSRF: ".RT::CurrentUser->new->loc($msg, @loc)); + + my $token = StoreRequestToken($ARGS); + $HTML::Mason::Commands::m->comp( + '/Elements/CSRF', + OriginalURL => $HTML::Mason::Commands::r->uri, + Reason => HTML::Mason::Commands::loc( $msg, @loc ), + Token => $token, + ); + # Calls abort, never gets here +} + package HTML::Mason::Commands; use vars qw/$r $m %session/; @@ -1197,6 +1445,7 @@ sub CreateTicket { my $cfid = $1; my $cf = RT::CustomField->new( $session{'CurrentUser'} ); + $cf->SetContextObject( $Queue ); $cf->Load($cfid); unless ( $cf->id ) { $RT::Logger->error( "Couldn't load custom field #" . $cfid ); @@ -1817,6 +2066,7 @@ sub ProcessObjectCustomFieldUpdates { foreach my $cf ( keys %{ $custom_fields_to_mod{$class}{$id} } ) { my $CustomFieldObj = RT::CustomField->new( $session{'CurrentUser'} ); + $CustomFieldObj->SetContextObject($Object); $CustomFieldObj->LoadById($cf); unless ( $CustomFieldObj->id ) { $RT::Logger->warning("Couldn't load custom field #$cf"); @@ -2322,6 +2572,91 @@ sub _parse_saved_search { return ( _load_container_object( $obj_type, $obj_id ), $search_id ); } +=head2 ScrubHTML content + +Removes unsafe and undesired HTML from the passed content + +=cut + +my $SCRUBBER; +sub ScrubHTML { + my $Content = shift; + $SCRUBBER = _NewScrubber() unless $SCRUBBER; + + $Content = '' if !defined($Content); + return $SCRUBBER->scrub($Content); +} + +=head2 _NewScrubber + +Returns a new L<HTML::Scrubber> object. + +If you need to be more lax about what HTML tags and attributes are allowed, +create C</opt/rt4/local/lib/RT/Interface/Web_Local.pm> with something like the +following: + + package HTML::Mason::Commands; + # Let tables through + push @SCRUBBER_ALLOWED_TAGS, qw(TABLE THEAD TBODY TFOOT TR TD TH); + 1; + +=cut + +our @SCRUBBER_ALLOWED_TAGS = qw( + A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5 + H6 DIV UL OL LI DL DT DD PRE BLOCKQUOTE +); + +our %SCRUBBER_ALLOWED_ATTRIBUTES = ( + # Match http, ftp and relative urls + # XXX: we also scrub format strings with this module then allow simple config options + href => qr{^(?:http:|ftp:|https:|/|__Web(?:Path|BaseURL|URL)__)}i, + face => 1, + size => 1, + target => 1, + style => qr{ + ^(?:\s* + (?:(?:background-)?color: \s* + (?:rgb\(\s* \d+, \s* \d+, \s* \d+ \s*\) | # rgb(d,d,d) + \#[a-f0-9]{3,6} | # #fff or #ffffff + [\w\-]+ # green, light-blue, etc. + ) | + text-align: \s* \w+ | + font-size: \s* [\w.\-]+ | + font-family: \s* [\w\s"',.\-]+ | + font-weight: \s* [\w\-]+ | + + # MS Office styles, which are probably fine. If we don't, then any + # associated styles in the same attribute get stripped. + mso-[\w\-]+?: \s* [\w\s"',.\-]+ + )\s* ;? \s*) + +$ # one or more of these allowed properties from here 'till sunset + }ix, +); + +our %SCRUBBER_RULES = (); + +sub _NewScrubber { + require HTML::Scrubber; + my $scrubber = HTML::Scrubber->new(); + $scrubber->default( + 0, + { + %SCRUBBER_ALLOWED_ATTRIBUTES, + '*' => 0, # require attributes be explicitly allowed + }, + ); + $scrubber->deny(qw[*]); + $scrubber->allow(@SCRUBBER_ALLOWED_TAGS); + $scrubber->rules(%SCRUBBER_RULES); + + # Scrubbing comments is vital since IE conditional comments can contain + # arbitrary HTML and we'd pass it right on through. + $scrubber->comment(0); + + return $scrubber; +} + package RT::Interface::Web; RT::Base->_ImportOverlays(); diff --git a/rt/lib/RT/Interface/Web/Handler.pm b/rt/lib/RT/Interface/Web/Handler.pm index 4bb648451..4f28f0232 100644 --- a/rt/lib/RT/Interface/Web/Handler.pm +++ b/rt/lib/RT/Interface/Web/Handler.pm @@ -75,7 +75,7 @@ sub DefaultHandlerArgs { ( static_source => (RT->Config->Get('DevelMode') ? '0' : '1'), use_object_files => (RT->Config->Get('DevelMode') ? '0' : '1'), autoflush => 0, - error_format => (RT->Config->Get('DevelMode') ? 'html': 'brief'), + error_format => (RT->Config->Get('DevelMode') ? 'html': 'rt_error'), request_class => 'RT::Interface::Web::Request', named_component_subs => $INC{'Devel/Cover.pm'} ? 1 : 0, ) }; @@ -205,6 +205,7 @@ sub NewHandler { $handler->interp->set_escape( h => \&RT::Interface::Web::EscapeUTF8 ); $handler->interp->set_escape( u => \&RT::Interface::Web::EscapeURI ); + $handler->interp->set_escape( j => \&RT::Interface::Web::EscapeJS ); return($handler); } @@ -271,4 +272,10 @@ sub CleanupRequest { } # }}} +sub HTML::Mason::Exception::as_rt_error { + my ($self) = @_; + $RT::Logger->error( $self->full_message ); + return "An internal RT error has occurred. Your administrator can find more details in RT's log files."; +} + 1; diff --git a/rt/lib/RT/ObjectCustomFieldValue_Overlay.pm b/rt/lib/RT/ObjectCustomFieldValue_Overlay.pm index 403d216ce..d140d9303 100644 --- a/rt/lib/RT/ObjectCustomFieldValue_Overlay.pm +++ b/rt/lib/RT/ObjectCustomFieldValue_Overlay.pm @@ -175,6 +175,9 @@ content, try "LargeContent" sub Content { my $self = shift; my $content = $self->SUPER::Content; + + return undef unless $self->CustomFieldObj->CurrentUserHasRight('SeeCustomField'); + if ( !(defined $content && length $content) && $self->ContentType && $self->ContentType eq 'text/plain' ) { return $self->LargeContent; } else { @@ -253,11 +256,11 @@ sub _FillInTemplateURL { # special case, whole value should be an URL if ( $url =~ /^__CustomField__/ ) { my $value = $self->Content; - # protect from javascript: URLs - if ( $value =~ /^\s*javascript:/i ) { + # protect from potentially malicious URLs + if ( $value =~ /^\s*(?:javascript|data):/i ) { my $object = $self->Object; $RT::Logger->error( - "Dangerouse value with JavaScript in custom field '". $self->CustomFieldObj->Name ."'" + "Potentially dangerous URL type in custom field '". $self->CustomFieldObj->Name ."'" ." on ". ref($object) ." #". $object->id ); return undef; diff --git a/rt/lib/RT/ObjectCustomField_Overlay.pm b/rt/lib/RT/ObjectCustomField_Overlay.pm index 689d62ff5..3b7d87945 100644 --- a/rt/lib/RT/ObjectCustomField_Overlay.pm +++ b/rt/lib/RT/ObjectCustomField_Overlay.pm @@ -119,7 +119,19 @@ sub Delete { sub CustomFieldObj { my $self = shift; my $id = shift || $self->CustomField; + + # To find out the proper context object to load the CF with, we need + # data from the CF -- namely, the record class. Go find that as the + # system user first. + my $system_CF = RT::CustomField->new( RT->SystemUser ); + $system_CF->Load( $id ); + my $class = $system_CF->RecordClassFromLookupType; + + my $obj = $class->new( $self->CurrentUser ); + $obj->Load( $self->ObjectId ); + my $CF = RT::CustomField->new( $self->CurrentUser ); + $CF->SetContextObject( $obj ); $CF->Load( $id ); return $CF; } diff --git a/rt/lib/RT/Queue_Overlay.pm b/rt/lib/RT/Queue_Overlay.pm index 5245af43f..0c8f16899 100644 --- a/rt/lib/RT/Queue_Overlay.pm +++ b/rt/lib/RT/Queue_Overlay.pm @@ -661,6 +661,7 @@ sub TicketTransactionCustomFields { my $cfs = RT::CustomFields->new( $self->CurrentUser ); if ( $self->CurrentUserHasRight('SeeQueue') ) { + $cfs->SetContextObject( $self ); $cfs->LimitToGlobalOrObjectId( $self->Id ); $cfs->LimitToLookupType( 'RT::Queue-RT::Ticket-RT::Transaction' ); $cfs->ApplySortOrder; @@ -1208,6 +1209,18 @@ sub CurrentUserHasRight { # }}} +=head2 CurrentUserCanSee + +Returns true if the current user can see the queue, using SeeQueue + +=cut + +sub CurrentUserCanSee { + my $self = shift; + + return $self->CurrentUserHasRight('SeeQueue'); +} + # {{{ sub HasRight =head2 HasRight diff --git a/rt/lib/RT/Scrip_Overlay.pm b/rt/lib/RT/Scrip_Overlay.pm index e91f8d64d..c5615c52b 100644 --- a/rt/lib/RT/Scrip_Overlay.pm +++ b/rt/lib/RT/Scrip_Overlay.pm @@ -507,12 +507,42 @@ sub Commit { # does an acl check and then passes off the call sub _Set { my $self = shift; + my %args = ( + Field => undef, + Value => undef, + @_, + ); unless ( $self->CurrentUserHasRight('ModifyScrips') ) { $RT::Logger->debug( "CurrentUser can't modify Scrips for " . $self->Queue . "\n" ); return ( 0, $self->loc('Permission Denied') ); } + + if (exists $args{Value}) { + if ($args{Field} eq 'Queue') { + if ($args{Value}) { + # moving to another queue + my $queue = RT::Queue->new( $self->CurrentUser ); + $queue->Load($args{Value}); + unless ($queue->Id and $queue->CurrentUserHasRight('ModifyScrips')) { + return ( 0, $self->loc('Permission Denied') ); + } + } else { + # moving to global + unless ($self->CurrentUser->HasRight( Object => RT->System, Right => 'ModifyScrips' )) { + return ( 0, $self->loc('Permission Denied') ); + } + } + } + elsif ($args{Field} eq 'Template') { + my $template = RT::Template->new( $self->CurrentUser ); + $template->Load($args{Value}); + unless ($template->Id and $template->CurrentUserHasQueueRight('ShowTemplate')) { + return ( 0, $self->loc('Permission Denied') ); + } + } + } return $self->__Set(@_); } diff --git a/rt/lib/RT/Scrips_Overlay.pm b/rt/lib/RT/Scrips_Overlay.pm index eecf29352..5dd83b72c 100644 --- a/rt/lib/RT/Scrips_Overlay.pm +++ b/rt/lib/RT/Scrips_Overlay.pm @@ -185,6 +185,15 @@ 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( @@ -196,6 +205,9 @@ sub Commit { $scrip->Commit( TicketObj => $self->{'TicketObj'}, TransactionObj => $self->{'TransactionObj'} ); } + + # Apply the bandaid. + $self->_RestoreCurrentUser( TicketObj => $self->{TicketObj} ) if $self->{TicketObj}; } @@ -216,6 +228,12 @@ 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'}, @@ -248,6 +266,10 @@ sub Prepare { } + # Apply the bandaid. + $self->_RestoreCurrentUser( TicketObj => $args{TicketObj} ) if $args{TicketObj}; + + return (@{$self->Prepared}); }; @@ -264,6 +286,39 @@ 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}); + +} # {{{ sup _SetupSourceObjects @@ -288,9 +343,13 @@ sub _SetupSourceObjects { @_ ); - if ( $args{'TicketObj'} ) { - # clone the ticket here as we need to change CurrentUser - $self->{'TicketObj'} = bless { %{$args{'TicketObj'} } }, 'RT::Ticket'; + 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 ); } else { diff --git a/rt/lib/RT/SearchBuilder.pm b/rt/lib/RT/SearchBuilder.pm index ec4a223c0..405a8fdc0 100644 --- a/rt/lib/RT/SearchBuilder.pm +++ b/rt/lib/RT/SearchBuilder.pm @@ -96,6 +96,19 @@ sub OrderByCols { return $self->SUPER::OrderByCols( @sort ); } +# If we're setting RowsPerPage or FirstRow, ensure we get a natural number or undef. +sub RowsPerPage { + my $self = shift; + return if @_ and defined $_[0] and $_[0] =~ /\D/; + return $self->SUPER::RowsPerPage(@_); +} + +sub FirstRow { + my $self = shift; + return if @_ and defined $_[0] and $_[0] =~ /\D/; + return $self->SUPER::FirstRow(@_); +} + =head2 LimitToEnabled Only find items that haven't been disabled diff --git a/rt/lib/RT/Shredder.pm b/rt/lib/RT/Shredder.pm index 024a50ba4..477a9f24e 100644 --- a/rt/lib/RT/Shredder.pm +++ b/rt/lib/RT/Shredder.pm @@ -349,6 +349,8 @@ sub CastObjectsToRecords } elsif ( UNIVERSAL::isa( $targets, 'SCALAR' ) || !ref $targets ) { $targets = $$targets if ref $targets; my ($class, $id) = split /-/, $targets; + RT::Shredder::Exception->throw( "Unsupported class $class" ) + unless $class =~ /^\w+(::\w+)*$/; $class = 'RT::'. $class unless $class =~ /^RTx?::/i; eval "require $class"; die "Couldn't load '$class' module" if $@; diff --git a/rt/lib/RT/Shredder/Plugin.pm b/rt/lib/RT/Shredder/Plugin.pm index b7c63ec7f..1b104ff27 100644 --- a/rt/lib/RT/Shredder/Plugin.pm +++ b/rt/lib/RT/Shredder/Plugin.pm @@ -167,6 +167,7 @@ sub LoadByName { my $self = shift; my $name = shift or return (0, "Name not specified"); + $name =~ /^\w+(::\w+)*$/ or return (0, "Invalid plugin name"); local $@; my $plugin = "RT::Shredder::Plugin::$name"; diff --git a/rt/lib/RT/Shredder/Queue.pm b/rt/lib/RT/Shredder/Queue.pm index 8ee109490..79b67d1a9 100644 --- a/rt/lib/RT/Shredder/Queue.pm +++ b/rt/lib/RT/Shredder/Queue.pm @@ -91,6 +91,7 @@ sub __DependsOn # Custom Fields $objs = RT::CustomFields->new( $self->CurrentUser ); + $objs->SetContextObject( $self ); $objs->LimitToQueue( $self->id ); push( @$list, $objs ); diff --git a/rt/lib/RT/Template_Overlay.pm b/rt/lib/RT/Template_Overlay.pm index e6f6374ee..21cb97a9f 100644 --- a/rt/lib/RT/Template_Overlay.pm +++ b/rt/lib/RT/Template_Overlay.pm @@ -94,10 +94,34 @@ sub _Accessible { sub _Set { my $self = shift; + my %args = ( + Field => undef, + Value => undef, + @_, + ); unless ( $self->CurrentUserHasQueueRight('ModifyTemplate') ) { return ( 0, $self->loc('Permission Denied') ); } + + if (exists $args{Value}) { + if ($args{Field} eq 'Queue') { + if ($args{Value}) { + # moving to another queue + my $queue = RT::Queue->new( $self->CurrentUser ); + $queue->Load($args{Value}); + unless ($queue->Id and $queue->CurrentUserHasRight('ModifyTemplate')) { + return ( 0, $self->loc('Permission Denied') ); + } + } else { + # moving to global + unless ($self->CurrentUser->HasRight( Object => RT->System, Right => 'ModifyTemplate' )) { + return ( 0, $self->loc('Permission Denied') ); + } + } + } + } + return $self->SUPER::_Set( @_ ); } diff --git a/rt/lib/RT/Ticket_Overlay.pm b/rt/lib/RT/Ticket_Overlay.pm index e8cb12863..2d23c2376 100644 --- a/rt/lib/RT/Ticket_Overlay.pm +++ b/rt/lib/RT/Ticket_Overlay.pm @@ -1305,7 +1305,7 @@ sub DeleteWatcher { } } else { - $RT::Logger->warn("$self -> DeleteWatcher got passed a bogus type"); + $RT::Logger->warning("$self -> DeleteWatcher got passed a bogus type"); return ( 0, $self->loc('Error in parameters to Ticket->DeleteWatcher') ); } @@ -2311,7 +2311,7 @@ sub _Links { my $links = $self->{ $cache_key } = RT::Links->new( $self->CurrentUser ); unless ( $self->CurrentUserHasRight('ShowTicket') ) { - $links->Limit( FIELD => 'id', VALUE => 0 ); + $links->Limit( FIELD => 'id', VALUE => 0, SUBCLAUSE => 'acl' ); return $links; } @@ -3547,6 +3547,17 @@ sub CurrentUserHasRight { # }}} +=head2 CurrentUserCanSee + +Returns true if the current user can see the ticket, using ShowTicket + +=cut + +sub CurrentUserCanSee { + my $self = shift; + return $self->CurrentUserHasRight('ShowTicket'); +} + # {{{ sub HasRight =head2 HasRight @@ -3665,7 +3676,9 @@ sub Transactions { sub TransactionCustomFields { my $self = shift; - return $self->QueueObj->TicketTransactionCustomFields; + my $cfs = $self->QueueObj->TicketTransactionCustomFields; + $cfs->SetContextObject( $self ); + return $cfs; } # }}} diff --git a/rt/lib/RT/Tickets_Overlay.pm b/rt/lib/RT/Tickets_Overlay.pm index a5d37a378..bc553473a 100644 --- a/rt/lib/RT/Tickets_Overlay.pm +++ b/rt/lib/RT/Tickets_Overlay.pm @@ -1069,6 +1069,12 @@ sub _GroupMembersJoin { FIELD2 => 'GroupId', ENTRYAGGREGATOR => 'AND', ); + $self->SUPER::Limit( + LEFTJOIN => $alias, + ALIAS => $alias, + FIELD => 'Disabled', + VALUE => 0, + ); $self->{'_sql_group_members_aliases'}{ $args{'GroupsAlias'} } = $alias unless $args{'New'}; @@ -1233,6 +1239,12 @@ sub _WatcherMembershipLimit { FIELD2 => 'id' ); + $self->Limit( + ALIAS => $groupmembers, + FIELD => 'Disabled', + VALUE => 0, + ); + $self->Join( ALIAS1 => $memberships, FIELD1 => 'MemberId', @@ -1240,6 +1252,13 @@ sub _WatcherMembershipLimit { FIELD2 => 'id' ); + $self->Limit( + ALIAS => $memberships, + FIELD => 'Disabled', + VALUE => 0, + ); + + $self->_CloseParen; } diff --git a/rt/lib/RT/Transaction_Overlay.pm b/rt/lib/RT/Transaction_Overlay.pm index fdd3e948f..5732964d0 100644 --- a/rt/lib/RT/Transaction_Overlay.pm +++ b/rt/lib/RT/Transaction_Overlay.pm @@ -512,7 +512,7 @@ sub Attachments { $self->{'attachments'} = RT::Attachments->new( $self->CurrentUser ); unless ( $self->CurrentUserCanSee ) { - $self->{'attachments'}->Limit(FIELD => 'id', VALUE => '0'); + $self->{'attachments'}->Limit(FIELD => 'id', VALUE => '0', SUBCLAUSE => 'acl'); return $self->{'attachments'}; } @@ -734,6 +734,7 @@ sub BriefDescription { if ( $self->Field ) { my $cf = RT::CustomField->new( $self->CurrentUser ); + $cf->SetContextObject( $self->Object ); $cf->Load( $self->Field ); $field = $cf->Name(); } @@ -1068,14 +1069,8 @@ sub CurrentUserCanSee { $cf->Load( $cf_id ); return 0 unless $cf->CurrentUserHasRight('SeeCustomField'); } - #if they ain't got rights to see, don't let em - elsif ( $self->__Value('ObjectType') eq "RT::Ticket" ) { - unless ( $self->CurrentUserHasRight('ShowTicket') ) { - return 0; - } - } - - return 1; + # Defer to the object in question + return $self->Object->CurrentUserCanSee("Transaction"); } # }}} @@ -1100,7 +1095,7 @@ sub OldValue { return $Object->Content; } else { - return $self->__Value('OldValue'); + return $self->_Value('OldValue'); } } @@ -1114,7 +1109,7 @@ sub NewValue { return $Object->Content; } else { - return $self->__Value('NewValue'); + return $self->_Value('NewValue'); } } @@ -1204,6 +1199,7 @@ sub CustomFieldValues { # do we want to cover this situation somehow here? unless ( defined $field && $field =~ /^\d+$/o ) { my $CFs = RT::CustomFields->new( $self->CurrentUser ); + $CFs->SetContextObject( $self->Object ); $CFs->Limit( FIELD => 'Name', VALUE => $field ); $CFs->LimitToLookupType($self->CustomFieldLookupType); $CFs->LimitToGlobalOrObjectId($self->Object->QueueObj->id); diff --git a/rt/lib/RT/URI.pm b/rt/lib/RT/URI.pm index facce0413..03489ad1a 100644 --- a/rt/lib/RT/URI.pm +++ b/rt/lib/RT/URI.pm @@ -132,7 +132,7 @@ sub FromURI { # Special case: integers passed in as URIs must be ticket ids if ($uri =~ /^(\d+)$/) { $scheme = "fsck.com-rt"; - } elsif ($uri =~ /^((?:\w|\.|-)+?):/) { + } elsif ($uri =~ /^((?!javascript|data)(?:\w|\.|-)+?):/i) { $scheme = $1; } else { diff --git a/rt/lib/RT/User_Overlay.pm b/rt/lib/RT/User_Overlay.pm index 37d138901..2b50fac82 100644 --- a/rt/lib/RT/User_Overlay.pm +++ b/rt/lib/RT/User_Overlay.pm @@ -1090,7 +1090,7 @@ sub IsPassword { # crypt() output return 0 unless crypt(encode_utf8($value), $stored) eq $stored; } else { - $RT::Logger->warn("Unknown password form"); + $RT::Logger->warning("Unknown password form"); return 0; } @@ -1622,6 +1622,37 @@ sub HasRight { return $self->PrincipalObj->HasRight(@_); } +=head2 CurrentUserCanSee [FIELD] + +Returns true if the current user can see the user, based on if it is +public, ourself, or we have AdminUsers + +=cut + +sub CurrentUserCanSee { + my $self = shift; + my ($what) = @_; + + # If it's public, fine. Note that $what may be "transaction", which + # doesn't have an Accessible value, and thus falls through below. + if ( $self->_Accessible( $what, 'public' ) ) { + return 1; + } + + # Users can see their own properties + elsif ( defined($self->Id) && $self->CurrentUser->Id == $self->Id ) { + return 1; + } + + # If the user has the admin users right, that's also enough + elsif ( $self->CurrentUser->HasRight( Right => 'AdminUsers', Object => $RT::System) ) { + return 1; + } + else { + return 0; + } +} + =head2 CurrentUserCanModify RIGHT If the user has rights for this object, either because @@ -1800,6 +1831,12 @@ sub WatchedQueues { FIELD => 'MemberId', VALUE => $self->PrincipalId, ); + $watched_queues->Limit( + ALIAS => $queues_alias, + FIELD => 'Disabled', + VALUE => 0, + ); + $RT::Logger->debug("WatchedQueues got " . $watched_queues->Count . " queues"); @@ -1908,7 +1945,9 @@ sub _Set { if ( $ret == 0 ) { return ( 0, $msg ); } if ( $args{'RecordTransaction'} == 1 ) { - + if ($args{'Field'} eq "Password") { + $args{'Value'} = $Old = '********'; + } my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction( Type => $args{'TransactionType'}, Field => $args{'Field'}, @@ -1935,33 +1974,9 @@ sub _Value { my $self = shift; my $field = shift; - #If the current user doesn't have ACLs, don't let em at it. - - my @PublicFields = qw( Name EmailAddress Organization Disabled - RealName NickName Gecos ExternalAuthId - AuthSystem ExternalContactInfoId - ContactInfoSystem ); - - #if the field is public, return it. - if ( $self->_Accessible( $field, 'public' ) ) { - return ( $self->SUPER::_Value($field) ); - - } - - #If the user wants to see their own values, let them - # TODO figure ouyt a better way to deal with this - elsif ( defined($self->Id) && $self->CurrentUser->Id == $self->Id ) { - return ( $self->SUPER::_Value($field) ); - } - - #If the user has the admin users right, return the field - elsif ( $self->CurrentUser->HasRight(Right =>'AdminUsers', Object => $RT::System) ) { - return ( $self->SUPER::_Value($field) ); - } - else { - return (undef); - } - + # Defer to the abstraction above to know if the field can be read + return $self->SUPER::_Value($field) if $self->CurrentUserCanSee($field); + return undef; } =head2 FriendlyName diff --git a/rt/lib/RT/Users_Overlay.pm b/rt/lib/RT/Users_Overlay.pm index 16ec5ed87..ecb42da6a 100644 --- a/rt/lib/RT/Users_Overlay.pm +++ b/rt/lib/RT/Users_Overlay.pm @@ -188,6 +188,9 @@ sub MemberOfGroup { FIELD1 => 'id', ALIAS2 => $groupalias, FIELD2 => 'MemberId' ); + $self->Limit( ALIAS => $groupalias, + FIELD => 'Disabled', + VALUE => 0 ); $self->Limit( ALIAS => "$groupalias", FIELD => 'GroupId', @@ -259,6 +262,11 @@ sub _JoinGroupMembers ALIAS2 => $principals, FIELD2 => 'id' ); + $self->Limit( + ALIAS => $group_members, + FIELD => 'Disabled', + VALUE => 0, + ) if $args{'IncludeSubgroupMembers'}; return $group_members; } |