[Rt-commit] rt branch, 4.0.0-releng, updated. rt-4.0.0rc7-65-ge77f11b
Kevin Falcone
falcone at bestpractical.com
Thu Apr 14 10:09:10 EDT 2011
The branch, 4.0.0-releng has been updated
via e77f11b09699ecc530f747d2fdc027ad331206dc (commit)
via c32b1967f8498a6abc5d683e7837c7b5ef7dbde2 (commit)
via 9b72895e7da56c497622e1d4b3d112bb95c1612c (commit)
via 88689bec08c3e93aa03aec4d9c3caf6246819a68 (commit)
via df67f7ae35f342faf55aecac7754cf942b32e83c (commit)
via f076f1babcd6fe7bb5e48fd04d05b428e24f1fc4 (commit)
via 895a4ccfe07bf20205985d194447cb892987919c (commit)
via 55270e6a59860edf0abfd9ad1cb8f0ea8cbbcfbe (commit)
via ce5c889e50780107e8815bff217f4146b01abcad (commit)
via daa0516c1b8950e20a697c927fe975b1763bd4d3 (commit)
via dacf74182d03d26d439351ce1a2fcfdfe2d714fc (commit)
via 86812b5c0b27984cc0ed4bd086fe8a17f1b7644e (commit)
via 2dcacd03350c5664855cda54c46bc8f8e8eaa296 (commit)
via 4c1be2c8ffee6fe69357efc16a4ab055955abb4c (commit)
via 2bf2ff20926304713031224ddc47ee501cdbada6 (commit)
via 34d86395c8d1351484390815e55f28b8d6974aa7 (commit)
via 54d03a7f6e0622c50b53117eb005638a874d461c (commit)
via a410481b08a7897f7d3c567ffb45cf985c2ec8ed (commit)
via f177355bc3858256c7ad0a47ccec6e14cd861c3d (commit)
via bd0ebe51688df364ac11b63728b771b67eb09f09 (commit)
via 007046d1c5bc9392cccdfa1ebb8e968e1d674b80 (commit)
via 104271918a70389a2f1f824451083555666fb79f (commit)
via d9a4f208930e7ca1f54d4f3dd7b578977715ae99 (commit)
via fd3cbb5caa64b5f206811ff01bb5aacd736412c8 (commit)
via 0e5e4222cedd91ad7423c03baf36cf447f85d356 (commit)
via 4be435eb9b1160f517e69d28a50859892024de55 (commit)
via 70fdf5ce99bd16d6838035afb514a538a5ca5d79 (commit)
via 0d40bae17fd2d6d12b2d896f038527880c4a0963 (commit)
via b8609fbafd8797100e47b3bf0d6cd556eba3c161 (commit)
via 75d1edd176cfa33db49421de2375c4abfac80559 (commit)
via 63979ac6895aacacbaf88ca0d7f276802517b391 (commit)
via e3646bc826680bb1b81ca00e581e4028368bae15 (commit)
via b6d14dc41fccb5ed1874771e6ce5b7150bec2891 (commit)
via 90041d856eb8709ff92f4a0222fde961263b08ac (commit)
via 33d44c6df2bcbda81952a8327759c3dbf2bec3ec (commit)
via cc01217166f2d554a35aeb2e60a005eba088f1a2 (commit)
via 52df246eea72348f62da5e9d6b935e249ba58be1 (commit)
via 0c0a8acbeab8a214b237aa3e61d785ec75a87031 (commit)
via 33a2809762a7edf18c8f2646eda988d140fb32da (commit)
via 8fc002641d0e2f25599db03d96a3a8171587a170 (commit)
via 86dff4a2fa8be4463c73d396f327ee672fe43117 (commit)
via 0c329f440ec58babfb40909e8c4fee6f2b3ad32f (commit)
from 8a16709443df76fa4b85c128e80654e059e9ed7c (commit)
Summary of changes:
lib/RT/CustomFieldValues/External.pm | 84 +++++++++-----------
lib/RT/Interface/Web.pm | 33 ++++++++
lib/RT/Interface/Web/Handler.pm | 9 ++
lib/RT/SearchBuilder.pm | 51 ++++++++++++-
lib/RT/Tickets.pm | 41 ++++++++--
share/html/Elements/Header | 3 +-
share/html/Helpers/Autocomplete/CustomFieldValues | 23 ++---
share/html/NoAuth/Logout.html | 3 +-
.../html/{m/logout => NoAuth/RichText/autohandler} | 10 ++-
share/html/Search/Chart | 4 +-
share/html/Search/Chart.html | 4 +-
share/html/Search/Elements/Chart | 3 +-
share/html/Search/Elements/SelectPersonType | 2 +-
t/api/tickets_overlay_sql.t | 30 +++++++-
t/web/charting.t | 69 ++++++++++++++++
t/web/compilation_errors.t | 2 +-
t/web/path-traversal.t | 40 +++++++++
t/web/private-components.t | 44 ++++++++++
t/web/query_builder.t | 29 +++++++-
t/web/richtext-autohandler.t | 13 +++
20 files changed, 411 insertions(+), 86 deletions(-)
copy share/html/{m/logout => NoAuth/RichText/autohandler} (90%)
create mode 100644 t/web/charting.t
create mode 100644 t/web/path-traversal.t
create mode 100644 t/web/private-components.t
create mode 100644 t/web/richtext-autohandler.t
- Log -----------------------------------------------------------------
commit 0c329f440ec58babfb40909e8c4fee6f2b3ad32f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Mar 25 16:16:22 2011 -0400
Tests for exposing private componets
diff --git a/t/web/private-components.t b/t/web/private-components.t
new file mode 100644
index 0000000..0d5f000
--- /dev/null
+++ b/t/web/private-components.t
@@ -0,0 +1,35 @@
+use strict;
+
+use RT::Test tests => 24;
+my ($baseurl, $agent) = RT::Test->started_ok;
+
+ok $agent->login, 'logged in';
+
+$agent->get_ok("/Elements/Refresh?Name=private");
+$agent->content_lacks("private");
+$agent->content_lacks("Refresh this page every");
+
+$agent->get_ok("/Ticket/Elements/ShowTime?minutes=42");
+$agent->content_lacks("42 min");
+
+$agent->get_ok("/Widgets/TitleBox?title=private");
+$agent->content_lacks("private");
+
+$agent->get_ok("/m/_elements/header?title=private");
+$agent->content_lacks("private");
+
+$agent->get_ok("/autohandler");
+$agent->content_lacks("comp called without component");
+
+$agent->get_ok("/NoAuth/js/autohandler");
+$agent->content_lacks("no next component");
+
+$agent->get_ok("/l");
+$agent->content_lacks("No handle/phrase");
+
+$agent->get_ok("/%61utohandler");
+$agent->content_lacks("comp called without component");
+
+$agent->get_ok("/%45lements/Refresh?Name=private");
+$agent->content_lacks("private");
+$agent->content_lacks("Refresh this page every");
commit 86dff4a2fa8be4463c73d396f327ee672fe43117
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 10:05:09 2011 -0400
All of these requests oughta result in an error code
diff --git a/t/web/private-components.t b/t/web/private-components.t
index 0d5f000..ceb2b34 100644
--- a/t/web/private-components.t
+++ b/t/web/private-components.t
@@ -5,31 +5,40 @@ my ($baseurl, $agent) = RT::Test->started_ok;
ok $agent->login, 'logged in';
-$agent->get_ok("/Elements/Refresh?Name=private");
+$agent->get("/Elements/Refresh?Name=private");
+is($agent->status, 403);
$agent->content_lacks("private");
$agent->content_lacks("Refresh this page every");
-$agent->get_ok("/Ticket/Elements/ShowTime?minutes=42");
+$agent->get("/Ticket/Elements/ShowTime?minutes=42");
+is($agent->status, 403);
$agent->content_lacks("42 min");
-$agent->get_ok("/Widgets/TitleBox?title=private");
+$agent->get("/Widgets/TitleBox?title=private");
+is($agent->status, 403);
$agent->content_lacks("private");
-$agent->get_ok("/m/_elements/header?title=private");
+$agent->get("/m/_elements/header?title=private");
+is($agent->status, 403);
$agent->content_lacks("private");
-$agent->get_ok("/autohandler");
+$agent->get("/autohandler");
+is($agent->status, 403);
$agent->content_lacks("comp called without component");
-$agent->get_ok("/NoAuth/js/autohandler");
+$agent->get("/NoAuth/js/autohandler");
+is($agent->status, 403);
$agent->content_lacks("no next component");
-$agent->get_ok("/l");
+$agent->get("/l");
+is($agent->status, 403);
$agent->content_lacks("No handle/phrase");
-$agent->get_ok("/%61utohandler");
+$agent->get("/%61utohandler");
+is($agent->status, 403);
$agent->content_lacks("comp called without component");
-$agent->get_ok("/%45lements/Refresh?Name=private");
+$agent->get("/%45lements/Refresh?Name=private");
+is($agent->status, 403);
$agent->content_lacks("private");
$agent->content_lacks("Refresh this page every");
commit 8fc002641d0e2f25599db03d96a3a8171587a170
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 10:05:45 2011 -0400
First pass at MaybeRejectPrivateComponentRequest
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 8e0b80b..05b4262 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -221,6 +221,8 @@ sub HandleRequest {
# Process session-related callbacks before any auth attempts
$HTML::Mason::Commands::m->callback( %$ARGS, CallbackName => 'Session', CallbackPage => '/autohandler' );
+ MaybeRejectPrivateComponentRequest();
+
MaybeShowNoAuthPage($ARGS);
AttemptExternalAuth($ARGS) if RT->Config->Get('WebExternalAuthContinuous') or not _UserLoggedIn();
@@ -440,6 +442,34 @@ sub MaybeShowNoAuthPage {
$m->abort;
}
+=head2 MaybeRejectPrivateComponentRequest
+
+This function will reject calls to private components, like those under
+C</Elements>. If the requested path is a private component then we will
+abort with a C<403> error.
+
+=cut
+
+sub MaybeRejectPrivateComponentRequest {
+ my $m = $HTML::Mason::Commands::m;
+ my $path = $m->base_comp->path;
+
+ if ($path =~ m{
+ / # leading slash
+ ( Elements |
+ _elements | # mobile UI
+ Widgets |
+ dhandler |
+ autohandler |
+ l ) # loc component
+ ( $ | / ) # trailing slash or end of path
+ }xi) {
+ $m->abort(403);
+ }
+
+ return;
+}
+
sub InitializeMenu {
$HTML::Mason::Commands::m->notes('menu', RT::Interface::Web::Menu->new());
$HTML::Mason::Commands::m->notes('page-menu', RT::Interface::Web::Menu->new());
commit 33a2809762a7edf18c8f2646eda988d140fb32da
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 10:35:50 2011 -0400
Use the requested path directly for private component checking
Otherwise Mason resolves the requested url into a file, and so we
see that the user "tried" to request a dhandler even if they did the
right thing and requested (say) squished CSS.
We have tests to ensure that PATH_INFO is properly sanitized and unescaped.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 05b4262..2bd1930 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -451,20 +451,19 @@ abort with a C<403> error.
=cut
sub MaybeRejectPrivateComponentRequest {
- my $m = $HTML::Mason::Commands::m;
- my $path = $m->base_comp->path;
+ my $path = $ENV{PATH_INFO};
if ($path =~ m{
/ # leading slash
( Elements |
_elements | # mobile UI
Widgets |
- dhandler |
- autohandler |
+ dhandler | # requesting this directly is suspicious
+ autohandler | # ditto
l ) # loc component
( $ | / ) # trailing slash or end of path
}xi) {
- $m->abort(403);
+ $HTML::Mason::Commands::m->abort(403);
}
return;
commit 0c0a8acbeab8a214b237aa3e61d785ec75a87031
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 11:32:00 2011 -0400
Explain why we're using PATH_INFO instead of request_comp
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 2bd1930..7dd4a0e 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -451,6 +451,14 @@ abort with a C<403> error.
=cut
sub MaybeRejectPrivateComponentRequest {
+ # We must use PATH_INFO directly because $m->base_comp and $m->request_comp
+ # are resolved to a file on disk, which is too late. This means that
+ # requesting squished CSS looks like the user requested the dhandler
+ # directly. What we really want is the URL directly requested by the user.
+ # Unfortunately we don't keep the Plack::Request around. If we did, it'd be
+ # cleaner to use $request->path_info here. Either way, PATH_INFO is
+ # unescaped for us, so requesting /%61utohandler doesn't circumvent this
+ # check. See t/web/private-components.t
my $path = $ENV{PATH_INFO};
if ($path =~ m{
commit 52df246eea72348f62da5e9d6b935e249ba58be1
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 11:35:22 2011 -0400
More explanation
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 7dd4a0e..8898ed3 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -452,13 +452,14 @@ abort with a C<403> error.
sub MaybeRejectPrivateComponentRequest {
# We must use PATH_INFO directly because $m->base_comp and $m->request_comp
- # are resolved to a file on disk, which is too late. This means that
- # requesting squished CSS looks like the user requested the dhandler
- # directly. What we really want is the URL directly requested by the user.
- # Unfortunately we don't keep the Plack::Request around. If we did, it'd be
+ # are resolved to a file on disk, which is too late, because when the user
+ # requests squished CSS, it looks like they requested the dhandler
+ # directly. What we really want is more or less the URL from the HTTP
+ # request. Unfortunately we don't keep the Plack::Request around that we
+ # construct in RT::Interface::Web::Handler::PSGIApp. If we did, it'd be
# cleaner to use $request->path_info here. Either way, PATH_INFO is
# unescaped for us, so requesting /%61utohandler doesn't circumvent this
- # check. See t/web/private-components.t
+ # check. See t/web/private-components.t.
my $path = $ENV{PATH_INFO};
if ($path =~ m{
commit cc01217166f2d554a35aeb2e60a005eba088f1a2
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Mar 28 12:08:10 2011 -0400
Use request_comp but don't check for dhandler
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 8898ed3..9b0ce4a 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -451,28 +451,23 @@ abort with a C<403> error.
=cut
sub MaybeRejectPrivateComponentRequest {
- # We must use PATH_INFO directly because $m->base_comp and $m->request_comp
- # are resolved to a file on disk, which is too late, because when the user
- # requests squished CSS, it looks like they requested the dhandler
- # directly. What we really want is more or less the URL from the HTTP
- # request. Unfortunately we don't keep the Plack::Request around that we
- # construct in RT::Interface::Web::Handler::PSGIApp. If we did, it'd be
- # cleaner to use $request->path_info here. Either way, PATH_INFO is
- # unescaped for us, so requesting /%61utohandler doesn't circumvent this
- # check. See t/web/private-components.t.
- my $path = $ENV{PATH_INFO};
+ my $m = $HTML::Mason::Commands::m;
+ my $path = $m->request_comp->path;
+
+ # We do not check for dhandler here, because requesting our dhandlers
+ # directly is okay. Mason will invoke the dhandler with a dhandler_arg of
+ # 'dhandler'.
if ($path =~ m{
/ # leading slash
( Elements |
_elements | # mobile UI
Widgets |
- dhandler | # requesting this directly is suspicious
- autohandler | # ditto
+ autohandler | # requesting this directly is suspicious
l ) # loc component
( $ | / ) # trailing slash or end of path
}xi) {
- $HTML::Mason::Commands::m->abort(403);
+ $m->abort(403);
}
return;
commit 33d44c6df2bcbda81952a8327759c3dbf2bec3ec
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Mar 28 13:50:42 2011 -0400
Remove SecondaryGroupBy, which is unused and a point of confusion
diff --git a/share/html/Search/Chart b/share/html/Search/Chart
index 429fea5..858b22e 100644
--- a/share/html/Search/Chart
+++ b/share/html/Search/Chart
@@ -48,7 +48,6 @@
<%args>
$Query => "id > 0"
$PrimaryGroupBy => 'Queue'
-$SecondaryGroupBy => undef
$ChartStyle => 'bars'
</%args>
<%init>
diff --git a/share/html/Search/Chart.html b/share/html/Search/Chart.html
index af02eab..b036565 100644
--- a/share/html/Search/Chart.html
+++ b/share/html/Search/Chart.html
@@ -47,12 +47,10 @@
%# END BPS TAGGED BLOCK }}}
<%args>
$PrimaryGroupBy => 'Queue'
-$SecondaryGroupBy => ''
$ChartStyle => 'bars'
$Description => undef
</%args>
<%init>
-$ARGS{SecondaryGroupBy} ||= '';
$ARGS{Query} ||= 'id > 0';
# FIXME: should be factored with RT::Report::Tickets::Label :(
@@ -74,7 +72,7 @@ my $title = loc( "Search results grouped by [_1]", $PrimaryGroupByLabel );
my $saved_search = $m->comp( '/Widgets/SavedSearch:new',
SearchType => 'Chart',
- SearchFields => [qw(Query PrimaryGroupBy SecondaryGroupBy ChartStyle)] );
+ SearchFields => [qw(Query PrimaryGroupBy ChartStyle)] );
my @actions = $m->comp( '/Widgets/SavedSearch:process', args => \%ARGS, self => $saved_search );
diff --git a/share/html/Search/Elements/Chart b/share/html/Search/Elements/Chart
index dae456f..6b71f5c 100644
--- a/share/html/Search/Elements/Chart
+++ b/share/html/Search/Elements/Chart
@@ -48,7 +48,6 @@
<%args>
$Query => "id > 0"
$PrimaryGroupBy => 'Queue'
-$SecondaryGroupBy => undef
$ChartStyle => 'bars'
</%args>
<%init>
commit 90041d856eb8709ff92f4a0222fde961263b08ac
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Mar 28 18:33:47 2011 -0400
Restrict PrimaryGroupBy to only the explicit options that we offer
Not limiting the PrimaryGroupBy allows:
* SQL injection; the "Group By" clause is not escaped when inserted
into the generated SQL. Use of DBI's prepared statements disallows
the attacker from running more than one SQL statement, thus
precluding destructive queries such as "DELETE FROM" or "DROP TABLE",
but complex data extraction is still feasable.
* Information leakage; we blindly allow grouping even by valid columns
such as 'Subject', which allows an attacker to see the subjects of
tickets they do not otherwise have permission to view.
$UseSQLForACLChecks protects against this by pre-limiting the
resultset to tickets the current user has access to.
diff --git a/share/html/Search/Chart b/share/html/Search/Chart
index 858b22e..cf56c24 100644
--- a/share/html/Search/Chart
+++ b/share/html/Search/Chart
@@ -65,7 +65,8 @@ if ($ChartStyle eq 'pie') {
use RT::Report::Tickets;
my $tix = RT::Report::Tickets->new( $session{'CurrentUser'} );
-
+my %AllowedGroupings = reverse $tix->Groupings( Query => $Query );
+$PrimaryGroupBy = 'Queue' unless exists $AllowedGroupings{$PrimaryGroupBy};
my ($count_name, $value_name) = $tix->SetupGroupings(
Query => $Query, GroupBy => $PrimaryGroupBy,
);
diff --git a/share/html/Search/Elements/Chart b/share/html/Search/Elements/Chart
index 6b71f5c..28ffa72 100644
--- a/share/html/Search/Elements/Chart
+++ b/share/html/Search/Elements/Chart
@@ -55,6 +55,8 @@ use RT::Report::Tickets;
$PrimaryGroupBy ||= 'Queue'; # make sure PrimaryGroupBy is not undef
my $tix = RT::Report::Tickets->new( $session{'CurrentUser'} );
+my %AllowedGroupings = reverse $tix->Groupings( Query => $Query );
+$PrimaryGroupBy = 'Queue' unless exists $AllowedGroupings{$PrimaryGroupBy};
my ($count_name, $value_name) = $tix->SetupGroupings(
Query => $Query, GroupBy => $PrimaryGroupBy,
);
diff --git a/t/web/charting.t b/t/web/charting.t
new file mode 100644
index 0000000..5618762
--- /dev/null
+++ b/t/web/charting.t
@@ -0,0 +1,69 @@
+use strict;
+use warnings;
+
+use RT::Test no_plan => 1;
+
+for my $n (1..7) {
+ my $ticket = RT::Ticket->new( RT->SystemUser );
+ my $req = 'root' . ($n % 2) . '@localhost';
+ my ( $ret, $msg ) = $ticket->Create(
+ Subject => "base ticket $_",
+ Queue => "General",
+ Owner => "root",
+ Requestor => $req,
+ MIMEObj => MIME::Entity->build(
+ From => $req,
+ To => 'rt at localhost',
+ Subject => "base ticket $_",
+ Data => "Content $_",
+ ),
+ );
+ ok( $ret, "ticket $n created: $msg" );
+}
+
+my ($url, $m) = RT::Test->started_ok;
+ok( $m->login, "Logged in" );
+
+# Test that defaults work
+$m->get_ok( "/Search/Chart.html?Query=id>0" );
+$m->content_like(qr{<th[^>]*>Queue\s*</th>\s*<th[^>]*>Tickets\s*</th>}, "Grouped by queue");
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>}, "Found results in table");
+$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
+
+$m->get_ok( "/Search/Chart?Query=id>0" );
+is( $m->content_type, "image/png" );
+ok( length($m->content), "Has content" );
+
+
+# Group by Queue
+$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Queue" );
+$m->content_like(qr{<th[^>]*>Queue\s*</th>\s*<th[^>]*>Tickets\s*</th>}, "Grouped by queue");
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>}, "Found results in table");
+$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
+
+$m->get_ok( "/Search/Chart?Query=id>0&PrimaryGroupBy=Queue" );
+is( $m->content_type, "image/png" );
+ok( length($m->content), "Has content" );
+
+
+# Group by Requestor email
+$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Requestor.EmailAddress" );
+$m->content_like(qr{<th[^>]*>Requestor\.EmailAddress\s*</th>\s*<th[^>]*>Tickets\s*</th>},
+ "Grouped by requestor");
+$m->content_like(qr{root0\@localhost</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>3</a>}, "Found results in table");
+$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
+
+$m->get_ok( "/Search/Chart?Query=id>0&PrimaryGroupBy=Requestor.Email" );
+is( $m->content_type, "image/png" );
+ok( length($m->content), "Has content" );
+
+
+# Group by Requestor phone -- which is bogus, and falls back to queue
+$m->get_ok( "/Search/Chart.html?Query=id>0&PrimaryGroupBy=Requestor.Phone" );
+$m->content_like(qr{General</a>\s*</td>\s*<td[^>]*>\s*<a[^>]*>7</a>},
+ "Found queue results in table, as a default");
+$m->content_like(qr{<img src="/Search/Chart\?}, "Found image");
+
+$m->get_ok( "/Search/Chart?Query=id>0&PrimaryGroupBy=Requestor.Phone" );
+is( $m->content_type, "image/png" );
+ok( length($m->content), "Has content" );
commit b6d14dc41fccb5ed1874771e6ce5b7150bec2891
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 28 19:27:48 2011 -0400
Test that values for IS and IS NOT are forced to NULL
This is a SQL injection vulnerability since DBIx::SearchBuilder doesn't
quote values when the IS and IS NOT operators are used. Our TicketSQL
parsing doesn't enforce NULL either, so arbitrary strings make it into
the generated SQL. At best this results in broken queries; at worst,
attacks allowing complex data extraction. The use of DBI's prepared
statements disallows an attacker from running more than one SQL
statement, thus precluding destructive queries such as "DELETE FROM" or
"DROP TABLE".
Right now the added tests fail, but a fix is forthcoming.
diff --git a/t/api/tickets_overlay_sql.t b/t/api/tickets_overlay_sql.t
index 7bcedd1..615e4fe 100644
--- a/t/api/tickets_overlay_sql.t
+++ b/t/api/tickets_overlay_sql.t
@@ -1,6 +1,6 @@
use RT;
-use RT::Test tests => 7, config => 'Set( %FullTextSearch, Enable => 1 );';
+use RT::Test tests => 15, config => 'Set( %FullTextSearch, Enable => 1 );';
{
@@ -66,6 +66,20 @@ my $string = 'subject/content SQL test';
is ($count, scalar @created, "number of returned tickets same as entered");
}
+diag "Make sure we don't barf on invalid input for IS / IS NOT";
+{
+ my ($status, $msg) = $tix->FromSQL("Subject IS 'foobar'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 0, "found no tickets";
+ unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
+ like $tix->BuildSelectQuery, qr/Subject IS NULL/, "found right clause";
+
+ my ($status, $msg) = $tix->FromSQL("Subject IS NOT 'foobar'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 2, "found two tickets";
+ unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
+ like $tix->BuildSelectQuery, qr/Subject IS NOT NULL/, "found right clause";
+}
}
commit e3646bc826680bb1b81ca00e581e4028368bae15
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 28 19:57:51 2011 -0400
Override Limit further to force values to NULL for IS and IS NOT
Values for IS and IS NOT aren't quoted by DBIx::SearchBuilder's Limit.
This leads to a SQL injection vulnerability when user provided values
are passed into Limit, such as through TicketSQL. Forcing to NULL
closes this vulnerability and prevents accidental invalid SQL across RT.
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index 01f08ef..ed4971d 100644
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -216,10 +216,27 @@ This Limit sub calls SUPER::Limit, but defaults "CASESENSITIVE" to 1, thus
making sure that by default lots of things don't do extra work trying to
match lower(colname) agaist lc($val);
+We also force VALUE to C<NULL> when the OPERATOR is C<IS> or C<IS NOT>.
+This ensures that we don't pass invalid SQL to the database or allow SQL
+injection attacks when we pass through user specified values.
+
=cut
sub Limit {
- shift->SUPER::Limit(CASESENSITIVE => 1, @_);
+ my $self = shift;
+ my %args = (
+ CASESENSITIVE => 1,
+ @_
+ );
+
+ # We use the same regex here that DBIx::SearchBuilder uses to exclude
+ # values from quoting
+ if ( ($args{'OPERATOR'} || '') =~ /IS/i ) {
+ # Don't pass anything but NULL for IS and IS NOT
+ $args{'VALUE'} = 'NULL';
+ }
+
+ $self->SUPER::Limit(%args);
}
=head2 ItemsOrderBy
commit 63979ac6895aacacbaf88ca0d7f276802517b391
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 28 20:04:20 2011 -0400
Test that our UI canonicalizes values to NULL for IS/IS NOT
RT::Interface::Web::QueryBuilder::Tree already does this. When we
refactor that crazy in RT::SQL, however, we need to ensure we don't
break the canonicalization in the UI.
diff --git a/t/web/query_builder.t b/t/web/query_builder.t
index 38ea19d..4ae4392 100644
--- a/t/web/query_builder.t
+++ b/t/web/query_builder.t
@@ -5,7 +5,7 @@ use HTTP::Request::Common;
use HTTP::Cookies;
use LWP;
use Encode;
-use RT::Test tests => 46;
+use RT::Test tests => 52;
my $cookie_jar = HTTP::Cookies->new;
my ($baseurl, $agent) = RT::Test->started_ok;
@@ -254,3 +254,30 @@ diag "send query with not quoted negative number";
"query is the same"
);
}
+
+diag "click advanced, enter an invalid SQL IS restriction, apply and check that we corrected it";
+{
+ my $response = $agent->get($url."Search/Edit.html");
+ ok( $response->is_success, "Fetched /Search/Edit.html" );
+ ok($agent->form_name('BuildQueryAdvanced'), "found the form");
+ $agent->field("Query", "Requestor.EmailAddress IS 'FOOBAR'");
+ $agent->submit;
+ is( getQueryFromForm($agent),
+ "Requestor.EmailAddress IS NULL",
+ "foobar is replaced by NULL"
+ );
+}
+
+diag "click advanced, enter an invalid SQL IS NOT restriction, apply and check that we corrected it";
+{
+ my $response = $agent->get($url."Search/Edit.html");
+ ok( $response->is_success, "Fetched /Search/Edit.html" );
+ ok($agent->form_name('BuildQueryAdvanced'), "found the form");
+ $agent->field("Query", "Requestor.EmailAddress IS NOT 'FOOBAR'");
+ $agent->submit;
+ is( getQueryFromForm($agent),
+ "Requestor.EmailAddress IS NOT NULL",
+ "foobar is replaced by NULL"
+ );
+}
+
commit 75d1edd176cfa33db49421de2375c4abfac80559
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 29 10:52:56 2011 -0400
Disallow SQL injection in FIELD argument to OrderBy
A number of locations blindly pass OrderBy clauses from the user into
the FIELD of OrderByCols, which provides no escaping or sanity checking.
As a short-term fix, limit the FIELD to /^[a-zA-Z0-9_]*$/ at the
RT::SearchBuilder level.
There is one place which injects logic into the ORderByCols FIELD;
namely, the "PAW sort" feature, which sorts the current user's tickets,
followed by unowned tickets, followed by all other tickets. Allow this
codepath by adding a FUNCTION field to OrderByCols which steps around
the limit mentioned above. This particular use is safe because no
user-specified data is used in the function which is built.
The severity of this SQL injection is limited by the ability to create a
single dangerous statement by appending to a "SELECT ... FROM ... ORDER
BY main." statement. The lack of parens around the SELECT statement
precludes a UNION attack, and the use of DBI prepared statements
disallows the attacker from running more than one SQL statement, thus
precluding destructive queries such as "DELETE FROM" or "DROP TABLE".
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index 01f08ef..f3ca85b 100644
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -117,6 +117,17 @@ sub JoinTransactions {
return $alias;
}
+sub OrderByCols {
+ my $self = shift;
+ my @sort;
+ for my $s (@_) {
+ next if defined $s->{FIELD} and $s->{FIELD} =~ /\W/;
+ $s->{FIELD} = $s->{FUNCTION} if $s->{FUNCTION};
+ push @sort, $s;
+ }
+ return $self->SUPER::OrderByCols( @sort );
+}
+
=head2 LimitToEnabled
Only find items that haven't been disabled
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 99b43a8..92344f7 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -1932,9 +1932,20 @@ sub OrderByCols {
foreach my $uid ( $self->CurrentUser->Id, RT->Nobody->Id ) {
if ( RT->Config->Get('DatabaseType') eq 'Oracle' ) {
my $f = ($row->{'ALIAS'} || 'main') .'.Owner';
- push @res, { %$row, ALIAS => '', FIELD => "CASE WHEN $f=$uid THEN 1 ELSE 0 END", ORDER => $order } ;
+ push @res, {
+ %$row,
+ FIELD => undef,
+ ALIAS => '',
+ FUNCTION => "CASE WHEN $f=$uid THEN 1 ELSE 0 END",
+ ORDER => $order
+ };
} else {
- push @res, { %$row, FIELD => "Owner=$uid", ORDER => $order } ;
+ push @res, {
+ %$row,
+ FIELD => undef,
+ FUNCTION => "Owner=$uid",
+ ORDER => $order
+ };
}
}
commit b8609fbafd8797100e47b3bf0d6cd556eba3c161
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Tue Mar 29 15:42:43 2011 -0400
Use only the integer number of seconds in the Refresh header
Otherwise attackers could inject additional content into the header to
convince the browser to, say, redirect to a different site
diff --git a/share/html/Elements/Header b/share/html/Elements/Header
index 72043f7..c594f48 100755
--- a/share/html/Elements/Header
+++ b/share/html/Elements/Header
@@ -55,7 +55,7 @@
<title><%$Title%></title>
% if ($Refresh && $Refresh =~ /^(\d+)/ && $1 > 0) {
- <meta http-equiv="refresh" content="<% $Refresh %>" />
+ <meta http-equiv="refresh" content="<% $1 %>" />
% }
<link rel="shortcut icon" href="<%RT->Config->Get('WebImagesURL')%>favicon.png" type="image/png" />
commit 0d40bae17fd2d6d12b2d896f038527880c4a0963
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 29 15:42:14 2011 -0400
Use closures instead of eval to construct external CF limits
This prevents an arbitrary code execution vulnerability in
__BuildLimitCheck, wherein the $quoted_value scalar would fail to escape
\'s, allowing the user-provided VALUE to escape the quoted string.
Simply replace the eval with a closure, which does not have the same
vulnerabilities.
The other use of eval, in __BuildAggregatorsCheck, does not interpolate
user-provided data, but has also been removed in the interests of
clarity and bullet-proofing. In doing so, the interpretation of
mixed-entryaggregators has changed slightly; association is now
left-to-right, not the previous defualt of && binding tighter than ||.
diff --git a/lib/RT/CustomFieldValues/External.pm b/lib/RT/CustomFieldValues/External.pm
index ffff11d..9766bad 100644
--- a/lib/RT/CustomFieldValues/External.pm
+++ b/lib/RT/CustomFieldValues/External.pm
@@ -123,57 +123,49 @@ sub __BuildLimitCheck {
my ($self, %args) = (@_);
return undef unless $args{'FIELD'} =~ /^(?:Name|Description)$/;
- $args{'OPERATOR'} ||= '=';
- my $quoted_value = $args{'VALUE'};
- if ( $quoted_value ) {
- $quoted_value =~ s/'/\\'/g;
- $quoted_value = "'$quoted_value'";
- }
-
- my $code = <<END;
-my \$record = shift;
-my \$value = \$record->$args{'FIELD'};
-my \$condition = $quoted_value;
-END
-
- if ( $args{'OPERATOR'} =~ /^(?:=|!=|<>)$/ ) {
- $code .= 'return 0 unless defined $value;';
- my %h = ( '=' => ' eq ', '!=' => ' ne ', '<>' => ' ne ' );
- $code .= 'return 0 unless $value'. $h{ $args{'OPERATOR'} } .'$condition;';
- $code .= 'return 1;'
- }
- elsif ( $args{'OPERATOR'} =~ /^(?:LIKE|NOT LIKE)$/i ) {
- $code .= 'return 0 unless defined $value;';
- my %h = ( 'LIKE' => ' =~ ', 'NOT LIKE' => ' !~ ' );
- $code .= 'return 0 unless $value'. $h{ uc $args{'OPERATOR'} } .'/\Q$condition/i;';
- $code .= 'return 1;'
- }
- else {
- $code .= 'return 0;'
- }
- $code = "sub {$code}";
- my $cb = eval "$code";
- $RT::Logger->error( "Couldn't build callback '$code': $@" ) if $@;
- return $cb;
+ my $condition = $args{VALUE};
+ my $op = $args{'OPERATOR'} || '=';
+ my $field = $args{FIELD};
+
+ return sub {
+ my $record = shift;
+ my $value = $record->$field;
+ return 0 unless defined $value;
+ if ($op eq "=") {
+ return 0 unless $value eq $condition;
+ } elsif ($op eq "!=" or $op eq "<>") {
+ return 0 unless $value ne $condition;
+ } elsif (uc($op) eq "LIKE") {
+ return 0 unless $value =~ /\Q$condition\E/i;
+ } elsif (rc($op) eq "NOT LIKE") {
+ return 0 unless $value !~ /\Q$condition\E/i;
+ } else {
+ return 0;
+ }
+ return 1;
+ };
}
sub __BuildAggregatorsCheck {
my $self = shift;
+ my @cbs = grep {$_->{CALLBACK}} @{ $self->{'__external_cf_limits'} };
+ return undef unless @cbs;
- my %h = ( OR => ' || ', AND => ' && ' );
-
- my $code = '';
- for( my $i = 0; $i < @{ $self->{'__external_cf_limits'} }; $i++ ) {
- next unless $self->{'__external_cf_limits'}->[$i]->{'CALLBACK'};
- $code .= $h{ uc($self->{'__external_cf_limits'}->[$i]->{'ENTRYAGGREGATOR'} || 'OR') } if $code;
- $code .= '$sb->{\'__external_cf_limits\'}->['. $i .']->{\'CALLBACK\'}->($record)';
- }
- return unless $code;
+ my %h = (
+ OR => sub { defined $_[0] ? ($_[0] || $_[1]) : $_[1] },
+ AND => sub { defined $_[0] ? ($_[0] && $_[1]) : $_[1] },
+ );
- $code = "sub { my (\$sb,\$record) = (\@_); return $code }";
- my $cb = eval "$code";
- $RT::Logger->error( "Couldn't build callback '$code': $@" ) if $@;
- return $cb;
+ return sub {
+ my ($sb, $record) = @_;
+ my $ok;
+ for my $limit ( @cbs ) {
+ $ok = $h{$limit->{ENTRYAGGREGATOR} || 'OR'}->(
+ $ok, $limit->{CALLBACK}->($record),
+ );
+ }
+ return $ok;
+ };
}
sub _DoSearch {
commit 70fdf5ce99bd16d6838035afb514a538a5ca5d79
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 29 16:27:49 2011 -0400
Limit the CF options in SQL, rather than by regex
c8d2b10 switched to using jQuery's autocompleter, but failed to change
the helper to do the limiting in SQL with the new parameter name
("term", from "query"). 4f2cc61 fixed this in the User autocompleter,
but failed to fix both autocompleters.
diff --git a/share/html/Helpers/Autocomplete/CustomFieldValues b/share/html/Helpers/Autocomplete/CustomFieldValues
index d667f77..65e5170 100644
--- a/share/html/Helpers/Autocomplete/CustomFieldValues
+++ b/share/html/Helpers/Autocomplete/CustomFieldValues
@@ -49,8 +49,10 @@
<% JSON::to_json( \@suggestions ) |n %>
% $m->abort;
<%INIT>
-my ($CustomField, $Value);
-$Value = $ARGS{query};
+# Only autocomplete the last value
+my $term = (split /\n/, $ARGS{term} || '')[-1];
+
+my $CustomField;
for my $k ( keys %ARGS ) {
next unless $k =~ /^Object-.*?-\d*-CustomField-(\d+)-Values?$/;
$CustomField = $1;
@@ -60,32 +62,25 @@ for my $k ( keys %ARGS ) {
$m->abort unless $CustomField;
my $CustomFieldObj = RT::CustomField->new( $session{'CurrentUser'} );
$CustomFieldObj->Load( $CustomField );
-my $values = $CustomFieldObj->Values;
+my $values = $CustomFieldObj->Values;
$values->Limit(
FIELD => 'Name',
OPERATOR => 'LIKE',
- VALUE => $Value,
- SUBCLAUSE => 'autcomplete',
+ VALUE => $term,
+ SUBCLAUSE => 'autocomplete',
);
-
$values->Limit(
ENTRYAGGREGATOR => 'OR',
FIELD => 'Description',
OPERATOR => 'LIKE',
- VALUE => $Value,
- SUBCLAUSE => 'autcomplete',
+ VALUE => $term,
+ SUBCLAUSE => 'autocomplete',
);
my @suggestions;
-# in case it accepts multiple values
-my $last_input = (split /\n/, $ARGS{term} || '')[-1];
-
while( my $value = $values->Next ) {
- # we need to filter by ourselves to use jqueryui's autocomplete
- next unless $value->Name =~ /$last_input/i;
-
push @suggestions,
{
value => $value->Name,
commit 4be435eb9b1160f517e69d28a50859892024de55
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue Mar 29 16:58:12 2011 -0400
Test that we're not allowed to bypass NoAuth
diff --git a/t/web/noauth.t b/t/web/noauth.t
new file mode 100644
index 0000000..12e6b3e
--- /dev/null
+++ b/t/web/noauth.t
@@ -0,0 +1,7 @@
+use strict;
+
+use RT::Test tests => 4;
+my ($baseurl, $agent) = RT::Test->started_ok;
+
+$agent->get("$baseurl/NoAuth/../Elements/HeaderJavascript");
+is($agent->status, 400);
commit 0e5e4222cedd91ad7423c03baf36cf447f85d356
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue Mar 29 14:44:21 2011 -0400
Prevent users from requesting /NoAuth/../Elements/Header
CGI.pm prevented us from being vulnerable to this by removing the .. and
turning that into /Elements/Header which we shipped off to Login.
4.0 uses PSGI which does not normalize the .. so you can walk around
NoAuth regex.
diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm
index 73f0b9e..2c8617a 100644
--- a/lib/RT/Interface/Web/Handler.pm
+++ b/lib/RT/Interface/Web/Handler.pm
@@ -210,6 +210,7 @@ sub CleanupRequest {
use RT::Interface::Web::Handler;
use CGI::Emulate::PSGI;
use Plack::Request;
+use Plack::Response;
use Plack::Util;
use Encode qw(encode_utf8);
@@ -229,6 +230,14 @@ sub PSGIApp {
my $req = Plack::Request->new($env);
+ # CGI.pm normalizes .. out of paths so when you requested
+ # /NoAuth/../Ticket/Display.html we saw Ticket/Display.html
+ # PSGI doesn't normalize .. so we have to deal ourselves.
+ if ( $req->path_info =~ m{/\.} ) {
+ $RT::Logger->crit("Invalid request for ".$req->path_info." aborting");
+ my $res = Plack::Response->new(400);
+ return $self->_psgi_response_cb($res->finalize,sub { $self->CleanupRequest });
+ }
$env->{PATH_INFO} = $self->_mason_dir_index( $h->interp, $req->path_info);
my $ret;
commit fd3cbb5caa64b5f206811ff01bb5aacd736412c8
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue Mar 29 17:28:59 2011 -0400
We throw a warning from the handler, handle it
diff --git a/t/web/noauth.t b/t/web/noauth.t
index 12e6b3e..1e514c9 100644
--- a/t/web/noauth.t
+++ b/t/web/noauth.t
@@ -1,7 +1,11 @@
use strict;
+use warnings;
+
+use RT::Test tests => 6;
-use RT::Test tests => 4;
my ($baseurl, $agent) = RT::Test->started_ok;
$agent->get("$baseurl/NoAuth/../Elements/HeaderJavascript");
is($agent->status, 400);
+# do this last, since it screws with agent state
+$agent->warning_like(qr/Invalid request.*aborting/,);
commit d9a4f208930e7ca1f54d4f3dd7b578977715ae99
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Mar 30 11:50:28 2011 -0400
A failing test that searches by invalid watcher subfields in TicketSQL
There's also a basic test to ensure valid subfields are allowed.
diff --git a/t/api/tickets_overlay_sql.t b/t/api/tickets_overlay_sql.t
index 7bcedd1..501b789 100644
--- a/t/api/tickets_overlay_sql.t
+++ b/t/api/tickets_overlay_sql.t
@@ -1,6 +1,6 @@
use RT;
-use RT::Test tests => 7, config => 'Set( %FullTextSearch, Enable => 1 );';
+use RT::Test tests => 11, config => 'Set( %FullTextSearch, Enable => 1 );';
{
@@ -33,6 +33,7 @@ my $string = 'subject/content SQL test';
my $t = RT::Ticket->new(RT->SystemUser);
ok( $t->Create( Queue => 'General',
+ Requestor => 'jesse at example.com',
Subject => 'another ticket',
MIMEObj => $Message,
MemberOf => $created[0]
@@ -66,6 +67,16 @@ my $string = 'subject/content SQL test';
is ($count, scalar @created, "number of returned tickets same as entered");
}
+{
+ my ($status, $msg) = $tix->FromSQL("Requestor.Signature LIKE 'foo'");
+ ok (!$status, "invalid query - Signature not valid") or diag("error: $msg");
+
+ my ($status, $msg) = $tix->FromSQL("Requestor.EmailAddress LIKE 'jesse'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 1, "found one ticket";
+ like $tix->First->Subject, qr/another ticket/, "found the right ticket";
+}
+
}
commit 104271918a70389a2f1f824451083555666fb79f
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Mar 30 12:10:35 2011 -0400
Limit watcher subfields to a valid subset
Previously you could search on user fields which shouldn't be
accessible, such as Comments and Password. Unfortunately we can't build
the valid list of fields from RT::User's accessible hashes since search
historically lets you limit on fields which aren't publically
accessible.
This also closes a SQL injection vulnerability which allowed you access
to the WHERE clause, potentially returning data not normally available.
The vulnerability exploited that everything after the first dot (.) in a
watcher TicketSQL clause was passed in to Limit as FIELD.
Like other TicketSQL methods, we die to throw an error on invalid
subfields.
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 99b43a8..e52cf29 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -154,6 +154,13 @@ our %FIELD_METADATA = (
HasNoAttribute => [ 'HASATTRIBUTE', 0 ],
);
+our %SEARCHABLE_SUBFIELDS = (
+ User => [qw(
+ EmailAddress Name RealName Nickname Organization Address1 Address2
+ WorkPhone HomePhone MobilePhone PagerPhone id
+ )],
+);
+
# Mapping of Field Type to Function
our %dispatch = (
ENUM => \&_EnumLimit,
@@ -873,6 +880,13 @@ sub _WatcherLimit {
my $type = $meta->[1] || '';
my $class = $meta->[2] || 'Ticket';
+ # Bail if the subfield is not allowed
+ if ( $rest{SUBKEY}
+ and not grep { $_ eq $rest{SUBKEY} } @{$SEARCHABLE_SUBFIELDS{'User'}})
+ {
+ die "Invalid watcher subfield: '$rest{SUBKEY}'";
+ }
+
# Owner was ENUM field, so "Owner = 'xxx'" allowed user to
# search by id and Name at the same time, this is workaround
# to preserve backward compatibility
diff --git a/share/html/Search/Elements/SelectPersonType b/share/html/Search/Elements/SelectPersonType
index 2d20973..6454acf 100644
--- a/share/html/Search/Elements/SelectPersonType
+++ b/share/html/Search/Elements/SelectPersonType
@@ -72,7 +72,7 @@ else {
@types = qw(Requestor Cc AdminCc Watcher Owner QueueCc QueueAdminCc QueueWatcher);
}
-my @subtypes = qw(EmailAddress Name RealName Nickname Organization Address1 Address2 WorkPhone HomePhone MobilePhone PagerPhone id);
+my @subtypes = @{ $RT::Tickets::SEARCHABLE_SUBFIELDS{'User'} };
</%INIT>
<%ARGS>
commit 007046d1c5bc9392cccdfa1ebb8e968e1d674b80
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Mar 30 16:53:30 2011 -0400
Lock down possible OCFV columns to the two that we use
The only codepath which uses the CF.{foo}.Content form is
_CustomFieldLimit itself, which calls into itself for IP address ranges.
However, this provides a SQL injection, since it does not check the
column given, nor does SearchBuilder escape it. Limit it to the only
two valid column types.
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 99b43a8..7033bdf 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -1252,7 +1252,7 @@ Try and turn a CF descriptor into (cfid, cfname) object pair.
sub _CustomFieldDecipher {
my ($self, $string) = @_;
- my ($queue, $field, $column) = ($string =~ /^(?:(.+?)\.)?{(.+)}(?:\.(.+))?$/);
+ my ($queue, $field, $column) = ($string =~ /^(?:(.+?)\.)?{(.+)}(?:\.(Content|LargeContent))?$/);
$field ||= ($string =~ /^{(.*?)}$/)[0] || $string;
my $cf;
commit bd0ebe51688df364ac11b63728b771b67eb09f09
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Apr 1 11:46:26 2011 -0400
Prevent FIELD- and OPERATOR- based SQL injection at the RT::SB level
Provide a whitelist of possible operators to use, and restrict field to
being /^\w*$/. Any arguments not matching this will instead log a
critical message and insert a clause which should never match.
In the two places we attempt to be clever, and generate the SQL
"FUNCTION( alias.field ) = 42" by putting "FUNCTION( alias" as the
ALIAS, and "field )" as the FIELD -- which violates this constraint.
Provide a new FUNCTION parameter, which walks around the new
restriction, and does the split across the '.' for you. This should
only be used if you are certain that you trust the data that the
FUNCTION argument is built up from.
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index 01f08ef..fd01915 100644
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -219,7 +219,34 @@ match lower(colname) agaist lc($val);
=cut
sub Limit {
- shift->SUPER::Limit(CASESENSITIVE => 1, @_);
+ my $self = shift;
+ my %ARGS = (
+ CASESENSITIVE => 1,
+ OPERATOR => '=',
+ @_,
+ );
+
+ if ($ARGS{FUNCTION}) {
+ ($ARGS{ALIAS}, $ARGS{FIELD}) = split /\./, delete $ARGS{FUNCTION}, 2;
+ $self->SUPER::Limit(%ARGS);
+ } elsif ($ARGS{FIELD} =~ /\W/
+ or $ARGS{OPERATOR} !~ /^(=|<|>|!=|<>|<=|>=
+ |(NOT\s*)?LIKE
+ |(NOT\s*)?(STARTS|ENDS)WITH
+ |(NOT\s*)?MATCHES
+ |IS(\s*NOT)?
+ |IN
+ |\@\@)$/ix) {
+ $RT::Logger->crit("Possible SQL injection attack: $ARGS{FIELD} $ARGS{OPERATOR}");
+ $self->SUPER::Limit(
+ %ARGS,
+ FIELD => 'id',
+ OPERATOR => '<',
+ VALUE => '0',
+ );
+ } else {
+ $self->SUPER::Limit(%ARGS);
+ }
}
=head2 ItemsOrderBy
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 99b43a8..c074029 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -790,13 +790,13 @@ sub _TransContentLimit {
}
my $field = $config->{'Field'} || 'Content';
+ $field = 'Content' if $field =~ /\W/;
if ( $db_type eq 'Oracle' ) {
my $dbh = $RT::Handle->dbh;
+ my $alias = $self->{_sql_trattachalias};
$self->_SQLLimit(
%rest,
- # XXX: Nasty hack
- ALIAS => 'CONTAINS( '. $self->{_sql_trattachalias},
- FIELD => $field . ', '. $dbh->quote($value) .')',
+ FUNCTION => "CONTAINS( $alias.$field, ".$dbh->quote($value) .")",
OPERATOR => '>',
VALUE => 0,
QUOTEVALUE => 0,
@@ -1436,9 +1436,7 @@ sub _CustomFieldLimit {
$args{'OPERATOR'} = 'NOT MATCHES';
}
elsif ( $op =~ /^[<>]=?$/ ) {
- # XXX: VERY DIRTY HACK
- $args{'ALIAS'} = 'TO_CHAR( '. $args{'ALIAS'};
- $args{'FIELD'} .= ')'; #, 1, '. (length($args{'VALUE'}) + 1) .')';
+ $args{'FUNCTION'} = "TO_CHAR( $args{'ALIAS'}.LargeContent )";
}
return %args;
};
commit f177355bc3858256c7ad0a47ccec6e14cd861c3d
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Fri Apr 1 15:29:34 2011 -0400
Stop direct access to richtext editor files
We have a dhandler that rewrites access, but nothing that stopped you
from directly invoking one of the files under the RichText editor.
This means you could ask mason to run a .js file or a .png.
You shouldn't be able to do anything interesting with this, but it seems
wrong so we're stopping it by providing an autohandler.
However, the .js or .png is still compiled before mason consults the
autohandler. Please don't add BEGIN blocks to your .js or .png files
We should really have static content outside the mason component root
diff --git a/share/html/NoAuth/RichText/autohandler b/share/html/NoAuth/RichText/autohandler
new file mode 100644
index 0000000..8164084
--- /dev/null
+++ b/share/html/NoAuth/RichText/autohandler
@@ -0,0 +1,56 @@
+%# BEGIN BPS TAGGED BLOCK {{{
+%#
+%# COPYRIGHT:
+%#
+%# This software is Copyright (c) 1996-2011 Best Practical Solutions, LLC
+%# <sales at bestpractical.com>
+%#
+%# (Except where explicitly superseded by other copyright notices)
+%#
+%#
+%# LICENSE:
+%#
+%# This work is made available to you under the terms of Version 2 of
+%# the GNU General Public License. A copy of that license should have
+%# been provided with this software, but in any event can be snarfed
+%# from www.gnu.org.
+%#
+%# This work is distributed in the hope that it will be useful, but
+%# WITHOUT ANY WARRANTY; without even the implied warranty of
+%# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+%# General Public License for more details.
+%#
+%# You should have received a copy of the GNU General Public License
+%# along with this program; if not, write to the Free Software
+%# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+%# 02110-1301 or visit their web page on the internet at
+%# http://www.gnu.org/licenses/old-licenses/gpl-2.0.html.
+%#
+%#
+%# CONTRIBUTION SUBMISSION POLICY:
+%#
+%# (The following paragraph is not intended to limit the rights granted
+%# to you to modify and distribute this software under the terms of
+%# the GNU General Public License and is only of importance to you if
+%# you choose to contribute your changes and enhancements to the
+%# community by submitting them to Best Practical Solutions, LLC.)
+%#
+%# By intentionally submitting any modifications, corrections or
+%# derivatives to this work, or any other work intended for use with
+%# Request Tracker, to Best Practical Solutions, LLC, you confirm that
+%# you are the copyright holder for those contributions and you grant
+%# Best Practical Solutions, LLC a nonexclusive, worldwide, irrevocable,
+%# royalty-free, perpetual, license to use, copy, create derivative
+%# works based on those contributions, and sublicense and distribute
+%# those contributions and any derivatives thereof.
+%#
+%# END BPS TAGGED BLOCK }}}
+<%init>
+my $file = $m->base_comp->source_file;
+if ($file =~ m{RichText/+ckeditor}) {
+ $RT::Logger->crit("Invalid request directly to the rich text editor: $file");
+ $m->abort(403);
+} else {
+ $m->call_next();
+}
+</%init>
diff --git a/t/web/richtext-autohandler.t b/t/web/richtext-autohandler.t
new file mode 100644
index 0000000..734426f
--- /dev/null
+++ b/t/web/richtext-autohandler.t
@@ -0,0 +1,13 @@
+use strict;
+
+use RT::Test tests => 9;
+my ($baseurl, $agent) = RT::Test->started_ok;
+
+$agent->get("$baseurl/NoAuth/RichText/ckeditor/config.js");
+is($agent->status, 403);
+$agent->content_lacks("config.disableNativeSpellChecker");
+
+$agent->get_ok("/NoAuth/RichText/config.js");
+$agent->content_contains("config.disableNativeSpellChecker");
+
+$agent->warning_like(qr/Invalid request directly to the rich text editor/,);
commit a410481b08a7897f7d3c567ffb45cf985c2ec8ed
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Apr 4 12:52:56 2011 -0400
Expand noauth tests and rename it to path-traversal.t
diff --git a/t/web/noauth.t b/t/web/path-traversal.t
similarity index 56%
rename from t/web/noauth.t
rename to t/web/path-traversal.t
index 1e514c9..2c53a1b 100644
--- a/t/web/noauth.t
+++ b/t/web/path-traversal.t
@@ -1,11 +1,15 @@
use strict;
use warnings;
-use RT::Test tests => 6;
+use RT::Test tests => 9;
my ($baseurl, $agent) = RT::Test->started_ok;
$agent->get("$baseurl/NoAuth/../Elements/HeaderJavascript");
is($agent->status, 400);
-# do this last, since it screws with agent state
$agent->warning_like(qr/Invalid request.*aborting/,);
+
+$agent->get("$baseurl/NoAuth/../../../etc/RT_Config.pm");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
commit 54d03a7f6e0622c50b53117eb005638a874d461c
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Mon Apr 4 15:02:17 2011 -0400
path-traversal test for a SendStaticFile dhandler
diff --git a/t/web/path-traversal.t b/t/web/path-traversal.t
index 2c53a1b..38a37ca 100644
--- a/t/web/path-traversal.t
+++ b/t/web/path-traversal.t
@@ -1,7 +1,7 @@
use strict;
use warnings;
-use RT::Test tests => 9;
+use RT::Test tests => 12;
my ($baseurl, $agent) = RT::Test->started_ok;
@@ -13,3 +13,7 @@ $agent->get("$baseurl/NoAuth/../../../etc/RT_Config.pm");
is($agent->status, 400);
$agent->warning_like(qr/Invalid request.*aborting/,);
+$agent->get("$baseurl/NoAuth/css/web2/images/../../../../../../etc/RT_Config.pm");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
commit 34d86395c8d1351484390815e55f28b8d6974aa7
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Tue Apr 5 12:38:29 2011 -0400
More tests for unsafe and safe URLs
Conflicts:
t/web/path-traversal.t
- Test numbering shenanigans
diff --git a/t/web/path-traversal.t b/t/web/path-traversal.t
index 38a37ca..14202de 100644
--- a/t/web/path-traversal.t
+++ b/t/web/path-traversal.t
@@ -1,7 +1,7 @@
use strict;
use warnings;
-use RT::Test tests => 12;
+use RT::Test tests => 22;
my ($baseurl, $agent) = RT::Test->started_ok;
@@ -9,6 +9,14 @@ $agent->get("$baseurl/NoAuth/../Elements/HeaderJavascript");
is($agent->status, 400);
$agent->warning_like(qr/Invalid request.*aborting/,);
+$agent->get("$baseurl/NoAuth/../%45lements/HeaderJavascript");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
+$agent->get("$baseurl/NoAuth/%2E%2E/Elements/HeaderJavascript");
+is($agent->status, 400);
+$agent->warning_like(qr/Invalid request.*aborting/,);
+
$agent->get("$baseurl/NoAuth/../../../etc/RT_Config.pm");
is($agent->status, 400);
$agent->warning_like(qr/Invalid request.*aborting/,);
@@ -17,3 +25,16 @@ $agent->get("$baseurl/NoAuth/css/web2/images/../../../../../../etc/RT_Config.pm"
is($agent->status, 400);
$agent->warning_like(qr/Invalid request.*aborting/,);
+# do not reject these URLs, even though they contain /. outside the path
+$agent->get("$baseurl/index.html?ignored=%2F%2E");
+is($agent->status, 200);
+
+$agent->get("$baseurl/index.html?ignored=/.");
+is($agent->status, 200);
+
+$agent->get("$baseurl/index.html#%2F%2E");
+is($agent->status, 200);
+
+$agent->get("$baseurl/index.html#/.");
+is($agent->status, 200);
+
commit 2bf2ff20926304713031224ddc47ee501cdbada6
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Wed Apr 6 11:27:36 2011 -0400
Avoid testing files out of RichText
diff --git a/t/web/compilation_errors.t b/t/web/compilation_errors.t
index 81be7e7..1f82ab9 100644
--- a/t/web/compilation_errors.t
+++ b/t/web/compilation_errors.t
@@ -5,7 +5,7 @@ use Test::More;
use File::Find;
BEGIN {
sub wanted {
- -f && /\.html$/ && $_ !~ /Logout.html$/;
+ -f && /\.html$/ && $_ !~ /Logout.html$/ && $File::Find::dir !~ /RichText/;
}
my $tests = 8;
find( sub { wanted() and $tests += 4 }, 'share/html/' );
commit 4c1be2c8ffee6fe69357efc16a4ab055955abb4c
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Wed Apr 6 11:31:05 2011 -0400
Silence warnings out of t/api/tickets_overlay_sql.t
diff --git a/t/api/tickets_overlay_sql.t b/t/api/tickets_overlay_sql.t
index 501b789..3927ec3 100644
--- a/t/api/tickets_overlay_sql.t
+++ b/t/api/tickets_overlay_sql.t
@@ -1,6 +1,6 @@
use RT;
-use RT::Test tests => 11, config => 'Set( %FullTextSearch, Enable => 1 );';
+use RT::Test tests => 12, config => 'Set( %FullTextSearch, Enable => 1 );';
{
@@ -68,10 +68,14 @@ my $string = 'subject/content SQL test';
}
{
+ my $warnings;
+ local $SIG{__WARN__} = sub { $warnings .= "@_" };
+
my ($status, $msg) = $tix->FromSQL("Requestor.Signature LIKE 'foo'");
ok (!$status, "invalid query - Signature not valid") or diag("error: $msg");
+ like($warnings, qr/Invalid watcher subfield: 'Signature'/);
- my ($status, $msg) = $tix->FromSQL("Requestor.EmailAddress LIKE 'jesse'");
+ ($status, $msg) = $tix->FromSQL("Requestor.EmailAddress LIKE 'jesse'");
ok ($status, "valid query") or diag("error: $msg");
is $tix->Count, 1, "found one ticket";
like $tix->First->Subject, qr/another ticket/, "found the right ticket";
commit 2dcacd03350c5664855cda54c46bc8f8e8eaa296
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Wed Apr 6 11:33:28 2011 -0400
Avoid redefining a couple variables
diff --git a/t/api/tickets_overlay_sql.t b/t/api/tickets_overlay_sql.t
index 615e4fe..860d007 100644
--- a/t/api/tickets_overlay_sql.t
+++ b/t/api/tickets_overlay_sql.t
@@ -74,7 +74,7 @@ diag "Make sure we don't barf on invalid input for IS / IS NOT";
unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
like $tix->BuildSelectQuery, qr/Subject IS NULL/, "found right clause";
- my ($status, $msg) = $tix->FromSQL("Subject IS NOT 'foobar'");
+ ($status, $msg) = $tix->FromSQL("Subject IS NOT 'foobar'");
ok ($status, "valid query") or diag("error: $msg");
is $tix->Count, 2, "found two tickets";
unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
commit 86812b5c0b27984cc0ed4bd086fe8a17f1b7644e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Apr 8 11:14:27 2011 -0400
Allow the logout page to specify a URL to redirect to
We pass in $m->notes because %ARGS is passed in wholesale in too many
places, and there is no way to validate the url obtained via %ARGS, due
to the callback.
diff --git a/share/html/Elements/Header b/share/html/Elements/Header
index c594f48..1eb7f09 100755
--- a/share/html/Elements/Header
+++ b/share/html/Elements/Header
@@ -55,7 +55,8 @@
<title><%$Title%></title>
% if ($Refresh && $Refresh =~ /^(\d+)/ && $1 > 0) {
- <meta http-equiv="refresh" content="<% $1 %>" />
+% my $URL = $m->notes->{LogoutURL}; $URL = $URL ? ";URL=$URL" : "";
+ <meta http-equiv="refresh" content="<% "$1$URL" %>" />
% }
<link rel="shortcut icon" href="<%RT->Config->Get('WebImagesURL')%>favicon.png" type="image/png" />
diff --git a/share/html/NoAuth/Logout.html b/share/html/NoAuth/Logout.html
index 103ae4f..fa21100 100755
--- a/share/html/NoAuth/Logout.html
+++ b/share/html/NoAuth/Logout.html
@@ -45,7 +45,7 @@
%# those contributions and any derivatives thereof.
%#
%# END BPS TAGGED BLOCK }}}
-<& /Elements/Header, Title => loc('Logout'), Refresh => RT->Config->Get('LogoutRefresh').";URL=$URL" &>
+<& /Elements/Header, Title => loc('Logout'), Refresh => RT->Config->Get('LogoutRefresh') &>
</div>
<div id="body" class="login-body">
@@ -81,4 +81,5 @@ if (keys %session) {
}
$m->callback( %ARGS, CallbackName => 'AfterSessionDelete' );
+$m->notes->{LogoutURL} = $URL;
</%INIT>
commit dacf74182d03d26d439351ce1a2fcfdfe2d714fc
Merge: 8a16709 007046d
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:15:37 2011 -0400
Merge branch 'security/customfield-column-injection' into 4.0.0-releng
Fixes CVE-2011-1686 CVE-2011-168
commit daa0516c1b8950e20a697c927fe975b1763bd4d3
Merge: dacf741 70fdf5c
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:19:01 2011 -0400
Merge branch 'security/external-cf-eval' into 4.0.0-releng
Fixes CVE-2011-1685
commit ce5c889e50780107e8815bff217f4146b01abcad
Merge: daa0516 2dcacd0
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:19:19 2011 -0400
Merge branch 'security/force-null' into 4.0.0-releng
CVE-2011-1686 CVE-2011-1687
commit 55270e6a59860edf0abfd9ad1cb8f0ea8cbbcfbe
Merge: ce5c889 bd0ebe5
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:19:56 2011 -0400
Merge branch 'security/limit-security-restriction' into 4.0.0-releng
Fixes CVE-2011-1686
Conflicts:
lib/RT/SearchBuilder.pm
diff --cc lib/RT/SearchBuilder.pm
index ed4971d,fd01915..afe6ede
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@@ -224,19 -220,33 +224,40 @@@ injection attacks when we pass through
sub Limit {
my $self = shift;
- my %args = (
+ my %ARGS = (
CASESENSITIVE => 1,
- @_
+ OPERATOR => '=',
+ @_,
);
+ # We use the same regex here that DBIx::SearchBuilder uses to exclude
+ # values from quoting
- if ( ($args{'OPERATOR'} || '') =~ /IS/i ) {
++ if ( $ARGS{'OPERATOR'} =~ /IS/i ) {
+ # Don't pass anything but NULL for IS and IS NOT
- $args{'VALUE'} = 'NULL';
++ $ARGS{'VALUE'} = 'NULL';
+ }
+
- $self->SUPER::Limit(%args);
+ if ($ARGS{FUNCTION}) {
+ ($ARGS{ALIAS}, $ARGS{FIELD}) = split /\./, delete $ARGS{FUNCTION}, 2;
+ $self->SUPER::Limit(%ARGS);
+ } elsif ($ARGS{FIELD} =~ /\W/
+ or $ARGS{OPERATOR} !~ /^(=|<|>|!=|<>|<=|>=
+ |(NOT\s*)?LIKE
+ |(NOT\s*)?(STARTS|ENDS)WITH
+ |(NOT\s*)?MATCHES
+ |IS(\s*NOT)?
+ |IN
+ |\@\@)$/ix) {
+ $RT::Logger->crit("Possible SQL injection attack: $ARGS{FIELD} $ARGS{OPERATOR}");
+ $self->SUPER::Limit(
+ %ARGS,
+ FIELD => 'id',
+ OPERATOR => '<',
+ VALUE => '0',
+ );
+ } else {
+ $self->SUPER::Limit(%ARGS);
+ }
}
=head2 ItemsOrderBy
commit 895a4ccfe07bf20205985d194447cb892987919c
Merge: 55270e6 75d1edd
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:20:12 2011 -0400
Merge branch 'security/orderby-injection' into 4.0.0-releng
Fixes CVE-2011-1686 CVE-2011-1687
commit f076f1babcd6fe7bb5e48fd04d05b428e24f1fc4
Merge: 895a4cc 34d8639
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:20:24 2011 -0400
Merge branch 'security/path-traversal' into 4.0.0-releng
Fixes CVE-2011-1688
commit df67f7ae35f342faf55aecac7754cf942b32e83c
Merge: f076f1b cc01217
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:20:41 2011 -0400
Merge branch 'security/private-components' into 4.0.0-releng
git merge security/private-components # CVE-2011-1689
commit 88689bec08c3e93aa03aec4d9c3caf6246819a68
Merge: df67f7a 90041d8
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:20:53 2011 -0400
Merge branch 'security/restrict-charting' into 4.0.0-releng
git merge security/restrict-charting # CVE-2011-1686 CVE-2011-1687
commit 9b72895e7da56c497622e1d4b3d112bb95c1612c
Merge: 88689be 2bf2ff2
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:21:09 2011 -0400
Merge branch 'security/richtext-autohandler' into 4.0.0-releng
commit c32b1967f8498a6abc5d683e7837c7b5ef7dbde2
Merge: 9b72895 4c1be2c
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:21:24 2011 -0400
Merge branch 'security/ticketsql-private-fields' into 4.0.0-releng
Fixes CVE-2011-1686 CVE-2011-1687
Conflicts:
t/api/tickets_overlay_sql.t
diff --cc t/api/tickets_overlay_sql.t
index 860d007,3927ec3..0397d4b
--- a/t/api/tickets_overlay_sql.t
+++ b/t/api/tickets_overlay_sql.t
@@@ -1,6 -1,6 +1,6 @@@
use RT;
- use RT::Test tests => 15, config => 'Set( %FullTextSearch, Enable => 1 );';
-use RT::Test tests => 12, config => 'Set( %FullTextSearch, Enable => 1 );';
++use RT::Test tests => 20, config => 'Set( %FullTextSearch, Enable => 1 );';
{
@@@ -66,21 -67,21 +67,34 @@@ my $string = 'subject/content SQL test'
is ($count, scalar @created, "number of returned tickets same as entered");
}
+diag "Make sure we don't barf on invalid input for IS / IS NOT";
+{
+ my ($status, $msg) = $tix->FromSQL("Subject IS 'foobar'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 0, "found no tickets";
+ unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
+ like $tix->BuildSelectQuery, qr/Subject IS NULL/, "found right clause";
+
+ ($status, $msg) = $tix->FromSQL("Subject IS NOT 'foobar'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 2, "found two tickets";
+ unlike $tix->BuildSelectQuery, qr/foobar/, "didn't find foobar in the select";
+ like $tix->BuildSelectQuery, qr/Subject IS NOT NULL/, "found right clause";
+}
+
+ {
+ my $warnings;
+ local $SIG{__WARN__} = sub { $warnings .= "@_" };
+
+ my ($status, $msg) = $tix->FromSQL("Requestor.Signature LIKE 'foo'");
+ ok (!$status, "invalid query - Signature not valid") or diag("error: $msg");
+ like($warnings, qr/Invalid watcher subfield: 'Signature'/);
+
+ ($status, $msg) = $tix->FromSQL("Requestor.EmailAddress LIKE 'jesse'");
+ ok ($status, "valid query") or diag("error: $msg");
+ is $tix->Count, 1, "found one ticket";
+ like $tix->First->Subject, qr/another ticket/, "found the right ticket";
+ }
-
-
}
commit e77f11b09699ecc530f747d2fdc027ad331206dc
Merge: c32b196 86812b5
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Wed Apr 13 21:21:39 2011 -0400
Merge branch 'security/validate-refresh' into 4.0.0-releng
Fixes CVE-2011-1689
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list